Repository: smithy-lang/smithy
Pull request #1948 (2023-08-24 20:16:34)

Fix member timestamp format node validation
The NodeValidationVisitor vists shapes, members, and then the targets of members. When visiting the target of a member, the context of the referring member was lost, which meant a member with a timestampFormat trait like http-date isn't something the target shape has access to, which will cause validation to fail. We now pass in the referring member as part of the validation context.

Closes #1946

*Issue #, if available:*

*Description of changes:*


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.



Patch:
diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/validation/NodeValidationVisitor.java b/smithy-model/src/main/java/software/amazon/smithy/model/validation/NodeValidationVisitor.java
index 8a196f0d5..733196c1d 100644
--- a/smithy-model/src/main/java/software/amazon/smithy/model/validation/NodeValidationVisitor.java
+++ b/smithy-model/src/main/java/software/amazon/smithy/model/validation/NodeValidationVisitor.java
@@ -355,9 +355,13 @@ public final class NodeValidationVisitor implements ShapeVisitor<List<Validation
     @Override
     public List<ValidationEvent> memberShape(MemberShape shape) {
         List<ValidationEvent> events = applyPlugins(shape);
-        events.addAll(model.getShape(shape.getTarget())
-                              .map(member -> member.accept(this))
-                              .orElse(ListUtils.of()));
+        model.getShape(shape.getTarget()).ifPresent(target -> {
+            // We only need to keep track of a single referring member, so a stack of members or anything like that
+            // isn't needed here.
+            validationContext.setReferringMember(shape);
+            events.addAll(target.accept(this));
+            validationContext.setReferringMember(null);
+        });
         return events;
     }
 
diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/validation/node/NodeValidatorPlugin.java b/smithy-model/src/main/java/software/amazon/smithy/model/validation/node/NodeValidatorPlugin.java
index 20269c0b3..3556d13ef 100644
--- a/smithy-model/src/main/java/software/amazon/smithy/model/validation/node/NodeValidatorPlugin.java
+++ b/smithy-model/src/main/java/software/amazon/smithy/model/validation/node/NodeValidatorPlugin.java
@@ -19,11 +19,13 @@ import java.util.Collections;
 import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
+import java.util.Optional;
 import java.util.Set;
 import software.amazon.smithy.model.FromSourceLocation;
 import software.amazon.smithy.model.Model;
 import software.amazon.smithy.model.node.Node;
 import software.amazon.smithy.model.selector.Selector;
+import software.amazon.smithy.model.shapes.MemberShape;
 import software.amazon.smithy.model.shapes.Shape;
 import software.amazon.smithy.model.validation.NodeValidationVisitor;
 import software.amazon.smithy.model.validation.Severity;
