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
mtdowling committed Aug 1, 2023
1 parent 98bdf6f commit 2d2a318
Show file tree
Hide file tree
Showing 6 changed files with 107 additions and 108 deletions.
10 changes: 7 additions & 3 deletions smithy-model/src/main/java/software/amazon/smithy/model/loader/LoadOperationProcessor.java
View file
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
81 changes: 25 additions & 56 deletions smithy-model/src/main/java/software/amazon/smithy/model/loader/ModelAssembler.java
View file
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
91 changes: 42 additions & 49 deletions smithy-model/src/main/java/software/amazon/smithy/model/loader/ModelValidator.java
View file
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
25 changes: 25 additions & 0 deletions smithy-model/src/test/java/software/amazon/smithy/model/loader/ModelAssemblerTest.java
View file
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()));
}
}
6 changes: 6 additions & 0 deletions ...s/software/amazon/smithy/model/errorfiles/loader/json-invalid-list-and-map-members.errors
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Loading

0 comments on commit 2d2a318

Please sign in to comment.