Skip to content

Server validation #546

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
adwsingh wants to merge 1 commit into from
Closed

Server validation #546

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion ...pes/src/main/java/software/amazon/smithy/java/aws/sdkv2/shapes/SdkJsonDocumentParser.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,12 @@ protected void after(Schema schema) {
}

@Override
public <T> void writeList(Schema schema, T state, int size, BiConsumer<T, ShapeSerializer> consumer) {
public <T extends List<?>> void writeList(
Schema schema,
T state,
int size,
BiConsumer<T, ShapeSerializer> consumer
) {
List<software.amazon.awssdk.core.document.Document> elements = size == -1
? new ArrayList<>()
: new ArrayList<>(size);
Expand Down
8 changes: 7 additions & 1 deletion ...e/src/main/java/software/amazon/smithy/java/client/core/endpoint/HostLabelSerializer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
import java.nio.ByteBuffer;
import java.time.Instant;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.function.BiConsumer;
import software.amazon.smithy.java.core.schema.Schema;
Expand Down Expand Up @@ -98,7 +99,12 @@ public void writeStruct(Schema schema, SerializableStruct struct) {
}

@Override
public <T> void writeList(Schema schema, T listState, int size, BiConsumer<T, ShapeSerializer> consumer) {
public <T extends List<?>> void writeList(
Schema schema,
T listState,
int size,
BiConsumer<T, ShapeSerializer> consumer
) {
// ignore
}

Expand Down
20 changes: 16 additions & 4 deletions ...n/java/software/amazon/smithy/java/codegen/generators/StructureDeserializerGenerator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,13 +30,15 @@ public void run() {
@Override
public Builder deserialize(${shapeDeserializer:N} decoder) {${?isError}
this.$$deserialized = true;${/isError}
decoder.readStruct($$SCHEMA, this, $$InnerDeserializer.INSTANCE);
decoder.readStruct($$SCHEMA, this, $$InnerDeserializer.INSTANCE);${?hasChecks}
this.disableChecks = true;${/hasChecks}
return this;
}

@Override
public Builder deserializeMember(${shapeDeserializer:N} decoder, ${sdkSchema:N} schema) {
decoder.readStruct(schema.assertMemberTargetIs($$SCHEMA), this, $$InnerDeserializer.INSTANCE);
decoder.readStruct(schema.assertMemberTargetIs($$SCHEMA), this, $$InnerDeserializer.INSTANCE);${?hasChecks}
this.disableChecks = true;${/hasChecks}
return this;
}

Expand Down Expand Up @@ -64,6 +66,8 @@ public void unknownMember(Builder builder, ${string:T} memberName) {
writer.putContext("union", shape.isUnionShape());
writer.putContext("illegalArg", IllegalArgumentException.class);
writer.putContext("isError", shape.hasTrait(ErrorTrait.class));
writer.putContext("hasChecks",
shape.isUnionShape() || shape.members().stream().anyMatch(CodegenUtils::isRequiredWithNoDefault));
writer.write(template);
writer.popState();
}
Expand All @@ -72,10 +76,18 @@ private void generateMemberSwitchCases(JavaWriter writer) {
int idx = 0;
for (var iter = CodegenUtils.getSortedMembers(shape).iterator(); iter.hasNext(); idx++) {
var member = iter.next();
boolean isTracked = CodegenUtils.isRequiredWithNoDefault(member);
boolean requiresNullCheck = CodegenUtils.requiresSetterNullCheck(symbolProvider, member);
writer.pushState();
writer.putContext("memberName", symbolProvider.toMemberName(member));
var memberName = symbolProvider.toMemberName(member);
var builderMethodName = memberName;
if (isTracked || requiresNullCheck) {
Copy link
Member

@mtdowling mtdowling Jan 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This all feels like it really needs some comments to explain what's going on

builderMethodName = memberName + "NoCheck";
}
writer.putContext("memberName", memberName);
writer.putContext("builderMethodName", builderMethodName);
writer.write(
"case $L -> builder.${memberName:L}($C);",
"case $L -> builder.${builderMethodName:L}($C);",
idx,
new DeserializerGenerator(writer, member, symbolProvider, model, service, "de", "member"));
writer.popState();
Expand Down
25 changes: 20 additions & 5 deletions ...core/src/main/java/software/amazon/smithy/java/codegen/generators/StructureGenerator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -228,10 +228,10 @@ public void run() {
memberSymbol.expectProperty(SymbolProperties.COLLECTION_IMMUTABLE_WRAPPER));
writer.putContext("collections", Collections.class);
writer.write(
"this.${memberName:L} = ${?nullable}builder.${memberName:L} == null ? null : ${/nullable}${collections:T}.${wrapper:L}(builder.${memberName:L});");
"this.${memberName:L} = builder.${memberName:L} == null ? null : ${collections:T}.${wrapper:L}(builder.${memberName:L});");
Copy link
Member

@mtdowling mtdowling Jan 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this changing?

Copy link
Contributor Author

@adwsingh adwsingh Jan 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The null checks are already present in the builder and we need to be able to construct a POJO with null lists during deserialization.

} else if (target.isBlobShape() && !CodegenUtils.isStreamingBlob(target)) {
writer.write(
"this.${memberName:L} = ${?nullable}builder.${memberName:L} == null ? null : ${/nullable}builder.${memberName:L}.duplicate();");
"this.${memberName:L} = builder.${memberName:L} == null ? null : builder.${memberName:L}.duplicate();");
} else {
writer.write("this.${memberName:L} = builder.${memberName:L};");
}
Expand Down Expand Up @@ -692,6 +692,7 @@ protected void generateProperties(JavaWriter writer) {

// Add presence tracker if any members are required by validation.
if (shape.members().stream().anyMatch(CodegenUtils::isRequiredWithNoDefault)) {
writer.write("private boolean disableChecks = false;");
writer.putContext("tracker", PresenceTracker.class);
writer.write("private final ${tracker:T} tracker = ${tracker:T}.of($$SCHEMA);");
}
Expand Down Expand Up @@ -728,11 +729,13 @@ protected void generateSetters(JavaWriter writer) {
writer.putContext("objects", Objects.class);
writer.putContext("throwable", Throwable.class);
for (var member : shape.members()) {
boolean isTracked = CodegenUtils.isRequiredWithNoDefault(member);
boolean requiresNullCheck = CodegenUtils.requiresSetterNullCheck(symbolProvider, member);
writer.pushState(new BuilderSetterSection(member));
writer.putContext("memberName", symbolProvider.toMemberName(member));
writer.putContext("memberSymbol", symbolProvider.toSymbol(member));
writer.putContext("tracked", CodegenUtils.isRequiredWithNoDefault(member));
writer.putContext("check", CodegenUtils.requiresSetterNullCheck(symbolProvider, member));
writer.putContext("tracked", isTracked);
writer.putContext("check", requiresNullCheck);
writer.putContext("schemaName", CodegenUtils.toMemberSchemaName(symbolProvider.toMemberName(member)));

writer.write(
Expand All @@ -743,6 +746,16 @@ protected void generateSetters(JavaWriter writer) {
return this;
}
""");

if (isTracked || requiresNullCheck) {
writer.write(
"""
private Builder ${memberName:L}NoCheck(${memberSymbol:T} ${memberName:L}) {
this.${memberName:L} = ${memberName:L};
return this;
}
""");
}
writer.popState();
}
if (shape.hasTrait(ErrorTrait.class)) {
Expand Down Expand Up @@ -775,7 +788,9 @@ protected void generateBuild(JavaWriter writer) {
writer.write("""
@Override
public ${shape:N} build() {${?hasRequiredMembers}
tracker.validate();${/hasRequiredMembers}
if (!disableChecks) {
Copy link
Member

@mtdowling mtdowling Jan 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is this and how does it work? Can you now create shapes that don't validate required members are present (so for example, an int gets initialized to 0 implicitly even when it was required but not set)?

Copy link
Contributor Author

@adwsingh adwsingh Jan 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is used to disable the checks in the builder if we are in deserialization mode.

tracker.validate();
}${/hasRequiredMembers}
return new ${shape:T}(this);
}
""");
Expand Down
3 changes: 2 additions & 1 deletion ...gen/core/src/main/java/software/amazon/smithy/java/codegen/generators/UnionGenerator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -252,6 +252,7 @@ private static final class UnionBuilderGenerator extends BuilderGenerator {
@Override
protected void generateProperties(JavaWriter writer) {
writer.write("private ${shape:T} value;");
writer.write("private boolean disableChecks;");
}

@Override
Expand Down Expand Up @@ -280,7 +281,7 @@ protected void generateSetters(JavaWriter writer) {
writer.putContext("illegalArgument", IllegalArgumentException.class);
writer.write("""
private BuildStage setValue(${shape:T} value) {
if (this.value != null) {
if (this.value != null && !this.disableChecks) {
if (this.value.type() == Type.$$UNKNOWN) {
throw new ${illegalArgument:T}("Cannot change union from unknown to known variant");
}
Expand Down
6 changes: 5 additions & 1 deletion core/src/main/java/software/amazon/smithy/java/core/schema/PresenceTracker.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,11 @@
* Tracks the presence of required fields
*/
@SmithyInternalApi
public abstract sealed class PresenceTracker {
public abstract sealed class PresenceTracker permits
PresenceTracker.BigRequiredMemberPresenceTracker,
PresenceTracker.NoOpPresenceTracker,
PresenceTracker.RequiredMemberPresenceTracker,
ShapeValidator.UnionPresenceTracker {

/**
* Sets a member as present.
Expand Down
18 changes: 18 additions & 0 deletions core/src/main/java/software/amazon/smithy/java/core/schema/Schema.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,8 @@ public abstract sealed class Schema implements MemberLookup
final double maxDoubleConstraint;
final ValidatorOfString stringValidation;
final boolean uniqueItemsConstraint;
final boolean hasRangeConstraint;
final String rangeValidationFailureMessage;

private Schema listMember;
private Schema mapKeyMember;
Expand Down Expand Up @@ -107,6 +109,7 @@ public abstract sealed class Schema implements MemberLookup

// Even root-level shapes have computed validation information to allow for validating root strings directly.
var validationState = SchemaBuilder.ValidationState.of(type, traits, stringEnumValues);
this.hasRangeConstraint = validationState.hasRangeConstraint();
this.minLengthConstraint = validationState.minLengthConstraint();
this.maxLengthConstraint = validationState.maxLengthConstraint();
this.minLongConstraint = validationState.minLongConstraint();
Expand All @@ -118,6 +121,13 @@ public abstract sealed class Schema implements MemberLookup
this.stringValidation = validationState.stringValidation();
this.uniqueItemsConstraint = type == ShapeType.LIST && hasTrait(TraitKey.UNIQUE_ITEMS_TRAIT);

if (this.minRangeConstraint != null || this.maxRangeConstraint != null) {
this.rangeValidationFailureMessage = ValidationError.RangeValidationFailure
.createMessage(this.minRangeConstraint, this.maxRangeConstraint);
} else {
this.rangeValidationFailureMessage = null;
}

// Only use the slow version of required member validation if there are > 64 required members.
this.requiredMemberCount = SchemaBuilder.computeRequiredMemberCount(type, members);
this.requiredStructureMemberBitfield = SchemaBuilder.computeRequiredBitField(
Expand All @@ -139,6 +149,7 @@ public abstract sealed class Schema implements MemberLookup
this.requiredMemberCount = builder.requiredMemberCount;
this.isRequiredByValidation = builder.isRequiredByValidation;

this.hasRangeConstraint = builder.validationState.hasRangeConstraint();
this.minLengthConstraint = builder.validationState.minLengthConstraint();
this.maxLengthConstraint = builder.validationState.maxLengthConstraint();
this.minRangeConstraint = builder.validationState.minRangeConstraint();
Expand All @@ -150,6 +161,13 @@ public abstract sealed class Schema implements MemberLookup
this.maxDoubleConstraint = builder.validationState.maxDoubleConstraint();
this.uniqueItemsConstraint = type == ShapeType.LIST && hasTrait(TraitKey.UNIQUE_ITEMS_TRAIT);

if (this.minRangeConstraint != null || this.maxRangeConstraint != null) {
this.rangeValidationFailureMessage = ValidationError.RangeValidationFailure
.createMessage(this.minRangeConstraint, this.maxRangeConstraint);
} else {
this.rangeValidationFailureMessage = null;
}

// Compute the expected bitfield, and adjust how it's computed based on if the target is a builder or not.
if (builder.target != null) {
this.requiredStructureMemberBitfield = SchemaBuilder.computeRequiredBitField(
Expand Down
5 changes: 5 additions & 0 deletions core/src/main/java/software/amazon/smithy/java/core/schema/SchemaBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -205,6 +205,7 @@ static <T> long computeRequiredBitField(
}

record ValidationState(
boolean hasRangeConstraint,
long minLengthConstraint,
long maxLengthConstraint,
BigDecimal minRangeConstraint,
Expand All @@ -224,6 +225,7 @@ static ValidationState of(ShapeType type, TraitMap traits, Set<String> stringEnu
double minDoubleConstraint;
double maxDoubleConstraint;
ValidatorOfString stringValidation;
boolean hasRangeConstraint;

// Precompute an allowed range, setting Long.MIN and Long.MAX when missing.
LengthTrait lengthTrait = traits.get(TraitKey.LENGTH_TRAIT);
Expand Down Expand Up @@ -255,6 +257,8 @@ static ValidationState of(ShapeType type, TraitMap traits, Set<String> stringEnu
maxRangeConstraint = null;
}

hasRangeConstraint = maxRangeConstraint != null || minRangeConstraint != null;

// Pre-compute allowable ranges so this doesn't have to be looked up during validation.
// BigInteger and BigDecimal just use the rangeConstraint BigDecimal directly.
switch (type) {
Expand Down Expand Up @@ -311,6 +315,7 @@ static ValidationState of(ShapeType type, TraitMap traits, Set<String> stringEnu
}

return new ValidationState(
hasRangeConstraint,
minLengthConstraint,
maxLengthConstraint,
minRangeConstraint,
Expand Down
Loading
Loading