From fa4e9d2118e7621bbeaf6b569b78069859e3ba90 Mon Sep 17 00:00:00 2001 From: PG1204 Date: Mon, 15 Jun 2026 12:39:09 -0700 Subject: [PATCH] feat: graceful API contract for malformed requests (P2) - Add GlobalExceptionHandler (@RestControllerAdvice): a malformed or empty request body now returns HTTP 200 with a PrAnalysisResponse envelope (overallRisk=PARSING_ERROR) instead of Spring's default 400, honoring the documented always-200 graceful-degradation contract. - Extract PayloadTooLargeException to a public top-level class; the advice maps an over-limit chunked body to 413, consistent with the P0 size filter (this case previously leaked through as a 400). - Tests: GlobalExceptionHandlerTest (unit, both branches) and a @WebMvcTest covering malformed/empty -> 200 envelope and valid -> service delegation. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../web/GlobalExceptionHandler.java | 68 ++++++++++++++++++ .../web/PayloadTooLargeException.java | 18 +++++ .../web/RequestSizeLimitFilter.java | 7 -- .../web/AnalysisControllerWebTest.java | 69 +++++++++++++++++++ .../web/GlobalExceptionHandlerTest.java | 50 ++++++++++++++ 5 files changed, 205 insertions(+), 7 deletions(-) create mode 100644 src/main/java/com/example/blast_radius/web/GlobalExceptionHandler.java create mode 100644 src/main/java/com/example/blast_radius/web/PayloadTooLargeException.java create mode 100644 src/test/java/com/example/blast_radius/web/AnalysisControllerWebTest.java create mode 100644 src/test/java/com/example/blast_radius/web/GlobalExceptionHandlerTest.java diff --git a/src/main/java/com/example/blast_radius/web/GlobalExceptionHandler.java b/src/main/java/com/example/blast_radius/web/GlobalExceptionHandler.java new file mode 100644 index 0000000..648546d --- /dev/null +++ b/src/main/java/com/example/blast_radius/web/GlobalExceptionHandler.java @@ -0,0 +1,68 @@ +package com.example.blast_radius.web; + +import com.example.blast_radius.model.OverallRisk; +import com.example.blast_radius.model.PrAnalysisResponse; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; +import org.springframework.http.HttpStatus; +import org.springframework.http.ResponseEntity; +import org.springframework.http.converter.HttpMessageNotReadableException; +import org.springframework.web.bind.annotation.ExceptionHandler; +import org.springframework.web.bind.annotation.RestControllerAdvice; + +import java.util.Map; + +/** + * Maps request-level failures onto the service's graceful-degradation contract. + * + *

A malformed or empty analysis request body would otherwise produce Spring's + * default 400 error page; instead it returns HTTP 200 with a + * {@link PrAnalysisResponse} envelope (overallRisk = {@code PARSING_ERROR}), + * matching every other failure path so the CI consumer always sees the same shape. + * + *

