Allow expected content-type to ignore parameters #3576
Conversation
djedward
force-pushed
the
main-contenttype
branch
3 times, most recently
from
109da20
to
fdf218b
Compare
CHANGELOG.next.toml
Outdated
| @@ -10,6 +10,15 @@ | |||
| # references = ["smithy-rs#920"] | |||
| # meta = { "breaking" = false, "tada" = false, "bug" = false, "target" = "client | server | all"} | |||
| # author = "rcoh" | |||
|
|
|||
| [[aws-sdk-rust]] | |||
There was a problem hiding this comment.
This should be [[smithy-rs]]. Could you please also add "target" = "server" to the meta tags. The change log tool will automatically list changes for the server under a separate heading in the release CHANGELOG.md.
Thanks.
There was a problem hiding this comment.
ah, I missed that. Should have picked up on it when adding the "target" had failed validation.
There was a problem hiding this comment.
We should add a protocol test for each of Smithy's protocols, given that content_type_header_classifier affects all protocols (i.e. Content-Type header comparison will now only take into account essence strings in all protocols).
We should contribute the test upstream to https://github.com/smithy-lang/smithy so all code generators benefit; to test it works against smithy-rs you can add it to codegen-core/common-test-models/rest-json-extras.smithy and confirm it passes locally with ./gradlew codegen-server-test:build -P modules='rest_json_extras'.
CHANGELOG.next.toml
Outdated
| message = """ | ||
| Content-Type header validation now ignores parameter portion of media types. | ||
| """ | ||
| references = ["smithy-rs#3576"] |
There was a problem hiding this comment.
I've added this and the tests requested above. I would still need to upstream to smithy-rs, but figured I'd get them here first and these can be the model for upstream.
| @@ -257,6 +257,13 @@ mod tests { | |||
| assert!(matches!(result.unwrap_err(), MissingContentTypeReason::ToStrError(_))); | |||
| } | |||
|
|
|||
| #[test] | |||
| fn valid_params_content_type() { | |||
There was a problem hiding this comment.
nit:
| fn valid_params_content_type() { | |
| fn valid_content_type_header_classifier_params() { |
just to match the naming pattern of the other tests that also contain the name of the function being tested
There was a problem hiding this comment.
I matched it to the ones above that were doing something_something_content_type, but happy to rename.
| @@ -107,7 +107,7 @@ fn content_type_header_classifier( | |||
| .parse::<mime::Mime>() | |||
| // `expected_content_type` comes from the codegen. | |||
| .expect("BUG: MIME parsing failed, `expected_content_type` is not valid. Please file a bug report under https://github.com/smithy-lang/smithy-rs/issues"); | |||
| if expected_content_type != found_mime { | |||
| if expected_content_type != found_mime && expected_content_type != found_mime.essence_str() { | |||
There was a problem hiding this comment.
Nice, I didn't know about essence_str(). Can't we simplify this to just the latter condition though?
| if expected_content_type != found_mime && expected_content_type != found_mime.essence_str() { | |
| if expected_content_type != found_mime.essence_str() { |
We control expected_content_type and it's a &str with just the essence string. So if the latter pair are not equal the first pair should also not be equal.
There was a problem hiding this comment.
I did that originally, but was wondering if someone would ever want to force a specific check. It seemed unlikely, but added the original check back in, just in case. Given the discussion on it today, I think just the essence_str check is enough.
djedward
force-pushed
the
main-contenttype
branch
from
fdf218b
to
f16c3f2
Compare
djedward
force-pushed
the
main-contenttype
branch
from
f16c3f2
to
6822765
Compare
Co-authored-by: david-perez <d@vidp.dev>
djedward
force-pushed
the
main-contenttype
branch
from
78576c5
to
cd41c2d
Compare
|
I've written the upstream tests. I just need to reach out to the smithy team to figure out how the validate those tests across the implementations. Edit: Discussed the process. Will get a PR for the tests and update this PR accordingly. |
Motivation and Context
An issue was raised about a mobile client that appends "; charset=utf-8" to the Content-Type when using restJson1. The latest RFC for "application/json" does not register a charset parameter, but indicates it is reasonable to accept it.
Description
This change loosens the validation of the expected content type to allow all parameters. I figured if the type and subtype match, we can leave it up to the parsing of the data to figure out whether it can read it. it is just as easy for a client to specify "application/json" and then provide invalid json data.
Testing
Checklist
CHANGELOG.next.tomlif I made changes to the smithy-rs codegen or runtime cratesCHANGELOG.next.tomlif I made changes to the AWS SDK, generated SDK code, or SDK runtime cratesBy submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.