Untangle external reader code #776

translatenix · 2024-11-04T08:12:11Z

See commit messages for details. Second commit is a pure rename refactoring.

HT154 · 2024-11-04T08:23:39Z

pkl-server/src/main/kotlin/org/pkl/server/Server.kt

+import org.pkl.core.evaluatorSettings.PklEvaluatorSettings
+import org.pkl.core.externalreader.ModuleReaderSpec
+import org.pkl.core.externalreader.ReaderProcess
+import org.pkl.core.externalreader.ResourceReaderSpec


Given that the resolver types are shared between the client readers and external readers, is org.pkl.core.externalreader really the right place for these types?

org.pkl.server depending on externalreader seems acceptable to me. These types couldn't stay in module/resource because that creates package cycles and is a layering violation: module/resource already export packagereader.ReaderProcess, hence packagereader shouldn't also depend on them.

pkl-core has serious modularity issues. I just tried to do some damage control in the few hours I had. I think I made a step in the right direction; perhaps you guys will find further improvements.

bioball · 2024-11-04T16:31:36Z

Thanks for the PR, and appreciate the help straightening this out!

FYI--this is a little late to get into the 0.27 release. However, we can get this into the main branch once this release is out.

We can also take some time to better look at the API of pkl-core and have a clear definition of what's public and what's not (and perhaps use JPMS).

translatenix · 2024-11-04T18:40:26Z

FYI--this is a little late to get into the 0.27 release.

Just keep in mind that this PR makes breaking changes to APIs introduced in 0.27.

We can also take some time to better look at the API of pkl-core and have a clear definition of what's public and what's not (and perhaps use JPMS).

I'd appreciate this. It will likely take quite a while until pkl-core is ready for JPMS. But we can take one step at a time: define which packages are public, enforce at build time that they don't export private packages, etc.

translatenix · 2024-11-04T18:51:25Z

For the next release, could we create a release branch at least one week before the release, and merge only bugfixes from then on?

bioball · 2024-11-04T20:27:55Z

Yeah, we should definitely add some more process here, including cut-off for features. We'll figure out what that should look like after we get 0.27 out and into the world.

Apologies for the lack of structure right now.

bioball

Most of these changes look good! Although, I think let's undo some of the name changes here.

bioball · 2024-11-11T20:42:20Z

pkl-core/src/main/java/org/pkl/core/externalreader/ReaderProcess.java

 import org.pkl.core.util.Nullable;

 /** An external process that reads Pkl modules and resources. */
