fix(go): apply bigint/decimal wire-format tags to OptionalNullable-wrapped JSON fields - #15
AshGodfrey wants to merge 8 commits into
Conversation
…apped JSON fields
|
Running ultrareview automatically — This changes the core JSON marshalling/unmarshalling template that all generated Go SDKs rely on, altering wire formats for OptionalNullable-wrapped bigint/decimal fields via reflection-based unwrapping; a subtle bug could break data integrity across every SDK consumer.. I'll post findings when complete. |
…as null instead of panicking in the tag-dispatch path
Generator Snapshot Testing
Snapshot results: https://github.com/speakeasy-api/openapi-generation-snapshots/issues/89#issuecomment-5678837904 Snapshot results are summarized above. Additional run context is linked for maintainers with access. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
Ultrareview completed in 19m 31s
All reported issues were addressed
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
…inst the inner type handleDefaultConstValue switched on the wrapper's map type, so a wrapped field carrying both a default and a tag-driven wire format (e.g. bigint:"string") rendered its default in the untagged form; the tagged unmarshal path then rejected the default when the field was absent from the payload. Unwrap the OptionalNullable type before selecting the default's rendering so defaults follow the same wire format as set values, on both the marshal and unmarshal sides.
| features: | ||
| - nullables | ||
| targets: | ||
| - go |
There was a problem hiding this comment.
| - go | |
| - go | |
| - cli | |
| - terraform |
There was a problem hiding this comment.
should the mockserver be updated accordingly?
templates/templates/mockserver/auxiliary/internal/sdk/utils/json.go.stmpl:302templates/templates/mockserver/auxiliary/internal/sdk/utils/json.go.stmpl:405templates/templates/mockserver/auxiliary/internal/sdk/utils/json.go.stmpl:511
Also wondering why this logic isn't centralized... I think we should try and pull the logic into common templates ` e.g.
templates/templates/common/go/optionalnullable/
├── reflect.go.stmpl
└── json_dispatch.go.stmpl
There was a problem hiding this comment.
Made a follow-up Linear ticket as the drift expands beyond this.
…Nullable wire-format fix [skip changelog]
…lable-wrapped JSON fields
There was a problem hiding this comment.
1 issue found across 4 files (changes from recent commits).
Confidence score: 2/5
- In
templates/templates/mockserver/auxiliary/internal/sdk/utils/json.go.stmpl, non-null unmarshaling into value-formbig.Intordecimal.Bigcan panic becauseinnerPtris unsettable while the decoders callSetwithout dereferencing; handle pointer/value targets correctly and add regression coverage for both types.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="templates/templates/mockserver/auxiliary/internal/sdk/utils/json.go.stmpl">
<violation number="1" location="templates/templates/mockserver/auxiliary/internal/sdk/utils/json.go.stmpl:540">
P1: When `T` is `big.Int` or `decimal.Big` instead of a pointer, non-null unmarshaling panics. `innerPtr` is an un-settable `*T`, and the existing big-number decoders call `v.Set` without moving to `v.Elem()` for this pointer shape; make the inner value settable or update those decoders to handle value-parameter wrappers.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
| // never gets this far: it is handled at the top of this function, where | ||
| // the wrapper's own UnmarshalJSON records the explicit null state. | ||
| if optionalnullable.IsOptionalNullableType(typ) { | ||
| innerPtr := reflect.New(typ.Elem().Elem()) |
There was a problem hiding this comment.
P1: When T is big.Int or decimal.Big instead of a pointer, non-null unmarshaling panics. innerPtr is an un-settable *T, and the existing big-number decoders call v.Set without moving to v.Elem() for this pointer shape; make the inner value settable or update those decoders to handle value-parameter wrappers.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At templates/templates/mockserver/auxiliary/internal/sdk/utils/json.go.stmpl, line 540:
<comment>When `T` is `big.Int` or `decimal.Big` instead of a pointer, non-null unmarshaling panics. `innerPtr` is an un-settable `*T`, and the existing big-number decoders call `v.Set` without moving to `v.Elem()` for this pointer shape; make the inner value settable or update those decoders to handle value-parameter wrappers.</comment>
<file context>
@@ -509,6 +532,24 @@ func unmarshalValue(value json.RawMessage, v reflect.Value, tag reflect.StructTa
+ // never gets this far: it is handled at the top of this function, where
+ // the wrapper's own UnmarshalJSON records the explicit null state.
+ if optionalnullable.IsOptionalNullableType(typ) {
+ innerPtr := reflect.New(typ.Elem().Elem())
+
+ if err := unmarshalValue(value, innerPtr, tag); err != nil {
</file context>
There was a problem hiding this comment.
Not reachable in generated code: bigint and decimal fields are always emitted as pointer types — sanitization.ts forces *big.Int and *decimal.Big because their marshal methods have pointer receivers — so the wrapper is always OptionalNullable[*big.Int]/OptionalNullable[*decimal.Big], which takes the double-pointer path in the big-number decoders. Value type parameters only occur for strings and enums, which decode through plain json.Unmarshal into the settable pointer. The go SDK template has the same shape for the same reason.
…e wire-format support [skip changelog]
There was a problem hiding this comment.
All reported issues were addressed across 8 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
…in the unmarshal unwrap path
…le-wire-format-tags # Conflicts: # templates/templates/go/tests/primary/requestbodies_additional_test.go.stmpl
2 participants
Why
A JSON body field wrapped in
optionalnullable.OptionalNullable[T]is marshalled and unmarshalled through the wrapper's ownMarshalJSON/UnmarshalJSON, which cannot see the field's struct tags. Tag-driven wire formats were silently dropped:bigint:"string"fields (and arrays of them) went on the wire as bare JSON numbers instead of quoted strings, violating the declaredtype: stringschema and silently corrupting values beyond 2^53 for any consumer that parses numbers as float64. A quoted string on the wire failed to decode at all (math/big: cannot unmarshal ...).decimal:"number"fields went on the wire as quoted strings instead of bare numbers (the inverse defect).The same field as a bare
*big.Int/*decimal.Bigserializes correctly, so the wrapped and unwrapped forms of the same schema disagreed on the wire.What changed
utils.MarshalJSON/UnmarshalJSON(Go auxiliaryjson.go) now unwrapOptionalNullable[T]and re-dispatch the inner value with the field's tag — reading the wrapper's set entry directly (rather thanGetUntyped(), whose dereference would lose pointer-receiver marshalers on the inner value) — so wrapped fields serialize exactly like their unwrapped counterparts.Tri-state behavior is unchanged: JSON
nullis still recorded via the wrapper'sUnmarshalJSONon the decode side (IsSet + IsNull), explicit null and unset still emit as before, and untagged wrapped fields (plain bigint/decimal, dates, objects, maps, unions, scalars) keep their existing wire format.Testing
bigint:"string"(scalar and array),decimal:"number", decimal-string, decimal-string maps, and the explicit-null case, plus a deserialize counterpart asserting quoted strings decode into the wrapped*big.Int/*decimal.Bigand the tri-state survives. Byte-level assertions are used because a round-trip echo shares the defect on both legs and cannot catch it. Verified these tests fail against the previous template state with exactly the defects described above.optionalnullableandinternal/utilspackage tests pass against the regenerated review SDK.From(&p)withpa nil*big.Int/*decimal.Big) previously panicked in the tag-dispatch path after unwrapping; it now marshals asnull, the same as the bare nil pointer, pinned by a serialize test.make lintpasses. Go, CLI, and Terraform review SDKs regenerated with no drift beyond the committed files; the Terraform review build's acceptance tests pass.Public-safety check
.claude/skills/public-repo-communication/SKILL.md).git diff --check.