more JsonParser methods should declare throws
pjfanning opened this issue · 7 comments
I'm suggesting this just for master branch (Jackson 3).
See https://github.com/FasterXML/jackson-dataformat-xml/pull/660/files
Both isExpectedNumberIntToken and isExpectedStartArrayToken should be declared to throw StreamConstraintsException or a super class like JacksonException.
Yes and no: yes, they definitely should have included checked IOException in 2.x.
My mistake, but unfortunately can't change (which you did not suggest I know, just mentioning for readers' benefit)
But in 3.x, base JacksonException (and thereby all subtypes) are unchecked RuntimeExceptions. So the only reason to add throws clauses is documentation.
Change was via:
https://github.com/FasterXML/jackson-future-ideas/wiki/JSTEP-4
I'm surprised that we've gone down the route of using RuntimeExceptions. Unfortunately, we have 'security' researchers who go around complaining when Exceptions are not checked (i.e do not extend RuntimeException and therefore require explicit handling). Basically, the expectation is that you don't have to read javadocs and the compiler catches when you don't handle the possible exceptions.
Originally I was opinion checked exceptions are preferable (which is why Jackson 1.x - 2.x have it), but the thing that really changed the situation is Java 8+ streams and functional programming -- checked exceptions become a royal pain as there is no good way to compose them in Java (essentially there's need to catch-and-rethrow-as-unchecked all over the place).
And functional style has become much more prevalent, especially for non-blocking processing, so this problem has become more and more annoying over time.
I tried to write about that in JSTEP-4 and hoped it was a known change.
Now... I wish there was a way to handle documentation aspect better (to make it easier to ensure throws clauses cover cases), but it is what it is and I think this is better trade-off.
Perhaps we should try to decorate all public methods of JsonParser, JsonGenerator, ObjectMapper/-Reader/-Writer with "throws JacksonException"?
(then again, ideally only subtypes of those are thrown, but figuring that out becomes impractical -- so perhaps the common subtype makes most sense)
As to catching exceptions: at least Jackson exception type hierarchy is simple enough with JacksonException being the lowest level Jackson-thrown exception (for both 2.x and 3.0, although less used in 2.x being added in 2.12), all other types extending from it.
And 3.0 does not thrown/pass IOExceptions (they get now wrapped).
I wasn't aware of security researchers' bringing up this issue... I can sort of see concern they might have, but realistically since unchecked exceptions (and Errors) exist regardless (thrown by JDK in some cases), it seems there are lots of ways unexpected exceptions can get through control flow in interesting ways.
... not to even mention "sneaky throws" style which allows forcing throwing of checked exceptions (used by Lombok but possible without it too).
I can see both sides of the argument.
I code mainly in Scala and it ignores Exceptions - you can choose to handle them or you can choose to ignore them and let them bubble up. Functional Programming purists will argue that you shouldn't throw exceptions anyway and that the return type should be a type that exposes all the possible result types including failure types.
I guess if we document how the JacksonException based runtime exception works, we can use that to close off any issues that are raised about us having no checked exceptions.
Right, exceptions have gone out of fashion for now. But there are trade-offs with all the alternatives as well and purists rarely propose particularly practical solutions.
At any rate, I think documentation is the way to go with 3.0. There's a bit of renaming but hierarchies are mostly unchanged. Except for IOException as base type was changed to RuntimeException
@cowtowncoder I hope I don't hijack the issue, but I'm using ObjectMapper and I noticed that JsonProcessingException is a checked exception.
Looking at the codebase, it seems this change has already been done 491b6d9/#661, but not yet released. Since it has been some time already, I'd like to ask (out of curiosity), is there an expectation when the version 3 is going to be released?
@renannprado There is no firm deadline, but one big blocker for me personally was that I wanted to get POJO Property introspection rewrite done -- and that kept on moving across releases. But it is finally done for 2.18. So I think there should finally be focus again on getting 3.0 ready for release.
I am thinking of the first release candidate getting out before end of 2024.
FWTW, there is a loose set of bigger things listed under JSTEP page:
https://github.com/FasterXML/jackson-future-ideas/wiki/JSTEP
Discussions for Jackson 3 planning should probably be done on jackson-dev Google group.