Repository: smithy-lang/smithy (java)

Issue:
Unused offset within smithy.model.pattern.SmithyPattern.Segment.Parse
Hi, I think there's a small bug with the Segment Parser. 

It appears that the `offset` variable is currently unused within the `smithy.model.pattern.SmithyPattern.Segment.Parse` function. 
https://github.com/awslabs/smithy/blob/2e1cdc4af7e7cdab3138f5f3912a3af930b569a2/smithy-model/src/main/java/software/amazon/smithy/model/pattern/SmithyPattern.java#L276

All callers of this function also appear to be calling this `parse` function incorrectly and are exhibiting correct parsing behaviour because the `offset` variable is unused.

The call to `Segment.parse` at https://github.com/awslabs/smithy/blob/2e1cdc4af7e7cdab3138f5f3912a3af930b569a2/smithy-model/src/main/java/software/amazon/smithy/model/pattern/UriPattern.java#L80
Should always have an offset of 0 because the string is first split into chunks by `/`.

The call to `Segment.parse` at https://github.com/awslabs/smithy/blob/2e1cdc4af7e7cdab3138f5f3912a3af930b569a2/smithy-model/src/test/java/software/amazon/smithy/model/pattern/SmithyPatternTest.java#L39
Same as above, should always be 0 given that the string is chunked by '/' first.

The call to `Segment.parse` at https://github.com/awslabs/smithy/blob/2e1cdc4af7e7cdab3138f5f3912a3af930b569a2/smithy-model/src/main/java/software/amazon/smithy/model/traits/EndpointTrait.java#L66
This should also have the offset always set to 0 because the string is first tokenized into chunks.

A new test should be added to verify that the offset is used correctly.

Alternatively, the `offset` parameter could be removed but this would be a breaking API change so less desirable, up to you though.

I'm happy to submit a PR to correct the issue if the above is correct. 



Pull request:
Improve segment parsing errors with offsets
Fixes #904 

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.


Patch:
diff --git a/docs/source-1.0/spec/aws/aws-core.rst b/docs/source-1.0/spec/aws/aws-core.rst
index 81f657826..8ae2c8cf9 100644
--- a/docs/source-1.0/spec/aws/aws-core.rst
+++ b/docs/source-1.0/spec/aws/aws-core.rst
@@ -26,10 +26,12 @@ Value type
     * :ref:`service-cloudformation-name`
     * :ref:`service-arn-namespace`
     * :ref:`service-cloudtrail-event-source`
+    * :ref:`service-doc-id`
     * :ref:`service-endpoint-prefix`
 
 The following example defines an AWS service that uses the default values of
-``cloudFormationService``, ``arnNamespace``, and ``cloudTrailEventSource``:
+``cloudFormationService``, ``arnNamespace``, ``cloudTrailEventSource`` and
+``docId``:
 
 .. tabs::
 
@@ -78,6 +80,7 @@ The following example provides explicit values for all properties:
             cloudFormationName: "FooBaz",
             arnNamespace: "myservice",
             cloudTrailEventSource: "myservice.amazon.aws",
+            docId: "some-value-2018-03-17",
             endpointPrefix: "my-endpoint"
         )
         service FooBaz {
@@ -97,6 +100,7 @@ The following example provides explicit values for all properties:
                             "sdkId": "Some Value",
                             "cloudFormationName": "FooBaz",
                             "arnNamespace": "myservice",
+                            "docId": "some-value-2018-03-17",
                             "cloudTrailEventSource": "myservice.amazon.aws"
                         }
                     }
@@ -233,6 +237,31 @@ Amazon CloudWatch is ``monitoring.amazonaws.com``. Such services will
 need to explicitly configure the ``cloudTrailEventSource`` setting.
 
 
