diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/loader/LoadOperationProcessor.java b/smithy-model/src/main/java/software/amazon/smithy/model/loader/LoadOperationProcessor.java index 4a01bcacc5f..a54959d73c5 100644 --- a/smithy-model/src/main/java/software/amazon/smithy/model/loader/LoadOperationProcessor.java +++ b/smithy-model/src/main/java/software/amazon/smithy/model/loader/LoadOperationProcessor.java @@ -31,6 +31,7 @@ import software.amazon.smithy.model.shapes.ShapeType; import software.amazon.smithy.model.traits.TraitFactory; import software.amazon.smithy.model.validation.ValidationEvent; +import software.amazon.smithy.model.validation.ValidationEventDecorator; final class LoadOperationProcessor implements Consumer { @@ -47,22 +48,25 @@ final class LoadOperationProcessor implements Consumer { TraitFactory traitFactory, Model prelude, boolean allowUnknownTraits, - Consumer validationEventListener + Consumer validationEventListener, + ValidationEventDecorator decorator ) { // Emit events as the come in. this.events = new ArrayList() { @Override public boolean add(ValidationEvent e) { + e = decorator.decorate(e); validationEventListener.accept(e); return super.add(e); } @Override public boolean addAll(Collection validationEvents) { + ensureCapacity(size() + validationEvents.size()); for (ValidationEvent e : validationEvents) { - validationEventListener.accept(e); + add(e); } - return super.addAll(validationEvents); + return true; } }; 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 d042acc42cb..03e9d6f417b 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 @@ -36,9 +36,7 @@ import java.util.Set; import java.util.function.Consumer; import java.util.function.Supplier; -import java.util.logging.Level; import java.util.logging.Logger; -import java.util.stream.Collectors; import java.util.stream.Stream; import software.amazon.smithy.model.Model; import software.amazon.smithy.model.SourceException; @@ -47,7 +45,6 @@ import software.amazon.smithy.model.shapes.ShapeId; import software.amazon.smithy.model.traits.Trait; import software.amazon.smithy.model.traits.TraitFactory; -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; @@ -487,10 +484,9 @@ public ModelAssembler disableValidation() { * while loading and validating the model. * *

The consumer could be invoked simultaneously by multiple threads. It's - * up to the consumer to perform any necessary synchronization. Providing - * an event listener is useful for things like CLIs so that events can - * be streamed to stdout as soon as they are encountered, rather than - * waiting until the entire model is parsed and validated. + * up to the consumer to perform any necessary synchronization. If a validator + * or decorator throws, then there is no guarantee that all validation events + * are emitted to the listener. * * @param eventListener Listener invoked for each ValidationEvent. * @return Returns the assembler. @@ -510,13 +506,19 @@ public ValidatedResult assemble() { if (traitFactory == null) { traitFactory = LazyTraitFactoryHolder.INSTANCE; } + if (validatorFactory == null) { validatorFactory = ModelValidator.defaultValidationFactory(); } + // Create a singular, composed event decorator used to modify events. + ValidationEventDecorator decorator = ValidationEventDecorator.compose(validatorFactory.loadDecorators()); + Model prelude = disablePrelude ? null : Prelude.getPreludeModel(); + + // As issues are encountered, they are decorated and then emitted. LoadOperationProcessor processor = new LoadOperationProcessor( - traitFactory, prelude, areUnknownTraitsAllowed(), validationEventListener); + traitFactory, prelude, areUnknownTraitsAllowed(), validationEventListener, decorator); List events = processor.events(); // Register manually added metadata. @@ -560,38 +562,27 @@ public ValidatedResult assemble() { } Model processedModel = processor.buildModel(); - Model transformed; // Do the 1.0 -> 2.0 transform before full-model validation. - try { - transformed = new ModelInteropTransformer(processedModel, events, processor::getShapeVersion) - .transform(); - } catch (SourceException e) { - // The transformation shouldn't throw, but if it does, return here with the original model. - LOGGER.log(Level.SEVERE, "Error in ModelInteropTransformer: ", e); - events.add(ValidationEvent.fromSourceException(e)); - return new ValidatedResult<>(processedModel, events); - } + Model transformed = new ModelInteropTransformer(processedModel, events, processor::getShapeVersion).transform(); - // 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); + if (disableValidation || LoaderUtils.containsErrorEvents(events)) { + // All events have been emitted and decorated at this point. + return new ValidatedResult<>(transformed, events); } - if (disableValidation) { - ValidationEventDecorator decorator = ValidationEventDecorator.compose(validatorFactory.loadDecorators()); - for (int idx = 0; idx < events.size(); idx++) { - events.set(idx, decorator.decorate(events.get(idx))); - } + try { + List mergedEvents = ModelValidator.builder() + .validators(validators) + .validatorFactory(validatorFactory, decorator) + .eventListener(validationEventListener) + .includeEvents(events) + .build() + .validate(transformed); + return new ValidatedResult<>(transformed, mergedEvents); + } catch (SourceException e) { + events.add(ValidationEvent.fromSourceException(e)); return new ValidatedResult<>(transformed, events); - } else { - try { - return validate(transformed, events); - } catch (SourceException e) { - events.add(ValidationEvent.fromSourceException(e)); - return new ValidatedResult<>(transformed, events); - } } } @@ -601,28 +592,6 @@ private void addMetadataToProcessor(Map metadataMap, LoadOperation } } - private ValidatedResult returnOnlyErrors(Model model, List events) { - ValidationEventDecorator decorator = ValidationEventDecorator.compose(validatorFactory.loadDecorators()); - return new ValidatedResult<>(model, events.stream() - .filter(event -> event.getSeverity() == Severity.ERROR) - .map(decorator::decorate) - .collect(Collectors.toList())); - } - - private ValidatedResult validate(Model model, List events) { - // Validate the model based on the explicit validators and model metadata. - // Note the ModelValidator handles emitting events to the validationEventListener. - List mergedEvents = ModelValidator.builder() - .validators(validators) - .validatorFactory(validatorFactory) - .eventListener(validationEventListener) - .includeEvents(events) - .build() - .validate(model); - - return new ValidatedResult<>(model, mergedEvents); - } - private boolean areUnknownTraitsAllowed() { Object allowUnknown = properties.get(ModelAssembler.ALLOW_UNKNOWN_TRAITS); return allowUnknown != null && (boolean) allowUnknown; 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 3d957c1015a..af9284f5251 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 @@ -84,16 +84,14 @@ private static final class LazyValidatorFactoryHolder { private final List events = new ArrayList<>(); private final List validators = new ArrayList<>(); private final List severityOverrides = new ArrayList<>(); - private final List suppressions = new ArrayList<>(); private final ValidationEventDecorator validationEventDecorator; private final Consumer eventListener; ModelValidator(Builder builder) { this.validatorFactory = builder.validatorFactory; this.eventListener = builder.eventListener; - this.validationEventDecorator = ValidationEventDecorator.compose(validatorFactory.loadDecorators()); + this.validationEventDecorator = builder.validationEventDecorator; this.events.addAll(builder.includeEvents); - this.suppressions.addAll(builder.suppressions); this.validators.addAll(builder.validators); } @@ -113,10 +111,10 @@ static ValidatorFactory defaultValidationFactory() { static final class Builder implements SmithyBuilder { private final List validators = new ArrayList<>(); - private final List suppressions = new ArrayList<>(); private final List includeEvents = new ArrayList<>(); private ValidatorFactory validatorFactory = LazyValidatorFactoryHolder.INSTANCE; private Consumer eventListener = event -> { }; + private ValidationEventDecorator validationEventDecorator; private Builder() {} @@ -133,47 +131,29 @@ public Builder validators(Collection validators) { } /** - * Adds a custom {@link Validator} to the ModelValidator. + * Adds a custom {@link Validator}. * * @param validator Validator to add. - * @return Returns the ModelValidator. + * @return Returns the builder. */ public Builder addValidator(Validator validator) { validators.add(Objects.requireNonNull(validator)); return this; } - /** - * Sets the {@link Suppression}s to use with the validator. - * - * @param suppressions Suppressions to set. - * @return Returns the ModelValidator. - */ - public Builder suppressions(Collection suppressions) { - this.suppressions.clear(); - suppressions.forEach(this::addSuppression); - return this; - } - - /** - * Adds a custom {@link Suppression} to the validator. - * - * @param suppression Suppression to add. - * @return Returns the ModelValidator. - */ - public Builder addSuppression(Suppression suppression) { - suppressions.add(Objects.requireNonNull(suppression)); - return this; - } - /** * Sets the factory used to find built-in {@link Validator}s and to load validators found in model metadata. * * @param validatorFactory Factory to use to load {@code Validator}s. - * @return Returns the ModelValidator. + * @param validationEventDecorator Provide a previously loaded and composed decorator. + * @return Returns the builder. */ - public Builder validatorFactory(ValidatorFactory validatorFactory) { + public Builder validatorFactory( + ValidatorFactory validatorFactory, + ValidationEventDecorator validationEventDecorator + ) { this.validatorFactory = Objects.requireNonNull(validatorFactory); + this.validationEventDecorator = validationEventDecorator; return this; } @@ -181,7 +161,7 @@ public Builder validatorFactory(ValidatorFactory validatorFactory) { * Sets a custom event listener that receives each {@link ValidationEvent} as it is emitted. * * @param eventListener Event listener that consumes each event. - * @return Returns the ModelValidator. + * @return Returns the builder. */ public Builder eventListener(Consumer eventListener) { this.eventListener = Objects.requireNonNull(eventListener); @@ -191,10 +171,11 @@ public Builder eventListener(Consumer eventListener) { /** * Includes a set of events that were already encountered in the result. * - *

Suppressions and severity overrides will be applied to the given {@code events}. + *

Suppressions and severity overrides will be applied to the given {@code events}. However, the validator + * assumes that the event has already been decroated and the event listener has already seen the event. * * @param events Events to include. - * @return Returns the ModelValidator. + * @return Returns the builder. */ public Builder includeEvents(List events) { this.includeEvents.clear(); @@ -213,7 +194,7 @@ public ModelValidator build() { private static final class LoadedModelValidator { private final Model model; - private final List suppressions; + private final List suppressions = new ArrayList<>(); private final List severityOverrides; private final List validators; private final List events = new ArrayList<>(); @@ -224,38 +205,50 @@ private LoadedModelValidator(Model model, ModelValidator validator) { this.model = model; this.validationEventDecorator = validator.validationEventDecorator; this.eventListener = validator.eventListener; - this.suppressions = new ArrayList<>(validator.suppressions); this.severityOverrides = new ArrayList<>(validator.severityOverrides); this.validators = new ArrayList<>(validator.validators); - pushEvents(validator.events); loadMetadataSuppressions(); loadMetadataSeverityOverrides(); + + // Given events have already been emitted and decorated, but have not been suppressed/elevated. + for (ValidationEvent event : validator.events) { + events.add(modifyEventSeverity(event)); + } + loadModelValidators(validator.validatorFactory); } private void loadMetadataSeverityOverrides() { model.getMetadataProperty(SEVERITY_OVERRIDES).ifPresent(value -> { - List values = value.expectArrayNode().getElementsAs(ObjectNode.class); - for (ObjectNode rule : values) { - try { - severityOverrides.add(SeverityOverride.fromMetadata(rule)); - } catch (SourceException e) { - pushEvent(ValidationEvent.fromSourceException(e)); + try { + List values = value.expectArrayNode().getElementsAs(ObjectNode.class); + for (ObjectNode rule : values) { + try { + severityOverrides.add(SeverityOverride.fromMetadata(rule)); + } catch (SourceException e) { + pushEvent(ValidationEvent.fromSourceException(e)); + } } + } catch (SourceException e) { + pushEvent(ValidationEvent.fromSourceException(e)); } }); } private void loadMetadataSuppressions() { model.getMetadataProperty(SUPPRESSIONS).ifPresent(value -> { - List values = value.expectArrayNode().getElementsAs(ObjectNode.class); - for (ObjectNode rule : values) { - try { - suppressions.add(Suppression.fromMetadata(rule)); - } catch (SourceException e) { - pushEvent(ValidationEvent.fromSourceException(e)); + try { + List values = value.expectArrayNode().getElementsAs(ObjectNode.class); + for (ObjectNode rule : values) { + try { + suppressions.add(Suppression.fromMetadata(rule)); + } catch (SourceException e) { + pushEvent(ValidationEvent.fromSourceException(e)); + } } + } catch (SourceException e) { + pushEvent(ValidationEvent.fromSourceException(e)); } }); } diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/validation/suppressions/MetadataSeverityOverride.java b/smithy-model/src/main/java/software/amazon/smithy/model/validation/suppressions/MetadataSeverityOverride.java index 1a1cb4791b3..bb781251bf1 100644 --- a/smithy-model/src/main/java/software/amazon/smithy/model/validation/suppressions/MetadataSeverityOverride.java +++ b/smithy-model/src/main/java/software/amazon/smithy/model/validation/suppressions/MetadataSeverityOverride.java @@ -55,6 +55,6 @@ static MetadataSeverityOverride fromNode(Node node) { @Override public Severity apply(ValidationEvent event) { - return namespaceMatcher.test(event) ? severity : event.getSeverity(); + return event.containsId(id) && namespaceMatcher.test(event) ? severity : event.getSeverity(); } } diff --git a/smithy-model/src/test/java/software/amazon/smithy/model/loader/ModelAssemblerTest.java b/smithy-model/src/test/java/software/amazon/smithy/model/loader/ModelAssemblerTest.java index 1b5ff58d82f..9578cc80454 100644 --- a/smithy-model/src/test/java/software/amazon/smithy/model/loader/ModelAssemblerTest.java +++ b/smithy-model/src/test/java/software/amazon/smithy/model/loader/ModelAssemblerTest.java @@ -1247,4 +1247,29 @@ public void failsOnInvalidJarJsonFile() throws URISyntaxException, IOException { assertThat(e.getMessage(), containsString("Invalid file referenced by Smithy JAR manifest")); } + + @Test + public void doesNotThrowOnInvalidSuppression() { + ObjectNode node = Node.objectNode() + .withMember("smithy", "1.0") + .withMember("metadata", Node.objectNode().withMember("suppressions", "hi!")); + + ValidatedResult result = new ModelAssembler().addDocumentNode(node).assemble(); + + assertThat(result.getValidationEvents(Severity.ERROR), not(empty())); + } + + @Test + public void modelLoadingErrorsAreEmittedToListener() { + ObjectNode node = Node.objectNode().withMember("smithy", Node.fromStrings("Hi", "there")); + List events = new ArrayList<>(); + + ValidatedResult result = new ModelAssembler() + .addDocumentNode(node) + .validationEventListener(events::add) + .assemble(); + + assertThat(result.getValidationEvents(Severity.ERROR), hasSize(1)); + assertThat(events, equalTo(result.getValidationEvents())); + } } diff --git a/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/loader/json-invalid-list-and-map-members.errors b/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/loader/json-invalid-list-and-map-members.errors index 9130e4c9e8e..3742a6e7a84 100644 --- a/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/loader/json-invalid-list-and-map-members.errors +++ b/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/loader/json-invalid-list-and-map-members.errors @@ -6,3 +6,9 @@ [ERROR] com.test#WrongMemberMap: Missing required member of shape `com.test#WrongMemberMap`: key | Model [ERROR] com.test#OtherWrongMemberMap: Missing required member of shape `com.test#OtherWrongMemberMap`: value | Model [ERROR] com.test#BothWrongMembersMap: Missing required members of shape `com.test#BothWrongMembersMap`: key, value | Model +[WARNING] com.test#WrongMemberList: Expected an object with possible properties of `member`, `mixins`, `traits`, `type`, but found additional properties: `foo` | Model +[WARNING] com.test#WrongMemberMap: Expected an object with possible properties of `key`, `mixins`, `traits`, `type`, `value`, but found additional properties: `foo` | Model +[WARNING] com.test#OtherWrongMemberMap: Expected an object with possible properties of `key`, `mixins`, `traits`, `type`, `value`, but found additional properties: `foo` | Model +[WARNING] com.test#BothWrongMembersMap: Expected an object with possible properties of `key`, `mixins`, `traits`, `type`, `value`, but found additional properties: `bar`, `foo` | Model +[WARNING] com.test#ExtraMemberList: Expected an object with possible properties of `member`, `mixins`, `traits`, `type`, but found additional properties: `foo` | Model +[WARNING] com.test#ExtraMemberMap: Expected an object with possible properties of `key`, `mixins`, `traits`, `type`, `value`, but found additional properties: `foo` | Model diff --git a/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/loader/json-invalid-set-members.errors b/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/loader/json-invalid-set-members.errors index 4806e262593..190b9e04e46 100644 --- a/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/loader/json-invalid-set-members.errors +++ b/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/loader/json-invalid-set-members.errors @@ -1,2 +1,4 @@ [ERROR] com.test#NoMemberSet: Missing required member of shape `com.test#NoMemberSet`: member | Model [ERROR] com.test#WrongMemberSet: Missing required member of shape `com.test#WrongMemberSet`: member | Model +[WARNING] com.test#WrongMemberSet: Expected an object with possible properties of `member`, `mixins`, `traits`, `type`, but found additional properties: `foo` | Model +[WARNING] com.test#ExtraMemberSet: Expected an object with possible properties of `member`, `mixins`, `traits`, `type`, but found additional properties: `foo` | Model diff --git a/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/severityOverrides/elevate-severity.errors b/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/severityOverrides/elevate-severity.errors index 3b489e045a9..c9e16fdfeab 100644 --- a/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/severityOverrides/elevate-severity.errors +++ b/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/severityOverrides/elevate-severity.errors @@ -1,2 +1,3 @@ [DANGER] smithy.example#IntegerTarget: Note the integer please | NoteTheInteger [DANGER] smithy.example#ListTarget: Note the list please | NoteTheList +[WARNING] smithy.example#MyString: Note the string please | NoteTheString diff --git a/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/severityOverrides/elevate-severity.smithy b/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/severityOverrides/elevate-severity.smithy index 50b9ef36f70..0be03d03631 100644 --- a/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/severityOverrides/elevate-severity.smithy +++ b/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/severityOverrides/elevate-severity.smithy @@ -19,6 +19,16 @@ metadata validators = [ selector: "[id|namespace = smithy.example] integer" } } + // This event is left alone. + { + name: "EmitEachSelector" + id: "NoteTheString" + message: "Note the string please" + severity: "WARNING" + configuration: { + selector: "[id|namespace = smithy.example] string" + } + } ] metadata severityOverrides = [ @@ -29,7 +39,7 @@ metadata severityOverrides = [ } { namespace: "smithy.example" - id: "NoteTheList" + id: "NoteTheInteger" severity: "DANGER" } // This one is ignored because it has a severity < DANGER. @@ -42,6 +52,8 @@ metadata severityOverrides = [ namespace smithy.example +string MyString + integer IntegerTarget list ListTarget { diff --git a/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/severityOverrides/suppressions-take-precedence.errors b/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/severityOverrides/suppressions-take-precedence.errors new file mode 100644 index 00000000000..171c6862279 --- /dev/null +++ b/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/severityOverrides/suppressions-take-precedence.errors @@ -0,0 +1 @@ +[SUPPRESSED] smithy.example#IntegerTarget: Note the integer please | NoteTheInteger diff --git a/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/severityOverrides/suppressions-take-precedence.smithy b/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/severityOverrides/suppressions-take-precedence.smithy new file mode 100644 index 00000000000..e1c994ac97c --- /dev/null +++ b/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/severityOverrides/suppressions-take-precedence.smithy @@ -0,0 +1,32 @@ +$version: "2.0" + +metadata validators = [ + { + name: "EmitEachSelector" + id: "NoteTheInteger" + message: "Note the integer please" + severity: "WARNING" + configuration: { + selector: "[id|namespace = smithy.example] integer" + } + } +] + +metadata suppressions = [ + { + namespace: "*" + id: "NoteTheInteger" + } +] + +metadata severityOverrides = [ + { + namespace: "smithy.example" + id: "NoteTheInteger" + severity: "DANGER" + } +] + +namespace smithy.example + +integer IntegerTarget