Java Deep

Pure Java, what else

Don not hit sysadmins with NPE!

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.

About these ads

4 responses to “Don not hit sysadmins with NPE!

  1. Martin Grajcar August 15, 2014 at 12:09 am

    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();

    Like

  2. Bo August 15, 2014 at 4:22 am

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

    Like

  3. JackFr August 15, 2014 at 5:34 pm

    moral != morale
    caching != catching

    Like

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s

Follow

Get every new post delivered to your Inbox.

Join 935 other followers

%d bloggers like this: