feat!: Support nullable fields by default - #219
Conversation
This adds a method to the Ptr type, allowing fields to be explicitly set to a nil value where as right now a nil in the input map will not set a pointer to nil on a struct. This allows the pointer on the struct to be set to nil on an opt in basis.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis pull request introduces explicit null handling by creating a new sentinel type to distinguish between Changes
Sequence DiagramsequenceDiagram
participant Input as Input Data
participant Provider as Data Provider
participant Detect as Explicit Null<br/>Detection
participant Schema as Pointer Schema<br/>Processor
participant Dest as Destination
Input->>Provider: Provide value (nil pointer<br/>or typed-nil interface)
Provider->>Detect: Check IsExplicitNullSource()
Detect-->>Provider: true (explicit null detected)
Provider-->>Schema: Emit ExplicitNullMarker()
Schema->>Detect: Check IsExplicitNull()
Detect-->>Schema: true (sentinel recognized)
Schema->>Dest: Clear pointer to zero value<br/>(if not required)
Dest-->>Schema: Pointer cleared
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkgs/internals/DataProviders.go`:
- Around line 51-55: The switch on field.Kind() in DataProviders.go fails to
treat typed nils inside interfaces or map values as explicit nulls; update the
logic in the branch handling reflect.Pointer/reflect.Interface (and the similar
block at the other occurrence) to first unwrap reflect.Interface values by
calling field.Elem() when field.Kind() == reflect.Interface and then test
IsNil() on all nillable kinds (reflect.Pointer, reflect.Slice, reflect.Map,
reflect.Chan, reflect.Func); when any unwrapped nillable value is nil, return
ExplicitNullMarker so Nullable() pointer schemas can distinguish present-null
from absent keys.
In `@slices.go`:
- Around line 167-173: The current check uses item == nil after calling
refVal.Index(idx).Interface(), which misses typed nils; instead inspect the
reflect.Value before calling Interface(): get elem := refVal.Index(idx) and if
elem.IsValid() and elem.CanInterface() and elem.Kind() is one of the nilable
kinds (Ptr, Interface, Map, Chan, Func, Slice) and elem.IsNil() then set item =
p.ExplicitNullMarker; otherwise set item = elem.Interface(). Update the code
paths around refVal.Index(idx), item, and p.ExplicitNullMarker so typed nil
slice elements are mapped to the sentinel just like untyped nils.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 2a35f3c3-d379-4242-aa3c-bb898de180c3
📒 Files selected for processing (5)
pkgs/internals/DataProviders.gopkgs/internals/zeroValues.gopointers.gopointers_nullable_test.goslices.go
Greptile SummaryThis PR changes Confidence Score: 5/5Safe to merge; no P0/P1 issues found — only two P2 observations about test coverage and sentinel type visibility. All P0/P1 concerns from previous threads have been addressed. The sentinel correctly avoids leaking to issue values for both pointer and primitive schemas. The two remaining findings are P2: a vacuously-passing slice test and an exported sentinel type that allows external DataProvider implementations to create matching instances. pkgs/internals/zeroValues.go — exported ExplicitNull type; pointers_nullable_test.go — slice nil test coverage gap. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["MapDataProvider.Get(key)"] --> B{key present?}
B -- No --> C["return nil (absent)"]
B -- Yes --> D{value == nil or typed nil ptr?}
D -- No --> E["return value"]
D -- Yes --> F["return ExplicitNullMarker()"]
F --> G["PointerSchema.process()"]
C --> G
E --> G
G --> H{IsParseZeroValue or isEmptyStruct?}
H -- No --> I["Allocate new ptr / Process sub-schema"]
H -- Yes --> J{required != nil?}
J -- Yes --> K["Strip sentinel from issue value, AddIssue + return"]
J -- No --> L{IsExplicitNull(ctx.Data)?}
L -- Yes --> M["reflect.Zero → clear dest pointer"]
L -- No --> N["no-op (key absent, preserve existing)"]
Reviews (4): Last reviewed commit: "chore: Cleanup from code review" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkgs/internals/DataProviders.go (1)
100-121: Typed-nil pointer handling in MapDataProvider is correct.The two-step check (
any(v) == nilfor the interface-nil case, thenreflect.ValueOf(v)for typed-nil pointers insidemap[string]any) properly distinguishes "present-null" from "absent". Rationale for omitting other nillable kinds is consistent withStructDataProvider.Get.Minor optional thought: consider extracting the reflect-based check into a shared helper alongside
isExplicitNullValueto keep the two code paths in lockstep if the policy ever changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkgs/internals/DataProviders.go` around lines 100 - 121, Refactor the explicit-null detection in MapDataProvider.Get into a shared helper so both code paths stay in sync: extract the two-step logic (the interface-nil check any(v) == nil and the typed-nil pointer reflect.ValueOf(v) check) into a function (e.g., isExplicitNullValue(v any) bool or extend the existing isExplicitNullValue) and replace the inline checks in MapDataProvider.Get with a single call to that helper; keep the behavior identical (only treat interface-nil and typed-nil pointer as explicit nulls, not slices/maps/chans/funcs).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkgs/internals/DataProviders.go`:
- Around line 100-121: Refactor the explicit-null detection in
MapDataProvider.Get into a shared helper so both code paths stay in sync:
extract the two-step logic (the interface-nil check any(v) == nil and the
typed-nil pointer reflect.ValueOf(v) check) into a function (e.g.,
isExplicitNullValue(v any) bool or extend the existing isExplicitNullValue) and
replace the inline checks in MapDataProvider.Get with a single call to that
helper; keep the behavior identical (only treat interface-nil and typed-nil
pointer as explicit nulls, not slices/maps/chans/funcs).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 38e3bd33-746d-4fae-971e-1d659a808df2
📒 Files selected for processing (3)
pkgs/internals/DataProviders.gopointers_nullable_test.goslices.go
✅ Files skipped from review due to trivial changes (1)
- pointers_nullable_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- slices.go
|
@greptile re-review please |
|
@Oudwins are you able to take a look at this possibly? If not or if its not something you want to try to add in at the moment then I'll fork in the mean time! Thank you! |
|
@elliotcourant what is the use cases for this? If you need it now I would recommend you fork. Not sure if this is something I would like to support tbh. I'm open to it if there is a clear common use case though. edit: Because any pointer in go will default to nil. So this is only useful if you are reusing structures right? Which is not very common I could be convinced this is how ptr should work by default though edit2: Also in general I have time to check this on weekends only but feel free to ping if something is urgent |
|
Basically I have a REST API for a budgeting app, and I have transaction objects that keep track of a "spending ID" to see which budget they have been spent from. Initially the value is nil and a user can update the budget through the UI to a spending ID to assign it to a budget. But if they want to unassign it from that budget, then with this schema thats not currently possible: PatchTransaction = zog.Struct(zog.Shape{
"spendingId": zog.Ptr(ID[models.Spending]().Optional()),
})Since the way this is done is by reading the existing transaction from the database, then parsing the request body onto that existing transaction via the schema. This way I can be sure that someone can't update a field they aren't supposed to or can't provide malformed data while achieving a "patch" behavior in go. But with this use case, the transaction model has this essentially type Transaction struct {
// Other fields...
SpendingId *ID[Spending] `json:"spendingId"`
}So the existing transaction would have this field defined, and in this scenario I'd love for the schema to be able to handle setting that field to nil while still validating the struct/request etc. Full code references here: https://github.com/monetr/monetr/blob/7f0b93a92c98afa74b7b31e67df6c0e0e5e85985/server/models/transaction.go#L39-L39 < Transaction model https://github.com/monetr/monetr/blob/7f0b93a92c98afa74b7b31e67df6c0e0e5e85985/server/schema/transaction.go#L24-L30 < Patch transaction schema https://github.com/monetr/monetr/blob/7f0b93a92c98afa74b7b31e67df6c0e0e5e85985/server/controller/transactions.go#L375-L389 < Patch transaction API code https://github.com/monetr/monetr/blob/7f0b93a92c98afa74b7b31e67df6c0e0e5e85985/server/controller/transactions_test.go#L2094-L2127 < Test demonstrating the problem (this test fails currently) https://github.com/monetr/monetr/blob/7f0b93a92c98afa74b7b31e67df6c0e0e5e85985/server/controller/helpers.go#L337-L376 < parse wrapper to turn zog schema issues into an API error response + copy and parse |
|
This also seems to mirror the behavior offered here https://zod.dev/api?id=nullables |
|
Hey! Cool app! I see the issue and I also think you are right that this might be a common-ish use case and should be implemented. Let me consider it a bit today/tomorrow |
|
Follow up: I thought more about this and this should be the default behaviour. nullable should not be a method. Parse should behave like unmarshal (see example below). ❯ go run ./tmp/json_unmarshal_null_demo
before: name="Old Name" age=42 emailIsNil=false email="old@example.com"
json: {}
err: <nil>
after: name="Old Name" age=42 emailIsNil=false email="old@example.com"
before: name="Old Name" age=42 emailIsNil=false email="old@example.com"
json: {"name": null, "email": null, "age": null}
err: <nil>
after: name="Old Name" age=42 emailIsNil=true email=nil
before: name="Old Name" age=42 emailIsNil=false email="new@example.com"
json: {"name": "New Name", "email": "new@example.com", "age": 10}
err: <nil>
after: name="New Name" age=10 emailIsNil=false email="new@example.com"
Additionally, I see you did some changes on the slice. The right semantics for the slice are these: json=[null] -> slice=[nil] We should not filter out nil values or anything like that |
|
I think I might have gotten some assumptions with slices wrong here. But are you sure this should be the default behavior with pointer? Does that blur the line for how absence of a field behaves? For example if a field is simply omitted in the input but its marked as required (not nil) on pointer, AND the field is present and has a value on the destination struct; I believe that is a different behavior than what this PR proposes. Unless I misunderstand. In my mind (and this might not be the correct way to think of things):
Does that make sense? |
Yes correct. I get is a little confusing from the perspective of the code because I called notNil required. But from an API perspective since go doesn't have absence of value type User struct {
Name string
Email *string
}
# Left as is because json is empty
before: name="Old Name" email="old@example.com"
json: {}
err: <nil>
after: name="Old Name" email="old@example.com"
# Updated because explicit null value
before: name="Old Name" email="old@example.com"
json: {email": null}
err: <nil>
after: name="Old Name" email=nil
This is exactly what we cannot model with my proposed api. Which might mean that what I proposed doesn't work. But I feel like this is unmarshaling logic that is leaking into our schemas. How would nullable behave for validate path? Just do nothing? How do we handle nil values in a map when parsing from a map? |
|
Okay I think I'm getting to the same page as you then overall. Because of how go already handles the implicit versus explicit nil (especially in regards to unmarshaling) the change to make this the default. But also to mirror the behavior of existing json.Unmarshal I think is the right way to go here since its predictable for go. It does somewhat muddy the schema/unmarshaling behaviors a bit, but I think that line was blurred with the basis of zod to begin with. Most tools (json-schema, openapi, etc) don't blur the line of parsing and schema like zod does. And thus zog being inspired by it also blurs that line a bit. What I'll do in this PR for now though is make this the new default behavior, which will make this a breaking change technically but we can call that out. I think I'll also revert my slice changes for now? I'm not sure how these are being used in practice and I don't want to make assumptions unless you want to provide some guidance there? |
|
Yes agree. Parsing by its nature muddies the waters between schema & unmarshaling which works better in js than in go due to how the languages themselves treat nil & missing values. However, in go I still think it has significant value as it allows for specific behaviours that are very difficult to achieve via I have been thinking for a while that its likely possible that I will be force to create a pretty big breaking change in the future in the http api to convert it into a wrapper schema which could potentially improve clarity on the user side for these kinds of things and also help fix some other nasty bits around the code. But I tend to want to avoid as many breaking changes as possible so will only go there if I feel its absolutely necessary. |
There was a problem hiding this comment.
🧹 Nitpick comments (3)
pkgs/internals/DataProviders.go (2)
48-78: LGTM — typed-nil unwrapping is correct.
IsExplicitNullSourcecorrectly distinguishes the two cases that previously collapsed: nil interface (field.IsNil()true) and a non-nil interface holding a typed-nil pointer (theinnerbranch). The deliberate exclusion of slice/map/chan/func is the right call to avoid changing semantics for non-pointer schemas that treat nil slices/maps as empty input.Optional nit: this helper is only called from
StructDataProvider.Getin this file; if no other internal package needs it, you could lower visibility toisExplicitNullSource. Non-blocking.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkgs/internals/DataProviders.go` around lines 48 - 78, The helper IsExplicitNullSource can be lowered to package-local since it's only used by StructDataProvider.Get; rename the exported function IsExplicitNullSource to isExplicitNullSource (update its declaration) and update all call sites (notably StructDataProvider.Get) to the new name so visibility is reduced without changing behavior.
103-124: LGTM — both nil-interface and typed-nil-pointer cases covered.Two-step detection (
any(v) == nilfor nil interface, thenreflect.ValueOf(v).Kind() == Pointer && IsNil()for typed-nil) coversMapDataProvider[any]with barenilvalues,MapDataProvider[any]with typed-nil pointer values, andMapDataProvider[*T]with nil values. The doc comment at lines 114-119 specifically calls out[any]; consider also mentioning that this branch additionally handles typed pointer maps (MapDataProvider[*T]) for completeness.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkgs/internals/DataProviders.go` around lines 103 - 124, Update the comment inside the MapDataProvider.Get method to also call out that the typed-nil-pointer branch handles maps with pointer element types (e.g., MapDataProvider[*T]) in addition to MapDataProvider[any]; specifically, edit the doc text around the existing note that references “[any]” (the comment between the any(v) == nil check and the reflect.ValueOf branch) to mention both MapDataProvider[any] with typed-nil pointers and MapDataProvider[*T] cases so readers understand the reflect-based nil-pointer handling covers pointer-typed element maps as well as interface-typed maps.pointers_nullable_test.go (1)
22-23: Userequirefor preconditions before dereferencing/indexing.These tests assert
NotNil/NotEmptyand then immediately dereference the pointer or index into the slice. Becauseassertdoes not abort the test on failure, a regression that violates the precondition would manifest as anilpointer deref panic or index-out-of-range rather than a clean failure with the intended message — making the actual cause harder to diagnose in CI.♻️ Suggested change (illustrative, applies to each affected site)
- "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require"- assert.NotNil(t, out.Tag) - assert.Equal(t, "keep-me", *out.Tag) + require.NotNil(t, out.Tag) + assert.Equal(t, "keep-me", *out.Tag)- assert.NotEmpty(t, errs) - assert.False(t, p.IsExplicitNull(errs[0].Value), "issue value must not be the internal sentinel") + require.NotEmpty(t, errs) + assert.False(t, p.IsExplicitNull(errs[0].Value), "issue value must not be the internal sentinel")For the slice test (lines 111-116),
require.Len(t, out, 3)similarly guards the indexing that follows.Also applies to: 49-50, 87-88, 112-116, 140-141, 152-154
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pointers_nullable_test.go` around lines 22 - 23, Replace non-fatal assertions that guard dereferences/indexing with fatal ones: for each site that does assert.NotNil(t, out.Tag) before using *out.Tag (reference: out.Tag), change to require.NotNil(t, out.Tag) so the test aborts instead of panicking; for slice indexing cases (reference: out slice usages where the test does assert.Len/NotEmpty then indexes into out), use require.Len(t, out, N) or require.NotEmpty(t, out) before indexing; similarly swap assert.NotEmpty/NotNil to require.* at the locations mentioned so preconditions are enforced before dereferencing or indexing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkgs/internals/DataProviders.go`:
- Around line 48-78: The helper IsExplicitNullSource can be lowered to
package-local since it's only used by StructDataProvider.Get; rename the
exported function IsExplicitNullSource to isExplicitNullSource (update its
declaration) and update all call sites (notably StructDataProvider.Get) to the
new name so visibility is reduced without changing behavior.
- Around line 103-124: Update the comment inside the MapDataProvider.Get method
to also call out that the typed-nil-pointer branch handles maps with pointer
element types (e.g., MapDataProvider[*T]) in addition to MapDataProvider[any];
specifically, edit the doc text around the existing note that references “[any]”
(the comment between the any(v) == nil check and the reflect.ValueOf branch) to
mention both MapDataProvider[any] with typed-nil pointers and
MapDataProvider[*T] cases so readers understand the reflect-based nil-pointer
handling covers pointer-typed element maps as well as interface-typed maps.
In `@pointers_nullable_test.go`:
- Around line 22-23: Replace non-fatal assertions that guard
dereferences/indexing with fatal ones: for each site that does assert.NotNil(t,
out.Tag) before using *out.Tag (reference: out.Tag), change to require.NotNil(t,
out.Tag) so the test aborts instead of panicking; for slice indexing cases
(reference: out slice usages where the test does assert.Len/NotEmpty then
indexes into out), use require.Len(t, out, N) or require.NotEmpty(t, out) before
indexing; similarly swap assert.NotEmpty/NotNil to require.* at the locations
mentioned so preconditions are enforced before dereferencing or indexing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: ae0cf7d7-1c9d-46cc-9e36-12c0acc92fa2
📒 Files selected for processing (4)
pkgs/internals/DataProviders.gopkgs/internals/zeroValues.gopointers.gopointers_nullable_test.go
elliotcourant
changed the title
elliotcourant
changed the title
|
Yeah it doesn't help that schemas and parsing are extremely opinionated, I've messed around with ozzo, json-schema and a few others trying to find something that works just right and this is the closest I've gotten. Changes made and I reverted the slice change as I'd rather leave that be for this PR and just address the core issue I'm having which is being able to set a field back to nil. This is all updated now based on the comments here. Let me know what your thoughts are! |
|
Glad to hear that. I'll take a look at this PR later today/tomorrow. |
Oudwins
left a comment
Oudwins
left a comment
There was a problem hiding this comment.
Looks pretty good. a few minor things but after that I think it will be ready to merge
| // A nil pointer or interface field is the struct-input equivalent of an | ||
| // explicit null key. Emit the sentinel so pointer schemas can clear the | ||
| // destination. Non-pointer schemas still short-circuit via IsParseZeroValue. | ||
| if IsExplicitNullSource(field) { |
There was a problem hiding this comment.
nit: I think I would limit this behaviour for now to just the map data provider as that is what the json parser uses.
There was a problem hiding this comment.
That makes sense I'll make that change!
|
|
||
| // Explicit null in the input clears the destination pointer. NotNil opts | ||
| // out of clearing and falls through to the required-error path below. | ||
| if p.IsExplicitNull(ctx.Data) && v.required == nil { |
There was a problem hiding this comment.
this requires an extra check for v.required which is not needed. The explicit check should be moved inside the isZero if past the check for if v.required != nil
|
One more thing before I forget. Please look over the comments and make sure they are minimal. Seemed a little excessive/ai in some places. Fine to have some explanation but ideally small and not repeated across multiple comments |
|
Sorry busy week I will come back to this this weekend! and yes I can update comments! |
Oudwins
left a comment
Oudwins
left a comment
There was a problem hiding this comment.
Looks pretty good. a few minor things but after that I think it will be ready to merge
| return dp, nil | ||
| } | ||
| if val == nil { | ||
| // Here we treat the sentinel and a nil (absence) as the same because we are |
There was a problem hiding this comment.
Are you sure this is right?
if
var userPtr *User
errs := userPtrSchema.Parse(data, &userPtr)
We should be setting the ptr to nil to keep the behaviour consistent. So
var userPtr *User = &User{}
errs := userPtrSchema.Parse(data, &userPtr) // where data = nil
// should result in userPtr == nil
|
@elliotcourant are you going to have time to come back and finalize this PR? Or should I take it on when I have capacity? No worries either way |
|
Sorry I have been busy with other stuff, I will make it back to this at some point but if its something you want to get in sooner rather than later it might be easier to take it over im sorry. |
|
Okay no worries! Take as muchos time as you need. Im working on schema generation right now. Basically done. Maybe after that once I have time I'll take a look at this if you haven't had the time until then. Good luck! Hoping whatever has you busy isn't too taxing! |
2 participants
This adds a method to the Ptr type, allowing fields to be explicitly set to a nil value where as right now a nil in the input map will not set a pointer to nil on a struct. This allows the pointer on the struct to be set to nil on an opt in basis.This also counteractsNotNilon the Ptr object, only NotNil or Nullable can be set, not both.This changes the default behavior of Ptr types in the schema to allow setting the value the Ptr is associated with to nil.
BREAKING CHANGE: This is a breaking change as it changes the default behavior of Ptr when nil values are explicitly provided as the input on the parsing of a schema. Previously nil values would be treated as if the key was not specified at all. Now nils are treated as a proper "lack of value" in the input, and will try to overwrite the pointer on the destination struct with the value
nil.Summary by CodeRabbit
Release Notes