Fix some adapter exceptions not including JSON document location - #3096
eamonnmcmanus merged 2 commits into
Conversation
0722abe to
666ab6d
Compare
| if (map.containsKey(key)) { | ||
| throw new JsonSyntaxException("duplicate key: " + key); | ||
| throw new JsonSyntaxException( | ||
| "Duplicate key '" + key + "'; at path " + in.getPreviousPath()); |
There was a problem hiding this comment.
Also changed message to start with capital letter to be consistent with the other exception messages
| throw new JsonSyntaxException( | ||
| "Duplicate key '" + key + "'; at path " + in.getPreviousPath()); | ||
| } | ||
| V value = valueTypeAdapter.read(in); |
There was a problem hiding this comment.
Moved reading of the value down so that getPreviousPath() is more accurate (pointing to the key instead of the value)
| String nextString = in.nextString(); | ||
| if (nextString.equals("null")) { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
Moved this out of the try-catch (URISyntaxException)
| throw new JsonIOException(e); | ||
| throw new JsonSyntaxException( | ||
| "Failed parsing '" + nextString + "' as URI; at path " + in.getPreviousPath(), e); | ||
| } |
There was a problem hiding this comment.
Changed this from throwing JsonIOException to JsonSyntaxException for consistency; this was the only case where JsonIOException was thrown in this class.
(Technically this is a behavior change, though it is probably unlikely that any user relies on this exact exception.)
| } | ||
| try { | ||
| return new URL(nextString); | ||
| } catch (MalformedURLException e) { |
There was a problem hiding this comment.
Added handling for (unchecked1) MalformedURLException, for better exception message and for consistency with URI handling (though there the exception is a checked one)
Footnotes
-
Edit: Actually a checked exception but a subclass of
IOExceptionand therefore needed notry-catchbecause the enclosingreadmethod already hasthrows IOException. ↩
eamonnmcmanus
left a comment
eamonnmcmanus
left a comment
There was a problem hiding this comment.
Thanks, this looks great!
I'm guessing that some people will have tests that need to be updated because of these changes, but those tests are fragile anyway. I'm guessing that that includes some of Google's internal tests, which I can easily update.
2 participants
Purpose
Improve exceptions & exception messages
Description
Some of the exceptions thrown by built-in adapters did not include the JSON document location, making it difficult for the user to troubleshoot.