JSONMappingException Location column number is one line Behind the actual location

[aslamkam]

Hi all,

On Jackson 2.9.8(Also attempted with 2.10.0.pr3), when sending JSON of the type JSON Request.txt:

{
 CustomerType: "A",
 "Status": 123
} 

The CustomerType is not enclosed in quotation marks and raises a JSONMappingException.
The exception raised has its column number one less than the actual location of the syntax error.

For example, in the JSON Request.txt, the column number in exception would be 1, when it actually should be 2.

I've ran similar loads against Jackson 2.2.3 and this error did not happen then.
Typo JSON.txt

{
 "CustomerTpe": "A",
 "Status": 123
}

In TypoJSON.txt, a JSONMappingException is also raised but the line number is accurate.(its 2)

I did a little more digging into the code,
JSONRequest.txt causes JSONParseException before it is converted to JSONMappingException
TypoJSON.txt causes UnRecognizedPropertyException before it is cast into JSONMappingException.

UnRecognizedPropertyException inherits from JSONMappingException while JSONParseException does not.

During the conversion process to JSONMappingException, JSONParseExcpetion's error location is overwritten by the JSONReader's location which is one line behind the actual syntax error.

If this is the intended behavior, please let me know.

[aslamkam]

I'm using ObjectMapper for mapping

[cowtowncoder]

Sounds like a problem, and could be due to not retaining location information. But would it be possible to have a simple unit test reproduction to show the specifics? (based on sample input you included).
I don't doubt this happens but details of handling may vary, easiest to fix and verify fix with a test.

[aslamkam]

I'm recreating the issue right now, I'll upload it to my account by tonight

[aslamkam]

Hi,

The issue has been recreated and is at https://github.com/aslamkam/JacksonIssues

Please note that I'm using a different JSONRequest.txt,
It is named payload.json and is in the TestCase folder of the project.

Recreating the issue let me see that not having quotation marks and being one of the entries of a JsonList were necessary to cause the error.

Let me know if there are some changes to the project that need to be made to make it a much better unit case. Or just point me to the documentation for how to do it.

Also, I know you've said this could be a problem but I need confirmation if this is a bug or isn't.

[cowtowncoder]

I do not have time to dig deep into this so I can not fully verify, but I can say this: if an exception already has valid Location information, that information should NOT get overwritten in general but SHOULD BE retained.

[aslamkam]

Hi,

Below are my findings and mind the formatting as I'm in mobile.

In the file JsonMappingException.java(link at end)
At line 329: method wrapWithPath
We enter the if condition as jsonParseException inherits from jsonProcessingException

Which leads us to line 222: method JsonMappingException

We cast processor as JsonParser and use it's getTokenLocation method to get location information.

My quandary is that the returned value is one line before the actual syntax error location in the JSON request.

I've tried the getCurrentLocation of JsonParser and I recieve accurate information on location of syntax error.

I'm August of 2015, Tatu left a comment on line 215 mentioning the reason for why they use getTokenlocation over getCurrentLocation.

