From 36aa4a340cfa39f052e7b47413f23983ca9b559c Mon Sep 17 00:00:00 2001 From: Richard Chen Date: Sat, 3 Dec 2022 20:35:33 -0500 Subject: [PATCH 1/7] Extend validation event ids to make them unique --- .../traits/HttpChecksumTraitValidator.java | 18 +-- .../http-checksum-header-conflicts.errors | 12 +- .../InputOutputStructureReuseValidator.java | 9 +- .../linters/NoninclusiveTermsValidator.java | 111 ++++++++++-------- .../input-output-structure-reuse.errors | 4 +- .../noninclusive-term-filter.errors | 26 ++-- .../model/validation/AbstractValidator.java | 102 ++++++++++++---- .../validation/NodeValidationVisitor.java | 20 ++-- .../validation/testrunner/SmithyTestCase.java | 6 +- .../validators/EnumTraitValidator.java | 12 +- .../HttpMethodSemanticsValidator.java | 26 ++-- .../validators/OperationValidator.java | 2 +- .../validators/PaginatedTraitValidator.java | 35 ++++-- .../validators/TargetValidator.java | 2 +- .../validators/UnstableTraitValidator.java | 8 +- .../amazon/smithy/model/ShapeMatcher.java | 2 +- .../annotation-trait-extra-properties.errors | 2 +- .../deprecated-shape/deprecated-shape.errors | 8 +- .../enums/enum-trait-validation.errors | 10 +- .../http-method-semantics-validator.errors | 15 +-- .../http-method-semantics-validator.json | 6 + ...alid-multiple-operations-same-shape.errors | 4 +- .../paginated/paginated-deeply-nested.errors | 4 +- .../paginated/paginated-invalid.errors | 2 +- .../paginated/paginated-map-tokens.errors | 4 +- .../trait-value/extra-trait-property.errors | 2 +- .../unstable-trait-validator.errors | 2 +- .../waiters/WaiterMatcherValidator.java | 34 +++--- ...emits-danger-and-warning-typechecks.errors | 4 +- .../errorfiles/invalid-errorType.errors | 2 +- .../errorfiles/invalid-jmespath-syntax.errors | 2 +- .../errorfiles/invalid-return-types.errors | 8 +- 32 files changed, 314 insertions(+), 190 deletions(-) diff --git a/smithy-aws-traits/src/main/java/software/amazon/smithy/aws/traits/HttpChecksumTraitValidator.java b/smithy-aws-traits/src/main/java/software/amazon/smithy/aws/traits/HttpChecksumTraitValidator.java index f8a87f64c55..ba6a93cf78d 100644 --- a/smithy-aws-traits/src/main/java/software/amazon/smithy/aws/traits/HttpChecksumTraitValidator.java +++ b/smithy-aws-traits/src/main/java/software/amazon/smithy/aws/traits/HttpChecksumTraitValidator.java @@ -185,11 +185,13 @@ private List validateHeaderConflicts(OperationShape operation, .map(HttpPrefixHeadersTrait::getValue) .ifPresent(headerPrefix -> { if (HttpChecksumTrait.CHECKSUM_PREFIX.startsWith(headerPrefix)) { + final String memberName = member.getId().getName(); + final 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 validateHeaderConflicts(OperationShape operation, .map(HttpHeaderTrait::getValue) .ifPresent(headerName -> { if (headerName.startsWith(HttpChecksumTrait.CHECKSUM_PREFIX)) { + final String memberName = member.getId().getName(); + final 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)); } }); } diff --git a/smithy-aws-traits/src/test/resources/software/amazon/smithy/aws/traits/errorfiles/http-checksum-header-conflicts.errors b/smithy-aws-traits/src/test/resources/software/amazon/smithy/aws/traits/errorfiles/http-checksum-header-conflicts.errors index a5ad203e924..2b1539db708 100644 --- a/smithy-aws-traits/src/test/resources/software/amazon/smithy/aws/traits/errorfiles/http-checksum-header-conflicts.errors +++ b/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- diff --git a/smithy-linters/src/main/java/software/amazon/smithy/linters/InputOutputStructureReuseValidator.java b/smithy-linters/src/main/java/software/amazon/smithy/linters/InputOutputStructureReuseValidator.java index 4281fb0c9b7..46c5f0f8abc 100644 --- a/smithy-linters/src/main/java/software/amazon/smithy/linters/InputOutputStructureReuseValidator.java +++ b/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 validate(Model model) { List 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().toString())); } 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().toString())); } } } diff --git a/smithy-linters/src/main/java/software/amazon/smithy/linters/NoninclusiveTermsValidator.java b/smithy-linters/src/main/java/software/amazon/smithy/linters/NoninclusiveTermsValidator.java index 4e688004be3..62667ffd7a4 100644 --- a/smithy-linters/src/main/java/software/amazon/smithy/linters/NoninclusiveTermsValidator.java +++ b/smithy-linters/src/main/java/software/amazon/smithy/linters/NoninclusiveTermsValidator.java @@ -58,6 +58,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 +125,7 @@ public List 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 getValidationEvents(TextInstance instance) { final Collection events = new ArrayList<>(); @@ -130,70 +134,79 @@ private Collection 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> termEntry, - String matchedText, - TextInstance instance + /** + * Creates a single ValidationEvent. + * @param instance TextInstance where the noninclusive term was found + * @param replacements List of suggested replacements for the noninclusive term + * @param matchedText the noninclusive term that was used + * @return the constructed ValidationEvent + */ + private ValidationEvent constructValidationEvent( + final TextInstance instance, + final List replacements, + final String matchedText ) { - final List 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)) - : ""; + final 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(assembleFullEventId(NAMESPACE, instance.getText(), matchedText)) + .message(String.format("%s namespace uses a non-inclusive term `%s`.%s", + instance.getText(), matchedText, replacementAddendum)) + .build(); case APPLIED_TRAIT: + final ValidationEvent validationEvent = + warning(instance.getShape(), instance.getTrait().getSourceLocation(), ""); + final 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(assembleFullEventId(TRAIT, idiomaticTraitName, matchedText)) + .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); + final String valuePropertyPathFormatted = formatPropertyPath(instance.getTraitPropertyPath()); + return validationEvent.toBuilder() + .message(String.format( + "'%s' trait value at path {%s} contains a non-inclusive term `%s`.%s", + idiomaticTraitName, valuePropertyPathFormatted, matchedText, replacementAddendum)) + .id(assembleFullEventId( + TRAIT, idiomaticTraitName, valuePropertyPathFormatted, matchedText)) + .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); } } + private static String getReplacementAddendum(String matchedText, List replacements) { + final List 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 traitPropertyPath) { return String.join("/", traitPropertyPath); } diff --git a/smithy-linters/src/test/resources/software/amazon/smithy/linters/errorfiles/input-output-structure-reuse.errors b/smithy-linters/src/test/resources/software/amazon/smithy/linters/errorfiles/input-output-structure-reuse.errors index b8d4e86eaa0..2d588e73e27 100644 --- a/smithy-linters/src/test/resources/software/amazon/smithy/linters/errorfiles/input-output-structure-reuse.errors +++ b/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.smithy.example#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.smithy.example#GetFoo diff --git a/smithy-linters/src/test/resources/software/amazon/smithy/linters/errorfiles/noninclusive-term-filter.errors b/smithy-linters/src/test/resources/software/amazon/smithy/linters/errorfiles/noninclusive-term-filter.errors index b9df6a47cb7..4fe9ffe9674 100644 --- a/smithy-linters/src/test/resources/software/amazon/smithy/linters/errorfiles/noninclusive-term-filter.errors +++ b/smithy-linters/src/test/resources/software/amazon/smithy/linters/errorfiles/noninclusive-term-filter.errors @@ -1,13 +1,13 @@ -[WARNING] ns.foo#MyMasterService: Service shape uses a non-inclusive term `Master`. Consider using one of the following terms instead: `Main`, `Parent`, `Primary` | NoninclusiveTerms -[WARNING] ns.foo#BlackListThings: Operation shape uses a non-inclusive term `BlackList`. Consider using one of the following terms instead: `DenyList` | NoninclusiveTerms -[WARNING] ns.foo#MyWhitelistTrait: Structure shape uses a non-inclusive term `Whitelist`. Consider using one of the following terms instead: `AllowList` | NoninclusiveTerms -[WARNING] ns.foo#AInput$master_member_name: Member shape uses a non-inclusive term `master`. Consider using one of the following terms instead: `main`, `parent`, `primary` | NoninclusiveTerms -[WARNING] ns.foo#NestedTraitStructure$master_key_violation: Member shape uses a non-inclusive term `master`. Consider using one of the following terms instead: `main`, `parent`, `primary` | NoninclusiveTerms -[WARNING] ns.foo#AInput$foo: 'ns.foo#MySimpleValueTrait' trait has a value that contains a non-inclusive term `slave`. Consider using one of the following terms instead: `child`, `clone`, `replica`, `secondary` | NoninclusiveTerms -[WARNING] ns.foo#AOutput: 'ns.foo#MyWhitelistTrait' trait value at path {document/foo/0/bar} contains a non-inclusive term `whitelist`. Consider using one of the following terms instead: `allowList` | NoninclusiveTerms -[WARNING] ns.foo#AOutput: 'ns.foo#MyWhitelistTrait' trait value at path {nested/master_key_violation} contains a non-inclusive term `whitelist`. Consider using one of the following terms instead: `allowList` | NoninclusiveTerms -[WARNING] ns.foo#AOutput: 'ns.foo#MyWhitelistTrait' trait value at path {string_value} contains a non-inclusive term `whitelist`. Consider using one of the following terms instead: `allowList` | NoninclusiveTerms -[WARNING] ns.foo#AOutput: 'ns.foo#MyWhitelistTrait' trait value at path {document/whitelist_doc_key_violation_1} contains a non-inclusive term `whitelist`. Consider using one of the following terms instead: `allowList` | NoninclusiveTerms -[WARNING] ns.foo#AOutput: 'ns.foo#MyWhitelistTrait' trait value at path {collection/2/key} contains a non-inclusive term `whitelist`. Consider using one of the following terms instead: `allowList` | NoninclusiveTerms -[WARNING] ns.foo#AOutput: 'ns.foo#MyWhitelistTrait' trait value at path {collection/1/blacklist_key} contains a non-inclusive term `blacklist`. Consider using one of the following terms instead: `denyList` | NoninclusiveTerms -[WARNING] ns.foo#MyUnionTrait$int_whitelist: Member shape uses a non-inclusive term `whitelist`. Consider using one of the following terms instead: `allowList` | NoninclusiveTerms +[WARNING] ns.foo#MyMasterService: Service shape uses a non-inclusive term `Master`. Consider using one of the following terms instead: `Main`, `Parent`, `Primary` | NoninclusiveTerms.SHAPE.Master +[WARNING] ns.foo#BlackListThings: Operation shape uses a non-inclusive term `BlackList`. Consider using one of the following terms instead: `DenyList` | NoninclusiveTerms.SHAPE.BlackList +[WARNING] ns.foo#MyWhitelistTrait: Structure shape uses a non-inclusive term `Whitelist`. Consider using one of the following terms instead: `AllowList` | NoninclusiveTerms.SHAPE.Whitelist +[WARNING] ns.foo#AInput$master_member_name: Member shape uses a non-inclusive term `master`. Consider using one of the following terms instead: `main`, `parent`, `primary` | NoninclusiveTerms.SHAPE.master +[WARNING] ns.foo#NestedTraitStructure$master_key_violation: Member shape uses a non-inclusive term `master`. Consider using one of the following terms instead: `main`, `parent`, `primary` | NoninclusiveTerms.SHAPE.master +[WARNING] ns.foo#AInput$foo: 'ns.foo#MySimpleValueTrait' trait has a value that contains a non-inclusive term `slave`. Consider using one of the following terms instead: `child`, `clone`, `replica`, `secondary` | NoninclusiveTerms.TRAIT.ns.foo#MySimpleValueTrait.slave +[WARNING] ns.foo#AOutput: 'ns.foo#MyWhitelistTrait' trait value at path {document/foo/0/bar} contains a non-inclusive term `whitelist`. Consider using one of the following terms instead: `allowList` | NoninclusiveTerms.TRAIT.ns.foo#MyWhitelistTrait.document/foo/0/bar.whitelist +[WARNING] ns.foo#AOutput: 'ns.foo#MyWhitelistTrait' trait value at path {nested/master_key_violation} contains a non-inclusive term `whitelist`. Consider using one of the following terms instead: `allowList` | NoninclusiveTerms.TRAIT.ns.foo#MyWhitelistTrait.nested/master_key_violation.whitelist +[WARNING] ns.foo#AOutput: 'ns.foo#MyWhitelistTrait' trait value at path {string_value} contains a non-inclusive term `whitelist`. Consider using one of the following terms instead: `allowList` | NoninclusiveTerms.TRAIT.ns.foo#MyWhitelistTrait.string_value.whitelist +[WARNING] ns.foo#AOutput: 'ns.foo#MyWhitelistTrait' trait value at path {document/whitelist_doc_key_violation_1} contains a non-inclusive term `whitelist`. Consider using one of the following terms instead: `allowList` | NoninclusiveTerms.TRAIT.ns.foo#MyWhitelistTrait.document/whitelist_doc_key_violation_1.whitelist +[WARNING] ns.foo#AOutput: 'ns.foo#MyWhitelistTrait' trait value at path {collection/2/key} contains a non-inclusive term `whitelist`. Consider using one of the following terms instead: `allowList` | NoninclusiveTerms.TRAIT.ns.foo#MyWhitelistTrait.collection/2/key.whitelist +[WARNING] ns.foo#AOutput: 'ns.foo#MyWhitelistTrait' trait value at path {collection/1/blacklist_key} contains a non-inclusive term `blacklist`. Consider using one of the following terms instead: `denyList` | NoninclusiveTerms.TRAIT.ns.foo#MyWhitelistTrait.collection/1/blacklist_key.blacklist +[WARNING] ns.foo#MyUnionTrait$int_whitelist: Member shape uses a non-inclusive term `whitelist`. Consider using one of the following terms instead: `allowList` | NoninclusiveTerms.SHAPE.whitelist diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/validation/AbstractValidator.java b/smithy-model/src/main/java/software/amazon/smithy/model/validation/AbstractValidator.java index 22065cf4185..a2771119632 100644 --- a/smithy-model/src/main/java/software/amazon/smithy/model/validation/AbstractValidator.java +++ b/smithy-model/src/main/java/software/amazon/smithy/model/validation/AbstractValidator.java @@ -28,48 +28,104 @@ public String getName() { return defaultName; } - protected final ValidationEvent error(Shape shape, String message) { - return createEvent(Severity.ERROR, shape, shape.getSourceLocation(), message); + protected final ValidationEvent error( + Shape shape, + String message, + String... additionalEventIdParts + ) { + return createEvent(Severity.ERROR, shape, shape.getSourceLocation(), message, additionalEventIdParts); } - protected final ValidationEvent error(Shape shape, FromSourceLocation location, String message) { - return createEvent(Severity.ERROR, shape, location, message); + protected final ValidationEvent error( + Shape shape, + FromSourceLocation location, + String message, + String... additionalEventIdParts + ) { + return createEvent(Severity.ERROR, shape, location, message, additionalEventIdParts); } - protected final ValidationEvent danger(Shape shape, String message) { - return createEvent(Severity.DANGER, shape, shape.getSourceLocation(), message); + protected final ValidationEvent danger( + Shape shape, + String message, + String... additionalEventIdParts + ) { + return createEvent(Severity.DANGER, shape, shape.getSourceLocation(), message, additionalEventIdParts); } - protected final ValidationEvent danger(Shape shape, FromSourceLocation location, String message) { - return createEvent(Severity.DANGER, shape, location, message); + protected final ValidationEvent danger( + Shape shape, + FromSourceLocation location, + String message, + String... additionalEventIdParts + ) { + return createEvent(Severity.DANGER, shape, location, message, additionalEventIdParts); } - protected final ValidationEvent warning(Shape shape, String message) { - return createEvent(Severity.WARNING, shape, shape.getSourceLocation(), message); + protected final ValidationEvent warning( + Shape shape, + String message, + String... additionalEventIdParts + ) { + return createEvent(Severity.WARNING, shape, shape.getSourceLocation(), message, additionalEventIdParts); } - protected final ValidationEvent warning(Shape shape, FromSourceLocation location, String message) { - return createEvent(Severity.WARNING, shape, location, message); + protected final ValidationEvent warning( + Shape shape, + FromSourceLocation location, + String message, + String... additionalEventIdParts + ) { + return createEvent(Severity.WARNING, shape, location, message, additionalEventIdParts); } - protected final ValidationEvent note(Shape shape, String message) { - return createEvent(Severity.NOTE, shape, shape.getSourceLocation(), message); + protected final ValidationEvent note( + Shape shape, + String message, + String... additionalEventIdParts + ) { + return createEvent(Severity.NOTE, shape, shape.getSourceLocation(), message, additionalEventIdParts); } - protected final ValidationEvent note(Shape shape, FromSourceLocation location, String message) { - return createEvent(Severity.NOTE, shape, location, message); + protected final ValidationEvent note( + Shape shape, + FromSourceLocation location, + String message, + String... additionalEventIdParts + ) { + return createEvent(Severity.NOTE, shape, location, message, additionalEventIdParts); } - protected final ValidationEvent createEvent(Severity severity, Shape shape, String message) { - return createEvent(severity, shape, shape.getSourceLocation(), message); + protected final ValidationEvent createEvent( + Severity severity, + Shape shape, + String message, + String... additionalEventIdParts + ) { + return createEvent(severity, shape, shape.getSourceLocation(), message, additionalEventIdParts); } - protected final ValidationEvent createEvent(Severity severity, Shape shape, FromSourceLocation loc, String msg) { - return createEvent(ValidationEvent.builder().severity(severity).message(msg) - .shapeId(shape.getId()).sourceLocation(loc.getSourceLocation())); + protected final ValidationEvent createEvent( + Severity severity, + Shape shape, + FromSourceLocation loc, + String msg, + String... additionalEventIdParts + ) { + return ValidationEvent.builder() + .severity(severity) + .message(msg) + .shapeId(shape.getId()) + .sourceLocation(loc.getSourceLocation()) + .id(assembleFullEventId(additionalEventIdParts)) + .build(); } - private ValidationEvent createEvent(ValidationEvent.Builder builder) { - return builder.id(getName()).build(); + protected final String assembleFullEventId(String... additionalEventIdParts) { + String eventId = getName(); + if (additionalEventIdParts.length > 0) { + eventId += "." + String.join(".", additionalEventIdParts); + } + return eventId; } } diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/validation/NodeValidationVisitor.java b/smithy-model/src/main/java/software/amazon/smithy/model/validation/NodeValidationVisitor.java index 063c8605418..7ba0957397e 100644 --- a/smithy-model/src/main/java/software/amazon/smithy/model/validation/NodeValidationVisitor.java +++ b/smithy-model/src/main/java/software/amazon/smithy/model/validation/NodeValidationVisitor.java @@ -309,7 +309,7 @@ public List structureShape(StructureShape shape) { if (!members.containsKey(entryKey)) { String message = String.format( "Invalid structure member `%s` found for `%s`", entryKey, shape.getId()); - events.add(event(message, Severity.WARNING)); + events.add(event(message, Severity.WARNING, shape.getId().toString(), entryKey)); } else { events.addAll(traverse(entryKey, entryValue).memberShape(members.get(entryKey))); } @@ -402,17 +402,23 @@ private List invalidSchema(Shape shape) { return ListUtils.of(event("Encountered invalid shape type: " + shape.getType())); } - private ValidationEvent event(String message) { - return event(message, Severity.ERROR); + private ValidationEvent event(String message, String... additionalEventIdParts) { + return event(message, Severity.ERROR, additionalEventIdParts); } - private ValidationEvent event(String message, Severity severity) { - return event(message, severity, value.getSourceLocation()); + private ValidationEvent event(String message, Severity severity, String... additionalEventIdParts) { + return event(message, severity, value.getSourceLocation(), additionalEventIdParts); } - private ValidationEvent event(String message, Severity severity, SourceLocation sourceLocation) { + private ValidationEvent event( + String message, + Severity severity, + SourceLocation sourceLocation, + String... additionalEventIdParts + ) { return ValidationEvent.builder() - .id(eventId) + .id(additionalEventIdParts.length > 0 + ? eventId + "." + String.join(".", additionalEventIdParts) : eventId) .severity(severity) .sourceLocation(sourceLocation) .shapeId(eventShapeId) diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/validation/testrunner/SmithyTestCase.java b/smithy-model/src/main/java/software/amazon/smithy/model/validation/testrunner/SmithyTestCase.java index 82196018cc0..94a54f83335 100644 --- a/smithy-model/src/main/java/software/amazon/smithy/model/validation/testrunner/SmithyTestCase.java +++ b/smithy-model/src/main/java/software/amazon/smithy/model/validation/testrunner/SmithyTestCase.java @@ -99,8 +99,8 @@ public String getModelLocation() { *

