-
Notifications
You must be signed in to change notification settings - Fork 280
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
[fix] Add support for handling remote file URIs. #629
base: main
Are you sure you want to change the base?
Conversation
Hey Carpe, thanks for the PR! I don't know if this is the right solution to the problem. As far as I can tell, there is no agreed upon protocol when opening files with a remote hostname.
Without a protocol here, I think some valid choices are:
I'm kind of leaning towards option 1 because it's the least amount of commitment and allows us to address this if the need arises later. Curious what @holzensp and @stackoverflow think |
@bioball Hi bioball, I got your idea, If both of them also suggest modifying it to directly throw an exception, I will submit a new commit to implement this. |
Agree that throwing an error seems like the best solution, as there's no consensus on how to handle host names in files. |
I would have expected NFS on Linux/Solaris/FreeBSD/etc. Agreed; randomly guessing protocols to start hammering seems risky, at best. Exception would be the way to go. |
@bioball hi bioball, I change to throw the exception directly. Can you help me to ensure satisfying the requirement? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @Carpe-Wang: can you add this check to org.pkl.core.module.ModuleKeys#file
?
That method should throw a URISyntaxException
if there is a hostname set. And we should remove the check from IoUtils#toUrl
; that's not quite the right place for it.
Thanks for the help here!
Hi @bioball , I want move check URL logic to @Override
public ResolvedModuleKey resolve(SecurityManager securityManager)
throws IOException, SecurityManagerException {
var uri = packageAssetUri.getUri();
try {
if (uri.getHost() != null) {
throw new URISyntaxException(uri.toString(), "Host not allowed");
}
}catch (URISyntaxException e) {
throw new IOException(e.getMessage());
}
securityManager.checkResolveModule(uri);
var dependency =
getProjectDependenciesManager().getResolvedDependency(packageAssetUri.getPackageUri());
var local = getLocalUri(dependency);
if (local != null) {
var resolved =
VmContext.get(null).getModuleResolver().resolve(local).resolve(securityManager);
return ResolvedModuleKeys.delegated(resolved, this);
}
var dep = (Dependency.RemoteDependency) dependency;
assert dep.getChecksums() != null;
var bytes = getPackageResolver().getBytes(packageAssetUri, false, dep.getChecksums());
return ResolvedModuleKeys.virtual(this, uri, new String(bytes, StandardCharsets.UTF_8), true);
} Do you think is okay? If I directly throw a URISyntaxException without catching it, I would need to modify the method signature to public |
I think we should add the check to the This is a pattern that we already have! Take a look at |
Summary
This PR adds support for handling remote file URIs within the project. It addresses the issue where the system could not correctly process URIs that point to remote files.
Changes
file:
URIs.Issue Link
Fixes #616