Uh oh!
There was an error while loading. Please reload this page.
JOHNZON-441 - implement currentEvent for RewindableJsonParser and JsonValueParserAdapter - #145
JOHNZON-441 - implement currentEvent for RewindableJsonParser and JsonValueParserAdapter#145jungm wants to merge 2 commits into
Conversation
…nValueParserAdapter
rmannibucau
left a comment
There was a problem hiding this comment.
I'm not sure the changes enable the ticket, looks like they break the contract to me, is it intended?
Also I think we can just drop this rewindablejsonparser now no? did you test?
| private <T> Stream<T> trackStream(final Stream<T> stream) { | ||
| pendingRewind = false; | ||
| last = delegate.currentEvent(); | ||
| final boolean parallel = stream.isParallel(); |
There was a problem hiding this comment.
if parallel nothing works I think
| } | ||
| } | ||
| }; | ||
| return StreamSupport.stream(tracking, parallel).onClose(stream::close); |
There was a problem hiding this comment.
why closing there, the caller must close it
| return jsonValue; | ||
| } | ||
| } | ||
| /* |
There was a problem hiding this comment.
maybe rework so the diff is accurate? (content is ok once done)
| } | ||
| @Override | ||
| public Event next() { |
There was a problem hiding this comment.
this potentially changes the behavior so should be refined I think if desired (looks ok upfront but having a broken test for it can be sane on the long run)
| return delegate.currentEvent(); | ||
| } | ||
| last = delegate.next(); | ||
| pendingRewind = true; |
There was a problem hiding this comment.
looks wrong to impl it there and violates the contract, if I call it on a new parser it should return null, the method is literally getLast() only (our getLast was pre JSON-P 2.1 as a minimum target)
jungm
commented
Aug 26, 2026
yeah you're right, this was all a huge hack because i thought currentEvent must point to the first real event (index 0 instead of -1), hence all of that advancing and masking it code. the tck even tests the other way around https://github.com/jakartaee/jsonp-api/blob/2.1.3-RELEASE/tck/tck-tests/src/main/java/ee/jakarta/tck/jsonp/api/jsonparsertests/ClientTests.java#L1862 So agree, we can probably just drop RewindableJsonParser entirely since its only delegating? |
rmannibucau
commented
Aug 26, 2026
@jungm think it is worth giving a try now we have the required API and wiring was updated |
Fixes https://issues.apache.org/jira/browse/JOHNZON-441