URI Pattern Validation and Conflict Resolution design doc

[sugmanue]

Issue #, if available: #1512 #1029

Description of changes:

Design proposal to change the way we validate URI patterns to allow conflicting ones while also specifying how the servers must route ambiguous requests.

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

[kubukoz]

I think this addresses the problem well, thank you! I have no questions so far, just a couple spelling change suggestions.

[guymguym]

hi @sugmanue this looks good, but I just wonder - wouldn't it be simple enough to assign an optional numeric priority to each conflicting route, so that api owners could solve any issue with the order of their routes. I would just say that the default priority is 0 if not provided explicitly, and only if there is a collision on the same route and priority, then the api is invalid...

[kubukoz]

I feel like doing priority with numbers is like good old z-index in CSS - at some point you're going to use the number 999 and regret this decision 😅

having well-defined rules seems more principled.

[sugmanue]

having well-defined rules seems more principled.

+1 on that, and also opens up the possibility of subtle ways of introducing API breaking modes by changing this value.

[guymguym]

I respect that you want to define rules, everyone loves rules, but with that approach I would just be worried that some api's will not be able to use it unless they add an ?x-id=OPNAME query param to disambiguate between operations. See in S3 for example - smithy-lang/smithy-rs#1012.

The question is if you prefer to have api writers use workarounds such as x-id=OPNAME - which is also not a backward compatible addition because older clients will not send it.

[guymguym]

@sugmanue , @kubukoz , thank you for the replies!

The point I still wanted to make is that using rules that reside "outside" the api definition, in a meta schema, is not going to be more backward compatible because changes to the rules could be implemented at different times for clients vs servers, which will introduce incompatibility. Adding the routing order as information inside the api definition is actually more stable, and means the api definition is self-contained and described the api behavior as a configuration. So if I keep the api definition in my sources I can track the changes of routing as well. Which I see as a better api design than externalizing the routing as rules.

That said, I understand that a priority could sounds too simple and therefore not sophisticated enough, although I think that such simplicity is usually a good sign, and while z-index is criticized it still provides a pragmatic solution to those non common collision cases.

Two others ways to define such an order:

1. rely on the order of operations in the service.operations list itself.
2. a colliding operation could define an http binding property like routeAfter=OPNAME that would mean that this operation will always be after the other OPNAME and allow it to match first.

Thanks. Guy

[mtdowling]

This looks great, Manuel!

I looked at the S3 Smithy model, and I think all of it can be represented by this proposal except for CopyObject which appears to use headers for routing, and assigning number based priorities to routes wouldn't help here. IMO, header-based routing wouldn't be worth the complexity nor something we'd want to proliferate.

Smithy validation errors without x-id

