Use HttpResponseException in annotation-processor #45053
Conversation
samvaity
force-pushed
the
exception-update
branch
from
a6551c7 to
af2ee68
Compare
API Change CheckAPIView identified API level changes in this PR and created the following API reviews |
samvaity
force-pushed
the
exception-update
branch
from
af2ee68 to
aea6d50
Compare
.../src/main/java/io/clientcore/annotation/processor/test/SimpleXmlSerializableServiceImpl.java
Outdated
Show resolved
Hide resolved
samvaity
force-pushed
the
exception-update
branch
2 times, most recently
from
f401cea to
ea9e072
Compare
...test/src/main/java/io/clientcore/annotation/processor/test/ParameterizedHostServiceImpl.java
Outdated
Show resolved
Hide resolved
|
API change check APIView has identified API level changes in this PR and created following API reviews. |
| if (value == null || value.toBytes().length == 0) { | ||
| throw CoreUtils.instantiateUnexpectedException(responseCode, networkResponse, null, null); | ||
| } else { | ||
| ParameterizedType returnType = null; |
There was a problem hiding this comment.
I'm guessing this is a placeholder until we begin processing the exceptionBodyClass in UnexpectedResponseExceptionDetail?
There was a problem hiding this comment.
Actually, let me just include it in this PR. Will do a follow up commit.
| } else { | ||
| exceptionMessage.append('"').append(new String(data.toBytes(), StandardCharsets.UTF_8)).append('"'); | ||
| } | ||
| if (decodedValue instanceof IOException || decodedValue instanceof IllegalStateException) { |
There was a problem hiding this comment.
I don't think this logic is correct with what has changed between annotation processor and RestProxy. The decoded value should be the declared exception type, ex ErrorValue.
| } else if (data == null || data.toBytes().length == 0) { | ||
| exceptionMessage.append("(empty body)"); | ||
| } else { | ||
| exceptionMessage.append('"').append(new String(data.toBytes(), StandardCharsets.UTF_8)).append('"'); |
There was a problem hiding this comment.
By the time this method is call data would have been consumed to create decodedValue. Attempting to retrieve content again here might cause unexpected exceptions such as a stream closed exception if the response was using streaming.
| if (value == null || value.toBytes().length == 0) { | ||
| networkResponse.close(); | ||
| throw CoreUtils.instantiateUnexpectedException(responseCode, networkResponse, null, null); | ||
| } else { | ||
| ParameterizedType returnType = CoreUtils.createParameterizedType(io.clientcore.annotation.processor.test.implementation.models.HttpBinJSON.class); | ||
| Object decoded = CoreUtils.decodeNetworkResponse(value, jsonSerializer, returnType); | ||
| networkResponse.close(); | ||
| throw CoreUtils.instantiateUnexpectedException(responseCode, networkResponse, value, decoded); | ||
| } |
There was a problem hiding this comment.
Would we be better off creating a utility method in Core to handle this logic, rather than repeating in every generated API?
| () -> createService(TestInterfaceClientImpl.TestInterfaceClientService.class).getBytes(getRequestUri())); | ||
|
|
||
| assertEquals("Status code 200, (1024-byte body)", e.getMessage()); | ||
| assertTrue(e.getMessage().startsWith("Unexpected character")); |
There was a problem hiding this comment.
This test logic to me feels off. The message here seems to be a deserialization error when handling the exception type.
| @HttpRequestInformation(method = HttpMethod.PUT, path = "put", expectedStatusCodes = { 201 }) | ||
| @UnexpectedResponseExceptionDetail(statusCode = { 400 }) | ||
| @UnexpectedResponseExceptionDetail(exceptionBodyClass = HttpBinJSON.class) | ||
| HttpBinJSON putWithUnexpectedResponseAndFallthroughExceptionType(@HostParam("uri") String uri, |
There was a problem hiding this comment.
Were these APIs removed or moved? They seem to test valid cases where different response codes correlate to different exception types.
There was a problem hiding this comment.
We don't support those different ExceptionTypes anymore right, but rather just a general HttpResponseException or CoreException?
There was a problem hiding this comment.
I explained it poorly as type is too vague.
Correct, we don't support different exception types, such as NotModifiedException extends HttpResponseException.
I was more talking on how we still allow different exception body types to be configured, which isn't handled.
| expectedResponseCheck = "responseCode == " + method.getExpectedStatusCodes().get(0) + ";"; | ||
| if (method.getExpectedStatusCodes() == null || method.getExpectedStatusCodes().isEmpty()) { | ||
| // If expectedStatusCodes is empty, treat <400 as expected | ||
| expectedResponseCheck = "responseCode < 400"; |
There was a problem hiding this comment.
Hmm, historically only 2xx was treated as expected.
Description
Closes #43728
If an SDK is being regenerated based on a new swagger spec, a link to the pull request containing these swagger spec changes has been included above.
All SDK Contribution checklist:
General Guidelines and Best Practices
Testing Guidelines