Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Untangle external reader code #776

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Untangle external reader code #776

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
54984d5
Untangle external reader code
translatenix Nov 4, 2024
1389907
Strip "External" from class names in package externalreader
translatenix Nov 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions pkl-commons-cli/src/main/kotlin/org/pkl/commons/cli/CliCommand.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ import java.util.regex.Pattern
import kotlin.io.path.isRegularFile
import org.pkl.core.*
import org.pkl.core.evaluatorSettings.PklEvaluatorSettings
import org.pkl.core.externalreader.ExternalReaderProcess
import org.pkl.core.externalreader.ReaderProcess
import org.pkl.core.http.HttpClient
import org.pkl.core.module.ModuleKeyFactories
import org.pkl.core.module.ModuleKeyFactory
Expand Down Expand Up @@ -191,7 +191,7 @@ abstract class CliCommand(protected val cliOptions: CliBaseOptions) {
// with the same spec. This avoids spawning multiple subprocesses if the same reader implements
// both reader types and/or multiple schemes.
(externalModuleReaders + externalResourceReaders).values.toSet().associateWith {
ExternalReaderProcess.of(it)
ReaderProcess.of(it)
}
}

Expand Down
10 changes: 4 additions & 6 deletions pkl-core/src/main/java/org/pkl/core/EvaluatorBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@
import java.util.regex.Pattern;
import org.pkl.core.SecurityManagers.StandardBuilder;
import org.pkl.core.evaluatorSettings.PklEvaluatorSettings.ExternalReader;
import org.pkl.core.externalreader.ExternalReaderProcess;
import org.pkl.core.externalreader.ReaderProcess;
import org.pkl.core.http.HttpClient;
import org.pkl.core.module.ModuleKeyFactories;
import org.pkl.core.module.ModuleKeyFactory;
Expand Down Expand Up @@ -498,21 +498,19 @@ public EvaluatorBuilder applyFromProject(Project project) {
}

// this isn't ideal as project and non-project ExternalReaderProcess instances can be dupes
var procs = new HashMap<ExternalReader, ExternalReaderProcess>();
var procs = new HashMap<ExternalReader, ReaderProcess>();
if (settings.externalModuleReaders() != null) {
for (var entry : settings.externalModuleReaders().entrySet()) {
addModuleKeyFactory(
ModuleKeyFactories.externalProcess(
entry.getKey(),
procs.computeIfAbsent(entry.getValue(), ExternalReaderProcess::of)));
entry.getKey(), procs.computeIfAbsent(entry.getValue(), ReaderProcess::of)));
}
}
if (settings.externalResourceReaders() != null) {
for (var entry : settings.externalResourceReaders().entrySet()) {
addResourceReader(
ResourceReaders.externalProcess(
entry.getKey(),
procs.computeIfAbsent(entry.getValue(), ExternalReaderProcess::of)));
entry.getKey(), procs.computeIfAbsent(entry.getValue(), ReaderProcess::of)));
}
}
return this;
Expand Down
4 changes: 2 additions & 2 deletions pkl-core/src/main/java/org/pkl/core/ast/builder/AstBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@
import org.pkl.core.ast.lambda.ApplyVmFunction1NodeGen;
import org.pkl.core.ast.member.*;
import org.pkl.core.ast.type.*;
import org.pkl.core.externalreader.ExternalReaderProcessException;
import org.pkl.core.externalreader.ReaderProcessException;
import org.pkl.core.module.ModuleKey;
import org.pkl.core.module.ModuleKeys;
import org.pkl.core.module.ResolvedModuleKey;
Expand Down Expand Up @@ -1864,7 +1864,7 @@ private URI resolveImport(String importUri, StringConstantContext importUriCtx)
.withHint(e.getHint())
.withSourceSection(createSourceSection(importUriCtx))
.build();
} catch (ExternalReaderProcessException e) {
} catch (ReaderProcessException e) {
throw exceptionBuilder()
.evalError("externalReaderFailure")
.withCause(e.getCause())
Expand Down
4 changes: 2 additions & 2 deletions pkl-core/src/main/java/org/pkl/core/ast/expression/unary/AbstractReadNode.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@
import java.net.URI;
import java.net.URISyntaxException;
import org.pkl.core.SecurityManagerException;
import org.pkl.core.externalreader.ExternalReaderProcessException;
import org.pkl.core.externalreader.ReaderProcessException;
import org.pkl.core.module.ModuleKey;
import org.pkl.core.packages.PackageLoadError;
import org.pkl.core.runtime.VmContext;
Expand Down Expand Up @@ -76,7 +76,7 @@ private URI resolveResource(ModuleKey moduleKey, String resourceUri) {
.build();
} catch (PackageLoadError | SecurityManagerException e) {
throw exceptionBuilder().withCause(e).build();
} catch (ExternalReaderProcessException e) {
} catch (ReaderProcessException e) {
throw exceptionBuilder().evalError("externalReaderFailure").withCause(e).build();
}

Expand Down
4 changes: 2 additions & 2 deletions pkl-core/src/main/java/org/pkl/core/ast/expression/unary/ImportGlobNode.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@
import java.net.URI;
import org.pkl.core.SecurityManagerException;
import org.pkl.core.ast.member.SharedMemberNode;
import org.pkl.core.externalreader.ExternalReaderProcessException;
import org.pkl.core.externalreader.ReaderProcessException;
import org.pkl.core.http.HttpClientInitException;
import org.pkl.core.module.ResolvedModuleKey;
import org.pkl.core.packages.PackageLoadError;
Expand Down Expand Up @@ -105,7 +105,7 @@ public Object executeGeneric(VirtualFrame frame) {
.evalError("invalidGlobPattern", globPattern)
.withHint(e.getMessage())
.build();
} catch (ExternalReaderProcessException e) {
} catch (ReaderProcessException e) {
throw exceptionBuilder().evalError("externalReaderFailure").withCause(e).build();
}
}
Expand Down
4 changes: 2 additions & 2 deletions pkl-core/src/main/java/org/pkl/core/ast/expression/unary/ReadGlobNode.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@
import org.graalvm.collections.EconomicMap;
import org.pkl.core.SecurityManagerException;
import org.pkl.core.ast.member.SharedMemberNode;
import org.pkl.core.externalreader.ExternalReaderProcessException;
import org.pkl.core.externalreader.ReaderProcessException;
import org.pkl.core.http.HttpClientInitException;
import org.pkl.core.module.ModuleKey;
import org.pkl.core.runtime.VmContext;
Expand Down Expand Up @@ -104,7 +104,7 @@ public Object read(String globPattern) {
.evalError("invalidGlobPattern", globPattern)
.withHint(e.getMessage())
.build();
} catch (ExternalReaderProcessException e) {
} catch (ReaderProcessException e) {
throw exceptionBuilder().evalError("externalReaderFailure").withCause(e).build();
}
}
Expand Down
19 changes: 7 additions & 12 deletions pkl-core/src/main/java/org/pkl/core/evaluatorSettings/PklEvaluatorSettings.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,24 +137,19 @@ public record Http(@Nullable Proxy proxy) {
}