Does this comment's reasoning still hold valid as I've provided a sample project with a testcase where the location information in the returned exception is wrong (one line behind it's actual location)

If so, then this isn't a big and this thread can be closed.

https://github.com/FasterXML/jackson-databind/blob/a6f8d9c5d4c5c85502860957e0c317591c1fd318/src/main/java/com/fasterxml/jackson/databind/JsonMappingException.java

[aslamkam]

Typo: In August of 2015..

The comment on line 215 mentions getCurrentLocation is for low level exceptions, I don't know what exceptions are low level but it seems JsonParseException does not qualify to use it?

Where can I find the low level exceptions of Jackson?
I'll look into that
As well as check the code for JsonParser.

[cowtowncoder]

Low-level exception would refer to problems streaming api (JsonParser) would have when tokenizing content, expanding quoted characters and so on. This would be separate from higher level problems that jackson-databind would report.

Now... while the comment is still probably valid as general comment, I don't think it means behavior is correct. Note that there is different constructor:

public JsonMappingException(Closeable processor, String msg, JsonLocation loc) { }

that would allow passing preferred Location. I think that if there is already a location for exception that triggered/caused another exception, original location should be retained.

So 2 things that would be helpful are:

1. Code that triggers behavior of first throwing parse exception, and one that then gets caught, new exception created
2. (possibly) Unit test around behavior.

[aslamkam]

I'm not sure about 1, is it asking me to create two testcases to trigger two exceptions.
Regardless created UnitTest for this thread at https://github.com/aslamkam/jackson-databind

Haven't made a pull request as I still do not know if this is genuine bug.

[aslamkam]

Could you explain again what is required for analysis? Particularly point 1. Also, I'm considering this a bug because in jackson 2.2, the line number of the syntax error was accurate, while in 2.10, the returned line number is one line behind the location of the actual syntax error.

[cowtowncoder]

Yes, it looks like offset reported is incorrect. It would be good to test location accuracy in 2 places:

1. For jackson-core, for different JsonParsers, to ensure JsonParseException has correct location
2. For jackson-databind, similarly, location should be retained -- this test should probably check that if exceptions are chained (that is getCause() of JsonMappingException points to JsonParseException), Locations are equal. Alternatively if no chaining exists, should just compare to column value hard-coded in test.

So, yes, to test either, exception must be triggered to verify Location included.

[cowtowncoder]

I will also label this as "good first issue" since someone else might be interested in helping get this test and fixed. This is part of an effort to surface more of (relatively) easy problems to new contributors:

https://github.com/FasterXML/jackson/wiki/Issues-For-New-Contributors

[LEgregius]

This issue appears to have occurred in the 2.9.7.
There was this issue fixed :
2128: Location information included twice for some JsonMappingExceptions

This patch f6cf181

Adds

    if (t instanceof JsonProcessingException) {
        return ((JsonProcessingException) t).getOriginalMessage();
    }

Which appears to strip off the some of the information, so for an example error in 2.9.6

{"user":[{"extra":null,"user":"Unexpected character ('u' (code 117)): was expecting double-quote to start field name\n at [Source: (StringReader); line: 4, column: 11]\n at [Source: (StringReader); line: 3, column: 9] (through reference chain: hello.Users["userList"]->java.util.ArrayList[0])"}]}

Turns to

{"user":[{"extra":null,"user":"Unexpected character ('u' (code 117)): was expecting double-quote to start field name\n at [Source: (StringReader); line: 3, column: 9] (through reference chain: hello.Users["userList"]->java.util.ArrayList[0])"}]}

The first error has a chain of messages where one gives the correct error, and outer context. The second just gives the outer context. I would argue that this bug fix actually not the correct fix, and should either be reverted or need to be updated.

[cowtowncoder]

I don't think that patch is wrong, although it may trigger or perhaps unmask the issue.

What patch does is to avoid using getMessage() since that includes Location information in String; and as such would keep chaining multiple locations. That is wrong. It does not, however, change Location -- but what is missing is taking the original location and using that instead of accessing different location. Original behavior would include both locations and perhaps some processing (IDE?) would only display last line, to display the original location?

So, it seems to me that in some constructors both original message and original location need to be used, instead of incorrect combination.

[ronsigal]

Hi @aslamkam, @cowtowncoder,

Has there been any progress on this issue? I ask on behalf of Red Hat's RESTEasy project.

Thanks,
Ron

[cowtowncoder]

@ronsigal Not to my knowledge. I do not have time to look more into this, but I think it should be relatively easy to reproduce and fix, should someone have time.
If so, fix could be added in 2.10.3.

[ronsigal]

Thanks,@cowtowncoder. I'll punt to our next release for now.

[istudens]

@cowtowncoder Hello, I've just filed a PR fixes this issue. It's filed against 2.10 at the moment. I can file upstream PR as well if you are fine with (review) it. Thanks.

[cowtowncoder]

2.10 is perfect: I can just merge up from there.

Thank you for the contribution, hope to review it soon.

[istudens]

A test-case for this issue has been added. The fix for #2128 is not complete as it preserves the original message, but not the original location.

[cowtowncoder]

@istudens Thank you for the fix -- will be in 2.10.3.