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

Make protocol and auth traits #273

Merged
merged 20 commits into from
Feb 8, 2020
Merged

Make protocol and auth traits #273

merged 20 commits into from
Feb 8, 2020

Conversation

mtdowling
Copy link
Member

This PR converts protocol and auth definitinions into meta-traits that are attached to other traits. This change leans into the idea of "traits are just shapes" and takes that a step further by making auth and protocol applications into normal traits. This removes the need for a custom model layer that we had previously which required things like custom Smithy build transforms and linters.

Followup PRs will updates documentation and further improve JSON schema and API Gateway integrations.

This commit starts the process of converting protocol and auth
definitinions into meta-traits that are attached to other traits. This
change leans into the idea of "traits are just shapes" and takes that a
step further by making auth and protocol applications into normal
traits. This removes the need for a custom model layer that we had
previously which required things like custom Smithy build transforms and
linters.
These were needed when protocol and auth applications weren't real
traits. Now that they're traits, they can be added and removed just like
any other trait.
The auth and protocol settings for protocol tests now use shape IDs with
idRef traits rather than raw strings. This provides more validation of
protocol test trait definitions.
OpenAPI conversion now uses the protocol auth traits. Further, Context
and security scheme converters are now generic over the protocol/auth
trait they convert, making them easier to implement.
This should really just be a known customization to implement when
writing an S3 client. It isn't really something I think any other team
would ever repeat.
@@ -20,4 +20,5 @@
<suppressions>
<suppress checks="EqualsHashCode" files="shapes/*"/>
<suppress checks="InnerAssignment" files="StringUtils"/>
<suppress checks="TypeName" files="AwsJson1_0Trait.java|AwsJson1_1Trait.java"/>
Copy link
Member Author

Choose a reason for hiding this comment

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

These names are flagged as not meeting Java conventions, but we need to carry the version numbers in the generated class.

@@ -437,54 +437,6 @@ orphaned shapes.
This transformer does not remove shapes from the prelude.


.. _includeAuth-transform:
Copy link
Member Author

Choose a reason for hiding this comment

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

These are no longer needed since they're just normal traits. You can include/exclude traits using includeTraits, excludeTraits, etc

@@ -114,7 +114,7 @@ The Smithy Gradle plugin relies on a ``smithy-build.json`` file found at the
root of a project to define the actual process of building the OpenAPI
specification. The following example defines a ``smithy-build.json`` file
that builds an OpenAPI specification from a service for the
``smithy.example#Weather`` service using the ``aws.rest-json-1.1`` protocol:
``smithy.example#Weather`` service using the ``aws.protocols#restJson1`` protocol:
Copy link
Member Author

Choose a reason for hiding this comment

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

We decided to use restJson1 rather than just restJson since we eventually plan to ship an improved REST+JSON protocol.

@@ -0,0 +1,125 @@
.. _aws-authentication:
Copy link
Member Author

Choose a reason for hiding this comment

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

Like protocols, I broke authentication traits into their own doc. Unlike protocols, I didn't give each trait it's own page. We could consider doing that if we perhaps add more extensive sigv4 documentation (for example, we may want to recreate the sigv4 test suite to be Smithy protocol tests).

docs/source/spec/aws-auth.rst Outdated Show resolved Hide resolved
],
"aws.protocols#restJson1": {},
"aws.auth#sigv4": {
"name": "someservice"
Copy link
Member Author

Choose a reason for hiding this comment

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

"name" is required (at least for now). This makes the sigv4 signing name much more explicit.

@@ -3,8 +3,9 @@

$version: "0.5.0"

namespace aws.protocols.tests.ec2
namespace aws.protocoltests.ec2
Copy link
Member Author

Choose a reason for hiding this comment

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

I renamed these tests to take a page from JPMS modules about split packages. It probably doesn't matter nor ever will, but just in case...

@@ -47,7 +49,7 @@
SCHEME_KEY, TYPE_KEY, AUTH_TYPE_KEY, URI_KEY, CREDENTIALS_KEY, IDENTITY_SOURCE_KEY,
IDENTITY_VALIDATION_EXPRESSION_KEY, RESULT_TTL_IN_SECONDS);

private final String scheme;
private final ShapeId scheme;
Copy link
Member Author

Choose a reason for hiding this comment

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

Authorizers now reference auth scheme shape IDs rather than arbitrary names

@@ -13,6 +13,7 @@
public void addsHttpDigestAuth() {
Model model = Model.assembler()
.addImport(getClass().getResource("http-digest-security.json"))
.discoverModels()
Copy link
Member Author

Choose a reason for hiding this comment

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

Since we're using dependencies now in tests, we need to use model discovery.

@@ -0,0 +1,103 @@
.. _aws-json-protocols:
Copy link
Contributor

Choose a reason for hiding this comment

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

For now could you at least link to the accompanying protocol tests? Maybe in a see-also type section

docs/source/spec/core.rst Outdated Show resolved Hide resolved
docs/source/spec/core.rst Show resolved Hide resolved
docs/source/spec/core.rst Show resolved Hide resolved
@mtdowling mtdowling merged commit 1e14051 into 0.10 Feb 8, 2020
@mtdowling mtdowling deleted the meta-protocol-and-auth branch February 8, 2020 19:29
@kstich kstich mentioned this pull request Apr 23, 2020
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