Extend validation event ids to make them unique (#1527)

* Extend validation event ids to make them unique

* address comments, mostly to simplify some of the eventIds

* optimize assembleFullEventId method

* minor edits for style

* unroll eventId varargs in AbstractValidator for performance

* fix eventIds for Waiter violations

* remove unused method overloads

Author: rchache

smithy-aws-traits/src/main/java/software/amazon/smithy/aws/traits/HttpChecksumTraitValidator.java

@@ -185,11 +185,13 @@ private List<ValidationEvent> validateHeaderConflicts(OperationShape operation,
                     .map(HttpPrefixHeadersTrait::getValue)
                     .ifPresent(headerPrefix -> {
                         if (HttpChecksumTrait.CHECKSUM_PREFIX.startsWith(headerPrefix)) {
+                            String memberName = member.getId().getName();
+                            String prefixString = headerPrefix.toLowerCase(Locale.US);
                             events.add(danger(operation, format("The `httpPrefixHeaders` binding of `%s` uses"
-                                            + " the prefix `%s` that conflicts with the prefix `%s` used by the"
-                                            + " `httpChecksum` trait.",
-                                    member.getId().getName(), headerPrefix.toLowerCase(Locale.US),
-                                    HttpChecksumTrait.CHECKSUM_PREFIX)));
+                                                    + " the prefix `%s` that conflicts with the prefix `%s` used by the"
+                                                    + " `httpChecksum` trait.",
+                                                memberName, prefixString, HttpChecksumTrait.CHECKSUM_PREFIX),
+                                    "HttpPrefixHeaders", memberName, prefixString));
                         }
                     });
 
@@ -200,10 +202,12 @@ private List<ValidationEvent> validateHeaderConflicts(OperationShape operation,
                     .map(HttpHeaderTrait::getValue)
                     .ifPresent(headerName -> {
                         if (headerName.startsWith(HttpChecksumTrait.CHECKSUM_PREFIX)) {
+                            String memberName = member.getId().getName();
+                            String headerString = headerName.toLowerCase(Locale.US);
                             events.add(warning(operation, format("The `httpHeader` binding of `%s` on `%s`"
-                                            + " starts with the prefix `%s` used by the `httpChecksum` trait.",
-                                    headerName.toLowerCase(Locale.US), member.getId().getName(),
-                                    HttpChecksumTrait.CHECKSUM_PREFIX)));
+                                                    + " starts with the prefix `%s` used by the `httpChecksum` trait.",
+                                                headerString, memberName, HttpChecksumTrait.CHECKSUM_PREFIX),
+                                    "HttpHeader", memberName, headerString));
                         }
                     });
         }

smithy-aws-traits/src/test/resources/software/amazon/smithy/aws/traits/errorfiles/http-checksum-header-conflicts.errors

@@ -1,9 +1,9 @@
 [SUPPRESSED] smithy.example#HeaderConflicts: This shape applies a trait that is unstable: aws.protocols#httpChecksum | UnstableTrait
 [SUPPRESSED] smithy.example#HeadersConflicts: This shape applies a trait that is unstable: aws.protocols#httpChecksum | UnstableTrait
 [SUPPRESSED] smithy.example#NoConflicts: This shape applies a trait that is unstable: aws.protocols#httpChecksum | UnstableTrait
