(jsonp,jsr-353) Support embedded binary in Arrays, as already done for Objects - #96
Open
pjfanning wants to merge 2 commits into
Open
(jsonp,jsr-353) Support embedded binary in Arrays, as already done for Objects#96pjfanning wants to merge 2 commits into
pjfanning wants to merge 2 commits into
Conversation
…r Objects
The [issue#5] handling that exposes embedded `byte[]` as Base64 text was only
added to `_deserializeObject()`, never to `_deserializeArray()`. So the same
binary value succeeded or failed purely by position:
{"k": <byte[]>} -> {"k":"AQID"}
[ <byte[]> ] -> MismatchedInputException: ... from Embedded Object value
Add the matching VALUE_EMBEDDED_OBJECT case to the Array path in both modules,
including the fall-through to `handleUnexpectedToken()` for embedded values
that are not `byte[]`.
Root-level embedded values are left alone: `_deserializeScalar()` still reports
them as unexpected, which is a separate question from the Array/Object
inconsistency fixed here.
NOTE: jsr-353 sets `<skipTests>true</skipTests>` in its pom, so only the
jakarta-jsonp test actually executes; the jsr-353 copy is kept so the twin
modules stay in sync.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Member
Author
|
Follow-up: #97 re-enables the jsr-353 suite (it has run zero tests since Jan 2025 due to the RI's clashing So the caveat in the description above ("only the jakarta-jsonp copy actually runs in CI") goes away if #97 lands first. The branches merge cleanly in either order. |
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.
Problem
The [issue#5] handling that exposes embedded
byte[]as Base64 text was added to_deserializeObject()but never to_deserializeArray(). The same binary value therefore succeeds or fails purely by position:Reproduced with
MAPPER.convertValue(Collections.singletonList(bytes), JsonValue.class), and it applies to anyVALUE_EMBEDDED_OBJECTreaching an Array — binary formats,TokenBuffer,convertValue.Fix
Add the matching
VALUE_EMBEDDED_OBJECTcase to the Array path, including the same fall-through tohandleUnexpectedToken()for embedded values that are notbyte[]. Applied to bothjakarta-jsonpandjsr-353so the twin modules do not drift again (cf. #92).Root-level embedded values are deliberately left alone —
_deserializeScalar()still reports them as unexpected, carrying its existing// Not sure what to do with itnote. That is a separate design question from the Array/Object inconsistency fixed here; happy to fold it in if you want root-levelbyte[]to become aJsonStringtoo.Tests
New
EmbeddedBinaryTestin both modules (5 cases): binary as an Object value, as an Array value, nested each way, and withJsonArrayas the declared target.jsr-353/pom.xmlsets<skipTests>true</skipTests>, so only the jakarta-jsonp copy actually runs in CI. I enabled the flag locally to check, and the whole jsr-353 suite errors before reaching any assertion —javax.json.JsonException: Provider org.glassfish.json.JsonProviderImpl not found— which is pre-existing on3.xand unrelated to this change. The jsr-353 test is included so the modules stay in sync if those tests are ever re-enabled; it does compile.jakarta-jsonpsuite: 28 tests, all green.