-public interface ExternalReaderProcess extends AutoCloseable {
+public interface ReaderProcess extends AutoCloseable {


This is a public API as of 0.27, so, let's just keep the name as-is.

I assume you mean just this interface? (There are quite a few breaking changes in this PR.)

Yup, the name of this interface!

bioball · 2024-11-11T20:44:39Z

pkl-core/src/main/java/org/pkl/core/evaluatorSettings/PklEvaluatorSettings.java

-      return new Proxy(addressUri, noProxy);
-    }
-
-    @SuppressWarnings("unchecked")


Either undo this change, or re-add and deprecate.

But, I think it's fine to just keep this method around; we don't gain much by inlining this.

The motivation is to remove a method from the public API that doesn't belong there. I can add it back as deprecated.

Sorry, I don't fully understand; what makes Proxy.create() not belong here?

Sorry, my impression was that Proxy.create() is an internal method used by Proxy.parse() that accidentally became a public API.

I feel that regular Java code should just use new Proxy(new URI(...)) or new Proxy(URI.create(...)). These will also throw more appropriate exceptions than PklException, which is mainly thrown by the evaluator and doesn't seem ideal for regular Java APIs that don't evaluate Pkl code.

One more thing I noticed is that similar classes such as Release and PklInfo use java.lang.String for pkl.base.Uri. (They also don't have their own package.) A potential problem with using java.net.URI for pkl.base.Uri is that valid Pkl objects can't be represented in Java.

bioball · 2024-11-11T21:01:47Z

pkl-core/src/main/java/org/pkl/core/externalreader/ModuleResolver.java

-    String scheme();
+public interface ModuleResolver {
+  static ModuleResolver of(MessageTransport transport, long evaluatorId) {
+    return new ModuleResolverImpl(transport, evaluatorId);


There aren't any layering violations here, are there?

Since we've now shipped this in 0.27, I think let's keep this as-is, and have org.pkl.core.externalreader.ModuleReaderSpec implement interface Spec here.

ModuleReaderSpec is a simple record, and callers of ModuleKeys.externalResolver may need to instantiate it.
I think the ModuleResolver.Spec interface has no value (at least not anymore) and just obfuscates the public API.

This PR already makes breaking API changes. If it's important to make this particular change less breaking, record ModuleReaderSpec could be renamed to ModuleResolver.Spec. However, I doubt this trade of breaking changes has much benefit.

I prefer the name ModuleReaderSpec over ModuleResolver.Spec because it makes more sense in the ReaderProcess interface:

interface ReaderProcess { // used to return ModuleResolver.Spec, which I found confusing ModuleReaderSpec getModuleReaderSpec(String scheme) throws IOException; // used to return ResourceResolver.Spec, which I found confusing ResourceReaderSpec getResourceReaderSpec(String scheme) throws IOException; }

bioball · 2024-11-11T21:04:48Z

pkl-core/src/main/java/org/pkl/core/externalreader/ResourceResolver.java

-    String scheme();
+public interface ResourceResolver {
+  static ResourceResolver of(MessageTransport transport, long evaluatorId) {
+    return new ResourceResolverImpl(transport, evaluatorId);


Same comment; let's keep this class as-is, and have ResourceReaderSpec implement this interface

- move the following classes into package externalreader: - ExternalModuleResolver - ExternalResourceResolver - MessageTransportModuleResolver (renamed to ExternalModuleResolverImpl, made package-private) - MessageTransportResourceResolver (renamed to ExternalResourceResolverImpl, made package-private) - replace interface ExternalModuleResolver.Spec with record ExternalModuleReaderSpec - replace interface ExternalResourceResolver.Spec with record ExternalResourceReaderSpec - translate between messaging.ResourceReaderSpec and ExternalResourceReaderSpec (eliminates dependency from messaging on higher layer) - translate between messaging.ResourceResolverSpec and ExternalResourceResolverSpec (eliminates dependency from messaging on higher layer) - add ServerMessages.ExternalReader and translate between this message component and the PklEvaluatorSettings.ExternalReader API - add ServerMessages.Proxy and translate between this message component and the PklEvaluatorSettings.Proxy API - change type of CreateEvaluatorRequest.allowedModules/allowedResources from List<Pattern>? to List<String>? - removes a lot of code - should not need to create a Pattern object to send a message - remove public method evaluatorSettings.PklEvaluatorSettings.Proxy.create() - only seems useful internally, inlined

- class names were too long/repetitive - package name provides sufficient context

HT154 reviewed Nov 4, 2024

View reviewed changes

bioball requested changes Nov 11, 2024

View reviewed changes

translatenix force-pushed the untangle branch from 7e4461e to 32b0fa8 Compare November 12, 2024 00:47

translatenix added 2 commits November 14, 2024 16:27

Strip "External" from class names in package externalreader

1389907

- class names were too long/repetitive - package name provides sufficient context

translatenix force-pushed the untangle branch from 32b0fa8 to 1389907 Compare November 15, 2024 00:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Untangle external reader code #776

Untangle external reader code #776

translatenix commented Nov 4, 2024

HT154 Nov 4, 2024

translatenix Nov 4, 2024 •

edited

Loading

bioball commented Nov 4, 2024

translatenix commented Nov 4, 2024 •

edited

Loading

translatenix commented Nov 4, 2024

bioball commented Nov 4, 2024

bioball left a comment

bioball Nov 11, 2024

translatenix Nov 11, 2024 •

edited

Loading

bioball Nov 13, 2024

bioball Nov 11, 2024

translatenix Nov 11, 2024

bioball Nov 13, 2024

translatenix Nov 13, 2024 •

edited

Loading

bioball Nov 11, 2024

translatenix Nov 11, 2024 •

edited

Loading

bioball Nov 11, 2024

Untangle external reader code #776

Are you sure you want to change the base?

Untangle external reader code #776

Conversation

translatenix commented Nov 4, 2024

Choose a reason for hiding this comment

translatenix Nov 4, 2024 • edited Loading

Choose a reason for hiding this comment

bioball commented Nov 4, 2024

translatenix commented Nov 4, 2024 • edited Loading

translatenix commented Nov 4, 2024

bioball commented Nov 4, 2024

bioball left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

translatenix Nov 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

translatenix Nov 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

translatenix Nov 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

translatenix Nov 4, 2024 •

edited

Loading

translatenix commented Nov 4, 2024 •

edited

Loading

translatenix Nov 11, 2024 •

edited

Loading

translatenix Nov 13, 2024 •

edited

Loading

translatenix Nov 11, 2024 •

edited

Loading