Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

httpResponseCode does not work with short #1115

Closed
david-perez opened this issue Mar 2, 2022 · 4 comments
Closed

httpResponseCode does not work with short #1115

david-perez opened this issue Mar 2, 2022 · 4 comments

Comments

@david-perez
Copy link
Contributor

structure AnOperationOutput {
    @httpResponseCode
    responseCode: Short
}
ERROR: com.amazonaws.simple#AnOperationOutput$responseCode (TraitTarget)
     @ /local/home/davidpz/workplace/smithy-ws/src/SmithyRsSource/codegen-server-test/model/simple.smithy
     |
  32 |     @httpResponseCode
     |                      ^
     = Trait `httpResponseCode` cannot be applied to `com.amazonaws.simple#AnOperationOutput$responseCode`. This trait may only be applied to shapes that match the following selector: structure > member :test(> integer)

HTTP response codes are constrained to the 100-999 inclusive range, according to the RFC. However, you can only apply httpResponseCode to integer shapes.

Ideally we would have httpResponseCode work only with short.

@david-perez
Copy link
Contributor Author

What if we emitted a warning if an @httpResponseCode-bound member shape does not have @range(100-999) attached too?

@mtdowling
Copy link
Member

I think continuing to limit httpResponseCode to integer is a net positive because it removes additional verification implementations need to do when generating code (no need to test with each numeric variant), and integer composes well with intEnum, allowing services to make the responseCode dynamic but still limited to a subset of predefined messages. You don't have that option with short, for example. I think the range of HTTP response status codes is implicit, too. It feels like an unnecessary burden on modelers to make them reify this HTTP restriction in their model, and instead should be left to frameworks and tooling to enforce.

@david-perez
Copy link
Contributor Author

Oh, TIL about intEnum. I guess it's a new thing in IDL v2. I'm guessing there aren't any protocol tests exercising those, or does the Smithy library convert them to enum shapes when passing the Model to code-generators like smithy-rs? Otherwise, smithy-rs should be breaking if you introduce a protocol test exercising those, since there is no specific code path to handle them...

What was the rationale for introducing intEnum shapes, as opposed to just allowing enum shapes to model integer values?

and integer composes well with intEnum [...] You don't have that option with short, for example.

So is the intention that the variants of an intEnum be generated using the same underlying type than integer? Why not short? Enums usually only contain a handful of variants, and those easily fit within the range of a short.


I think the range of HTTP response status codes is implicit, too [...] and instead should be left to frameworks and tooling to enforce.

How though? I think it should be documented in the spec what happens when the response code falls out of the valid range. That's what I advocated for in #1116, but it got closed arguing that it wasn't necessary because response codes fell within the realms of the HTTP RFC. It seems like with #1254, code generators have the freedom to interpret response codes outside of the valid range prescribed in the HTTP RFC as undefined behavior? I think it'd be nicer if the Smithy spec had a provision telling clients and servers what should they do in these cases, so as to avoid discrepancies among generator implementations.

@mtdowling
Copy link
Member

Otherwise, smithy-rs should be breaking if you introduce a protocol test exercising those, since there is no specific code path to handle them...

intEnum is a subtype of integer, so implementations that don't understand it or special case them treat them as integer. If smithy-rs used the visitors, then it automatically works that way (though it should be updated before smithy-rs 1.0).

What was the rationale for introducing intEnum shapes, as opposed to just allowing enum shapes to model integer values?

intEnum is a subtype of integer. If clients receive a non-modeled integer, they need to be able to carry the value without failing to allow a service to add more values over time. If we supported more numeric types, then we'd have needed to add way more types for numeric types that are seldom used. This would have been confusing for modelers IMO and much more of a burden to implement. More details here https://github.com/awslabs/smithy/blob/main/designs/enum-shapes.md

So is the intention that the variants of an intEnum be generated using the same underlying type than integer? Why not short? Enums usually only contain a handful of variants, and those easily fit within the range of a short.

The values of intEnum aren't auto assigned or anything. They can contain any value and larger values than short.

How though?

A 500 makes sense to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants