fix(metadata): close the DATE-array storage gap left by #275 - #278
Merged
Conversation
Two root defects in the JSON array-write storage path, deliberately left unfixed by the #275 batch as out-of-scope scope-creep triggers: - MetaField.setObject(Object,Object) converted via the field's SCALAR getDataType() instead of the array-aware getEffectiveDataType(), so an isArray field's List value was corrupted (comma-joined / bracketed toString()) before setObjectAttribute's own instanceof check rejected it. Broke setBoolean/setInt/setLong/setDouble/setStringArray, and every MetaObjectDeserializer array-read branch that routes through it. - DataConverter had no DATE_ARRAY case (the commented-out `//toDateArray(val)` fragment), so no entry point could store a List<Date> on an isArray field.date/field.timestamp field. Fix: getEffectiveDataType() in MetaField.setObject (a strict no-op for every non-array field); a new DataConverter.toDateArray mirroring the sibling toLongArray/toBooleanArray shape, wired into case DATE_ARRAY. BYTE_ARRAY/SHORT_ARRAY stay on the unsupported arm (field.byte/field.short are non-functional stubs). Also closes the deferred findings this unblocked: MetaObjectSerializer's DATE-array element loop now converts via DataConverter.toDate(o) instead of a hard cast; field.timestamp (incl. @localTime) array coverage; a full-Gson-pipeline round trip for the previously-untestable DATE-array read path in MetaObjectDeserializer; null-array-itself pins extended to every touched type; and a stale MetaObjectDeserializer comment describing the DATE-array branch as blocked, now rewritten to match reality. Blast-radius checked: every MetaField.setObject caller either passes a scalar value (no-op under the fix) or already routed around the array bug via setObjectArray/setValue; no caller depended on the corrupting conversion succeeding. Refs #275 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015TqsuDye2SfXGf43vuoD3n
…ld widening (#275) Review fix round 1 on the #275 DATE-array storage gap fix. Important #1: MetaObjectDeserializer's readDateElement fell through to el.getAsString() for a JsonNull element (JsonNull is not a JsonPrimitive), which throws UnsupportedOperationException naming no field -- the same bare-throw shape C1 eliminated on the write side. Reachable by design: MetaObjectSerializer deliberately emits JsonNull.INSTANCE at a null element position (pinned by an existing test), so the serializer's own output could not be read back by its sibling deserializer. Fixed with an isJsonNull() guard as the first line of readDateElement, matching what write already emits; added a full-pipeline null-element round-trip test; corrected the readFieldValue DATE-case comment that (falsely) claimed the branch already round-tripped end to end. Important #2: the report's blast-radius argument conflated "no-op for a non-array field" with "no-op for every setObject call site" -- getEffectiveDataType() == getDataType() is a property of the field, not the value. An array-typed field receiving a scalar JSON value (live in MetaObjectDeserializer's own else-arms) genuinely changed from a loud InvalidValueException to a silent single-element wrap (or, for STRING, a comma-split) -- verified both directions by temporarily reverting the E2 fix and confirming the old exception. Not a new rule: it converges with DataObjectBase._setObjectAttribute's pre-existing effective-type conversion, which is what produced this task's own E1 RED evidence in the first place. Added six pinning tests and corrected the report's claim in place. Refs #275 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015TqsuDye2SfXGf43vuoD3n
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.
Closes the carry-forward the #275 batch deliberately left unfixed. That batch fixed the serializer's write side for
@isArrayfields but stopped at its bounded-scope clause, which named these exact files as scope-creep triggers.Two root defects
MetaField.setObjectconverted via the field's SCALAR type.getDataType()on an@isArrayfield ran aListthrough a scalar converter — comma-joining a STRING array, bracketed-toString()for the rest — andsetObjectAttributethen rejected the corrupted scalar. SosetBoolean/setInt/setLong/setDouble/setStringArraywere all broken for array fields, andMetaObjectDeserializer's own array-read branches threw. Now usesgetEffectiveDataType(), which is provably a no-op for every non-array field.DataConverterhad noDATE_ARRAYimplementation (case DATE_ARRAY:→unsupported(), with a commented-out call showing it was anticipated and never built). AddstoDateArray, mirroring its sibling converters exactly.BYTE_ARRAY/SHORT_ARRAYdeliberately stay unsupported —field.byte/field.shortwere cut from the metamodel as non-functional stubs.Also closed
The two
_blockedByPreexistingSetterBugpins become real round-trips, plus five findings deferred only because this gap made them untestable: the per-element hard cast,field.timestamparray coverage, the real-Gson-pipeline exercise, null-array breadth, and the DATE-array read path.Review caught a genuine asymmetry: the serializer emits
JsonNullat a null element position — pinned by its own test — while the deserializer threw reading it back. Fixed with a guard placed before theisJsonPrimitivecheck (JsonNullis not a primitive), proven by a full-pipeline round-trip.Also pins a deliberate widening this change introduces: an array-typed field receiving a scalar value previously threw and now converts (comma-splitting
"a,b"→["a","b"]). That convergessetObjectwith the primary storage path, which already converted against the effective type.Verification
metadata1347/1347 ·omdb,om63/63 · two independent reviews, one fix round, re-reviewed clean.