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

fix: Files.exist() will now always return false for buckets that don't exist. #863

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

lbergelson
Copy link
Contributor

@sydney-munro Here's a potential solution to #859 which makes sense to me. The implementation could probably be improved since this adds an extra request in many cases.

It might also make more sense to replace the NoSuchFileException here with a more specific IOException like BucketDoesNotExistException which will be handled appropriately by calling code that understands IOException. Adding a new runtime exception here (like FileSystemNotFoundException) would cause issues since Files.exists() and potentially other methods have special handling for the IOExceptions.

Previously Files.exist() would either crash with a StorageException or return true when
checking a path that to a bucket that doesn't exist. This is technically a breaking change
since previously all paths that ended in / were considered to exist when usePseudoDirectories
was set.

Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

  • Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
  • Ensure the tests and linter pass
  • Code coverage does not decrease (if any source code was changed)
  • Appropriate docs were updated (if necessary) <- I haven't touched these yet.

Fixes #859 ☕️

…that don't exist

Previously Files.exist() would either crash with a StorageException or return true when
checking a path that to a bucket that doesn't exist.  This is technically a breaking change
since previously all paths that ended in / were considered to exist when usePseudoDirectories
was set.
@lbergelson lbergelson requested a review from a team as a code owner March 15, 2022 22:39
@product-auto-label product-auto-label bot added the api: storage Issues related to the googleapis/java-storage-nio API. label Mar 15, 2022
@lbergelson
Copy link
Contributor Author

There are a bunch of test failures, it looks like most are because Files.exist() was used to trigger requester pays exceptions in tests and now it doesn't in those cases. I'll take a look tomorrow.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: storage Issues related to the googleapis/java-storage-nio API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Inconsistent behavior in Files.exists()
1 participant