My opinion is that having a null pointer exception and getting it into the log without catching, handling and re-throwing it in another exception is not inherently bad. If we can do nothing better then it should not be a problem. The thing is that in practice almost always there is a better way to handle the situation.

Recently I was pair programming and we debugged some web code. We could not get through the authentication filter on the development environment and the authentication was not really in scope for the debugging so we decided to switch that totally off for the time. The next thing was an NPE. We looked at the code and we saw at the line something like

principal = SecurityContextHolder.getContext().getAuthentication().getPrincipal();

It was obvious: since the authentication was switched off the result of getAuthentication() was null, and therefore calling getPrincipal() on null caused the NPE. Should we modify the code to check if there is authentication information and throw a different exception? What would be the benefit to do that? The code runs slower, gets bigger (and bigger code is harder to maintain). And the NPE and the code source together are obvious. There is no need to change.

On second thought the NPE may not be that obvious for a support guy operating the code somewhere at the other side of this rotating ball. He may not have handy access to the source code and may not understand easily that the root cause for the NPE was the misconfiguration of the authentication layer. He/she has to start the server, it does not work and calls the support, raises a ticket. You as a responsible developer being at the farthest end of the support line may woke up during your finest sleep just to tell him/her that the authentication layer was misconfigured. Than you regret that you could have told it in the logs and in the exception.

Authentication auth = SecurityContextHolder.getContext().getAuthentication();
if( auth == null ){
  throw new BadConfigurationException("The authentication layer is not properly configured SecurityContextHolder.getContext().getAuthentication() returned null."
}
principal = auth.getPrincipal();

Not that big deal and pays back on the long run.

Moral: What seems to be obvious for the developer during development may not be that easy for the system administrator. Admins are not familiar with the code, may not have access to source code, have less experience in programming. On the other hand they have great experience setting up and running systems. It is a hard work, they deserve proper log messages and talkative exceptions.

Comments imported from Wordpress

Martin Grajcar 2014-08-15 00:09:56

I’d say that throwing an NPE is perfectly fine, assuming it gets a message:

Authentication auth = SecurityContextHolder.getContext().getAuthentication(); Preconditions.checkNotNull(auth, "The authentication layer is not properly configured SecurityContextHolder.getContext().getAuthentication() returned null."); principal = auth.getPrincipal();

Bo 2014-08-15 04:22:05

A developer that ignores valid returns (which null is on getAuthentication()), deserves to be woken at night :D

JackFr 2014-08-15 17:34:44

moral != morale caching != catching

Peter Verhas 2014-08-15 18:22:23

Thanks. I corrected and I learnt one new English word. The other one was typo that the spellchecker did not cache :-)

Michael Jacob 2015-09-23 00:41:44

Sometimes those null checks really get into your way. Either because they’ll add 18 indentation levels to your code, or because they’ll make it hard to read, or you might be in a performance critical path (yes, even with Java that can happen).

In that case you can wrap your code in a try block, catch that NPE and then check what was null to produce a nice error message. But make sure your half-finished work doesn’t have any unwanted outside effect!

BTW: I also have one of those checkNotNull() helper methods, but mine is, generics by thanked, inlinable. Very helpful every time Eclipse decides to force me to check enum.values() for null elements and such nonsense. Or thinks that a library’s "public static final Foo foo = new Foo();" needs to be null-checked…

Comments

Please leave your comments using Disqus, or just press one of the happy faces. If for any reason you do not want to leave a comment here, you can still create a Github ticket.