-
Notifications
You must be signed in to change notification settings - Fork 218
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add inclusive words filter #931
Merged
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
200 changes: 200 additions & 0 deletions
200
smithy-linters/src/main/java/software/amazon/smithy/linters/NoninclusiveTermsValidator.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,200 @@ | ||
/* | ||
* Copyright 2021 Amazon.com, Inc. or its affiliates. All Rights Reserved. | ||
* | ||
* Licensed under the Apache License, Version 2.0 (the "License"). | ||
* You may not use this file except in compliance with the License. | ||
* A copy of the License is located at | ||
* | ||
* http://aws.amazon.com/apache2.0 | ||
* | ||
* or in the "license" file accompanying this file. This file is distributed | ||
* on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either | ||
* express or implied. See the License for the specific language governing | ||
* permissions and limitations under the License. | ||
*/ | ||
|
||
package software.amazon.smithy.linters; | ||
|
||
import java.util.ArrayList; | ||
import java.util.Collection; | ||
import java.util.Collections; | ||
import java.util.HashMap; | ||
import java.util.List; | ||
import java.util.Map; | ||
import java.util.stream.Collectors; | ||
import software.amazon.smithy.model.Model; | ||
import software.amazon.smithy.model.SourceLocation; | ||
import software.amazon.smithy.model.knowledge.TextIndex; | ||
import software.amazon.smithy.model.knowledge.TextInstance; | ||
import software.amazon.smithy.model.node.NodeMapper; | ||
import software.amazon.smithy.model.traits.Trait; | ||
import software.amazon.smithy.model.validation.AbstractValidator; | ||
import software.amazon.smithy.model.validation.Severity; | ||
import software.amazon.smithy.model.validation.ValidationEvent; | ||
import software.amazon.smithy.model.validation.ValidationUtils; | ||
import software.amazon.smithy.model.validation.ValidatorService; | ||
import software.amazon.smithy.utils.ListUtils; | ||
import software.amazon.smithy.utils.MapUtils; | ||
import software.amazon.smithy.utils.StringUtils; | ||
|
||
/** | ||
* <p>Validates that all shape names and values do not contain non-inclusive terms. | ||
*/ | ||
public final class NoninclusiveTermsValidator extends AbstractValidator { | ||
static final Map<String, List<String>> BUILT_IN_NONINCLUSIVE_TERMS = MapUtils.of( | ||
"master", ListUtils.of("primary", "parent", "main"), | ||
"slave", ListUtils.of("secondary", "replica", "clone", "child"), | ||
"blacklist", ListUtils.of("denyList"), | ||
"whitelist", ListUtils.of("allowList") | ||
); | ||
|
||
public static final class Provider extends ValidatorService.Provider { | ||
public Provider() { | ||
super(NoninclusiveTermsValidator.class, node -> { | ||
NodeMapper mapper = new NodeMapper(); | ||
return new NoninclusiveTermsValidator( | ||
mapper.deserialize(node, NoninclusiveTermsValidator.Config.class)); | ||
}); | ||
} | ||
} | ||
|
||
/** | ||
* NoninclusiveTermsValidator configuration. | ||
*/ | ||
public static final class Config { | ||
private Map<String, List<String>> terms = MapUtils.of(); | ||
private boolean excludeDefaults; | ||
|
||
public Map<String, List<String>> getTerms() { | ||
return terms; | ||
} | ||
|
||
public void setTerms(Map<String, List<String>> terms) { | ||
this.terms = terms; | ||
} | ||
|
||
public boolean getExcludeDefaults() { | ||
return excludeDefaults; | ||
} | ||
|
||
public void setExcludeDefaults(boolean excludeDefaults) { | ||
this.excludeDefaults = excludeDefaults; | ||
} | ||
} | ||
|
||
private final Map<String, List<String>> termsMap; | ||
|
||
private NoninclusiveTermsValidator(Config config) { | ||
Map<String, List<String>> termsMapInit = new HashMap<>(BUILT_IN_NONINCLUSIVE_TERMS); | ||
if (!config.getExcludeDefaults()) { | ||
termsMapInit.putAll(config.getTerms()); | ||
termsMap = Collections.unmodifiableMap(termsMapInit); | ||
} else { | ||
if (config.getTerms().isEmpty()) { | ||
//This configuration combination makes the validator a no-op. | ||
throw new IllegalArgumentException("Cannot set 'excludeDefaults' to true and leave " | ||
+ "'terms' empty or unspecified."); | ||
} | ||
termsMap = Collections.unmodifiableMap(config.getTerms()); | ||
} | ||
} | ||
|
||
/** | ||
* Runs a full text scan on a given model and stores the resulting TextOccurrences objects. | ||
* | ||
* Namespaces are checked against a global set per model. | ||
* | ||
* @param model Model to validate. | ||
* @return a list of ValidationEvents found by the implementer of getValidationEvents per the | ||
* TextOccurrences provided by this traversal. | ||
*/ | ||
@Override | ||
public List<ValidationEvent> validate(Model model) { | ||
TextIndex textIndex = TextIndex.of(model); | ||
List<ValidationEvent> validationEvents = new ArrayList<>(); | ||
for (TextInstance text : textIndex.getTextInstances()) { | ||
validationEvents.addAll(getValidationEvents(text)); | ||
} | ||
return validationEvents; | ||
} | ||
|
||
/** | ||
* Generates zero or more @see ValidationEvents and returns them in a collection. | ||
* | ||
* @param occurrence text occurrence found in the body of the model | ||
*/ | ||
private Collection<ValidationEvent> getValidationEvents(TextInstance instance) { | ||
final Collection<ValidationEvent> events = new ArrayList<>(); | ||
for (Map.Entry<String, List<String>> termEntry : termsMap.entrySet()) { | ||
final String termLower = termEntry.getKey().toLowerCase(); | ||
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))); | ||
} | ||
} | ||
} | ||
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)) | ||
: ""; | ||
switch (instance.getLocationType()) { | ||
DavidOgunsAWS marked this conversation as resolved.
Show resolved
Hide resolved
|
||
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); | ||
case APPLIED_TRAIT: | ||
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); | ||
} 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); | ||
} | ||
default: | ||
throw new IllegalStateException(); | ||
} | ||
} | ||
|
||
private static String formatPropertyPath(List<String> traitPropertyPath) { | ||
return String.join("/", traitPropertyPath); | ||
} | ||
} |
1 change: 1 addition & 0 deletions
1
...main/resources/META-INF/services/software.amazon.smithy.model.validation.ValidatorService
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
6 changes: 6 additions & 0 deletions
6
...esources/software/amazon/smithy/linters/errorfiles/noninclusive-term-filter-append.errors
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
[WARNING] -: ns.foo namespace uses a non-inclusive term `foo`. Consider using one of the following terms instead: `bar` | 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 | ||
[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#AInput$foo: Member shape uses a non-inclusive term `foo`. Consider using one of the following terms instead: `bar` | NoninclusiveTerms | ||
[WARNING] ns.foo#AInput$foo: 'documentation' trait has a value that contains a non-inclusive term `apple`. Consider using one of the following terms instead: `banana` | NoninclusiveTerms | ||
[WARNING] ns.foo#BlackListThings: 'documentation' trait has a value that contains a non-inclusive term `replacement`. | NoninclusiveTerms |
63 changes: 63 additions & 0 deletions
63
.../resources/software/amazon/smithy/linters/errorfiles/noninclusive-term-filter-append.json
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,63 @@ | ||
{ | ||
"smithy": "1.0", | ||
"shapes": { | ||
"ns.foo#MyMasterService": { | ||
"type": "service", | ||
"version": "2021-10-17", | ||
"operations": [ | ||
{ | ||
"target": "ns.foo#A" | ||
}, | ||
{ | ||
"target": "ns.foo#BlackListThings" | ||
} | ||
] | ||
}, | ||
"ns.foo#A": { | ||
"type": "operation", | ||
"input": { | ||
"target": "ns.foo#AInput" | ||
}, | ||
"output": { | ||
"target": "ns.foo#AOutput" | ||
}, | ||
"traits": { | ||
"smithy.api#readonly": {} | ||
} | ||
}, | ||
"ns.foo#AInput": { | ||
"type": "structure", | ||
"members": { | ||
"foo": { | ||
"target": "smithy.api#String", | ||
"traits": { | ||
"smithy.api#documentation": "These docs are apples!" | ||
} | ||
} | ||
} | ||
}, | ||
"ns.foo#AOutput": { | ||
"type": "structure" | ||
}, | ||
"ns.foo#BlackListThings": { | ||
"type": "operation", | ||
"traits": { | ||
"smithy.api#documentation": "Non-inclusive word with no replacement suggestion." | ||
} | ||
} | ||
}, | ||
"metadata": { | ||
"validators": [ | ||
{ | ||
"name": "NoninclusiveTerms", | ||
"configuration": { | ||
"terms": { | ||
"apple": ["banana"], | ||
"foo": ["bar"], | ||
"replacement": [] | ||
} | ||
} | ||
} | ||
] | ||
} | ||
} |
3 changes: 3 additions & 0 deletions
3
...ources/software/amazon/smithy/linters/errorfiles/noninclusive-term-filter-override.errors
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
[WARNING] -: ns.foo namespace uses a non-inclusive term `foo`. Consider using one of the following terms instead: `bar` | NoninclusiveTerms | ||
[WARNING] ns.foo#AInput$foo: Member shape uses a non-inclusive term `foo`. Consider using one of the following terms instead: `bar` | NoninclusiveTerms | ||
[WARNING] ns.foo#AInput$foo: 'documentation' trait has a value that contains a non-inclusive term `apple`. Consider using one of the following terms instead: `banana` | NoninclusiveTerms |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are either of these properties required? What's the default value for
appendDefaults
if it's not required?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will add documentation. It is worth noting there is a non-trivial, maybe counter intuitive defaults behavior:
Though
appendDefaults
defaults to false, ifnoninclusiveTerms
mappings is entirely unset or empty,appendDefaults
behaves as if it were true -- the built in mappings are present.noninclusiveTerms
has to be non-empty beforeappendDefaults
behavior applies. If this behavior is acceptable, then I'll focus on clear and concise documentation for it. If not, then I should change the implementation.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Property structure have changed a bit. But current properties are documented for required or not, along with what the default values are
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the reasoning for this structure change because you weren't happy with the behavior? I feel like a terms map and a
includeDefaults
-like boolean that defaults to true would be pretty clear and usable for customers.