Skip to content
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 validation events decorators #1774

Merged
merged 8 commits into from
May 24, 2023
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -47,5 +47,7 @@ public final class ColorTheme {
public static final Style DIFF_TITLE = Style.of(Style.BG_BRIGHT_BLACK, Style.WHITE);
public static final Style DIFF_EVENT_TITLE = Style.of(Style.BG_BRIGHT_BLUE, Style.BLACK);

public static final Style HINT_TITLE = Style.of(Style.BRIGHT_GREEN);

private ColorTheme() {}
}
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,13 @@ public String format(ValidationEvent event) {

writeMessage(writer, event.getMessage());
writer.append(ls);
event.getHint().ifPresent(hint -> {
String hintLabel = "HINT: ";
writer.append(ls);
colors.style(writer, hintLabel, ColorTheme.HINT_TITLE);
writer.append(StyleHelper.formatMessage(hint, LINE_LENGTH - hintLabel.length(), colors));
writer.append(ls);
});

return writer.toString();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -51,23 +51,60 @@ public void formatsEventsWithNoColors() {
}

@Test
public void formatsEventsWithColors() {
PrettyAnsiValidationFormatter pretty = createFormatter(AnsiColorFormatter.FORCE_COLOR);
String formatted = formatTestEventWithSeverity(pretty, Severity.ERROR);
public void formatsEventsWithHintWithNoColors() {
PrettyAnsiValidationFormatter pretty = createFormatter(AnsiColorFormatter.NO_COLOR);
String formatted = formatTestEventWithSeverity(pretty, Severity.ERROR, "Some hint");

assertThat(formatted, equalTo(
"\n"
+ "\u001B[31m── \u001B[0m\u001B[41;30m ERROR \u001B[0m\u001B[31m ───────────────────────────────────────────────────────────────── \u001B[0mFoo\n"
+ "\u001B[90mShape: \u001B[0m\u001B[95msmithy.example#Foo\u001B[0m\n"
+ "\u001B[90mFile: \u001B[0m\u001B[90mbuild/resources/test/software/amazon/smithy/cli/commands/valid-model.smithy:5:1\u001B[0m\n"
+ "── ERROR ───────────────────────────────────────────────────────────────── Foo\n"
+ "Shape: smithy.example#Foo\n"
+ "File: build/resources/test/software/amazon/smithy/cli/commands/valid-model.smithy:5:1\n"
+ "\n"
+ "\u001B[90m4| \u001B[0m\n"
+ "\u001B[90m5| \u001B[0mresource Foo {\n"
+ "\u001B[90m |\u001B[0m \u001B[31m^\u001B[0m\n"
+ "4| \n"
+ "5| resource Foo {\n"
+ " | ^\n"
+ "\n"
+ "Hello, \u001B[36mthere\u001B[0m\n"));
+ "Hello, `there`\n\n"
+ "HINT: Some hint\n")); // keeps ticks because formatting is disabled.
}

@Test
public void formatsEventsWithColors() {
PrettyAnsiValidationFormatter pretty = createFormatter(AnsiColorFormatter.FORCE_COLOR);
String formatted = formatTestEventWithSeverity(pretty, Severity.ERROR);

assertThat(formatted, equalTo(
"\n"
+ "\u001B[31m── \u001B[0m\u001B[41;30m ERROR \u001B[0m\u001B[31m ───────────────────────────────────────────────────────────────── \u001B[0mFoo\n"
+ "\u001B[90mShape: \u001B[0m\u001B[95msmithy.example#Foo\u001B[0m\n"
+ "\u001B[90mFile: \u001B[0m\u001B[90mbuild/resources/test/software/amazon/smithy/cli/commands/valid-model.smithy:5:1\u001B[0m\n"
+ "\n"
+ "\u001B[90m4| \u001B[0m\n"
+ "\u001B[90m5| \u001B[0mresource Foo {\n"
+ "\u001B[90m |\u001B[0m \u001B[31m^\u001B[0m\n"
+ "\n"
+ "Hello, \u001B[36mthere\u001B[0m\n"));
}

