Skip to content

Commit df3becd

Browse files
add NPE validation to RequestHeaders
1 parent c4b350b commit df3becd

5 files changed

Lines changed: 122 additions & 1 deletion

File tree

CHANGELOG.md

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
1616
literal value `abc # prod`, which passes `validateApiKey` (printable ASCII)
1717
and surfaces later as a confusing `AuthenticationError` far from the .env
1818
source that caused it.
19+
- `RequestHeaders` canonical constructor now rejects a `null` `headers` map
20+
with a clear `NullPointerException` naming the field, replacing the bare
21+
`Map.copyOf(null)` NPE that left consumers hunting for the offending
22+
argument. The wire-format deserializer additionally intercepts a top-level
23+
JSON `null` body via `JsonDeserializer#getNullValue` and surfaces it as a
24+
`ParseError` carrying the endpoint URL, status, and request id — preventing
25+
a malformed `/headers/` response from manifesting as an opaque NPE further
26+
down the call stack.
1927

2028
### Added
2129
- Project scaffold per ADRs 001–007: Gradle Kotlin DSL build, JDK 17 toolchain,

src/main/java/com/marketdata/sdk/RequestHeadersDeserializer.java

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
import com.fasterxml.jackson.core.type.TypeReference;
55
import com.fasterxml.jackson.databind.DeserializationContext;
66
import com.fasterxml.jackson.databind.JsonDeserializer;
7+
import com.fasterxml.jackson.databind.JsonMappingException;
78
import com.marketdata.sdk.utilities.RequestHeaders;
89
import java.io.IOException;
910
import java.util.Map;
@@ -14,6 +15,14 @@
1415
* record at the Jackson layer rather than via an annotation on the record (per ADR-007: response
1516
* records don't carry {@code @JsonDeserialize}; deserializers register programmatically on the
1617
* parser's {@code ObjectMapper}).
18+
*
19+
* <p>A literal JSON {@code null} body — or any other path that would leave the parser holding a
20+
* {@code null} map — is short-circuited to a {@link JsonMappingException} so {@link
21+
* JsonResponseParser} surfaces it as a {@link com.marketdata.sdk.exception.ParseError} with the
22+
* request's URL/status/id attached. Without the guard, Jackson would return {@code null} from the
23+
* top-level {@code readValue} (via {@link #getNullValue}, its standard null-routing seam) and the
24+
* NPE would surface much later — uncaught by the parser's {@code catch (IOException)} and far less
25+
* useful to a consumer trying to diagnose a malformed response.
1726
*/
1827
final class RequestHeadersDeserializer extends JsonDeserializer<RequestHeaders> {
1928

@@ -22,6 +31,25 @@ final class RequestHeadersDeserializer extends JsonDeserializer<RequestHeaders>
2231
@Override
2332
public RequestHeaders deserialize(JsonParser p, DeserializationContext ctxt) throws IOException {
2433
Map<String, String> raw = p.readValueAs(MAP_OF_STRINGS);
34+
if (raw == null) {
35+
// Defense in depth: a top-level JSON null is intercepted by getNullValue(ctxt) below before
36+
// deserialize() is ever called, so this branch is reachable only via a pathological future
37+
// Jackson behavior. Better to fail with a clean JsonMappingException than to let the null
38+
// reach the record's requireNonNull and bypass the parser's IOException catch.
39+
throw JsonMappingException.from(p, "expected a JSON object for /headers/ body, got null map");
40+
}
2541
return new RequestHeaders(raw);
2642
}
43+
44+
/**
45+
* Jackson routes a top-level JSON {@code null} through this seam instead of calling {@link
46+
* #deserialize}. Default behavior returns {@code null}; we instead throw so the wire-null case
47+
* produces a {@link com.marketdata.sdk.exception.ParseError} with the endpoint URL in scope,
48+
* matching the failure shape of any other malformed body.
49+
*/
50+
@Override
51+
public RequestHeaders getNullValue(DeserializationContext ctxt) throws JsonMappingException {
52+
throw JsonMappingException.from(
53+
ctxt, "expected a JSON object for /headers/ body, got JSON null");
54+
}
2755
}

src/main/java/com/marketdata/sdk/utilities/RequestHeaders.java

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
package com.marketdata.sdk.utilities;
22

33
import java.util.Map;
4+
import java.util.Objects;
45

56
/**
67
* Response shape for {@code GET /headers/} — the request headers echoed back by the server (with
@@ -10,11 +11,16 @@
1011
* surfaces them.
1112
*
1213
* @param headers all headers the server received, lower-cased keys to values. The map is
13-
* defensively copied and immutable.
14+
* defensively copied and immutable. Never {@code null} — the package is {@code @NullMarked},
15+
* and the canonical constructor rejects a {@code null} argument with a {@link
16+
* NullPointerException} naming the field. The wire-format deserializer pre-checks for a JSON
17+
* {@code null} token and surfaces a {@link com.marketdata.sdk.exception.ParseError} instead, so
18+
* consumers never see a bare NPE from the wire path.
1419
*/
1520
public record RequestHeaders(Map<String, String> headers) {
1621

1722
public RequestHeaders {
23+
Objects.requireNonNull(headers, "headers");
1824
headers = Map.copyOf(headers);
1925
}
2026
}
Lines changed: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,60 @@
1+
package com.marketdata.sdk;
2+
3+
import static org.assertj.core.api.Assertions.assertThat;
4+
import static org.assertj.core.api.Assertions.assertThatThrownBy;
5+
6+
import com.marketdata.sdk.utilities.RequestHeaders;
7+
import java.util.HashMap;
8+
import java.util.Map;
9+
import org.junit.jupiter.api.Test;
10+
11+
/**
12+
* Focused tests for the {@link RequestHeaders} record's canonical constructor. The wire-level path
13+
* (a server returning a JSON-{@code null} body) is exercised end-to-end in {@link
14+
* UtilitiesResourceTest}; this class documents the public-API contract that consumers see when
15+
* constructing the record directly.
16+
*/
17+
class RequestHeadersTest {
18+
19+
@Test
20+
void constructorRejectsNullMapWithNamedFieldMessage() {
21+
// The package is @NullMarked, so `null` violates the public contract. Pre-checking with
22+
// requireNonNull yields a clear "headers" message; without it, Map.copyOf(null) throws a bare
23+
// NPE that leaves the consumer hunting for which constructor argument was null.
24+
assertThatThrownBy(() -> new RequestHeaders(null))
25+
.isInstanceOf(NullPointerException.class)
26+
.hasMessageContaining("headers");
27+
}
28+
29+
@Test
30+
void constructorAcceptsEmptyMap() {
31+
// An empty map is a legitimate (if unusual) value — Map.copyOf preserves emptiness and the
32+
// result is still immutable.
33+
RequestHeaders rh = new RequestHeaders(Map.of());
34+
35+
assertThat(rh.headers()).isEmpty();
36+
}
37+
38+
@Test
39+
void constructorDefensivelyCopiesTheInputMap() {
40+
// Map.copyOf snapshots the input. A consumer mutating the original after construction must
41+
// not be able to mutate the record's view — this is the defensive-copy guarantee the Javadoc
42+
// promises.
43+
Map<String, String> mutable = new HashMap<>();
44+
mutable.put("accept", "*/*");
45+
RequestHeaders rh = new RequestHeaders(mutable);
46+
47+
mutable.put("authorization", "Bearer leaked");
48+
49+
assertThat(rh.headers()).containsOnlyKeys("accept");
50+
}
51+
52+
@Test
53+
void headersAccessorReturnsImmutableView() {
54+
// The Map.copyOf result is unmodifiable; consumers attempting to mutate get UOE rather than
55+
// silently corrupting the record's invariant.
56+
RequestHeaders rh = new RequestHeaders(Map.of("accept", "*/*"));
57+
58+
assertThat(rh.headers()).isUnmodifiable();
59+
}
60+
}

src/test/java/com/marketdata/sdk/UtilitiesResourceTest.java

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -274,6 +274,25 @@ void headersSyncUnwrapsAuthenticationFailureFromCompletionException() {
274274
assertThatThrownBy(utilities::headers).isInstanceOf(AuthenticationError.class);
275275
}
276276

277+
/**
278+
* A literal JSON {@code null} body for {@code /headers/} must surface as a {@link
279+
* com.marketdata.sdk.exception.ParseError} carrying the request URL, status, and id — never as a
280+
* raw {@code NullPointerException} from the {@link RequestHeaders} canonical constructor. The
281+
* {@link RequestHeadersDeserializer} pre-check converts the null token into a {@code
282+
* JsonMappingException}, which {@link JsonResponseParser} wraps with the support context.
283+
*/
284+
@Test
285+
void headersJsonNullBodySurfacesParseErrorNotNpe() {
286+
CapturingClient client =
287+
new CapturingClient(200, "null".getBytes(), HttpHeaders.of(Map.of(), (a, b) -> true));
288+
UtilitiesResource utilities = resourceWith(client);
289+
290+
assertThatThrownBy(utilities::headers)
291+
.isInstanceOf(com.marketdata.sdk.exception.ParseError.class)
292+
.hasMessageContaining("/headers/")
293+
.hasMessageContaining("JSON null");
294+
}
295+
277296
// ---------- stub HttpClient ----------
278297

279298
private static final class CapturingClient extends TestHttpClients.StubHttpClient {

0 commit comments

Comments
 (0)