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

Paths.getPath() not working #14

Closed
stefanofornari opened this issue Nov 18, 2023 · 11 comments
Closed

Paths.getPath() not working #14

stefanofornari opened this issue Nov 18, 2023 · 11 comments

Comments

@stefanofornari
Copy link

stefanofornari commented Nov 18, 2023

To reproduce:

Paths.get(URI.create("sftp://somewhere.com));

Expected:
As per NIO FileSystem behaviour get() should create a file system and return a path

Actual:

java.nio.file.FileSystemNotFoundException: sftp://example.org
	at com.github.robtimus.filesystems.FileSystemMap.get(FileSystemMap.java:165)
	at com.github.robtimus.filesystems.sftp.SFTPFileSystemProvider.getExistingFileSystem(SFTPFileSystemProvider.java:135)
	at com.github.robtimus.filesystems.sftp.SFTPFileSystemProvider.getPath(SFTPFileSystemProvider.java:129)

Reading the readme I understand that the FS should be created before calling Paths.get(), but this does not make much sense IMHO for the following reasons:

  1. if we have already a FS then we can use it to get a path and Paths would be useless
  2. it forces to create a direct dependency between an app and stfp-fs, which is exactly what NIO FS (and Paths) tries to avoid
  3. an app is forced to have specific set up for this library even if never used.
@robtimus
Copy link
Owner

@stefanofornari are you sure you're using the right library? I can get the same error only by not adding sftp-fs to the class or module path, and using a URI that starts "sftp://".

However, I think the issue you're describing is still valid, because if I do add sftp-fs, I get the following message:

jshell> Paths.get(URI.create("sftp://somewhere.com"))
|  Exception java.nio.file.FileSystemNotFoundException: sftp://somewhere.com
|        at FileSystemMap.get (FileSystemMap.java:165)
|        at SFTPFileSystemProvider.getExistingFileSystem (SFTPFileSystemProvider.java:135)
|        at SFTPFileSystemProvider.getPath (SFTPFileSystemProvider.java:129)
|        at Path.of (Path.java:209)
|        at Paths.get (Paths.java:98)
|        at (#2:1)

I could change the code so that Paths.get (and Path.of) create a new file system if none exists yet, but that prevents you from providing configuration (using the env map). That in turn means that authentication won't be possible with the current code. I could allow the username and password to both be given in the URL (which I explicitly don't allow for security reasons), but that would still prevent any other form of authentication.

Another disadvantage of automatically creating file systems is resource management. If a file system is explicitly created, callers know that they need to close it. Who's going to close automatically created file systems? Would you think of calling path.getFileSystem().close()?

@robtimus
Copy link
Owner

robtimus commented Nov 18, 2023

Note that the creation of the file system doesn't have to be located anywhere near the retrieval of Path instances. It could be created at application start-up and closed at application shutdown. Using Paths.get would then be allowed during the entire application runtime. However, I think the username is required in the URI, because the filesystem on the same server could be different for two different users.

@stefanofornari
Copy link
Author

stefanofornari commented Nov 19, 2023

Hi @robtimus , thanks for looking into it and sorry for the not accurate description, I missed up with different tests. I fixed it.

Back to the topic, yes, I believe you should provide the possibility that the most basic information is provided through the URL (e.g. credentials). If you want to avoid that by mistake credentials are exposed for example in a toString() I recommend to not include credentials information in your SFTPPath.toUri() and toString() implementation.

You have definitely a point about more complex configuration. I believe this is a flaw in current NIO FS architecture that can be worked around through system properties, property files or query string depending on the complexity you need to afford.

However keep in mind the final goal is like the following use case: let's say we implement ls in java, let's say jls, which shows the content of a directory based on NIO FS. The users of our jls should be able to just add a jar in the class path and issue the command:

jls sftp://somewhere.com/dir

This should work out of the box because those users will not have the option to compile the command to add your initialization code.

HTH

PS1: please also note that FileSystems.newFileSystem(URI.create("ftp://example.org"), env); tries to connect immediately to the server, which means you can't really create the file system in advance, maybe you can consider a lazy approach.

PS2: I have noticed you have to repositories for ftp and sftp; is there a reason?

@robtimus
Copy link
Owner

I still have to think about how to properly handle your case. Using anonymous access or possibly by providing credentials through the URI would solve some issues, but additional configuration can become complex, even using system properties or a query string, especially for values that cannot be easily be converted to and from strings. Perhaps a way to provide system-wide defaults could work, but that would need to be made thread-safe.

Regarding PS1: env can be configured with an SFTPPoolConfig instance. This defaults to an initial size of 1 and maximum size of 5, but both can be changed; the initial size can be 0.

Regarding PS2: FTP and FTPs are completely different from SFTP, which uses SSH.

@stefanofornari
Copy link
Author

Regarding PS2: FTP and FTPs are completely different from SFTP, which uses SSH.

Ah, cool!

@stefanofornari
Copy link
Author

stefanofornari commented Nov 19, 2023

I still have to think about how to properly handle your case. Using anonymous access or possibly by providing credentials through the URI would solve some issues, but additional configuration can become complex,

I do not think this is "my case", it is the way users expect a NIO File System to work (you can check other implementations as well). IMHO you should follow th 80-20 rule: 80% of the users will most likely be fine with simple configuration on the URL. The remaining 20%, can use newFileSystem() directly, which is anyway part of the interface,

@robtimus
Copy link
Owner

The Javadoc says that throwing a FileSystemNotFoundException is allowed if "The file system, identified by the URI, does not exist and cannot be created automatically". The question remains is - is it reasonable to create a file system automatically without authentication or other configuration? That's what I have to think about. I don't want to implement something that doesn't work well, because I would be stuck with it.

@stefanofornari
Copy link
Author

stefanofornari commented Nov 19, 2023

I am not sure why you are so much concerned about it, and again: you would not create a file system without authentication or configuration, you have to use the URI for it. Actually, authentication is even embedded in the URI object, you do not even have to care about it. For the configuration, what we said before is still valid. BTW, it is how JDBC works and again, check other NIO file systems for reference.

@robtimus
Copy link
Owner

For the simple case where you authenticate with a username/password combination this could work yes, but out of all servers I communicate with over HTTP, only a small number uses that. All of the others use key pairs, using ssh-agent or PuTTY's pageant. Without any additional configuration that wouldn't be possible right now.

@stefanofornari
Copy link
Author

stefanofornari commented Nov 19, 2023

And for those, in step one, users will have to use newFileSystem() like they do today.
In the future you may think of something like ftp:<user>:<password>@<hostname>/<path>?config=...whatever...
Another approach is what AWS SDK uses (which I actually do not like that much) by the means of an "out-of-bound" configuration in user's home properties... there are many options here. Or you can use a singleton and FTPFileSystem can read it... You have many options.

servers I communicate with over HTTP, only a small number uses that. All of the others use key pairs, using ssh-agent or PuTTY's pageant. Without any additional configuration that wouldn't be possible right now.

I am not very familiar with that, but I tent to believe a property file will solve the issue in 80% of cases.

@robtimus
Copy link
Owner

I've just released version 3.3 which improves both newFileSystem and getPath.

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

No branches or pull requests

2 participants