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

Non-2xx HTTP response code emits a DANGER validation error #1386

Closed
jdisanti opened this issue Sep 7, 2022 · 8 comments · Fixed by #1962
Closed

Non-2xx HTTP response code emits a DANGER validation error #1386

jdisanti opened this issue Sep 7, 2022 · 8 comments · Fixed by #1962
Labels
documentation This is a problem with documentation. guidance Question that needs advice or information.

Comments

@jdisanti
Copy link
Contributor

jdisanti commented Sep 7, 2022

The spec for the @http trait states that for the code:

The provided value SHOULD be between 100 and 599, and it MUST be between 100 and 999.

However, when providing a non-2xx value for code, such as 418 (why not?), the Smithy validation emits a DANGER message:

[DANGER] aws.protocoltests.misc#ResponseCodeRequiredOperation: Expected an `http` code in the 2xx range, but found 418 | HttpResponseCodeSemantics

Which is correct? The spec or the error?

@mtdowling
Copy link
Member

mtdowling commented Oct 17, 2022

It seems fair for the reference implementation as a tool to provide reasonable validation like this. We could add a note to the spec that the status code SHOULD be in the 2xx range. Returning a successful response outside this range isn't properly adhering to the semantics of HTTP, which is kind of the point of the REST HTTP bindings for Smithy.

@yisraelU
Copy link

To pick up on this topic,
why is choosing the Http trait considered choosing REST?

@mtdowling
Copy link
Member

Sure, I should have said HTTP and not REST. Smithy's HTTP bindings do control how requests and responses are serialized over HTTP. There is plenty of guidance in the HTTP spec for what is and isn't a successful status code. The@http trait on an operation is meant to capture the status code of a successful response.

@yisraelU
Copy link

Ok , so if our focus is purely HTTP, the 300 range carries with it quite a few success responses that do not prompt further action for example statuses used in Oauth flows

@mtdowling
Copy link
Member

Do you have a concrete example? OAuth isn't something you'd typically want to model directly in Smithy but rather you'd abstract over it with an auth trait.

@yisraelU
Copy link

No i dont have any other cases like that .
I see what you are saying

@hambrosia
Copy link

I'm curious about how the redirect-based SAML flow is typically documented with Smithy.
For example as shown here:

https://en.wikipedia.org/wiki/SAML_2.0#/media/File:Saml2-browser-sso-redirect-post.png

Let's say a service has a /login endpoint that uses SAML with an an external identity provider. The service provider, or application that the user is interacting with, will redirect the user several times to the identity provider, and also maintain an endpoint to handle call backs from the identity provider.

Are there examples of using the auth trait to abstract over this?

@mtdowling
Copy link
Member

We don't have documentation specifically for that. I would not try to model those redirects in a Smithy model. They aren't something your users call directly or should have to contend with. It's something generated clients should handle on their behalf. It would be an implementation detail of a hypothetical SAML SSO redirect auth trait.

Putting the developer experience aside -- you could try to model these things if you wanted to, you just need to suppress the validation. It's not a blocker, but a nudge. If someone were trying to model SAML or something directly, and they had to stop and think about whether they should because of the validation, then I think the validator is working as intended.

@kstich kstich added the documentation This is a problem with documentation. label Aug 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation This is a problem with documentation. guidance Question that needs advice or information.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants