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

"json" protocol v1.0 #29

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

milt
Copy link

@milt milt commented Jun 22, 2018

This PR begins to address #4 by adding support for "json" protocol apis, version 1.0 only.
Confirmed working with DynamoDB and DDBStreams.

Other notable changes:

  • Gen:
    • portkey.awsgen/gen-api dispatches on protocol, then version
    • portkey.awsgen/gen-operation is a multimethod, dispatch fn looks at protocol and version if present.
    • portkey.awsgen/gen-api reports on unsupported versions.
    • portkey.awsgen/generate-files! now looks for endpoints with both the munged and unmunged name, and does not attempt to generate if they are absent, preventing a runtime NPE.
  • Client:
    • portkey.aws/*endpoint-override* is a dynamic var that can be used to override the endpoint uri for local testing, etc.
    • portkey.aws/params-toquerystring no longer puts a ? on URIs if there are no query params.
    • portkey.aws/-json-call provides request processing for json v1.0 apis with the "X-AMZ-Target" header.
    • portkey.aws/-json-call finds exceptions that only exist in the body of the response
    • POSSIBLY_BREAKING - both request functions now wrap errors going into the ex-info map in an additional map, wherein the :type key contains the spec keyword of the exception, and the body key contains the exception map (with message) itself.

Smells:

  • There's a lot of duplicated code in the method forportkey.awsgen/gen-operation for json protocol, and the -json-call request fn. This should be refactored away as we add more protocols.
  • Generation logging with versions is a little noisy, maybe some better spacing/formatting can be applied
  • Many "rest-json" protocol ns's have a jsonVersion, so it may be wise to further dispatch on those for services that don't generate or fail at runtime.

@cgrand
Copy link
Contributor

cgrand commented Jun 22, 2018

Fantastic work Milton! @dupuchba do you have time to review it since there’s overlap with your current work?

@milt
Copy link
Author

milt commented Jun 22, 2018

@cgrand thanks, I love where this project is going!
One last review note , I added two additional commits to produce slightly more useable errors (that have the spec keyword, in addition to the shape as a message), but might break downstream projects that rely on the exception map being the root of the ex-info map.

@dupuchba
Copy link
Collaborator

@milt Thanks a lot for your work !!!

I won't be able to review it today but I'll take some time tomorrow !

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.

3 participants