The validation events encountered while validating a model are * compared against the expected validation events. An actual event (A) is * considered a match with an expected event (E) if A and E target the - * same shape, use the same validation event ID, have the same severity, - * and the message of E starts with the suppression reason or message + * same shape, have the same severity, the eventId of A contains the eventId + * of E, and the message of E starts with the suppression reason or message * of A. * * @param validatedResult Result of creating and validating the model. @@ -135,7 +135,7 @@ private static boolean compareEvents(ValidationEvent expected, ValidationEvent a String comparedMessage = expected.getMessage().replace("\n", "\\n").replace("\r", "\\n"); return expected.getSeverity() == actual.getSeverity() - && expected.getId().equals(actual.getId()) + && actual.containsId(expected.getId()) && expected.getShapeId().equals(actual.getShapeId()) // Normalize new lines. && normalizedActualMessage.startsWith(comparedMessage); diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/validation/validators/EnumTraitValidator.java b/smithy-model/src/main/java/software/amazon/smithy/model/validation/validators/EnumTraitValidator.java index 1465406efa8..686e05ade3f 100644 --- a/smithy-model/src/main/java/software/amazon/smithy/model/validation/validators/EnumTraitValidator.java +++ b/smithy-model/src/main/java/software/amazon/smithy/model/validation/validators/EnumTraitValidator.java @@ -71,10 +71,14 @@ private List validateEnumTrait(Shape shape, EnumTrait trait) { "Duplicate enum trait values found with the same `name` property of '%s'", name))); } if (!RECOMMENDED_NAME_PATTERN.matcher(name).find()) { - events.add(warning(shape, trait, String.format( - "The name `%s` does not match the recommended enum name format of beginning with an " - + "uppercase letter, followed by any number of uppercase letters, numbers, or underscores.", - name))); + events.add(warning( + shape, + trait, + String.format("The name `%s` does not match the recommended enum name format of beginning " + + "with an uppercase letter, followed by any number of uppercase letters, numbers, " + + "or underscores.", name), + name) + ); } } } diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/validation/validators/HttpMethodSemanticsValidator.java b/smithy-model/src/main/java/software/amazon/smithy/model/validation/validators/HttpMethodSemanticsValidator.java index 3bb8e85584c..20b8bd04fa0 100644 --- a/smithy-model/src/main/java/software/amazon/smithy/model/validation/validators/HttpMethodSemanticsValidator.java +++ b/smithy-model/src/main/java/software/amazon/smithy/model/validation/validators/HttpMethodSemanticsValidator.java @@ -29,7 +29,6 @@ import software.amazon.smithy.model.traits.ReadonlyTrait; import software.amazon.smithy.model.validation.AbstractValidator; import software.amazon.smithy.model.validation.ValidationEvent; -import software.amazon.smithy.model.validation.ValidationUtils; import software.amazon.smithy.utils.MapUtils; /** @@ -97,27 +96,30 @@ private List validateOperation( HttpMethodSemantics semantics = EXPECTED.get(method); if (semantics.warningWhenModeled != null) { - events.add(warning(shape, trait, semantics.warningWhenModeled)); + events.add(warning(shape, trait, semantics.warningWhenModeled, method)); } boolean isReadonly = shape.getTrait(ReadonlyTrait.class).isPresent(); if (semantics.isReadonly != null && semantics.isReadonly != isReadonly) { events.add(warning(shape, trait, String.format( "This operation uses the `%s` method in the `http` trait, but %s marked with the readonly trait", - method, isReadonly ? "is" : "is not"))); + method, isReadonly ? "is" : "is not"), + method, ReadonlyTrait.ID.toString())); } boolean isIdempotent = shape.getTrait(IdempotentTrait.class).isPresent(); if (semantics.isIdempotent != null && semantics.isIdempotent != isIdempotent) { events.add(warning(shape, trait, String.format( "This operation uses the `%s` method in the `http` trait, but %s marked with the idempotent trait", - method, isIdempotent ? "is" : "is not"))); + method, isIdempotent ? "is" : "is not"), + method, IdempotentTrait.ID.toString())); } List payloadBindings = bindingIndex.getRequestBindings(shape, HttpBinding.Location.PAYLOAD); List documentBindings = bindingIndex.getRequestBindings(shape, HttpBinding.Location.DOCUMENT); if (semantics.allowsRequestPayload != null && !semantics.allowsRequestPayload) { if (!payloadBindings.isEmpty()) { + final String payloadMember = payloadBindings.get(0).getMemberName(); events.add(danger(shape, trait, String.format( "This operation uses the `%s` method in the `http` trait, but the `%s` member is sent as the " + "payload of the request because it is marked with the `httpPayload` trait. Many HTTP " @@ -125,19 +127,23 @@ private List validateOperation( + "other parts of the HTTP request such as a query string parameter using the `httpQuery` " + "trait, a header using the `httpHeader` trait, or a path segment using the `httpLabel` " + "trait.", - method, payloadBindings.get(0).getMemberName() - ))); + method, payloadMember), + method, payloadMember) + ); } else if (!documentBindings.isEmpty()) { - events.add(danger(shape, trait, String.format( + for (HttpBinding payloadMember : documentBindings) { + events.add(danger(shape, trait, String.format( "This operation uses the `%s` method in the `http` trait, but the following members " - + "are sent as part of the payload of the request: %s. These members are sent as part " + + "are sent as part of the payload of the request: `%s`. These members are sent as part " + "of the payload because they are not explicitly configured to be sent in headers, in the " + "query string, or in a URI segment. Many HTTP clients do not support payloads with %1$s " + "requests. Consider binding these members to other parts of the HTTP request such as " + "query string parameters using the `httpQuery` trait, headers using the `httpHeader` " + "trait, or URI segments using the `httpLabel` trait.", - method, ValidationUtils.tickedList(documentBindings.stream().map(HttpBinding::getMemberName)) - ))); + method, payloadMember.getMemberName()), + method, payloadMember.getMemberName() + )); + } } } diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/validation/validators/OperationValidator.java b/smithy-model/src/main/java/software/amazon/smithy/model/validation/validators/OperationValidator.java index 1ac72a3d4ae..6d743712a95 100644 --- a/smithy-model/src/main/java/software/amazon/smithy/model/validation/validators/OperationValidator.java +++ b/smithy-model/src/main/java/software/amazon/smithy/model/validation/validators/OperationValidator.java @@ -141,7 +141,7 @@ private ValidationEvent emitBadInputOutputName(Shape operation, String property, return ValidationEvent.builder() .severity(Severity.WARNING) .shape(operation) - .id(OPERATION_INPUT_OUTPUT_NAME) + .id(String.join(".", OPERATION_INPUT_OUTPUT_NAME, property, target.toString())) .message(String.format( "The %s of this operation should target a shape that starts with the operation's name, '%s', " + "but the targeted shape is `%s`", property, operation.getId().getName(), target)) diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/validation/validators/PaginatedTraitValidator.java b/smithy-model/src/main/java/software/amazon/smithy/model/validation/validators/PaginatedTraitValidator.java index 9f778b83b52..be1cc4db3e5 100644 --- a/smithy-model/src/main/java/software/amazon/smithy/model/validation/validators/PaginatedTraitValidator.java +++ b/smithy-model/src/main/java/software/amazon/smithy/model/validation/validators/PaginatedTraitValidator.java @@ -61,6 +61,9 @@ public final class PaginatedTraitValidator extends AbstractValidator { private static final Set TOKEN_SHAPES = SetUtils.of(ShapeType.STRING, ShapeType.MAP); private static final Set DANGER_TOKEN_SHAPES = SetUtils.of(ShapeType.MAP); private static final Pattern PATH_PATTERN = Pattern.compile("\\."); + private static final String DEEPLY_NESTED = "DEEPLY_NESTED"; + private static final String SHOULD_NOT_BE_REQUIRED = "SHOULD_NOT_BE_REQUIRED"; + private static final String WRONG_SHAPE_TYPE = "WRONG_SHAPE_TYPE"; @Override public List validate(Model model) { @@ -91,9 +94,14 @@ private List validateOperation( events.addAll(validateMember(opIndex, model, null, operation, trait, pageSizeValidator)); pageSizeValidator.getMember(model, opIndex, operation, trait) .filter(MemberShape::isRequired) - .ifPresent(member -> events.add(warning(operation, trait, String.format( - "paginated trait `%s` member `%s` should not be required", - pageSizeValidator.propertyName(), member.getMemberName())))); + .ifPresent(member -> events.add(warning( + operation, + trait, + String.format( + "paginated trait `%s` member `%s` should not be required", + pageSizeValidator.propertyName(), member.getMemberName()), + pageSizeValidator.propertyName(), member.getMemberName(), SHOULD_NOT_BE_REQUIRED) + )); // Validate output. events.addAll(validateMember(opIndex, model, null, operation, trait, new OutputTokenValidator())); @@ -165,20 +173,25 @@ private List validateMember( if (validator.dangerTargets().contains(target.getType())) { Set preferredTargets = new TreeSet<>(validator.validTargets()); preferredTargets.removeAll(validator.dangerTargets()); + final String traitName = validator.propertyName(); + final String memberName = member.getId().getName(); + final String targetType = target.getType().toString(); events.add(danger(operation, trait, String.format( - "%spaginated trait `%s` member `%s` targets a %s shape, but this is not recommended. " - + "One of [%s] SHOULD be targeted.", - prefix, validator.propertyName(), member.getId().getName(), target.getType(), - ValidationUtils.tickedList(preferredTargets)))); + "%spaginated trait `%s` member `%s` targets a %s shape, but this is not recommended. " + + "One of [%s] SHOULD be targeted.", + prefix, traitName, memberName, targetType, ValidationUtils.tickedList(preferredTargets)), + traitName, memberName, WRONG_SHAPE_TYPE, targetType + )); } } if (validator.pathsAllowed() && PATH_PATTERN.split(memberPath).length > 2) { events.add(warning(operation, trait, String.format( - "%spaginated trait `%s` contains a path with more than two parts, which can make your API " - + "cumbersome to use", - prefix, validator.propertyName() - ))); + "%spaginated trait `%s` contains a path with more than two parts, which can make your API " + + "cumbersome to use", + prefix, validator.propertyName()), + validator.propertyName(), DEEPLY_NESTED + )); } return events; diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/validation/validators/TargetValidator.java b/smithy-model/src/main/java/software/amazon/smithy/model/validation/validators/TargetValidator.java index 26229574074..d6bee7b9b5e 100644 --- a/smithy-model/src/main/java/software/amazon/smithy/model/validation/validators/TargetValidator.java +++ b/smithy-model/src/main/java/software/amazon/smithy/model/validation/validators/TargetValidator.java @@ -204,7 +204,7 @@ private void validateDeprecatedTargets( deprecatedTrait.getMessage().ifPresent(message -> builder.append(". ").append(message)); deprecatedTrait.getSince().ifPresent(since -> builder.append(" (since ").append(since).append(')')); events.add(ValidationEvent.builder() - .id("DeprecatedShape") + .id(String.format("DeprecatedShape.%s.%s", relType.name(), target.getId())) .severity(Severity.WARNING) .shape(shape) .message(builder.toString()) diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/validation/validators/UnstableTraitValidator.java b/smithy-model/src/main/java/software/amazon/smithy/model/validation/validators/UnstableTraitValidator.java index bdac51a51fb..e17526ca4fc 100644 --- a/smithy-model/src/main/java/software/amazon/smithy/model/validation/validators/UnstableTraitValidator.java +++ b/smithy-model/src/main/java/software/amazon/smithy/model/validation/validators/UnstableTraitValidator.java @@ -35,8 +35,12 @@ public List validate(Model model) { for (Shape trait : model.getShapesWithTrait(UnstableTrait.class)) { for (Shape appliedTo : model.getShapesWithTrait(trait)) { - events.add(warning(appliedTo, trait, format( - "This shape applies a trait that is unstable: %s", trait.toShapeId()))); + events.add(warning( + appliedTo, + trait, + format("This shape applies a trait that is unstable: %s", trait.toShapeId()), + trait.toShapeId().toString()) + ); } } diff --git a/smithy-model/src/test/java/software/amazon/smithy/model/ShapeMatcher.java b/smithy-model/src/test/java/software/amazon/smithy/model/ShapeMatcher.java index 3f98741b430..f7531792164 100644 --- a/smithy-model/src/test/java/software/amazon/smithy/model/ShapeMatcher.java +++ b/smithy-model/src/test/java/software/amazon/smithy/model/ShapeMatcher.java @@ -117,7 +117,7 @@ private static boolean findEvent( for (ValidationEvent event : result.getValidationEvents()) { if (event.getShapeId().filter(sid -> sid.equals(id.toShapeId())).isPresent() && event.getSeverity() == severity - && event.getId().equals(eventId) + && event.containsId(eventId) && event.getMessage().contains(contains)) { return true; } diff --git a/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/annotation-trait-extra-properties.errors b/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/annotation-trait-extra-properties.errors index 3c01e7d2fdf..7192f56e252 100644 --- a/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/annotation-trait-extra-properties.errors +++ b/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/annotation-trait-extra-properties.errors @@ -1 +1 @@ -[WARNING] ns.foo#Baz: Error validating trait `sensitive`: Invalid structure member `test` found for `smithy.api#sensitive` | TraitValue +[WARNING] ns.foo#Baz: Error validating trait `sensitive`: Invalid structure member `test` found for `smithy.api#sensitive` | TraitValue.smithy.api#sensitive.test diff --git a/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/deprecated-shape/deprecated-shape.errors b/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/deprecated-shape/deprecated-shape.errors index 892a168ed4c..7ff88e33459 100644 --- a/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/deprecated-shape/deprecated-shape.errors +++ b/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/deprecated-shape/deprecated-shape.errors @@ -1,4 +1,4 @@ -[WARNING] smithy.example#Test$foo1: Member targets a deprecated shape, smithy.example#Foo1 | DeprecatedShape -[WARNING] smithy.example#Test$foo2: Member targets a deprecated shape, smithy.example#Foo2 (since 2.0) | DeprecatedShape -[WARNING] smithy.example#Test$foo3: Member targets a deprecated shape, smithy.example#Foo3. hello (since 2.0) | DeprecatedShape -[WARNING] smithy.example#Test: Applies a deprecated mixin, smithy.example#DeprecatedMixin | DeprecatedShape +[WARNING] smithy.example#Test$foo1: Member targets a deprecated shape, smithy.example#Foo1 | DeprecatedShape.MEMBER_TARGET.smithy.example#Foo1 +[WARNING] smithy.example#Test$foo2: Member targets a deprecated shape, smithy.example#Foo2 (since 2.0) | DeprecatedShape.MEMBER_TARGET.smithy.example#Foo2 +[WARNING] smithy.example#Test$foo3: Member targets a deprecated shape, smithy.example#Foo3. hello (since 2.0) | DeprecatedShape.MEMBER_TARGET.smithy.example#Foo3 +[WARNING] smithy.example#Test: Applies a deprecated mixin, smithy.example#DeprecatedMixin | DeprecatedShape.MIXIN.smithy.example#DeprecatedMixin diff --git a/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/enums/enum-trait-validation.errors b/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/enums/enum-trait-validation.errors index b6bf1525082..e0c11741acc 100644 --- a/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/enums/enum-trait-validation.errors +++ b/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/enums/enum-trait-validation.errors @@ -1,9 +1,9 @@ [WARNING] ns.foo#ValidNoName: Enums should define the `name` property to allow rich types to be generated in code generators. | EnumNamesPresent -[WARNING] ns.foo#Warn1: The name `_bar` does not match the recommended enum name format of beginning with an uppercase letter, followed by any number of uppercase letters, numbers, or underscores. | EnumTrait -[WARNING] ns.foo#Warn1: The name `baz` does not match the recommended enum name format of beginning with an uppercase letter, followed by any number of uppercase letters, numbers, or underscores. | EnumTrait -[WARNING] ns.foo#Invalid2: The name `invalid!` does not match the recommended enum name format of beginning with an uppercase letter, followed by any number of uppercase letters, numbers, or underscores. | EnumTrait -[WARNING] ns.foo#Invalid2: The name `invalid2!` does not match the recommended enum name format of beginning with an uppercase letter, followed by any number of uppercase letters, numbers, or underscores. | EnumTrait -[WARNING] ns.foo#Invalid3: The name `a` does not match the recommended enum name format of beginning with an uppercase letter, followed by any number of uppercase letters, numbers, or underscores. | EnumTrait +[WARNING] ns.foo#Warn1: The name `_bar` does not match the recommended enum name format of beginning with an uppercase letter, followed by any number of uppercase letters, numbers, or underscores. | EnumTrait._bar +[WARNING] ns.foo#Warn1: The name `baz` does not match the recommended enum name format of beginning with an uppercase letter, followed by any number of uppercase letters, numbers, or underscores. | EnumTrait.baz +[WARNING] ns.foo#Invalid2: The name `invalid!` does not match the recommended enum name format of beginning with an uppercase letter, followed by any number of uppercase letters, numbers, or underscores. | EnumTrait.invalid! +[WARNING] ns.foo#Invalid2: The name `invalid2!` does not match the recommended enum name format of beginning with an uppercase letter, followed by any number of uppercase letters, numbers, or underscores. | EnumTrait.invalid2! +[WARNING] ns.foo#Invalid3: The name `a` does not match the recommended enum name format of beginning with an uppercase letter, followed by any number of uppercase letters, numbers, or underscores. | EnumTrait.a [ERROR] ns.foo#Invalid1: `foo` enum value body is missing the `name` property; if any enum trait value contains a `name` property, then all values must contain the `name` property. | EnumTrait [ERROR] ns.foo#Invalid2: Error validating trait `enum`.0.name: String value provided for `smithy.api#EnumConstantBodyName` must match regular expression: ^[a-zA-Z_]+[a-zA-Z_0-9]*$ | TraitValue [ERROR] ns.foo#Invalid2: Error validating trait `enum`.1.name: String value provided for `smithy.api#EnumConstantBodyName` must match regular expression: ^[a-zA-Z_]+[a-zA-Z_0-9]*$ | TraitValue diff --git a/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/http-method-semantics-validator.errors b/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/http-method-semantics-validator.errors index b8a1123b292..25c6b8d3271 100644 --- a/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/http-method-semantics-validator.errors +++ b/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/http-method-semantics-validator.errors @@ -1,7 +1,8 @@ -[DANGER] ns.foo#K: This operation uses the `GET` method in the `http` trait, but the `payload` member is sent as the payload of the request because it is marked with the `httpPayload` trait. Many HTTP clients do not support payloads with GET requests. Consider binding this member to other parts of the HTTP request such as a query string parameter using the `httpQuery` trait, a header using the `httpHeader` trait, or a path segment using the `httpLabel` trait. | HttpMethodSemantics -[DANGER] ns.foo#L: This operation uses the `GET` method in the `http` trait, but the following members are sent as part of the payload of the request: `payload`. These members are sent as part of the payload because they are not explicitly configured to be sent in headers, in the query string, or in a URI segment. Many HTTP clients do not support payloads with GET requests. Consider binding these members to other parts of the HTTP request such as query string parameters using the `httpQuery` trait, headers using the `httpHeader` trait, or URI segments using the `httpLabel` trait. | HttpMethodSemantics -[DANGER] ns.foo#M: This operation uses the `DELETE` method in the `http` trait, but the following members are sent as part of the payload of the request: `payload`. These members are sent as part of the payload because they are not explicitly configured to be sent in headers, in the query string, or in a URI segment. Many HTTP clients do not support payloads with DELETE requests. Consider binding these members to other parts of the HTTP request such as query string parameters using the `httpQuery` trait, headers using the `httpHeader` trait, or URI segments using the `httpLabel` trait. | HttpMethodSemantics -[WARNING] ns.foo#G: This operation uses the `POST` method in the `http` trait, but is marked with the readonly trait | HttpMethodSemantics -[WARNING] ns.foo#H: This operation uses the `DELETE` method in the `http` trait, but is not marked with the idempotent trait | HttpMethodSemantics -[WARNING] ns.foo#I: This operation uses the `GET` method in the `http` trait, but is not marked with the readonly trait | HttpMethodSemantics -[WARNING] ns.foo#Options: OPTIONS requests are typically used as part of CORS and should not be modeled explicitly. They are an implementation detail that should not appear in generated client or server code. Instead, tooling should use the `cors` trait on a service to automatically configure CORS on clients and servers. It is the responsibility of service frameworks and API gateways to automatically manage OPTIONS requests. For example, OPTIONS requests are automatically created when using Smithy models with Amazon API Gateway. | HttpMethodSemantics +[DANGER] ns.foo#K: This operation uses the `GET` method in the `http` trait, but the `payload` member is sent as the payload of the request because it is marked with the `httpPayload` trait. Many HTTP clients do not support payloads with GET requests. Consider binding this member to other parts of the HTTP request such as a query string parameter using the `httpQuery` trait, a header using the `httpHeader` trait, or a path segment using the `httpLabel` trait. | HttpMethodSemantics.GET.payload +[DANGER] ns.foo#L: This operation uses the `GET` method in the `http` trait, but the following members are sent as part of the payload of the request: `payload`. These members are sent as part of the payload because they are not explicitly configured to be sent in headers, in the query string, or in a URI segment. Many HTTP clients do not support payloads with GET requests. Consider binding these members to other parts of the HTTP request such as query string parameters using the `httpQuery` trait, headers using the `httpHeader` trait, or URI segments using the `httpLabel` trait. | HttpMethodSemantics.GET.payload +[DANGER] ns.foo#L: This operation uses the `GET` method in the `http` trait, but the following members are sent as part of the payload of the request: `morePayload`. These members are sent as part of the payload because they are not explicitly configured to be sent in headers, in the query string, or in a URI segment. Many HTTP clients do not support payloads with GET requests. Consider binding these members to other parts of the HTTP request such as query string parameters using the `httpQuery` trait, headers using the `httpHeader` trait, or URI segments using the `httpLabel` trait. | HttpMethodSemantics.GET.morePayload +[DANGER] ns.foo#M: This operation uses the `DELETE` method in the `http` trait, but the following members are sent as part of the payload of the request: `payload`. These members are sent as part of the payload because they are not explicitly configured to be sent in headers, in the query string, or in a URI segment. Many HTTP clients do not support payloads with DELETE requests. Consider binding these members to other parts of the HTTP request such as query string parameters using the `httpQuery` trait, headers using the `httpHeader` trait, or URI segments using the `httpLabel` trait. | HttpMethodSemantics.DELETE.payload +[WARNING] ns.foo#G: This operation uses the `POST` method in the `http` trait, but is marked with the readonly trait | HttpMethodSemantics.POST.smithy.api#readonly +[WARNING] ns.foo#H: This operation uses the `DELETE` method in the `http` trait, but is not marked with the idempotent trait | HttpMethodSemantics.DELETE.smithy.api#idempotent +[WARNING] ns.foo#I: This operation uses the `GET` method in the `http` trait, but is not marked with the readonly trait | HttpMethodSemantics.GET.smithy.api#readonly +[WARNING] ns.foo#Options: OPTIONS requests are typically used as part of CORS and should not be modeled explicitly. They are an implementation detail that should not appear in generated client or server code. Instead, tooling should use the `cors` trait on a service to automatically configure CORS on clients and servers. It is the responsibility of service frameworks and API gateways to automatically manage OPTIONS requests. For example, OPTIONS requests are automatically created when using Smithy models with Amazon API Gateway. | HttpMethodSemantics.OPTIONS diff --git a/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/http-method-semantics-validator.json b/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/http-method-semantics-validator.json index 8722f7743b4..d55c1449aa7 100644 --- a/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/http-method-semantics-validator.json +++ b/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/http-method-semantics-validator.json @@ -152,6 +152,12 @@ "traits": { "smithy.api#required": {} } + }, + "morePayload": { + "target": "smithy.api#String", + "traits": { + "smithy.api#required": {} + } } }, "traits": { diff --git a/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/operation/input-output-traits/invalid-multiple-operations-same-shape.errors b/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/operation/input-output-traits/invalid-multiple-operations-same-shape.errors index b34f96ffa50..0bb9368717e 100644 --- a/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/operation/input-output-traits/invalid-multiple-operations-same-shape.errors +++ b/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/operation/input-output-traits/invalid-multiple-operations-same-shape.errors @@ -1,4 +1,4 @@ -[WARNING] smithy.example#GetFoo2: The input of this operation should target a shape that starts with the operation's name, 'GetFoo2', but the targeted shape is `smithy.example#GetFooInput` | OperationInputOutputName -[WARNING] smithy.example#GetFoo2: The output of this operation should target a shape that starts with the operation's name, 'GetFoo2', but the targeted shape is `smithy.example#GetFooOutput` | OperationInputOutputName +[WARNING] smithy.example#GetFoo2: The input of this operation should target a shape that starts with the operation's name, 'GetFoo2', but the targeted shape is `smithy.example#GetFooInput` | OperationInputOutputName.input.smithy.example#GetFooInput +[WARNING] smithy.example#GetFoo2: The output of this operation should target a shape that starts with the operation's name, 'GetFoo2', but the targeted shape is `smithy.example#GetFooOutput` | OperationInputOutputName.output.smithy.example#GetFooOutput [ERROR] smithy.example#GetFooInput: Shapes marked with the @input trait cannot be used as input by multiple operations: `smithy.example#GetFoo`, `smithy.example#GetFoo2` | OperationInputOutputMisuse [ERROR] smithy.example#GetFooOutput: Shapes marked with the @output trait cannot be used as output by multiple operations: `smithy.example#GetFoo`, `smithy.example#GetFoo2` | OperationInputOutputMisuse diff --git a/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/paginated/paginated-deeply-nested.errors b/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/paginated/paginated-deeply-nested.errors index 4e034982a89..3a1c681c117 100644 --- a/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/paginated/paginated-deeply-nested.errors +++ b/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/paginated/paginated-deeply-nested.errors @@ -1,2 +1,2 @@ -[WARNING] ns.foo#DeeplyNested: paginated trait `items` contains a path with more than two parts, which can make your API cumbersome to use | PaginatedTrait -[WARNING] ns.foo#DeeplyNested: paginated trait `outputToken` contains a path with more than two parts, which can make your API cumbersome to use | PaginatedTrait +[WARNING] ns.foo#DeeplyNested: paginated trait `items` contains a path with more than two parts, which can make your API cumbersome to use | PaginatedTrait.items.DEEPLY_NESTED +[WARNING] ns.foo#DeeplyNested: paginated trait `outputToken` contains a path with more than two parts, which can make your API cumbersome to use | PaginatedTrait.outputToken.DEEPLY_NESTED diff --git a/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/paginated/paginated-invalid.errors b/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/paginated/paginated-invalid.errors index 3a9ce8f2a3f..545476d3fd1 100644 --- a/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/paginated/paginated-invalid.errors +++ b/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/paginated/paginated-invalid.errors @@ -5,7 +5,7 @@ [ERROR] ns.foo#Invalid3: paginated trait `outputToken` member `OutputNotString` targets a integer shape, but must target one of the following: [`map`, `string`] | PaginatedTrait [ERROR] ns.foo#Invalid4: paginated trait `inputToken` member `nextToken` must not be required | PaginatedTrait [ERROR] ns.foo#Invalid4: paginated trait `outputToken` member `nextToken` must not be required | PaginatedTrait -[WARNING] ns.foo#Invalid4: paginated trait `pageSize` member `pageSize` should not be required | PaginatedTrait +[WARNING] ns.foo#Invalid4: paginated trait `pageSize` member `pageSize` should not be required | PaginatedTrait.pageSize.pageSize.SHOULD_NOT_BE_REQUIRED [ERROR] ns.foo#Invalid5: paginated trait `pageSize` member `Invalid5Input` targets a string shape, but must target one of the following: [`integer`] | PaginatedTrait [ERROR] ns.foo#Invalid6: paginated trait `items` member `Invalid6Output` targets a string shape, but must target one of the following: [`list`, `map`] | PaginatedTrait [ERROR] ns.foo#Invalid7: paginated trait `items` targets a member `items` that does not exist | PaginatedTrait diff --git a/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/paginated/paginated-map-tokens.errors b/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/paginated/paginated-map-tokens.errors index 4a47be1d7c5..2b5dba872eb 100644 --- a/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/paginated/paginated-map-tokens.errors +++ b/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/paginated/paginated-map-tokens.errors @@ -1,2 +1,2 @@ -[DANGER] ns.foo#MapTokens: paginated trait `inputToken` member `MapTokensInput` targets a map shape, but this is not recommended. One of [`string`] SHOULD be targeted. | PaginatedTrait -[DANGER] ns.foo#MapTokens: paginated trait `outputToken` member `MapTokensOutput` targets a map shape, but this is not recommended. One of [`string`] SHOULD be targeted. | PaginatedTrait +[DANGER] ns.foo#MapTokens: paginated trait `inputToken` member `MapTokensInput` targets a map shape, but this is not recommended. One of [`string`] SHOULD be targeted. | PaginatedTrait.inputToken.MapTokensInput.WRONG_SHAPE_TYPE.map +[DANGER] ns.foo#MapTokens: paginated trait `outputToken` member `MapTokensOutput` targets a map shape, but this is not recommended. One of [`string`] SHOULD be targeted. | PaginatedTrait.outputToken.MapTokensOutput.WRONG_SHAPE_TYPE.map diff --git a/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/trait-value/extra-trait-property.errors b/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/trait-value/extra-trait-property.errors index c8614d5eef9..b6e1dacbe9c 100644 --- a/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/trait-value/extra-trait-property.errors +++ b/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/trait-value/extra-trait-property.errors @@ -1 +1 @@ -[WARNING] smithy.example#MyInteger: Error validating trait `range`: Invalid structure member `maxx` found for `smithy.api#range` | TraitValue +[WARNING] smithy.example#MyInteger: Error validating trait `range`: Invalid structure member `maxx` found for `smithy.api#range` | TraitValue.smithy.api#range.maxx diff --git a/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/unstable-trait-validator.errors b/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/unstable-trait-validator.errors index e8527d0a44f..e4b1405b9a1 100644 --- a/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/unstable-trait-validator.errors +++ b/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/unstable-trait-validator.errors @@ -1 +1 @@ -[WARNING] ns.foo#MyString: This shape applies a trait that is unstable: ns.foo#fooTrait | UnstableTrait +[WARNING] ns.foo#MyString: This shape applies a trait that is unstable: ns.foo#fooTrait | UnstableTrait.ns.foo#fooTrait diff --git a/smithy-waiters/src/main/java/software/amazon/smithy/waiters/WaiterMatcherValidator.java b/smithy-waiters/src/main/java/software/amazon/smithy/waiters/WaiterMatcherValidator.java index 59cdc3f68e6..7d3245902da 100644 --- a/smithy-waiters/src/main/java/software/amazon/smithy/waiters/WaiterMatcherValidator.java +++ b/smithy-waiters/src/main/java/software/amazon/smithy/waiters/WaiterMatcherValidator.java @@ -94,9 +94,10 @@ public List visitErrorType(Matcher.ErrorTypeMember errorType) { } } - addEvent(Severity.WARNING, INVALID_ERROR_TYPE, String.format( + addEvent(Severity.WARNING, String.format( "errorType '%s' not found on operation. This operation defines the following errors: %s", - error, operation.getErrors())); + error, operation.getErrors()), + INVALID_ERROR_TYPE, waiterName, String.valueOf(acceptorIndex), error); return events; } @@ -114,10 +115,11 @@ private void validatePathMatcher(LiteralExpression input, PathMatcher pathMatche case BOOLEAN_EQUALS: // A booleanEquals comparator requires an `expected` value of "true" or "false". if (!pathMatcher.getExpected().equals("true") && !pathMatcher.getExpected().equals("false")) { - addEvent(Severity.ERROR, NON_SUPPRESSABLE_ERROR, String.format( + addEvent(Severity.ERROR, String.format( "Waiter acceptors with a %s comparator must set their `expected` value to 'true' or " - + "'false', but found '%s'.", - PathComparator.BOOLEAN_EQUALS, pathMatcher.getExpected())); + + "'false', but found '%s'.", + PathComparator.BOOLEAN_EQUALS, pathMatcher.getExpected()), + NON_SUPPRESSABLE_ERROR); } validateReturnType(pathMatcher.getComparator(), RuntimeType.BOOLEAN, returnType); break; @@ -138,18 +140,21 @@ private RuntimeType validatePath(LiteralExpression input, String path) { } return result.getReturnType(); } catch (JmespathException e) { - addEvent(Severity.ERROR, NON_SUPPRESSABLE_ERROR, String.format( - "Invalid JMESPath expression (%s): %s", path, e.getMessage())); + addEvent(Severity.ERROR, String.format( + "Invalid JMESPath expression (%s): %s", path, e.getMessage()), + NON_SUPPRESSABLE_ERROR); return RuntimeType.ANY; } } private void validateReturnType(PathComparator comparator, RuntimeType expected, RuntimeType actual) { if (actual != RuntimeType.ANY && actual != expected) { - addEvent(Severity.DANGER, JMESPATH_PROBLEM, String.format( + addEvent(Severity.DANGER, String.format( "Waiter acceptors with a %s comparator must return a `%s` type, but this acceptor was " - + "statically determined to return a `%s` type.", - comparator, expected, actual)); + + "statically determined to return a `%s` type.", + comparator, expected, actual), + JMESPATH_PROBLEM, waiterName, String.valueOf(acceptorIndex), comparator.toString(), + actual.toString()); } } @@ -175,13 +180,14 @@ private void addJmespathEvent(String path, ExpressionProblem problem) { } String problemMessage = problem.message + " (" + problem.line + ":" + problem.column + ")"; - addEvent(severity, severity == Severity.ERROR ? NON_SUPPRESSABLE_ERROR : JMESPATH_PROBLEM, String.format( - "Problem found in JMESPath expression (%s): %s", path, problemMessage)); + addEvent(severity, String.format("Problem found in JMESPath expression (%s): %s", path, problemMessage), + severity == Severity.ERROR ? NON_SUPPRESSABLE_ERROR : JMESPATH_PROBLEM, waiterName, + String.valueOf(acceptorIndex)); } - private void addEvent(Severity severity, String id, String message) { + private void addEvent(Severity severity, String message, String... eventIdParts) { events.add(ValidationEvent.builder() - .id(id) + .id(String.join(".", eventIdParts)) .shape(operation) .sourceLocation(waitable) .severity(severity) diff --git a/smithy-waiters/src/test/resources/software/amazon/smithy/waiters/errorfiles/emits-danger-and-warning-typechecks.errors b/smithy-waiters/src/test/resources/software/amazon/smithy/waiters/errorfiles/emits-danger-and-warning-typechecks.errors index 218f8ee06a7..44d6efccf0e 100644 --- a/smithy-waiters/src/test/resources/software/amazon/smithy/waiters/errorfiles/emits-danger-and-warning-typechecks.errors +++ b/smithy-waiters/src/test/resources/software/amazon/smithy/waiters/errorfiles/emits-danger-and-warning-typechecks.errors @@ -1,3 +1,3 @@ -[DANGER] smithy.example#A: Waiter `Invalid1`, acceptor 0: Problem found in JMESPath expression (`10`.foo): Object field 'foo' extraction performed on number (1:6) | WaitableTraitJmespathProblem -[DANGER] smithy.example#A: Waiter `Invalid2`, acceptor 0: Waiter acceptors with a booleanEquals comparator must return a `boolean` type, but this acceptor was statically determined to return a `null` type. | WaitableTraitJmespathProblem +[DANGER] smithy.example#A: Waiter `Invalid1`, acceptor 0: Problem found in JMESPath expression (`10`.foo): Object field 'foo' extraction performed on number (1:6) | WaitableTraitJmespathProblem.Invalid1.0 +[DANGER] smithy.example#A: Waiter `Invalid2`, acceptor 0: Waiter acceptors with a booleanEquals comparator must return a `boolean` type, but this acceptor was statically determined to return a `null` type. | WaitableTraitJmespathProblem.Invalid2.0.booleanEquals.null [WARNING] smithy.example#A: Waiter `Invalid2`, acceptor 0: Problem found in JMESPath expression (`true` < `false`): Invalid comparator '<' for boolean (1:10) | WaitableTraitJmespathProblem diff --git a/smithy-waiters/src/test/resources/software/amazon/smithy/waiters/errorfiles/invalid-errorType.errors b/smithy-waiters/src/test/resources/software/amazon/smithy/waiters/errorfiles/invalid-errorType.errors index 8e64d042b84..74b19f01895 100644 --- a/smithy-waiters/src/test/resources/software/amazon/smithy/waiters/errorfiles/invalid-errorType.errors +++ b/smithy-waiters/src/test/resources/software/amazon/smithy/waiters/errorfiles/invalid-errorType.errors @@ -1 +1 @@ -[WARNING] smithy.example#A: Waiter `A`, acceptor 0: errorType 'Nope' not found on operation. This operation defines the following errors: [smithy.example#OhNo] | WaitableTraitInvalidErrorType +[WARNING] smithy.example#A: Waiter `A`, acceptor 0: errorType 'Nope' not found on operation. This operation defines the following errors: [smithy.example#OhNo] | WaitableTraitInvalidErrorType.A.0.Nope diff --git a/smithy-waiters/src/test/resources/software/amazon/smithy/waiters/errorfiles/invalid-jmespath-syntax.errors b/smithy-waiters/src/test/resources/software/amazon/smithy/waiters/errorfiles/invalid-jmespath-syntax.errors index 763428c7f91..d8a318aa4e8 100644 --- a/smithy-waiters/src/test/resources/software/amazon/smithy/waiters/errorfiles/invalid-jmespath-syntax.errors +++ b/smithy-waiters/src/test/resources/software/amazon/smithy/waiters/errorfiles/invalid-jmespath-syntax.errors @@ -1,3 +1,3 @@ [ERROR] smithy.example#A: Waiter `Invalid1`, acceptor 0: Invalid JMESPath expression (||): Syntax error | WaitableTrait [ERROR] smithy.example#A: Waiter `Invalid2`, acceptor 0: Problem found in JMESPath expression (length(`10`)): length function argument 0 error | WaitableTrait -[DANGER] smithy.example#A: Waiter `Invalid2`, acceptor 0: Waiter acceptors with a booleanEquals comparator must return a `boolean` type, but this acceptor was statically determined to return a `number` type. | WaitableTraitJmespathProblem +[DANGER] smithy.example#A: Waiter `Invalid2`, acceptor 0: Waiter acceptors with a booleanEquals comparator must return a `boolean` type, but this acceptor was statically determined to return a `number` type. | WaitableTraitJmespathProblem.Invalid2.0.booleanEquals.number diff --git a/smithy-waiters/src/test/resources/software/amazon/smithy/waiters/errorfiles/invalid-return-types.errors b/smithy-waiters/src/test/resources/software/amazon/smithy/waiters/errorfiles/invalid-return-types.errors index 41be4b1669d..7c7a538f9d0 100644 --- a/smithy-waiters/src/test/resources/software/amazon/smithy/waiters/errorfiles/invalid-return-types.errors +++ b/smithy-waiters/src/test/resources/software/amazon/smithy/waiters/errorfiles/invalid-return-types.errors @@ -1,4 +1,4 @@ -[DANGER] smithy.example#A: Waiter `Invalid1`, acceptor 0: Waiter acceptors with a booleanEquals comparator must return a `boolean` type, but this acceptor was statically determined to return a `number` type. | WaitableTraitJmespathProblem -[DANGER] smithy.example#A: Waiter `Invalid1`, acceptor 1: Waiter acceptors with a stringEquals comparator must return a `string` type, but this acceptor was statically determined to return a `number` type. | WaitableTraitJmespathProblem -[DANGER] smithy.example#A: Waiter `Invalid1`, acceptor 2: Waiter acceptors with a allStringEquals comparator must return a `array` type, but this acceptor was statically determined to return a `number` type. | WaitableTraitJmespathProblem -[DANGER] smithy.example#A: Waiter `Invalid1`, acceptor 3: Waiter acceptors with a anyStringEquals comparator must return a `array` type, but this acceptor was statically determined to return a `number` type. | WaitableTraitJmespathProblem +[DANGER] smithy.example#A: Waiter `Invalid1`, acceptor 0: Waiter acceptors with a booleanEquals comparator must return a `boolean` type, but this acceptor was statically determined to return a `number` type. | WaitableTraitJmespathProblem.Invalid1.0.booleanEquals.number +[DANGER] smithy.example#A: Waiter `Invalid1`, acceptor 1: Waiter acceptors with a stringEquals comparator must return a `string` type, but this acceptor was statically determined to return a `number` type. | WaitableTraitJmespathProblem.Invalid1.1.stringEquals.number +[DANGER] smithy.example#A: Waiter `Invalid1`, acceptor 2: Waiter acceptors with a allStringEquals comparator must return a `array` type, but this acceptor was statically determined to return a `number` type. | WaitableTraitJmespathProblem.Invalid1.2.allStringEquals.number +[DANGER] smithy.example#A: Waiter `Invalid1`, acceptor 3: Waiter acceptors with a anyStringEquals comparator must return a `array` type, but this acceptor was statically determined to return a `number` type. | WaitableTraitJmespathProblem.Invalid1.3.anyStringEquals.number From 4c8251c3ff77f47d634f023f3461a85bb5bec206 Mon Sep 17 00:00:00 2001 From: Richard Chen Date: Fri, 23 Dec 2022 11:53:00 -0800 Subject: [PATCH 2/7] address comments, mostly to simplify some of the eventIds --- .../traits/HttpChecksumTraitValidator.java | 4 +- .../http-checksum-header-conflicts.errors | 12 ++--- .../InputOutputStructureReuseValidator.java | 8 +-- .../linters/NoninclusiveTermsValidator.java | 17 +++--- .../input-output-structure-reuse.errors | 4 +- .../noninclusive-term-filter.errors | 26 +++++----- .../HttpMethodSemanticsValidator.java | 52 ++++++++++--------- .../validators/OperationValidator.java | 2 +- .../validators/PaginatedTraitValidator.java | 12 ++--- .../validators/TargetValidator.java | 2 +- .../deprecated-shape/deprecated-shape.errors | 8 +-- .../http-method-semantics-validator.errors | 13 +++-- ...alid-multiple-operations-same-shape.errors | 4 +- .../paginated/paginated-deeply-nested.errors | 4 +- .../paginated/paginated-invalid.errors | 2 +- .../paginated/paginated-map-tokens.errors | 4 +- 16 files changed, 89 insertions(+), 85 deletions(-) diff --git a/smithy-aws-traits/src/main/java/software/amazon/smithy/aws/traits/HttpChecksumTraitValidator.java b/smithy-aws-traits/src/main/java/software/amazon/smithy/aws/traits/HttpChecksumTraitValidator.java index ba6a93cf78d..9b2cf5f891e 100644 --- a/smithy-aws-traits/src/main/java/software/amazon/smithy/aws/traits/HttpChecksumTraitValidator.java +++ b/smithy-aws-traits/src/main/java/software/amazon/smithy/aws/traits/HttpChecksumTraitValidator.java @@ -191,7 +191,7 @@ private List validateHeaderConflicts(OperationShape operation, + " the prefix `%s` that conflicts with the prefix `%s` used by the" + " `httpChecksum` trait.", memberName, prefixString, HttpChecksumTrait.CHECKSUM_PREFIX), - "httpPrefixHeaders", memberName, prefixString)); + "HttpPrefixHeaders", memberName, prefixString)); } }); @@ -207,7 +207,7 @@ private List validateHeaderConflicts(OperationShape operation, events.add(warning(operation, format("The `httpHeader` binding of `%s` on `%s`" + " starts with the prefix `%s` used by the `httpChecksum` trait.", headerString, memberName, HttpChecksumTrait.CHECKSUM_PREFIX), - "httpHeader", memberName, headerString)); + "HttpHeader", memberName, headerString)); } }); } diff --git a/smithy-aws-traits/src/test/resources/software/amazon/smithy/aws/traits/errorfiles/http-checksum-header-conflicts.errors b/smithy-aws-traits/src/test/resources/software/amazon/smithy/aws/traits/errorfiles/http-checksum-header-conflicts.errors index 2b1539db708..3176f0fe82f 100644 --- a/smithy-aws-traits/src/test/resources/software/amazon/smithy/aws/traits/errorfiles/http-checksum-header-conflicts.errors +++ b/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.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- +[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- diff --git a/smithy-linters/src/main/java/software/amazon/smithy/linters/InputOutputStructureReuseValidator.java b/smithy-linters/src/main/java/software/amazon/smithy/linters/InputOutputStructureReuseValidator.java index 46c5f0f8abc..9bbc3b0010a 100644 --- a/smithy-linters/src/main/java/software/amazon/smithy/linters/InputOutputStructureReuseValidator.java +++ b/smithy-linters/src/main/java/software/amazon/smithy/linters/InputOutputStructureReuseValidator.java @@ -39,8 +39,8 @@ public Provider() { } } - private static final String INPUT = "INPUT"; - private static final String OUTPUT = "OUTPUT"; + private static final String INPUT = "Input"; + private static final String OUTPUT = "Output"; @Override public List validate(Model model) { @@ -66,14 +66,14 @@ private void validateInputOutputSet( + "@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()), - INPUT, operation.getId().toString())); + 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()), - OUTPUT, operation.getId().toString())); + OUTPUT, operation.getId().getName())); } } } diff --git a/smithy-linters/src/main/java/software/amazon/smithy/linters/NoninclusiveTermsValidator.java b/smithy-linters/src/main/java/software/amazon/smithy/linters/NoninclusiveTermsValidator.java index 62667ffd7a4..8673975dd1f 100644 --- a/smithy-linters/src/main/java/software/amazon/smithy/linters/NoninclusiveTermsValidator.java +++ b/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,9 +59,9 @@ public Provider() { } } - private static final String TRAIT = "TRAIT"; - private static final String SHAPE = "SHAPE"; - private static final String NAMESPACE = "NAMESPACE"; + private static final String TRAIT = "Trait"; + private static final String SHAPE = "Shape"; + private static final String NAMESPACE = "Namespace"; /** * NoninclusiveTermsValidator configuration. @@ -159,7 +160,7 @@ private ValidationEvent constructValidationEvent( return ValidationEvent.builder() .severity(Severity.WARNING) .sourceLocation(SourceLocation.none()) - .id(assembleFullEventId(NAMESPACE, instance.getText(), matchedText)) + .id(assembleFullEventId(NAMESPACE, instance.getText(), matchedText.toLowerCase(Locale.US))) .message(String.format("%s namespace uses a non-inclusive term `%s`.%s", instance.getText(), matchedText, replacementAddendum)) .build(); @@ -171,7 +172,7 @@ private ValidationEvent constructValidationEvent( return validationEvent.toBuilder() .message(String.format("'%s' trait has a value that contains a non-inclusive term `%s`.%s", idiomaticTraitName, matchedText, replacementAddendum)) - .id(assembleFullEventId(TRAIT, idiomaticTraitName, matchedText)) + .id(assembleFullEventId(TRAIT, matchedText.toLowerCase(Locale.US), idiomaticTraitName)) .build(); } else { final String valuePropertyPathFormatted = formatPropertyPath(instance.getTraitPropertyPath()); @@ -179,8 +180,8 @@ private ValidationEvent constructValidationEvent( .message(String.format( "'%s' trait value at path {%s} contains a non-inclusive term `%s`.%s", idiomaticTraitName, valuePropertyPathFormatted, matchedText, replacementAddendum)) - .id(assembleFullEventId( - TRAIT, idiomaticTraitName, valuePropertyPathFormatted, matchedText)) + .id(assembleFullEventId(TRAIT, matchedText.toLowerCase(Locale.US), idiomaticTraitName, + valuePropertyPathFormatted)) .build(); } case SHAPE: @@ -190,7 +191,7 @@ private ValidationEvent constructValidationEvent( String.format("%s shape uses a non-inclusive term `%s`.%s", StringUtils.capitalize(instance.getShape().getType().toString()), matchedText, replacementAddendum), - SHAPE, matchedText); + SHAPE, matchedText.toLowerCase(Locale.US)); } } diff --git a/smithy-linters/src/test/resources/software/amazon/smithy/linters/errorfiles/input-output-structure-reuse.errors b/smithy-linters/src/test/resources/software/amazon/smithy/linters/errorfiles/input-output-structure-reuse.errors index 2d588e73e27..630f04f8f10 100644 --- a/smithy-linters/src/test/resources/software/amazon/smithy/linters/errorfiles/input-output-structure-reuse.errors +++ b/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.INPUT.smithy.example#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.smithy.example#GetFoo +[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 diff --git a/smithy-linters/src/test/resources/software/amazon/smithy/linters/errorfiles/noninclusive-term-filter.errors b/smithy-linters/src/test/resources/software/amazon/smithy/linters/errorfiles/noninclusive-term-filter.errors index 4fe9ffe9674..50e535aeaee 100644 --- a/smithy-linters/src/test/resources/software/amazon/smithy/linters/errorfiles/noninclusive-term-filter.errors +++ b/smithy-linters/src/test/resources/software/amazon/smithy/linters/errorfiles/noninclusive-term-filter.errors @@ -1,13 +1,13 @@ -[WARNING] ns.foo#MyMasterService: Service shape uses a non-inclusive term `Master`. Consider using one of the following terms instead: `Main`, `Parent`, `Primary` | NoninclusiveTerms.SHAPE.Master -[WARNING] ns.foo#BlackListThings: Operation shape uses a non-inclusive term `BlackList`. Consider using one of the following terms instead: `DenyList` | NoninclusiveTerms.SHAPE.BlackList -[WARNING] ns.foo#MyWhitelistTrait: Structure shape uses a non-inclusive term `Whitelist`. Consider using one of the following terms instead: `AllowList` | NoninclusiveTerms.SHAPE.Whitelist -[WARNING] ns.foo#AInput$master_member_name: Member shape uses a non-inclusive term `master`. Consider using one of the following terms instead: `main`, `parent`, `primary` | NoninclusiveTerms.SHAPE.master -[WARNING] ns.foo#NestedTraitStructure$master_key_violation: Member shape uses a non-inclusive term `master`. Consider using one of the following terms instead: `main`, `parent`, `primary` | NoninclusiveTerms.SHAPE.master -[WARNING] ns.foo#AInput$foo: 'ns.foo#MySimpleValueTrait' trait has a value that contains a non-inclusive term `slave`. Consider using one of the following terms instead: `child`, `clone`, `replica`, `secondary` | NoninclusiveTerms.TRAIT.ns.foo#MySimpleValueTrait.slave -[WARNING] ns.foo#AOutput: 'ns.foo#MyWhitelistTrait' trait value at path {document/foo/0/bar} contains a non-inclusive term `whitelist`. Consider using one of the following terms instead: `allowList` | NoninclusiveTerms.TRAIT.ns.foo#MyWhitelistTrait.document/foo/0/bar.whitelist -[WARNING] ns.foo#AOutput: 'ns.foo#MyWhitelistTrait' trait value at path {nested/master_key_violation} contains a non-inclusive term `whitelist`. Consider using one of the following terms instead: `allowList` | NoninclusiveTerms.TRAIT.ns.foo#MyWhitelistTrait.nested/master_key_violation.whitelist -[WARNING] ns.foo#AOutput: 'ns.foo#MyWhitelistTrait' trait value at path {string_value} contains a non-inclusive term `whitelist`. Consider using one of the following terms instead: `allowList` | NoninclusiveTerms.TRAIT.ns.foo#MyWhitelistTrait.string_value.whitelist -[WARNING] ns.foo#AOutput: 'ns.foo#MyWhitelistTrait' trait value at path {document/whitelist_doc_key_violation_1} contains a non-inclusive term `whitelist`. Consider using one of the following terms instead: `allowList` | NoninclusiveTerms.TRAIT.ns.foo#MyWhitelistTrait.document/whitelist_doc_key_violation_1.whitelist -[WARNING] ns.foo#AOutput: 'ns.foo#MyWhitelistTrait' trait value at path {collection/2/key} contains a non-inclusive term `whitelist`. Consider using one of the following terms instead: `allowList` | NoninclusiveTerms.TRAIT.ns.foo#MyWhitelistTrait.collection/2/key.whitelist -[WARNING] ns.foo#AOutput: 'ns.foo#MyWhitelistTrait' trait value at path {collection/1/blacklist_key} contains a non-inclusive term `blacklist`. Consider using one of the following terms instead: `denyList` | NoninclusiveTerms.TRAIT.ns.foo#MyWhitelistTrait.collection/1/blacklist_key.blacklist -[WARNING] ns.foo#MyUnionTrait$int_whitelist: Member shape uses a non-inclusive term `whitelist`. Consider using one of the following terms instead: `allowList` | NoninclusiveTerms.SHAPE.whitelist +[WARNING] ns.foo#MyMasterService: Service shape uses a non-inclusive term `Master`. Consider using one of the following terms instead: `Main`, `Parent`, `Primary` | NoninclusiveTerms.Shape.master +[WARNING] ns.foo#BlackListThings: Operation shape uses a non-inclusive term `BlackList`. Consider using one of the following terms instead: `DenyList` | NoninclusiveTerms.Shape.blacklist +[WARNING] ns.foo#MyWhitelistTrait: Structure shape uses a non-inclusive term `Whitelist`. Consider using one of the following terms instead: `AllowList` | NoninclusiveTerms.Shape.whitelist +[WARNING] ns.foo#AInput$master_member_name: Member shape uses a non-inclusive term `master`. Consider using one of the following terms instead: `main`, `parent`, `primary` | NoninclusiveTerms.Shape.master +[WARNING] ns.foo#NestedTraitStructure$master_key_violation: Member shape uses a non-inclusive term `master`. Consider using one of the following terms instead: `main`, `parent`, `primary` | NoninclusiveTerms.Shape.master +[WARNING] ns.foo#AInput$foo: 'ns.foo#MySimpleValueTrait' trait has a value that contains a non-inclusive term `slave`. Consider using one of the following terms instead: `child`, `clone`, `replica`, `secondary` | NoninclusiveTerms.Trait.slave.ns.foo#MySimpleValueTrait +[WARNING] ns.foo#AOutput: 'ns.foo#MyWhitelistTrait' trait value at path {document/foo/0/bar} contains a non-inclusive term `whitelist`. Consider using one of the following terms instead: `allowList` | NoninclusiveTerms.Trait.whitelist.ns.foo#MyWhitelistTrait.document/foo/0/bar +[WARNING] ns.foo#AOutput: 'ns.foo#MyWhitelistTrait' trait value at path {nested/master_key_violation} contains a non-inclusive term `whitelist`. Consider using one of the following terms instead: `allowList` | NoninclusiveTerms.Trait.whitelist.ns.foo#MyWhitelistTrait.nested/master_key_violation +[WARNING] ns.foo#AOutput: 'ns.foo#MyWhitelistTrait' trait value at path {string_value} contains a non-inclusive term `whitelist`. Consider using one of the following terms instead: `allowList` | NoninclusiveTerms.Trait.whitelist.ns.foo#MyWhitelistTrait.string_value +[WARNING] ns.foo#AOutput: 'ns.foo#MyWhitelistTrait' trait value at path {document/whitelist_doc_key_violation_1} contains a non-inclusive term `whitelist`. Consider using one of the following terms instead: `allowList` | NoninclusiveTerms.Trait.whitelist.ns.foo#MyWhitelistTrait.document/whitelist_doc_key_violation_1 +[WARNING] ns.foo#AOutput: 'ns.foo#MyWhitelistTrait' trait value at path {collection/2/key} contains a non-inclusive term `whitelist`. Consider using one of the following terms instead: `allowList` | NoninclusiveTerms.Trait.whitelist.ns.foo#MyWhitelistTrait.collection/2/key +[WARNING] ns.foo#AOutput: 'ns.foo#MyWhitelistTrait' trait value at path {collection/1/blacklist_key} contains a non-inclusive term `blacklist`. Consider using one of the following terms instead: `denyList` | NoninclusiveTerms.Trait.blacklist.ns.foo#MyWhitelistTrait.collection/1/blacklist_key +[WARNING] ns.foo#MyUnionTrait$int_whitelist: Member shape uses a non-inclusive term `whitelist`. Consider using one of the following terms instead: `allowList` | NoninclusiveTerms.Shape.whitelist diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/validation/validators/HttpMethodSemanticsValidator.java b/smithy-model/src/main/java/software/amazon/smithy/model/validation/validators/HttpMethodSemanticsValidator.java index 20b8bd04fa0..e34024072a4 100644 --- a/smithy-model/src/main/java/software/amazon/smithy/model/validation/validators/HttpMethodSemanticsValidator.java +++ b/smithy-model/src/main/java/software/amazon/smithy/model/validation/validators/HttpMethodSemanticsValidator.java @@ -29,6 +29,7 @@ import software.amazon.smithy.model.traits.ReadonlyTrait; import software.amazon.smithy.model.validation.AbstractValidator; import software.amazon.smithy.model.validation.ValidationEvent; +import software.amazon.smithy.model.validation.ValidationUtils; import software.amazon.smithy.utils.MapUtils; /** @@ -66,6 +67,12 @@ public final class HttpMethodSemanticsValidator extends AbstractValidator { "PUT", new HttpMethodSemantics(false, true, true, null), "PATCH", new HttpMethodSemantics(false, null, true, null)); + private static final String UNEXPECTED_PAYLOAD = "UnexpectedPayload"; + private static final String MISSING_READONLY_TRAIT = "MissingReadonlyTrait"; + private static final String MISSING_IDEMPOTENT_TRAIT = "MissingIdempotentTrait"; + private static final String UNNECESSARY_READONLY_TRAIT = "UnnecessaryReadonlyTrait"; + private static final String UNNECESSARY_IDEMPOTENT_TRAIT = "UnnecessaryIdempotentTrait"; + @Override public List validate(Model model) { if (!model.isTraitApplied(HttpTrait.class)) { @@ -104,7 +111,7 @@ private List validateOperation( events.add(warning(shape, trait, String.format( "This operation uses the `%s` method in the `http` trait, but %s marked with the readonly trait", method, isReadonly ? "is" : "is not"), - method, ReadonlyTrait.ID.toString())); + isReadonly ? UNNECESSARY_READONLY_TRAIT : MISSING_READONLY_TRAIT)); } boolean isIdempotent = shape.getTrait(IdempotentTrait.class).isPresent(); @@ -112,38 +119,35 @@ private List validateOperation( events.add(warning(shape, trait, String.format( "This operation uses the `%s` method in the `http` trait, but %s marked with the idempotent trait", method, isIdempotent ? "is" : "is not"), - method, IdempotentTrait.ID.toString())); + isIdempotent ? UNNECESSARY_IDEMPOTENT_TRAIT : MISSING_IDEMPOTENT_TRAIT)); } List payloadBindings = bindingIndex.getRequestBindings(shape, HttpBinding.Location.PAYLOAD); List documentBindings = bindingIndex.getRequestBindings(shape, HttpBinding.Location.DOCUMENT); if (semantics.allowsRequestPayload != null && !semantics.allowsRequestPayload) { if (!payloadBindings.isEmpty()) { - final String payloadMember = payloadBindings.get(0).getMemberName(); events.add(danger(shape, trait, String.format( - "This operation uses the `%s` method in the `http` trait, but the `%s` member is sent as the " - + "payload of the request because it is marked with the `httpPayload` trait. Many HTTP " - + "clients do not support payloads with %1$s requests. Consider binding this member to " - + "other parts of the HTTP request such as a query string parameter using the `httpQuery` " - + "trait, a header using the `httpHeader` trait, or a path segment using the `httpLabel` " - + "trait.", - method, payloadMember), - method, payloadMember) + "This operation uses the `%s` method in the `http` trait, but the `%s` member is sent as the " + + "payload of the request because it is marked with the `httpPayload` trait. Many HTTP " + + "clients do not support payloads with %1$s requests. Consider binding this member to " + + "other parts of the HTTP request such as a query string parameter using the `httpQuery` " + + "trait, a header using the `httpHeader` trait, or a path segment using the `httpLabel` " + + "trait.", + method, payloadBindings.get(0).getMemberName()), + UNEXPECTED_PAYLOAD) ); } else if (!documentBindings.isEmpty()) { - for (HttpBinding payloadMember : documentBindings) { - events.add(danger(shape, trait, String.format( - "This operation uses the `%s` method in the `http` trait, but the following members " - + "are sent as part of the payload of the request: `%s`. These members are sent as part " - + "of the payload because they are not explicitly configured to be sent in headers, in the " - + "query string, or in a URI segment. Many HTTP clients do not support payloads with %1$s " - + "requests. Consider binding these members to other parts of the HTTP request such as " - + "query string parameters using the `httpQuery` trait, headers using the `httpHeader` " - + "trait, or URI segments using the `httpLabel` trait.", - method, payloadMember.getMemberName()), - method, payloadMember.getMemberName() - )); - } + events.add(danger(shape, trait, String.format( + "This operation uses the `%s` method in the `http` trait, but the following members " + + "are sent as part of the payload of the request: %s. These members are sent as part " + + "of the payload because they are not explicitly configured to be sent in headers, in the " + + "query string, or in a URI segment. Many HTTP clients do not support payloads with %1$s " + + "requests. Consider binding these members to other parts of the HTTP request such as " + + "query string parameters using the `httpQuery` trait, headers using the `httpHeader` " + + "trait, or URI segments using the `httpLabel` trait.", + method, ValidationUtils.tickedList(documentBindings.stream().map(HttpBinding::getMemberName))), + UNEXPECTED_PAYLOAD) + ); } } diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/validation/validators/OperationValidator.java b/smithy-model/src/main/java/software/amazon/smithy/model/validation/validators/OperationValidator.java index 6d743712a95..6ef51166812 100644 --- a/smithy-model/src/main/java/software/amazon/smithy/model/validation/validators/OperationValidator.java +++ b/smithy-model/src/main/java/software/amazon/smithy/model/validation/validators/OperationValidator.java @@ -141,7 +141,7 @@ private ValidationEvent emitBadInputOutputName(Shape operation, String property, return ValidationEvent.builder() .severity(Severity.WARNING) .shape(operation) - .id(String.join(".", OPERATION_INPUT_OUTPUT_NAME, property, target.toString())) + .id(String.join(".", OPERATION_INPUT_OUTPUT_NAME, property)) .message(String.format( "The %s of this operation should target a shape that starts with the operation's name, '%s', " + "but the targeted shape is `%s`", property, operation.getId().getName(), target)) diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/validation/validators/PaginatedTraitValidator.java b/smithy-model/src/main/java/software/amazon/smithy/model/validation/validators/PaginatedTraitValidator.java index be1cc4db3e5..c722e5c6740 100644 --- a/smithy-model/src/main/java/software/amazon/smithy/model/validation/validators/PaginatedTraitValidator.java +++ b/smithy-model/src/main/java/software/amazon/smithy/model/validation/validators/PaginatedTraitValidator.java @@ -61,9 +61,9 @@ public final class PaginatedTraitValidator extends AbstractValidator { private static final Set TOKEN_SHAPES = SetUtils.of(ShapeType.STRING, ShapeType.MAP); private static final Set DANGER_TOKEN_SHAPES = SetUtils.of(ShapeType.MAP); private static final Pattern PATH_PATTERN = Pattern.compile("\\."); - private static final String DEEPLY_NESTED = "DEEPLY_NESTED"; - private static final String SHOULD_NOT_BE_REQUIRED = "SHOULD_NOT_BE_REQUIRED"; - private static final String WRONG_SHAPE_TYPE = "WRONG_SHAPE_TYPE"; + private static final String DEEPLY_NESTED = "DeeplyNested"; + private static final String SHOULD_NOT_BE_REQUIRED = "ShouldNotBeRequired"; + private static final String WRONG_SHAPE_TYPE = "WrongShapeType"; @Override public List validate(Model model) { @@ -100,7 +100,7 @@ private List validateOperation( String.format( "paginated trait `%s` member `%s` should not be required", pageSizeValidator.propertyName(), member.getMemberName()), - pageSizeValidator.propertyName(), member.getMemberName(), SHOULD_NOT_BE_REQUIRED) + SHOULD_NOT_BE_REQUIRED, pageSizeValidator.propertyName()) )); // Validate output. @@ -180,7 +180,7 @@ private List validateMember( "%spaginated trait `%s` member `%s` targets a %s shape, but this is not recommended. " + "One of [%s] SHOULD be targeted.", prefix, traitName, memberName, targetType, ValidationUtils.tickedList(preferredTargets)), - traitName, memberName, WRONG_SHAPE_TYPE, targetType + WRONG_SHAPE_TYPE, traitName )); } } @@ -190,7 +190,7 @@ private List validateMember( "%spaginated trait `%s` contains a path with more than two parts, which can make your API " + "cumbersome to use", prefix, validator.propertyName()), - validator.propertyName(), DEEPLY_NESTED + DEEPLY_NESTED, validator.propertyName() )); } diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/validation/validators/TargetValidator.java b/smithy-model/src/main/java/software/amazon/smithy/model/validation/validators/TargetValidator.java index d6bee7b9b5e..f6f8090d875 100644 --- a/smithy-model/src/main/java/software/amazon/smithy/model/validation/validators/TargetValidator.java +++ b/smithy-model/src/main/java/software/amazon/smithy/model/validation/validators/TargetValidator.java @@ -204,7 +204,7 @@ private void validateDeprecatedTargets( deprecatedTrait.getMessage().ifPresent(message -> builder.append(". ").append(message)); deprecatedTrait.getSince().ifPresent(since -> builder.append(" (since ").append(since).append(')')); events.add(ValidationEvent.builder() - .id(String.format("DeprecatedShape.%s.%s", relType.name(), target.getId())) + .id(String.format("DeprecatedShape.%s", target.getId())) .severity(Severity.WARNING) .shape(shape) .message(builder.toString()) diff --git a/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/deprecated-shape/deprecated-shape.errors b/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/deprecated-shape/deprecated-shape.errors index 7ff88e33459..1d897b87456 100644 --- a/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/deprecated-shape/deprecated-shape.errors +++ b/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/deprecated-shape/deprecated-shape.errors @@ -1,4 +1,4 @@ -[WARNING] smithy.example#Test$foo1: Member targets a deprecated shape, smithy.example#Foo1 | DeprecatedShape.MEMBER_TARGET.smithy.example#Foo1 -[WARNING] smithy.example#Test$foo2: Member targets a deprecated shape, smithy.example#Foo2 (since 2.0) | DeprecatedShape.MEMBER_TARGET.smithy.example#Foo2 -[WARNING] smithy.example#Test$foo3: Member targets a deprecated shape, smithy.example#Foo3. hello (since 2.0) | DeprecatedShape.MEMBER_TARGET.smithy.example#Foo3 -[WARNING] smithy.example#Test: Applies a deprecated mixin, smithy.example#DeprecatedMixin | DeprecatedShape.MIXIN.smithy.example#DeprecatedMixin +[WARNING] smithy.example#Test$foo1: Member targets a deprecated shape, smithy.example#Foo1 | DeprecatedShape.smithy.example#Foo1 +[WARNING] smithy.example#Test$foo2: Member targets a deprecated shape, smithy.example#Foo2 (since 2.0) | DeprecatedShape.smithy.example#Foo2 +[WARNING] smithy.example#Test$foo3: Member targets a deprecated shape, smithy.example#Foo3. hello (since 2.0) | DeprecatedShape.smithy.example#Foo3 +[WARNING] smithy.example#Test: Applies a deprecated mixin, smithy.example#DeprecatedMixin | DeprecatedShape.smithy.example#DeprecatedMixin diff --git a/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/http-method-semantics-validator.errors b/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/http-method-semantics-validator.errors index 25c6b8d3271..e63b8308ae7 100644 --- a/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/http-method-semantics-validator.errors +++ b/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/http-method-semantics-validator.errors @@ -1,8 +1,7 @@ -[DANGER] ns.foo#K: This operation uses the `GET` method in the `http` trait, but the `payload` member is sent as the payload of the request because it is marked with the `httpPayload` trait. Many HTTP clients do not support payloads with GET requests. Consider binding this member to other parts of the HTTP request such as a query string parameter using the `httpQuery` trait, a header using the `httpHeader` trait, or a path segment using the `httpLabel` trait. | HttpMethodSemantics.GET.payload -[DANGER] ns.foo#L: This operation uses the `GET` method in the `http` trait, but the following members are sent as part of the payload of the request: `payload`. These members are sent as part of the payload because they are not explicitly configured to be sent in headers, in the query string, or in a URI segment. Many HTTP clients do not support payloads with GET requests. Consider binding these members to other parts of the HTTP request such as query string parameters using the `httpQuery` trait, headers using the `httpHeader` trait, or URI segments using the `httpLabel` trait. | HttpMethodSemantics.GET.payload -[DANGER] ns.foo#L: This operation uses the `GET` method in the `http` trait, but the following members are sent as part of the payload of the request: `morePayload`. These members are sent as part of the payload because they are not explicitly configured to be sent in headers, in the query string, or in a URI segment. Many HTTP clients do not support payloads with GET requests. Consider binding these members to other parts of the HTTP request such as query string parameters using the `httpQuery` trait, headers using the `httpHeader` trait, or URI segments using the `httpLabel` trait. | HttpMethodSemantics.GET.morePayload -[DANGER] ns.foo#M: This operation uses the `DELETE` method in the `http` trait, but the following members are sent as part of the payload of the request: `payload`. These members are sent as part of the payload because they are not explicitly configured to be sent in headers, in the query string, or in a URI segment. Many HTTP clients do not support payloads with DELETE requests. Consider binding these members to other parts of the HTTP request such as query string parameters using the `httpQuery` trait, headers using the `httpHeader` trait, or URI segments using the `httpLabel` trait. | HttpMethodSemantics.DELETE.payload -[WARNING] ns.foo#G: This operation uses the `POST` method in the `http` trait, but is marked with the readonly trait | HttpMethodSemantics.POST.smithy.api#readonly -[WARNING] ns.foo#H: This operation uses the `DELETE` method in the `http` trait, but is not marked with the idempotent trait | HttpMethodSemantics.DELETE.smithy.api#idempotent -[WARNING] ns.foo#I: This operation uses the `GET` method in the `http` trait, but is not marked with the readonly trait | HttpMethodSemantics.GET.smithy.api#readonly +[DANGER] ns.foo#K: This operation uses the `GET` method in the `http` trait, but the `payload` member is sent as the payload of the request because it is marked with the `httpPayload` trait. Many HTTP clients do not support payloads with GET requests. Consider binding this member to other parts of the HTTP request such as a query string parameter using the `httpQuery` trait, a header using the `httpHeader` trait, or a path segment using the `httpLabel` trait. | HttpMethodSemantics.UnexpectedPayload +[DANGER] ns.foo#L: This operation uses the `GET` method in the `http` trait, but the following members are sent as part of the payload of the request: `morePayload`, `payload`. These members are sent as part of the payload because they are not explicitly configured to be sent in headers, in the query string, or in a URI segment. Many HTTP clients do not support payloads with GET requests. Consider binding these members to other parts of the HTTP request such as query string parameters using the `httpQuery` trait, headers using the `httpHeader` trait, or URI segments using the `httpLabel` trait. | HttpMethodSemantics.UnexpectedPayload +[DANGER] ns.foo#M: This operation uses the `DELETE` method in the `http` trait, but the following members are sent as part of the payload of the request: `payload`. These members are sent as part of the payload because they are not explicitly configured to be sent in headers, in the query string, or in a URI segment. Many HTTP clients do not support payloads with DELETE requests. Consider binding these members to other parts of the HTTP request such as query string parameters using the `httpQuery` trait, headers using the `httpHeader` trait, or URI segments using the `httpLabel` trait. | HttpMethodSemantics.UnexpectedPayload +[WARNING] ns.foo#G: This operation uses the `POST` method in the `http` trait, but is marked with the readonly trait | HttpMethodSemantics.UnnecessaryReadonlyTrait +[WARNING] ns.foo#H: This operation uses the `DELETE` method in the `http` trait, but is not marked with the idempotent trait | HttpMethodSemantics.MissingIdempotentTrait +[WARNING] ns.foo#I: This operation uses the `GET` method in the `http` trait, but is not marked with the readonly trait | HttpMethodSemantics.MissingReadonlyTrait [WARNING] ns.foo#Options: OPTIONS requests are typically used as part of CORS and should not be modeled explicitly. They are an implementation detail that should not appear in generated client or server code. Instead, tooling should use the `cors` trait on a service to automatically configure CORS on clients and servers. It is the responsibility of service frameworks and API gateways to automatically manage OPTIONS requests. For example, OPTIONS requests are automatically created when using Smithy models with Amazon API Gateway. | HttpMethodSemantics.OPTIONS diff --git a/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/operation/input-output-traits/invalid-multiple-operations-same-shape.errors b/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/operation/input-output-traits/invalid-multiple-operations-same-shape.errors index 0bb9368717e..07f41cbe0bf 100644 --- a/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/operation/input-output-traits/invalid-multiple-operations-same-shape.errors +++ b/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/operation/input-output-traits/invalid-multiple-operations-same-shape.errors @@ -1,4 +1,4 @@ -[WARNING] smithy.example#GetFoo2: The input of this operation should target a shape that starts with the operation's name, 'GetFoo2', but the targeted shape is `smithy.example#GetFooInput` | OperationInputOutputName.input.smithy.example#GetFooInput -[WARNING] smithy.example#GetFoo2: The output of this operation should target a shape that starts with the operation's name, 'GetFoo2', but the targeted shape is `smithy.example#GetFooOutput` | OperationInputOutputName.output.smithy.example#GetFooOutput +[WARNING] smithy.example#GetFoo2: The input of this operation should target a shape that starts with the operation's name, 'GetFoo2', but the targeted shape is `smithy.example#GetFooInput` | OperationInputOutputName.input +[WARNING] smithy.example#GetFoo2: The output of this operation should target a shape that starts with the operation's name, 'GetFoo2', but the targeted shape is `smithy.example#GetFooOutput` | OperationInputOutputName.output [ERROR] smithy.example#GetFooInput: Shapes marked with the @input trait cannot be used as input by multiple operations: `smithy.example#GetFoo`, `smithy.example#GetFoo2` | OperationInputOutputMisuse [ERROR] smithy.example#GetFooOutput: Shapes marked with the @output trait cannot be used as output by multiple operations: `smithy.example#GetFoo`, `smithy.example#GetFoo2` | OperationInputOutputMisuse diff --git a/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/paginated/paginated-deeply-nested.errors b/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/paginated/paginated-deeply-nested.errors index 3a1c681c117..a48cb05bb15 100644 --- a/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/paginated/paginated-deeply-nested.errors +++ b/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/paginated/paginated-deeply-nested.errors @@ -1,2 +1,2 @@ -[WARNING] ns.foo#DeeplyNested: paginated trait `items` contains a path with more than two parts, which can make your API cumbersome to use | PaginatedTrait.items.DEEPLY_NESTED -[WARNING] ns.foo#DeeplyNested: paginated trait `outputToken` contains a path with more than two parts, which can make your API cumbersome to use | PaginatedTrait.outputToken.DEEPLY_NESTED +[WARNING] ns.foo#DeeplyNested: paginated trait `items` contains a path with more than two parts, which can make your API cumbersome to use | PaginatedTrait.DeeplyNested.items +[WARNING] ns.foo#DeeplyNested: paginated trait `outputToken` contains a path with more than two parts, which can make your API cumbersome to use | PaginatedTrait.DeeplyNested.outputToken diff --git a/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/paginated/paginated-invalid.errors b/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/paginated/paginated-invalid.errors index 545476d3fd1..90123a12a99 100644 --- a/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/paginated/paginated-invalid.errors +++ b/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/paginated/paginated-invalid.errors @@ -5,7 +5,7 @@ [ERROR] ns.foo#Invalid3: paginated trait `outputToken` member `OutputNotString` targets a integer shape, but must target one of the following: [`map`, `string`] | PaginatedTrait [ERROR] ns.foo#Invalid4: paginated trait `inputToken` member `nextToken` must not be required | PaginatedTrait [ERROR] ns.foo#Invalid4: paginated trait `outputToken` member `nextToken` must not be required | PaginatedTrait -[WARNING] ns.foo#Invalid4: paginated trait `pageSize` member `pageSize` should not be required | PaginatedTrait.pageSize.pageSize.SHOULD_NOT_BE_REQUIRED +[WARNING] ns.foo#Invalid4: paginated trait `pageSize` member `pageSize` should not be required | PaginatedTrait.ShouldNotBeRequired.pageSize [ERROR] ns.foo#Invalid5: paginated trait `pageSize` member `Invalid5Input` targets a string shape, but must target one of the following: [`integer`] | PaginatedTrait [ERROR] ns.foo#Invalid6: paginated trait `items` member `Invalid6Output` targets a string shape, but must target one of the following: [`list`, `map`] | PaginatedTrait [ERROR] ns.foo#Invalid7: paginated trait `items` targets a member `items` that does not exist | PaginatedTrait diff --git a/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/paginated/paginated-map-tokens.errors b/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/paginated/paginated-map-tokens.errors index 2b5dba872eb..9f6bc968c2f 100644 --- a/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/paginated/paginated-map-tokens.errors +++ b/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/paginated/paginated-map-tokens.errors @@ -1,2 +1,2 @@ -[DANGER] ns.foo#MapTokens: paginated trait `inputToken` member `MapTokensInput` targets a map shape, but this is not recommended. One of [`string`] SHOULD be targeted. | PaginatedTrait.inputToken.MapTokensInput.WRONG_SHAPE_TYPE.map -[DANGER] ns.foo#MapTokens: paginated trait `outputToken` member `MapTokensOutput` targets a map shape, but this is not recommended. One of [`string`] SHOULD be targeted. | PaginatedTrait.outputToken.MapTokensOutput.WRONG_SHAPE_TYPE.map +[DANGER] ns.foo#MapTokens: paginated trait `inputToken` member `MapTokensInput` targets a map shape, but this is not recommended. One of [`string`] SHOULD be targeted. | PaginatedTrait.WrongShapeType.inputToken +[DANGER] ns.foo#MapTokens: paginated trait `outputToken` member `MapTokensOutput` targets a map shape, but this is not recommended. One of [`string`] SHOULD be targeted. | PaginatedTrait.WrongShapeType.outputToken From 2beacc2ffbcc44b602dbe503e5509281d9b2875d Mon Sep 17 00:00:00 2001 From: Richard Chen Date: Tue, 10 Jan 2023 14:14:24 -0800 Subject: [PATCH 3/7] optimize assembleFullEventId method --- .../amazon/smithy/model/validation/AbstractValidator.java | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/validation/AbstractValidator.java b/smithy-model/src/main/java/software/amazon/smithy/model/validation/AbstractValidator.java index a2771119632..4b7184da50d 100644 --- a/smithy-model/src/main/java/software/amazon/smithy/model/validation/AbstractValidator.java +++ b/smithy-model/src/main/java/software/amazon/smithy/model/validation/AbstractValidator.java @@ -123,7 +123,9 @@ protected final ValidationEvent createEvent( protected final String assembleFullEventId(String... additionalEventIdParts) { String eventId = getName(); - if (additionalEventIdParts.length > 0) { + if (additionalEventIdParts.length == 1) { + eventId += "." + additionalEventIdParts[0]; + } else if (additionalEventIdParts.length > 1) { eventId += "." + String.join(".", additionalEventIdParts); } return eventId; From fa1dbe3d086ff4effd2b7e1475162f07c17465be Mon Sep 17 00:00:00 2001 From: Richard Chen Date: Tue, 10 Jan 2023 18:05:36 -0800 Subject: [PATCH 4/7] minor edits for style --- .../traits/HttpChecksumTraitValidator.java | 8 +++--- .../linters/NoninclusiveTermsValidator.java | 25 ++++++------------- .../model/validation/AbstractValidator.java | 10 +++----- .../validators/OperationValidator.java | 2 +- .../validators/PaginatedTraitValidator.java | 6 ++--- .../validators/TargetValidator.java | 2 +- 6 files changed, 21 insertions(+), 32 deletions(-) diff --git a/smithy-aws-traits/src/main/java/software/amazon/smithy/aws/traits/HttpChecksumTraitValidator.java b/smithy-aws-traits/src/main/java/software/amazon/smithy/aws/traits/HttpChecksumTraitValidator.java index 9b2cf5f891e..047b38d6815 100644 --- a/smithy-aws-traits/src/main/java/software/amazon/smithy/aws/traits/HttpChecksumTraitValidator.java +++ b/smithy-aws-traits/src/main/java/software/amazon/smithy/aws/traits/HttpChecksumTraitValidator.java @@ -185,8 +185,8 @@ private List validateHeaderConflicts(OperationShape operation, .map(HttpPrefixHeadersTrait::getValue) .ifPresent(headerPrefix -> { if (HttpChecksumTrait.CHECKSUM_PREFIX.startsWith(headerPrefix)) { - final String memberName = member.getId().getName(); - final String prefixString = headerPrefix.toLowerCase(Locale.US); + 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.", @@ -202,8 +202,8 @@ private List validateHeaderConflicts(OperationShape operation, .map(HttpHeaderTrait::getValue) .ifPresent(headerName -> { if (headerName.startsWith(HttpChecksumTrait.CHECKSUM_PREFIX)) { - final String memberName = member.getId().getName(); - final String headerString = headerName.toLowerCase(Locale.US); + 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.", headerString, memberName, HttpChecksumTrait.CHECKSUM_PREFIX), diff --git a/smithy-linters/src/main/java/software/amazon/smithy/linters/NoninclusiveTermsValidator.java b/smithy-linters/src/main/java/software/amazon/smithy/linters/NoninclusiveTermsValidator.java index 8673975dd1f..c09a3a628e5 100644 --- a/smithy-linters/src/main/java/software/amazon/smithy/linters/NoninclusiveTermsValidator.java +++ b/smithy-linters/src/main/java/software/amazon/smithy/linters/NoninclusiveTermsValidator.java @@ -141,19 +141,10 @@ private Collection getValidationEvents(TextInstance instance) { return events; } - /** - * Creates a single ValidationEvent. - * @param instance TextInstance where the noninclusive term was found - * @param replacements List of suggested replacements for the noninclusive term - * @param matchedText the noninclusive term that was used - * @return the constructed ValidationEvent - */ - private ValidationEvent constructValidationEvent( - final TextInstance instance, - final List replacements, - final String matchedText - ) { - final String replacementAddendum = getReplacementAddendum(matchedText, replacements); + private ValidationEvent constructValidationEvent(TextInstance instance, + List replacements, + String matchedText) { + String replacementAddendum = getReplacementAddendum(matchedText, replacements); switch (instance.getLocationType()) { case NAMESPACE: //Cannot use any warning() overloads because there is no shape associated with the event. @@ -165,9 +156,9 @@ private ValidationEvent constructValidationEvent( instance.getText(), matchedText, replacementAddendum)) .build(); case APPLIED_TRAIT: - final ValidationEvent validationEvent = + ValidationEvent validationEvent = warning(instance.getShape(), instance.getTrait().getSourceLocation(), ""); - final String idiomaticTraitName = Trait.getIdiomaticTraitName(instance.getTrait()); + String idiomaticTraitName = Trait.getIdiomaticTraitName(instance.getTrait()); if (instance.getTraitPropertyPath().isEmpty()) { return validationEvent.toBuilder() .message(String.format("'%s' trait has a value that contains a non-inclusive term `%s`.%s", @@ -175,7 +166,7 @@ private ValidationEvent constructValidationEvent( .id(assembleFullEventId(TRAIT, matchedText.toLowerCase(Locale.US), idiomaticTraitName)) .build(); } else { - final String valuePropertyPathFormatted = formatPropertyPath(instance.getTraitPropertyPath()); + String valuePropertyPathFormatted = formatPropertyPath(instance.getTraitPropertyPath()); return validationEvent.toBuilder() .message(String.format( "'%s' trait value at path {%s} contains a non-inclusive term `%s`.%s", @@ -196,7 +187,7 @@ private ValidationEvent constructValidationEvent( } private static String getReplacementAddendum(String matchedText, List replacements) { - final List caseCorrectedEntryValue = replacements.stream() + List caseCorrectedEntryValue = replacements.stream() .map(replacement -> Character.isUpperCase(matchedText.charAt(0)) ? StringUtils.capitalize(replacement) : StringUtils.uncapitalize(replacement)) diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/validation/AbstractValidator.java b/smithy-model/src/main/java/software/amazon/smithy/model/validation/AbstractValidator.java index 4b7184da50d..4e7fdb3e039 100644 --- a/smithy-model/src/main/java/software/amazon/smithy/model/validation/AbstractValidator.java +++ b/smithy-model/src/main/java/software/amazon/smithy/model/validation/AbstractValidator.java @@ -122,12 +122,10 @@ protected final ValidationEvent createEvent( } protected final String assembleFullEventId(String... additionalEventIdParts) { - String eventId = getName(); - if (additionalEventIdParts.length == 1) { - eventId += "." + additionalEventIdParts[0]; - } else if (additionalEventIdParts.length > 1) { - eventId += "." + String.join(".", additionalEventIdParts); + StringBuilder buf = new StringBuilder(getName()); + for (String part : additionalEventIdParts) { + buf.append('.').append(part); } - return eventId; + return buf.toString(); } } diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/validation/validators/OperationValidator.java b/smithy-model/src/main/java/software/amazon/smithy/model/validation/validators/OperationValidator.java index 6ef51166812..42e48cd01fa 100644 --- a/smithy-model/src/main/java/software/amazon/smithy/model/validation/validators/OperationValidator.java +++ b/smithy-model/src/main/java/software/amazon/smithy/model/validation/validators/OperationValidator.java @@ -141,7 +141,7 @@ private ValidationEvent emitBadInputOutputName(Shape operation, String property, return ValidationEvent.builder() .severity(Severity.WARNING) .shape(operation) - .id(String.join(".", OPERATION_INPUT_OUTPUT_NAME, property)) + .id(OPERATION_INPUT_OUTPUT_NAME + "." + property) .message(String.format( "The %s of this operation should target a shape that starts with the operation's name, '%s', " + "but the targeted shape is `%s`", property, operation.getId().getName(), target)) diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/validation/validators/PaginatedTraitValidator.java b/smithy-model/src/main/java/software/amazon/smithy/model/validation/validators/PaginatedTraitValidator.java index c722e5c6740..08dbdf56ec3 100644 --- a/smithy-model/src/main/java/software/amazon/smithy/model/validation/validators/PaginatedTraitValidator.java +++ b/smithy-model/src/main/java/software/amazon/smithy/model/validation/validators/PaginatedTraitValidator.java @@ -173,9 +173,9 @@ private List validateMember( if (validator.dangerTargets().contains(target.getType())) { Set preferredTargets = new TreeSet<>(validator.validTargets()); preferredTargets.removeAll(validator.dangerTargets()); - final String traitName = validator.propertyName(); - final String memberName = member.getId().getName(); - final String targetType = target.getType().toString(); + String traitName = validator.propertyName(); + String memberName = member.getId().getName(); + String targetType = target.getType().toString(); events.add(danger(operation, trait, String.format( "%spaginated trait `%s` member `%s` targets a %s shape, but this is not recommended. " + "One of [%s] SHOULD be targeted.", diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/validation/validators/TargetValidator.java b/smithy-model/src/main/java/software/amazon/smithy/model/validation/validators/TargetValidator.java index f6f8090d875..206dd408dd0 100644 --- a/smithy-model/src/main/java/software/amazon/smithy/model/validation/validators/TargetValidator.java +++ b/smithy-model/src/main/java/software/amazon/smithy/model/validation/validators/TargetValidator.java @@ -204,7 +204,7 @@ private void validateDeprecatedTargets( deprecatedTrait.getMessage().ifPresent(message -> builder.append(". ").append(message)); deprecatedTrait.getSince().ifPresent(since -> builder.append(" (since ").append(since).append(')')); events.add(ValidationEvent.builder() - .id(String.format("DeprecatedShape.%s", target.getId())) + .id("DeprecatedShape." + target.getId()) .severity(Severity.WARNING) .shape(shape) .message(builder.toString()) From 787f208b3522547f5f45e44392b3370d26a99ccf Mon Sep 17 00:00:00 2001 From: Richard Chen Date: Wed, 25 Jan 2023 21:21:02 -0500 Subject: [PATCH 5/7] unroll eventId varargs in AbstractValidator for performance --- .../linters/NoninclusiveTermsValidator.java | 10 +- .../model/validation/AbstractValidator.java | 437 ++++++++++++++++-- .../HttpBindingsMissingValidator.java | 2 +- 3 files changed, 416 insertions(+), 33 deletions(-) diff --git a/smithy-linters/src/main/java/software/amazon/smithy/linters/NoninclusiveTermsValidator.java b/smithy-linters/src/main/java/software/amazon/smithy/linters/NoninclusiveTermsValidator.java index c09a3a628e5..5ea29032692 100644 --- a/smithy-linters/src/main/java/software/amazon/smithy/linters/NoninclusiveTermsValidator.java +++ b/smithy-linters/src/main/java/software/amazon/smithy/linters/NoninclusiveTermsValidator.java @@ -151,7 +151,8 @@ private ValidationEvent constructValidationEvent(TextInstance instance, return ValidationEvent.builder() .severity(Severity.WARNING) .sourceLocation(SourceLocation.none()) - .id(assembleFullEventId(NAMESPACE, instance.getText(), matchedText.toLowerCase(Locale.US))) + .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(); @@ -163,7 +164,8 @@ private ValidationEvent constructValidationEvent(TextInstance instance, return validationEvent.toBuilder() .message(String.format("'%s' trait has a value that contains a non-inclusive term `%s`.%s", idiomaticTraitName, matchedText, replacementAddendum)) - .id(assembleFullEventId(TRAIT, matchedText.toLowerCase(Locale.US), idiomaticTraitName)) + .id(getName() + "." + TRAIT + "." + + matchedText.toLowerCase(Locale.US) + "." + idiomaticTraitName) .build(); } else { String valuePropertyPathFormatted = formatPropertyPath(instance.getTraitPropertyPath()); @@ -171,8 +173,8 @@ private ValidationEvent constructValidationEvent(TextInstance instance, .message(String.format( "'%s' trait value at path {%s} contains a non-inclusive term `%s`.%s", idiomaticTraitName, valuePropertyPathFormatted, matchedText, replacementAddendum)) - .id(assembleFullEventId(TRAIT, matchedText.toLowerCase(Locale.US), idiomaticTraitName, - valuePropertyPathFormatted)) + .id(getName() + "." + TRAIT + "." + matchedText.toLowerCase(Locale.US) + + "." + idiomaticTraitName + "." + valuePropertyPathFormatted) .build(); } case SHAPE: diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/validation/AbstractValidator.java b/smithy-model/src/main/java/software/amazon/smithy/model/validation/AbstractValidator.java index 4e7fdb3e039..da3c4b7f6d3 100644 --- a/smithy-model/src/main/java/software/amazon/smithy/model/validation/AbstractValidator.java +++ b/smithy-model/src/main/java/software/amazon/smithy/model/validation/AbstractValidator.java @@ -28,104 +28,485 @@ public String getName() { return defaultName; } + protected final ValidationEvent error( + Shape shape, + String message + ) { + return createEvent(Severity.ERROR, shape, shape.getSourceLocation(), message); + } + + protected final ValidationEvent error( + Shape shape, + String message, + String eventIdSubpart1 + ) { + return createEvent(Severity.ERROR, shape, shape.getSourceLocation(), message, eventIdSubpart1); + } + protected final ValidationEvent error( Shape shape, String message, - String... additionalEventIdParts + String eventIdSubpart1, + String eventIdSubpart2 ) { - return createEvent(Severity.ERROR, shape, shape.getSourceLocation(), message, additionalEventIdParts); + return createEvent(Severity.ERROR, shape, shape.getSourceLocation(), message, eventIdSubpart1, eventIdSubpart2); } protected final ValidationEvent error( + Shape shape, + String message, + String eventIdSubpart1, + String eventIdSubpart2, + String eventIdSubpart3 + ) { + return createEvent(Severity.ERROR, shape, shape.getSourceLocation(), message, eventIdSubpart1, eventIdSubpart2, + eventIdSubpart3); + } + + protected final ValidationEvent error( + Shape shape, + String message, + String eventIdSubpart1, + String eventIdSubpart2, + String eventIdSubpart3, + String eventIdSubpart4 + ) { + return createEvent(Severity.ERROR, shape, shape.getSourceLocation(), message, eventIdSubpart1, eventIdSubpart2, + eventIdSubpart3, eventIdSubpart4); + } + + protected final ValidationEvent error( + Shape shape, + FromSourceLocation location, + String message + ) { + return createEvent(Severity.ERROR, shape, location, message); + } + + protected final ValidationEvent error( + Shape shape, + FromSourceLocation location, + String message, + String eventIdSubpart1 + ) { + return createEvent(Severity.ERROR, shape, location, message, eventIdSubpart1); + } + + protected final ValidationEvent error( + Shape shape, + FromSourceLocation location, + String message, + String eventIdSubpart1, + String eventIdSubpart2 + ) { + return createEvent(Severity.ERROR, shape, location, message, eventIdSubpart1, eventIdSubpart2); + } + + protected final ValidationEvent error( + Shape shape, + FromSourceLocation location, + String message, + String eventIdSubpart1, + String eventIdSubpart2, + String eventIdSubpart3 + ) { + return createEvent(Severity.ERROR, shape, location, message, eventIdSubpart1, eventIdSubpart2, eventIdSubpart3); + } + + protected final ValidationEvent error( + Shape shape, + FromSourceLocation location, + String message, + String eventIdSubpart1, + String eventIdSubpart2, + String eventIdSubpart3, + String eventIdSubpart4 + ) { + return createEvent(Severity.ERROR, shape, location, message, eventIdSubpart1, eventIdSubpart2, eventIdSubpart3, + eventIdSubpart4); + } + + protected final ValidationEvent danger( + Shape shape, + String message + ) { + return createEvent(Severity.DANGER, shape, shape.getSourceLocation(), message); + } + + protected final ValidationEvent danger( + Shape shape, + String message, + String eventIdSubpart1 + ) { + return createEvent(Severity.DANGER, shape, shape.getSourceLocation(), message, eventIdSubpart1); + } + + protected final ValidationEvent danger( + Shape shape, + String message, + String eventIdSubpart1, + String eventIdSubpart2 + ) { + return createEvent(Severity.DANGER, shape, shape.getSourceLocation(), message, eventIdSubpart1, + eventIdSubpart2); + } + + protected final ValidationEvent danger( + Shape shape, + String message, + String eventIdSubpart1, + String eventIdSubpart2, + String eventIdSubpart3 + ) { + return createEvent(Severity.DANGER, shape, shape.getSourceLocation(), message, eventIdSubpart1, eventIdSubpart2, + eventIdSubpart3); + } + + protected final ValidationEvent danger( + Shape shape, + String message, + String eventIdSubpart1, + String eventIdSubpart2, + String eventIdSubpart3, + String eventIdSubpart4 + ) { + return createEvent(Severity.DANGER, shape, shape.getSourceLocation(), message, eventIdSubpart1, eventIdSubpart2, + eventIdSubpart3, eventIdSubpart4); + } + + protected final ValidationEvent danger( + Shape shape, + FromSourceLocation location, + String message + ) { + return createEvent(Severity.DANGER, shape, location, message); + } + + protected final ValidationEvent danger( Shape shape, FromSourceLocation location, String message, - String... additionalEventIdParts + String eventIdSubpart1 ) { - return createEvent(Severity.ERROR, shape, location, message, additionalEventIdParts); + return createEvent(Severity.DANGER, shape, location, message, eventIdSubpart1); } protected final ValidationEvent danger( Shape shape, + FromSourceLocation location, + String message, + String eventIdSubpart1, + String eventIdSubpart2 + ) { + return createEvent(Severity.DANGER, shape, location, message, eventIdSubpart1, eventIdSubpart2); + } + + protected final ValidationEvent danger( + Shape shape, + FromSourceLocation location, String message, - String... additionalEventIdParts + String eventIdSubpart1, + String eventIdSubpart2, + String eventIdSubpart3 ) { - return createEvent(Severity.DANGER, shape, shape.getSourceLocation(), message, additionalEventIdParts); + return createEvent(Severity.DANGER, shape, location, message, eventIdSubpart1, eventIdSubpart2, + eventIdSubpart3); } protected final ValidationEvent danger( Shape shape, FromSourceLocation location, String message, - String... additionalEventIdParts + String eventIdSubpart1, + String eventIdSubpart2, + String eventIdSubpart3, + String eventIdSubpart4 + ) { + return createEvent(Severity.DANGER, shape, location, message, eventIdSubpart1, eventIdSubpart2, eventIdSubpart3, + eventIdSubpart4); + } + + protected final ValidationEvent warning( + Shape shape, + String message + ) { + return createEvent(Severity.WARNING, shape, shape.getSourceLocation(), message); + } + + protected final ValidationEvent warning( + Shape shape, + String message, + String eventIdSubpart1 ) { - return createEvent(Severity.DANGER, shape, location, message, additionalEventIdParts); + return createEvent(Severity.WARNING, shape, shape.getSourceLocation(), message, eventIdSubpart1); } protected final ValidationEvent warning( Shape shape, String message, - String... additionalEventIdParts + String eventIdSubpart1, + String eventIdSubpart2 ) { - return createEvent(Severity.WARNING, shape, shape.getSourceLocation(), message, additionalEventIdParts); + return createEvent(Severity.WARNING, shape, shape.getSourceLocation(), message, eventIdSubpart1, + eventIdSubpart2); + } + + protected final ValidationEvent warning( + Shape shape, + String message, + String eventIdSubpart1, + String eventIdSubpart2, + String eventIdSubpart3 + ) { + return createEvent(Severity.WARNING, shape, shape.getSourceLocation(), message, eventIdSubpart1, + eventIdSubpart2, eventIdSubpart3); + } + + protected final ValidationEvent warning( + Shape shape, + String message, + String eventIdSubpart1, + String eventIdSubpart2, + String eventIdSubpart3, + String eventIdSubpart4 + ) { + return createEvent(Severity.WARNING, shape, shape.getSourceLocation(), message, eventIdSubpart1, + eventIdSubpart2, eventIdSubpart3, eventIdSubpart4); + } + + protected final ValidationEvent warning( + Shape shape, + FromSourceLocation location, + String message + ) { + return createEvent(Severity.WARNING, shape, location, message); + } + + protected final ValidationEvent warning( + Shape shape, + FromSourceLocation location, + String message, + String eventIdSubpart1 + ) { + return createEvent(Severity.WARNING, shape, location, message, eventIdSubpart1); } protected final ValidationEvent warning( Shape shape, FromSourceLocation location, String message, - String... additionalEventIdParts + String eventIdSubpart1, + String eventIdSubpart2 ) { - return createEvent(Severity.WARNING, shape, location, message, additionalEventIdParts); + return createEvent(Severity.WARNING, shape, location, message, eventIdSubpart1, eventIdSubpart2); + } + + protected final ValidationEvent warning( + Shape shape, + FromSourceLocation location, + String message, + String eventIdSubpart1, + String eventIdSubpart2, + String eventIdSubpart3 + ) { + return createEvent(Severity.WARNING, shape, location, message, eventIdSubpart1, eventIdSubpart2, + eventIdSubpart3); + } + + protected final ValidationEvent warning( + Shape shape, + FromSourceLocation location, + String message, + String eventIdSubpart1, + String eventIdSubpart2, + String eventIdSubpart3, + String eventIdSubpart4 + ) { + return createEvent(Severity.WARNING, shape, location, message, eventIdSubpart1, eventIdSubpart2, + eventIdSubpart3, eventIdSubpart4); + } + + protected final ValidationEvent note( + Shape shape, + String message + ) { + return createEvent(Severity.NOTE, shape, shape.getSourceLocation(), message); + } + + protected final ValidationEvent note( + Shape shape, + String message, + String eventIdSubpart1 + ) { + return createEvent(Severity.NOTE, shape, shape.getSourceLocation(), message, eventIdSubpart1); + } + + protected final ValidationEvent note( + Shape shape, + String message, + String eventIdSubpart1, + String eventIdSubpart2 + ) { + return createEvent(Severity.NOTE, shape, shape.getSourceLocation(), message, eventIdSubpart1, eventIdSubpart2); + } + + protected final ValidationEvent note( + Shape shape, + String message, + String eventIdSubpart1, + String eventIdSubpart2, + String eventIdSubpart3 + ) { + return createEvent(Severity.NOTE, shape, shape.getSourceLocation(), message, eventIdSubpart1, eventIdSubpart2, + eventIdSubpart3); } protected final ValidationEvent note( Shape shape, String message, - String... additionalEventIdParts + String eventIdSubpart1, + String eventIdSubpart2, + String eventIdSubpart3, + String eventIdSubpart4 ) { - return createEvent(Severity.NOTE, shape, shape.getSourceLocation(), message, additionalEventIdParts); + return createEvent(Severity.NOTE, shape, shape.getSourceLocation(), message, eventIdSubpart1, eventIdSubpart2, + eventIdSubpart3, eventIdSubpart4); + } + + protected final ValidationEvent note( + Shape shape, + FromSourceLocation location, + String message + ) { + return createEvent(Severity.NOTE, shape, location, message); } protected final ValidationEvent note( Shape shape, FromSourceLocation location, String message, - String... additionalEventIdParts + String eventIdSubpart1 ) { - return createEvent(Severity.NOTE, shape, location, message, additionalEventIdParts); + return createEvent(Severity.NOTE, shape, location, message, eventIdSubpart1); + } + + protected final ValidationEvent note( + Shape shape, + FromSourceLocation location, + String message, + String eventIdSubpart1, + String eventIdSubpart2 + ) { + return createEvent(Severity.NOTE, shape, location, message, eventIdSubpart1, eventIdSubpart2); + } + + protected final ValidationEvent note( + Shape shape, + FromSourceLocation location, + String message, + String eventIdSubpart1, + String eventIdSubpart2, + String eventIdSubpart3 + ) { + return createEvent(Severity.NOTE, shape, location, message, eventIdSubpart1, eventIdSubpart2, eventIdSubpart3); + } + + protected final ValidationEvent note( + Shape shape, + FromSourceLocation location, + String message, + String eventIdSubpart1, + String eventIdSubpart2, + String eventIdSubpart3, + String eventIdSubpart4 + ) { + return createEvent(Severity.NOTE, shape, location, message, eventIdSubpart1, eventIdSubpart2, eventIdSubpart3, + eventIdSubpart4); + } + + protected final ValidationEvent createEvent( + Severity severity, + Shape shape, + FromSourceLocation loc, + String message + ) { + return ValidationEvent.builder() + .severity(severity) + .message(message) + .shapeId(shape.getId()) + .sourceLocation(loc.getSourceLocation()) + .id(getName()) + .build(); + } + + protected final ValidationEvent createEvent( + Severity severity, + Shape shape, + FromSourceLocation loc, + String message, + String eventIdSubpart1 + ) { + return ValidationEvent.builder() + .severity(severity) + .message(message) + .shapeId(shape.getId()) + .sourceLocation(loc.getSourceLocation()) + .id(getName() + "." + eventIdSubpart1) + .build(); } protected final ValidationEvent createEvent( Severity severity, Shape shape, + FromSourceLocation loc, String message, - String... additionalEventIdParts + String eventIdSubpart1, + String eventIdSubpart2 ) { - return createEvent(severity, shape, shape.getSourceLocation(), message, additionalEventIdParts); + return ValidationEvent.builder() + .severity(severity) + .message(message) + .shapeId(shape.getId()) + .sourceLocation(loc.getSourceLocation()) + .id(getName() + "." + eventIdSubpart1 + "." + eventIdSubpart2) + .build(); } protected final ValidationEvent createEvent( Severity severity, Shape shape, FromSourceLocation loc, - String msg, - String... additionalEventIdParts + String message, + String eventIdSubpart1, + String eventIdSubpart2, + String eventIdSubpart3 ) { return ValidationEvent.builder() .severity(severity) - .message(msg) + .message(message) .shapeId(shape.getId()) .sourceLocation(loc.getSourceLocation()) - .id(assembleFullEventId(additionalEventIdParts)) + .id(getName() + "." + eventIdSubpart1 + "." + eventIdSubpart2 + "." + eventIdSubpart3) .build(); } - protected final String assembleFullEventId(String... additionalEventIdParts) { - StringBuilder buf = new StringBuilder(getName()); - for (String part : additionalEventIdParts) { - buf.append('.').append(part); - } - return buf.toString(); + protected final ValidationEvent createEvent( + Severity severity, + Shape shape, + FromSourceLocation loc, + String message, + String eventIdSubpart1, + String eventIdSubpart2, + String eventIdSubpart3, + String eventIdSubpart4 + ) { + return ValidationEvent.builder() + .severity(severity) + .message(message) + .shapeId(shape.getId()) + .sourceLocation(loc.getSourceLocation()) + .id(getName() + "." + eventIdSubpart1 + "." + eventIdSubpart2 + + "." + eventIdSubpart3 + "." + eventIdSubpart4) + .build(); } } diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/validation/validators/HttpBindingsMissingValidator.java b/smithy-model/src/main/java/software/amazon/smithy/model/validation/validators/HttpBindingsMissingValidator.java index b3e1822d35c..a51072c05c9 100644 --- a/smithy-model/src/main/java/software/amazon/smithy/model/validation/validators/HttpBindingsMissingValidator.java +++ b/smithy-model/src/main/java/software/amazon/smithy/model/validation/validators/HttpBindingsMissingValidator.java @@ -79,7 +79,7 @@ private boolean hasBindings(OperationShape op) { } private ValidationEvent createEvent(Severity severity, ServiceShape service, OperationShape operation) { - return createEvent(severity, operation, String.format( + return createEvent(severity, operation, operation.getSourceLocation(), String.format( "One or more operations in the `%s` service define the `http` trait, but this " + "operation is missing the `http` trait.", service.getId())); } From f1ddd83fdfa480c0133385d5c1024483b2d97116 Mon Sep 17 00:00:00 2001 From: Richard Chen Date: Tue, 31 Jan 2023 16:46:40 -0500 Subject: [PATCH 6/7] fix eventIds for Waiter violations --- .../smithy/waiters/WaiterMatcherValidator.java | 15 ++++++++++----- .../emits-danger-and-warning-typechecks.errors | 6 +++--- .../waiters/errorfiles/invalid-errorType.errors | 2 +- .../errorfiles/invalid-jmespath-syntax.errors | 2 +- .../errorfiles/invalid-return-types.errors | 8 ++++---- 5 files changed, 19 insertions(+), 14 deletions(-) diff --git a/smithy-waiters/src/main/java/software/amazon/smithy/waiters/WaiterMatcherValidator.java b/smithy-waiters/src/main/java/software/amazon/smithy/waiters/WaiterMatcherValidator.java index 7d3245902da..b618f6ac00d 100644 --- a/smithy-waiters/src/main/java/software/amazon/smithy/waiters/WaiterMatcherValidator.java +++ b/smithy-waiters/src/main/java/software/amazon/smithy/waiters/WaiterMatcherValidator.java @@ -40,6 +40,9 @@ final class WaiterMatcherValidator implements Matcher.Visitor visitErrorType(Matcher.ErrorTypeMember errorType) { addEvent(Severity.WARNING, String.format( "errorType '%s' not found on operation. This operation defines the following errors: %s", error, operation.getErrors()), - INVALID_ERROR_TYPE, waiterName, String.valueOf(acceptorIndex), error); + INVALID_ERROR_TYPE, waiterName, String.valueOf(acceptorIndex)); return events; } @@ -153,8 +156,7 @@ private void validateReturnType(PathComparator comparator, RuntimeType expected, "Waiter acceptors with a %s comparator must return a `%s` type, but this acceptor was " + "statically determined to return a `%s` type.", comparator, expected, actual), - JMESPATH_PROBLEM, waiterName, String.valueOf(acceptorIndex), comparator.toString(), - actual.toString()); + JMESPATH_PROBLEM, RETURN_TYPE_MISMATCH, waiterName, String.valueOf(acceptorIndex)); } } @@ -167,22 +169,25 @@ private LiteralExpression createCurrentNodeFromShape(Shape shape) { private void addJmespathEvent(String path, ExpressionProblem problem) { Severity severity; + String eventId; switch (problem.severity) { case ERROR: severity = Severity.ERROR; + eventId = NON_SUPPRESSABLE_ERROR; break; case DANGER: severity = Severity.DANGER; + eventId = JMESPATH_PROBLEM + "." + JMES_PATH_DANGER + "." + waiterName + "." + acceptorIndex; break; default: severity = Severity.WARNING; + eventId = JMESPATH_PROBLEM + "." + JMES_PATH_WARNING + "." + waiterName + "." + acceptorIndex; break; } String problemMessage = problem.message + " (" + problem.line + ":" + problem.column + ")"; addEvent(severity, String.format("Problem found in JMESPath expression (%s): %s", path, problemMessage), - severity == Severity.ERROR ? NON_SUPPRESSABLE_ERROR : JMESPATH_PROBLEM, waiterName, - String.valueOf(acceptorIndex)); + eventId); } private void addEvent(Severity severity, String message, String... eventIdParts) { diff --git a/smithy-waiters/src/test/resources/software/amazon/smithy/waiters/errorfiles/emits-danger-and-warning-typechecks.errors b/smithy-waiters/src/test/resources/software/amazon/smithy/waiters/errorfiles/emits-danger-and-warning-typechecks.errors index 44d6efccf0e..0fe3ffcec12 100644 --- a/smithy-waiters/src/test/resources/software/amazon/smithy/waiters/errorfiles/emits-danger-and-warning-typechecks.errors +++ b/smithy-waiters/src/test/resources/software/amazon/smithy/waiters/errorfiles/emits-danger-and-warning-typechecks.errors @@ -1,3 +1,3 @@ -[DANGER] smithy.example#A: Waiter `Invalid1`, acceptor 0: Problem found in JMESPath expression (`10`.foo): Object field 'foo' extraction performed on number (1:6) | WaitableTraitJmespathProblem.Invalid1.0 -[DANGER] smithy.example#A: Waiter `Invalid2`, acceptor 0: Waiter acceptors with a booleanEquals comparator must return a `boolean` type, but this acceptor was statically determined to return a `null` type. | WaitableTraitJmespathProblem.Invalid2.0.booleanEquals.null -[WARNING] smithy.example#A: Waiter `Invalid2`, acceptor 0: Problem found in JMESPath expression (`true` < `false`): Invalid comparator '<' for boolean (1:10) | WaitableTraitJmespathProblem +[DANGER] smithy.example#A: Waiter `Invalid1`, acceptor 0: Problem found in JMESPath expression (`10`.foo): Object field 'foo' extraction performed on number (1:6) | WaitableTraitJmespathProblem.JmespathEventDanger.Invalid1.0 +[DANGER] smithy.example#A: Waiter `Invalid2`, acceptor 0: Waiter acceptors with a booleanEquals comparator must return a `boolean` type, but this acceptor was statically determined to return a `null` type. | WaitableTraitJmespathProblem.ReturnTypeMismatch.Invalid2.0 +[WARNING] smithy.example#A: Waiter `Invalid2`, acceptor 0: Problem found in JMESPath expression (`true` < `false`): Invalid comparator '<' for boolean (1:10) | WaitableTraitJmespathProblem.JmespathEventWarning.Invalid2.0 diff --git a/smithy-waiters/src/test/resources/software/amazon/smithy/waiters/errorfiles/invalid-errorType.errors b/smithy-waiters/src/test/resources/software/amazon/smithy/waiters/errorfiles/invalid-errorType.errors index 74b19f01895..d817a554d40 100644 --- a/smithy-waiters/src/test/resources/software/amazon/smithy/waiters/errorfiles/invalid-errorType.errors +++ b/smithy-waiters/src/test/resources/software/amazon/smithy/waiters/errorfiles/invalid-errorType.errors @@ -1 +1 @@ -[WARNING] smithy.example#A: Waiter `A`, acceptor 0: errorType 'Nope' not found on operation. This operation defines the following errors: [smithy.example#OhNo] | WaitableTraitInvalidErrorType.A.0.Nope +[WARNING] smithy.example#A: Waiter `A`, acceptor 0: errorType 'Nope' not found on operation. This operation defines the following errors: [smithy.example#OhNo] | WaitableTraitInvalidErrorType.A.0 diff --git a/smithy-waiters/src/test/resources/software/amazon/smithy/waiters/errorfiles/invalid-jmespath-syntax.errors b/smithy-waiters/src/test/resources/software/amazon/smithy/waiters/errorfiles/invalid-jmespath-syntax.errors index d8a318aa4e8..939dae48fa1 100644 --- a/smithy-waiters/src/test/resources/software/amazon/smithy/waiters/errorfiles/invalid-jmespath-syntax.errors +++ b/smithy-waiters/src/test/resources/software/amazon/smithy/waiters/errorfiles/invalid-jmespath-syntax.errors @@ -1,3 +1,3 @@ [ERROR] smithy.example#A: Waiter `Invalid1`, acceptor 0: Invalid JMESPath expression (||): Syntax error | WaitableTrait [ERROR] smithy.example#A: Waiter `Invalid2`, acceptor 0: Problem found in JMESPath expression (length(`10`)): length function argument 0 error | WaitableTrait -[DANGER] smithy.example#A: Waiter `Invalid2`, acceptor 0: Waiter acceptors with a booleanEquals comparator must return a `boolean` type, but this acceptor was statically determined to return a `number` type. | WaitableTraitJmespathProblem.Invalid2.0.booleanEquals.number +[DANGER] smithy.example#A: Waiter `Invalid2`, acceptor 0: Waiter acceptors with a booleanEquals comparator must return a `boolean` type, but this acceptor was statically determined to return a `number` type. | WaitableTraitJmespathProblem.ReturnTypeMismatch.Invalid2.0 diff --git a/smithy-waiters/src/test/resources/software/amazon/smithy/waiters/errorfiles/invalid-return-types.errors b/smithy-waiters/src/test/resources/software/amazon/smithy/waiters/errorfiles/invalid-return-types.errors index 7c7a538f9d0..de03fe56c60 100644 --- a/smithy-waiters/src/test/resources/software/amazon/smithy/waiters/errorfiles/invalid-return-types.errors +++ b/smithy-waiters/src/test/resources/software/amazon/smithy/waiters/errorfiles/invalid-return-types.errors @@ -1,4 +1,4 @@ -[DANGER] smithy.example#A: Waiter `Invalid1`, acceptor 0: Waiter acceptors with a booleanEquals comparator must return a `boolean` type, but this acceptor was statically determined to return a `number` type. | WaitableTraitJmespathProblem.Invalid1.0.booleanEquals.number -[DANGER] smithy.example#A: Waiter `Invalid1`, acceptor 1: Waiter acceptors with a stringEquals comparator must return a `string` type, but this acceptor was statically determined to return a `number` type. | WaitableTraitJmespathProblem.Invalid1.1.stringEquals.number -[DANGER] smithy.example#A: Waiter `Invalid1`, acceptor 2: Waiter acceptors with a allStringEquals comparator must return a `array` type, but this acceptor was statically determined to return a `number` type. | WaitableTraitJmespathProblem.Invalid1.2.allStringEquals.number -[DANGER] smithy.example#A: Waiter `Invalid1`, acceptor 3: Waiter acceptors with a anyStringEquals comparator must return a `array` type, but this acceptor was statically determined to return a `number` type. | WaitableTraitJmespathProblem.Invalid1.3.anyStringEquals.number +[DANGER] smithy.example#A: Waiter `Invalid1`, acceptor 0: Waiter acceptors with a booleanEquals comparator must return a `boolean` type, but this acceptor was statically determined to return a `number` type. | WaitableTraitJmespathProblem.ReturnTypeMismatch.Invalid1.0 +[DANGER] smithy.example#A: Waiter `Invalid1`, acceptor 1: Waiter acceptors with a stringEquals comparator must return a `string` type, but this acceptor was statically determined to return a `number` type. | WaitableTraitJmespathProblem.ReturnTypeMismatch.Invalid1.1 +[DANGER] smithy.example#A: Waiter `Invalid1`, acceptor 2: Waiter acceptors with a allStringEquals comparator must return a `array` type, but this acceptor was statically determined to return a `number` type. | WaitableTraitJmespathProblem.ReturnTypeMismatch.Invalid1.2 +[DANGER] smithy.example#A: Waiter `Invalid1`, acceptor 3: Waiter acceptors with a anyStringEquals comparator must return a `array` type, but this acceptor was statically determined to return a `number` type. | WaitableTraitJmespathProblem.ReturnTypeMismatch.Invalid1.3 From 67221a84980058d092912343fab5e2f03fbfa49c Mon Sep 17 00:00:00 2001 From: Richard Chen Date: Tue, 31 Jan 2023 17:12:47 -0500 Subject: [PATCH 7/7] remove unused method overloads --- .../model/validation/AbstractValidator.java | 120 ------------------ 1 file changed, 120 deletions(-) diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/validation/AbstractValidator.java b/smithy-model/src/main/java/software/amazon/smithy/model/validation/AbstractValidator.java index da3c4b7f6d3..35fbeb2a4f5 100644 --- a/smithy-model/src/main/java/software/amazon/smithy/model/validation/AbstractValidator.java +++ b/smithy-model/src/main/java/software/amazon/smithy/model/validation/AbstractValidator.java @@ -63,18 +63,6 @@ protected final ValidationEvent error( eventIdSubpart3); } - protected final ValidationEvent error( - Shape shape, - String message, - String eventIdSubpart1, - String eventIdSubpart2, - String eventIdSubpart3, - String eventIdSubpart4 - ) { - return createEvent(Severity.ERROR, shape, shape.getSourceLocation(), message, eventIdSubpart1, eventIdSubpart2, - eventIdSubpart3, eventIdSubpart4); - } - protected final ValidationEvent error( Shape shape, FromSourceLocation location, @@ -113,19 +101,6 @@ protected final ValidationEvent error( return createEvent(Severity.ERROR, shape, location, message, eventIdSubpart1, eventIdSubpart2, eventIdSubpart3); } - protected final ValidationEvent error( - Shape shape, - FromSourceLocation location, - String message, - String eventIdSubpart1, - String eventIdSubpart2, - String eventIdSubpart3, - String eventIdSubpart4 - ) { - return createEvent(Severity.ERROR, shape, location, message, eventIdSubpart1, eventIdSubpart2, eventIdSubpart3, - eventIdSubpart4); - } - protected final ValidationEvent danger( Shape shape, String message @@ -162,18 +137,6 @@ protected final ValidationEvent danger( eventIdSubpart3); } - protected final ValidationEvent danger( - Shape shape, - String message, - String eventIdSubpart1, - String eventIdSubpart2, - String eventIdSubpart3, - String eventIdSubpart4 - ) { - return createEvent(Severity.DANGER, shape, shape.getSourceLocation(), message, eventIdSubpart1, eventIdSubpart2, - eventIdSubpart3, eventIdSubpart4); - } - protected final ValidationEvent danger( Shape shape, FromSourceLocation location, @@ -213,19 +176,6 @@ protected final ValidationEvent danger( eventIdSubpart3); } - protected final ValidationEvent danger( - Shape shape, - FromSourceLocation location, - String message, - String eventIdSubpart1, - String eventIdSubpart2, - String eventIdSubpart3, - String eventIdSubpart4 - ) { - return createEvent(Severity.DANGER, shape, location, message, eventIdSubpart1, eventIdSubpart2, eventIdSubpart3, - eventIdSubpart4); - } - protected final ValidationEvent warning( Shape shape, String message @@ -262,18 +212,6 @@ protected final ValidationEvent warning( eventIdSubpart2, eventIdSubpart3); } - protected final ValidationEvent warning( - Shape shape, - String message, - String eventIdSubpart1, - String eventIdSubpart2, - String eventIdSubpart3, - String eventIdSubpart4 - ) { - return createEvent(Severity.WARNING, shape, shape.getSourceLocation(), message, eventIdSubpart1, - eventIdSubpart2, eventIdSubpart3, eventIdSubpart4); - } - protected final ValidationEvent warning( Shape shape, FromSourceLocation location, @@ -313,19 +251,6 @@ protected final ValidationEvent warning( eventIdSubpart3); } - protected final ValidationEvent warning( - Shape shape, - FromSourceLocation location, - String message, - String eventIdSubpart1, - String eventIdSubpart2, - String eventIdSubpart3, - String eventIdSubpart4 - ) { - return createEvent(Severity.WARNING, shape, location, message, eventIdSubpart1, eventIdSubpart2, - eventIdSubpart3, eventIdSubpart4); - } - protected final ValidationEvent note( Shape shape, String message @@ -361,18 +286,6 @@ protected final ValidationEvent note( eventIdSubpart3); } - protected final ValidationEvent note( - Shape shape, - String message, - String eventIdSubpart1, - String eventIdSubpart2, - String eventIdSubpart3, - String eventIdSubpart4 - ) { - return createEvent(Severity.NOTE, shape, shape.getSourceLocation(), message, eventIdSubpart1, eventIdSubpart2, - eventIdSubpart3, eventIdSubpart4); - } - protected final ValidationEvent note( Shape shape, FromSourceLocation location, @@ -411,19 +324,6 @@ protected final ValidationEvent note( return createEvent(Severity.NOTE, shape, location, message, eventIdSubpart1, eventIdSubpart2, eventIdSubpart3); } - protected final ValidationEvent note( - Shape shape, - FromSourceLocation location, - String message, - String eventIdSubpart1, - String eventIdSubpart2, - String eventIdSubpart3, - String eventIdSubpart4 - ) { - return createEvent(Severity.NOTE, shape, location, message, eventIdSubpart1, eventIdSubpart2, eventIdSubpart3, - eventIdSubpart4); - } - protected final ValidationEvent createEvent( Severity severity, Shape shape, @@ -489,24 +389,4 @@ protected final ValidationEvent createEvent( .id(getName() + "." + eventIdSubpart1 + "." + eventIdSubpart2 + "." + eventIdSubpart3) .build(); } - - protected final ValidationEvent createEvent( - Severity severity, - Shape shape, - FromSourceLocation loc, - String message, - String eventIdSubpart1, - String eventIdSubpart2, - String eventIdSubpart3, - String eventIdSubpart4 - ) { - return ValidationEvent.builder() - .severity(severity) - .message(message) - .shapeId(shape.getId()) - .sourceLocation(loc.getSourceLocation()) - .id(getName() + "." + eventIdSubpart1 + "." + eventIdSubpart2 - + "." + eventIdSubpart3 + "." + eventIdSubpart4) - .build(); - } }