fix: prevent crash when checking if a missing file exists #856#858
fix: prevent crash when checking if a missing file exists #856#858BenWhitehead merged 1 commit intogoogleapis:mainfrom
Conversation
| private int maxChannelReopens = 0; | ||
| private @Nullable String userProject = null; | ||
| // This of this as "clear userProject if not RequesterPays" | ||
| // Think of this as "clear userProject if not RequesterPays" |
There was a problem hiding this comment.
opportunistic typo fix
| */ | ||
| public CloudStorageFileSystemProvider withNoUserProject() { | ||
| return new CloudStorageFileSystemProvider("", this.storageOptions); | ||
| return new CloudStorageFileSystemProvider(null, this.storageOptions); |
There was a problem hiding this comment.
This matches how I've seen providers created when there's no project specified.
There was a problem hiding this comment.
To me this does not seem like a necessary change if we are already modifying the check in CloudStoragePath.java to check for both empty and null. I think we should either change this everywhere or change it nowhere and I think a refactoring change to make it consistent everywhere can be done at a later date.
There was a problem hiding this comment.
It's not necessary, I can remove it. It just seemed weird that if you specify no user project then you get a null, but when it removes the user project you get an empty string. I think a refactoring pass to make it consistent would good in order to prevent similar bugs in the future though.
|
I didn't change the attribute setting in the config because I wasn't sure what the right thing to do there is. |
| */ | ||
| public CloudStorageFileSystemProvider withNoUserProject() { | ||
| return new CloudStorageFileSystemProvider("", this.storageOptions); | ||
| return new CloudStorageFileSystemProvider(null, this.storageOptions); |
There was a problem hiding this comment.
To me this does not seem like a necessary change if we are already modifying the check in CloudStoragePath.java to check for both empty and null. I think we should either change this everywhere or change it nowhere and I think a refactoring change to make it consistent everywhere can be done at a later date.
|
@sydney-munro Note that this issue is manifesting for us in our downstream project as a "User project specified in the request is invalid" (See broadinstitute/gatk#7716) We believe that the patch in this PR will resolve this. Unfortunately we are seeing this error all over the place with the latest |
Fixes a crash that occurred when autoDetectRequesterPays is set and a Files.exists() call is made on a file that doesn't exist. Refs: googleapis#856
7bfca45 to
16908f9
Compare
|
@sydney-munro I removed the change you asked about. I think it would be a good idea to make it consistent but so far I don't know of any actual issues because of it. |
|
@sydney-munro Thank you! |
|
Merge-on-green attempted to merge your PR for 6 hours, but it was not mergeable because either one of your required status checks failed, one of your required reviews was not approved, or there is a do not merge label. Learn more about your required status checks here: https://help.github.com/en/github/administering-a-repository/enabling-required-status-checks. You can remove and reapply the label to re-run the bot. |
|
@sydney-munro It didn't merge for some reason. It looks like owl-bot hasn't completed yet. I'm not sure what that is though or how to fix the problem. |
|
@sydney-munro Would it be possible to get a release with this patch once it's merged? Thanks! |
|
@sydney-munro Thank you for the rerun. Looks like it's green now so hopefully it can be merged. |
Fixes a crash that occurred when autoDetectRequesterPays is set and
a Files.exists() call is made on a file that doesn't exist.
Refs: #856
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:
Fixes #856 ☕️
If you write sample code, please follow the samples format.