Upplication / Amazon-S3-FileSystem-NIO2

An S3 File System Provider for Java 7
MIT License
122 stars 67 forks source link

allow 'first' element to be a path string, not simply a bucket name #16

Closed johnhartman closed 10 years ago

johnhartman commented 10 years ago

I noticed that the FileSystem.getPath implementation does not allow the 'first' element to be a path string, such as '/foo/bar/. According to the javadoc, it should allow the 'first' element to be a path string, and process it accordingly. I added code to support this.

jarnaiz commented 10 years ago

Hello John,

Thanks for your suggestion, time and PR :)

The first element, with absoulte path, is the bucket, and if i change this, I dont know how to set the bucket. Maybe the Filestore class represent the buckets and in the URI you need to set the endpoint and the default bucket....

In any case these change break the compatibily with the current usage.

I need to think about these, any suggestion is welcome.

Some example in windows: http://howtodoinjava.com/2013/07/08/how-to-define-path-in-java-nio/

pditommaso commented 10 years ago

In my opinion the current syntax is coherent with the Amazon definition which requires a S3 path to start with the bucket name.

http://docs.aws.amazon.com/cli/latest/reference/s3/

I think it should not change.

Cheers, Paolo

On Mon, Aug 11, 2014 at 4:00 PM, Javier notifications@github.com wrote:

Hello John,

Thanks for your suggestion, time and PR :)

The first element, with absoulte path, is the bucket, and if i change this, I dont know how to set the bucket. Maybe the Filestore class represent the buckets and in the URI you need to set the endpoint and the default bucket....

In any case these change break the compatibily with the current usage.

I need to think about these, any suggestion is welcome.

Some example in windows: http://howtodoinjava.com/2013/07/08/how-to-define-path-in-java-nio/

— Reply to this email directly or view it on GitHub https://github.com/Upplication/Amazon-S3-FileSystem-NIO2/pull/16#issuecomment-51783475 .

johnhartman commented 10 years ago

Hi Folks--

I looked at the windows examples from Lokesh and couldn't believe that the outputs would not have file separators so I ran them myself and got the following results:

C:\Lokesh\Setup\workspace\NIOExamples\src\sample.txt C:\Lokesh\Setup\workspace\NIOExamples\src\sample.txt C:\Lokesh\Setup\workspace\NIOExamples\src\sample.txt C:\Lokesh\Setup\workspace\NIOExamples\src\sample.txt C:\Lokesh\Setup\workspace\NIOExamples\src\sample.txt C:\cygwin\home\jhartman\match\src\sample.txt C:\Lokesh\Setup\workspace\NIOExamples\src\sample.txt C:\Lokesh\Setup\workspace\NIOExamples\src\sample.txt C:\cygwin\home\jhartman\match\src\sample.txt C:\Lokesh\Setup\workspace\NIOExamples\src\sample.txt C:\Users\jhartman\downloads\somefile.txt

Note that the output contains file separators, '\' in windows. His outputs are incorrect for some reason.

Note too that in these examples the 'first' element can be a 'path string', as noted in the javadoc description of FileSystem.getPath().

    Path path2 =

fs.getPath("C:/Lokesh/Setup/workspace/NIOExamples/src", "sample.txt");

C:\Lokesh\Setup\workspace\NIOExamples\src\sample.txt

It's important that the implementation accommodate 'first' elements which are path strings because 1) it's in the (poorly written) spec:

and 2) without this, it very much limits the usability of the implementation. The purpose of the abstract FileSystem class is to hide the details of the implementation. The fact that the first path element in this s3 implementation is the s3 bucket name should not be exposed to the abstract FileSystem consumer. Instead the bucket should be handled transparently, thus allowing any (s3) path to be injected into a consuming component, which then operates relative to the injected path. We don't want consumers to have to write conditional code like "if (myFileSystem instance of S3FileSystem) ... else ...". We want them to write purely to the abstract NIO classes--Path, Files, Filesystem, ...

Thoughts?

best, John

On Mon, Aug 11, 2014 at 7:19 AM, Paolo Di Tommaso notifications@github.com wrote:

In my opinion the current syntax is coherent with the Amazon definition which requires a S3 path to start with the bucket name.

http://docs.aws.amazon.com/cli/latest/reference/s3/

I think it should not change.

Cheers, Paolo

On Mon, Aug 11, 2014 at 4:00 PM, Javier notifications@github.com wrote:

Hello John,

Thanks for your suggestion, time and PR :)

The first element, with absoulte path, is the bucket, and if i change this, I dont know how to set the bucket. Maybe the Filestore class represent the buckets and in the URI you need to set the endpoint and the default bucket....

In any case these change break the compatibily with the current usage.

I need to think about these, any suggestion is welcome.

Some example in windows: http://howtodoinjava.com/2013/07/08/how-to-define-path-in-java-nio/

— Reply to this email directly or view it on GitHub < https://github.com/Upplication/Amazon-S3-FileSystem-NIO2/pull/16#issuecomment-51783475>

.

— Reply to this email directly or view it on GitHub https://github.com/Upplication/Amazon-S3-FileSystem-NIO2/pull/16#issuecomment-51786013 .

jarnaiz commented 10 years ago

Hello,

mmm, ok i think i understand you, but to me, the bucket represent the same as the filestore in windows, like C:/. In the windows implementations if you dont set C:/ like this

Path path2 = fs.getPath("/hello", "world.txt");

The implementation handle this and use the default fileStore, C:\hello\world.txt. But in S3 dont exists the concept of default bucket. And i dont know how to handle this in a correct way.

Anyway i change the way you can create S3Path, and now is more powerful:

You can create Paths like:

Path path = fs.getPath("/bucket", "fileordir");
Path path = fs.getPath("relativedir", "fileordir");
Path path = fs.getPath("/bucket/dir", "file");
Path path = fs.getPath("/bucket/dir", "dir/file");
Path path = fs.getPath("/bucket/dir", "dir", "file");
Path path = fs.getPath("/bucket/dir", "dir", "dir/file");
johnhartman commented 10 years ago

Okay, so now one can create paths relative to the bucket. That's wonderful. Not need to pull my changes in that case.

Regarding a S3 and defaults, I agree: there is no such thing as a default bucket. Therefore the Filesystems.getNewFileSystem() and Filesystems.newFileSystem() calls should throw exceptions if the S3 URI does not contain a bucket name, that is, does not contain at least a first path element. If it does, then, the whole path is the root of the filesystem and getPath() calls are relative to it.

Sounds like you are on the right 'path' ;)

best, John

jarnaiz commented 10 years ago

thanks :)

I will close this pull request.

Cheers