Skip to content

Commit

Permalink
Add NodeValidationVisitor checks for invalid nulls (#2393)
Browse files Browse the repository at this point in the history 
* Add validation for null members in structures, lists and maps

* Apply suggestions from code review

Simplify conditional check, return immediately for each case, change msg formatting

Co-authored-by: Kevin Stich <kevin@kstich.com>

* Update NodeValidationVisitor.java

Remove unnecessary breaks

* Fix test exception msg assertions

---------

Co-authored-by: Maxim Korolkov <mkorolko@amazon.com>
Co-authored-by: Kevin Stich <kevin@kstich.com>
  • Loading branch information
@maximk000 @kstich
3 people authored Sep 9, 2024
1 parent 1e4458e commit 8418149
Show file tree
Hide file tree
Showing 4 changed files with 146 additions and 0 deletions.
25 changes: 25 additions & 0 deletions ...hy-model/src/main/java/software/amazon/smithy/model/validation/NodeValidationVisitor.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -378,6 +378,9 @@ public List<ValidationEvent> unionShape(UnionShape shape) {
@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.
Expand All @@ -388,6 +391,28 @@ public List<ValidationEvent> memberShape(MemberShape shape) {
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);
Expand Down
99 changes: 99 additions & 0 deletions ...odel/src/test/java/software/amazon/smithy/model/validation/NodeValidationVisitorTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -31,7 +36,9 @@
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;

Expand Down Expand Up @@ -344,4 +351,96 @@ public void canConfigureToSupportNull() {

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());
}
}
21 changes: 21 additions & 0 deletions smithy-model/src/test/resources/software/amazon/smithy/model/validation/node-validator.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
},
Expand Down Expand Up @@ -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": {
Expand Down
1 change: 1 addition & 0 deletions ...ftware/amazon/smithy/rulesengine/traits/errorfiles/invalid-static-null-param-value.errors
View file
Original file line number Diff line number Diff line change
@@ -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

0 comments on commit 8418149

Please sign in to comment.