Improve segment parsing errors with offsets

smithy-lang · Jul 20, 2023 · c419b33 · c419b33
1 parent 02ee935
commit c419b33
Show file tree

Hide file tree

Showing 3 changed files with 26 additions and 13 deletions.
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
@@ -235,10 +235,14 @@ public enum Type { LITERAL, LABEL, GREEDY_LABEL }
         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,27 +253,33 @@ public Segment(String content, Type segmentType) {
             }
         }
 
-        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.");
             }
         }
 
         /**
          * 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 static Segment parse(String content, int offset) {
                 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
@@ -78,7 +78,8 @@ public static UriPattern parse(String uri) {
         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<>();

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
@@ -99,7 +99,7 @@ public void labelsMustNotIncludeEmptySegments() {
                     .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 void greedyLabelsMustBeLastLabelInPattern() {
     @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 void labelsMustMatchRegex() {
         });
 
         assertThat(thrown.getMessage(), containsString("Invalid label name"));
+        assertThat(thrown.getMessage(), containsString("at index 1"));
     }
 
     @Test
@@ -192,5 +193,6 @@ public void labelsMustSpanEntireSegment() {
         });
 
         assertThat(thrown.getMessage(), containsString("Literal segments must not contain"));
+        assertThat(thrown.getMessage(), containsString("at index 1"));
     }
 }