From 110b1010d3826616fe5359e28b51d3131c7284f6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20Sugawara=20=28=E2=88=A9=EF=BD=80-=C2=B4=29?= =?UTF-8?q?=E2=8A=83=E2=94=81=E7=82=8E=E7=82=8E=E7=82=8E=E7=82=8E=E7=82=8E?= Date: Tue, 16 May 2023 17:18:30 -0700 Subject: [PATCH 1/8] Add validation events decorators Adds a new interface `ValidationEventDecorator` that libraries can implement and plug into Smithy by making those discoverable using the java builtin [`ServiceLoader`](https://docs.oracle.com/javase/8/docs/api/java/util/ServiceLoader.html). All of the `ValidationEventDecorator` instances are loaded and each `ValidationEvent` is passed to each of the decorators. The decorators can make use of the `ValidationEvent#eventId()` method to quickly filter out the events that it knows how to decorate form the events it does not care about, and the `ValidationEvent#toBuilder()` to create a builder that can be used to mutate the instance. The current use case is to add hints to a know set of events to point the user into the right solution for it (e.g., you need to pull X or Y dependency for this unresolved shape). Since often these solutions might require specific details about the environment in which Smithy is being used, those decorators cannot be part of Smithy itself but now can be implemented by libraries and loaded automatically by Smithy in a similar way in which validators are currently loaded. For instance, a decorator that suggest the user to use or remove unused structures might look like this ```java @Override public ValidationEvent decorate(ValidationEvent ev) { if (ev.containsId("UnreferencedShape")) { if (ev.getMessage().contains("The structure ")) { return ev.toBuilder() .hint("Consider using this structure in your service or remove it from the model") .build(); } } return ev; } ``` --- .../amazon/smithy/cli/ColorTheme.java | 2 + .../PrettyAnsiValidationFormatter.java | 7 ++ .../PrettyAnsiValidationFormatterTest.java | 62 ++++++++++++++--- .../smithy/model/loader/ModelAssembler.java | 16 +++++ .../smithy/model/loader/ModelValidator.java | 52 ++++++++++++++- .../LineValidationEventFormatter.java | 4 ++ .../model/validation/ValidationEvent.java | 31 ++++++++- .../validation/ValidationEventDecorator.java | 34 ++++++++++ .../model/validation/ValidatorFactory.java | 25 ++++++- .../validators/TargetValidator.java | 7 +- .../ValidationEventDecoratorTest.java | 66 +++++++++++++++++++ .../model/validation/ValidationEventTest.java | 52 +++++++++++++++ 12 files changed, 341 insertions(+), 17 deletions(-) create mode 100644 smithy-model/src/main/java/software/amazon/smithy/model/validation/ValidationEventDecorator.java create mode 100644 smithy-model/src/test/java/software/amazon/smithy/model/validation/ValidationEventDecoratorTest.java diff --git a/smithy-cli/src/main/java/software/amazon/smithy/cli/ColorTheme.java b/smithy-cli/src/main/java/software/amazon/smithy/cli/ColorTheme.java index 0e9e6e33f32..5e012d005fb 100644 --- a/smithy-cli/src/main/java/software/amazon/smithy/cli/ColorTheme.java +++ b/smithy-cli/src/main/java/software/amazon/smithy/cli/ColorTheme.java @@ -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() {} } diff --git a/smithy-cli/src/main/java/software/amazon/smithy/cli/commands/PrettyAnsiValidationFormatter.java b/smithy-cli/src/main/java/software/amazon/smithy/cli/commands/PrettyAnsiValidationFormatter.java index 728642db73b..550fdabde85 100644 --- a/smithy-cli/src/main/java/software/amazon/smithy/cli/commands/PrettyAnsiValidationFormatter.java +++ b/smithy-cli/src/main/java/software/amazon/smithy/cli/commands/PrettyAnsiValidationFormatter.java @@ -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(); } diff --git a/smithy-cli/src/test/java/software/amazon/smithy/cli/commands/PrettyAnsiValidationFormatterTest.java b/smithy-cli/src/test/java/software/amazon/smithy/cli/commands/PrettyAnsiValidationFormatterTest.java index 4d3c9a314e9..35593ce2f55 100644 --- a/smithy-cli/src/test/java/software/amazon/smithy/cli/commands/PrettyAnsiValidationFormatterTest.java +++ b/smithy-cli/src/test/java/software/amazon/smithy/cli/commands/PrettyAnsiValidationFormatterTest.java @@ -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); @@ -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)); } diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/loader/ModelAssembler.java b/smithy-model/src/main/java/software/amazon/smithy/model/loader/ModelAssembler.java index 53c2abab1bc..c02dc859315 100644 --- a/smithy-model/src/main/java/software/amazon/smithy/model/loader/ModelAssembler.java +++ b/smithy-model/src/main/java/software/amazon/smithy/model/loader/ModelAssembler.java @@ -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; @@ -96,6 +97,7 @@ public final class ModelAssembler { private boolean disableValidation; private final Map> inputStreamModels = new LinkedHashMap<>(); private final List validators = new ArrayList<>(); + private final List decorators = new ArrayList<>(); private final List documentNodes = new ArrayList<>(); private final List mergeModels = new ArrayList<>(); private final List shapes = new ArrayList<>(); @@ -122,6 +124,7 @@ public ModelAssembler copy() { assembler.validatorFactory = validatorFactory; assembler.inputStreamModels.putAll(inputStreamModels); assembler.validators.addAll(validators); + assembler.decorators.addAll(decorators); assembler.documentNodes.addAll(documentNodes); assembler.mergeModels.addAll(mergeModels); assembler.shapes.addAll(shapes); @@ -165,6 +168,7 @@ public ModelAssembler reset() { mergeModels.clear(); inputStreamModels.clear(); validators.clear(); + decorators.clear(); documentNodes.clear(); disablePrelude = false; disableValidation = false; @@ -209,6 +213,17 @@ public ModelAssembler addValidator(Validator validator) { return this; } + /** + * Registers a validation event decorator to be used when validating the model. + * + * @param decorator Decorator to register. + * @return Returns the assembler. + */ + public ModelAssembler addDecorator(ValidationEventDecorator decorator) { + decorators.add(Objects.requireNonNull(decorator)); + return this; + } + /** * Adds a string containing an unparsed model to the assembler. * @@ -604,6 +619,7 @@ private ValidatedResult validate(Model model, List event // Note the ModelValidator handles emitting events to the validationEventListener. List mergedEvents = new ModelValidator() .validators(validators) + .decorators(decorators) .validatorFactory(validatorFactory) .eventListener(validationEventListener) .includeEvents(events) diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/loader/ModelValidator.java b/smithy-model/src/main/java/software/amazon/smithy/model/loader/ModelValidator.java index cdc8bbd30ea..3c05d2a6776 100644 --- a/smithy-model/src/main/java/software/amazon/smithy/model/loader/ModelValidator.java +++ b/smithy-model/src/main/java/software/amazon/smithy/model/loader/ModelValidator.java @@ -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; @@ -65,6 +66,7 @@ private static final class LazyValidatorFactoryHolder { ); private final List validators = new ArrayList<>(); + private final List decorators = new ArrayList<>(); private final List suppressions = new ArrayList<>(); private final List includeEvents = new ArrayList<>(); private ValidatorFactory validatorFactory; @@ -93,6 +95,29 @@ public ModelValidator addValidator(Validator validator) { return this; } + /** + * Adds a custom {@link ValidationEventDecorator} to the ModelValidator. + * + * @param decorator Decorator to add. + * @return Returns the ModelValidator. + */ + public ModelValidator addDecorator(ValidationEventDecorator decorator) { + decorators.add(Objects.requireNonNull(decorator)); + return this; + } + + /** + * Sets the custom {@link ValidationEventDecorator}s to use when running the ModelValidator. + * + * @param decorators Decorators to set. + * @return Returns the ModelValidator. + */ + public ModelValidator decorators(Collection decorators) { + this.decorators.clear(); + decorators.forEach(this::addDecorator); + return this; + } + /** * Sets the {@link Suppression}s to use with the validator. * @@ -169,6 +194,7 @@ public Validator createValidator() { } List staticValidators = resolveStaticValidators(); + List staticDecorators = resolveStaticDecorators(); return model -> { List coreEvents = new ArrayList<>(); @@ -186,6 +212,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); @@ -194,7 +222,9 @@ public Validator createValidator() { } List result = modelValidators.parallelStream() - .flatMap(validator -> validator.validate(model).stream()) + .map(validator -> validator.validate(model)) + .map(events -> decorateEvents(staticDecorators, events)) + .flatMap(Collection::stream) .filter(ModelValidator::filterPrelude) .map(event -> suppressEvent(model, event, modelSuppressions)) // Emit events as they occur during validation. @@ -214,6 +244,20 @@ public Validator createValidator() { }; } + private List decorateEvents(List decorators, + List events) { + if (!decorators.isEmpty()) { + List decorated = new ArrayList<>(events.size()); + for (ValidationEventDecorator decorator : decorators) { + for (ValidationEvent event : events) { + decorated.add(decorator.decorate(event)); + } + } + return decorated; + } + return events; + } + private List resolveStaticValidators() { List resolvedValidators = new ArrayList<>(validatorFactory.loadBuiltinValidators()); resolvedValidators.addAll(validators); @@ -222,6 +266,12 @@ private List resolveStaticValidators() { return resolvedValidators; } + private List resolveStaticDecorators() { + List resolvedDecorators = new ArrayList<>(validatorFactory.loadBuiltinDecorators()); + resolvedDecorators.addAll(decorators); + return resolvedDecorators; + } + private static boolean filterPrelude(ValidationEvent event) { // Don't emit any non-error events for prelude shapes and traits. // This prevents custom validators from unnecessarily needing to diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/validation/LineValidationEventFormatter.java b/smithy-model/src/main/java/software/amazon/smithy/model/validation/LineValidationEventFormatter.java index 03f7abbea62..e3b39670c19 100644 --- a/smithy-model/src/main/java/software/amazon/smithy/model/validation/LineValidationEventFormatter.java +++ b/smithy-model/src/main/java/software/amazon/smithy/model/validation/LineValidationEventFormatter.java @@ -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", diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/validation/ValidationEvent.java b/smithy-model/src/main/java/software/amazon/smithy/model/validation/ValidationEvent.java index d295d0162d5..f286c7d145e 100644 --- a/smithy-model/src/main/java/software/amazon/smithy/model/validation/ValidationEvent.java +++ b/smithy-model/src/main/java/software/amazon/smithy/model/validation/ValidationEvent.java @@ -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) { @@ -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() { @@ -141,6 +143,7 @@ public Builder toBuilder() { builder.eventId = eventId; builder.shapeId = shapeId; builder.suppressionReason = suppressionReason; + builder.hint = hint; return builder; } @@ -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; @@ -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())) @@ -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(); } @@ -294,6 +300,15 @@ public Optional 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 getHint() { + return Optional.ofNullable(hint); + } + /** * Builds ValidationEvent values. */ @@ -305,6 +320,7 @@ public static final class Builder implements SmithyBuilder { private String eventId; private ShapeId shapeId; private String suppressionReason; + private String hint; private Builder() {} @@ -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); diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/validation/ValidationEventDecorator.java b/smithy-model/src/main/java/software/amazon/smithy/model/validation/ValidationEventDecorator.java new file mode 100644 index 00000000000..275037f1625 --- /dev/null +++ b/smithy-model/src/main/java/software/amazon/smithy/model/validation/ValidationEventDecorator.java @@ -0,0 +1,34 @@ +/* + * Copyright 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.model.validation; + +/** + * Validation events decorators take validations events and transform them by adding more contextual information, + * usually adding a hint to let the user know what can it be done to solve the issue. This might add context specific + * information that is not relevant for all cases such as links to internal knowledge sites or explicit instructions + * relevant only to the context where Smithy is being used. + */ +@FunctionalInterface +public interface ValidationEventDecorator { + + /** + * Takes an event and potentially updates it to decorate it. Returns the same event if this decorators does not know + * how to handle the event. + * + * @return The decorated event or the original one if no decoration took place. + */ + ValidationEvent decorate(ValidationEvent ev); +} diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/validation/ValidatorFactory.java b/smithy-model/src/main/java/software/amazon/smithy/model/validation/ValidatorFactory.java index 844d934a20e..51fc5c104f3 100644 --- a/smithy-model/src/main/java/software/amazon/smithy/model/validation/ValidatorFactory.java +++ b/smithy-model/src/main/java/software/amazon/smithy/model/validation/ValidatorFactory.java @@ -39,6 +39,15 @@ public interface ValidatorFactory { */ List loadBuiltinValidators(); + /** + * Returns a list of decorators. + * + * @return Returns the loaded decorators. + */ + default List loadBuiltinDecorators() { + return Collections.emptyList(); + } + /** * Creates and configures a Validator by name. * @@ -63,11 +72,17 @@ public interface ValidatorFactory { * @param services ValidatorService instances to use to create validators. * @return Returns the created ValidatorFactory. */ - static ValidatorFactory createServiceFactory(Iterable validators, Iterable services) { + static ValidatorFactory createServiceFactory( + Iterable validators, + Iterable services, + Iterable decorators + ) { List serviceList = new ArrayList<>(); services.forEach(serviceList::add); List validatorsList = new ArrayList<>(); validators.forEach(validatorsList::add); + List decoratorsList = new ArrayList<>(); + decorators.forEach(decoratorsList::add); return new ValidatorFactory() { @Override @@ -75,6 +90,11 @@ public List loadBuiltinValidators() { return Collections.unmodifiableList(validatorsList); } + @Override + public List loadBuiltinDecorators() { + return Collections.unmodifiableList(decoratorsList); + } + @Override public Optional createValidator(String name, ObjectNode configuration) { return serviceList.stream() @@ -95,6 +115,7 @@ public Optional createValidator(String name, ObjectNode configuration static ValidatorFactory createServiceFactory(ClassLoader classLoader) { return createServiceFactory( ServiceLoader.load(Validator.class, classLoader), - ServiceLoader.load(ValidatorService.class, classLoader)); + ServiceLoader.load(ValidatorService.class, classLoader), + ServiceLoader.load(ValidationEventDecorator.class, classLoader)); } } 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 bbee958916e..2070e54fb5a 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 @@ -53,6 +53,8 @@ public final class TargetValidator extends AbstractValidator { private static final Set INVALID_MEMBER_TARGETS = SetUtils.of( ShapeType.SERVICE, ShapeType.RESOURCE, ShapeType.OPERATION, ShapeType.MEMBER); + private static final String UNRESOLVED_SHAPE_PART = "UnresolvedShape"; + // Relationship types listed here are checked to see if a shape refers to a deprecated shape. private static final Map RELATIONSHIP_TYPE_DEPRECATION_MAPPINGS = MapUtils.of( RelationshipType.MEMBER_TARGET, "Member targets a deprecated shape", @@ -236,7 +238,8 @@ private ValidationEvent unresolvedTarget(Model model, Shape shape, Relationship if (rel.getRelationshipType() == RelationshipType.MEMBER_TARGET) { // Don't show the relationship type for invalid member targets. return error(shape, String.format( - "member shape targets an unresolved shape `%s`%s", rel.getNeighborShapeId(), suggestionText)); + "member shape targets an unresolved shape `%s`%s", rel.getNeighborShapeId(), suggestionText), + UNRESOLVED_SHAPE_PART); } else { // Use "a" or "an" depending on if the relationship starts with a vowel. String indefiniteArticle = isUppercaseVowel(rel.getRelationshipType().toString().charAt(0)) @@ -248,7 +251,7 @@ private ValidationEvent unresolvedTarget(Model model, Shape shape, Relationship indefiniteArticle, rel.getRelationshipType().toString().toLowerCase(Locale.US), rel.getNeighborShapeId(), - suggestionText)); + suggestionText), UNRESOLVED_SHAPE_PART); } } diff --git a/smithy-model/src/test/java/software/amazon/smithy/model/validation/ValidationEventDecoratorTest.java b/smithy-model/src/test/java/software/amazon/smithy/model/validation/ValidationEventDecoratorTest.java new file mode 100644 index 00000000000..9985cfd79b8 --- /dev/null +++ b/smithy-model/src/test/java/software/amazon/smithy/model/validation/ValidationEventDecoratorTest.java @@ -0,0 +1,66 @@ +/* + * Copyright 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.model.validation; + +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.equalTo; + +import java.util.Set; +import org.junit.jupiter.api.Test; +import software.amazon.smithy.model.Model; +import software.amazon.smithy.model.shapes.ShapeId; +import software.amazon.smithy.utils.SetUtils; + +public class ValidationEventDecoratorTest { + + static final String HINT = "Consider connecting this structure to a service"; + static final String UNREFERENCED_SHAPE_EVENT_ID = "UnreferencedShape"; + static final Set STRUCT_SHAPE_IDS = SetUtils.of(ShapeId.from("ns.foo#Structure"), + ShapeId.from("ns.foo#Structure2"), + ShapeId.from("ns.foo#Structure3")); + + @Test + public void something() { + ValidatedResult result = Model.assembler() + .addImport(NodeValidationVisitorTest.class.getResource("node-validator" + + ".json")) + .addDecorator(new DumpHintValidationEventDecorator()) + .assemble(); + for (ValidationEvent event : result.getValidationEvents()) { + ShapeId eventShapeId = event.getShapeId().orElse(null); + if (STRUCT_SHAPE_IDS.contains(eventShapeId)) { + assertThat(event.getHint().isPresent(), equalTo(true)); + assertThat(event.getHint().get(), equalTo(HINT)); + } else { + assertThat(event.getHint().isPresent(), equalTo(false)); + } + } + } + + static class DumpHintValidationEventDecorator implements ValidationEventDecorator { + @Override + public ValidationEvent decorate(ValidationEvent ev) { + if (ev.containsId(UNREFERENCED_SHAPE_EVENT_ID)) { + // This is fragile and might fail if we change the message, but the message is all we currently have + // to tell apart specific instances apart. + if (ev.getMessage().contains("The structure ")) { + return ev.toBuilder().hint(HINT).build(); + } + } + return ev; + } + } +} diff --git a/smithy-model/src/test/java/software/amazon/smithy/model/validation/ValidationEventTest.java b/smithy-model/src/test/java/software/amazon/smithy/model/validation/ValidationEventTest.java index 564a0827a0c..f4ab51bfd1d 100644 --- a/smithy-model/src/test/java/software/amazon/smithy/model/validation/ValidationEventTest.java +++ b/smithy-model/src/test/java/software/amazon/smithy/model/validation/ValidationEventTest.java @@ -92,6 +92,23 @@ public void loadsWithFromNode() { assertThat(event.getShapeId().get(), is(id)); } + @Test + public void loadsWithFromNodeWithHint() { + ShapeId id = ShapeId.from("ns.foo#baz"); + ValidationEvent event = ValidationEvent.fromNode(Node.parse( + "{\"id\": \"abc.foo\", \"severity\": \"SUPPRESSED\", \"suppressionReason\": \"my reason\", " + + "\"shapeId\": \"ns.foo#baz\", \"message\": \"The message\", " + + "\"hint\": \"The hint\", \"filename\": \"/path/to/file.smithy\", \"line\": 7, \"column\": 2}")); + + assertThat(event.getSeverity(), equalTo(Severity.SUPPRESSED)); + assertThat(event.getMessage(), equalTo("The message")); + assertThat(event.getId(), equalTo("abc.foo")); + assertThat(event.getSuppressionReason().get(), equalTo("my reason")); + assertThat(event.getShapeId().get(), is(id)); + assertThat(event.getHint().get(),equalTo("The hint")); + } + + @Test public void hasGetters() { ShapeId id = ShapeId.from("ns.foo#baz"); @@ -101,6 +118,7 @@ public void hasGetters() { .shapeId(id) .id("abc.foo") .suppressionReason("my reason") + .hint("The hint") .build(); assertThat(event.getSeverity(), equalTo(Severity.SUPPRESSED)); @@ -108,6 +126,7 @@ public void hasGetters() { assertThat(event.getId(), equalTo("abc.foo")); assertThat(event.getSuppressionReason().get(), equalTo("my reason")); assertThat(event.getShapeId().get(), is(id)); + assertThat(event.getHint().get(), equalTo("The hint")); assertThat(event.getSeverity(), is(Severity.SUPPRESSED)); } @@ -145,6 +164,7 @@ public void createsEventBuilderFromEvent() { .severity(Severity.SUPPRESSED) .shapeId(ShapeId.from("ns.foo#baz")) .id("abc.foo") + .hint("The hint") .suppressionReason("my reason") .build(); ValidationEvent other = event.toBuilder().build(); @@ -268,6 +288,23 @@ public void differentSuppressionReasonAreNotEqual() { assertNotEquals(a.hashCode(), b.hashCode()); } + @Test + public void differentHintAreNotEqual() { + ValidationEvent a = ValidationEvent.builder() + .message("The message") + .severity(Severity.SUPPRESSED) + .shapeId(ShapeId.from("ns.foo#bar")) + .id("abc.foo") + .suppressionReason("my reason") + .hint("The hint") + .sourceLocation(SourceLocation.none()) + .build(); + ValidationEvent b = a.toBuilder().hint("other hint").build(); + + assertNotEquals(a, b); + assertNotEquals(a.hashCode(), b.hashCode()); + } + @Test public void toStringContainsSeverityAndEventId() { ValidationEvent a = ValidationEvent.builder() @@ -318,6 +355,21 @@ public void toStringContainsSuppressionReason() { assertEquals(a.toString(), "[SUPPRESSED] ns.foo#baz: The message (Foo baz bar) | abc.foo file:1:2"); } + @Test + public void toStringDoesContainsHint() { + ValidationEvent a = ValidationEvent.builder() + .message("The message") + .severity(Severity.SUPPRESSED) + .id("abc.foo") + .shapeId(ShapeId.from("ns.foo#baz")) + .suppressionReason("Foo baz bar") + .hint("The hint") + .sourceLocation(new SourceLocation("file", 1, 2)) + .build(); + + assertEquals(a.toString(), "[SUPPRESSED] ns.foo#baz: The message (Foo baz bar) [The hint] | abc.foo file:1:2"); + } + @Test public void convertsToNode() { ValidationEvent a = ValidationEvent.builder() From cea15f6b535c6e986def5c1133a8dfbe7cf55bbe Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20Sugawara=20=28=E2=88=A9=EF=BD=80-=C2=B4=29?= =?UTF-8?q?=E2=8A=83=E2=94=81=E7=82=8E=E7=82=8E=E7=82=8E=E7=82=8E=E7=82=8E?= Date: Wed, 17 May 2023 16:26:39 -0700 Subject: [PATCH 2/8] Load the decorators using its own factory In order to be able to decorate events that are returned before running the validation logic we need to pull the decoration logic up to the ModelAssambler itself. For this, a new factory was created to load the decorators instead of coupling this logic with the ValidatorFactory one and the assembler will take care of loading them and passing them down to the validation logic. --- .../smithy/model/loader/LoaderTraitMap.java | 3 +- .../smithy/model/loader/ModelAssembler.java | 62 +++++++++++++++---- .../smithy/model/loader/ModelValidator.java | 43 +++++++------ .../validation/ValidationEventDecorator.java | 8 ++- .../ValidationEventDecoratorFactory.java | 59 ++++++++++++++++++ .../model/validation/ValidatorFactory.java | 25 +------- .../ValidationEventDecoratorTest.java | 19 +++--- 7 files changed, 155 insertions(+), 64 deletions(-) create mode 100644 smithy-model/src/main/java/software/amazon/smithy/model/validation/ValidationEventDecoratorFactory.java diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/loader/LoaderTraitMap.java b/smithy-model/src/main/java/software/amazon/smithy/model/loader/LoaderTraitMap.java index 25e184beabd..132d1661b89 100644 --- a/smithy-model/src/main/java/software/amazon/smithy/model/loader/LoaderTraitMap.java +++ b/smithy-model/src/main/java/software/amazon/smithy/model/loader/LoaderTraitMap.java @@ -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> traits = new HashMap<>(); @@ -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) diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/loader/ModelAssembler.java b/smithy-model/src/main/java/software/amazon/smithy/model/loader/ModelAssembler.java index c02dc859315..ecd2e3b9d6c 100644 --- a/smithy-model/src/main/java/software/amazon/smithy/model/loader/ModelAssembler.java +++ b/smithy-model/src/main/java/software/amazon/smithy/model/loader/ModelAssembler.java @@ -51,6 +51,7 @@ 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.ValidationEventDecoratorFactory; import software.amazon.smithy.model.validation.Validator; import software.amazon.smithy.model.validation.ValidatorFactory; import software.amazon.smithy.utils.Pair; @@ -94,10 +95,11 @@ public final class ModelAssembler { private TraitFactory traitFactory; private ValidatorFactory validatorFactory; + private ValidationEventDecoratorFactory validationEventDecoratorFactory; private boolean disableValidation; private final Map> inputStreamModels = new LinkedHashMap<>(); private final List validators = new ArrayList<>(); - private final List decorators = new ArrayList<>(); + private final List validationEventDecorators = new ArrayList<>(); private final List documentNodes = new ArrayList<>(); private final List mergeModels = new ArrayList<>(); private final List shapes = new ArrayList<>(); @@ -113,6 +115,12 @@ static final class LazyTraitFactoryHolder { static final TraitFactory INSTANCE = TraitFactory.createServiceFactory(ModelAssembler.class.getClassLoader()); } + // Lazy initialization holder class idiom to hold a default event validation decorator factory. + static final class LazyValidationEventDecoratorFactoryHolder { + static final ValidationEventDecoratorFactory INSTANCE = + ValidationEventDecoratorFactory.createServiceFactory(ModelAssembler.class.getClassLoader()); + } + /** * Creates a copy of the current model assembler. * @@ -122,9 +130,10 @@ public ModelAssembler copy() { ModelAssembler assembler = new ModelAssembler(); assembler.traitFactory = traitFactory; assembler.validatorFactory = validatorFactory; + assembler.validationEventDecoratorFactory = validationEventDecoratorFactory; assembler.inputStreamModels.putAll(inputStreamModels); assembler.validators.addAll(validators); - assembler.decorators.addAll(decorators); + assembler.validationEventDecorators.addAll(validationEventDecorators); assembler.documentNodes.addAll(documentNodes); assembler.mergeModels.addAll(mergeModels); assembler.shapes.addAll(shapes); @@ -168,7 +177,7 @@ public ModelAssembler reset() { mergeModels.clear(); inputStreamModels.clear(); validators.clear(); - decorators.clear(); + validationEventDecorators.clear(); documentNodes.clear(); disablePrelude = false; disableValidation = false; @@ -202,6 +211,22 @@ public ModelAssembler validatorFactory(ValidatorFactory validatorFactory) { return this; } + /** + * Sets a custom {@link ValidationEventDecoratorFactory} used to resolve validation event decorator definitions. + * + *

Note that if you do not provide an explicit decoratorFactory, a + * default factory is utilized that uses service discovery. + * + * @param validationEventDecoratorFactory ValidationEventDecorator factory to use. + * @return Returns the assembler. + */ + public ModelAssembler validationEventDecoratorFactory( + ValidationEventDecoratorFactory validationEventDecoratorFactory + ) { + this.validationEventDecoratorFactory = Objects.requireNonNull(validationEventDecoratorFactory); + return this; + } + /** * Registers a validator to be used when validating the model. * @@ -219,8 +244,8 @@ public ModelAssembler addValidator(Validator validator) { * @param decorator Decorator to register. * @return Returns the assembler. */ - public ModelAssembler addDecorator(ValidationEventDecorator decorator) { - decorators.add(Objects.requireNonNull(decorator)); + public ModelAssembler addValidationEventDecorator(ValidationEventDecorator decorator) { + validationEventDecorators.add(Objects.requireNonNull(decorator)); return this; } @@ -524,6 +549,9 @@ public ValidatedResult assemble() { if (traitFactory == null) { traitFactory = LazyTraitFactoryHolder.INSTANCE; } + if (validationEventDecoratorFactory == null) { + validationEventDecoratorFactory = LazyValidationEventDecoratorFactoryHolder.INSTANCE; + } Model prelude = disablePrelude ? null : Prelude.getPreludeModel(); LoadOperationProcessor processor = new LoadOperationProcessor( @@ -587,7 +615,7 @@ public ValidatedResult assemble() { // If ERROR validation events occur while loading, then performing more // granular semantic validation will only obscure the root cause of errors. if (LoaderUtils.containsErrorEvents(events)) { - return returnOnlyErrors(transformed, events); + return returnOnlyErrors(transformed, events, resolveDecorators()); } if (disableValidation) { @@ -602,16 +630,28 @@ public ValidatedResult assemble() { } } + private List resolveDecorators() { + List resolvedDecorators = + new ArrayList<>(validationEventDecoratorFactory.loadDecorators()); + resolvedDecorators.addAll(validationEventDecorators); + return resolvedDecorators; + } + private void addMetadataToProcessor(Map metadataMap, LoadOperationProcessor processor) { for (Map.Entry entry : metadataMap.entrySet()) { processor.accept(new LoadOperation.PutMetadata(Version.UNKNOWN, entry.getKey(), entry.getValue())); } } - private ValidatedResult returnOnlyErrors(Model model, List events) { - return new ValidatedResult<>(model, events.stream() - .filter(event -> event.getSeverity() == Severity.ERROR) - .collect(Collectors.toList())); + private ValidatedResult returnOnlyErrors( + Model model, + List events, + List decorators + ) { + List filteredEvents = events.stream() + .filter(event -> event.getSeverity() == Severity.ERROR) + .collect(Collectors.toList()); + return new ValidatedResult<>(model, ModelValidator.decorateEvents(decorators, filteredEvents)); } private ValidatedResult validate(Model model, List events) { @@ -619,7 +659,7 @@ private ValidatedResult validate(Model model, List event // Note the ModelValidator handles emitting events to the validationEventListener. List mergedEvents = new ModelValidator() .validators(validators) - .decorators(decorators) + .validationEventDecorators(resolveDecorators()) .validatorFactory(validatorFactory) .eventListener(validationEventListener) .includeEvents(events) diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/loader/ModelValidator.java b/smithy-model/src/main/java/software/amazon/smithy/model/loader/ModelValidator.java index 3c05d2a6776..72310b886f2 100644 --- a/smithy-model/src/main/java/software/amazon/smithy/model/loader/ModelValidator.java +++ b/smithy-model/src/main/java/software/amazon/smithy/model/loader/ModelValidator.java @@ -66,7 +66,7 @@ private static final class LazyValidatorFactoryHolder { ); private final List validators = new ArrayList<>(); - private final List decorators = new ArrayList<>(); + private final List validationEventDecorators = new ArrayList<>(); private final List suppressions = new ArrayList<>(); private final List includeEvents = new ArrayList<>(); private ValidatorFactory validatorFactory; @@ -101,8 +101,8 @@ public ModelValidator addValidator(Validator validator) { * @param decorator Decorator to add. * @return Returns the ModelValidator. */ - public ModelValidator addDecorator(ValidationEventDecorator decorator) { - decorators.add(Objects.requireNonNull(decorator)); + public ModelValidator addValidationEventDecorator(ValidationEventDecorator decorator) { + validationEventDecorators.add(Objects.requireNonNull(decorator)); return this; } @@ -112,9 +112,9 @@ public ModelValidator addDecorator(ValidationEventDecorator decorator) { * @param decorators Decorators to set. * @return Returns the ModelValidator. */ - public ModelValidator decorators(Collection decorators) { - this.decorators.clear(); - decorators.forEach(this::addDecorator); + public ModelValidator validationEventDecorators(Collection decorators) { + this.validationEventDecorators.clear(); + decorators.forEach(this::addValidationEventDecorator); return this; } @@ -194,7 +194,6 @@ public Validator createValidator() { } List staticValidators = resolveStaticValidators(); - List staticDecorators = resolveStaticDecorators(); return model -> { List coreEvents = new ArrayList<>(); @@ -213,7 +212,7 @@ public Validator createValidator() { 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); + coreEvents = decorateEvents(validationEventDecorators, coreEvents); // Emit any events that have already occurred. coreEvents.forEach(eventListener); @@ -223,7 +222,7 @@ public Validator createValidator() { List result = modelValidators.parallelStream() .map(validator -> validator.validate(model)) - .map(events -> decorateEvents(staticDecorators, events)) + .map(events -> decorateEvents(validationEventDecorators, events)) .flatMap(Collection::stream) .filter(ModelValidator::filterPrelude) .map(event -> suppressEvent(model, event, modelSuppressions)) @@ -244,13 +243,27 @@ public Validator createValidator() { }; } - private List decorateEvents(List decorators, - List events) { + /** + * Applies each of the given decorators to each of the validation events and returns the resulting list of + * validation events. + * + * @param decorators The decorators to apply + * @param events The events to decorate + * @return The decorated events + */ + public static List decorateEvents( + List decorators, + List events + ) { if (!decorators.isEmpty()) { List decorated = new ArrayList<>(events.size()); for (ValidationEventDecorator decorator : decorators) { for (ValidationEvent event : events) { - decorated.add(decorator.decorate(event)); + if (decorator.canDecorate(event)) { + decorated.add(decorator.decorate(event)); + } else { + decorated.add(event); + } } } return decorated; @@ -266,12 +279,6 @@ private List resolveStaticValidators() { return resolvedValidators; } - private List resolveStaticDecorators() { - List resolvedDecorators = new ArrayList<>(validatorFactory.loadBuiltinDecorators()); - resolvedDecorators.addAll(decorators); - return resolvedDecorators; - } - private static boolean filterPrelude(ValidationEvent event) { // Don't emit any non-error events for prelude shapes and traits. // This prevents custom validators from unnecessarily needing to diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/validation/ValidationEventDecorator.java b/smithy-model/src/main/java/software/amazon/smithy/model/validation/ValidationEventDecorator.java index 275037f1625..4df1b8b2481 100644 --- a/smithy-model/src/main/java/software/amazon/smithy/model/validation/ValidationEventDecorator.java +++ b/smithy-model/src/main/java/software/amazon/smithy/model/validation/ValidationEventDecorator.java @@ -21,8 +21,14 @@ * information that is not relevant for all cases such as links to internal knowledge sites or explicit instructions * relevant only to the context where Smithy is being used. */ -@FunctionalInterface public interface ValidationEventDecorator { + /** + * Returns true if this decorator knows how to decorate this event, usually by looking at the event id. + * + * @param ev The event to test against + * @return true if this decorator knows how to decorate this event + */ + boolean canDecorate(ValidationEvent ev); /** * Takes an event and potentially updates it to decorate it. Returns the same event if this decorators does not know diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/validation/ValidationEventDecoratorFactory.java b/smithy-model/src/main/java/software/amazon/smithy/model/validation/ValidationEventDecoratorFactory.java new file mode 100644 index 00000000000..4fcbd52ce7a --- /dev/null +++ b/smithy-model/src/main/java/software/amazon/smithy/model/validation/ValidationEventDecoratorFactory.java @@ -0,0 +1,59 @@ +/* + * Copyright 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.model.validation; + +import java.util.ArrayList; +import java.util.Collections; +import java.util.List; +import java.util.ServiceLoader; + +/** + * Creates {@link ValidationEventDecorator} instances. + */ +public interface ValidationEventDecoratorFactory { + /** + * Returns a list of decorators. + * + * @return Returns the loaded decorators. + */ + List loadDecorators(); + + /** + * Creates a {@code ValidationEventDecoratorFactory} that uses the given collection of + * {@code ValidationEventDecorator}. + * + * @param services List of TraitService provider instances. + * @return Returns the created TraitFactory. + */ + static ValidationEventDecoratorFactory createServiceFactory(Iterable services) { + List decorators = new ArrayList<>(); + services.forEach(decorators::add); + List unmodifiableDecorators = Collections.unmodifiableList(decorators); + return () -> unmodifiableDecorators; + } + + /** + * Creates a {@code ValidationEventDecoratorFactory} that discovers {@code ValidationEventDecorator} providers using + * the given ClassLoader. + * + * @param classLoader Class loader used to find TraitService providers. + * @return Returns the created ValidationEventDecoratorFactory. + */ + static ValidationEventDecoratorFactory createServiceFactory(ClassLoader classLoader) { + return createServiceFactory(ServiceLoader.load(ValidationEventDecorator.class, classLoader)); + } + +} diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/validation/ValidatorFactory.java b/smithy-model/src/main/java/software/amazon/smithy/model/validation/ValidatorFactory.java index 51fc5c104f3..844d934a20e 100644 --- a/smithy-model/src/main/java/software/amazon/smithy/model/validation/ValidatorFactory.java +++ b/smithy-model/src/main/java/software/amazon/smithy/model/validation/ValidatorFactory.java @@ -39,15 +39,6 @@ public interface ValidatorFactory { */ List loadBuiltinValidators(); - /** - * Returns a list of decorators. - * - * @return Returns the loaded decorators. - */ - default List loadBuiltinDecorators() { - return Collections.emptyList(); - } - /** * Creates and configures a Validator by name. * @@ -72,17 +63,11 @@ default List loadBuiltinDecorators() { * @param services ValidatorService instances to use to create validators. * @return Returns the created ValidatorFactory. */ - static ValidatorFactory createServiceFactory( - Iterable validators, - Iterable services, - Iterable decorators - ) { + static ValidatorFactory createServiceFactory(Iterable validators, Iterable services) { List serviceList = new ArrayList<>(); services.forEach(serviceList::add); List validatorsList = new ArrayList<>(); validators.forEach(validatorsList::add); - List decoratorsList = new ArrayList<>(); - decorators.forEach(decoratorsList::add); return new ValidatorFactory() { @Override @@ -90,11 +75,6 @@ public List loadBuiltinValidators() { return Collections.unmodifiableList(validatorsList); } - @Override - public List loadBuiltinDecorators() { - return Collections.unmodifiableList(decoratorsList); - } - @Override public Optional createValidator(String name, ObjectNode configuration) { return serviceList.stream() @@ -115,7 +95,6 @@ public Optional createValidator(String name, ObjectNode configuration static ValidatorFactory createServiceFactory(ClassLoader classLoader) { return createServiceFactory( ServiceLoader.load(Validator.class, classLoader), - ServiceLoader.load(ValidatorService.class, classLoader), - ServiceLoader.load(ValidationEventDecorator.class, classLoader)); + ServiceLoader.load(ValidatorService.class, classLoader)); } } diff --git a/smithy-model/src/test/java/software/amazon/smithy/model/validation/ValidationEventDecoratorTest.java b/smithy-model/src/test/java/software/amazon/smithy/model/validation/ValidationEventDecoratorTest.java index 9985cfd79b8..65fd058e90f 100644 --- a/smithy-model/src/test/java/software/amazon/smithy/model/validation/ValidationEventDecoratorTest.java +++ b/smithy-model/src/test/java/software/amazon/smithy/model/validation/ValidationEventDecoratorTest.java @@ -37,7 +37,7 @@ public void something() { ValidatedResult result = Model.assembler() .addImport(NodeValidationVisitorTest.class.getResource("node-validator" + ".json")) - .addDecorator(new DumpHintValidationEventDecorator()) + .addValidationEventDecorator(new DummyHintValidationEventDecorator()) .assemble(); for (ValidationEvent event : result.getValidationEvents()) { ShapeId eventShapeId = event.getShapeId().orElse(null); @@ -50,17 +50,16 @@ public void something() { } } - static class DumpHintValidationEventDecorator implements ValidationEventDecorator { + static class DummyHintValidationEventDecorator implements ValidationEventDecorator { + + @Override + public boolean canDecorate(ValidationEvent ev) { + return ev.containsId(UNREFERENCED_SHAPE_EVENT_ID) && ev.getMessage().contains("The structure "); + } + @Override public ValidationEvent decorate(ValidationEvent ev) { - if (ev.containsId(UNREFERENCED_SHAPE_EVENT_ID)) { - // This is fragile and might fail if we change the message, but the message is all we currently have - // to tell apart specific instances apart. - if (ev.getMessage().contains("The structure ")) { - return ev.toBuilder().hint(HINT).build(); - } - } - return ev; + return ev.toBuilder().hint(HINT).build(); } } } From f89a84c7c976a486eb0bfc01921e83103c982fc1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20Sugawara=20=28=E2=88=A9=EF=BD=80-=C2=B4=29?= =?UTF-8?q?=E2=8A=83=E2=94=81=E7=82=8E=E7=82=8E=E7=82=8E=E7=82=8E=E7=82=8E?= Date: Thu, 18 May 2023 10:03:44 -0700 Subject: [PATCH 3/8] Fix an event duplication bug --- .../smithy/model/loader/ModelValidator.java | 23 ++++++++----- .../ValidationEventDecoratorTest.java | 33 ++++++++++++++++++- 2 files changed, 47 insertions(+), 9 deletions(-) diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/loader/ModelValidator.java b/smithy-model/src/main/java/software/amazon/smithy/model/loader/ModelValidator.java index 72310b886f2..37a84984672 100644 --- a/smithy-model/src/main/java/software/amazon/smithy/model/loader/ModelValidator.java +++ b/smithy-model/src/main/java/software/amazon/smithy/model/loader/ModelValidator.java @@ -23,6 +23,8 @@ import java.util.Optional; import java.util.Set; import java.util.function.Consumer; +import java.util.logging.Level; +import java.util.logging.Logger; import java.util.stream.Collectors; import software.amazon.smithy.model.Model; import software.amazon.smithy.model.SourceException; @@ -50,7 +52,7 @@ * loaded from metadata. */ final class ModelValidator { - + private static final Logger LOGGER = Logger.getLogger(ModelValidator.class.getName()); private static final String SUPPRESSIONS = "suppressions"; // Lazy initialization holder class idiom to hold a default validator factory. @@ -256,17 +258,22 @@ public static List decorateEvents( List events ) { if (!decorators.isEmpty()) { - List decorated = new ArrayList<>(events.size()); - for (ValidationEventDecorator decorator : decorators) { + try { + List decorated = new ArrayList<>(events.size()); for (ValidationEvent event : events) { - if (decorator.canDecorate(event)) { - decorated.add(decorator.decorate(event)); - } else { - decorated.add(event); + ValidationEvent decoratedEvent = event; + for (ValidationEventDecorator decorator : decorators) { + if (decorator.canDecorate(event)) { + decoratedEvent = decorator.decorate(event); + } } + decorated.add(decoratedEvent); } + return decorated; + } catch (Throwable e) { + LOGGER.log(Level.WARNING, e, () -> "A validation events decorator throw an exception, " + + "using the original set of events."); } - return decorated; } return events; } diff --git a/smithy-model/src/test/java/software/amazon/smithy/model/validation/ValidationEventDecoratorTest.java b/smithy-model/src/test/java/software/amazon/smithy/model/validation/ValidationEventDecoratorTest.java index 65fd058e90f..bde410c8c6e 100644 --- a/smithy-model/src/test/java/software/amazon/smithy/model/validation/ValidationEventDecoratorTest.java +++ b/smithy-model/src/test/java/software/amazon/smithy/model/validation/ValidationEventDecoratorTest.java @@ -17,7 +17,10 @@ import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.hasSize; +import static org.hamcrest.Matchers.notNullValue; +import java.util.List; import java.util.Set; import org.junit.jupiter.api.Test; import software.amazon.smithy.model.Model; @@ -33,7 +36,7 @@ public class ValidationEventDecoratorTest { ShapeId.from("ns.foo#Structure3")); @Test - public void something() { + public void canDecorateValidationEvents() { ValidatedResult result = Model.assembler() .addImport(NodeValidationVisitorTest.class.getResource("node-validator" + ".json")) @@ -50,6 +53,21 @@ public void something() { } } + @Test + public void returnsOriginalEventsWhenDecoratorsThrow() { + ValidatedResult result = Model.assembler() + .addImport(NodeValidationVisitorTest.class.getResource("node-validator" + + ".json")) + .addValidationEventDecorator(new ThrowingValidationEventDecorator()) + .assemble(); + List events = result.getValidationEvents(); + assertThat(events, notNullValue()); + assertThat(events, hasSize(31)); + for (ValidationEvent event : result.getValidationEvents()) { + assertThat(event.getHint().isPresent(), equalTo(false)); + } + } + static class DummyHintValidationEventDecorator implements ValidationEventDecorator { @Override @@ -62,4 +80,17 @@ public ValidationEvent decorate(ValidationEvent ev) { return ev.toBuilder().hint(HINT).build(); } } + + static class ThrowingValidationEventDecorator implements ValidationEventDecorator { + + @Override + public boolean canDecorate(ValidationEvent ev) { + return ev.containsId(UNREFERENCED_SHAPE_EVENT_ID) && ev.getMessage().contains("The structure "); + } + + @Override + public ValidationEvent decorate(ValidationEvent ev) { + throw new RuntimeException("ups"); + } + } } From 862c2c39aafe0f114568a2c5c44cab52b4b7d390 Mon Sep 17 00:00:00 2001 From: Manuel Sugawara Date: Mon, 22 May 2023 12:52:34 -0700 Subject: [PATCH 4/8] Update smithy-model/src/main/java/software/amazon/smithy/model/validation/ValidationEventDecorator.java Co-authored-by: Michael Dowling --- .../smithy/model/validation/ValidationEventDecorator.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/validation/ValidationEventDecorator.java b/smithy-model/src/main/java/software/amazon/smithy/model/validation/ValidationEventDecorator.java index 4df1b8b2481..a18560cbd7f 100644 --- a/smithy-model/src/main/java/software/amazon/smithy/model/validation/ValidationEventDecorator.java +++ b/smithy-model/src/main/java/software/amazon/smithy/model/validation/ValidationEventDecorator.java @@ -16,7 +16,7 @@ package software.amazon.smithy.model.validation; /** - * Validation events decorators take validations events and transform them by adding more contextual information, + * Validation event decorators take validation events and transform them by adding more contextual information, * usually adding a hint to let the user know what can it be done to solve the issue. This might add context specific * information that is not relevant for all cases such as links to internal knowledge sites or explicit instructions * relevant only to the context where Smithy is being used. From de1333c3729a1eec0c98937feb60e9f67eb38122 Mon Sep 17 00:00:00 2001 From: Manuel Sugawara Date: Mon, 22 May 2023 13:34:21 -0700 Subject: [PATCH 5/8] Update smithy-model/src/main/java/software/amazon/smithy/model/loader/ModelValidator.java Co-authored-by: Michael Dowling --- .../software/amazon/smithy/model/loader/ModelValidator.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/loader/ModelValidator.java b/smithy-model/src/main/java/software/amazon/smithy/model/loader/ModelValidator.java index 37a84984672..800e442b848 100644 --- a/smithy-model/src/main/java/software/amazon/smithy/model/loader/ModelValidator.java +++ b/smithy-model/src/main/java/software/amazon/smithy/model/loader/ModelValidator.java @@ -271,7 +271,7 @@ public static List decorateEvents( } return decorated; } catch (Throwable e) { - LOGGER.log(Level.WARNING, e, () -> "A validation events decorator throw an exception, " + LOGGER.log(Level.WARNING, e, () -> "A validation event decorator threw an exception, " + "using the original set of events."); } } From 458baebafb0609f463c8c8090942fe175b9161d6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20Sugawara=20=28=E2=88=A9=EF=BD=80-=C2=B4=29?= =?UTF-8?q?=E2=8A=83=E2=94=81=E7=82=8E=E7=82=8E=E7=82=8E=E7=82=8E=E7=82=8E?= Date: Mon, 22 May 2023 16:54:31 -0700 Subject: [PATCH 6/8] Address pull request comments * Move back the responsibility of loading the decorators to the ValidatorFactory and pass it down to the ModelValidator from the ModelAssembler * Overwrite the events in place in the list instead of crating a new list and copying + decorating. * Create a new method to decorate a single event and use it as part of the stream that returns core error events. * Remove the posibility of manually adding decorators in the validator and assembler and just use the ones loaded by the `ValidatorFacotry`. We don't need this now and we can add it back if and when needed. --- .../smithy/model/loader/ModelAssembler.java | 71 +++-------------- .../smithy/model/loader/ModelValidator.java | 78 +++++++------------ .../ValidationEventDecoratorFactory.java | 59 -------------- .../model/validation/ValidatorFactory.java | 25 +++++- .../ValidationEventDecoratorTest.java | 35 ++++++++- 5 files changed, 93 insertions(+), 175 deletions(-) delete mode 100644 smithy-model/src/main/java/software/amazon/smithy/model/validation/ValidationEventDecoratorFactory.java rename smithy-model/src/test/java/software/amazon/smithy/model/{validation => loader}/ValidationEventDecoratorTest.java (71%) diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/loader/ModelAssembler.java b/smithy-model/src/main/java/software/amazon/smithy/model/loader/ModelAssembler.java index ecd2e3b9d6c..e3c2875aa75 100644 --- a/smithy-model/src/main/java/software/amazon/smithy/model/loader/ModelAssembler.java +++ b/smithy-model/src/main/java/software/amazon/smithy/model/loader/ModelAssembler.java @@ -51,7 +51,6 @@ 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.ValidationEventDecoratorFactory; import software.amazon.smithy.model.validation.Validator; import software.amazon.smithy.model.validation.ValidatorFactory; import software.amazon.smithy.utils.Pair; @@ -95,11 +94,9 @@ public final class ModelAssembler { private TraitFactory traitFactory; private ValidatorFactory validatorFactory; - private ValidationEventDecoratorFactory validationEventDecoratorFactory; private boolean disableValidation; private final Map> inputStreamModels = new LinkedHashMap<>(); private final List validators = new ArrayList<>(); - private final List validationEventDecorators = new ArrayList<>(); private final List documentNodes = new ArrayList<>(); private final List mergeModels = new ArrayList<>(); private final List shapes = new ArrayList<>(); @@ -115,12 +112,6 @@ static final class LazyTraitFactoryHolder { static final TraitFactory INSTANCE = TraitFactory.createServiceFactory(ModelAssembler.class.getClassLoader()); } - // Lazy initialization holder class idiom to hold a default event validation decorator factory. - static final class LazyValidationEventDecoratorFactoryHolder { - static final ValidationEventDecoratorFactory INSTANCE = - ValidationEventDecoratorFactory.createServiceFactory(ModelAssembler.class.getClassLoader()); - } - /** * Creates a copy of the current model assembler. * @@ -130,10 +121,8 @@ public ModelAssembler copy() { ModelAssembler assembler = new ModelAssembler(); assembler.traitFactory = traitFactory; assembler.validatorFactory = validatorFactory; - assembler.validationEventDecoratorFactory = validationEventDecoratorFactory; assembler.inputStreamModels.putAll(inputStreamModels); assembler.validators.addAll(validators); - assembler.validationEventDecorators.addAll(validationEventDecorators); assembler.documentNodes.addAll(documentNodes); assembler.mergeModels.addAll(mergeModels); assembler.shapes.addAll(shapes); @@ -177,7 +166,6 @@ public ModelAssembler reset() { mergeModels.clear(); inputStreamModels.clear(); validators.clear(); - validationEventDecorators.clear(); documentNodes.clear(); disablePrelude = false; disableValidation = false; @@ -211,22 +199,6 @@ public ModelAssembler validatorFactory(ValidatorFactory validatorFactory) { return this; } - /** - * Sets a custom {@link ValidationEventDecoratorFactory} used to resolve validation event decorator definitions. - * - *

Note that if you do not provide an explicit decoratorFactory, a - * default factory is utilized that uses service discovery. - * - * @param validationEventDecoratorFactory ValidationEventDecorator factory to use. - * @return Returns the assembler. - */ - public ModelAssembler validationEventDecoratorFactory( - ValidationEventDecoratorFactory validationEventDecoratorFactory - ) { - this.validationEventDecoratorFactory = Objects.requireNonNull(validationEventDecoratorFactory); - return this; - } - /** * Registers a validator to be used when validating the model. * @@ -238,17 +210,6 @@ public ModelAssembler addValidator(Validator validator) { return this; } - /** - * Registers a validation event decorator to be used when validating the model. - * - * @param decorator Decorator to register. - * @return Returns the assembler. - */ - public ModelAssembler addValidationEventDecorator(ValidationEventDecorator decorator) { - validationEventDecorators.add(Objects.requireNonNull(decorator)); - return this; - } - /** * Adds a string containing an unparsed model to the assembler. * @@ -549,8 +510,8 @@ public ValidatedResult assemble() { if (traitFactory == null) { traitFactory = LazyTraitFactoryHolder.INSTANCE; } - if (validationEventDecoratorFactory == null) { - validationEventDecoratorFactory = LazyValidationEventDecoratorFactoryHolder.INSTANCE; + if (validatorFactory == null) { + validatorFactory = ModelValidator.defaultValidationFactory(); } Model prelude = disablePrelude ? null : Prelude.getPreludeModel(); @@ -615,11 +576,12 @@ public ValidatedResult assemble() { // If ERROR validation events occur while loading, then performing more // granular semantic validation will only obscure the root cause of errors. if (LoaderUtils.containsErrorEvents(events)) { - return returnOnlyErrors(transformed, events, resolveDecorators()); + return returnOnlyErrors(transformed, events); } if (disableValidation) { - return new ValidatedResult<>(transformed, events); + List decorators = validatorFactory.loadBuiltinDecorators(); + return new ValidatedResult<>(transformed, ModelValidator.decorateEvents(decorators, events)); } try { @@ -630,28 +592,18 @@ public ValidatedResult assemble() { } } - private List resolveDecorators() { - List resolvedDecorators = - new ArrayList<>(validationEventDecoratorFactory.loadDecorators()); - resolvedDecorators.addAll(validationEventDecorators); - return resolvedDecorators; - } - private void addMetadataToProcessor(Map metadataMap, LoadOperationProcessor processor) { for (Map.Entry entry : metadataMap.entrySet()) { processor.accept(new LoadOperation.PutMetadata(Version.UNKNOWN, entry.getKey(), entry.getValue())); } } - private ValidatedResult returnOnlyErrors( - Model model, - List events, - List decorators - ) { - List filteredEvents = events.stream() - .filter(event -> event.getSeverity() == Severity.ERROR) - .collect(Collectors.toList()); - return new ValidatedResult<>(model, ModelValidator.decorateEvents(decorators, filteredEvents)); + private ValidatedResult returnOnlyErrors(Model model, List events) { + List decorators = validatorFactory.loadBuiltinDecorators(); + return new ValidatedResult<>(model, events.stream() + .filter(event -> event.getSeverity() == Severity.ERROR) + .map(event -> ModelValidator.decorateEvent(decorators, event)) + .collect(Collectors.toList())); } private ValidatedResult validate(Model model, List events) { @@ -659,7 +611,6 @@ private ValidatedResult validate(Model model, List event // Note the ModelValidator handles emitting events to the validationEventListener. List mergedEvents = new ModelValidator() .validators(validators) - .validationEventDecorators(resolveDecorators()) .validatorFactory(validatorFactory) .eventListener(validationEventListener) .includeEvents(events) diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/loader/ModelValidator.java b/smithy-model/src/main/java/software/amazon/smithy/model/loader/ModelValidator.java index 800e442b848..7a900626f81 100644 --- a/smithy-model/src/main/java/software/amazon/smithy/model/loader/ModelValidator.java +++ b/smithy-model/src/main/java/software/amazon/smithy/model/loader/ModelValidator.java @@ -68,7 +68,6 @@ private static final class LazyValidatorFactoryHolder { ); private final List validators = new ArrayList<>(); - private final List validationEventDecorators = new ArrayList<>(); private final List suppressions = new ArrayList<>(); private final List includeEvents = new ArrayList<>(); private ValidatorFactory validatorFactory; @@ -97,29 +96,6 @@ public ModelValidator addValidator(Validator validator) { return this; } - /** - * Adds a custom {@link ValidationEventDecorator} to the ModelValidator. - * - * @param decorator Decorator to add. - * @return Returns the ModelValidator. - */ - public ModelValidator addValidationEventDecorator(ValidationEventDecorator decorator) { - validationEventDecorators.add(Objects.requireNonNull(decorator)); - return this; - } - - /** - * Sets the custom {@link ValidationEventDecorator}s to use when running the ModelValidator. - * - * @param decorators Decorators to set. - * @return Returns the ModelValidator. - */ - public ModelValidator validationEventDecorators(Collection decorators) { - this.validationEventDecorators.clear(); - decorators.forEach(this::addValidationEventDecorator); - return this; - } - /** * Sets the {@link Suppression}s to use with the validator. * @@ -196,6 +172,7 @@ public Validator createValidator() { } List staticValidators = resolveStaticValidators(); + List staticDecorators = validatorFactory.loadBuiltinDecorators(); return model -> { List coreEvents = new ArrayList<>(); @@ -214,7 +191,7 @@ public Validator createValidator() { coreEvents.addAll(suppressEvents(model, new TargetValidator().validate(model), modelSuppressions)); coreEvents.addAll(suppressEvents(model, new ResourceCycleValidator().validate(model), modelSuppressions)); // Decorate all the events - coreEvents = decorateEvents(validationEventDecorators, coreEvents); + coreEvents = decorateEvents(staticDecorators, coreEvents); // Emit any events that have already occurred. coreEvents.forEach(eventListener); @@ -224,17 +201,17 @@ public Validator createValidator() { List result = modelValidators.parallelStream() .map(validator -> validator.validate(model)) - .map(events -> decorateEvents(validationEventDecorators, events)) .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))); } } @@ -245,39 +222,38 @@ public Validator createValidator() { }; } - /** - * Applies each of the given decorators to each of the validation events and returns the resulting list of - * validation events. - * - * @param decorators The decorators to apply - * @param events The events to decorate - * @return The decorated events - */ - public static List decorateEvents( + static List decorateEvents( List decorators, List events ) { if (!decorators.isEmpty()) { - try { - List decorated = new ArrayList<>(events.size()); - for (ValidationEvent event : events) { - ValidationEvent decoratedEvent = event; - for (ValidationEventDecorator decorator : decorators) { - if (decorator.canDecorate(event)) { - decoratedEvent = decorator.decorate(event); - } - } - decorated.add(decoratedEvent); - } - return decorated; - } catch (Throwable e) { - LOGGER.log(Level.WARNING, e, () -> "A validation event decorator threw an exception, " - + "using the original set of events."); + for (int idx = 0; idx < events.size(); idx++) { + events.set(idx, decorateEvent(decorators, events.get(idx))); } } return events; } + static ValidationEvent decorateEvent(List decorators, ValidationEvent event) { + try { + ValidationEvent decoratedEvent = event; + for (ValidationEventDecorator decorator : decorators) { + if (decorator.canDecorate(event)) { + decoratedEvent = decorator.decorate(decoratedEvent); + } + } + return decoratedEvent; + } catch (Throwable e) { + LOGGER.log(Level.WARNING, e, () -> "A validation events decorator throw an exception, " + + "using the original set of events."); + } + return event; + } + + static ValidatorFactory defaultValidationFactory() { + return LazyValidatorFactoryHolder.INSTANCE; + } + private List resolveStaticValidators() { List resolvedValidators = new ArrayList<>(validatorFactory.loadBuiltinValidators()); resolvedValidators.addAll(validators); diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/validation/ValidationEventDecoratorFactory.java b/smithy-model/src/main/java/software/amazon/smithy/model/validation/ValidationEventDecoratorFactory.java deleted file mode 100644 index 4fcbd52ce7a..00000000000 --- a/smithy-model/src/main/java/software/amazon/smithy/model/validation/ValidationEventDecoratorFactory.java +++ /dev/null @@ -1,59 +0,0 @@ -/* - * Copyright 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.model.validation; - -import java.util.ArrayList; -import java.util.Collections; -import java.util.List; -import java.util.ServiceLoader; - -/** - * Creates {@link ValidationEventDecorator} instances. - */ -public interface ValidationEventDecoratorFactory { - /** - * Returns a list of decorators. - * - * @return Returns the loaded decorators. - */ - List loadDecorators(); - - /** - * Creates a {@code ValidationEventDecoratorFactory} that uses the given collection of - * {@code ValidationEventDecorator}. - * - * @param services List of TraitService provider instances. - * @return Returns the created TraitFactory. - */ - static ValidationEventDecoratorFactory createServiceFactory(Iterable services) { - List decorators = new ArrayList<>(); - services.forEach(decorators::add); - List unmodifiableDecorators = Collections.unmodifiableList(decorators); - return () -> unmodifiableDecorators; - } - - /** - * Creates a {@code ValidationEventDecoratorFactory} that discovers {@code ValidationEventDecorator} providers using - * the given ClassLoader. - * - * @param classLoader Class loader used to find TraitService providers. - * @return Returns the created ValidationEventDecoratorFactory. - */ - static ValidationEventDecoratorFactory createServiceFactory(ClassLoader classLoader) { - return createServiceFactory(ServiceLoader.load(ValidationEventDecorator.class, classLoader)); - } - -} diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/validation/ValidatorFactory.java b/smithy-model/src/main/java/software/amazon/smithy/model/validation/ValidatorFactory.java index 844d934a20e..51fc5c104f3 100644 --- a/smithy-model/src/main/java/software/amazon/smithy/model/validation/ValidatorFactory.java +++ b/smithy-model/src/main/java/software/amazon/smithy/model/validation/ValidatorFactory.java @@ -39,6 +39,15 @@ public interface ValidatorFactory { */ List loadBuiltinValidators(); + /** + * Returns a list of decorators. + * + * @return Returns the loaded decorators. + */ + default List loadBuiltinDecorators() { + return Collections.emptyList(); + } + /** * Creates and configures a Validator by name. * @@ -63,11 +72,17 @@ public interface ValidatorFactory { * @param services ValidatorService instances to use to create validators. * @return Returns the created ValidatorFactory. */ - static ValidatorFactory createServiceFactory(Iterable validators, Iterable services) { + static ValidatorFactory createServiceFactory( + Iterable validators, + Iterable services, + Iterable decorators + ) { List serviceList = new ArrayList<>(); services.forEach(serviceList::add); List validatorsList = new ArrayList<>(); validators.forEach(validatorsList::add); + List decoratorsList = new ArrayList<>(); + decorators.forEach(decoratorsList::add); return new ValidatorFactory() { @Override @@ -75,6 +90,11 @@ public List loadBuiltinValidators() { return Collections.unmodifiableList(validatorsList); } + @Override + public List loadBuiltinDecorators() { + return Collections.unmodifiableList(decoratorsList); + } + @Override public Optional createValidator(String name, ObjectNode configuration) { return serviceList.stream() @@ -95,6 +115,7 @@ public Optional createValidator(String name, ObjectNode configuration static ValidatorFactory createServiceFactory(ClassLoader classLoader) { return createServiceFactory( ServiceLoader.load(Validator.class, classLoader), - ServiceLoader.load(ValidatorService.class, classLoader)); + ServiceLoader.load(ValidatorService.class, classLoader), + ServiceLoader.load(ValidationEventDecorator.class, classLoader)); } } diff --git a/smithy-model/src/test/java/software/amazon/smithy/model/validation/ValidationEventDecoratorTest.java b/smithy-model/src/test/java/software/amazon/smithy/model/loader/ValidationEventDecoratorTest.java similarity index 71% rename from smithy-model/src/test/java/software/amazon/smithy/model/validation/ValidationEventDecoratorTest.java rename to smithy-model/src/test/java/software/amazon/smithy/model/loader/ValidationEventDecoratorTest.java index bde410c8c6e..b35a94efba4 100644 --- a/smithy-model/src/test/java/software/amazon/smithy/model/validation/ValidationEventDecoratorTest.java +++ b/smithy-model/src/test/java/software/amazon/smithy/model/loader/ValidationEventDecoratorTest.java @@ -13,18 +13,27 @@ * permissions and limitations under the License. */ -package software.amazon.smithy.model.validation; +package software.amazon.smithy.model.loader; import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.hasSize; import static org.hamcrest.Matchers.notNullValue; +import java.util.Arrays; import java.util.List; +import java.util.Optional; import java.util.Set; import org.junit.jupiter.api.Test; import software.amazon.smithy.model.Model; +import software.amazon.smithy.model.node.ObjectNode; import software.amazon.smithy.model.shapes.ShapeId; +import software.amazon.smithy.model.validation.NodeValidationVisitorTest; +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.SetUtils; public class ValidationEventDecoratorTest { @@ -40,7 +49,7 @@ public void canDecorateValidationEvents() { ValidatedResult result = Model.assembler() .addImport(NodeValidationVisitorTest.class.getResource("node-validator" + ".json")) - .addValidationEventDecorator(new DummyHintValidationEventDecorator()) + .validatorFactory(testFactory(new DummyHintValidationEventDecorator())) .assemble(); for (ValidationEvent event : result.getValidationEvents()) { ShapeId eventShapeId = event.getShapeId().orElse(null); @@ -58,7 +67,7 @@ public void returnsOriginalEventsWhenDecoratorsThrow() { ValidatedResult result = Model.assembler() .addImport(NodeValidationVisitorTest.class.getResource("node-validator" + ".json")) - .addValidationEventDecorator(new ThrowingValidationEventDecorator()) + .validatorFactory(testFactory(new ThrowingValidationEventDecorator())) .assemble(); List events = result.getValidationEvents(); assertThat(events, notNullValue()); @@ -68,6 +77,26 @@ public void returnsOriginalEventsWhenDecoratorsThrow() { } } + static ValidatorFactory testFactory(ValidationEventDecorator decorator) { + ValidatorFactory defaultValidatorFactory = ModelValidator.defaultValidationFactory(); + return new ValidatorFactory() { + @Override + public List loadBuiltinValidators() { + return defaultValidatorFactory.loadBuiltinValidators(); + } + + @Override + public List loadBuiltinDecorators() { + return Arrays.asList(decorator); + } + + @Override + public Optional createValidator(String name, ObjectNode configuration) { + return defaultValidatorFactory.createValidator(name, configuration); + } + }; + } + static class DummyHintValidationEventDecorator implements ValidationEventDecorator { @Override From 027c2b7e993669bbe10b4c8c858f402deb492a7d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20Sugawara=20=28=E2=88=A9=EF=BD=80-=C2=B4=29?= =?UTF-8?q?=E2=8A=83=E2=94=81=E7=82=8E=E7=82=8E=E7=82=8E=E7=82=8E=E7=82=8E?= Date: Tue, 23 May 2023 10:39:27 -0700 Subject: [PATCH 7/8] Address pull request comments * Do not catch exceptions thrown by loaded decorators. We will ship like this for now and add later if the need arises. * Add a new method to create the validation factory instead of using the existing one to avoid braking backwards compatibility. --- .../smithy/model/loader/ModelValidator.java | 19 +++------- .../model/validation/ValidatorFactory.java | 36 +++++++++++++++++++ .../loader/ValidationEventDecoratorTest.java | 22 ++++++------ 3 files changed, 51 insertions(+), 26 deletions(-) diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/loader/ModelValidator.java b/smithy-model/src/main/java/software/amazon/smithy/model/loader/ModelValidator.java index 7a900626f81..ad1629d532e 100644 --- a/smithy-model/src/main/java/software/amazon/smithy/model/loader/ModelValidator.java +++ b/smithy-model/src/main/java/software/amazon/smithy/model/loader/ModelValidator.java @@ -23,8 +23,6 @@ import java.util.Optional; import java.util.Set; import java.util.function.Consumer; -import java.util.logging.Level; -import java.util.logging.Logger; import java.util.stream.Collectors; import software.amazon.smithy.model.Model; import software.amazon.smithy.model.SourceException; @@ -52,7 +50,6 @@ * loaded from metadata. */ final class ModelValidator { - private static final Logger LOGGER = Logger.getLogger(ModelValidator.class.getName()); private static final String SUPPRESSIONS = "suppressions"; // Lazy initialization holder class idiom to hold a default validator factory. @@ -235,19 +232,13 @@ static List decorateEvents( } static ValidationEvent decorateEvent(List decorators, ValidationEvent event) { - try { - ValidationEvent decoratedEvent = event; - for (ValidationEventDecorator decorator : decorators) { - if (decorator.canDecorate(event)) { - decoratedEvent = decorator.decorate(decoratedEvent); - } + ValidationEvent decoratedEvent = event; + for (ValidationEventDecorator decorator : decorators) { + if (decorator.canDecorate(event)) { + decoratedEvent = decorator.decorate(decoratedEvent); } - return decoratedEvent; - } catch (Throwable e) { - LOGGER.log(Level.WARNING, e, () -> "A validation events decorator throw an exception, " - + "using the original set of events."); } - return event; + return decoratedEvent; } static ValidatorFactory defaultValidationFactory() { diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/validation/ValidatorFactory.java b/smithy-model/src/main/java/software/amazon/smithy/model/validation/ValidatorFactory.java index 51fc5c104f3..a29b135346a 100644 --- a/smithy-model/src/main/java/software/amazon/smithy/model/validation/ValidatorFactory.java +++ b/smithy-model/src/main/java/software/amazon/smithy/model/validation/ValidatorFactory.java @@ -72,6 +72,42 @@ default List loadBuiltinDecorators() { * @param services ValidatorService instances to use to create validators. * @return Returns the created ValidatorFactory. */ + static ValidatorFactory createServiceFactory(Iterable validators, Iterable services) { + List serviceList = new ArrayList<>(); + services.forEach(serviceList::add); + List validatorsList = new ArrayList<>(); + validators.forEach(validatorsList::add); + + return new ValidatorFactory() { + @Override + public List loadBuiltinValidators() { + return Collections.unmodifiableList(validatorsList); + } + + @Override + public Optional createValidator(String name, ObjectNode configuration) { + return serviceList.stream() + .filter(service -> service.getName().equals(name)) + .map(service -> service.createValidator(configuration)) + .findFirst(); + } + }; + } + + /** + * Creates a ValidatorFactory that uses a collection of built-in validators + * and a collection of ValidatorService instances for configurable validators. + * + *

The validators and services passed to this method are copied into a + * List before creating the actual factory to avoid holding on to + * {@code ServiceLoader} VM-wide instances that potentially utilize a + * Thread's context ClassLoader. + * + * @param validators Built-in validators to provide from the factory. + * @param services ValidatorService instances to use to create validators. + * @param decorators ValidationEventDecorator instances to use to create decorators. + * @return Returns the created ValidatorFactory. + */ static ValidatorFactory createServiceFactory( Iterable validators, Iterable services, diff --git a/smithy-model/src/test/java/software/amazon/smithy/model/loader/ValidationEventDecoratorTest.java b/smithy-model/src/test/java/software/amazon/smithy/model/loader/ValidationEventDecoratorTest.java index b35a94efba4..04ca78aea83 100644 --- a/smithy-model/src/test/java/software/amazon/smithy/model/loader/ValidationEventDecoratorTest.java +++ b/smithy-model/src/test/java/software/amazon/smithy/model/loader/ValidationEventDecoratorTest.java @@ -19,11 +19,13 @@ import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.hasSize; import static org.hamcrest.Matchers.notNullValue; +import static org.junit.jupiter.api.Assertions.assertThrows; import java.util.Arrays; import java.util.List; import java.util.Optional; import java.util.Set; +import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.Test; import software.amazon.smithy.model.Model; import software.amazon.smithy.model.node.ObjectNode; @@ -63,18 +65,14 @@ public void canDecorateValidationEvents() { } @Test - public void returnsOriginalEventsWhenDecoratorsThrow() { - ValidatedResult result = Model.assembler() - .addImport(NodeValidationVisitorTest.class.getResource("node-validator" - + ".json")) - .validatorFactory(testFactory(new ThrowingValidationEventDecorator())) - .assemble(); - List events = result.getValidationEvents(); - assertThat(events, notNullValue()); - assertThat(events, hasSize(31)); - for (ValidationEvent event : result.getValidationEvents()) { - assertThat(event.getHint().isPresent(), equalTo(false)); - } + public void exceptionsAreNotCaughtWhenDecoratorsThrow() { + assertThrows(RuntimeException.class, () -> { + Model.assembler() + .addImport(NodeValidationVisitorTest.class.getResource("node-validator" + + ".json")) + .validatorFactory(testFactory(new ThrowingValidationEventDecorator())) + .assemble(); + }); } static ValidatorFactory testFactory(ValidationEventDecorator decorator) { From 8fba22054b0fc61fe4af3d4f24262f2f5f00cba0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20Sugawara=20=28=E2=88=A9=EF=BD=80-=C2=B4=29?= =?UTF-8?q?=E2=8A=83=E2=94=81=E7=82=8E=E7=82=8E=E7=82=8E=E7=82=8E=E7=82=8E?= Date: Wed, 24 May 2023 15:44:51 -0700 Subject: [PATCH 8/8] Address pull request comments Remove the 'builtin' qualifier that doesn't make sense in this context --- .../software/amazon/smithy/model/loader/ModelAssembler.java | 4 ++-- .../software/amazon/smithy/model/loader/ModelValidator.java | 2 +- .../amazon/smithy/model/validation/ValidatorFactory.java | 4 ++-- .../smithy/model/loader/ValidationEventDecoratorTest.java | 2 +- 4 files changed, 6 insertions(+), 6 deletions(-) diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/loader/ModelAssembler.java b/smithy-model/src/main/java/software/amazon/smithy/model/loader/ModelAssembler.java index e3c2875aa75..365ffe1f94f 100644 --- a/smithy-model/src/main/java/software/amazon/smithy/model/loader/ModelAssembler.java +++ b/smithy-model/src/main/java/software/amazon/smithy/model/loader/ModelAssembler.java @@ -580,7 +580,7 @@ public ValidatedResult assemble() { } if (disableValidation) { - List decorators = validatorFactory.loadBuiltinDecorators(); + List decorators = validatorFactory.loadDecorators(); return new ValidatedResult<>(transformed, ModelValidator.decorateEvents(decorators, events)); } @@ -599,7 +599,7 @@ private void addMetadataToProcessor(Map metadataMap, LoadOperation } private ValidatedResult returnOnlyErrors(Model model, List events) { - List decorators = validatorFactory.loadBuiltinDecorators(); + List decorators = validatorFactory.loadDecorators(); return new ValidatedResult<>(model, events.stream() .filter(event -> event.getSeverity() == Severity.ERROR) .map(event -> ModelValidator.decorateEvent(decorators, event)) diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/loader/ModelValidator.java b/smithy-model/src/main/java/software/amazon/smithy/model/loader/ModelValidator.java index ad1629d532e..27f540cb77e 100644 --- a/smithy-model/src/main/java/software/amazon/smithy/model/loader/ModelValidator.java +++ b/smithy-model/src/main/java/software/amazon/smithy/model/loader/ModelValidator.java @@ -169,7 +169,7 @@ public Validator createValidator() { } List staticValidators = resolveStaticValidators(); - List staticDecorators = validatorFactory.loadBuiltinDecorators(); + List staticDecorators = validatorFactory.loadDecorators(); return model -> { List coreEvents = new ArrayList<>(); diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/validation/ValidatorFactory.java b/smithy-model/src/main/java/software/amazon/smithy/model/validation/ValidatorFactory.java index a29b135346a..a338a37e4a8 100644 --- a/smithy-model/src/main/java/software/amazon/smithy/model/validation/ValidatorFactory.java +++ b/smithy-model/src/main/java/software/amazon/smithy/model/validation/ValidatorFactory.java @@ -44,7 +44,7 @@ public interface ValidatorFactory { * * @return Returns the loaded decorators. */ - default List loadBuiltinDecorators() { + default List loadDecorators() { return Collections.emptyList(); } @@ -127,7 +127,7 @@ public List loadBuiltinValidators() { } @Override - public List loadBuiltinDecorators() { + public List loadDecorators() { return Collections.unmodifiableList(decoratorsList); } diff --git a/smithy-model/src/test/java/software/amazon/smithy/model/loader/ValidationEventDecoratorTest.java b/smithy-model/src/test/java/software/amazon/smithy/model/loader/ValidationEventDecoratorTest.java index 04ca78aea83..ce744045187 100644 --- a/smithy-model/src/test/java/software/amazon/smithy/model/loader/ValidationEventDecoratorTest.java +++ b/smithy-model/src/test/java/software/amazon/smithy/model/loader/ValidationEventDecoratorTest.java @@ -84,7 +84,7 @@ public List loadBuiltinValidators() { } @Override - public List loadBuiltinDecorators() { + public List loadDecorators() { return Arrays.asList(decorator); }