Nio check access #18095
Nio check access #18095
Conversation
rickle-msft
requested review from
alzimmermsft,
amishra-dev,
gapra-msft,
jaschrep-msft and
kasobol-msft
as code owners
ghost
added
the
| readAttributes(path, BasicFileAttributes.class); | ||
| } catch(IOException e) { | ||
| if (e.getCause() != null && e.getCause() instanceof BlobStorageException | ||
| && BlobErrorCode.BLOB_NOT_FOUND.equals(((BlobStorageException) e.getCause()).getErrorCode())) { |
There was a problem hiding this comment.
Could this also be ContainerNotFound?
There was a problem hiding this comment.
Once I add support for containers, yes. A good reminder that I'll have to update a lot of error handling like that
| } | ||
| } | ||
|
|
||
| class checkAccessIoExceptionResponse extends HttpResponse { |
| os.close() | ||
|
|
||
| when: | ||
| fs.provider().checkAccess(path) |
There was a problem hiding this comment.
you could probably just do
expect:
fs.provider().checkAccess(path)
There was a problem hiding this comment.
I think it's functionally the same. If all I'm doing is check the call succeeds, I usually like to make that explicit by saying I don't want an exception.
| * Checks the existence, and optionally the accessibility, of a file. | ||
| * <p> | ||
| * This method may only be used to check the existence of a file. It is not possible to determine the permissions | ||
| * granted to a given client, so if any mode argument is specified, an {@link UnsupportedOperationException} will be |
There was a problem hiding this comment.
Should this be UnsupportedOperationException or AccessDeniedException?
There was a problem hiding this comment.
(per the jdk docs for this method: "the requested access would be denied or the access cannot be determined because the Java virtual machine has insufficient privileges or other reasons." I think it's reasonable to put this under access cannot be determined)
| // Read attributes already wraps BlobStorageException in an IOException. | ||
| try { | ||
| readAttributes(path, BasicFileAttributes.class); | ||
| } catch(IOException e) { |
There was a problem hiding this comment.
Just a thought - should auth errors be wrapped into AccessDeniedExceptions?
There was a problem hiding this comment.
Good question. I tend to think that debugging will be easier if issues with underlying blob stuff is presented differently from nio stuff. I can see an argument where that kind of breaks the point of mapping one to the other, but an AccessDeniedException seems like it would have a response of "well let me go grant access" which is impossible in this case, and an IOException caused by a BlobStorageException kind of leads me down a path of something is wrong with my account or nio configs. What do you think?
There was a problem hiding this comment.
If that's the general pattern we've aimed for with nio then I'm fine with that.
There was a problem hiding this comment.
I'm not sure if it's generalized yet haha. Still could change if you think otherwise. But we'd have to add that explicit check in a lot of different places, which I'm also not a fan of because I think we're likely to forget one.
|
This pull request is protected by Check Enforcer. What is Check Enforcer?Check Enforcer helps ensure all pull requests are covered by at least one check-run (typically an Azure Pipeline). When all check-runs associated with this pull request pass then Check Enforcer itself will pass. Why am I getting this message?You are getting this message because Check Enforcer did not detect any check-runs being associated with this pull request within five minutes. This may indicate that your pull request is not covered by any pipelines and so Check Enforcer is correctly blocking the pull request being merged. What should I do now?If the check-enforcer check-run is not passing and all other check-runs associated with this PR are passing (excluding license-cla) then you could try telling Check Enforcer to evaluate your pull request again. You can do this by adding a comment to this pull request as follows: What if I am onboarding a new service?Often, new services do not have validation pipelines associated with them, in order to bootstrap pipelines for a new service, you can issue the following command as a pull request comment: |
...azure-storage-blob-nio/src/main/java/com/azure/storage/blob/nio/AzureFileSystemProvider.java
Show resolved
Hide resolved
...azure-storage-blob-nio/src/main/java/com/azure/storage/blob/nio/AzureFileSystemProvider.java
Outdated
Show resolved
Hide resolved
sdk/storage/azure-storage-blob-nio/src/test/java/com/azure/storage/blob/nio/APISpec.groovy
Show resolved
Hide resolved
rickle-msft
force-pushed
the
nioCheckAccess
branch
from
db80495 to
82ddd75
Compare
Fixed examples and added billing info (Azure#18095) * Fixed examples and added billing info * Fixed some methods with no billing info * Fixed a buggy doc * Fix tagging issue to pass lintdiff Co-authored-by: Daniel Rocha <[email protected]>
Resolves #17999 and #18018