Repository: smithy-lang/smithy
Pull request #2393 (2024-09-04 22:32:39)

Add validation for null members in structures, lists and maps
#### Background
Previously, a NullNode could be provided as a required structure member without emitting a ValidationEvent. So an example test below would fail. 

```    @Test
    public void nullRequiredStructureMember() {
        ObjectNode structure = ObjectNode.builder()
                .withMember("foo", Node.nullNode())
                .build();
        NodeValidationVisitor visitor = NodeValidationVisitor.builder()
                .value(structure)
                .model(MODEL)
                .allowOptionalNull(true)
                .build();
        List<ValidationEvent> events = MODEL
                .expectShape(ShapeId.from("ns.foo#Structure"))
                .accept(visitor);

        assertThat(events, hasSize(1));
        assertThat(events.get(0).getMessage(), containsString(
                "foo: Required member `foo` for structure ns.foo#Structure for cannot be null"));
    }

ns.foo#Structure schema:
"ns.foo#Structure": {
    "type": "structure",
    "members": {
        "foo": {
            "target": "ns.foo#String",
            "traits": {
                "smithy.api#required": {}
            }
        },
...
```

This happens because the memberShape function would check the member's [target](https://github.com/smithy-lang/smithy/blob/main/smithy-model/src/main/java/software/amazon/smithy/model/validation/NodeValidationVisitor.java#L385) instead of itself, which always satisfies the !shape.isMemberShape() [condition](https://github.com/smithy-lang/smithy/blob/main/smithy-model/src/main/java/software/amazon/smithy/model/validation/NodeValidationVisitor.java#L410) in the invalidShape function.

To fix this I added a new function to check null member nodes for structures, lists and maps. It relies on the `isMemberNullable` function, which checks that:
- If a structure member, member cannot be required
- If a list member, the list must have the sparse trait
- If a map value, the map must have the sparse trait (this case is not explicit but happens due to a fall through to the [default case](https://github.com/smithy-lang/smithy/blob/1e4458ee825b24490c03fbf22f4b2f79111c7ae5/smithy-model/src/main/java/software/amazon/smithy/model/knowledge/NullableIndex.java#L191))


#### Testing
Added testcases:
- One that emits ValidationEvent when NullNode is provided as a required structure member
- One that emits ValidationEvent when NullNode is provided as member for a non-sparse list
- One that doesn't emit ValidationEvent when NullNode is provided as member for a sparse list
- One that emits ValidationEvent when NullNode is provided as a value for a non-sparse map
- One that doesn't emit ValidationEvent when NullNode is provided as a value for a sparse map

---
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 8c7a00bb0..e9ae31c84 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
@@ -378,6 +378,9 @@ public final class NodeValidationVisitor implements ShapeVisitor<List<Validation
     @Override
     public List<ValidationEvent> memberShape(MemberShape shape) {
         List<ValidationEvent> events = applyPlugins(shape);
+        if (value.isNullNode()) {
+            events.addAll(checkNullMember(shape));
+        }
         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.
@@ -388,6 +391,28 @@ public final class NodeValidationVisitor implements ShapeVisitor<List<Validation
         return events;
     }
 
+    public List<ValidationEvent> checkNullMember(MemberShape shape) {
+        if (!nullableIndex.isMemberNullable(shape)) {
+            switch (model.expectShape(shape.getContainer()).getType()) {
+                case LIST:
+                    return ListUtils.of(event(
+                            String.format(
+                                    "Non-sparse list shape `%s` cannot contain null values", shape.getContainer())));
+                case MAP:
+                    return ListUtils.of(event(
+                            String.format(
+                                    "Non-sparse map shape `%s` cannot contain null values", shape.getContainer())));
+                case STRUCTURE:
+                    return ListUtils.of(event(
+                            String.format("Required structure member `%s` for `%s` cannot be null",
+                                    shape.getMemberName(), shape.getContainer())));
+                default:
+                    break;
+            }
+        }
+        return ListUtils.of();
+    }
+
     @Override
     public List<ValidationEvent> operationShape(OperationShape shape) {
         return invalidSchema(shape);


Test patch:
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 1712d5103..b0e715947 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
@@ -16,9 +16,14 @@
 package software.amazon.smithy.model.validation;
 
 import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.contains;
 import static org.hamcrest.Matchers.containsInAnyOrder;
+import static org.hamcrest.Matchers.containsString;
 import static org.hamcrest.Matchers.empty;
+import static org.hamcrest.Matchers.equalTo;
+import static org.hamcrest.Matchers.hasSize;
 import static org.hamcrest.Matchers.not;
+import static org.junit.jupiter.api.Assertions.assertTrue;
 
 import java.util.Arrays;
 import java.util.Collection;
@@ -31,7 +36,9 @@ import org.junit.jupiter.api.Test;
 import org.junit.jupiter.params.ParameterizedTest;
 import org.junit.jupiter.params.provider.MethodSource;
 import software.amazon.smithy.model.Model;
+import software.amazon.smithy.model.node.ArrayNode;
 import software.amazon.smithy.model.node.Node;
+import software.amazon.smithy.model.node.ObjectNode;
 import software.amazon.smithy.model.shapes.ShapeId;
 import software.amazon.smithy.model.validation.node.TimestampValidationStrategy;
 
@@ -344,4 +351,96 @@ public class NodeValidationVisitorTest {
 
         assertThat(events, empty());
     }
+
+    @Test
+    public void nullRequiredStructureMember() {
+        ObjectNode structure = ObjectNode.builder()
+                .withMember("foo", Node.nullNode())
+                .build();
+        NodeValidationVisitor visitor = NodeValidationVisitor.builder()
+                .value(structure)
+                .model(MODEL)
+                .allowOptionalNull(true)
+                .build();
+        List<ValidationEvent> events = MODEL
+                .expectShape(ShapeId.from("ns.foo#Structure"))
+                .accept(visitor);
+
+        assertThat(events, hasSize(1));
+        assertThat(events.get(0).getMessage(), containsString(
+                "foo: Required structure member `foo` for `ns.foo#Structure` cannot be null"));
+    }
+
+
+    @Test
+    public void nullNonSparseListMember() {
+        ArrayNode list = ArrayNode.builder()
+                .withValue(Node.nullNode())
+                .build();
+        NodeValidationVisitor visitor = NodeValidationVisitor.builder()
+                .value(list)
+                .model(MODEL)
+                .allowOptionalNull(true)
+                .build();
+        List<ValidationEvent> events = MODEL
+                .expectShape(ShapeId.from("ns.foo#List"))
+                .accept(visitor);
+
+        assertThat(events, hasSize(1));
+        assertThat(events.get(0).getMessage(), containsString(
+                "0: Non-sparse list shape `ns.foo#List` cannot contain null values"));
+    }
+
+    @Test
+    public void nullSparseListMember() {
+        ArrayNode list = ArrayNode.builder()
+                .withValue(Node.nullNode())
+                .build();
+        NodeValidationVisitor visitor = NodeValidationVisitor.builder()
+                .value(list)
+                .model(MODEL)
+                .allowOptionalNull(true)
+                .build();
+        List<ValidationEvent> events = MODEL
+                .expectShape(ShapeId.from("ns.foo#SparseList"))
+                .accept(visitor);
+
+        assertThat(events, empty());
+    }
+
+    @Test
+    public void nullNonSparseMapValue() {
+        ObjectNode map = ObjectNode.builder()
+                .withMember("a", Node.nullNode())
+                .build();
+        NodeValidationVisitor visitor = NodeValidationVisitor.builder()
+                .value(map)
+                .model(MODEL)
+                .allowOptionalNull(true)
+                .build();
+        List<ValidationEvent> events = MODEL
+                .expectShape(ShapeId.from("ns.foo#Map"))
+                .accept(visitor);
+
+        assertThat(events, hasSize(1));
+        assertThat(events.get(0).getMessage(), containsString(
+                "a: Non-sparse map shape `ns.foo#Map` cannot contain null values"));
+    }
+
+    @Test
+    public void nullSparseMapValue() {
+        ObjectNode map = ObjectNode.builder()
+                .withMember("a", Node.nullNode())
+                .build();
+        NodeValidationVisitor visitor = NodeValidationVisitor.builder()
+                .value(map)
+                .model(MODEL)
+                .allowOptionalNull(true)
+                .build();
+        List<ValidationEvent> events = MODEL
+                .expectShape(ShapeId.from("ns.foo#SparseMap"))
+                .accept(visitor);
+
+        assertThat(events, empty());
+    }
 }
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 f4100e4c5..753bc144e 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
@@ -159,6 +159,15 @@
                 "smithy.api#uniqueItems": {}
             }
         },
