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

Change the HTTP bindings validation to support any non-conflicting URI #2220

Merged
merged 3 commits into from
Apr 9, 2024

Conversation

sugmanue
Copy link
Contributor

Background

Changes the Smithy validation of the HTTP bindings to allow URI patterns that before were considered in conflict. In particular the following constraint was removed

A label and a literal SHOULD NOT both occupy the same segment in patterns which are equivalent to that point if they share a host.

And the docs changed as

A label and a literal MAY both occupy the same segment in patterns that are equivalent to that point if they share a host. Server implementations MUST route ambiguous requests to the operation with the most specific URI pattern

This feature will allow customers the use of URI path patterns that are not ambiguous but that are not currently supported, such as:

  • /abc/{xyz} and
  • /abc/{xyz}/cde

and also those that can be ambiguous such as

  • /abc/bcd/{xyz}
  • /abc/{xyz}/cde

Servers are expected to use Specificity Routing to route request that might be ambiguous at runtime as explained in the design doc "URI Pattern Validation and Conflict Resolution".

Testing

  • Unit tests

Links

  • Links to additional context, if necessary
  • Issue #, if applicable (see here for a list of keywords to use for linking issues)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@sugmanue sugmanue requested a review from a team as a code owner March 28, 2024 21:14
@sugmanue sugmanue requested a review from syall March 28, 2024 21:14
Copy link
Contributor

@syall syall left a comment

Choose a reason for hiding this comment

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

Only had a question about the greedy label validator.

Also for the greedy label test files, "labels" is spelled as "lables" in the file names.

docs/source-2.0/spec/http-bindings.rst Show resolved Hide resolved
]
}

@http(code: 200, method: "GET", uri: "/example/{greedyOne+}/bar/{greedyTwo+}")
Copy link
Contributor

Choose a reason for hiding this comment

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

Should greedyOne+ emit a "greedy label should be the last label" event?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It doesn't as the logic is written,

if (segments.get(j).isGreedyLabel()) {
    events.add(danger(shape, trait,
                      "At most one greedy label segment may exist in a pattern: " + pattern));
} else if (segments.get(j).isLabel()) {
    events.add(danger(shape, trait,
                      "A greedy label must be the last label in its pattern: " + pattern));
}

It could if we convert the else if into an if by itself, but that will emit two events for this case, I feel like one would be better as the second is implied by the first.

Copy link
Contributor

Choose a reason for hiding this comment

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

IMO I think it’ll help to over-emit events since these are events that could be suppressed.

The best case is to force users to explicitly suppress events per extra greedy label and per greedy label that’s not last, but here it’s “blanketed” into one event.

@sugmanue
Copy link
Contributor Author

sugmanue commented Apr 9, 2024

Also for the greedy label test files, "labels" is spelled as "lables" in the file names.

🤦 good catch, I will fix it in the next revision.

@sugmanue sugmanue merged commit b3ee7fa into main Apr 9, 2024
14 checks passed
@sugmanue sugmanue deleted the sugmanue/specificity-routing branch April 9, 2024 17:11
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.

2 participants