+.. _service-doc-id:
+
+``docId``
+===========================
+
+The ``docId`` property is a ``string`` value that is used to implement linking
+between service and SDK documentation for AWS services.
+
+This will default to the ``sdkId`` value in lower case followed by the service
+``version`` property, separated by dashes. For the example below, the value
+for this property would default to ``some-value-2018-03-17``.
+
+.. code-block:: smithy
+
+    $version: "1.0"
+    namespace aws.fooBaz
+
+    use aws.api#service
+
+    @service(sdkId: "Some Value")
+    service FooBaz {
+        version: "2018-03-17",
+    }
+
+
 .. _service-endpoint-prefix:
 
 ``endpointPrefix``
diff --git a/docs/source-2.0/aws/aws-core.rst b/docs/source-2.0/aws/aws-core.rst
index 5e082f881..99f6af5db 100644
--- a/docs/source-2.0/aws/aws-core.rst
+++ b/docs/source-2.0/aws/aws-core.rst
@@ -25,10 +25,12 @@ Value type
     * :ref:`service-cloudformation-name`
     * :ref:`service-arn-namespace`
     * :ref:`service-cloudtrail-event-source`
+    * :ref:`service-doc-id`
     * :ref:`service-endpoint-prefix`
 
 The following example defines an AWS service that uses the default values of
-``cloudFormationService``, ``arnNamespace``, and ``cloudTrailEventSource``:
+``cloudFormationService``, ``arnNamespace``, ``cloudTrailEventSource`` and
+``docId``:
 
 .. code-block:: smithy
 
@@ -58,6 +60,7 @@ The following example provides explicit values for all properties:
         cloudFormationName: "FooBaz"
         arnNamespace: "myservice"
         cloudTrailEventSource: "myservice.amazon.aws"
+        docId: "some-value-2018-03-17"
         endpointPrefix: "my-endpoint"
     )
     service FooBaz {
@@ -194,6 +197,32 @@ Amazon CloudWatch is ``monitoring.amazonaws.com``. Such services will
 need to explicitly configure the ``cloudTrailEventSource`` setting.
 
 
+.. _service-doc-id:
+
+``docId``
+===========================
+
+The ``docId`` property is a ``string`` value that is used to implement linking
+between service and SDK documentation for AWS services.
+
+This will default to the ``sdkId`` value in lower case followed by the service
+``version`` property, separated by dashes. For the example below, the value
+for this property would default to ``some-value-2018-03-17``.
+
+.. code-block:: smithy
+
+    $version: "2"
+
+    namespace aws.fooBaz
+
+    use aws.api#service
+
+    @service(sdkId: "Some Value")
+    service FooBaz {
+        version: "2018-03-17"
+    }
+
+
 .. _service-endpoint-prefix:
 
 ``endpointPrefix``
diff --git a/smithy-aws-traits/src/main/java/software/amazon/smithy/aws/traits/ServiceTrait.java b/smithy-aws-traits/src/main/java/software/amazon/smithy/aws/traits/ServiceTrait.java
index 705abc16f..b6f27c09a 100644
--- a/smithy-aws-traits/src/main/java/software/amazon/smithy/aws/traits/ServiceTrait.java
+++ b/smithy-aws-traits/src/main/java/software/amazon/smithy/aws/traits/ServiceTrait.java
@@ -19,10 +19,12 @@ import java.util.Locale;
 import java.util.Objects;
 import java.util.Optional;
 import java.util.logging.Logger;
+import software.amazon.smithy.model.Model;
 import software.amazon.smithy.model.SourceException;
 import software.amazon.smithy.model.node.Node;
 import software.amazon.smithy.model.node.ObjectNode;
 import software.amazon.smithy.model.node.StringNode;
+import software.amazon.smithy.model.shapes.ServiceShape;
 import software.amazon.smithy.model.shapes.ShapeId;
 import software.amazon.smithy.model.traits.AbstractTrait;
 import software.amazon.smithy.model.traits.AbstractTraitBuilder;
@@ -38,19 +40,23 @@ public final class ServiceTrait extends AbstractTrait implements ToSmithyBuilder
     public static final ShapeId ID = ShapeId.from("aws.api#service");
     private static final Logger LOGGER = Logger.getLogger(ServiceTrait.class.getName());
 
+    private final ShapeId target;
     private final String cloudFormationName;
     private final String arnNamespace;
     private final String sdkId;
     private final String cloudTrailEventSource;
+    private final String docId;
     private final String endpointPrefix;
 
     private ServiceTrait(Builder builder) {
         super(ID, builder.getSourceLocation());
+        this.target = SmithyBuilder.requiredState("target", builder.target);
         this.sdkId = SmithyBuilder.requiredState("sdkId", builder.sdkId);
         this.arnNamespace = SmithyBuilder.requiredState("arnNamespace", builder.arnNamespace);
         this.cloudFormationName = SmithyBuilder.requiredState("cloudFormationName", builder.cloudFormationName);
         this.cloudTrailEventSource = SmithyBuilder.requiredState(
                 "cloudTrailEventSource", builder.cloudTrailEventSource);
+        this.docId = builder.docId;
         this.endpointPrefix = SmithyBuilder.requiredState("endpointPrefix", builder.endpointPrefix);
     }
 
@@ -77,6 +83,8 @@ public final class ServiceTrait extends AbstractTrait implements ToSmithyBuilder
                     .ifPresent(builder::cloudFormationName);
             objectNode.getStringMember("cloudTrailEventSource").map(StringNode::getValue)
                     .ifPresent(builder::cloudTrailEventSource);
+            objectNode.getStringMember("docId").map(StringNode::getValue)
+                    .ifPresent(builder::docId);
             objectNode.getStringMember("endpointPrefix")
                     .map(StringNode::getValue)
                     .ifPresent(builder::endpointPrefix);
@@ -140,6 +148,28 @@ public final class ServiceTrait extends AbstractTrait implements ToSmithyBuilder
         return cloudTrailEventSource;
     }
 
