awslabs / aws-java-nio-spi-for-s3

A Java NIO.2 service provider for Amazon S3
Apache License 2.0
55 stars 19 forks source link

Files.move/copy seems to put source object under directory with name of target #430

Closed hubfruser closed 2 weeks ago

hubfruser commented 2 months ago

Notice this on aws-java-nio-spi-for-s3 1.2.4

Not able to use 2.0.x so not sure if the "issue" exists on 2.0 or not. At least for now I have to stick with JRE 8 and am not able to use higher version (JRE 11, etc). It will take quite a while to do the Java upgrade and it's completely of out of my control about JRE upgrade.

Scenario

Below was the code I used:

String S3_BUCKET = "s3://mybucket";   // no '/' at end of bucket name
URI s3uri = URI.create(S3_BUCKET);
String fileName = "testfile";                  // no '/' in name

try (FileSystem fileSystem = FileSystems.newFileSystem(s3uri, Collections.EMPTY_MAP)) {//for 1.2.4, it seems I may have to create a Filesystem instead of default one which access windows traditional file system on my machine.
    Path filePathTmp = fileSystem.getPath(fileName + ".tmp");  
    Path filePath = fileSystem.getPath(fileName); 
    String content = "hello s3";

    Files.write(filePathTmp, content.getBytes(StandardCharsets.UTF_8));

    Files.move(filePathTmp, filePath, StandardCopyOption.REPLACE_EXISTING);
} 

I expected "testfile.tmp" get created first, then renamed(moved) to "testfile" in the bucket. But what actually happened was that "testfile.tmp" was created first with content of "hello s3", then a new folder named "testfile" was created, and "testfile.tmp" was moved into this folder. No file "testfile" exists except this new folder after the operation.

I tried to use Files.copy then File.delete to replace Files.move, the result was the same (Files.move actually was implemented as Files.copy + Files.delete).

Possible places causing the issue:

When I looked at source code, I think above behavior was caused in S3Path.resolve( ) in software.amazon.nio.spi.s3.S3Path.

in current code (seems for 2.0.0, but 1.2.4 has same or similar logic) on line 448:

concatenatedPath = this + PATH_SEPARATOR + s3Other;

It seems to me, above line concatenates source and target object, with a PATH_SEPARATOR in between, causing the issue I saw above: instead of moving/renaming objects, above line creates a new path which is a concatenation of target and source name, making source object under a new directory named of target object.

After further look, it seems several lines above (around line 439) should have returned s3Other immediately without proceeding to above mentioned code.

    if (s3Other.isAbsolute()) {
        return s3Other;
    }

In this case, s3Other's path or pathRepresentation is 'helloniotmp', this object's pathRepresentation is '/hellonio'. The code 's s3Other isAbsolute returned false (because S3Path.isAbsolute() checks if 'helloniotmp' value starts with '/'). Thus code proceeds to creating new path with concatenation of target/source value.

I tried to prefix the file name with '/', like String fileName = "/testfile"; but got same issue.

I wonder if this is my library usage issue (e.g. miss '/' at beginning/end of my object name) or I did anything wrong above in my code. Thanks for your time to look into it.

markjschreiber commented 1 month ago

Interesting I will see if this also exists in v2.

markjschreiber commented 1 month ago

Confirmed this bug also exists in V2.

markjschreiber commented 1 month ago

Hmm, it looks like the current impl and tests don't really account for how S3 thinks about directories (i.e. they don't really exist) and what is a directory is really only defined by the existence of a trailing /