fix: serialise OffsetDateTime as RFC 3339 - #609
Open
dudanogueira wants to merge 1 commit into
Open
Conversation
OffsetDateTime.toString() omits the seconds when the second and the nanosecond are both zero, so a timestamp on an exact minute boundary went on the wire as "2024-03-01T00:00Z". RFC 3339's partial-time requires hour:minute:second, and the server rejects the short form -- breaking writes and filters alike for most timestamps written by hand. DateUtil owned the read side but had no write-side counterpart, which is why the same toString() was copy-pasted into all six marshalling sites. It now has toRFC3339(), which always writes the seconds and keeps the fraction variable-width so sub-second precision is neither invented nor truncated. The array and list variants in Filter and InsertManyRequest were affected too, not just the three scalar sites in the report. Reading is unchanged and stays lenient: OffsetDateTime.parse accepts both forms, so timestamps written by older clients still load. Closes #605 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WmY5dAGWCccWDoqkKNC2JU
There was a problem hiding this comment.
Orca Security Scan Summary
| Status | Check | Issues by priority | |
|---|---|---|---|
| Infrastructure as Code | View in Orca | ||
| SAST | View in Orca | ||
| Secrets | View in Orca | ||
| Vulnerabilities | View in Orca |
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.
Motivation
OffsetDateTime.toString()drops the seconds when the second and the nanosecond are both zero, so2024-03-01T00:00:00Zgoes on the wire as2024-03-01T00:00Z. RFC 3339'spartial-timerequireshour ":" minute ":" second, and Weaviate rejects the short form:Every timestamp on an exact minute boundary is affected — which is most timestamps written by hand — and it breaks writes and filters alike.
toString()is a display format, not a wire format.Approach
DateUtilalready owned the read side (fromISO8601) but had no write-side counterpart, which is exactly why the sametoString()call got copy-pasted into every marshalling site. It now hastoRFC3339(), and all six sites call it.The formatter is built with
DateTimeFormatterBuilderrather thanofPattern("yyyy-MM-dd'T'HH:mm:ssXXX"): the pattern form would fix the reported bug while silently truncating sub-second precision that works today.appendFraction(NANO_OF_SECOND, 0, 9, true)keeps the fraction variable-width, so it is neither invented for whole seconds nor truncated for nanos.Scope: six call sites, not three
The issue named three. The list and array variants have the same bug:
Filter.DateOperand.appendToFilter.DateArrayOperand.formattedcontainsAny/containsNoneInsertManyRequestscalarInsertManyRequestList<OffsetDateTime>InsertManyRequestOffsetDateTime[]DateUtilGsonTypeAdapterAggregate filters, boost filters and
deleteManyall funnel throughFilter, so they are fixed without separate changes.Filter.DateOperand.toString()is left alone — it feeds the human-readableFilter.toString(), not the wire.Key areas for review
DateUtil.RFC3339— the formatter itself; the fraction handling is the part worth a second lookOffsetDateTime.parse, whose default formatter treats seconds and fraction as optional, so it already accepts both spellings and data written by older clients still loads. This is what makes the change safe to ship without a migration.Testing
The reason no existing test caught this: every date test seeded from
OffsetDateTime.now(), which practically never has second and nano zero. The new tests use literals.Rfc3339DateTest— 26 cases: formatting across minute-boundary/millis/nanos/non-UTC/negative offsets, round-trip through the reader, the old truncated form still parsing, all eight filter comparison paths (includingcreatedAt()/lastUpdatedAt(), which takeOffsetDateTimeonly and so had noStringworkaround), the array operand, and the threeInsertManyRequestshapes.JSONTest— fiveOffsetDateTimerows; the file had none. Each row asserts both directions.DataITest.testDataTypesandSearchITest.test_filterCreateUpdateTimenow use minute-boundary values, so the existing round-trip and filter assertions guard the regression against a real server.Verified the tests fail without the fix — with
src/mainreverted, theJSONTestrows fail withexpected:<"2024-03-01T00:00[:00]Z"> but was:<"2024-03-01T00:00[]Z">.Locally green: 418 unit tests, and
DataITest+SearchITest+OrmITestagainst a container (48 run, 3 skipped by version gates).Breaking changes
None. Public API is unchanged;
DateUtil.toRFC3339is additive, and the wire format only becomes more standards-compliant.Closes #605
🤖 Generated with Claude Code
https://claude.ai/code/session_01WmY5dAGWCccWDoqkKNC2JU