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

add header validator #29

Merged
merged 4 commits into from
Jan 6, 2022
Merged

add header validator #29

merged 4 commits into from
Jan 6, 2022

Conversation

lewisjkl
Copy link
Contributor

@lewisjkl lewisjkl commented Jan 5, 2022

Adds validator to make sure headers cannot be set if they are automatically set down the line.

Closes #28.

@lewisjkl lewisjkl force-pushed the add-header-validator branch from d21a962 to 7c6f900 Compare January 5, 2022 23:04
return model.getShapesWithTrait(HttpHeaderTrait.class).stream().flatMap(headerShape -> {
String value = headerShape.getTrait(HttpHeaderTrait.class).get().getValue();
if (disallowedHeaderNames.contains(value)) {
return Stream.of(error(headerShape, String.format("Header named `%s` should not be present since it is automatically filled", value)));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should mention the @mediaType trait here

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure, tbh. Like, the simpleRestJson protocol doesn't take it into consideration

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, error is too hard a severity. It might be that someone provides an implementation that allows for it, so
the error severity should only be used if we check that the header is in the closure of a simpleRestJson service.

If we don't perform that check, it should be a warning, not an error

public List<ValidationEvent> validate(Model model) {
return model.getShapesWithTrait(HttpHeaderTrait.class).stream().flatMap(headerShape -> {
String value = headerShape.getTrait(HttpHeaderTrait.class).get().getValue();
if (disallowedHeaderNames.contains(value)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Headers are case insensitive, so this doesn't quite cut it

@lewisjkl lewisjkl requested a review from Baccata January 6, 2022 17:51
@Baccata Baccata merged commit df0d14a into main Jan 6, 2022
@Baccata Baccata deleted the add-header-validator branch January 6, 2022 19:43
Baccata added a commit that referenced this pull request May 10, 2022
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

Successfully merging this pull request may close these issues.

Ensure headers referenced in smithy specs are actually settable
3 participants