Skip to content

Commit

Permalink
De-depude model components by location and value
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
mtdowling committed Sep 15, 2020
1 parent 42444bc commit 41daa48
Show file tree
Hide file tree
Showing 4 changed files with 58 additions and 11 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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<Node> 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<Node> 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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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();

Expand All @@ -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()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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));
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
metadata abc = [1]

namespace smithy.example

@tags(["a", "b"])
string MyString

0 comments on commit 41daa48

Please sign in to comment.