Issue 163: delete files recursively

[edmang]

Pull Request Description

This pull request closes #163

Acceptance Test

• Building the code with mvn clean install -Pintegration-tests still works.

Questions

• Does this pull request break backward compatibility?

  • Yes
  • No

• Does this pull request require other pull requests to be merged first?

  • Yes, please see #...
  • No

• Does this require an update of the documentation?

  • Yes, please see [in FileSystemProvider class, the delete(Path) method will not throw DirectoryNotEmptyException anymore]
  • No

[carlspring]

Hi @dmndZhWng ,

Thank you for you contribution!

Could you please make the test a bit more extensive and include the following:

• Create some files in the directory
• Create a few nested directories with files with them
• Check for the existence of the above files after performing the delete operation

Also, would you mind signing the ICLA, as described in the Contributing page?

Also, please, feel free to join our chat channel, if you'd like to learn more about the project and/or like to find out what else you could help with.

Kind regards,

Martin

[edmang]

Hi, I ve submit a new PR with some code formatting + fix + Unit Test.
Plus :
1/ I add a new Mock method for submit deleteObjectsRequest (multi request at same time)
2/ I removed the Unit Test for deleteDirectoryWithEntries (because we do not need it anymore since the delete method now handles the case)
However, I d like to know if is there a way to have a technical credential for the Integration test ?

[carlspring]

Hi, I ve submit a new PR with some code formatting + fix + Unit Test.

Thanks for your fixes! We'll review them and get back to you!

Plus :
1/ I add a new Mock method for submit deleteObjectsRequest (multi request at same time)
2/ I removed the Unit Test for deleteDirectoryWithEntries (because we do not need it anymore since the delete method now handles the case)

Great! Thank you very much!

However, I d like to know if is there a way to have a technical credential for the Integration test ?

You would have to create a free tier account and test this.

Have you had a chance to sign the ICLA as I mentioned earlier?

Kind regards,

Martin

[steve-todorov]

@dmndZhWng thanks for your PR! :)

I did some digging today and updated the original issue #163.

The AWS API has limitations - 1000 objects per listing and deletion.
With the proposed implementation you will be deleting only the first 1000 objects and ignoring the rest. What you should do is use a loop and continue deleting until there are no objects in the path or an error occurs (@carlspring should we fail or just print a warning?).

[carlspring]

With the proposed implementation you will be deleting only the first 1000 objects and ignoring the rest. What you should do is use a loop and continue deleting until there are no objects in the path or an error occurs (@carlspring should we fail or just print a warning?).

There is no point in warning, or throwing an error here. It should just loop until there are no more.

[steve-todorov]

We should at least print warnings with potential exceptions or error messages returned by the API (if any)

[edmang]

I ve made some small changes concerning my previous dev :

1/ I successfully made an Integration test too :).

2/ I replaced the Files.walk by an recursion call because the documentation says that we cannot rely on the order returned (but the order of path to delete has a real importance in my case)

3/ I added the delete by batch logic

4/ Concerning the error/warning handling, I notice that the only error thrown is a NoSuchFileException, it happened only when we provide an inexisting path.

[carlspring]

Thanks, @dmndZhWng !

@ptirador , would you like to review and test this? :)

[ptirador]

HI @dmndZhWng! Thanks for your PR! Could you please fix the changes that I requested?
It woukd also be great to create an integration test in FilesITTestclass, something similar to deleteDir method but with filled directories instead of empty.
Thanks.

[ptirador]

Ping @dmndZhWng! Did you have a chance to look at the changes that I requested?
Thanks!

[edmang]

@ptirador sorry for my late answer, i just seen your last request concerning the import file.*, and i ve just made the change. please have a look and do not hesitate if any missing. thanks ;).

[ptirador]

LGTM @dmndZhWng! Thank you so much.
cc @carlspring @steve-todorov

[steve-todorov]

LGTM! Thank you so much for your work on this and @ptirador for reviewing! :)

We will merge this after #184 since currently the tests are not being executed.

[edmang]

@steve-todorov ,@ptirador I have a question concerning the point 4. of what you mentioned in the Issue.
Because since we cannot pass a boolean forceContinue as argument in the delete method (because it inherits from FileSystemProvider).

Maybe we could :

• throw the SdkException at S3FileSystemProvider::delete level (hence, let the user to decide how to handle that case) ?
• or wrap the SDKException and throw it as an IOException in order to keep only 1 base exception for the delete method ?

Because in the current version, we might get directly a SDKException if something happens when delete ...

Do not hesitate if you have any idea / preference.

[steve-todorov]

Oh sorry, I missed that question!

If the thrown exception is NoSuchFileException then you can just catch it and add a debug log with something like Deleting [/path/to/file] was skipped because the path was not found.. For other cases I think we should throw an IOException as soon as the error happens.

@ptirador / @carlspring thoughts?

[edmang]

I think maybe this case has to be handled with the same way too (i.e. log and skipped) ?
try (final DirectoryStream<Path> stream = Files.newDirectoryStream(s3Path)) because Files.newDirectoryStream might throw a SecurityException in the case of the default provider, and a security manager is installed, the SecurityManager#checkRead(String) checkRead method is invoked to check read access to the directory.
@steve-todorov / @ptirador / @carlspring thoughts?

[ptirador]

Oh sorry, I missed that question!

If the thrown exception is NoSuchFileException then you can just catch it and add a debug log with something like Deleting [/path/to/file] was skipped because the path was not found.. For other cases I think we should throw an IOException as soon as the error happens.

@ptirador / @carlspring thoughts?

I agree with this solution. Thanks :)

[ptirador]

Hi @dmndZhWng , could you take a look at these small changes?
Thanks!

[edmang]

hello done :) @ptirador . do not hesitate if any other questions

[ptirador]

LGTM @dmndZhWng! Thanks.
cc @carlspring

[carlspring]

Thanks for reviewing, @ptirador !

This will be merged once @steve-todorov had merged #184 and #186 and tested this pull request.

[steve-todorov]

I have rebased, squashed and tested. The S3 failure in GH Actions because secrets are not passed down forks (known GH Actions issue). I have manually tested this code with S3 and it's working as expected! :)

Thanks to everybody for your active work on this! I'm proceeding with merging into the master :)

[carlspring]

@dmndZhWng ,

Thank you very much for your contribution! Would you like to have a look at #160 as well?

@ptirador , @steve-todorov : Thanks for reviewing and merging this! :)

[edmang]

@carlspring yes sure, there are some common logic i think

[carlspring]

Excellent! Thank you!