Serialize and deserialize azure core exception type. #5159
Conversation
sima-zhu
requested review from
alzimmermsft,
gapra-msft,
hemanttanwar,
jaschrep-msft,
jianghaolu,
rickle-msft and
srnagar
as code owners
…ze and deserialize.
sdk/core/azure-core-test/src/main/java/com/azure/core/test/http/PlaybackClient.java
Outdated
Show resolved
Hide resolved
sdk/core/azure-core-test/src/main/java/com/azure/core/test/models/NetworkCallError.java
Show resolved
Hide resolved
sdk/core/azure-core-test/src/main/java/com/azure/core/test/models/NetworkCallError.java
Show resolved
Hide resolved
...torage/azure-storage-blob/src/main/java/com/azure/storage/blob/BlobServiceClientBuilder.java
Outdated
Show resolved
Hide resolved
sdk/storage/azure-storage-blob/src/test/java/com/azure/storage/blob/BlockBlobAPITest.groovy
Outdated
Show resolved
Hide resolved
sdk/storage/azure-storage-blob/src/test/java/com/azure/storage/blob/RetryTest.groovy
Show resolved
Hide resolved
sdk/storage/azure-storage-blob/src/test/java/com/azure/storage/blob/ServiceAPITest.groovy
Show resolved
Hide resolved
sdk/core/azure-core-test/src/main/java/com/azure/core/test/models/NetworkCallError.java
Show resolved
Hide resolved
sdk/core/azure-core-test/src/main/java/com/azure/core/test/models/NetworkCallError.java
Show resolved
Hide resolved
|
Can you please update the description to explain what this PR is attempting to achieve, and why. Is there an issue filed that describes this? Please provide more context. |
| throw logger.logExceptionAsWarning(Exceptions.propagate(networkCallRecord.exception().get())); | ||
| } | ||
|
|
||
| constructExceptionType(networkCallRecord); |
There was a problem hiding this comment.
You should change this. At present the constructExceptionType call is odd, as it may throw an exception but it won't if the networkCallRecord.exception() is null. You should reinstitute the if block you removed, and then make constructExceptionType return an exception that is then definitively thrown.
There was a problem hiding this comment.
the step only takes affect when exception is not null.
Will add a checking before calling the method.
Thanks for catching this.
There was a problem hiding this comment.
My suggestion and what you implemented are slightly different. Please re-read and clarify with me if necessary.
|
|
||
| package com.azure.core.implementation.exception; | ||
|
|
||
| public class UnexpectedLengthException extends IllegalStateException { |
There was a problem hiding this comment.
You are making this class non-public API. Do you ever document anywhere in the storage APIs that this is thrown in certain situations?
There was a problem hiding this comment.
Good call. I will update the API which will throw the exception. This is not intended to use by client. It is for http request validation.
sdk/storage/azure-storage-blob/src/test/java/com/azure/storage/blob/ServiceAPITest.groovy
Show resolved
Hide resolved
| Mono<Response<BlockBlobItem>> upload = blockBlobAsyncClient | ||
| .uploadWithResponse(fbb.subscribeOn(Schedulers.elastic()), length, headers, metadata, accessConditions, context); | ||
| .uploadWithResponse(fbb, length, headers, metadata, accessConditions, context); |
There was a problem hiding this comment.
Did we want to remove the subscribeOn here?
|
Close it as this is not the approach we take |
Summary:
Problem to solve:
Currently, playback can only deserialize specific exception type using switch case. Moreover, the exception must have default constructor. Try to generic the process so people do not need to continue adding different exceptions. Also, the UnexpectedLengthException asks value constructor, which is also a shortage for the playback.
Issue related:
#4193