-[WARNING] smithy.example#HeaderConflicts: The `httpHeader` binding of `x-amz-checksum-crc32` on `HeaderConflictError` starts with the prefix `x-amz-checksum-` used by the `httpChecksum` trait. | HttpChecksumTrait
-[WARNING] smithy.example#HeaderConflicts: The `httpHeader` binding of `x-amz-checksum-crc32` on `HeaderConflictsInput` starts with the prefix `x-amz-checksum-` used by the `httpChecksum` trait. | HttpChecksumTrait
-[WARNING] smithy.example#HeaderConflicts: The `httpHeader` binding of `x-amz-checksum-crc32` on `HeaderConflictsOutput` starts with the prefix `x-amz-checksum-` used by the `httpChecksum` trait. | HttpChecksumTrait
-[DANGER] smithy.example#HeadersConflicts: The `httpPrefixHeaders` binding of `HeadersConflictError` uses the prefix `x-amz-checksum-` that conflicts with the prefix `x-amz-checksum-` used by the `httpChecksum` trait. | HttpChecksumTrait
-[DANGER] smithy.example#HeadersConflicts: The `httpPrefixHeaders` binding of `HeadersConflictsInput` uses the prefix `x-amz-checksum-` that conflicts with the prefix `x-amz-checksum-` used by the `httpChecksum` trait. | HttpChecksumTrait
-[DANGER] smithy.example#HeadersConflicts: The `httpPrefixHeaders` binding of `HeadersConflictsOutput` uses the prefix `x-amz-checksum-` that conflicts with the prefix `x-amz-checksum-` used by the `httpChecksum` trait. | HttpChecksumTrait
+[WARNING] smithy.example#HeaderConflicts: The `httpHeader` binding of `x-amz-checksum-crc32` on `HeaderConflictError` starts with the prefix `x-amz-checksum-` used by the `httpChecksum` trait. | HttpChecksumTrait.HttpHeader.HeaderConflictError.x-amz-checksum-crc32
+[WARNING] smithy.example#HeaderConflicts: The `httpHeader` binding of `x-amz-checksum-crc32` on `HeaderConflictsInput` starts with the prefix `x-amz-checksum-` used by the `httpChecksum` trait. | HttpChecksumTrait.HttpHeader.HeaderConflictsInput.x-amz-checksum-crc32
+[WARNING] smithy.example#HeaderConflicts: The `httpHeader` binding of `x-amz-checksum-crc32` on `HeaderConflictsOutput` starts with the prefix `x-amz-checksum-` used by the `httpChecksum` trait. | HttpChecksumTrait.HttpHeader.HeaderConflictsOutput.x-amz-checksum-crc32
+[DANGER] smithy.example#HeadersConflicts: The `httpPrefixHeaders` binding of `HeadersConflictError` uses the prefix `x-amz-checksum-` that conflicts with the prefix `x-amz-checksum-` used by the `httpChecksum` trait. | HttpChecksumTrait.HttpPrefixHeaders.HeadersConflictError.x-amz-checksum-
+[DANGER] smithy.example#HeadersConflicts: The `httpPrefixHeaders` binding of `HeadersConflictsInput` uses the prefix `x-amz-checksum-` that conflicts with the prefix `x-amz-checksum-` used by the `httpChecksum` trait. | HttpChecksumTrait.HttpPrefixHeaders.HeadersConflictsInput.x-amz-checksum-
+[DANGER] smithy.example#HeadersConflicts: The `httpPrefixHeaders` binding of `HeadersConflictsOutput` uses the prefix `x-amz-checksum-` that conflicts with the prefix `x-amz-checksum-` used by the `httpChecksum` trait. | HttpChecksumTrait.HttpPrefixHeaders.HeadersConflictsOutput.x-amz-checksum-

smithy-linters/src/main/java/software/amazon/smithy/linters/InputOutputStructureReuseValidator.java

@@ -39,6 +39,9 @@ public Provider() {
         }
     }
 
+    private static final String INPUT = "Input";
+    private static final String OUTPUT = "Output";
+
     @Override
     public List<ValidationEvent> validate(Model model) {
         List<ValidationEvent> events = new ArrayList<>();
@@ -62,13 +65,15 @@ private void validateInputOutputSet(
                     "This structure is the input of `%s`, but it is not marked with the "
                     + "@input trait. The @input trait gives operations more flexibility to "
                     + "evolve their top-level input members in ways that would otherwise "
-                    + "be backward incompatible.", operation.getId())));
+                    + "be backward incompatible.", operation.getId()),
+                    INPUT, operation.getId().getName()));
         }
 
         if (!output.hasTrait(OutputTrait.class)) {
             events.add(warning(output, String.format(
                     "This structure is the output of `%s`, but it is not marked with "
-                    + "the @output trait.", operation.getId())));
+                    + "the @output trait.", operation.getId()),
+                    OUTPUT, operation.getId().getName()));
         }
     }
 }

smithy-linters/src/main/java/software/amazon/smithy/linters/NoninclusiveTermsValidator.java

