feat: union schema - #228
Open
Oudwins wants to merge 16 commits into
Open
Conversation
Contributor
WalkthroughAdds ChangesUnion Schema
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant UnionSchema
participant ChildSchema
participant Destination
Caller->>UnionSchema: Validate(dest, options)
UnionSchema->>UnionSchema: Create branch context and clone destination
UnionSchema->>ChildSchema: Validate branch
ChildSchema-->>UnionSchema: Issues or success
alt branch succeeds
UnionSchema->>Destination: Commit cloned destination
UnionSchema-->>Caller: Return success
else branch fails
UnionSchema->>UnionSchema: Preserve issues and try next child
end
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
Oudwins
force-pushed
the
feat/union-schema
branch
from
July 5, 2026 16:44
fc4bbc6 to
9cc7846
Compare
Oudwins
marked this pull request as ready for review
July 7, 2026 08:20
Contributor
Greptile SummaryThis PR adds
Confidence Score: 4/5Safe to merge; the union execution logic is correct and all previously flagged gaps are closed. The branch-isolation approach (fresh SchemaCtx per branch, deep clone of destination, commit-only-on-success) is sound and well-tested. All three earlier issues are resolved. Two minor cloneUnionValue edge cases remain but are unlikely to trigger in normal usage. union.go (cloneUnionValue non-pointer rollback path and slice cycle-key width) Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[UnionSchema.process / validate] --> B[listStart = len errors]
B --> C{for each branch schema}
C --> D[numIssues = len errors]
D --> E[newUnionBranchCtx: clone dest, fresh SchemaCtx, Exit=false, DType=branch type]
E --> F[s.process / s.validate on branchCtx]
F --> G{len errors == numIssues?}
G -- Yes: branch succeeded --> H[commit clone to original dest]
H --> I[truncate errors to listStart]
I --> J[return success]
G -- No: branch failed --> K[Free branchCtx, keep errors]
K --> C
C -- all branches exhausted --> L[return: all errors remain]
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A[UnionSchema.process / validate] --> B[listStart = len errors]
B --> C{for each branch schema}
C --> D[numIssues = len errors]
D --> E[newUnionBranchCtx: clone dest, fresh SchemaCtx, Exit=false, DType=branch type]
E --> F[s.process / s.validate on branchCtx]
F --> G{len errors == numIssues?}
G -- Yes: branch succeeded --> H[commit clone to original dest]
H --> I[truncate errors to listStart]
I --> J[return success]
G -- No: branch failed --> K[Free branchCtx, keep errors]
K --> C
C -- all branches exhausted --> L[return: all errors remain]
Reviews (3): Last reviewed commit: "wp" | Re-trigger Greptile |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
union.go (1)
58-83: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate loop logic between
processandvalidate.Both methods implement the identical try-until-success loop, differing only in calling
s.process(ctx)vss.validate(ctx). Consider extracting a shared helper parameterized by the schema method to call.♻️ Suggested refactor
+func (u *UnionSchema) tryEach(ctx *p.SchemaCtx, run func(ZogSchema, *p.SchemaCtx)) { + listStart := len(ctx.Errors.List) + for _, s := range u.schemas { + numIssues := len(ctx.Errors.List) + run(s, ctx) + if len(ctx.Errors.List) == numIssues { + ctx.Errors.List = ctx.Errors.List[:listStart] + return // success + } + } +} + func (u *UnionSchema) process(ctx *p.SchemaCtx) { - // Wrap the context and only go to the next one on fail. Keeping all the errors and appending at the end - listStart := len(ctx.Errors.List) - for _, s := range u.schemas { - numIssues := len(ctx.Errors.List) - s.process(ctx) - if len(ctx.Errors.List) == numIssues { - ctx.Errors.List = ctx.Errors.List[:listStart] - return // success - } - } + u.tryEach(ctx, ZogSchema.process) } func (u *UnionSchema) validate(ctx *p.SchemaCtx) { - // Wrap the context and only go to the next one on fail. Keeping all the errors and appending at the end - listStart := len(ctx.Errors.List) - for _, s := range u.schemas { - numIssues := len(ctx.Errors.List) - s.validate(ctx) - if len(ctx.Errors.List) == numIssues { - ctx.Errors.List = ctx.Errors.List[:listStart] - return // success - } - } + u.tryEach(ctx, ZogSchema.validate) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@union.go` around lines 58 - 83, The UnionSchema.process and UnionSchema.validate methods duplicate the same try-until-success loop, so extract the shared retry logic into a helper and pass in the schema operation to invoke. Keep the existing behavior of preserving and truncating ctx.Errors.List, and have both process and validate delegate to the new helper using their respective s.process and s.validate calls.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@union.go`:
- Around line 58-83: The UnionSchema.process and UnionSchema.validate paths are
reusing the same ctx.ValPtr across branch attempts, so a failed branch can
mutate the input and affect later branches. Update these methods to evaluate
each schema against an isolated snapshot or cloned value from SchemaCtx before
calling s.process or s.validate, and only commit the result when a branch
succeeds. Keep the existing error-collection behavior while ensuring each branch
in UnionSchema behaves like an independent alternative.
In `@zconst/consts.go`:
- Line 38: ZogTypeValues is missing TypeUnion, so union schemas are not accepted
during kind validation. Update the ZogTypeValues slice in the consts definitions
to include TypeUnion alongside the other allowed ZogType entries, so
zss_document_schema.go can validate union kinds correctly.
---
Nitpick comments:
In `@union.go`:
- Around line 58-83: The UnionSchema.process and UnionSchema.validate methods
duplicate the same try-until-success loop, so extract the shared retry logic
into a helper and pass in the schema operation to invoke. Keep the existing
behavior of preserving and truncating ctx.Errors.List, and have both process and
validate delegate to the new helper using their respective s.process and
s.validate calls.
🪄 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: af4d696b-2aae-44ec-a678-824d3f6ccb28
📒 Files selected for processing (3)
union.gounion_validate_test.gozconst/consts.go
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkgs/zss/schema/zss_document_schema.go`:
- Line 135: Update the recursive schema definitions for fields, union children,
and the element positions of ptr/preprocess/boxed to reject nil schema pointers
during validation. Ensure required-position checks count only valid non-nil
schemas, so unions cannot satisfy their minimum child count with nil entries and
recursive fields cannot contain nil values.
In `@union.go`:
- Around line 71-72: Update the union branch setup around newUnionBranchCtx and
s.process so each branch receives a deep clone of ctx.Data rather than the
shared mutable value. Preserve the existing branch processing flow while
ensuring maps, slices, pointers, and nested mutable values are independently
isolated for every alternative.
- Around line 18-20: Update EXPERIMENTAL_UNION to enforce that schemas contains
at least two branches before constructing the UnionSchema, matching the
ZSSSchemaSchema children minimum and preventing empty or single-branch unions.
- Around line 184-191: Update the struct handling in cloneUnionValue so
unexported pointer, slice, and map fields are either deep-cloned without unsafe
access or the union branch is rejected; do not leave them aliased by
clone.Set(value). Preserve recursive cloning for exported fields and ensure
failed branches cannot mutate caller-owned state.
🪄 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: c0cbdabf-dcd9-4ec4-b5ca-17001c8960a3
📒 Files selected for processing (4)
pkgs/zss/schema/zss_document_schema.gopkgs/zss/toZSS_test.gounion.gounion_validate_test.go
| "goTypes": z.Slice(ZSSGoTypeSchema), | ||
| "required": z.Ptr(ZSSTestSchema), | ||
| "defaultValue": z.EXPERIMENTAL_ANY(), | ||
| "fields": z.EXPERIMENTAL_MAP[string, *zsscore.ZSSSchema](z.String(), z.Ptr(self())), |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Reject nil schemas in required recursive positions.
fields, union children, and the element fields for ptr/preprocess/boxed accept nil pointers. For example, a union with two null children passes Min(2) despite containing no schemas.
Proposed fix
- "fields": z.EXPERIMENTAL_MAP[string, *zsscore.ZSSSchema](z.String(), z.Ptr(self())),
+ "fields": z.EXPERIMENTAL_MAP[string, *zsscore.ZSSSchema](z.String(), z.Ptr(self()).NotNil()),
- "element": z.Ptr(self()),
+ "element": z.Ptr(self()).NotNil(),
- "element": z.Ptr(self()),
+ "element": z.Ptr(self()).NotNil(),
- "element": z.Ptr(self()),
+ "element": z.Ptr(self()).NotNil(),
- "children": z.Slice(z.Ptr(self())).Required().Min(2),
+ "children": z.Slice(z.Ptr(self()).NotNil()).Required().Min(2),Also applies to: 139-160, 172-175
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkgs/zss/schema/zss_document_schema.go` at line 135, Update the recursive
schema definitions for fields, union children, and the element positions of
ptr/preprocess/boxed to reject nil schema pointers during validation. Ensure
required-position checks count only valid non-nil schemas, so unions cannot
satisfy their minimum child count with nil entries and recursive fields cannot
contain nil values.
Comment on lines
+18
to
+20
| func EXPERIMENTAL_UNION(schemas []ZogSchema, options ...SchemaOption) *UnionSchema { | ||
| s := &UnionSchema{ | ||
| schemas: schemas, |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Require at least two union branches. An empty union succeeds without running any branch, and a single-branch union serializes to children that ZSSSchemaSchema rejects (Min(2)). Enforce the minimum here or relax the schema contract.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@union.go` around lines 18 - 20, Update EXPERIMENTAL_UNION to enforce that
schemas contains at least two branches before constructing the UnionSchema,
matching the ZSSSchemaSchema children minimum and preventing empty or
single-branch unions.
Comment on lines
+71
to
+72
| branchCtx, commit := newUnionBranchCtx(ctx, s, ctx.Data) | ||
| s.process(branchCtx) |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Clone mutable parse input for each branch.
Only the destination is isolated; every branch receives the same ctx.Data. A failing preprocess or custom branch can mutate map, slice, or pointer input in place and affect later alternatives.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@union.go` around lines 71 - 72, Update the union branch setup around
newUnionBranchCtx and s.process so each branch receives a deep clone of ctx.Data
rather than the shared mutable value. Preserve the existing branch processing
flow while ensuring maps, slices, pointers, and nested mutable values are
independently isolated for every alternative.
Comment on lines
+184
to
+191
| case reflect.Struct: | ||
| clone := reflect.New(value.Type()).Elem() | ||
| clone.Set(value) | ||
| for i := 0; i < value.NumField(); i++ { | ||
| if value.Type().Field(i).PkgPath == "" { | ||
| clone.Field(i).Set(cloneUnionValue(value.Field(i), visited)) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant file first, then inspect the surrounding implementation.
git ls-files union.go
wc -l union.go
sed -n '1,280p' union.goRepository: Oudwins/zog
Length of output: 5872
🏁 Script executed:
#!/bin/bash
set -euo pipefail
git ls-files union.go
wc -l union.go
sed -n '1,280p' union.goRepository: Oudwins/zog
Length of output: 5872
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the reflection-heavy parsing paths that run inside union branches.
rg -n "Field\\(|CanSet\\(|PkgPath|Set\\(|Elem\\(\\)\\.Set|NewSchemaCtx|process\\(|validate\\(" . \
--glob '!**/*_test.go'
# Focus on likely schema implementations and context plumbing.
fd -a 'schema' .
fd -a 'context' .
fd -a 'ctx' .Repository: Oudwins/zog
Length of output: 11892
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the files that define schema traversal and context mutation.
git ls-files | rg '(^|/)(.*schema.*|.*ctx.*|.*context.*|.*internals.*)\.(go)$'Repository: Oudwins/zog
Length of output: 903
🏁 Script executed:
#!/bin/bash
set -euo pipefail
git ls-files | rg '(^|/)(.*schema.*|.*ctx.*|.*context.*|.*internals.*)\.(go)$'Repository: Oudwins/zog
Length of output: 903
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the reflection-heavy paths that union branches may execute.
rg -n "Field\\(|CanSet\\(|PkgPath|Set\\(|Elem\\(\\)\\.Set|NewSchemaCtx|process\\(|validate\\(" . \
--glob '!**/*_test.go'
printf '\n--- likely files ---\n'
git ls-files | rg '(^|/)(.*schema.*|.*ctx.*|.*context.*|.*internals.*)\.(go)$'Repository: Oudwins/zog
Length of output: 11610
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the struct traversal and field access rules.
wc -l struct.go pkgs/internals/DataProviders.go pkgs/internals/processors.go custom.go pointers.go slices.go maps.go
printf '\n--- struct.go ---\n'
sed -n '1,260p' struct.go
printf '\n--- pkgs/internals/DataProviders.go ---\n'
sed -n '1,220p' pkgs/internals/DataProviders.go
printf '\n--- pkgs/internals/processors.go ---\n'
sed -n '1,220p' pkgs/internals/processors.go
printf '\n--- custom.go ---\n'
sed -n '1,220p' custom.go
printf '\n--- pointers.go ---\n'
sed -n '1,220p' pointers.go
printf '\n--- slices.go ---\n'
sed -n '1,280p' slices.go
printf '\n--- maps.go ---\n'
sed -n '1,300p' maps.goRepository: Oudwins/zog
Length of output: 38786
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Focus on the exact union cloning logic with line numbers for the struct case.
cat -n union.go | sed -n '100,210p'Repository: Oudwins/zog
Length of output: 4050
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Probe whether a shallow struct copy aliases pointer/map/slice fields in reflect.
python3 - <<'PY'
from textwrap import dedent
print(dedent("""
package main
import (
"fmt"
"reflect"
)
type S struct {
ExportedPtr *int
unexportedPtr *int
ExportedSlice []int
unexportedSlice []int
ExportedMap map[string]int
unexportedMap map[string]int
}
func main() {
x := 1
y := 2
z := 3
s := S{
ExportedPtr: &x,
unexportedPtr: &y,
ExportedSlice: []int{10, 20},
unexportedSlice: []int{30, 40},
ExportedMap: map[string]int{"a": 1},
unexportedMap: map[string]int{"b": 2},
}
v := reflect.ValueOf(s)
c := reflect.New(v.Type()).Elem()
c.Set(v)
fmt.Println("ptr exported alias", c.FieldByName("ExportedPtr").Pointer() == v.FieldByName("ExportedPtr").Pointer())
fmt.Println("ptr unexported alias", c.FieldByName("unexportedPtr").Pointer() == v.FieldByName("unexportedPtr").Pointer())
fmt.Println("slice exported alias", c.FieldByName("ExportedSlice").Pointer() == v.FieldByName("ExportedSlice").Pointer())
fmt.Println("slice unexported alias", c.FieldByName("unexportedSlice").Pointer() == v.FieldByName("unexportedSlice").Pointer())
fmt.Println("map exported alias", c.FieldByName("ExportedMap").Pointer() == v.FieldByName("ExportedMap").Pointer())
fmt.Println("map unexported alias", c.FieldByName("unexportedMap").Pointer() == v.FieldByName("unexportedMap").Pointer())
}
"""))
PYRepository: Oudwins/zog
Length of output: 1430
Deep-clone or reject unexported reference fields in union branches.
clone.Set(value) copies the struct shallowly, but only exported fields are recursively isolated. Unexported pointer, slice, and map fields still alias the original destination, so a failed branch can leak mutations into caller state.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@union.go` around lines 184 - 191, Update the struct handling in
cloneUnionValue so unexported pointer, slice, and map fields are either
deep-cloned without unsafe access or the union branch is rejected; do not leave
them aliased by clone.Set(value). Preserve recursive cloning for exported fields
and ensure failed branches cannot mutate caller-owned state.
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
1 participant
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.
Summary by CodeRabbit
EXPERIMENTAL_UNION(...)schema support to parse/validate by trying branches until the first match.anyOf.