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

Disambiguate AWS paths with fragment parameters #11

Open
wants to merge 6 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@pimterry
Copy link
Contributor

pimterry commented Mar 14, 2019

This fixes #2.

This PR is pretty complicated and does a few things:

  • Changes the fragment used in AWS paths to make the paths routable by examining parameters
  • Changes the parameter parsing and output so we can find the relevant parameters
  • Adds deprecation info for operations
  • Adds proper externalDocs links to operation docs, where available (only S3)
  • Includes the correct produces/consumes for query specs using XML (previously this was blank)

It's in separate commits, which should help with reviewing!

Path fragments

Right now, all ambiguous paths are named with #{operationId}, e.g. /{bucket}#DeleteBucket for the S3 delete bucket operation. In this format, it's impossible to look at a given request and automatically tell OpenAPI operation it's performing.

This PR changes this so that every path includes a fragment that specifies the required parameters of an operation, which can be used to match it when the path is ambiguous. For example, /{Bucket}/{Key}#select&select-type=2, which matches requests where a select parameter is present (with any or an empty value) and the select-type parameter is set to 2. These can overlap, so the most specific matching path should be used.

Fragment parameters may be query parameters, or header parameters. The names match the name of the parameter in the corresponding operation, regardless of location.

In some specs, near-identical endpoints are defined repeatedly, for the current & deprecated versions of the same operation. In that case, we add a deprecated! parameter to the fragment for the deprecated version, so they don't conflict. That means routing etc will always find the current one, but anybody sending requests, or browsing the spec can still see & use both.

Some example paths from the generated S3 spec:

  • PUT /{Bucket}/{Key} - adds an object to a bucket
  • PUT /{Bucket}/{Key}#x-amz-copy-source - copies an object into a bucket, specifying the source bucket in the x-amz-copy-source header param
  • GET /{Bucket}#lifecycle - get the lifecycle of a bucket
  • GET /{Bucket}#lifecycle&deprecated! - a now-deprecated & replaced endpoint for getting the lifecycle of a bucket

Param parsing

This isn't enough. The current implementation doesn't actually create parameters for most parameters in the AWS data, so we can't easily tell whether a given parameter in a fragment is a header or query parameter, or anything else about it.

Instead, right now every operation with parameters is given a single 'body' parameter, which includes every other specified parameter. For 'json' protocol services that's broadly correct, but it's not correct for any others, which are the vast majority.

This PR implements most of the per-protocol logic to define more-or-less correct parameters for each operation. This makes fragment routing possible, gives us better parameter data (including param descriptions etc), and makes the specs much more accurate.

This is close, but doesn't work 100%. For all primitive typed params, I believe it's correct. For object & array params, it simplifies things drastically. Nested object parameters are definitely not included, and I suspect others aren't quite right. Nonetheless, it's much closer than the current state. To take this further, I think we'd want to go to OpenAPI 3 (so we can use full JSON schema for params), and refactor the overall structure a fair bit (in short: I'd build the tree top-down, rather than trying to change it in place as in transformShape, which is super hard as it misses a lot of necessary context at each step).

A params comparison, to make this clearer:

  • An RDS endpoint with the current spec:

Screenshot from 2019-03-14 14-26-10

  • An RDS endpoint with the new spec:

Screenshot from 2019-03-14 14-27-11

Should be easy to run and check the diff locally, but to save time here's a couple of examples:
AWS-examples.zip

@MikeRalphson

This comment has been minimized.

Copy link
Contributor

MikeRalphson commented Mar 15, 2019

Can we drop the ! from deprecated!? I always find when it is used like this it's really unclear whether the ! is an affirmation or a negation.

@pimterry pimterry force-pushed the pimterry:path-disambiguation branch from 2ce78f9 to 56ad257 Mar 15, 2019

@pimterry

This comment has been minimized.

Copy link
Contributor Author

pimterry commented Mar 15, 2019

Sure, done.

It was there to try and make it clearer that it's not just another query parameter, but it's not strictly necessary at all.

@MikeRalphson

This comment has been minimized.

Copy link
Contributor

MikeRalphson commented Mar 15, 2019

I've put a shout out on the openapi-directory gitter channel to see if anyone has a dependency on the current fragment format or thoughts generally on the PR (though I'm not really expecting much feedback).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.