Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

JsonParserSequence skips a token on a switched Parser. #296

Closed
newkek opened this issue Jun 30, 2016 · 4 comments
Closed

JsonParserSequence skips a token on a switched Parser. #296

newkek opened this issue Jun 30, 2016 · 4 comments
Milestone

Comments

@newkek
Copy link

newkek commented Jun 30, 2016

Having 2 parsers concatenated with JsonParserSequence.createFlattened(parser1, parser2).

If the second parser is on a token that is not null and should not be skipped, the JsonParserSequence will still skip it. JsonParserSequence's nextToken() calls nextToken() on the new delegate which may cause that we miss a token.

For more details : forum question

I'll open a PR for this.
Thanks.

@cowtowncoder
Copy link
Member

Implemented, added simple test; would be great if you could verify that it works for you before 2.8.0 is released.

@cowtowncoder
Copy link
Member

Hmmh. Actually, trying to change the behavior breaks a few tests in jackson-databind.
Not sure how safe the change is for usage; or whether it actually is even the right thing to do.
I will have to thing about this, and potentially just revert the change unless I can think of reasonable heuristic. Or perhaps just add different constructor for alternate behavior, defaulting to old behavior with existing construction.

@cowtowncoder cowtowncoder reopened this Jul 3, 2016
cowtowncoder added a commit that referenced this issue Jul 4, 2016
@newkek
Copy link
Author

newkek commented Jul 5, 2016

The constructor and createFlattened() with the boolean in argument sound good to me. I ran the tests you wrote for this and they pass for me so I guess it must be ok. Btw I had opened #297 for this so I guess you can close it...

Thanks.

@cowtowncoder
Copy link
Member

New functionality is now in 2.8.0 -- thank you for working through this, let me know if I mangled something.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants