Well - we either need to throw an exception in WebUtils or modify ALL
code that references it to throw an exception. Since NO code that
references it accounts for the fact that it might return null, I
thought it was appropriate that they throw an exception if it isn't
defined - and I updated the JavaDoc to reflect.
I'm fine adding the required methods or renaming the existing one, but
NO current code would reference the other methods so I don't really
see much value at this point. We could just rename them if that makes
you feel better, but the JavaDoc is pretty explicit as to the behavior
and I can't think of why we'd want to call the unrequired methods
given our architectural patterns (subclassing for web versions of
things).
This came about on my end because our integration tests were calling
those methods and receiving NPEs which require you to dig through
JSecurity source code to figure out why they are occurring. I like
the fact that the new error message is descriptive and consistent vs.
a NPE when things are setup wrong.
On Aug 14, 2008, at 1:30 PM, Les Hazlewood wrote:
This is a quick question for Jeremy, but anyone please feel free to
chime in
if you want.
I noticed that WebUtils.getServletRequest and getServletResponse were
changed to throw an exception if either weren't bound to the
ThreadContext.
I'm not quite sure I feel this is totally appropriate. I see those
methods
as convenient web-utility wrappers around the ThreadContext only,
which
itself does not throw exceptions when something doesn't exist.
What do you think of adding a WebUtils.getRequiredServletRequest and
getRequiredServletResponse that do what you expect (throw an
exception), and
the regular get* methods don't?
This seems a tad more in line with how the ThreadContext works to
me, with
the added benefit of being more self documenting as well. Would
that be ok?
