[DT-4058] Update NA exception signature - #3046
Conversation
A downstream 401 whose status line omits the HTTP reason phrase leaves the
status message null. The previous single-argument call bound that null to the
NotAuthorizedException challenge parameter, which threw
NullPointerException("Primary challenge parameter must not be null.") instead.
Callers that catch NotAuthorizedException could not handle it, so the NPE
reached the resource layer and became a 500.
The second test pins the message. A lone String argument binds to challenge,
not to message, so the old code also dropped the downstream reason phrase from
the exception message.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
🟢 Approval recommended
The change is small, directly addresses the reported failure mode, and includes focused tests that would fail under the original NPE behavior.
Pull request overview
Fixes a production-only failure mode where a downstream 401 without an HTTP reason phrase causes HttpClientUtil.handleHttpRequest to throw an unintended NullPointerException, leading to a 500 for authenticated GET /api/user/me in BEEs. The change updates the unauthorized exception construction and adds targeted unit coverage to prevent regressions.
Changes:
- Update
HttpClientUtil.handleHttpRequestto avoid null propagation when translating downstream 401 responses intoNotAuthorizedException. - Refactor existing caching tests to use
assertDoesNotThrowrather than manual try/catch +fail. - Add unit tests covering 401 responses with and without an HTTP reason phrase.
File summaries
| File | Description |
|---|---|
| src/main/java/org/broadinstitute/consent/http/util/HttpClientUtil.java | Adjusts 401 handling to prevent NPE when the downstream status message is null. |
| src/test/java/org/broadinstitute/consent/http/util/HttpClientUtilTest.java | Improves existing tests and adds coverage for 401 responses lacking a reason phrase. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
Addresses
https://broadworkbench.atlassian.net/browse/DT-4058
Summary
Authenticated
GET /api/user/mecalls return a 500 in BEEs:{"message":"Primary challenge parameter must not be null.","code":500}The endpoint works in dev, staging, and prod.
Why only BEEs failed
ECM answers 401 when the user has not accepted the Terms of Service.
NihService.syncAccountexpects this and catchesNotAuthorizedException. Itcannot catch an NPE, so the NPE reaches
Resource.createExceptionResponse,which has no handler for it and returns a 500.
Tomcat omits the reason phrase by default, so ECM sends
HTTP/1.1 401with anempty phrase. The two environments then differ:
Unauthorized. Consent never sees a null.Testing
I have manually tested in a BEE. Steps to reproduce:
GET /api/user/meand correctly see a 404POST /api/userto create an entry for your authenticated accountGET /api/user/meand see your user information.Have you read CONTRIBUTING.md lately? If not, do that first.