Skip to content

Commit

Permalink
Extend validation event ids to make them unique (#1527)
Browse files Browse the repository at this point in the history 
* Extend validation event ids to make them unique

* address comments, mostly to simplify some of the eventIds

* optimize assembleFullEventId method

* minor edits for style

* unroll eventId varargs in AbstractValidator for performance

* fix eventIds for Waiter violations

* remove unused method overloads
  • Loading branch information
@rchache
rchache authored Feb 15, 2023
1 parent 32b111f commit 2eef778
Show file tree
Hide file tree
Showing 33 changed files with 581 additions and 194 deletions.
18 changes: 11 additions & 7 deletions ...ws-traits/src/main/java/software/amazon/smithy/aws/traits/HttpChecksumTraitValidator.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -185,11 +185,13 @@ private List<ValidationEvent> validateHeaderConflicts(OperationShape operation,
.map(HttpPrefixHeadersTrait::getValue)
.ifPresent(headerPrefix -> {
if (HttpChecksumTrait.CHECKSUM_PREFIX.startsWith(headerPrefix)) {
String memberName = member.getId().getName();
String prefixString = headerPrefix.toLowerCase(Locale.US);
events.add(danger(operation, format("The `httpPrefixHeaders` binding of `%s` uses"
+ " the prefix `%s` that conflicts with the prefix `%s` used by the"
+ " `httpChecksum` trait.",
member.getId().getName(), headerPrefix.toLowerCase(Locale.US),
HttpChecksumTrait.CHECKSUM_PREFIX)));
+ " the prefix `%s` that conflicts with the prefix `%s` used by the"
+ " `httpChecksum` trait.",
memberName, prefixString, HttpChecksumTrait.CHECKSUM_PREFIX),
"HttpPrefixHeaders", memberName, prefixString));
}
});

Expand All @@ -200,10 +202,12 @@ private List<ValidationEvent> validateHeaderConflicts(OperationShape operation,
.map(HttpHeaderTrait::getValue)
.ifPresent(headerName -> {
if (headerName.startsWith(HttpChecksumTrait.CHECKSUM_PREFIX)) {
String memberName = member.getId().getName();
String headerString = headerName.toLowerCase(Locale.US);
events.add(warning(operation, format("The `httpHeader` binding of `%s` on `%s`"
+ " starts with the prefix `%s` used by the `httpChecksum` trait.",
headerName.toLowerCase(Locale.US), member.getId().getName(),
HttpChecksumTrait.CHECKSUM_PREFIX)));
+ " starts with the prefix `%s` used by the `httpChecksum` trait.",
headerString, memberName, HttpChecksumTrait.CHECKSUM_PREFIX),
"HttpHeader", memberName, headerString));
}
});
}
Expand Down
12 changes: 6 additions & 6 deletions ...ources/software/amazon/smithy/aws/traits/errorfiles/http-checksum-header-conflicts.errors
View file
Original file line number Diff line number Diff line change
@@ -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-
9 changes: 7 additions & 2 deletions ...ters/src/main/java/software/amazon/smithy/linters/InputOutputStructureReuseValidator.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,9 @@ public Provider() {
}
}

private static final String INPUT = "Input";
private static final String OUTPUT = "Output";

@Override
public List<ValidationEvent> validate(Model model) {
List<ValidationEvent> events = new ArrayList<>();
Expand All @@ -62,13 +65,15 @@ private void validateInputOutputSet(
"This structure is the input of `%s`, but it is not marked with the "
+ "@input trait. The @input trait gives operations more flexibility to "
+ "evolve their top-level input members in ways that would otherwise "
+ "be backward incompatible.", operation.getId())));
+ "be backward incompatible.", operation.getId()),
INPUT, operation.getId().getName()));
}

