Include location in NumberFormatException thrown by JsonTreeReader - #3062
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
eamonnmcmanus
left a comment
There was a problem hiding this comment.
Thanks for helping improve consistency here!
| try { | ||
| result = primitive.getAsDouble(); | ||
| } catch (NumberFormatException e) { | ||
| NumberFormatException rethrown = |
There was a problem hiding this comment.
It's annoying that NumberFormatException doesn't have a constructor with a cause parameter!
Given that the code pattern here occurs three times, I think it would make sense to introduce a helper method, so you could write this:
throw numberFormatException("Expected a double " ..., e);|
Done — introduced a |
289b4a8 to
1be033b
Compare
Signed-off-by: dong0713 <417851790@qq.com>
8974bff to
6cce44d
Compare
Signed-off-by: Bowang <417851790@qq.com>
Problem
JsonReader.nextDouble(),nextLong()andnextInt()already include the JSON path in theNumberFormatExceptionthrown when a value cannot be parsed. This was added in #3000 as a fix for #1564.However, the corresponding methods in
JsonTreeReaderwere missing this handling. As a result, the two public deserialization entry points behave inconsistently:Both paths consume untrusted JSON and report parse failures to the caller, so they should report the same location information.
Fix
Mirror the existing
JsonReaderhandling: wrap theJsonPrimitiveconversion innextDouble(),nextLong()andnextInt()with a try-catch, and rethrow aNumberFormatExceptioncontaining the offending value andlocationString(), preserving the original exception as the cause (viainitCause(e)), exactly as the streamingJsonReaderdoes.Tests
Added three regression tests in
JsonTreeReaderTest(testNextDouble/NextInt/NextLongNumberFormatExceptionContainsLocation), asserting the message contains the JSON path (path $.x/path $.y), paralleling the existing tests added forJsonReaderinJsonReaderTest.Full test suite:
Tests run: 4607, Failures: 0, Errors: 0, Skipped: 20— no regressions.Notes
JsonTreeReaderoverrides the sameJsonReadermethods, so the omission is a direct symmetric gap.Gson.fromJsononly catchesEOFException/IllegalStateException/IOException, so the bareNumberFormatExceptionfrom the tree path reaches the caller unwrapped, making the missing location directly user-visible.