From 41daa4841fff3e392715e3d262aca6b11bfeb24a Mon Sep 17 00:00:00 2001 From: Michael Dowling Date: Mon, 14 Sep 2020 15:31:16 -0700 Subject: [PATCH] De-depude model components by location and value While we previously de-duplicated models by filename/URL, we recently removed support for de-duping models by location and equality. This can happen, for example, when a Model is created through a ModelAssembler, and then added back to another ModelAssembler. If the files that came together to create the original model are added to the second ModelAssembler, then the previous file-base de-dupe would not catch the duplicate model and would fail in the assembler. This change updates the ModelAssembler to also de-dupe shapes, trait value, and metadata based on source location and equality. If one of these components conflict, and the conflcit is exactly the same, and defined at the same exact file/line/column, then the duplcate is ignored. Note that model components with no source location (i.e., weren't loaded from a file and were just created) are never deconflicted in this way because we don't reliably know that they are in fact the exact same unless we know where the components originate from. --- .../model/loader/MetadataContainer.java | 30 +++++++++++++------ .../smithy/model/loader/TraitContainer.java | 17 +++++++++-- .../model/loader/ModelAssemblerTest.java | 16 ++++++++++ .../smithy/model/loader/dedupe-models.smithy | 6 ++++ 4 files changed, 58 insertions(+), 11 deletions(-) create mode 100644 smithy-model/src/test/resources/software/amazon/smithy/model/loader/dedupe-models.smithy diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/loader/MetadataContainer.java b/smithy-model/src/main/java/software/amazon/smithy/model/loader/MetadataContainer.java index 970bb49827c..7b1049dac15 100644 --- a/smithy-model/src/main/java/software/amazon/smithy/model/loader/MetadataContainer.java +++ b/smithy-model/src/main/java/software/amazon/smithy/model/loader/MetadataContainer.java @@ -56,22 +56,34 @@ final class MetadataContainer { * @param value Value to set. */ void putMetadata(String key, Node value) { - if (!data.containsKey(key)) { - data.put(key, value); - } else if (data.get(key).isArrayNode() && value.isArrayNode()) { - ArrayNode previous = data.get(key).expectArrayNode(); - List merged = new ArrayList<>(previous.getElements()); + Node previous = data.putIfAbsent(key, value); + + if (previous == null) { + return; + } + + if (LoaderUtils.isSameLocation(value, previous) && value.equals(previous)) { + // The assumption here is that if the metadata value is exactly the + // same and from the same location, then the same model file was + // included more than once in a way that side-steps file and URL + // de-duplication. For example, this can occur when a Model is assembled + // through a ModelAssembler using model discovery, then the Model is + // added to a subsequent ModelAssembler, and then model discovery is + // performed again using the same classpath. + LOGGER.finer(() -> "Ignoring duplicate metadata key from same exact location: " + key); + } else if (previous.isArrayNode() && value.isArrayNode()) { + ArrayNode previousArray = previous.expectArrayNode(); + List merged = new ArrayList<>(previousArray.getElements()); merged.addAll(value.expectArrayNode().getElements()); - ArrayNode mergedArray = new ArrayNode(merged, value.getSourceLocation()); - data.put(key, mergedArray); - } else if (!data.get(key).equals(value)) { + data.put(key, new ArrayNode(merged, value.getSourceLocation())); + } else if (!previous.equals(value)) { events.add(ValidationEvent.builder() .id(Validator.MODEL_ERROR) .severity(Severity.ERROR) .sourceLocation(value) .message(format( "Metadata conflict for key `%s`. Defined in both `%s` and `%s`", - key, value.getSourceLocation(), data.get(key).getSourceLocation())) + key, value.getSourceLocation(), previous.getSourceLocation())) .build()); } else { LOGGER.fine(() -> "Ignoring duplicate metadata definition of " + key); diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/loader/TraitContainer.java b/smithy-model/src/main/java/software/amazon/smithy/model/loader/TraitContainer.java index 98f42e78ef2..61fc44a82a0 100644 --- a/smithy-model/src/main/java/software/amazon/smithy/model/loader/TraitContainer.java +++ b/smithy-model/src/main/java/software/amazon/smithy/model/loader/TraitContainer.java @@ -146,6 +146,20 @@ public void onTrait(ShapeId target, Trait value) { if (traits.containsKey(traitId)) { Trait previousTrait = traits.get(traitId); + + if (LoaderUtils.isSameLocation(previousTrait, value) && previousTrait.equals(value)) { + // The assumption here is that if the trait value is exactly the + // same and from the same location, then the same model file was + // included more than once in a way that side-steps file and URL + // de-duplication. For example, this can occur when a Model is assembled + // through a ModelAssembler using model discovery, then the Model is + // added to a subsequent ModelAssembler, and then model discovery is + // performed again using the same classpath. + LOGGER.finest(() -> String.format("Ignoring duplicate %s trait value on %s at same exact location", + traitId, target)); + return; + } + Node previous = previousTrait.toNode(); Node updated = value.toNode(); @@ -157,8 +171,7 @@ public void onTrait(ShapeId target, Trait value) { return; } } else if (previous.equals(updated)) { - LOGGER.fine(() -> String.format( - "Ignoring duplicate %s trait value on %s", traitId, target)); + LOGGER.fine(() -> String.format("Ignoring duplicate %s trait value on %s", traitId, target)); return; } else { events.add(ValidationEvent.builder() 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 f1560523d32..89bd291fc19 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 @@ -51,6 +51,7 @@ import software.amazon.smithy.model.node.Node; import software.amazon.smithy.model.node.ObjectNode; import software.amazon.smithy.model.node.StringNode; +import software.amazon.smithy.model.shapes.ModelSerializer; import software.amazon.smithy.model.shapes.Shape; import software.amazon.smithy.model.shapes.ShapeId; import software.amazon.smithy.model.shapes.ShapeType; @@ -616,4 +617,19 @@ public void createsDynamicTraitWhenTraitFactoryReturnsEmpty() { assertTrue(model.expectShape(id).findTrait(traitId).isPresent()); assertThat(model.expectShape(id).findTrait(traitId).get(), instanceOf(DynamicTrait.class)); } + + @Test + public void dedupesExactSameTraitsAndMetadataFromSameLocation() { + Model model = Model.assembler() + .addImport(getClass().getResource("dedupe-models.smithy")) + .assemble() + .unwrap(); + Model model2 = Model.assembler() + .addModel(model) + .addImport(getClass().getResource("dedupe-models.smithy")) + .assemble() + .unwrap(); + + assertThat(model, equalTo(model2)); + } } diff --git a/smithy-model/src/test/resources/software/amazon/smithy/model/loader/dedupe-models.smithy b/smithy-model/src/test/resources/software/amazon/smithy/model/loader/dedupe-models.smithy new file mode 100644 index 00000000000..014be9ca80a --- /dev/null +++ b/smithy-model/src/test/resources/software/amazon/smithy/model/loader/dedupe-models.smithy @@ -0,0 +1,6 @@ +metadata abc = [1] + +namespace smithy.example + +@tags(["a", "b"]) +string MyString