Lines 104 and 110- the statusCode variable can be null. When the NullPointerException is thrown from line 110, it has no stack trace and is very difficult to debug.

Comment From: rstoyanchev

If you see the constructor signature, statusCode is not marked @Nullable, and is not expected to be null. That doesn't enforce it of course but at least in our code there shouldn't be a case where null is passed in, and this should be primarily an internally used exception.

Can you provide more details about the circumstances and the code path under which this happens? We could add a check that turns it into an IllegalArgumentException but I'm not sure how much would that help for "very difficult to debug"? I guess I'm not sure what do you mean with "has no stack trace".

Comment From: jimgood

Thanks for responding. I only came across this because the consulting group that built out one of our systems was catching HttpClientErrorException and rethrowing a new instance, for reasons I'll never understand. But they used the 2-argument constructor and unknowingly passing in a null value for getStatusCode by doing this:

} catch (HttpClientErrorException ex) {
    LogUtil.utilExceptionHandling(ex, null, LOGGER, LogLevel.ERROR);
    throw new HttpClientErrorException(ex.getStatusCode(), ex.getResponseBodyAsString());
}

This constructor calls the superclass constructor and that goes up a couple layers before it reaches HttpStatusCodeException, where the NPE happens. So, the Javadoc for HttpClientErrorException doesn't expressly say that passing null values could cause an NPE.

Sorry if that wasn't clear.

And, when running in the debugger, the NPE that was thrown had no stack trace. I don't know what the deal was with that. In order for me to trace this to the line of code in HttpStatusCodeException, I had to step through the debugger until it got to that point.

Comment From: michael-o

We could add a check that turns it into an IllegalArgumentException but I'm not sure how much would that help for "very difficult to debug"? I guess I'm not sure what do you mean with "has no stack trace".

Please don't use IAE for null checks.

Comment From: jimgood

By "has no stack trace" I mean when I stepped through the code in the debugger and got to the point where the exception was thrown, the exception did not contain any stack trace information when looking at its properties through the debugger.

I'm not sure this warrants a code fix. I think the best thing would be to add Javadoc to the subclasses so that people don't make the same mistake. That two argument constructor will result in an NPE if null values are passed.

Comment From: rstoyanchev

Please don't use IAE for null checks.

@michael-o, I'm afraid it's too late for that. Assert.notNot is used throughout the framework. It catches and flags illegal argument values as early as possible.

@jimgood, I'm still missing how the HttpClientErrorException that is being caught and re-thrown could have been created in the first place without causing the same problem?

I think the best thing would be to add Javadoc to the subclasses so that people don't make the same mistake.

If you take a look at the Javadoc for that constructor, its arguments are marked with @Nullable where applicable. This is how we handle it throughout the framework and it also allows IDE's to warn about null issues. When @Nullable was rolled our comprehensively throughout the framework we discovered many issues and refined many areas of the code.