Fix sourceLocation propagation in Traits #864
Conversation
There was a problem hiding this comment.
I saw sourceLocation propagated in createNode and toBuilder in other Traits, but probably missed in some other Traits too, like IdRefTrait.createNode. So want to confirm they are needed and if so will fix all.
| } | ||
|
|
||
| @Override | ||
| protected Node createNode() { | ||
| return throttling ? Node.objectNode().withMember(THROTTLING, true) : Node.objectNode(); | ||
| ObjectNode.Builder nodeBuilder = Node.objectNodeBuilder().sourceLocation(getSourceLocation()); |
There was a problem hiding this comment.
I'm unsure about the need for this propagation.
/**
* The result of toNode is used for hashCodes and equality. Subclasses
* must implement createNode to turn the trait into a Node. This is then
* cached for subsequent retrievals.
*
* @return Returns the trait as a node.
*/
protected abstract Node createNode();
And Node's equals doesn't look at sourceLocation. So does propagating sourceLocation matter?
There was a problem hiding this comment.
Yes we need it set here because we use SourceLocation in places like the NodeValidationVisitor (e.g., https://github.com/awslabs/smithy/blob/main/smithy-model/src/main/java/software/amazon/smithy/model/validation/node/PatternTraitPlugin.java#L43)
| @@ -69,7 +73,8 @@ public ShapeId getShapeId() { | |||
| @Override | |||
| public RetryableTrait createTrait(ShapeId target, Node value) { | |||
| ObjectNode node = value.expectObjectNode(); | |||
| return builder().throttling(node.getBooleanMemberOrDefault(THROTTLING)).build(); | |||
| Builder builder = builder().sourceLocation(value.getSourceLocation()); | |||
There was a problem hiding this comment.
This is used to create a modified Trait instance. Does continuing the sourceLocation make sense?
There was a problem hiding this comment.
Probably helps with some traceability. Most existing tratis propagate it, so updating any traits, like RetryableTrait, that aren't doing so.
gosar
changed the title
...eway-traits/src/main/java/software/amazon/smithy/aws/apigateway/traits/IntegrationTrait.java
Outdated
Show resolved
Hide resolved
| @@ -70,6 +70,7 @@ public Provider() { | |||
|
|
|||
| @Override | |||
| public Trait createTrait(ShapeId target, Node value) { | |||
| // BUG: sourceLocation | |||
There was a problem hiding this comment.
value.getSourceLocation()
| @@ -88,6 +91,7 @@ public Provider() { | |||
|
|
|||
| @Override | |||
| public ClientDiscoveredEndpointTrait createTrait(ShapeId target, Node value) { | |||
| // BUG: sourceLocation | |||
There was a problem hiding this comment.
You can use value.getSourceLocation() for all of these
| @@ -336,12 +337,14 @@ protected ObjectNode createNode() { | |||
| NodeMapper mapper = new NodeMapper(); | |||
| mapper.disableToNodeForClass(IntegrationTrait.class); | |||
| mapper.setOmitEmptyValues(true); | |||
| return mapper.serialize(this).expectObjectNode(); | |||
| ObjectNode.Builder builder = mapper.serialize(this).expectObjectNode().toBuilder(); | |||
| return builder.sourceLocation(getSourceLocation()).build(); | |||
There was a problem hiding this comment.
@JordonPhillips this still is necessary after #865 right?
There was a problem hiding this comment.
It is needed. Because of https://github.com/awslabs/smithy/blob/main/smithy-model/src/main/java/software/amazon/smithy/model/node/NodeMapper.java#L315. You refer to it here #865 (comment).
There was a problem hiding this comment.
It isn't. This implementation of ToNode is using the NodeMapper, which will set it for you. The implementation is identical to the same method in this test trait. I modeled that test class after this exact trait
There was a problem hiding this comment.
You are right. I did not notice the disableToNodeForClass part. I was also confused by ObjectNode.toBuilder() not propagating the sourceLocation, which affected my local testing.
Most existing toBuilders do this, but these ones were missing that logic.
Most existing createNode methods do this, but these ones were missing that logic.
Most existing createTrait methods do this, but these ones were missing that logic.
Those cases will be fixed with smithy-lang#865
They are fixed via smithy-lang#865
The sourceLocation should propogate from
Across traits, this wasn't done consistently.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.