Update Throws to use ClientLogger.logAndThrow #4756
Conversation
alzimmermsft
added
Storage
alzimmermsft
requested review from
jaschrep-msft,
rickle-msft and
sima-zhu
as code owners
| return urlBuilder.toURL(); | ||
| } catch (MalformedURLException e) { | ||
| throw new RuntimeException(String.format("Invalid URL on %s: %s" + getClass().getSimpleName(), azureBlobStorage.getUrl()), e); | ||
| logger.logAndThrow(new IllegalStateException(String.format("Invalid URL on %s: %s" + getClass().getSimpleName(), azureBlobStorage.getUrl()), e)); |
There was a problem hiding this comment.
It throws different exception in different client.
It does not make sense to throw IllegalArgument as it is not an argument here.
IllegalState is using for the following case:
Signals that a method has been invoked at an illegal or inappropriate time. In other words, the Java environment or Java application is not in an appropriate state for the requested operation.
Can we return the RuntimeException? What's the concern of throwing RuntimeException?
|
We have a lot of Storage PRs right now. @mssfang and @sima-zhu could you review this PR with @alzimmermsft? After our local team has done the review / iteration / updates, @alzimmermsft can then circle back with @rickle-msft or @jaschrep-msft for a final sanity check. |
|
@sima-zhu yes, please still code review the PR and give feedback regardless if this will get merged this week or next. Given the high number of Storage PRs we should start reviewing our own team's PRs to the storage/sdk as a first pass before tagging the storage team to lighten their PR load a bit more. |
| // Throwing is preferred to Single.error because this will error out immediately instead of waiting until | ||
| // subscription. | ||
| throw new UnsupportedOperationException("ETag access conditions are not supported for this API."); | ||
| logger.logAndThrow(new UnsupportedOperationException(ETAG_NOT_ALLOWED_MESSAGE)); |
There was a problem hiding this comment.
It is not really needed to add a new message field. No need to be consistent. If you think the entire block is duplicate, it is much better to have a helper method.
| // subscription. | ||
| throw new UnsupportedOperationException( | ||
| "If-Modified-Since is the only HTTP access condition supported for this API"); | ||
| logger.logAndThrow(new UnsupportedOperationException("If-Modified-Since is the only HTTP access condition supported for this API")); |
| options = options == null ? new ListBlobsOptions() : options; | ||
| if (options.details().snapshots()) { | ||
| throw new UnsupportedOperationException("Including snapshots in a hierarchical listing is not supported."); | ||
| logger.logAndThrow(new UnsupportedOperationException("Including snapshots in a hierarchical listing is not supported.")); |
There was a problem hiding this comment.
return null, Also applied to some other places.
| logger.logAndThrow(new IllegalArgumentException(e)); | ||
| } | ||
|
|
||
| StringBuilder canonicalName = new StringBuilder("/blob"); |
There was a problem hiding this comment.
The statements here does not have the chance of throwing MalformatedURLException. It is better to leave outside.
| signature = sharedKeyCredentials.computeHmac256(stringToSign); | ||
| } catch (InvalidKeyException e) { | ||
| throw new Error(e); // The key should have been validated by now. If it is no longer valid here, we fail. | ||
| logger.logAndThrow(new IllegalStateException(e)); // The key should have been validated by now. If it is no longer valid here, we fail. |
There was a problem hiding this comment.
IllegalStateException seems not very relevant.
|
|
||
| if (sasTokenCredential == null && sharedKeyCredential == null) { | ||
| throw new IllegalArgumentException("Credentials are required for authorization"); | ||
| logger.logAndThrow(new IllegalStateException("Credentials are required for authorization")); |
There was a problem hiding this comment.
IllegalStateException usually thrown when something wrong invoked in runtime. It is more like a RuntimeException
| } catch (MalformedURLException e) { | ||
| throw new RuntimeException(String.format("Invalid URL on %s: %s" + getClass().getSimpleName(), | ||
| azureFileStorageClient.getUrl()), e); | ||
| logger.logAndThrow(new IllegalStateException(String.format("Invalid URL on %s: %s" + getClass().getSimpleName(), |
There was a problem hiding this comment.
Wrong usage of string format.
| channel.close(); | ||
| } catch (IOException e) { | ||
| throw new UncheckedIOException(e); | ||
| logger.logAndThrow(new UncheckedIOException(e)); |
There was a problem hiding this comment.
always end with 'return' statement after logger.logAndThrow()
| // Throwing is preferred to Mono.error because this will error out immediately instead of waiting until | ||
| // subscription. | ||
| throw new IllegalArgumentException("Duration must be -1 or between 15 and 60."); | ||
| logger.logAndThrow(new IllegalArgumentException("Duration must be -1 or between 15 and 60.")); |
There was a problem hiding this comment.
always end with 'return' statement after logger.logAndThrow()
| stream.write(bf.array()); | ||
| } catch (IOException e) { | ||
| throw new UncheckedIOException(e); | ||
| logger.logAndThrow(new UncheckedIOException(e)); |
There was a problem hiding this comment.
always end with 'return' statement after logger.logAndThrow()
| } | ||
| } catch (MalformedURLException ex) { | ||
| throw new IllegalArgumentException("The Azure Storage Blob endpoint url is malformed."); | ||
| logger.logAndThrow(new IllegalArgumentException("The Azure Storage Blob endpoint url is malformed.")); |
There was a problem hiding this comment.
always end with 'return' statement after logger.logAndThrow()
There was a problem hiding this comment.
@mssfang can you work with @alzimmermsft and make sure this starts failing CI very early in Preview 3? We need to quickly get out o the world that code can pass CI but break the build after merge.
|
|
||
| if (ImplUtils.isNullOrEmpty(accountName) || ImplUtils.isNullOrEmpty(accountKey)) { | ||
| throw new IllegalArgumentException("Connection string must contain 'AccountName' and 'AccountKey'."); | ||
| logger.logAndThrow(new IllegalArgumentException("Connection string must contain 'AccountName' and 'AccountKey'.")); |
There was a problem hiding this comment.
always end with 'return' statement after logger.logAndThrow()
| public int read(final byte[] b, final int off, final int len) throws IOException { | ||
| if (off < 0 || len < 0 || len > b.length - off) { | ||
| throw new IndexOutOfBoundsException(); | ||
| logger.logAndThrow(new IndexOutOfBoundsException()); |
There was a problem hiding this comment.
always end with 'return' statement after logger.logAndThrow()
|
|
||
| if (blobRangeOffset < 0 || (blobRangeLength != null && blobRangeLength <= 0)) { | ||
| throw new IndexOutOfBoundsException(); | ||
| logger.logAndThrow(new IndexOutOfBoundsException()); |
There was a problem hiding this comment.
always end with 'return' statement after logger.logAndThrow()
|
|
||
| if (n < 0 || this.currentAbsoluteReadPosition + n > this.streamLength + this.blobRangeOffset) { | ||
| throw new IndexOutOfBoundsException(); | ||
| logger.logAndThrow(new IndexOutOfBoundsException()); |
There was a problem hiding this comment.
always end with 'return' statement after logger.logAndThrow()
| Utility.assertNotNull("expiry", expiry); | ||
| if (start != null && !start.isBefore(expiry)) { | ||
| throw new IllegalArgumentException("`start` must be null or a datetime before `expiry`."); | ||
| logger.logAndThrow(new IllegalArgumentException("`start` must be null or a datetime before `expiry`.")); |
There was a problem hiding this comment.
always end with 'return' statement after logger.logAndThrow()
|
Changes to use |
4 participants
Updates all instance based throws to use ClientLogger.logAndThrow instead as per our spec. Additionally, there was some changes to the exception types being thrown to better align with the spec as well.