fix: validate schema field IDs during unmarshal#1315
Conversation
tanmayrauth
left a comment
tanmayrauth
left a comment
There was a problem hiding this comment.
LGTM, one nit on test coverage.
| assert.True(t, tableSchemaSimple.Equals(&schema)) | ||
| } | ||
|
|
||
| func TestUnmarshalSchemaRejectsDuplicateFieldIDs(t *testing.T) { |
There was a problem hiding this comment.
Nice — the top-level and nested-struct cases are the important ones. Since checkDuplicateFieldIDs also walks list element IDs and map key/value IDs (schema.go:118-130), would you consider adding a list or map dup-ID case too? Those branches are only exercised indirectly right now, and they're the easiest to silently break later.
fallintoplace
force-pushed
the
fix/schema-unmarshal-duplicate-field-ids
branch
from
b5da3aa to
30d2edd
Compare
zeroshade
left a comment
zeroshade
left a comment
There was a problem hiding this comment.
Thanks for adding this. The duplicate-ID detection is correct, and I like the recursion into struct/list/map plus the negative-placeholder carve-out. Requesting changes on one gap relative to the PR title (validate schema field IDs) - noted inline.
| return err | ||
| } | ||
|
|
||
| if err := checkDuplicateFieldIDs(nil, aux.Fields); err != nil { |
There was a problem hiding this comment.
This catches duplicate positive IDs, but missing list/map IDs still slip through. checkDuplicateFieldIDs only records f.ID > 0 (schema.go:101), and an omitted list element-id or map key-id/value-id decodes to 0 - so a schema that omits one of those still unmarshals cleanly (a lone 0 isn't a duplicate). If the goal is to validate field IDs on unmarshal, it'd be worth rejecting missing/zero element/key/value IDs too (e.g. decode them as *int and error when absent), while still allowing the negative placeholders used during construction. Is the broader validation in scope here, or is this intentionally limited to duplicates?
3 participants
Summary
Why
Schema constructors already reject duplicate positive field IDs, but Schema.UnmarshalJSON assigned decoded fields without running the same check. Malformed schema JSON could enter the system and later corrupt ID-based lookups, projections, name mapping, or writes.
Testing