Issue #13670 - Returning ISE in all getParameter use cases outlined by Servlet 6.1 spec #13678
Conversation
…y Servlet 6.1 spec
joakime
added
Bug
| catch (Throwable t) | ||
| { | ||
| // Per Servlet https://github.com/jakartaee/servlet/issues/431 this should only | ||
| // ever throw an IllegalStateException | ||
| throw new IllegalStateException(t); | ||
| } |
There was a problem hiding this comment.
I'm not sure I agree with this as the whole point of uncheck throwables is that that they do not need to be declared. The contract for getParameter has declared ISE as a politeness not as a binding contract. Specifically, I would not expect throwables like OutOfMemoryError or ClassFormatError to be caught and wrapped as an ISE.
The javadoc of this method specifically allows for other handling of exceptional cases:
Containers may provide container specific options to handle some or all of these errors in an alternative manner that may include not throwing an exception.
So I read this as allowing other exception to be thrown .
The whole point of BadMessageException being a RuntimeException, is that it is unlikely to be caught by an application and flow back to Jetty, where it can be handled correctly.
There was a problem hiding this comment.
At best I could be convinced to catch(RuntimeException| Exception) and wrap as an ISE.... or better yet a custom list of exceptions that we know we throw if we detect the kinds of conditions the javadoc mentions that should be ISEs.
Even then, I think we should have a compliance mode to determine if we wrap unchecked exceptions with ISE or not.
There was a problem hiding this comment.
See the conversation ..
The purpose of "Containers may provide container specific options to handle some or all of these errors in an alternative manner that may include not throwing an exception." is to allow optional "container configuration" to change behavior away from the servlet spec.
We don't have such a configuration right now.
So we default to Servlet Spec behavior. (what this PR is doing)
If we want to honor the "container specific options" then we need a boolean like boolean containerSpecificGetParameterBehavior = false; which defaults to servlet spec behavior.
Allowing users to turn on the "container configuration" for our behavior.
There was a problem hiding this comment.
@gregw I can add a a configuration to ServletContextHandler to allow setting this behavior (as the spec states).
There was a problem hiding this comment.
At best I could be convinced to
catch(RuntimeException| Exception)and wrap as an ISE.... or better yet a custom list of exceptions that we know we throw if we detect the kinds of conditions the javadoc mentions that should be ISEs.
The javadoc uses the language "@throws IllegalStateException if parameter parsing is triggered and a problem is encountered parsing the parameters including, but not limited to: invalid percent (%nn) encoding; ..."
That's not a specific list of failures, but all failures, there's no specific list, no limiting to specific conditions, it's all conditions that would cause a failure in these getParameter*() methods.
Even then, I think we should have a compliance mode to determine if we wrap unchecked exceptions with ISE or not.
That would introduce a new ServletCompliance mode.
But I feel that this isn't needed, as the javadoc is unambiguous in this case.
There was a problem hiding this comment.
Added an exclusion for java.lang.VirtualMachineError and java.lang.LinkageError, but any larger exclusion list is just invalid IMO (this includes RuntimeException and Throwable which have many recoverable non-fatal exceptions already)
As outlined in Servlet 6.1 (EE11) Spec, the getParameter*() methods should only ever return ISE.
See details in ...