+    /**
+     * Returns the documentation identifier value for the service.
+     *
+     * <p> When value on trait is not set, this method defaults to the lower
+     * cased value of the sdkId followed by the service version, separated by
+     * dashes.
+     *
+     * @return Returns the documentation identifier value for the service name.
+     */
+    public String getDocId(Model model) {
+        return getDocId().orElse(buildDefaultDocId(model));
+    }
+
+    protected Optional<String> getDocId() {
+        return Optional.ofNullable(docId);
+    }
+
+    private String buildDefaultDocId(Model model) {
+        String version = model.expectShape(target, ServiceShape.class).getVersion();
+        return sdkId.replace(" ", "-").toLowerCase(Locale.US) + "-" + version;
+    }
+
     /**
      * Returns the endpoint prefix for the service.
      *
@@ -162,11 +192,13 @@ public final class ServiceTrait extends AbstractTrait implements ToSmithyBuilder
     @Override
     public Builder toBuilder() {
         return new Builder()
+                .target(target)
                 .sdkId(sdkId)
                 .sourceLocation(getSourceLocation())
                 .cloudFormationName(cloudFormationName)
                 .arnNamespace(arnNamespace)
                 .cloudTrailEventSource(cloudTrailEventSource)
+                .docId(docId)
                 .endpointPrefix(endpointPrefix);
     }
 
@@ -174,10 +206,12 @@ public final class ServiceTrait extends AbstractTrait implements ToSmithyBuilder
     protected Node createNode() {
         return Node.objectNodeBuilder()
                 .sourceLocation(getSourceLocation())
+                .withMember("target", Node.from(target.getName()))
                 .withMember("sdkId", Node.from(sdkId))
                 .withMember("arnNamespace", Node.from(getArnNamespace()))
                 .withMember("cloudFormationName", Node.from(getCloudFormationName()))
                 .withMember("cloudTrailEventSource", Node.from(getCloudTrailEventSource()))
+                .withOptionalMember("docId", getDocId().map(Node::from))
                 .withMember("endpointPrefix", Node.from(getEndpointPrefix()))
                 .build();
     }
@@ -193,25 +227,31 @@ public final class ServiceTrait extends AbstractTrait implements ToSmithyBuilder
         } else {
             ServiceTrait os = (ServiceTrait) other;
             return sdkId.equals(os.sdkId)
+                    && target.equals(os.target)
                     && arnNamespace.equals(os.arnNamespace)
                     && cloudFormationName.equals(os.cloudFormationName)
                     && cloudTrailEventSource.equals(os.cloudTrailEventSource)
+                    && docId == null
+                        ? os.docId == null
+                        : docId.equals(os.docId)
                     && endpointPrefix.equals(os.endpointPrefix);
         }
     }
 
     @Override
     public int hashCode() {
-        return Objects.hash(toShapeId(), sdkId, arnNamespace, cloudFormationName,
-                            cloudTrailEventSource, endpointPrefix);
+        return Objects.hash(toShapeId(), target, sdkId, arnNamespace, cloudFormationName,
+                            cloudTrailEventSource, docId, endpointPrefix);
     }
 
     /** Builder for {@link ServiceTrait}. */
     public static final class Builder extends AbstractTraitBuilder<ServiceTrait, Builder> {
+        private ShapeId target;
         private String sdkId;
         private String cloudFormationName;
         private String arnNamespace;
         private String cloudTrailEventSource;
+        private String docId;
         private String endpointPrefix;
 
         private Builder() {}
@@ -226,6 +266,8 @@ public final class ServiceTrait extends AbstractTrait implements ToSmithyBuilder
         }
 
         public ServiceTrait build(ShapeId target) {
+            this.target = target;
+
             // Fill in default values if they weren't set.
             if (arnNamespace == null) {
                 arnNamespace(target.getName().toLowerCase(Locale.US));
@@ -236,7 +278,7 @@ public final class ServiceTrait extends AbstractTrait implements ToSmithyBuilder
             }
 
             if (cloudTrailEventSource == null) {
-                cloudTrailEventSource = arnNamespace + ".amazonaws.com";
+                cloudTrailEventSource(arnNamespace + ".amazonaws.com");
             }
 
             if (endpointPrefix == null) {
@@ -246,6 +288,17 @@ public final class ServiceTrait extends AbstractTrait implements ToSmithyBuilder
             return new ServiceTrait(this);
         }
 
+        /**
+         * Sets the target shape to which the trait is applied.
+         *
+         * @param target the ShapeId targeted by the trait.
+         * @return Returns the builder.
+         */
+        private Builder target(ShapeId target) {
+            this.target = target;
+            return this;
+        }
+
         /**
          * Sets the AWS CloudFormation resource type service name.
          *
@@ -296,6 +349,17 @@ public final class ServiceTrait extends AbstractTrait implements ToSmithyBuilder
             return this;
         }
 
+        /**
+         * Set the documentation identifier for the service.
+         *
+         * @param docId documentation identifier for the service.
+         * @return Returns the builder.
+         */
+        public Builder docId(String docId) {
+            this.docId = docId;
+            return this;
+        }
+
         /**
          * Set the endpoint prefix used to construct client endpoints.
          *
diff --git a/smithy-aws-traits/src/main/resources/META-INF/smithy/aws.api.smithy b/smithy-aws-traits/src/main/resources/META-INF/smithy/aws.api.smithy
index a48ea0d63..baf9cb499 100644
--- a/smithy-aws-traits/src/main/resources/META-INF/smithy/aws.api.smithy
+++ b/smithy-aws-traits/src/main/resources/META-INF/smithy/aws.api.smithy
@@ -186,6 +186,12 @@ structure service {
     /// to the `arnNamespace` plus `.amazonaws.com`.
     cloudTrailEventSource: String
 
+    /// The `docId` property is a string value that defines the identifier
+    /// used to implemention linking between service and SDK documentation for
+    /// AWS services. If not specified, this value defaults to the `sdkId` in
+    /// lower case plus the service `version` property, separated by dashes.
+    docId: String
+
     /// The `endpointPrefix` property is a string value that identifies which
     /// endpoint in a given region should be used to connect to the service.
     /// For example, most services in the AWS standard partition have endpoints
diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/pattern/SmithyPattern.java b/smithy-model/src/main/java/software/amazon/smithy/model/pattern/SmithyPattern.java
index f7ed50f98..82bcb7716 100644
--- a/smithy-model/src/main/java/software/amazon/smithy/model/pattern/SmithyPattern.java
+++ b/smithy-model/src/main/java/software/amazon/smithy/model/pattern/SmithyPattern.java
@@ -235,10 +235,14 @@ public class SmithyPattern {
         private final Type segmentType;
 
         public Segment(String content, Type segmentType) {
+            this(content, segmentType, null);
+        }
+
+        public Segment(String content, Type segmentType, Integer offset) {
             this.content = Objects.requireNonNull(content);
             this.segmentType = segmentType;
 
-            checkForInvalidContents();
+            checkForInvalidContents(offset);
 
             if (segmentType == Type.GREEDY_LABEL) {
                 asString = "{" + content + "+}";
@@ -249,19 +253,25 @@ public class SmithyPattern {
             }
         }
 
-        private void checkForInvalidContents() {
+        private void checkForInvalidContents(Integer offset) {
+            String offsetString = "";
+            if (offset != null) {
+                offsetString += " at index " + offset;
+            }
             if (segmentType == Type.LITERAL) {
                 if (content.isEmpty()) {
-                    throw new InvalidPatternException("Segments must not be empty");
+                    throw new InvalidPatternException("Segments must not be empty" + offsetString);
                 } else if (content.contains("{") || content.contains("}")) {
                     throw new InvalidPatternException(
-                            "Literal segments must not contain `{` or `}` characters. Found segment `" + content + "`");
+                            "Literal segments must not contain `{` or `}` characters. Found segment `"
+                                    + content + "`" + offsetString);
                 }
             } else if (content.isEmpty()) {
-                throw new InvalidPatternException("Empty label declaration in pattern.");
+                throw new InvalidPatternException("Empty label declaration in pattern" + offsetString + ".");
             } else if (!ShapeId.isValidIdentifier(content)) {
                 throw new InvalidPatternException(
-                        "Invalid label name in pattern: '" + content + "'. Labels must contain value identifiers.");
+                        "Invalid label name in pattern: '" + content + "'" + offsetString
+                                + ". Labels must contain value identifiers.");
             }
         }
 
@@ -269,7 +279,7 @@ public class SmithyPattern {
          * Parse a segment from the given offset.
          *
          * @param content Content of the segment.
-         * @param offset Character offset where the segment starts.
+         * @param offset Character offset where the segment starts in the containing pattern.
          * @return Returns the created segment.
          * @throws InvalidPatternException if the segment is invalid.
          */
@@ -279,9 +289,9 @@ public class SmithyPattern {
                 content = labelType == Type.GREEDY_LABEL
                           ? content.substring(1, content.length() - 2)
                           : content.substring(1, content.length() - 1);
-                return new Segment(content, labelType);
+                return new Segment(content, labelType, offset);
             } else {
-                return new Segment(content, Type.LITERAL);
+                return new Segment(content, Type.LITERAL, offset);
             }
         }
 
diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/pattern/UriPattern.java b/smithy-model/src/main/java/software/amazon/smithy/model/pattern/UriPattern.java
index 26be0d4fa..3df67138b 100644
--- a/smithy-model/src/main/java/software/amazon/smithy/model/pattern/UriPattern.java
+++ b/smithy-model/src/main/java/software/amazon/smithy/model/pattern/UriPattern.java
@@ -78,7 +78,8 @@ public final class UriPattern extends SmithyPattern {
         for (int i = 1; i < unparsedSegments.length; i++) {
             String segment = unparsedSegments[i];
             segments.add(Segment.parse(segment, offset));
-            offset += segment.length();
+            // Add one to account for `/`
+            offset += segment.length() + 1;
         }
 
         Map<String, String> queryLiterals = new LinkedHashMap<>();


Test patch:
diff --git a/smithy-aws-traits/src/test/java/software/amazon/smithy/aws/traits/EventSourceValidatorTest.java b/smithy-aws-traits/src/test/java/software/amazon/smithy/aws/traits/EventSourceValidatorTest.java
index 91906dce3..2515de84e 100644
--- a/smithy-aws-traits/src/test/java/software/amazon/smithy/aws/traits/EventSourceValidatorTest.java
+++ b/smithy-aws-traits/src/test/java/software/amazon/smithy/aws/traits/EventSourceValidatorTest.java
@@ -10,20 +10,22 @@ import org.junit.jupiter.api.Assertions;
 import org.junit.jupiter.api.Test;
 import software.amazon.smithy.model.Model;
 import software.amazon.smithy.model.shapes.ServiceShape;
+import software.amazon.smithy.model.shapes.ShapeId;
 import software.amazon.smithy.model.validation.Severity;
 import software.amazon.smithy.model.validation.ValidationEvent;
 
 public class EventSourceValidatorTest {
     @Test
     public void detectsWhenEventSourceIsUnexpected() {
+        ShapeId id = ShapeId.from("smithy.example#Foo");
         ServiceTrait trait = ServiceTrait.builder()
                 .sdkId("Foo")
                 .arnNamespace("foo")
                 .cloudTrailEventSource("REPLACE_ME_LATER")
                 .cloudFormationName("AWS::Foo")
-                .build();
+                .build(id);
         ServiceShape service = ServiceShape.builder()
-                .id("smithy.example#Foo")
+                .id(id)
                 .version("123")
                 .addTrait(trait)
                 .build();
@@ -39,14 +41,15 @@ public class EventSourceValidatorTest {
 
     @Test
     public void detectsWhenEventSourceIsPlaceholder() {
+        ShapeId id = ShapeId.from("smithy.example#Foo");
         ServiceTrait trait = ServiceTrait.builder()
                 .sdkId("Foo")
                 .arnNamespace("foo")
                 .cloudTrailEventSource("notfoo.amazonaws.com")
                 .cloudFormationName("AWS::Foo")
-                .build();
+                .build(id);
         ServiceShape service = ServiceShape.builder()
-                .id("smithy.example#Foo")
+                .id(id)
                 .version("123")
                 .addTrait(trait)
                 .build();
@@ -63,14 +66,15 @@ public class EventSourceValidatorTest {
 
     @Test
     public void ignoresKnownExceptions() {
+        ShapeId id = ShapeId.from("smithy.example#Foo");
         ServiceTrait trait = ServiceTrait.builder()
                 .sdkId("Foo")
                 .arnNamespace("cloudwatch")
                 .cloudTrailEventSource("monitoring.amazonaws.com")
                 .cloudFormationName("AWS::Foo")
-                .build();
+                .build(id);
         ServiceShape service = ServiceShape.builder()
-                .id("smithy.example#Foo")
+                .id(id)
                 .version("123")
                 .addTrait(trait)
                 .build();
diff --git a/smithy-aws-traits/src/test/java/software/amazon/smithy/aws/traits/ServiceTraitTest.java b/smithy-aws-traits/src/test/java/software/amazon/smithy/aws/traits/ServiceTraitTest.java
index 6009d0b6a..2765bc170 100644
--- a/smithy-aws-traits/src/test/java/software/amazon/smithy/aws/traits/ServiceTraitTest.java
+++ b/smithy-aws-traits/src/test/java/software/amazon/smithy/aws/traits/ServiceTraitTest.java
@@ -18,6 +18,7 @@ package software.amazon.smithy.aws.traits;
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.hamcrest.Matchers.equalTo;
 import static org.hamcrest.Matchers.instanceOf;
+import static org.junit.jupiter.api.Assertions.assertFalse;
 import static org.junit.jupiter.api.Assertions.assertThrows;
 import static org.junit.jupiter.api.Assertions.assertTrue;
 
@@ -47,12 +48,13 @@ public class ServiceTraitTest {
         assertThat(serviceTrait.getCloudTrailEventSource(), equalTo("foo.amazonaws.com"));
         assertThat(serviceTrait.getEndpointPrefix(), equalTo("foo"));
         assertThat(serviceTrait.toBuilder().build(), equalTo(serviceTrait));
+        assertFalse(serviceTrait.getDocId().isPresent());
     }
 
     @Test
     public void loadsTraitWithOptionalValues() {
         Node node = Node.parse("{\"sdkId\": \"Foo\", \"arnNamespace\": \"service\", \"cloudFormationName\": \"Baz\", "
-                        + "\"endpointPrefix\": \"endpoint-prefix\"}");
+                        + "\"endpointPrefix\": \"endpoint-prefix\", \"docId\": \"doc-id\"}");
         TraitFactory provider = TraitFactory.createServiceFactory();
         Optional<Trait> trait = provider.createTrait(ServiceTrait.ID, ShapeId.from("ns.foo#foo"), node);
 
@@ -63,6 +65,8 @@ public class ServiceTraitTest {
         assertThat(serviceTrait.getArnNamespace(), equalTo("service"));
         assertThat(serviceTrait.getCloudFormationName(), equalTo("Baz"));
         assertThat(serviceTrait.getEndpointPrefix(), equalTo("endpoint-prefix"));
+        assertTrue(serviceTrait.getDocId().isPresent());
+        assertThat(serviceTrait.getDocId().get(), equalTo("doc-id"));
     }
 
     @Test
@@ -90,13 +94,14 @@ public class ServiceTraitTest {
                 .assemble()
                 .unwrap();
         ServiceShape service = result
-                .expectShape(ShapeId.from("ns.foo#SomeService"))
-                .asServiceShape().get();
-        ServiceTrait trait = service.getTrait(ServiceTrait.class).get();
+                .expectShape(ShapeId.from("ns.foo#SomeService"), ServiceShape.class);
+        ServiceTrait trait = service.expectTrait(ServiceTrait.class);
 
         assertThat(trait.getSdkId(), equalTo("Some Value"));
         assertThat(trait.getCloudFormationName(), equalTo("SomeService"));
         assertThat(trait.getArnNamespace(), equalTo("service"));
         assertThat(trait.getEndpointPrefix(), equalTo("some-service"));
+        assertFalse(trait.getDocId().isPresent());
+        assertThat(trait.getDocId(result), equalTo("some-value-2018-03-17"));
     }
 }
diff --git a/smithy-model/src/test/java/software/amazon/smithy/model/pattern/SmithyPatternTest.java b/smithy-model/src/test/java/software/amazon/smithy/model/pattern/SmithyPatternTest.java
index 3ebbe4a90..eae5c47b7 100644
--- a/smithy-model/src/test/java/software/amazon/smithy/model/pattern/SmithyPatternTest.java
+++ b/smithy-model/src/test/java/software/amazon/smithy/model/pattern/SmithyPatternTest.java
@@ -99,7 +99,7 @@ public class SmithyPatternTest {
                     .build();
         });
 
-        assertThat(thrown.getMessage(), containsString("Segments must not be empty"));
+        assertThat(thrown.getMessage(), containsString("Segments must not be empty at index 1"));
     }
 
     @Test
@@ -158,14 +158,14 @@ public class SmithyPatternTest {
     @Test
     public void noEmptyLabels() {
         Throwable thrown = Assertions.assertThrows(InvalidPatternException.class, () -> {
-            String target = "/{}";
+            String target = "/a/{}";
             SmithyPattern.builder()
                     .segments(parser(target))
                     .pattern(target)
                     .build();
         });
 
-        assertThat(thrown.getMessage(), containsString("Empty label declaration"));
+        assertThat(thrown.getMessage(), containsString("Empty label declaration in pattern at index 2"));
     }
 
     @Test
@@ -179,6 +179,7 @@ public class SmithyPatternTest {
         });
 
         assertThat(thrown.getMessage(), containsString("Invalid label name"));
+        assertThat(thrown.getMessage(), containsString("at index 1"));
     }
 
     @Test
@@ -192,5 +193,6 @@ public class SmithyPatternTest {
         });
 
         assertThat(thrown.getMessage(), containsString("Literal segments must not contain"));
+        assertThat(thrown.getMessage(), containsString("at index 1"));
     }
 }