@@ -72,6 +74,7 @@ public interface NodeValidatorPlugin {
     final class Context {
         private final Model model;
         private final Set<NodeValidationVisitor.Feature> features;
+        private MemberShape referringMember;
 
         // Use an LRU cache to ensure the Selector cache doesn't grow too large
         // when given bad inputs.
@@ -121,6 +124,14 @@ public interface NodeValidatorPlugin {
         public boolean hasFeature(NodeValidationVisitor.Feature feature) {
             return features.contains(feature);
         }
+
+        public void setReferringMember(MemberShape referringMember) {
+            this.referringMember = referringMember;
+        }
+
+        public Optional<MemberShape> getReferringMember() {
+            return Optional.ofNullable(referringMember);
+        }
     }
 
     @SmithyInternalApi
diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/validation/node/TimestampFormatPlugin.java b/smithy-model/src/main/java/software/amazon/smithy/model/validation/node/TimestampFormatPlugin.java
index 6833bc757..8518356cd 100644
--- a/smithy-model/src/main/java/software/amazon/smithy/model/validation/node/TimestampFormatPlugin.java
+++ b/smithy-model/src/main/java/software/amazon/smithy/model/validation/node/TimestampFormatPlugin.java
@@ -39,7 +39,13 @@ final class TimestampFormatPlugin implements NodeValidatorPlugin {
     @Override
     public void apply(Shape shape, Node value, Context context, Emitter emitter) {
         if (shape instanceof TimestampShape) {
-            validate(shape, shape.getTrait(TimestampFormatTrait.class).orElse(null), value, emitter);
+            // Don't validate the timestamp target if a referring member had the timestampFormat trait.
+            boolean fromMemberWithTrait = context.getReferringMember()
+                    .filter(member -> member.hasTrait(TimestampFormatTrait.class))
+                    .isPresent();
+            if (!fromMemberWithTrait) {
+                validate(shape, shape.getTrait(TimestampFormatTrait.class).orElse(null), value, emitter);
+            }
         } else if (shape instanceof MemberShape && shape.getTrait(TimestampFormatTrait.class).isPresent()) {
             // Only perform timestamp format validation on a member when it references
             // a timestamp shape and the member has an explicit timestampFormat trait.


Test patch:
diff --git a/smithy-model/src/test/java/software/amazon/smithy/model/loader/ValidationEventDecoratorTest.java b/smithy-model/src/test/java/software/amazon/smithy/model/loader/ValidationEventDecoratorTest.java
index ce7440451..c7e1279a6 100644
--- a/smithy-model/src/test/java/software/amazon/smithy/model/loader/ValidationEventDecoratorTest.java
+++ b/smithy-model/src/test/java/software/amazon/smithy/model/loader/ValidationEventDecoratorTest.java
@@ -44,7 +44,8 @@ public class ValidationEventDecoratorTest {
     static final String UNREFERENCED_SHAPE_EVENT_ID = "UnreferencedShape";
     static final Set<ShapeId> STRUCT_SHAPE_IDS = SetUtils.of(ShapeId.from("ns.foo#Structure"),
                                                              ShapeId.from("ns.foo#Structure2"),
-                                                             ShapeId.from("ns.foo#Structure3"));
+                                                             ShapeId.from("ns.foo#Structure3"),
+                                                             ShapeId.from("ns.foo#Structure4"));
 
     @Test
     public void canDecorateValidationEvents() {
diff --git a/smithy-model/src/test/java/software/amazon/smithy/model/validation/NodeValidationVisitorTest.java b/smithy-model/src/test/java/software/amazon/smithy/model/validation/NodeValidationVisitorTest.java
index 0d5b2a07f..185173a3f 100644
--- a/smithy-model/src/test/java/software/amazon/smithy/model/validation/NodeValidationVisitorTest.java
+++ b/smithy-model/src/test/java/software/amazon/smithy/model/validation/NodeValidationVisitorTest.java
@@ -257,10 +257,11 @@ public class NodeValidationVisitorTest {
                 // timestamp member with format.
                 {"ns.foo#TimestampList", "[\"1985-04-12T23:20:50.52Z\"]", null},
                 {"ns.foo#TimestampList", "[\"1985-04-12T23:20:50.52-07:00\"]", new String[] {
-                        "0: Invalid string value, `1985-04-12T23:20:50.52-07:00`, provided for timestamp, `smithy.api#Timestamp`. Expected an RFC 3339 formatted timestamp (e.g., \"1985-04-12T23:20:50.52Z\")",
                         "0: Invalid string value, `1985-04-12T23:20:50.52-07:00`, provided for timestamp, `ns.foo#TimestampList$member`. Expected an RFC 3339 formatted timestamp (e.g., \"1985-04-12T23:20:50.52Z\")"
                 }},
                 {"ns.foo#TimestampList", "[123]", new String[] {"0: Expected a string value for a date-time timestamp (e.g., \"1985-04-12T23:20:50.52Z\")"}},
+                {"ns.foo#Structure4", "{\"httpDate\": 1234}", new String[] {"httpDate: Invalid value provided for http-date formatted timestamp. Expected a string value that matches the IMF-fixdate production of RFC 7231 section-7.1.1.1. Found: number"}},
+                {"ns.foo#Structure4", "{\"httpDateTarget\": 1234}", new String[] {"httpDateTarget: Invalid value provided for http-date formatted timestamp. Expected a string value that matches the IMF-fixdate production of RFC 7231 section-7.1.1.1. Found: number"}},
 
                 // timestamp member with no format.
                 {"ns.foo#TimestampListNoFormatTrait", "[123]", null},
diff --git a/smithy-model/src/test/resources/software/amazon/smithy/model/validation/node-validator.json b/smithy-model/src/test/resources/software/amazon/smithy/model/validation/node-validator.json
index d7ea24ca1..da895424e 100644
--- a/smithy-model/src/test/resources/software/amazon/smithy/model/validation/node-validator.json
+++ b/smithy-model/src/test/resources/software/amazon/smithy/model/validation/node-validator.json
@@ -268,6 +268,20 @@
                 }
             }
         },
+        "ns.foo#Structure4": {
+            "type": "structure",
+            "members": {
+                "httpDate": {
+                    "target": "smithy.api#Timestamp",
+                    "traits": {
+                        "smithy.api#timestampFormat": "http-date"
+                    }
+                },
+                "httpDateTarget": {
+                    "target": "ns.foo#HttpDate"
+                }
+            }
+        },
         "ns.foo#Service": {
             "type": "service",
             "version": "2017-17-01",