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.

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

Add ResourceReaderFactory abstraction #647

Closed
wants to merge 1 commit into from

Conversation

HT154
Copy link
Contributor

@HT154 HT154 commented Sep 15, 2024

This is preparatory work for SPICE-0009. It is being contributed in a separate pull request to ease review.

This is required to allow deferred launch of external reader processes without requiring up-front configuration of scheme->process mappings. More details on why this is required here: apple/pkl-evolution#10 (comment)

One concern here (which applies before this change as well) is that the semantics of module key factories and resource reader factories is reversed. For module key factories, the first factory that answers for a URI scheme "wins", while for resource reader factories, the last reader factory that answers for a URI scheme "wins". This PR preserves that behavior, but it may be worth reconsidering this design at this time.

This behavior can be observed in practice in ResourceReadersEvaluatorTest.`module path` where ResourceReaders.modulePath is added after the pre-configured ResourceReaders.classPath and takes precedence.

This is a fairly large breaking API change, but in practice most clients will only need to replace a few lines in their calls to EvaluatorBuilder, eg.

.addResourceReader(ResourceReaders.environmentVariable())
// becomes
.addResourceReaderFactory(ResourceReaderFactories.environmentVariable())

Comment on lines +25 to +37
/**
* Returns a {@link ResourceReader} for the given absolute normalized URI, or {@code
* Optional.empty()} if this factory cannot handle the given URI.
*
* <p>Implementations must not perform any I/O related to the given URI. For example, they must
* not check if the module represented by the given URI exists.
*
* <p>Throws {@link URISyntaxException} if the given URI has invalid syntax.
*
* @param uri an absolute normalized URI
* @return a resource for the given URI
*/
Optional<ResourceReader> create(URI uri);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For implementing external readers, this method will be the point at which the child process is spawned (if it is not already running) and the resource/module readers it implements will be discovered. When the process needs to be spawned, this call will incur that one-time delay.

This is preparatory work for [SPICE-0009](apple/pkl-evolution#10).
It is being contributed in a separate pull request to ease review.

This is required to allow deferred launch of external reader processes without requiring up-front configuration of scheme->process mappings.
More details on why this is required here: apple/pkl-evolution#10 (comment)

One concern here (which applies before this change as well) is that the semantics of module key factories and resource reader factories is reversed.
For module key factories, the first factory that answers for a URI scheme "wins", while for resource reader factories, the _last_ reader factory that answers for a URI scheme "wins".
This PR preserves that behavior, but it may be worth reconsidering this design at this time.

This behavior can be observed in practice in <code>ResourceReadersEvaluatorTest.\`module path\`</code> where `ResourceReaders.modulePath` is added after the pre-configured `ResourceReaders.classPath` and takes precedence.

This is a fairly large breaking API change, but in practice most clients will only need to replace a few lines in their calls to `EvaluatorBuilder`, eg.
```java
.addResourceReader(ResourceReaders.environmentVariable())
// becomes
.addResourceReaderFactory(ResourceReaderFactories.environmentVariable())
```
@bioball
Copy link
Contributor

bioball commented Sep 17, 2024

One concern here (which applies before this change as well) is that the semantics of module key factories and resource reader factories is reversed. For module key factories, the first factory that answers for a URI scheme "wins", while for resource reader factories, the last reader factory that answers for a URI scheme "wins". This PR preserves that behavior, but it may be worth reconsidering this design at this time.

Ah, that's pretty unfortunate... that would be a pretty surprising change of behavior, for interfaces that otherwise look so similar. It'd probably be better to flip the order for resource readers, at the cost of a bigger breaking change here.

I'm am wondering if we do want the factory approach here. Do we need external module/resource readers to have unknown schemes ahead of time? Let's chat more about it on the spice.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants