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

Jackson reports wrong locations for JsonEOFException #455

Closed
wastevenson opened this issue Apr 28, 2018 · 13 comments
Closed

Jackson reports wrong locations for JsonEOFException #455

wastevenson opened this issue Apr 28, 2018 · 13 comments
Milestone

Comments

@wastevenson
Copy link

I noticed the the locations reported for JsonEOFException are wrong for both InputStream and Reader based parsers. The values are about 2x higher than they should be. It looks like the problem is that in _loadMore for UTF8StreamJsonParser and in ReaderBasedJsonParser the value for _currInputRowStart is adjusted even if the read fails but _inputPtr is set to 0 only if the read succeeds. When the location is calculated for the exception the count from the last buffer is included in both from _inputPtr and in _currInputRowStart doubling the value.

@cowtowncoder
Copy link
Member

Sounds plausible. Would it be possible to get a simple reproduction with test input stream and/or reader that would exhibit the problem?

@cowtowncoder cowtowncoder added 2.9 need-test-case Needs a test case (unit test or such) to proceed and removed active labels May 22, 2018
@DiggidyDale
Copy link

Is there any update on this, as I am currently having an issue and I can't pinpoint the issue as my JSON string of 173 characters is reporting an EOF issue at column 345

@cowtowncoder
Copy link
Member

cowtowncoder commented Sep 24, 2018

At this pointI will not have time to work on this any time soon. So any progress would likely be from contributions.

But as importantly, please note need-test-case tag: for anyone to work on this, reproduction is needed.

@toddobryan
Copy link

toddobryan commented Sep 5, 2019

With current master, here's a test case that's probably related to this:

    public void testLocation() throws Exception
    {
        JsonParser p = createParserUsingReader("42");
        while (p.nextToken() != null) {
            p.nextToken();
        }
        System.out.println(p.getCurrentLocation());
    }

The results are: [Source: (StringReader); line: 1, column: 9]

Note that that's for a StringReader with 2 characters.

@cowtowncoder
Copy link
Member

Thank you @toddobryan!

@cowtowncoder cowtowncoder added 2.10 and removed need-test-case Needs a test case (unit test or such) to proceed labels Sep 5, 2019
@cowtowncoder
Copy link
Member

Added a failing unit test -- definitely seems to double up counts; I wonder if that is due to special handling of root-level values. Hope to look into this again soon; problem affects both Reader (char-based) and InputStream (byte-based) parsers, via this trivial test.

@toddobryan
Copy link

Sending a pull request that includes tests for getTokenLocation() and getCurrentLocation() at the end of the stream.

@cowtowncoder
Copy link
Member

cowtowncoder commented Sep 5, 2019

Thanks, but I already added tests (based on snippet above) -- but perhaps you could modify what I added, as I only tested getCurrentLocation().

@toddobryan
Copy link

Oh, my PR includes a fix.

@cowtowncoder
Copy link
Member

ahhh! Excellent.

So, just one process thing in that case: unless I have asked for and received CLA, that would be needed before the first contribution. It can be found here:

https://github.com/FasterXML/jackson/blob/master/contributor-agreement.pdf

and the usual mechanism is to print it, fill, sign & scan, email to info at fasterxml dot com.
Only needs to be done once and good from that point on for all Jackson projects.

toddobryan added a commit to toddobryan/jackson-core that referenced this issue Sep 5, 2019
… string at end of input

Fixes the getCurrentLocation() issue by moving changes to _currInputProcessed
and _currInputRowStart inside successful branch. Also includes checks for
getTokenLocation() that make sure correct beginning location of tokens is
reported and that getCurrentLocation() correctly reports the end of
stream when reading is finished.

Tests would be much nicer with parameterized tests from Junit5 since it's
hard to tell which test case fails if one does. That's what the commented
out print statements are for. Let me know if you'd like something different,
but I originally had them separated out and forgot to add methods when I
added to the enum.

Also, the idea for the enum and much of the test data is from
https://github.com/leadpony/jsonp-test-suite, which is also Apache 2
licensed. I had to rework the expected results because the JsonP
getLocation() works very differently. Let me know if you need a
CLA from the author of that project and I'll approach him.
@toddobryan
Copy link

#557

I'll deal with the CLA later today.

@cowtowncoder
Copy link
Member

Thank you!

@cowtowncoder
Copy link
Member

@toddobryan Hi there! Sorry to bother you but I was wondering if you might be able to do this next week? Was hoping to get this in 2.10.0.
Not a big deal if not, will then go in 2.10.1, but it's a good improvement.

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

4 participants