gen: pointer-wrap nullable struct-shaped fields - #38
Open
ysyneu wants to merge 1 commit into
Open
Conversation
isNullable only recognized the OpenAPI 3.1 type: ["T", "null"] union
form used for scalar fields; it silently ignored the OpenAPI 3.0
`nullable: true` boolean form, the only marker this org's spec can put
on a `$ref` or inline-object field (a `$ref`/object sibling can't
carry a `type` array naming a non-null member). Struct-shaped fields
were also never pointer-wrapped when nullable, even when detected: a
bare Go struct's zero value has no way to represent "the API said
null", unlike the pointers/maps/slices encoding/json already nils out
on decode. A `null` from the API for such a field silently decoded
into a zero-value struct indistinguishable from a legitimately empty
object — e.g. ToolCatalogResponse.target ("null when locator could not
be uniquely resolved") and ToolInvokeResponse[.results[]].error
("mutually exclusive with data") both lost their null/present
distinction entirely.
Add isStructSchema to resolve a property schema ($ref or inline,
following allOf) to whether it emits as a Go struct, then recognize
both nullable spellings and pointer-wrap any nullable field that
resolves to a struct, in both directions (unlike the pre-existing
scalar rule, which stays request-only by deliberate, unchanged
policy — see the comment above it).
Regenerated: 5 fields flip from value to pointer type —
ToolCatalogResponse.{Target,Error}, ToolInvokeResponse.{Target,Error},
and ToolInvokeResponseResultsItem.Error — the only fields in the
vendored spec currently marked `nullable: true` on a struct-shaped
schema. All spec-example round-trip tests still pass unchanged.
Two other known-nullable $ref fields, ScheduleItem.{cur_oncall,
next_oncall} and SessionItem.bound_environment, are unaffected: the
vendored spec documents their nullability in prose only ("or null when
nobody is on-call", "Null until the first message") with no formal
marker for a generator to act on. Verified end-to-end against a
locally patched copy of the spec that adds the marker to those three
fields: regeneration correctly produces *ScheduleOncallGroup and
*EnvironmentBinding pointer fields, confirming the mechanism is
correct and ready for whenever the spec gains that marker.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Defect
A nullable
$ref(or inline-object) response field decodes to a bare Gostruct value, not a pointer.
encoding/json's null-handling only nils outpointers, maps, and slices on decode — a struct value has no such state.
So when the API sends a real
nullfor one of these fields, it silentlybecomes a zero-value struct, indistinguishable from a legitimately empty
object. A caller has no way to tell "the server said null" from "the
server sent an object with everything at its zero value."
Root cause
internal/cmd/gen'sisNullable()only recognized the OpenAPI 3.1 unionform
type: ["T", "null"]— the convention this spec uses for everynullable scalar field. It silently ignored the OpenAPI 3.0 boolean form
nullable: true, which is the only marker this org's OpenAPI can put on a$refor inline-object field at all (a$ref/object sibling can't carry atypearray naming a non-null member the way a scalar can). Even where astruct-shaped field WAS detected as nullable, nothing pointer-wrapped it —
the existing pointerization rule only ever applied to scalars, and only on
the request side.
Fix
isNullable()now recognizes both spellings.isStructSchemahelper resolves a property schema ($ref or inline,following
allOf) to whether it emits as a Go struct (as opposed to ascalar, enum, alias, slice, or map) — mirroring the same resolution
goTypeOf's object branch already does.both directions (request and response) — unlike the pre-existing
scalar rule, which stays response-side-unwrapped by deliberate, unchanged
policy (see the comment above it: decoding null into a nullable scalar
response field yielding its zero value was an accepted tradeoff for
scalars specifically; it does not extend to structs, which have no
nil-able zero state at all).
The fix lives entirely in the generator (
internal/cmd/gen/main.go); nogenerated file is hand-edited.
What actually changes
5 fields in the vendored spec are currently marked
nullable: trueon astruct-shaped schema, and flip from value to pointer type on regen:
ToolCatalogResponse.{Target,Error},ToolInvokeResponse.{Target,Error},ToolInvokeResponseResultsItem.Error. Example:ToolCatalogResponse.targetis documented as
`null` when locator could not be uniquely resolved;ToolInvokeResponseResultsItem.erroris documented as "mutually exclusivewith
data" — both cases where a caller genuinely needs to distinguish"absent" from "present but empty."
All spec-example round-trip tests (
roundtrip_test.go, which decodes everyendpoint's example payload into its generated type) pass unchanged against
the new pointer fields.
What does NOT change (yet) — follow-up needed upstream
Two other fields are documented as nullable but carry no formal spec
marker at all, only prose in their description:
ScheduleItem.cur_oncall/.next_oncall($ref: ScheduleOncallGroup) —"Current on-call group, or null when nobody is on-call." / "Next on-call
group, or null when unknown."
SessionItem.bound_environment($ref: EnvironmentBinding) —EnvironmentBinding's own schema description says "Null until the firstmessage."
Neither
nullable: truenor atype: [..., "null"]array is present forthese — the generator has no signal to act on. I verified the mechanism is
correct and ready for when that marker exists: patching a local copy of the
spec to add
nullable: trueto these three properties and regeneratingcorrectly produces
*ScheduleOncallGroupand*EnvironmentBindingpointerfields (then reverted — not part of this PR). Closing this gap needs the
spec's source of truth to start emitting
nullable: truealongside a$refwhen the underlying field is a nullable object, the same way italready does for nullable scalars; once that lands and this repo re-syncs
its vendored spec, no further change is needed here.
Consumer impact
flashduty-cliis the only consumer ofgo-flashdutyI found with realusage near these fields. It does not currently call the
ToolCatalogResponse/ToolInvokeResponseendpoints at all (zero grephits), so this PR's actual field changes have zero impact on it today.
ScheduleItem/SessionItemgain a spec marker ina future change:
flashduty-cli/internal/cli/oncall.gohas 9 usage sitesof
.CurOncall/.NextOncallthat would need small, mechanical updates(dropping
&where the field is passed to a function that already takesa pointer, and a couple of nil-guards). Noting this now so it's not a
surprise later; not part of this PR.
Verification
That one gosec hit is pre-existing and unrelated (confirmed present on
unmodified
feat/ai-sreviagit stash; the file is untouched by this PR).Load-bearing test proof: reverted the
isNullable/isStructSchemachangesin
internal/cmd/gen/main.gowhile keeping the new tests → 3 of the 4 newtests failed (RED); restored the fix → all green. The 4th test (nullable
scalar response fields stay value types) passed in both states, confirming
it's a genuine regression guard rather than accidentally coupled to the fix.