ERROR: com.amazonaws.s3#AbortMultipartUpload (HttpUriConflict)
     @ /Users/dowling/projects/aws-sdk-js-v3/codegen/sdk-codegen/aws-models/s3.json
     |
  49 |     "com.amazonaws.s3#AbortMultipartUpload": {
     |                                              ^
     = Operation URI, `/{Bucket}/{Key+}`, conflicts with other operation URIs in the same service: [`com.amazonaws.s3#DeleteObject` (/{Bucket}/{Key+})]

ERROR: com.amazonaws.s3#CopyObject (HttpUriConflict)
     @ /Users/dowling/projects/aws-sdk-js-v3/codegen/sdk-codegen/aws-models/s3.json
     |
1581 |     "com.amazonaws.s3#CopyObject": {
     |                                    ^
     = Operation URI, `/{Bucket}/{Key+}`, conflicts with other operation URIs in the same service: [`com.amazonaws.s3#PutObject` (/{Bucket}/{Key+}), `com.amazonaws.s3#UploadPartCopy` (/{Bucket}/{Key+}), `com.amazonaws.s3#UploadPart` (/{Bucket}/{Key+})]

ERROR: com.amazonaws.s3#DeleteObject (HttpUriConflict)
     @ /Users/dowling/projects/aws-sdk-js-v3/codegen/sdk-codegen/aws-models/s3.json
     |
3168 |     "com.amazonaws.s3#DeleteObject": {
     |                                      ^
     = Operation URI, `/{Bucket}/{Key+}`, conflicts with other operation URIs in the same service: [`com.amazonaws.s3#AbortMultipartUpload` (/{Bucket}/{Key+})]

ERROR: com.amazonaws.s3#GetObject (HttpUriConflict)
     @ /Users/dowling/projects/aws-sdk-js-v3/codegen/sdk-codegen/aws-models/s3.json
     |
4995 |     "com.amazonaws.s3#GetObject": {
     |                                   ^
     = Operation URI, `/{Bucket}/{Key+}`, conflicts with other operation URIs in the same service: [`com.amazonaws.s3#ListParts` (/{Bucket}/{Key+})]

ERROR: com.amazonaws.s3#ListParts (HttpUriConflict)
     @ /Users/dowling/projects/aws-sdk-js-v3/codegen/sdk-codegen/aws-models/s3.json
     |
8504 |     "com.amazonaws.s3#ListParts": {
     |                                   ^
     = Operation URI, `/{Bucket}/{Key+}`, conflicts with other operation URIs in the same service: [`com.amazonaws.s3#GetObject` (/{Bucket}/{Key+})]

ERROR: com.amazonaws.s3#PutObject (HttpUriConflict)
     @ /Users/dowling/projects/aws-sdk-js-v3/codegen/sdk-codegen/aws-models/s3.json
     |
11121 |     "com.amazonaws.s3#PutObject": {
     |                                   ^
     = Operation URI, `/{Bucket}/{Key+}`, conflicts with other operation URIs in the same service: [`com.amazonaws.s3#CopyObject` (/{Bucket}/{Key+}), `com.amazonaws.s3#UploadPartCopy` (/{Bucket}/{Key+}), `com.amazonaws.s3#UploadPart` (/{Bucket}/{Key+})]

ERROR: com.amazonaws.s3#UploadPart (HttpUriConflict)
     @ /Users/dowling/projects/aws-sdk-js-v3/codegen/sdk-codegen/aws-models/s3.json
     |
13681 |     "com.amazonaws.s3#UploadPart": {
     |                                    ^
     = Operation URI, `/{Bucket}/{Key+}`, conflicts with other operation URIs in the same service: [`com.amazonaws.s3#CopyObject` (/{Bucket}/{Key+}), `com.amazonaws.s3#PutObject` (/{Bucket}/{Key+}), `com.amazonaws.s3#UploadPartCopy` (/{Bucket}/{Key+})]

ERROR: com.amazonaws.s3#UploadPartCopy (HttpUriConflict)
     @ /Users/dowling/projects/aws-sdk-js-v3/codegen/sdk-codegen/aws-models/s3.json
     |
13701 |     "com.amazonaws.s3#UploadPartCopy": {
     |                                        ^
     = Operation URI, `/{Bucket}/{Key+}`, conflicts with other operation URIs in the same service: [`com.amazonaws.s3#CopyObject` (/{Bucket}/{Key+}), `com.amazonaws.s3#PutObject` (/{Bucket}/{Key+}), `com.amazonaws.s3#UploadPart` (/{Bucket}/{Key+})]

The thing we'd need to make more explicitly supported is placing dynamic query strings in the URI in addition to input parameters. This would remove the x-id thing for everything in the S3 model except CopyObject (TBD if/how there's a different solution for CopyObject, e.g., supporting x-amz-copy-source in the query string too). Here's a disambiguation example we could use instead of x-id.

AbortMultipartUpload: DELETE /{Bucket}/{Key+}?uploadId
DeleteObject:         DELETE /{Bucket}/{Key+}

Smithy validation allows this right now, but it's currently undefined behavior. It would be nice to add statements around if/how this works in the proposal.

[guymguym]

@mtdowling you can see the summary of unsupported s3 routing cases in this comment -
smithy-lang/smithy-rs#1012 (comment)

for example in the case you mentioned about ?uploadId we cannot match the route based on a query param with variable input value? Here is the longer description -

These cases were added because a required query param cannot be specified in the route URI when its value is an actual input field (like ?uploadId=123 or ?partNumber=456), and not a fixed api value (like ?list-type=2). In the smithy spec you cannot specify just a key like ?uploadId in the uri to separate those cases because that's just like ?uploadId= and will fail to route and a value is provided. Maybe the spec can allow it with ?uploadId=* ?

[mtdowling]

@guymguym: The way I think it could (and maybe even already) works is that servers would route based on static query string parameters regardless of if they have a value (e.g., ?acl vs ?acl=true). So in the ?uploadId example, they could do the routing based on the presence of the query string parameter, and later, after an operation is matched, deserialize based on the query string binding of a member. We'd only ever want to do this for required input parameters since it would be used for routing. We'd also need to ensure that clients are able to handle this case.

[zeyad001]

Any update on this?

[sugmanue]

@zeyad001 I been busy working on something else, sorry about that. I will update the document and post a updated revision soon and move it from there.