Fix http error registration #118
Conversation
| } | ||
| // If model specifies error code use it otherwise default to 400. | ||
| // See https://awslabs.github.io/smithy/1.0/spec/core/http-traits.html#httperror-trait | ||
| val httpStatusCode = errShape.getTrait<HttpErrorTrait>()?.code ?: 400 |
There was a problem hiding this comment.
correctness
400 is correct default for client errors, needs to be 500 for server errors:
Default HTTP status codes
The httpError trait is used to set a custom HTTP response status code. By default, error structures with no httpError trait use the default HTTP status code of the error trait.
400 is used for "client" errors
500 is used for "server" errors
See error trait
There was a problem hiding this comment.
Ah, I was thinking that server errors would be for server-side codegen but that's not it. Nice catch!
| // If model specifies error code use it otherwise default to 400. | ||
| // See https://awslabs.github.io/smithy/1.0/spec/core/http-traits.html#httperror-trait | ||
| val defaultCode = if (errShape.getTrait<ErrorTrait>()?.isClientError ?: error("Expected Error trait on shape $errShape")) 400 else 500 | ||
| val httpStatusCode = errShape.getTrait<HttpErrorTrait>()?.code ?: defaultCode | ||
| writer.write("register(code = #S, deserializer = $deserializerName(), httpStatusCode = $httpStatusCode)", code) |
There was a problem hiding this comment.
Suggestion: These lines are duplicated in RestJsonErrorMiddleware.kt. Seems like it could be moved up to ModeledExceptionsMiddleware or some other common location.
There was a problem hiding this comment.
Yes, it's true. I'm a recovering over-abstractioner. I like the rule of thee here to inform when abstractions should be in DRY cases. So I kept these as is as there is only two, but if there becomes a third then I'd make a change as you suggest.
https://en.wikipedia.org/wiki/Rule_of_three_(computer_programming)
| } | ||
| // If model specifies error code use it otherwise default to 400. | ||
| // See https://awslabs.github.io/smithy/1.0/spec/core/http-traits.html#httperror-trait | ||
| val defaultCode = if (errShape.getTrait<ErrorTrait>()?.isClientError ?: error("Expected Error trait on shape $errShape")) 400 else 500 |
There was a problem hiding this comment.
nit
expectTrait already does the check
val defaultCode = if(errShape.expectTrait<ErrorTrait>().isClientError) 400 else 500| if (httpStatusCode != null) { | ||
| writer.write("register(code = #S, deserializer = $deserializerName(), httpStatusCode = $httpStatusCode)", code) | ||
| } | ||
| // If model specifies error code use it otherwise default to 400. |
There was a problem hiding this comment.
nit
comment is only partially right
If model specifies error code use it otherwise default to 400/500
kggilmer
force-pushed
the
fix-http-error-registration
branch
from
34dbf3b to
1e8ed70
Compare
3 participants
Issue #, if available: smithy-lang/smithy-kotlin#303
Description of changes:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.