if (!output.hasTrait(OutputTrait.class)) {
events.add(warning(output, String.format(
"This structure is the output of `%s`, but it is not marked with "
+ "the @output trait.", operation.getId())));
+ "the @output trait.", operation.getId()),
OUTPUT, operation.getId().getName()));
}
}
}
105 changes: 56 additions & 49 deletions smithy-linters/src/main/java/software/amazon/smithy/linters/NoninclusiveTermsValidator.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -58,6 +59,10 @@ public Provider() {
}
}

private static final String TRAIT = "Trait";
private static final String SHAPE = "Shape";
private static final String NAMESPACE = "Namespace";

/**
* NoninclusiveTermsValidator configuration.
*/
Expand Down Expand Up @@ -121,7 +126,7 @@ public List<ValidationEvent> validate(Model model) {
/**
* Generates zero or more @see ValidationEvents and returns them in a collection.
*
* @param occurrence text occurrence found in the body of the model
* @param instance text occurrence found in the body of the model
*/
private Collection<ValidationEvent> getValidationEvents(TextInstance instance) {
final Collection<ValidationEvent> events = new ArrayList<>();
Expand All @@ -130,70 +135,72 @@ private Collection<ValidationEvent> getValidationEvents(TextInstance instance) {
final int startIndex = instance.getText().toLowerCase().indexOf(termLower);
if (startIndex != -1) {
final String matchedText = instance.getText().substring(startIndex, startIndex + termLower.length());
switch (instance.getLocationType()) {
case NAMESPACE:
//Cannot use any warning() overloads because there is no shape associated with the event.
events.add(ValidationEvent.builder()
.sourceLocation(SourceLocation.none())
.id(this.getClass().getSimpleName().replaceFirst("Validator$", ""))
.severity(Severity.WARNING)
.message(formatNonInclusiveTermsValidationMessage(termEntry, matchedText, instance))
.build());
break;
case APPLIED_TRAIT:
events.add(warning(instance.getShape(),
instance.getTrait().getSourceLocation(),
formatNonInclusiveTermsValidationMessage(termEntry, matchedText, instance)));
break;
case SHAPE:
default:
events.add(warning(instance.getShape(),
instance.getShape().getSourceLocation(),
formatNonInclusiveTermsValidationMessage(termEntry, matchedText, instance)));
}
events.add(constructValidationEvent(instance, termEntry.getValue(), matchedText));
}
}
return events;
}

private static String formatNonInclusiveTermsValidationMessage(
Map.Entry<String, List<String>> termEntry,
String matchedText,
TextInstance instance
) {
final List<String> caseCorrectedEntryValue = termEntry.getValue().stream()
.map(replacement -> Character.isUpperCase(matchedText.charAt(0))
? StringUtils.capitalize(replacement)
: StringUtils.uncapitalize(replacement))
.collect(Collectors.toList());
String replacementAddendum = !termEntry.getValue().isEmpty()
? String.format(" Consider using one of the following terms instead: %s",
ValidationUtils.tickedList(caseCorrectedEntryValue))
: "";
private ValidationEvent constructValidationEvent(TextInstance instance,
List<String> replacements,
String matchedText) {
String replacementAddendum = getReplacementAddendum(matchedText, replacements);
switch (instance.getLocationType()) {
case SHAPE:
return String.format("%s shape uses a non-inclusive term `%s`.%s",
StringUtils.capitalize(instance.getShape().getType().toString()),
matchedText, replacementAddendum);
case NAMESPACE:
return String.format("%s namespace uses a non-inclusive term `%s`.%s",
instance.getText(), matchedText, replacementAddendum);
//Cannot use any warning() overloads because there is no shape associated with the event.
return ValidationEvent.builder()
.severity(Severity.WARNING)
.sourceLocation(SourceLocation.none())
.id(getName() + "." + NAMESPACE + "." + instance.getText()
+ "." + matchedText.toLowerCase(Locale.US))
.message(String.format("%s namespace uses a non-inclusive term `%s`.%s",
instance.getText(), matchedText, replacementAddendum))
.build();
case APPLIED_TRAIT:
ValidationEvent validationEvent =
warning(instance.getShape(), instance.getTrait().getSourceLocation(), "");
String idiomaticTraitName = Trait.getIdiomaticTraitName(instance.getTrait());
if (instance.getTraitPropertyPath().isEmpty()) {
return String.format("'%s' trait has a value that contains a non-inclusive term `%s`.%s",
Trait.getIdiomaticTraitName(instance.getTrait()), matchedText,
replacementAddendum);
return validationEvent.toBuilder()
.message(String.format("'%s' trait has a value that contains a non-inclusive term `%s`.%s",
idiomaticTraitName, matchedText, replacementAddendum))
.id(getName() + "." + TRAIT + "."
+ matchedText.toLowerCase(Locale.US) + "." + idiomaticTraitName)
.build();
} else {
String valuePropertyPathFormatted = formatPropertyPath(instance.getTraitPropertyPath());
return String.format("'%s' trait value at path {%s} contains a non-inclusive term `%s`.%s",
Trait.getIdiomaticTraitName(instance.getTrait()), valuePropertyPathFormatted,
matchedText, replacementAddendum);
return validationEvent.toBuilder()
.message(String.format(
"'%s' trait value at path {%s} contains a non-inclusive term `%s`.%s",
idiomaticTraitName, valuePropertyPathFormatted, matchedText, replacementAddendum))
.id(getName() + "." + TRAIT + "." + matchedText.toLowerCase(Locale.US)
+ "." + idiomaticTraitName + "." + valuePropertyPathFormatted)
.build();
}
case SHAPE:
default:
throw new IllegalStateException();
return warning(instance.getShape(),
instance.getShape().getSourceLocation(),
String.format("%s shape uses a non-inclusive term `%s`.%s",
StringUtils.capitalize(instance.getShape().getType().toString()),
matchedText, replacementAddendum),
SHAPE, matchedText.toLowerCase(Locale.US));
}
}

private static String getReplacementAddendum(String matchedText, List<String> replacements) {
List<String> caseCorrectedEntryValue = replacements.stream()
.map(replacement -> Character.isUpperCase(matchedText.charAt(0))
? StringUtils.capitalize(replacement)
: StringUtils.uncapitalize(replacement))
.collect(Collectors.toList());
String replacementAddendum = !replacements.isEmpty()
? String.format(" Consider using one of the following terms instead: %s",
ValidationUtils.tickedList(caseCorrectedEntryValue))
: "";
return replacementAddendum;
}

private static String formatPropertyPath(List<String> traitPropertyPath) {
return String.join("/", traitPropertyPath);
}
Expand Down
4 changes: 2 additions & 2 deletions ...t/resources/software/amazon/smithy/linters/errorfiles/input-output-structure-reuse.errors
View file
Original file line number Diff line number Diff line change
@@ -1,2 +1,2 @@
[WARNING] smithy.example#GetFooInput: This structure is the input of `smithy.example#GetFoo`, but it is not marked with the @input trait. The @input trait gives operations more flexibility to evolve their top-level input members in ways that would otherwise be backward incompatible. | InputOutputStructureReuse
[WARNING] smithy.example#GetFooOutput: This structure is the output of `smithy.example#GetFoo`, but it is not marked with the @output trait. | InputOutputStructureReuse
[WARNING] smithy.example#GetFooInput: This structure is the input of `smithy.example#GetFoo`, but it is not marked with the @input trait. The @input trait gives operations more flexibility to evolve their top-level input members in ways that would otherwise be backward incompatible. | InputOutputStructureReuse.Input.GetFoo
[WARNING] smithy.example#GetFooOutput: This structure is the output of `smithy.example#GetFoo`, but it is not marked with the @output trait. | InputOutputStructureReuse.Output.GetFoo
Loading

0 comments on commit 2eef778

Please sign in to comment.