feat: Support clearing nullable fields via NullFields - #585
feat: Support clearing nullable fields via NullFields#585devin-ai-integration[bot] wants to merge 2 commits into
Conversation
Params structs with nullable optional fields gain an additive
NullFields []string and a generated MarshalJSON. Listing a field's
JSON name in NullFields sends it as explicit JSON null to clear it
(e.g. NullFields: []string{"external_id"}). A nil pointer not listed
stays omitted; entries are validated against the struct's nullable
fields and an unknown name returns a marshal error. Non-breaking.
Original prompt from heather
|
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
Greptile SummaryThis PR introduces a Google-API-client-style
Confidence Score: 3/5Merging carries a real data-loss risk: a caller who sets a field value and mistakenly also lists that field in NullFields will silently clear the value at the API instead of setting it, with no error surfaced. The MarshalJSON pattern is mechanically sound across all ten files, but the silent-override behaviour — where a concrete field value is overwritten by a NullFields entry without any validation error — creates a footgun that can cause unintended data clearing in production. Combined with doc comments that advertise an invalid example field name for most structs, these two issues together warrant attention before merging. organizations.go and user_management.go are the most likely targets for the concurrent-set-and-null mistake; nullable_clearing_test.go needs a test case for that scenario; every changed file needs its NullFields comment example updated to a field name actually valid for that struct. Important Files Changed
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| for _, f := range p.NullFields { | ||
| if !nullable[f] { | ||
| return nil, fmt.Errorf("OrganizationsCreateParams: %q is not a nullable field", f) | ||
| } | ||
| m[f] = nil | ||
| } |
There was a problem hiding this comment.
NullFields
If a caller sets, say, ExternalID: workos.String("foo") and also includes "external_id" in NullFields, the MarshalJSON will first serialize the concrete value, then overwrite it with null in the map before the final marshal — sending {"external_id": null} to the API. The concrete value is silently lost with no error. This same pattern is replicated across every struct in the PR. A guard that detects this contradictory state and returns an error would prevent unintended data clearing.
Prompt To Fix With AI
This is a comment left during a code review.
Path: organizations.go
Line: 69-74
Comment:
**Concrete field value silently discarded when field is also in `NullFields`**
If a caller sets, say, `ExternalID: workos.String("foo")` and also includes `"external_id"` in `NullFields`, the MarshalJSON will first serialize the concrete value, then overwrite it with `null` in the map before the final marshal — sending `{"external_id": null}` to the API. The concrete value is silently lost with no error. This same pattern is replicated across every struct in the PR. A guard that detects this contradictory state and returns an error would prevent unintended data clearing.
How can I resolve this? If you propose a fix, please make it concise.| package workos_test | ||
|
|
||
| import ( | ||
| "encoding/json" | ||
| "testing" | ||
|
|
||
| "github.com/stretchr/testify/require" | ||
| "github.com/workos/workos-go/v9" | ||
| ) | ||
|
|
||
| // These tests verify the oagen-generated NullFields / MarshalJSON behavior for | ||
| // clearing nullable fields: | ||
| // - a concrete pointer serializes as its value | ||
| // - a nil pointer not listed in NullFields is omitted | ||
| // - a field listed in NullFields serializes as explicit JSON null | ||
| // - an unknown / non-nullable NullFields entry returns a marshal error | ||
| // - NullFields itself never appears in the JSON output | ||
|
|
||
| func TestNullableClearing_ConcreteValue(t *testing.T) { | ||
| data, err := json.Marshal(workos.OrganizationsUpdateParams{ | ||
| ExternalID: workos.String("ext-123"), | ||
| }) | ||
| require.NoError(t, err) | ||
|
|
||
| var m map[string]any | ||
| require.NoError(t, json.Unmarshal(data, &m)) | ||
| require.Equal(t, "ext-123", m["external_id"]) | ||
| require.NotContains(t, m, "NullFields") | ||
| } | ||
|
|
||
| func TestNullableClearing_OmittedNilPointer(t *testing.T) { | ||
| data, err := json.Marshal(workos.OrganizationsUpdateParams{}) | ||
| require.NoError(t, err) | ||
|
|
||
| var m map[string]any | ||
| require.NoError(t, json.Unmarshal(data, &m)) | ||
| require.NotContains(t, m, "external_id") | ||
| require.NotContains(t, m, "NullFields") | ||
| } | ||
|
|
||
| func TestNullableClearing_ExplicitNull(t *testing.T) { | ||
| data, err := json.Marshal(workos.OrganizationsUpdateParams{ | ||
| NullFields: []string{"external_id"}, | ||
| }) | ||
| require.NoError(t, err) | ||
|
|
||
| var m map[string]any | ||
| require.NoError(t, json.Unmarshal(data, &m)) | ||
| require.Contains(t, m, "external_id") | ||
| require.Nil(t, m["external_id"]) | ||
| require.NotContains(t, m, "NullFields") | ||
| } | ||
|
|
||
| func TestNullableClearing_InvalidField(t *testing.T) { | ||
| _, err := json.Marshal(workos.OrganizationsUpdateParams{ | ||
| NullFields: []string{"not_a_real_field"}, | ||
| }) | ||
| require.Error(t, err) | ||
| } | ||
|
|
||
| func TestNullableClearing_SentinelNeverLeaks(t *testing.T) { | ||
| data, err := json.Marshal(workos.OrganizationsUpdateParams{ | ||
| Name: workos.String("New Name"), | ||
| NullFields: []string{"external_id"}, | ||
| }) | ||
| require.NoError(t, err) | ||
| require.NotContains(t, string(data), "NullFields") | ||
| } | ||
|
|
||
| func TestNullableClearing_User(t *testing.T) { | ||
| data, err := json.Marshal(workos.UserManagementUpdateParams{ | ||
| NullFields: []string{"external_id"}, | ||
| }) | ||
| require.NoError(t, err) | ||
|
|
||
| var m map[string]any | ||
| require.NoError(t, json.Unmarshal(data, &m)) | ||
| require.Contains(t, m, "external_id") | ||
| require.Nil(t, m["external_id"]) | ||
| } |
There was a problem hiding this comment.
The suite covers concrete value, omitted nil, explicit null, invalid field name, and NullFields-not-leaked. It does not test the case where a caller sets a non-nil field value AND includes that same field name in NullFields simultaneously (e.g. ExternalID: workos.String("x"), NullFields: []string{"external_id"}). Given that this combination silently discards the concrete value, a test documenting the resulting behaviour (or the desired validation error) would protect against accidental regression.
Prompt To Fix With AI
This is a comment left during a code review.
Path: nullable_clearing_test.go
Line: 1-80
Comment:
**No test for the concurrent set-and-null conflict case**
The suite covers concrete value, omitted nil, explicit null, invalid field name, and NullFields-not-leaked. It does not test the case where a caller sets a non-nil field value AND includes that same field name in `NullFields` simultaneously (e.g. `ExternalID: workos.String("x"), NullFields: []string{"external_id"}`). Given that this combination silently discards the concrete value, a test documenting the resulting behaviour (or the desired validation error) would protect against accidental regression.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
2 participants
Description
Lets callers clear a nullable API field (e.g. an Organization/User
external_id) through the normal params structs, instead of hand-building a raw request with JSONnull.Go can't express the difference between "absent", "null", and "value" with a plain
*string+omitempty: a nil pointer is omitted, and&""sends an empty string (which the API rejects with 422). There was no way to send JSONnull.This adds an additive, non-breaking mechanism (Google-API-client style): params structs with nullable optional fields gain a
NullFields []stringand a generatedMarshalJSON. Existing pointer fields are unchanged.Semantics:
NullFields→ omitted (unchanged) — same as beforeNullFields→ explicit JSONnull(clears it)NullFieldsitself never appears in the JSON outputValidation
MarshalJSONvalidates eachNullFieldsentry against the struct's actual nullable field names (an allowlist generated per struct). An unknown, typoed, or non-nullable entry returns a marshal error instead of silently injecting a bogus"field": null.What changed
All changes are oagen-generated (PR workos/oagen-emitters#189) — no hand-maintained runtime file is required. Applied to both regular params structs and hidden-params body structs. The
encoding/json/fmtimports are pulled in only where a clearable field exists.Part of the cross-SDK effort (Ruby workos/workos-ruby#521, Python workos/workos-python#693) to make nullable clearing consistent.
Testing
nullable_clearing_test.gocovers concrete value, omitted nil pointer, explicit null, invalidNullFieldsentry (error), and thatNullFieldsis never serialized, for Organization and Userexternal_id.go build ./...,go vet ./..., and fullgo test ./...all green;gofmtclean.Documentation
Does this require changes to the WorkOS Docs? E.g. the API Reference or code snippets need updates.
If yes, link a related docs PR and add a docs maintainer as a reviewer. Their approval is required.
Link to Devin session: https://app.devin.ai/sessions/127c630f46c54bc0be571814c75047e8