Fix #1310: Count JSON tokens #1296
Conversation
src/main/java/com/fasterxml/jackson/core/json/async/NonBlockingJsonParserBase.java
Outdated
Show resolved
Hide resolved
| @@ -467,7 +467,7 @@ private final JsonToken _finishBOM(int bytesHandled) throws IOException | |||
| } | |||
| _pending32 = bytesHandled; | |||
| _minorState = MINOR_ROOT_BOM; | |||
| return (_currToken = JsonToken.NOT_AVAILABLE); | |||
| return _updateToken(JsonToken.NOT_AVAILABLE); | |||
There was a problem hiding this comment.
This would work incorrectly since it can happen multiple times in a row (f.ex when skipping over white-space separating tokens); should only update once token type is determined.
So maybe third method _updateTokenNA() or such, similar to _updateTokenToNull()?
pjfanning
force-pushed
the
token-count
branch
2 times, most recently
from
08d5d31 to
5f1fa6e
Compare
| @@ -782,6 +795,22 @@ protected final void _wrapError(String msg, Throwable t) throws JsonParseExcepti | |||
| throw _constructReadException(msg, t); | |||
| } | |||
|
|
|||
| protected final JsonToken _updateToken(final JsonToken token) throws StreamConstraintsException { | |||
| _currToken = token; | |||
| if (streamReadConstraints().hasMaxTokenCount() && token != JsonToken.NOT_AVAILABLE) { | |||
There was a problem hiding this comment.
@cowtowncoder I'm doing the hasMaxTokenCount check here to try to minimise the impact of this code when hasMaxTokenCount=false. Testing for hasMaxTokenCount in the validateTokenCount will mean that we do a fair amount of unnecessary work if hasMaxTokenCount=false.
This change needs more testing and microbenchmarking but this is what I think is the best initial strategy.
There was a problem hiding this comment.
As a further optimization, I think we should just have
protected final boolean _hasTokenCountLimit;
initialized on parser being constructed. But need to see how to do that given that for now StreamReadConstraints are not properly passed to ParserMinimalBase. I can probably change code to pass it tho. Will create an issue and try to get that done.
I think we can also remove JsonToken.NOT_AVAILABLE check: there's the other method to call and only non-blocking/Async parsers (JSON, Smile) ever set that token, nothing else.
more changes don't increment token count for NOT_AVAILABLE handle case when token is set to null use _updateTokenToNull null check not needed now add test Update TokenCountTest.java Update StreamReadConstraints.java add validation try to reduce overhead for when maxTokenCount is not needed Update StreamReadConstraints.java Update TokenCountTest.java
|
Created #1310 as the issue covering this PR. I think implementation makes sense, as-is. So that's fine. But merging to master/3.0 may be challenging, so what I think I will do is create another PR for a subset of this one, to handle parts that are likely to cause most problems. That can be merged in 2.18, then master, and then sync pr. |
|
@pjfanning Ok I think this is good: I merged update-token parts via separate PR into 2.18, this PR & master and remaining diffs are bit smaller and hopefully merge with few conflicts. |
|
@pjfanning Is it ok to merge this? (only asking since it's still a Draft) |
|
@cowtowncoder I've marked this as ready for review |
cowtowncoder
added
the
processing-limits
Idea is to be able to apply a StreamReadConstraint that limits total JSON tokens that you can have in an input doc.
See FasterXML/jackson#231 (reply in thread)