+        "ns.foo#SparseList": {
+            "type": "list",
+            "member": {
+                "target": "ns.foo#String"
+            },
+            "traits": {
+                "smithy.api#sparse": {}
+            }
+        },
         "ns.foo#String": {
             "type": "string"
         },
@@ -186,6 +195,18 @@
                 }
             }
         },
+        "ns.foo#SparseMap": {
+            "type": "map",
+            "key": {
+                "target": "ns.foo#KeyString"
+            },
+            "value": {
+                "target": "ns.foo#List"
+            },
+            "traits": {
+                "smithy.api#sparse": {}
+            }
+        },
         "ns.foo#Structure": {
             "type": "structure",
             "members": {
diff --git a/smithy-rules-engine/src/test/resources/software/amazon/smithy/rulesengine/traits/errorfiles/invalid-static-null-param-value.errors b/smithy-rules-engine/src/test/resources/software/amazon/smithy/rulesengine/traits/errorfiles/invalid-static-null-param-value.errors
index 02a83acbc..a82da2b80 100644
--- a/smithy-rules-engine/src/test/resources/software/amazon/smithy/rulesengine/traits/errorfiles/invalid-static-null-param-value.errors
+++ b/smithy-rules-engine/src/test/resources/software/amazon/smithy/rulesengine/traits/errorfiles/invalid-static-null-param-value.errors
@@ -1,2 +1,3 @@
 [WARNING] smithy.example#OperationNull: This shape applies a trait that is unstable: smithy.rules#staticContextParams | UnstableTrait
 [ERROR] smithy.example#OperationNull: The operation `smithy.example#OperationNull` is marked with `smithy.rules#staticContextParams` which contains a key `nullParam` with an unsupported document type value `null`. | StaticContextParamsTrait
+[ERROR] smithy.example#OperationNull: Error validating trait `smithy.rules#staticContextParams`.nullParam.value: Required structure member `value` for `smithy.rules#StaticContextParamDefinition` cannot be null | TraitValue