When I was working as a software engineering intern at Salesforce, I did a lot of backend API work with Java and the Spring framework. I had been using Java pretty often since early high school (yeah, I was one of those kids on the robotics team) but working on such a massive codebase was a totally different beast. The level of documentation and test coverage was impressive, but no software is perfect, no matter how smart the people who designed it were.

Tech debt is ultimately unavoidable in any large-scale codebase. Developers have to add features quickly, and sometimes it’s the right decision to take the quick and dirty route over a more perfectionist approach. Still, this debt can accumulate quickly, and if left unchecked will ultimately lead to performance issues and bugs down the road.

A New Endpoint Parameter

At this point I had picked up a pretty good habit of always adding new tests for any additional code I wrote, and here it finally paid off.

We had an API endpoint that took a list of ContactIDs as its only parameter, and I was adding an optional DateTime parameter to this endpoint for a new feature that was being written. The result was only four new lines of controller logic, but I decided to add a couple new tests for this anyway. I noticed there wasn’t a test for malformed ContactIDs (null and empty lists were covered) so I threw one of those in there too.

I was pretty surprised when this test failed with a Null Pointer Exception instead of returning the proper error message to the caller. Since the ContactID definitely wasn’t null, I figured something weird must be going on to be giving me the NPE. The stack trace was originating deep in some unfamiliar chunk of business logic, so I decided to open a new ticket and investigate it myself since I was already working in that context.

Success & Failure Responses

The API endpoint in question handled a certain action that could be performed on a selection of contacts. Sometimes, this action would succeed for certain contacts, but fail for others, so we had a custom error response class to keep track of this. This response would be used to determine whether the UI should display a success message, an error message, or both.

Because of this, our error response class only stored 1) the error/success messages to be displayed, and 2) the number of error/success messages to display. The constructor looked something like this:

public ErrorResponse(int successCount, int errorCount, String errorMsg);

We also had another (overloaded) constructor to specify a custom success message, if the default message didn’t cut it:

public ErrorResponse(int successCount, int errorCount, String successMsg, String errorMsg);

So, for example, code returning both a success and an error might do something like:

return new ErrorResponse(1, 1, "Great!", "Uh oh...");

Then, other parts of the code would use the response like so:

if (resp.getSuccessCount() > 0) {
    resp.getSuccessMsg();
}

Everything worked fine this way, until…

We Added Warning Messages

This ErrorResponse class was used by a number of different API endpoints, and over time it had experienced some growing pains. At some point we needed to support warning messages too, so the constructors were refactored to look like this:

public ErrorResponse(int successCount, int errorCount, String warningMsg, String errorMsg);

public ErrorResponse(int successCount, int errorCount, String successMsg, String warningMsg, String errorMsg);

When this refactoring was done, it must have been done manually (pro tip: your IDE probably has a way to automatically refactor constructors!) and they forgot to update a couple instances to match the new signatures.

If you recall our example use case above, the following is now assigning a warning message and leaving the success message blank, though the successCount field would indicate otherwise:

return new ErrorResponse(1, 1, "Great!", "Uh oh...");

The response handler would see a non-zero value for successCount, try to read the custom success message field, and… BOOM, Null Pointer Exception!

Killing the Bug

The immediate solution was to manually review all of these constructor usages to make sure the count fields and the message fields agreed with one another. The ErrorResponse class also wasn’t following the “trust, but verity” mantra very well: beyond just checking that successCount or errorCount is greater than zero, I also added some extra logic to verify that the messages weren’t null before trying to read them.

Finally, I filed a ticket to rewrite the entire ErrorResponse class to better suit its new purpose. Initially the class was only used to display default success & error messages, hence the successCount/errorCount fields. Over time, business needs changed, so additional fields like the custom message text fields were added. Having both didn’t really make sense, but at the time it was a lot faster to just add a new overloaded constructor than to rewrite all of the old code. We created a new class like Response(String message, ReponseType type) and then simply returned a list of these from the controller.