@Test
public void formatsEventsWithHintWithColors() {
PrettyAnsiValidationFormatter pretty = createFormatter(AnsiColorFormatter.FORCE_COLOR);
String formatted = formatTestEventWithSeverity(pretty, Severity.ERROR, "Some hint");

assertThat(formatted, equalTo(
"\n"
+ "\u001B[31m── \u001B[0m\u001B[41;30m ERROR \u001B[0m\u001B[31m ───────────────────────────────────────────────────────────────── \u001B[0mFoo\n"
+ "\u001B[90mShape: \u001B[0m\u001B[95msmithy.example#Foo\u001B[0m\n"
+ "\u001B[90mFile: \u001B[0m\u001B[90mbuild/resources/test/software/amazon/smithy/cli/commands/valid-model.smithy:5:1\u001B[0m\n"
+ "\n"
+ "\u001B[90m4| \u001B[0m\n"
+ "\u001B[90m5| \u001B[0mresource Foo {\n"
+ "\u001B[90m |\u001B[0m \u001B[31m^\u001B[0m\n"
+ "\n"
+ "Hello, \u001B[36mthere\u001B[0m\n\n"
+ "\u001B[92mHINT: \u001B[0mSome hint\n"));
}
@Test
public void doesNotIncludeSourceLocationNoneInOutput() {
PrettyAnsiValidationFormatter pretty = createFormatter(AnsiColorFormatter.NO_COLOR);
Expand Down Expand Up @@ -116,12 +153,17 @@ private PrettyAnsiValidationFormatter createFormatter(ColorFormatter colors) {
}

private String formatTestEventWithSeverity(PrettyAnsiValidationFormatter pretty, Severity severity) {
return formatTestEventWithSeverity(pretty, severity, null);
}

private String formatTestEventWithSeverity(PrettyAnsiValidationFormatter pretty, Severity severity, String hint) {
Model model = Model.assembler().addImport(getClass().getResource("valid-model.smithy")).assemble().unwrap();
ValidationEvent event = ValidationEvent.builder()
.id("Foo")
.severity(severity)
.shape(model.expectShape(ShapeId.from("smithy.example#Foo")))
.message("Hello, `there`")
.hint(hint)
.build();
return normalizeLinesAndFiles(pretty.format(event));
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@
final class LoaderTraitMap {

private static final Logger LOGGER = Logger.getLogger(LoaderTraitMap.class.getName());
private static final String UNRESOLVED_TRAIT_SUFFIX = ".UnresolvedTrait";

private final TraitFactory traitFactory;
private final Map<ShapeId, Map<ShapeId, Node>> traits = new HashMap<>();
Expand Down Expand Up @@ -114,7 +115,7 @@ private void validateTraitIsKnown(ShapeId target, ShapeId traitId, Trait trait,
if (!shapeMap.isRootShapeDefined(traitId) && (trait == null || !trait.isSynthetic())) {
Severity severity = allowUnknownTraits ? Severity.WARNING : Severity.ERROR;
events.add(ValidationEvent.builder()
.id(Validator.MODEL_ERROR)
.id(Validator.MODEL_ERROR + UNRESOLVED_TRAIT_SUFFIX)
.severity(severity)
.sourceLocation(sourceLocation)
.shapeId(target)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@
import software.amazon.smithy.model.validation.Severity;
import software.amazon.smithy.model.validation.ValidatedResult;
import software.amazon.smithy.model.validation.ValidationEvent;
import software.amazon.smithy.model.validation.ValidationEventDecorator;
import software.amazon.smithy.model.validation.Validator;
import software.amazon.smithy.model.validation.ValidatorFactory;
import software.amazon.smithy.utils.Pair;
Expand Down Expand Up @@ -509,6 +510,9 @@ public ValidatedResult<Model> assemble() {
if (traitFactory == null) {
traitFactory = LazyTraitFactoryHolder.INSTANCE;
}
if (validatorFactory == null) {
validatorFactory = ModelValidator.defaultValidationFactory();
}

Model prelude = disablePrelude ? null : Prelude.getPreludeModel();
LoadOperationProcessor processor = new LoadOperationProcessor(
Expand Down Expand Up @@ -576,7 +580,8 @@ public ValidatedResult<Model> assemble() {
}

if (disableValidation) {
return new ValidatedResult<>(transformed, events);
List<ValidationEventDecorator> decorators = validatorFactory.loadBuiltinDecorators();
return new ValidatedResult<>(transformed, ModelValidator.decorateEvents(decorators, events));
}

try {
Expand All @@ -594,9 +599,11 @@ private void addMetadataToProcessor(Map<String, Node> metadataMap, LoadOperation
}

private ValidatedResult<Model> returnOnlyErrors(Model model, List<ValidationEvent> events) {
List<ValidationEventDecorator> decorators = validatorFactory.loadBuiltinDecorators();
return new ValidatedResult<>(model, events.stream()
.filter(event -> event.getSeverity() == Severity.ERROR)
.collect(Collectors.toList()));
.filter(event -> event.getSeverity() == Severity.ERROR)
.map(event -> ModelValidator.decorateEvent(decorators, event))
.collect(Collectors.toList()));
}

private ValidatedResult<Model> validate(Model model, List<ValidationEvent> events) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
import software.amazon.smithy.model.validation.Severity;
import software.amazon.smithy.model.validation.ValidatedResult;
import software.amazon.smithy.model.validation.ValidationEvent;
import software.amazon.smithy.model.validation.ValidationEventDecorator;
import software.amazon.smithy.model.validation.Validator;
import software.amazon.smithy.model.validation.ValidatorFactory;
import software.amazon.smithy.model.validation.suppressions.Suppression;
Expand All @@ -49,7 +50,6 @@
* loaded from metadata.
*/
final class ModelValidator {

private static final String SUPPRESSIONS = "suppressions";

// Lazy initialization holder class idiom to hold a default validator factory.
Expand Down Expand Up @@ -169,6 +169,7 @@ public Validator createValidator() {
}

List<Validator> staticValidators = resolveStaticValidators();
List<ValidationEventDecorator> staticDecorators = validatorFactory.loadBuiltinDecorators();

return model -> {
List<ValidationEvent> coreEvents = new ArrayList<>();
Expand All @@ -186,6 +187,8 @@ public Validator createValidator() {
// which will only obscure the root cause.
coreEvents.addAll(suppressEvents(model, new TargetValidator().validate(model), modelSuppressions));
coreEvents.addAll(suppressEvents(model, new ResourceCycleValidator().validate(model), modelSuppressions));
// Decorate all the events
coreEvents = decorateEvents(staticDecorators, coreEvents);
// Emit any events that have already occurred.
coreEvents.forEach(eventListener);

Expand All @@ -194,16 +197,18 @@ public Validator createValidator() {
}

List<ValidationEvent> result = modelValidators.parallelStream()
.flatMap(validator -> validator.validate(model).stream())
.map(validator -> validator.validate(model))
.flatMap(Collection::stream)
.filter(ModelValidator::filterPrelude)
.map(event -> suppressEvent(model, event, modelSuppressions))
.map(event -> decorateEvent(staticDecorators, event))
// Emit events as they occur during validation.
.peek(eventListener)
.collect(Collectors.toList());

for (ValidationEvent event : includeEvents) {
if (ModelValidator.filterPrelude(event)) {
result.add(suppressEvent(model, event, modelSuppressions));
result.add(decorateEvent(staticDecorators, suppressEvent(model, event, modelSuppressions)));
}
}

Expand All @@ -214,6 +219,32 @@ public Validator createValidator() {
};
}

static List<ValidationEvent> decorateEvents(
List<ValidationEventDecorator> decorators,
List<ValidationEvent> events
) {
if (!decorators.isEmpty()) {
for (int idx = 0; idx < events.size(); idx++) {
events.set(idx, decorateEvent(decorators, events.get(idx)));
}
}
return events;
}

static ValidationEvent decorateEvent(List<ValidationEventDecorator> decorators, ValidationEvent event) {
ValidationEvent decoratedEvent = event;
for (ValidationEventDecorator decorator : decorators) {
if (decorator.canDecorate(event)) {
decoratedEvent = decorator.decorate(decoratedEvent);
}
}
return decoratedEvent;
}

static ValidatorFactory defaultValidationFactory() {
return LazyValidatorFactoryHolder.INSTANCE;
}

private List<Validator> resolveStaticValidators() {
List<Validator> resolvedValidators = new ArrayList<>(validatorFactory.loadBuiltinValidators());
resolvedValidators.addAll(validators);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,10 @@ public String format(ValidationEvent event) {
if (reason != null) {
message += " (" + reason + ")";
}
String hint = event.getHint().orElse(null);
if (hint != null) {
message += " [" + hint + "]";
}

return String.format(
"[%s] %s: %s | %s %s:%s:%s",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ public final class ValidationEvent
private final Severity severity;
private final ShapeId shapeId;
private final String suppressionReason;
private final String hint;
private int hash;

private ValidationEvent(Builder builder) {
Expand All @@ -61,6 +62,7 @@ private ValidationEvent(Builder builder) {
this.eventId = SmithyBuilder.requiredState("id", builder.eventId);
this.shapeId = builder.shapeId;
this.suppressionReason = builder.suppressionReason;
this.hint = builder.hint;
}

public static Builder builder() {
Expand Down Expand Up @@ -141,6 +143,7 @@ public Builder toBuilder() {
builder.eventId = eventId;
builder.shapeId = shapeId;
builder.suppressionReason = suppressionReason;
builder.hint = hint;
return builder;
}

Expand All @@ -158,14 +161,15 @@ public boolean equals(Object o) {
&& severity.equals(other.severity)
&& eventId.equals(other.eventId)
&& getShapeId().equals(other.getShapeId())
&& getSuppressionReason().equals(other.getSuppressionReason());
&& getSuppressionReason().equals(other.getSuppressionReason())
&& getHint().equals(other.getHint());
}

@Override
public int hashCode() {
int result = hash;
if (result == 0) {
result = Objects.hash(eventId, shapeId, severity, sourceLocation, message, suppressionReason);
result = Objects.hash(eventId, shapeId, severity, sourceLocation, message, suppressionReason, hint);
hash = result;
}
return result;
Expand All @@ -184,6 +188,7 @@ public Node toNode() {
.withOptionalMember("shapeId", getShapeId().map(Object::toString).map(Node::from))
.withMember("message", Node.from(getMessage()))
.withOptionalMember("suppressionReason", getSuppressionReason().map(Node::from))
.withOptionalMember("hint", getHint().map(Node::from))
.withMember("filename", Node.from(getSourceLocation().getFilename()))
.withMember("line", Node.from(getSourceLocation().getLine()))
.withMember("column", Node.from(getSourceLocation().getColumn()))
Expand All @@ -205,6 +210,7 @@ public static ValidationEvent fromNode(Node node) {
.expectMember("severity", Severity::fromNode, builder::severity)
.expectStringMember("message", builder::message)
.getStringMember("suppressionReason", builder::suppressionReason)
.getStringMember("hint", builder::hint)
.getMember("shapeId", ShapeId::fromNode, builder::shapeId);
return builder.build();
}
Expand Down Expand Up @@ -294,6 +300,15 @@ public Optional<String> getSuppressionReason() {
return Optional.ofNullable(suppressionReason);
}

/**
* Get an optional hint that adds more detail about how to fix a specific issue.
*
* @return Returns the hint if available.
*/
public Optional<String> getHint() {
return Optional.ofNullable(hint);
}

/**
* Builds ValidationEvent values.
*/
Expand All @@ -305,6 +320,7 @@ public static final class Builder implements SmithyBuilder<ValidationEvent> {
private String eventId;
private ShapeId shapeId;
private String suppressionReason;
private String hint;

private Builder() {}

Expand Down Expand Up @@ -402,6 +418,17 @@ public Builder suppressionReason(String eventSuppressionReason) {
return this;
}

/**
* Sets an optional hint adding more detail about how to fix a specific issue.
*
* @param hint Hint to set
* @return Returns the builder.
*/
public Builder hint(String hint) {
this.hint = hint;
return this;
}

@Override
public ValidationEvent build() {
return new ValidationEvent(this);
Expand Down
Loading