The one carve-out: an over-limit body that slipped past the Content-Length + * fast path (chunked transfer) arrives here wrapping a {@link PayloadTooLargeException}. + * That is surfaced as 413 — consistent with {@link RequestSizeLimitFilter} — rather + * than masked as a successful 200 analysis. + */ +@RestControllerAdvice +public class GlobalExceptionHandler { + + private static final Logger log = LoggerFactory.getLogger(GlobalExceptionHandler.class); + + @ExceptionHandler(HttpMessageNotReadableException.class) + public ResponseEntity handleUnreadableBody(HttpMessageNotReadableException ex) { + if (hasCause(ex, PayloadTooLargeException.class)) { + log.warn("Rejected over-limit request body (no Content-Length): {}", rootMessage(ex)); + return ResponseEntity.status(HttpStatus.CONTENT_TOO_LARGE) + .body(Map.of( + "error", "Request body exceeds the configured size limit", + "status", HttpStatus.CONTENT_TOO_LARGE.value())); + } + + log.warn("Malformed analysis request body: {}", rootMessage(ex)); + return ResponseEntity.ok(PrAnalysisResponse.error(OverallRisk.PARSING_ERROR, null)); + } + + /** True if {@code type} appears anywhere in the exception's cause chain. */ + private boolean hasCause(Throwable ex, Class type) { + for (Throwable t = ex; t != null; t = t.getCause()) { + if (type.isInstance(t)) { + return true; + } + if (t.getCause() == t) { + break; + } + } + return false; + } + + /** The deepest cause's message, for a concise, root-cause log line. */ + private String rootMessage(Throwable ex) { + Throwable root = ex; + while (root.getCause() != null && root.getCause() != root) { + root = root.getCause(); + } + return root.getMessage(); + } +} diff --git a/src/main/java/com/example/blast_radius/web/PayloadTooLargeException.java b/src/main/java/com/example/blast_radius/web/PayloadTooLargeException.java new file mode 100644 index 0000000..a9b2193 --- /dev/null +++ b/src/main/java/com/example/blast_radius/web/PayloadTooLargeException.java @@ -0,0 +1,18 @@ +package com.example.blast_radius.web; + +import java.io.IOException; + +/** + * Signals that a request body grew past the configured size limit while being read. + * + *

Thrown by {@link RequestSizeLimitFilter}'s streaming guard for chunked requests + * (or a lying Content-Length). It extends {@link IOException} so it propagates through + * the servlet input-stream contract; {@code GlobalExceptionHandler} recognizes it in + * the cause chain and maps it to HTTP 413, consistent with the Content-Length fast path. + */ +public class PayloadTooLargeException extends IOException { + + public PayloadTooLargeException(long limit) { + super("Request body exceeds limit of " + limit + " bytes"); + } +} diff --git a/src/main/java/com/example/blast_radius/web/RequestSizeLimitFilter.java b/src/main/java/com/example/blast_radius/web/RequestSizeLimitFilter.java index bfaf303..f806fd6 100644 --- a/src/main/java/com/example/blast_radius/web/RequestSizeLimitFilter.java +++ b/src/main/java/com/example/blast_radius/web/RequestSizeLimitFilter.java @@ -74,13 +74,6 @@ private void reject(HttpServletRequest request, HttpServletResponse response, lo "Request body exceeds limit of " + maxRequestBytes + " bytes"); } - /** Signals that a request body grew past the configured limit mid-read. */ - static final class PayloadTooLargeException extends IOException { - PayloadTooLargeException(long limit) { - super("Request body exceeds limit of " + limit + " bytes"); - } - } - /** Wraps a request so its body stream is bounded to {@code limit} bytes. */ private static final class LimitingRequestWrapper extends HttpServletRequestWrapper { private final long limit; diff --git a/src/test/java/com/example/blast_radius/web/AnalysisControllerWebTest.java b/src/test/java/com/example/blast_radius/web/AnalysisControllerWebTest.java new file mode 100644 index 0000000..6f2099f --- /dev/null +++ b/src/test/java/com/example/blast_radius/web/AnalysisControllerWebTest.java @@ -0,0 +1,69 @@ +package com.example.blast_radius.web; + +import com.example.blast_radius.controller.AnalysisController; +import com.example.blast_radius.model.OverallRisk; +import com.example.blast_radius.model.PrAnalysisResponse; +import com.example.blast_radius.service.AnalysisService; +import org.junit.jupiter.api.Test; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.boot.webmvc.test.autoconfigure.WebMvcTest; +import org.springframework.http.MediaType; +import org.springframework.test.context.bean.override.mockito.MockitoBean; +import org.springframework.test.web.servlet.MockMvc; + +import java.util.List; + +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.when; +import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.post; +import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.jsonPath; +import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status; + +/** + * Verifies the controller + GlobalExceptionHandler contract: malformed input + * yields the always-200 envelope; valid input is delegated to the service. + */ +@WebMvcTest(AnalysisController.class) +class AnalysisControllerWebTest { + + @Autowired + private MockMvc mockMvc; + + @MockitoBean + private AnalysisService analysisService; + + @Test + void malformedJson_returns200WithParsingErrorEnvelope() throws Exception { + mockMvc.perform(post("/analysis/pr") + .contentType(MediaType.APPLICATION_JSON) + .content("not json at all")) + .andExpect(status().isOk()) + .andExpect(jsonPath("$.overallRisk").value("PARSING_ERROR")) + .andExpect(jsonPath("$.impactAreas").isEmpty()) + .andExpect(jsonPath("$.suggestedTests").isEmpty()); + } + + @Test + void emptyBody_returns200WithParsingErrorEnvelope() throws Exception { + mockMvc.perform(post("/analysis/pr") + .contentType(MediaType.APPLICATION_JSON)) + .andExpect(status().isOk()) + .andExpect(jsonPath("$.overallRisk").value("PARSING_ERROR")); + } + + @Test + void validJson_delegatesToService() throws Exception { + PrAnalysisResponse stub = new PrAnalysisResponse(); + stub.setOverallRisk(OverallRisk.LOW); + stub.setImpactAreas(List.of("FooController#list")); + stub.setSuggestedTests(List.of("Test it")); + when(analysisService.analyze(any())).thenReturn(stub); + + mockMvc.perform(post("/analysis/pr") + .contentType(MediaType.APPLICATION_JSON) + .content("{\"baseBranch\":\"main\",\"targetBranch\":\"f\",\"diff\":\"x\"}")) + .andExpect(status().isOk()) + .andExpect(jsonPath("$.overallRisk").value("LOW")) + .andExpect(jsonPath("$.impactAreas[0]").value("FooController#list")); + } +} diff --git a/src/test/java/com/example/blast_radius/web/GlobalExceptionHandlerTest.java b/src/test/java/com/example/blast_radius/web/GlobalExceptionHandlerTest.java new file mode 100644 index 0000000..6f4346a --- /dev/null +++ b/src/test/java/com/example/blast_radius/web/GlobalExceptionHandlerTest.java @@ -0,0 +1,50 @@ +package com.example.blast_radius.web; + +import com.example.blast_radius.model.OverallRisk; +import com.example.blast_radius.model.PrAnalysisResponse; +import org.junit.jupiter.api.Test; +import org.springframework.http.HttpInputMessage; +import org.springframework.http.ResponseEntity; +import org.springframework.http.converter.HttpMessageNotReadableException; + +import java.util.Map; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertInstanceOf; +import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.mockito.Mockito.mock; + +class GlobalExceptionHandlerTest { + + private final GlobalExceptionHandler handler = new GlobalExceptionHandler(); + private final HttpInputMessage inputMessage = mock(HttpInputMessage.class); + + @Test + void malformedBody_returns200ParsingErrorEnvelope() { + HttpMessageNotReadableException ex = + new HttpMessageNotReadableException("JSON parse error", inputMessage); + + ResponseEntity response = handler.handleUnreadableBody(ex); + + assertEquals(200, response.getStatusCode().value()); + assertInstanceOf(PrAnalysisResponse.class, response.getBody()); + PrAnalysisResponse body = (PrAnalysisResponse) response.getBody(); + assertEquals(OverallRisk.PARSING_ERROR, body.getOverallRisk()); + assertTrue(body.getImpactAreas().isEmpty()); + assertTrue(body.getSuggestedTests().isEmpty()); + } + + @Test + void overLimitChunkedBody_returns413() { + // Simulates the streaming size guard tripping during body read. + HttpMessageNotReadableException ex = new HttpMessageNotReadableException( + "I/O error while reading input", new PayloadTooLargeException(1024), inputMessage); + + ResponseEntity response = handler.handleUnreadableBody(ex); + + assertEquals(413, response.getStatusCode().value()); + assertInstanceOf(Map.class, response.getBody()); + Map body = (Map) response.getBody(); + assertEquals(413, body.get("status")); + } +}