@@ -20,6 +20,7 @@
 import java.util.Collections;
 import java.util.HashMap;
 import java.util.List;
+import java.util.Locale;
 import java.util.Map;
 import java.util.stream.Collectors;
 import software.amazon.smithy.model.Model;
@@ -58,6 +59,10 @@ public Provider() {
         }
     }
 
+    private static final String TRAIT = "Trait";
+    private static final String SHAPE = "Shape";
+    private static final String NAMESPACE = "Namespace";
+
     /**
      * NoninclusiveTermsValidator configuration.
      */
@@ -121,7 +126,7 @@ public List<ValidationEvent> validate(Model model) {
     /**
      * Generates zero or more @see ValidationEvents and returns them in a collection.
      *
-     * @param occurrence text occurrence found in the body of the model
+     * @param instance text occurrence found in the body of the model
      */
     private Collection<ValidationEvent> getValidationEvents(TextInstance instance) {
         final Collection<ValidationEvent> events = new ArrayList<>();
@@ -130,70 +135,72 @@ private Collection<ValidationEvent> getValidationEvents(TextInstance instance) {
             final int startIndex = instance.getText().toLowerCase().indexOf(termLower);
             if (startIndex != -1) {
                 final String matchedText = instance.getText().substring(startIndex, startIndex + termLower.length());
-                switch (instance.getLocationType()) {
-                    case NAMESPACE:
-                        //Cannot use any warning() overloads because there is no shape associated with the event.
-                        events.add(ValidationEvent.builder()
-                                .sourceLocation(SourceLocation.none())
-                                .id(this.getClass().getSimpleName().replaceFirst("Validator$", ""))
-                                .severity(Severity.WARNING)
-                                .message(formatNonInclusiveTermsValidationMessage(termEntry, matchedText, instance))
-                                .build());
-                        break;
-                    case APPLIED_TRAIT:
-                        events.add(warning(instance.getShape(),
-                                instance.getTrait().getSourceLocation(),
-                                formatNonInclusiveTermsValidationMessage(termEntry, matchedText, instance)));
-                        break;
-                    case SHAPE:
-                    default:
-                        events.add(warning(instance.getShape(),
-                                instance.getShape().getSourceLocation(),
-                                formatNonInclusiveTermsValidationMessage(termEntry, matchedText, instance)));
-                }
+                events.add(constructValidationEvent(instance, termEntry.getValue(), matchedText));
             }
         }
         return events;
     }
 
-    private static String formatNonInclusiveTermsValidationMessage(
-            Map.Entry<String, List<String>> termEntry,
-            String matchedText,
-            TextInstance instance
-    ) {
-        final List<String> caseCorrectedEntryValue = termEntry.getValue().stream()
-            .map(replacement -> Character.isUpperCase(matchedText.charAt(0))
-                  ? StringUtils.capitalize(replacement)
-                  : StringUtils.uncapitalize(replacement))
-            .collect(Collectors.toList());
-        String replacementAddendum = !termEntry.getValue().isEmpty()
-                ? String.format(" Consider using one of the following terms instead: %s",
-                    ValidationUtils.tickedList(caseCorrectedEntryValue))
-                : "";
+    private ValidationEvent constructValidationEvent(TextInstance instance,
+                                                     List<String> replacements,
+                                                     String matchedText) {
+        String replacementAddendum = getReplacementAddendum(matchedText, replacements);
         switch (instance.getLocationType()) {
-            case SHAPE:
-                return String.format("%s shape uses a non-inclusive term `%s`.%s",
-                        StringUtils.capitalize(instance.getShape().getType().toString()),
-                        matchedText, replacementAddendum);
             case NAMESPACE:
-                return String.format("%s namespace uses a non-inclusive term `%s`.%s",
-                        instance.getText(), matchedText, replacementAddendum);
+                //Cannot use any warning() overloads because there is no shape associated with the event.
+                return ValidationEvent.builder()
+                        .severity(Severity.WARNING)
+                        .sourceLocation(SourceLocation.none())
+                        .id(getName() + "." + NAMESPACE + "." + instance.getText()
+                                + "." + matchedText.toLowerCase(Locale.US))
+                        .message(String.format("%s namespace uses a non-inclusive term `%s`.%s",
+                                instance.getText(), matchedText, replacementAddendum))
+                        .build();
             case APPLIED_TRAIT:
+                ValidationEvent validationEvent =
+                        warning(instance.getShape(), instance.getTrait().getSourceLocation(), "");
+                String idiomaticTraitName = Trait.getIdiomaticTraitName(instance.getTrait());
                 if (instance.getTraitPropertyPath().isEmpty()) {
-                    return String.format("'%s' trait has a value that contains a non-inclusive term `%s`.%s",
-                            Trait.getIdiomaticTraitName(instance.getTrait()), matchedText,
-                            replacementAddendum);
+                    return validationEvent.toBuilder()
+                            .message(String.format("'%s' trait has a value that contains a non-inclusive term `%s`.%s",
+                                    idiomaticTraitName, matchedText, replacementAddendum))
+                            .id(getName() + "." + TRAIT + "."
+                                    + matchedText.toLowerCase(Locale.US) + "." + idiomaticTraitName)
+                            .build();
                 } else {
                     String valuePropertyPathFormatted = formatPropertyPath(instance.getTraitPropertyPath());
-                    return String.format("'%s' trait value at path {%s} contains a non-inclusive term `%s`.%s",
-                            Trait.getIdiomaticTraitName(instance.getTrait()), valuePropertyPathFormatted,
-                            matchedText, replacementAddendum);
+                    return validationEvent.toBuilder()
+                            .message(String.format(
+                                    "'%s' trait value at path {%s} contains a non-inclusive term `%s`.%s",
+                                    idiomaticTraitName, valuePropertyPathFormatted, matchedText, replacementAddendum))
+                            .id(getName() + "." + TRAIT + "." + matchedText.toLowerCase(Locale.US)
+                                    + "." + idiomaticTraitName + "." + valuePropertyPathFormatted)
+                            .build();
                 }
+            case SHAPE:
             default:
-                throw new IllegalStateException();
+                return warning(instance.getShape(),
+                        instance.getShape().getSourceLocation(),
+                        String.format("%s shape uses a non-inclusive term `%s`.%s",
+                                StringUtils.capitalize(instance.getShape().getType().toString()),
+                                matchedText, replacementAddendum),
+                        SHAPE, matchedText.toLowerCase(Locale.US));
         }
     }
 
+    private static String getReplacementAddendum(String matchedText, List<String> replacements) {
+        List<String> caseCorrectedEntryValue = replacements.stream()
+                .map(replacement -> Character.isUpperCase(matchedText.charAt(0))
+                        ? StringUtils.capitalize(replacement)
+                        : StringUtils.uncapitalize(replacement))
+                .collect(Collectors.toList());
+        String replacementAddendum = !replacements.isEmpty()
+                ? String.format(" Consider using one of the following terms instead: %s",
+                ValidationUtils.tickedList(caseCorrectedEntryValue))
+                : "";
+        return replacementAddendum;
+    }
+
     private static String formatPropertyPath(List<String> traitPropertyPath) {
         return String.join("/", traitPropertyPath);
     }

smithy-linters/src/test/resources/software/amazon/smithy/linters/errorfiles/input-output-structure-reuse.errors

@@ -1,2 +1,2 @@
-[WARNING] smithy.example#GetFooInput: This structure is the input of `smithy.example#GetFoo`, but it is not marked with the @input trait. The @input trait gives operations more flexibility to evolve their top-level input members in ways that would otherwise be backward incompatible. | InputOutputStructureReuse
-[WARNING] smithy.example#GetFooOutput: This structure is the output of `smithy.example#GetFoo`, but it is not marked with the @output trait. | InputOutputStructureReuse
+[WARNING] smithy.example#GetFooInput: This structure is the input of `smithy.example#GetFoo`, but it is not marked with the @input trait. The @input trait gives operations more flexibility to evolve their top-level input members in ways that would otherwise be backward incompatible. | InputOutputStructureReuse.Input.GetFoo
+[WARNING] smithy.example#GetFooOutput: This structure is the output of `smithy.example#GetFoo`, but it is not marked with the @output trait. | InputOutputStructureReuse.Output.GetFoo