public record Proxy(@Nullable URI address, @Nullable List<String> noProxy) {
public static Proxy create(@Nullable String address, @Nullable List<String> noProxy) {
URI addressUri;
try {
addressUri = address == null ? null : new URI(address);
} catch (URISyntaxException e) {
throw new PklException(ErrorMessages.create("invalidUri", address));
}
return new Proxy(addressUri, noProxy);
}

@SuppressWarnings("unchecked")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@translatenix translatenix Nov 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

public static @Nullable Proxy parse(Value input) {
if (input instanceof PNull) {
return null;
} else if (input instanceof PObject proxy) {
var address = (String) proxy.get("address");
@SuppressWarnings("unchecked")
var noProxy = (List<String>) proxy.get("noProxy");
return create(address, noProxy);
try {
var addressUri = address == null ? null : new URI(address);
return new Proxy(addressUri, noProxy);
} catch (URISyntaxException e) {
throw new PklException(ErrorMessages.create("invalidUri", address));
}
} else {
throw PklBugException.unreachableCode();
}
Expand Down
8 changes: 4 additions & 4 deletions ...der/ExternalReaderMessagePackDecoder.java → ...re/externalreader/MessagePackDecoder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,20 +21,20 @@
import org.msgpack.core.MessagePack;
import org.msgpack.core.MessageUnpacker;
import org.msgpack.value.Value;
import org.pkl.core.externalreader.ExternalReaderMessages.*;
import org.pkl.core.externalreader.ReaderMessages.*;
import org.pkl.core.messaging.BaseMessagePackDecoder;
import org.pkl.core.messaging.DecodeException;
import org.pkl.core.messaging.Message;
import org.pkl.core.messaging.Message.Type;
import org.pkl.core.util.Nullable;

final class ExternalReaderMessagePackDecoder extends BaseMessagePackDecoder {
final class MessagePackDecoder extends BaseMessagePackDecoder {

public ExternalReaderMessagePackDecoder(MessageUnpacker unpacker) {
public MessagePackDecoder(MessageUnpacker unpacker) {
super(unpacker);
}

public ExternalReaderMessagePackDecoder(InputStream inputStream) {
public MessagePackDecoder(InputStream inputStream) {
this(MessagePack.newDefaultUnpacker(inputStream));
}

Expand Down
8 changes: 4 additions & 4 deletions ...der/ExternalReaderMessagePackEncoder.java → ...re/externalreader/MessagePackEncoder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,19 +19,19 @@
import java.io.OutputStream;
import org.msgpack.core.MessagePack;
import org.msgpack.core.MessagePacker;
import org.pkl.core.externalreader.ExternalReaderMessages.*;
import org.pkl.core.externalreader.ReaderMessages.*;
import org.pkl.core.messaging.BaseMessagePackEncoder;
import org.pkl.core.messaging.Message;
import org.pkl.core.messaging.ProtocolException;
import org.pkl.core.util.Nullable;

final class ExternalReaderMessagePackEncoder extends BaseMessagePackEncoder {
final class MessagePackEncoder extends BaseMessagePackEncoder {

public ExternalReaderMessagePackEncoder(MessagePacker packer) {
public MessagePackEncoder(MessagePacker packer) {
super(packer);
}

public ExternalReaderMessagePackEncoder(OutputStream outputStream) {
public MessagePackEncoder(OutputStream outputStream) {
this(MessagePack.newDefaultPacker(outputStream));
}

Expand Down
19 changes: 19 additions & 0 deletions pkl-core/src/main/java/org/pkl/core/externalreader/ModuleReaderSpec.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
/*
* Copyright © 2024 Apple Inc. and the Pkl project authors. All rights reserved.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* https://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package org.pkl.core.externalreader;

public record ModuleReaderSpec(
String scheme, boolean hasHierarchicalUris, boolean isLocal, boolean isGlobbable) {}
17 changes: 6 additions & 11 deletions ...l/core/module/ExternalModuleResolver.java → ...l/core/externalreader/ModuleResolver.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,24 +13,19 @@
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package org.pkl.core.module;
package org.pkl.core.externalreader;

import java.io.IOException;
import java.net.URI;
import java.util.List;
import org.pkl.core.SecurityManager;
import org.pkl.core.SecurityManagerException;
import org.pkl.core.messaging.MessageTransport;
import org.pkl.core.module.PathElement;

public interface ExternalModuleResolver {

interface Spec {
boolean hasHierarchicalUris();

boolean isGlobbable();

boolean isLocal();

String scheme();
public interface ModuleResolver {
static ModuleResolver of(MessageTransport transport, long evaluatorId) {
return new ModuleResolverImpl(transport, evaluatorId);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@translatenix translatenix Nov 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
}

}

String resolveModule(SecurityManager securityManager, URI uri)
Expand Down
10 changes: 6 additions & 4 deletions ...aging/MessageTransportModuleResolver.java → ...re/externalreader/ModuleResolverImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package org.pkl.core.messaging;
package org.pkl.core.externalreader;

import java.io.IOException;
import java.net.URI;
Expand All @@ -26,21 +26,23 @@
import java.util.concurrent.Future;
import org.pkl.core.SecurityManager;
import org.pkl.core.SecurityManagerException;
import org.pkl.core.messaging.MessageTransport;
import org.pkl.core.messaging.MessageTransports;
import org.pkl.core.messaging.Messages.ListModulesRequest;
import org.pkl.core.messaging.Messages.ListModulesResponse;
import org.pkl.core.messaging.Messages.ReadModuleRequest;
import org.pkl.core.messaging.Messages.ReadModuleResponse;
import org.pkl.core.module.ExternalModuleResolver;
import org.pkl.core.messaging.ProtocolException;
import org.pkl.core.module.PathElement;

public class MessageTransportModuleResolver implements ExternalModuleResolver {
final class ModuleResolverImpl implements ModuleResolver {
private final MessageTransport transport;
private final long evaluatorId;
private final Map<URI, Future<String>> readResponses = new ConcurrentHashMap<>();
private final Map<URI, Future<List<PathElement>>> listResponses = new ConcurrentHashMap<>();
private final Random requestIdGenerator = new Random();

public MessageTransportModuleResolver(MessageTransport transport, long evaluatorId) {
ModuleResolverImpl(MessageTransport transport, long evaluatorId) {
this.transport = transport;
this.evaluatorId = evaluatorId;
}
Expand Down
16 changes: 7 additions & 9 deletions ...xternalreader/ExternalReaderMessages.java → ...l/core/externalreader/ReaderMessages.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,38 +20,36 @@
import org.pkl.core.messaging.Messages.ResourceReaderSpec;
import org.pkl.core.util.Nullable;

final class ExternalReaderMessages {
private ExternalReaderMessages() {}
final class ReaderMessages {
private ReaderMessages() {}

public record InitializeModuleReaderRequest(long requestId, String scheme)
implements Server.Request {
record InitializeModuleReaderRequest(long requestId, String scheme) implements Server.Request {
public Type type() {
return Type.INITIALIZE_MODULE_READER_REQUEST;
}
}

public record InitializeResourceReaderRequest(long requestId, String scheme)
implements Server.Request {
record InitializeResourceReaderRequest(long requestId, String scheme) implements Server.Request {
public Type type() {
return Type.INITIALIZE_RESOURCE_READER_REQUEST;
}
}

public record InitializeModuleReaderResponse(long requestId, @Nullable ModuleReaderSpec spec)
record InitializeModuleReaderResponse(long requestId, @Nullable ModuleReaderSpec spec)
implements Client.Response {
public Type type() {
return Type.INITIALIZE_MODULE_READER_RESPONSE;
}
}

public record InitializeResourceReaderResponse(long requestId, @Nullable ResourceReaderSpec spec)
record InitializeResourceReaderResponse(long requestId, @Nullable ResourceReaderSpec spec)
implements Client.Response {
public Type type() {
return Type.INITIALIZE_RESOURCE_READER_RESPONSE;
}
}

public record CloseExternalProcess() implements Server.OneWay {
record CloseExternalProcess() implements Server.OneWay {
public Type type() {
return Type.CLOSE_EXTERNAL_PROCESS;
}
Expand Down
Loading