Support modeled exceptions in event streams #655
Conversation
varunnvs92
commented
Aug 10, 2018
varunnvs92
commented
- Add support for modeled exceptions in event streams
- Fix bug in AddExceptionShapes class where we only generate exceptions declared in operations. But event shapes can have modeled exceptions as members.
| @@ -46,28 +44,21 @@ | |||
| // Java shape models, to be constructed | |||
| final Map<String, ShapeModel> javaShapes = new HashMap<String, ShapeModel>(); | |||
|
|
|||
| for (Map.Entry<String, Operation> entry : getServiceModel().getOperations().entrySet()) { | |||
| for (Map.Entry<String, Shape> shape : getServiceModel().getShapes().entrySet()) { | |||
| if (shape.getValue().isException()) { | |||
There was a problem hiding this comment.
I think this will be okay to do but you may want to validate this by importing the Dev preview package to see if any unexpected source changes occur in the generated client.
There was a problem hiding this comment.
I validated the shape names but makes sense to review the entire generated code
There was a problem hiding this comment.
I verified that there is no diff in the generated code.
| Throwable exception = exceptionUnmarshaller.handle(adaptMessageToResponse(m, true), | ||
| EMPTY_EXECUTION_ATTRIBUTES); | ||
| SdkHttpFullResponse errorResponse = adaptMessageToResponse(m, true); | ||
| if (!errorResponse.firstMatchingHeader(X_AMZN_REQUEST_ID_HEADER).isPresent() && |
There was a problem hiding this comment.
I think you can omit the first part of the conditional. The error response shouldn't have a request id.
There was a problem hiding this comment.
ok, I was thinking in case the format changes in future. Will update
| import software.amazon.awssdk.core.runtime.transform.Unmarshaller; | ||
|
|
||
| @SdkProtectedApi | ||
| public class EventStreamExceptionJsonUnmarshaller<T extends AwsServiceException> |
There was a problem hiding this comment.
Should we keep this around for backwards compat and add a ReviewBeforeRelease so we only make a breaking change once at full GA?
| return null; | ||
| } | ||
|
|
||
| if (filteredHeaders.size() > 1) { |
There was a problem hiding this comment.
What is the customer experience in this situation? We generally try to be as resilient as possible when unmarshlaling the exceptions so we can preserve as much info as possible about the error.
There was a problem hiding this comment.
Are you saying we should combine all the error codes? I think its confusing if service multiple headers representing the error code with different values. If they do, maybe we can select the first option and return it? what do you think?
There was a problem hiding this comment.
Whatever we do we should try to fail as gracefully a possible so that we still generate a service exception rather than a runtime one. I'm thinking selecting the first one we find and log a warning.
| assertThat(e.statusCode()).isEqualTo(500); | ||
| assertThat(e.awsErrorDetails().errorCode()).isEqualTo(errorCode); | ||
| assertThat(e.awsErrorDetails().errorMessage()).isEqualTo("foo"); | ||
| assertThat(e.awsErrorDetails().serviceName()).isEqualTo("kinesis"); |
There was a problem hiding this comment.
Where does this value come from? Is it unique?
There was a problem hiding this comment.
It comes from the service name generated in the client interface
There was a problem hiding this comment.
hmmm I don't think we should be using that value as it's not very unique and for some services it's vague like 'execute-api'. I'll create a issue in our backlog to address this as it's not related to this PR.
There was a problem hiding this comment.
I will remove the change to populate the service name then
There was a problem hiding this comment.
You can leave it in if you want. We are already exposing it for normal exceptions so we'll have to figure out the best way forward regardless.
varunnvs92
force-pushed
the
varunkn/EventStreamModeledExceptions
branch
from
2a4c6e9 to
c996f7c
Compare
varunnvs92
force-pushed
the
varunkn/EventStreamModeledExceptions
branch
from
c996f7c to
bd96ba9
Compare
…55dea7e4 Pull request: release <- staging/d4ff341a-63a3-4ef2-9a90-e1f455dea7e4
