Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
23 changes: 23 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,20 @@ policy in [docs/versioning.md](docs/versioning.md).

### Fixed

- **Numeric wire values no longer truncate into generated narrow types**
(#109). Three holes in the otherwise-uniform range-check posture: an
`intEnum` member in a document body cast the raw wire int64 straight into
its `int32`-backed `enum class`, so 2^32+2 silently aliased onto a valid
enumerator (byte / short / integer members already rejected out-of-range
values, and intEnum *text* bindings were already bounded — the body path
now shares the same check); a `float` member cast the parsed double
unchecked on both the body and text-binding paths, so a finite wire value
beyond float range (`1e300`) was undefined behavior per [conv.double] —
the new `smithy::FloatFromDouble` rejects it while NaN/±Infinity still
pass; and
the jsonRpc2 client truncated `error.code` to `int` *before* its 100–599
range test, classifying 2^32+404 as HTTP 404 (with 5xx codes wrongly
marked retryable) — codes are now classified on the full int64.
- **Modeled redirects: a 3xx `@httpResponseCode` is a success, not an error**
(#184). Generated clients tested `status < 200 || status > 299` when the
output bound `@httpResponseCode`, so every modeled redirect came back as a
Expand All @@ -31,6 +45,15 @@ policy in [docs/versioning.md](docs/versioning.md).

### Added

- **Servers validate intEnum membership** (#109). String enums already
failed request validation outside the modeled value set; intEnum members
were accepted silently. They now produce the same suite-exact
`ValidationException` message
(`Member must satisfy enum value set: [1, 2]`), with `@internal` members
staying wire-valid but unadvertised — string-enum policy throughout.
Clients deliberately keep
unknown-but-in-range values for forward compatibility, and the asymmetry is
pinned by `examples/roundtrip/rest/numeric_bounds_wire_test.cc`.
- **Redirects are documented and covered** — a "Redirects (3xx)" section in
[docs/server-guide.md](docs/server-guide.md) covering both spellings and the
`@suppress(["HttpResponseCodeSemantics"])` a static non-2xx `@http` code
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -582,9 +582,13 @@ private static boolean writeSimpleTextParse(
"$Lstatic_cast<$L>(*parsed_num)$L", open, context.cppSymbols().typeRef(target), close);
}
case FLOAT -> {
// Checked narrowing: a finite text value beyond float range would be
// UB to cast (UBSan float-cast-overflow), so it fails the parse.
w.write("auto parsed_num = helpers::ParseDoubleText($L);", valueExpr);
w.write("if (!parsed_num) return std::move(parsed_num).error();");
w.write("$Lstatic_cast<float>(*parsed_num)$L", open, close);
w.write("auto narrowed_num = smithy::FloatFromDouble(*parsed_num);");
w.write("if (!narrowed_num) return std::move(narrowed_num).error();");
w.write("$L*narrowed_num$L", open, close);
}
case DOUBLE -> {
w.write("auto parsed_num = helpers::ParseDoubleText($L);", valueExpr);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,9 @@ public List<String> clientIncludes() {

@Override
public void writeClientHelpers(CppWriter w, CppContext context) {
// ParseError declares a std::int64_t local; don't borrow <cstdint>
// transitively (SF.10).
w.addInclude("<cstdint>");
ProtocolSupport.writeSanitizeErrorCode(w);
ProtocolSupport.writeParsedErrorStruct(w);
// jsonRpc2 error identity lives in the envelope's error object rather than
Expand All @@ -110,9 +113,11 @@ public void writeClientHelpers(CppWriter w, CppContext context) {
w.openBlock(
"if (const smithy::Document* code = error->Find(\"code\"); "
+ "code != nullptr && code->is_int()) {");
w.write("const auto rpc_code = static_cast<int>(code->as_int());");
// Classified on the full int64: narrowing first would alias 2^32+404
// onto 404 and misroute status/retryability (issue #109).
w.write("const std::int64_t rpc_code = code->as_int();");
w.write(
"parsed.status = rpc_code >= 100 && rpc_code < 600 ? rpc_code "
"parsed.status = rpc_code >= 100 && rpc_code < 600 ? static_cast<int>(rpc_code) "
+ ": (rpc_code == -32603 ? 500 : 400);");
w.closeBlock("}");
w.write(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -190,10 +190,10 @@ String minimalExpression(Shape shape) {
private String minimalExpression(Shape shape, MemberShape member) {
return switch (shape.getType()) {
case BOOLEAN -> "false";
case BYTE, SHORT, INTEGER, LONG, FLOAT, DOUBLE, INT_ENUM ->
minimalNumberExpression(shape, member);
case BYTE, SHORT, INTEGER, LONG, FLOAT, DOUBLE -> minimalNumberExpression(shape, member);
case STRING -> minimalStringExpression(shape, member);
case ENUM -> minimalEnumExpression(shape);
case INT_ENUM -> minimalIntEnumExpression(shape);
case BLOB -> "smithy::Blob()";
case TIMESTAMP -> "smithy::Timestamp::FromEpochMilliseconds(0)";
case DOCUMENT -> "smithy::Document(smithy::DocumentMap{})";
Expand Down Expand Up @@ -226,7 +226,6 @@ private String minimalNumberExpression(Shape shape, MemberShape member) {
.orElse(java.math.BigDecimal.ZERO);
String plain = value.stripTrailingZeros().toPlainString();
return switch (shape.getType()) {
case INT_ENUM -> "static_cast<" + typeName(shape) + ">(" + plain + ")";
case LONG -> plain + "LL";
case FLOAT -> (plain.contains(".") ? plain : plain + ".0") + "F";
case DOUBLE -> plain.contains(".") ? plain : plain + ".0";
Expand Down Expand Up @@ -272,6 +271,14 @@ private String minimalEnumExpression(Shape shape) {
return typeName(shape) + "::FromString(" + CppLiterals.stringLiteral(first) + ")";
}

private String minimalIntEnumExpression(Shape shape) {
// The first modeled member, not 0: a default-constructed intEnum is only
// valid when 0 happens to be in the value set, and servers validate
// membership (issue #109).
Integer first = shape.asIntEnumShape().orElseThrow().getEnumValues().values().iterator().next();
return "static_cast<" + typeName(shape) + ">(" + first + ")";
}

private String minimalStructureExpression(StructureShape shape) {
if (shape.getId().toString().equals("smithy.api#Unit")) {
return "smithy::Unit{}";
Expand Down Expand Up @@ -320,7 +327,9 @@ private boolean needsExplicitMinimal(
}
boolean constrainedDefault =
switch (shape.getType()) {
case ENUM -> true;
// intEnum joins enum: the default-constructed value (0 / unknown)
// fails server-side membership validation (issue #109).
case ENUM, INT_ENUM -> true;
case STRING ->
effective(shape, member, software.amazon.smithy.model.traits.LengthTrait.class)
.flatMap(software.amazon.smithy.model.traits.LengthTrait::getMin)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -195,12 +195,13 @@ void writeDeserializeInto(
w.write("if (!$L->is_bool()) $L", docExpr, wrong);
w.write("$L = $L->as_bool();", outExpr, docExpr);
}
case BYTE, SHORT, INTEGER, LONG -> {
case BYTE, SHORT, INTEGER, LONG, INT_ENUM -> {
String type = context.cppSymbols().typeRef(shape);
w.write("if (!$L->is_int()) $L", docExpr, wrong);
if (shape.getType() != software.amazon.smithy.model.shapes.ShapeType.LONG) {
// Narrower integers reject out-of-range wire values instead of
// truncating (the malformed-request suite pins this).
// Narrower integers — intEnum included, its underlying type is
// int32 — reject out-of-range wire values instead of truncating
// (the malformed-request suite pins this).
String bounds =
switch (shape.getType()) {
case BYTE -> "-128 || " + docExpr + "->as_int() > 127";
Expand All @@ -216,12 +217,21 @@ void writeDeserializeInto(
}
w.write("$L = static_cast<$L>($L->as_int());", outExpr, type, docExpr);
}
case INT_ENUM -> {
String type = context.cppSymbols().typeRef(shape);
w.write("if (!$L->is_int()) $L", docExpr, wrong);
w.write("$L = static_cast<$L>($L->as_int());", outExpr, type, docExpr);
case FLOAT -> {
// The double→float narrowing is checked: a finite wire value beyond
// float range would be UB to cast (UBSan float-cast-overflow).
w.openBlock("{");
w.write("auto parsed = smithy::DoubleFromDocument(*$L);", docExpr);
w.write(
"if (!parsed) return smithy::Error::Serialization($S);", path + ": expected a number");
w.write("auto narrowed = smithy::FloatFromDouble(*parsed);");
w.write(
"if (!narrowed) return smithy::Error::Serialization($S);",
path + ": value out of range");
w.write("$L = *narrowed;", outExpr);
w.closeBlock("}");
}
case FLOAT, DOUBLE -> {
case DOUBLE -> {
String type = context.cppSymbols().typeRef(shape);
w.openBlock("{");
w.write("auto parsed = smithy::DoubleFromDocument(*$L);", docExpr);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,8 @@ private boolean memberHasConstraints(MemberShape member) {
|| hasTrait(member, target, RangeTrait.class)
|| hasTrait(member, target, PatternTrait.class)
|| hasTrait(member, target, UniqueItemsTrait.class)
|| target.isEnumShape();
|| target.isEnumShape()
|| target.isIntEnumShape();
}

private static boolean hasTrait(MemberShape member, Shape target, Class<? extends Trait> t) {
Expand Down Expand Up @@ -418,6 +419,11 @@ private void writeValueChecks(
: valueExpr;
writeEnumCheck(w, memberTarget, checked, pathVar);
}
if (memberTarget.isIntEnumShape()) {
// intEnum can't be a map key (keys are strings), so enumAsRawString
// never applies here.
writeIntEnumCheck(w, memberTarget, valueExpr, pathVar);
}
}

private static String plainNumber(BigDecimal value) {
Expand Down Expand Up @@ -646,6 +652,39 @@ private void writeUniqueItemsCheck(CppWriter w, String valueExpr, String pathVar
w.closeBlock("}");
}

private void writeIntEnumCheck(CppWriter w, Shape target, String valueExpr, String pathVar) {
var shape = target.asIntEnumShape().orElseThrow();
String type = context.cppSymbols().toSymbol(target).getName();
// Validity spans every member — @internal included, mirroring string
// enums, whose internal members FromString to a real enumerator and pass
// the kUnknown check — while the advertised set in the message omits
// them (same policy as writeEnumCheck).
String condition =
shape.members().stream()
.map(
member ->
valueExpr
+ " != "
+ type
+ "::"
+ TypeGenerators.enumConstant(member.getMemberName()))
.collect(java.util.stream.Collectors.joining(" && "));
String set =
shape.members().stream()
.filter(
member -> !member.hasTrait(software.amazon.smithy.model.traits.InternalTrait.class))
.map(member -> String.valueOf(shape.getEnumValues().get(member.getMemberName())))
.collect(java.util.stream.Collectors.joining(", "));
w.openBlock("if ($L) {", condition);
w.write(
"helpers::AddValidationFailure(failures, $L, \"Value at '\" + $L + \"' failed to satisfy "
+ "constraint: Member must satisfy enum value set: [$L]\");",
pathVar,
pathVar,
set);
w.closeBlock("}");
}

private void writeEnumCheck(CppWriter w, Shape target, String valueExpr, String pathVar) {
// @internal enum members stay valid on the wire but are omitted from the
// advertised value set (the suite pins this, incl. legacy @enum internal tags).
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,44 @@ void serverParsesTheInverseOfTheClientRequest() {
assertTrue(server.contains("body_doc->Find(\"body\")"), server);
}

@Test
void floatTextBindingsNarrowThroughTheCheckedHelper() {
// No checked-in fixture binds a float to a text position, so the golden
// trees can't pin this: a finite query/label/header value beyond float
// range must fail the parse via smithy::FloatFromDouble instead of
// hitting the UB static_cast (issue #109). Doubles stay unnarrowed.
String server =
PluginTestHarness.generate(
"""
$version: "2.0"
namespace test.rest

use alloy#simpleRestJson

@simpleRestJson
service Nums { version: "1", operations: [GetStats] }

@http(method: "GET", uri: "/stats")
operation GetStats {
input := {
@httpQuery("ratio")
ratio: Float

@httpQuery("precise")
precise: Double
}
output := { info: String }
}
""",
"test.rest#Nums",
"test::rest")
.expectFileString("/src/server.cc");
assertTrue(server.contains("smithy::FloatFromDouble(*parsed_num)"), server);
assertTrue(
server.contains("if (!narrowed_num) return std::move(narrowed_num).error();"), server);
assertEquals(1, count(server, "FloatFromDouble"), server);
}

@Test
void serverRoutesCarryContentNegotiationAndErrorIdentity() {
String server = generateFiles().expectFileString("/src/server.cc");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,43 @@ void unionsEnforceExactlyOneMemberToleratingTheTypeKey() {
assertTrue(serde.contains("unknown or missing union member"), serde);
}

@Test
void narrowedNumericsRejectOutOfRangeWireValuesInsteadOfTruncating() {
// intEnum shares the int32 bounds check with Integer members (its
// underlying type is int32 — an unchecked cast would alias 2^32+2 onto a
// valid enumerator), and float rejects finite doubles beyond float range
// (the raw cast is UB; UBSan float-cast-overflow). Double stays
// uncheckable by construction. Issue #109.
String serde =
generateSerde(
"""
$version: "2.0"
namespace test.serde

service Svc { version: "1", operations: [Op] }
operation Op { input := { payload: Payload } }

structure Payload {
weight: Weight
ratio: Float
precise: Double
}

intEnum Weight {
LIGHT = 1
HEAVY = 2
}
""");
int intEnumCheck = serde.indexOf("Payload.weight: value out of range");
int intEnumCast = serde.indexOf("static_cast<types::Weight>(member->as_int())");
assertTrue(intEnumCheck >= 0, serde);
assertTrue(intEnumCast >= 0, serde);
assertTrue(intEnumCheck < intEnumCast, "range check must precede the narrowing cast");
assertTrue(serde.contains("smithy::FloatFromDouble"), serde);
assertTrue(serde.contains("Payload.ratio: value out of range"), serde);
assertFalse(serde.contains("Payload.precise: value out of range"), serde);
}

@Test
void timestampFormatTraitOverridesTheProtocolDefault() {
String serde = generateSerde(KITCHEN_MODEL);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -104,4 +104,38 @@ void lengthCountsCodePointsForStringsAndElementsForCollections() {
assertTrue(server.contains("(*value.tags).size()"), server);
assertTrue(server.contains("Member must have length greater than or equal to 1"), server);
}

@Test
void intEnumMembershipFollowsTheStringEnumPolicy() {
// intEnum members validate against the value set the way string enums do
// (issue #109; upstream suite message form, malformed-enum.smithy
// convention with the int spellings smithy-rs/-typescript emit): validity
// spans every member — @internal included — while the advertised set in
// the message omits internal members.
String server =
generateServer(
"""
$version: "2.0"
namespace test.validation
use smithy.cpp.protocols#jsonRpc2

@jsonRpc2
service Svc { version: "1", operations: [Op] }
operation Op { input := { weight: Weight } }

intEnum Weight {
LIGHT = 1
HEAVY = 2

@internal
LEGACY = 99
}
""");
assertTrue(server.contains("!= Weight::kLight"), server);
assertTrue(server.contains("!= Weight::kHeavy"), server);
assertTrue(server.contains("!= Weight::kLegacy"), server);
assertTrue(
server.contains("failed to satisfy constraint: Member must satisfy enum value set: [1, 2]"),
server);
}
}
2 changes: 1 addition & 1 deletion docs/generated-types.md
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ compatibility contract: changes to it are breaking for consumers of generated co
| `structure` | `struct` with public members | Aggregate; `operator==` and `operator<=>` defaulted; every member value-initialized with `{}` |
| `union` | class over `std::variant` | See below |
| `enum` | class with nested `enum class Value` | See below; unknown wire values preserved |
| `intEnum` | `enum class X : std::int32_t` | |
| `intEnum` | `enum class X : std::int32_t` | Wire values outside int32 fail the parse; unknown in-range values are preserved (servers additionally validate membership) |
| `smithy.api#Unit` | `smithy::Unit` | Never declared; maps to the runtime type |
| `bigInteger` / `bigDecimal` | — | Rejected with a clear error (planned) |
| `@streaming` blob member | trait ignored | Generates as a fully buffered `smithy::Blob`; see the README's [Current limitations](../README.md#current-limitations) |
Expand Down
2 changes: 1 addition & 1 deletion docs/model-evolution.md
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,7 @@ compiling) are separate questions:
| Add optional member | ✅ old readers ignore it; absent → unset `std::optional` | ✅ additive |
| Add member with `@default` | ✅ absent → the default | ✅ additive (plain member, default-initialized) |
| Add operation | ✅ old clients never call it | ⚠️ handlers must implement the new pure-virtual method (compile error guides) |
| Add enum value | ✅ old readers preserve unknown values (`Value::kUnknown` + original text) | ✅ additive |
| Add enum value | ✅ old *clients* preserve unknown values (string enums: `Value::kUnknown` + original text; intEnums keep the in-range value) — ⚠️ old *servers* reject values outside their modeled set with 400 `ValidationException` | ✅ additive |
| Promote optional → `@required` + `@default` | ✅ absence on the wire keeps the default (the generator's evolution leniency) | ✅ member becomes plain (non-optional) — call sites reading `.has_value()` need updating |
| Promote optional → `@required` (no default) | ❌ old writers that omit it now fail deserialization | ❌ member type changes |
| Rename member | ❌ wire key changes — unless the old wire name is kept via `@jsonName` | ❌ compile errors at every use |
Expand Down
3 changes: 2 additions & 1 deletion docs/server-guide.md
Original file line number Diff line number Diff line change
Expand Up @@ -495,7 +495,8 @@ way the chat example's e2e tests do.

Inputs are validated against the model's constraint traits after parsing and before your
handler runs: `@required` (top-level body/query/header members), `@length` (strings count
Unicode code points), `@range`, `@pattern`, `@uniqueItems`, and enum membership, recursively
Unicode code points), `@range`, `@pattern`, `@uniqueItems`, and enum membership (string enums
and intEnums alike), recursively
through structures, unions, lists, and maps. Failures never reach the handler — the server
responds with the standard 400 `ValidationException` wire shape (`message` summary plus a
`fieldList` of per-member `{path, message}` entries, JSON-pointer paths like `/list/0`), with
Expand Down
Loading
Loading