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

Correct documentation for @length and @auth traits #988

Merged
merged 6 commits into from
Nov 23, 2021

Conversation

crossleydominic
Copy link
Contributor

Issue:
A couple of instances of incorrect documentation.

Description of changes:

  1. Fixes the trait selector for @length documentation to match what is currently defined in the prelude.smithy.
  2. Fixes the incorrect example for @auth traits and how effective authentication schemes are determined. This documentation has also been expanded to make it clearer. These examples can be verified correct by applying the following patch:
diff --git a/smithy-model/src/test/java/software/amazon/smithy/model/knowledge/ServiceIndexTest.java b/smithy-model/src/test/java/software/amazon/smithy/model/knowledge/ServiceIndexTest.java
index c36e335eb..a7a50748f 100644
--- a/smithy-model/src/test/java/software/amazon/smithy/model/knowledge/ServiceIndexTest.java
+++ b/smithy-model/src/test/java/software/amazon/smithy/model/knowledge/ServiceIndexTest.java
@@ -102,7 +102,7 @@ public class ServiceIndexTest {
         ServiceIndex serviceIndex = ServiceIndex.of(model);
         Map<ShapeId, Trait> auth = serviceIndex.getEffectiveAuthSchemes(
                 ShapeId.from("smithy.example#ServiceWithNoAuthTrait"),
-                ShapeId.from("smithy.example#OperationWithNoAuthTrait"));
+                ShapeId.from("smithy.example#OperationA"));
 
         assertThat(auth.keySet(), hasSize(3));
         assertThat(auth, hasKey(HttpBasicAuthTrait.ID));
@@ -115,7 +115,7 @@ public class ServiceIndexTest {
         ServiceIndex serviceIndex = ServiceIndex.of(model);
         Map<ShapeId, Trait> auth = serviceIndex.getEffectiveAuthSchemes(
                 ShapeId.from("smithy.example#ServiceWithAuthTrait"),
-                ShapeId.from("smithy.example#OperationWithNoAuthTrait"));
+                ShapeId.from("smithy.example#OperationC"));
 
         assertThat(auth.keySet(), hasSize(2));
         assertThat(auth, hasKey(HttpBasicAuthTrait.ID));
@@ -123,11 +123,22 @@ public class ServiceIndexTest {
     }
 
     @Test
-    public void getsAuthSchemesOfOperationWithAuthTrait() {
+    public void getsAuthSchemesOfOperationWithAuthTraitAndServiceWithAuthTrait() {
         ServiceIndex serviceIndex = ServiceIndex.of(model);
         Map<ShapeId, Trait> auth = serviceIndex.getEffectiveAuthSchemes(
                 ShapeId.from("smithy.example#ServiceWithAuthTrait"),
-                ShapeId.from("smithy.example#OperationWithAuthTrait"));
+                ShapeId.from("smithy.example#OperationD"));
+
+        assertThat(auth.keySet(), hasSize(1));
+        assertThat(auth, hasKey(HttpBearerAuthTrait.ID));
+    }
+
+    @Test
+    public void getsAuthSchemesOfOperationWithAuthTraitAndServiceWithNoAuthTrait() {
+        ServiceIndex serviceIndex = ServiceIndex.of(model);
+        Map<ShapeId, Trait> auth = serviceIndex.getEffectiveAuthSchemes(
+                ShapeId.from("smithy.example#ServiceWithNoAuthTrait"),
+                ShapeId.from("smithy.example#OperationB"));
 
         assertThat(auth.keySet(), hasSize(1));
         assertThat(auth, hasKey(HttpDigestAuthTrait.ID));
diff --git a/smithy-model/src/test/resources/software/amazon/smithy/model/knowledge/service-index-finds-auth-schemes.smithy b/smithy-model/src/test/resources/software/amazon/smithy/model/knowledge/service-index-finds-auth-schemes.smithy
index e95bc72d3..f5e5e236a 100644
--- a/smithy-model/src/test/resources/software/amazon/smithy/model/knowledge/service-index-finds-auth-schemes.smithy
+++ b/smithy-model/src/test/resources/software/amazon/smithy/model/knowledge/service-index-finds-auth-schemes.smithy
@@ -6,11 +6,22 @@ namespace smithy.example
 service ServiceWithNoAuthTrait {
     version: "2020-01-29",
     operations: [
-        OperationWithNoAuthTrait,
-        OperationWithAuthTrait
+        OperationA,
+        OperationB
     ]
 }
 
+// This operation does not have the @auth trait and is bound to a service
+// without the @auth trait. The effective set of authentication schemes it
+// supports are: httpBasicAuth, httpDigestAuth and httpBearerAuth
+operation OperationA {}
+
+// This operation does have the @auth trait and is bound to a service
+// without the @auth trait. The effective set of authentication schemes it
+// supports are: httpDigestAuth.
+@auth([httpDigestAuth])
+operation OperationB {}
+
 @httpBasicAuth
 @httpDigestAuth
 @httpBearerAuth
@@ -18,12 +29,18 @@ service ServiceWithNoAuthTrait {
 service ServiceWithAuthTrait {
     version: "2020-01-29",
     operations: [
-        OperationWithNoAuthTrait,
-        OperationWithAuthTrait
+        OperationC,
+        OperationD
     ]
 }
 
-operation OperationWithNoAuthTrait {}
+// This operation does not have the @auth trait and is bound to a service
+// with the @auth trait. The effective set of authentication schemes it
+// supports are: httpBasicAuth, httpDigestAuth
+operation OperationC {}
 
-@auth([httpDigestAuth])
-operation OperationWithAuthTrait {}
+// This operation not have the @auth trait and is bound to a service
+// with the @auth trait. The effective set of authentication schemes it
+// supports are: httpBearerAuth
+@auth([httpBearerAuth])
+operation OperationD {}

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

@crossleydominic crossleydominic requested a review from a team as a code owner November 18, 2021 08:43
Copy link
Member

@mtdowling mtdowling left a comment

Choose a reason for hiding this comment

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

Thanks for submitting this! I just noticed a few typos, but looks great.

docs/source/1.0/spec/core/auth-traits.rst Outdated Show resolved Hide resolved
docs/source/1.0/spec/core/auth-traits.rst Outdated Show resolved Hide resolved
docs/source/1.0/spec/core/auth-traits.rst Outdated Show resolved Hide resolved
docs/source/1.0/spec/core/auth-traits.rst Outdated Show resolved Hide resolved
docs/source/1.0/spec/core/auth-traits.rst Outdated Show resolved Hide resolved
crossleydominic and others added 4 commits November 22, 2021 13:51
Co-authored-by: Michael Dowling <m@mtdowling.com>
Co-authored-by: Michael Dowling <m@mtdowling.com>
Co-authored-by: Michael Dowling <m@mtdowling.com>
@crossleydominic
Copy link
Contributor Author

All the suggested changes have been made. Thanks.

@JordonPhillips JordonPhillips merged commit 54659b3 into smithy-lang:main Nov 23, 2021
@JordonPhillips
Copy link
Contributor

Thanks for helping out!

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.

4 participants