Skip to content

Commit

Permalink
Further improve how validation is handled
Browse files Browse the repository at this point in the history
  • Loading branch information
mtdowling committed Aug 1, 2023
1 parent 98bdf6f commit a7e702c
Show file tree
Hide file tree
Showing 11 changed files with 155 additions and 110 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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<LoadOperation> {

Expand All @@ -47,22 +48,25 @@ final class LoadOperationProcessor implements Consumer<LoadOperation> {
TraitFactory traitFactory,
Model prelude,
boolean allowUnknownTraits,
Consumer<ValidationEvent> validationEventListener
Consumer<ValidationEvent> validationEventListener,
ValidationEventDecorator decorator
) {
// Emit events as the come in.
this.events = new ArrayList<ValidationEvent>() {
@Override
public boolean add(ValidationEvent e) {
e = decorator.decorate(e);
validationEventListener.accept(e);
return super.add(e);
}

@Override
public boolean addAll(Collection<? extends ValidationEvent> validationEvents) {
ensureCapacity(size() + validationEvents.size());
for (ValidationEvent e : validationEvents) {
validationEventListener.accept(e);
add(e);
}
return super.addAll(validationEvents);
return true;
}
};

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -487,10 +484,9 @@ public ModelAssembler disableValidation() {
* while loading and validating the model.
*
* <p>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.
Expand All @@ -510,13 +506,19 @@ public ValidatedResult<Model> 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<ValidationEvent> events = processor.events();

// Register manually added metadata.
Expand Down Expand Up @@ -560,38 +562,27 @@ public ValidatedResult<Model> 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<ValidationEvent> 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);
}
}
}

Expand All @@ -601,28 +592,6 @@ private void addMetadataToProcessor(Map<String, Node> metadataMap, LoadOperation
}
}

private ValidatedResult<Model> returnOnlyErrors(Model model, List<ValidationEvent> 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<Model> validate(Model model, List<ValidationEvent> events) {
// Validate the model based on the explicit validators and model metadata.
// Note the ModelValidator handles emitting events to the validationEventListener.
List<ValidationEvent> 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;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -84,16 +84,14 @@ private static final class LazyValidatorFactoryHolder {
private final List<ValidationEvent> events = new ArrayList<>();
private final List<Validator> validators = new ArrayList<>();
private final List<SeverityOverride> severityOverrides = new ArrayList<>();
private final List<Suppression> suppressions = new ArrayList<>();
private final ValidationEventDecorator validationEventDecorator;
private final Consumer<ValidationEvent> 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);
}

Expand All @@ -113,10 +111,10 @@ static ValidatorFactory defaultValidationFactory() {
static final class Builder implements SmithyBuilder<ModelValidator> {

private final List<Validator> validators = new ArrayList<>();
private final List<Suppression> suppressions = new ArrayList<>();
private final List<ValidationEvent> includeEvents = new ArrayList<>();
private ValidatorFactory validatorFactory = LazyValidatorFactoryHolder.INSTANCE;
private Consumer<ValidationEvent> eventListener = event -> { };
private ValidationEventDecorator validationEventDecorator;

private Builder() {}

Expand All @@ -133,55 +131,37 @@ public Builder validators(Collection<? extends Validator> 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<? extends Suppression> 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;
}

/**
* 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<ValidationEvent> eventListener) {
this.eventListener = Objects.requireNonNull(eventListener);
Expand All @@ -191,10 +171,11 @@ public Builder eventListener(Consumer<ValidationEvent> eventListener) {
/**
* Includes a set of events that were already encountered in the result.
*
* <p>Suppressions and severity overrides will be applied to the given {@code events}.
* <p>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<ValidationEvent> events) {
this.includeEvents.clear();
Expand All @@ -213,7 +194,7 @@ public ModelValidator build() {
private static final class LoadedModelValidator {

private final Model model;
private final List<Suppression> suppressions;
private final List<Suppression> suppressions = new ArrayList<>();
private final List<SeverityOverride> severityOverrides;
private final List<Validator> validators;
private final List<ValidationEvent> events = new ArrayList<>();
Expand All @@ -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<ObjectNode> 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<ObjectNode> 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<ObjectNode> 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<ObjectNode> 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));
}
});
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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<Model> 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<ValidationEvent> events = new ArrayList<>();

ValidatedResult<Model> result = new ModelAssembler()
.addDocumentNode(node)
.validationEventListener(events::add)
.assemble();

assertThat(result.getValidationEvents(Severity.ERROR), hasSize(1));
assertThat(events, equalTo(result.getValidationEvents()));
}
}
Loading

0 comments on commit a7e702c

Please sign in to comment.