fix(loader): refuse wrong-typed JSON dates instead of coercing them to NULL - #669
Merged
azimafroozeh merged 2 commits intoSep 6, 2026
Merged
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
5 tasks
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.
What & why
Closes #628. A JSONL load through
omnigraph loadorPOST /graphs/{id}/loadstored NULL for a nullableDateorDateTimeproperty whose JSON value was a float (19723.0), a boolean, or an object, and reported success; the same row throughPOST /graphs/{id}/load/ndjsonwas refused.null, integer, then string, and fall through toOk(None), the value a realnullreturns, so the column builder cannot tell absent from unrecognized; only the strict mode's input-versus-output null comparison recovered the difference.invalid Date value 19723.0 for property 'since': expected an integer day count or a date string(DateTime: integer millisecond count or datetime string). Both conversion modes, scalars and list items, nullable or not, inherit it from the parser.nullload as before.Backing issue / RFC
wrong_typed_date_values_are_refused_in_both_modes_issue_628andload_refuses_float_epoch_in_nullable_date_issue_628incrates/omnigraph/src/loader/mod.rssatisfy the Fix Regression Gate)Checklist
load_jsonltest that pins the message and an unchanged store version after the refusal)docs/user/mutations/index.md§Bulk loading names the accepted shapes and the refusal;skills/omnigraph/SKILL.mdandreferences/queries.mdgain integer epoch milliseconds forDateTime;docs/dev/ingestion.mdstates the compatibility coercion rule and the dates exception; release note)Local verification
cargo test -p omnigraph-engine --lib loader::— 29 passedcargo test -p omnigraph-gqt— 116 self-tests + 13 cases passedcargo clippy --workspace --all-targets— cleancargo fmt --all --check— cleanscripts/check-docs.py— OK (129 files)typos— cleancargo test -p omnigraph-server— all targets green (387 passed, 0 failed, 2 ignored)cargo test -p omnigraph-cli— all targets green (337 passed, 0 failed, 15 ignored)s3_storage— not run: the change is confined to two parser functions whose only callers are the loader column arms covered aboveNotes for reviewers
DateandDateTimerefuse a wrong JSON type.String, integer, float, andBoolproperties keep the compatibility loader's historical NULL coercion, now stated indocs/dev/ingestion.md. Widening the refusal to every scalar is a contract change for existingloadusers and is deliberately not part of this fix.strict property 'day' expects Date32, got 19723.0becomes theinvalid Date valuemessage. No test asserted the old text for dates..gqtbecause a seed load failure is a harness error, not an expectable outcome; theFIXME(#628)lines above them name the conversion to one.gqtcase once GQ has aloadstatement.Datestring carrying a UTC offset stores the UTC day. That is the string parser, pre-existing, and reported separately.