diff --git a/CHANGELOG.MD b/CHANGELOG.MD index 7385cf92..b3b8b814 100644 --- a/CHANGELOG.MD +++ b/CHANGELOG.MD @@ -6,6 +6,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Added + +- Add functionality to ensure a failed SDS call results in the Operation Outcome being forwarded as the response, rather + than a spring framework internal server error being returned. + ## [1.1.2] - 2025-04-23 - Updated Dockerfile to set -XX:MaxRAMPercentage=75.0, optimizing JVM memory allocation for better performance diff --git a/gpcc-mocks/src/main/java/uk/nhs/adaptors/gpccmocks/common/OperationOutcomes.java b/gpcc-mocks/src/main/java/uk/nhs/adaptors/gpccmocks/common/OperationOutcomes.java index e9a2ed2c..a2f31448 100644 --- a/gpcc-mocks/src/main/java/uk/nhs/adaptors/gpccmocks/common/OperationOutcomes.java +++ b/gpcc-mocks/src/main/java/uk/nhs/adaptors/gpccmocks/common/OperationOutcomes.java @@ -14,7 +14,7 @@ public static ResponseEntity badRequest(String message) { .message(message) .build(); var body = TemplateUtils.fillTemplate(OPERATION_OUTCOME, model); - return new ResponseEntity<>(body, HttpStatus.BAD_REQUEST); + return new ResponseEntity<>(body, ControllerHelpers.getResponseHeaders(), HttpStatus.BAD_REQUEST); } public static ResponseEntity invalidNhsNumber(String message) { @@ -24,7 +24,7 @@ public static ResponseEntity invalidNhsNumber(String message) { .message(message) .build(); var body = TemplateUtils.fillTemplate(OPERATION_OUTCOME, model); - return new ResponseEntity<>(body, HttpStatus.BAD_REQUEST); + return new ResponseEntity<>(body, ControllerHelpers.getResponseHeaders(), HttpStatus.BAD_REQUEST); } public static ResponseEntity patientNotFound(String message) { @@ -34,7 +34,7 @@ public static ResponseEntity patientNotFound(String message) { .message(message) .build(); var body = TemplateUtils.fillTemplate(OPERATION_OUTCOME, model); - return new ResponseEntity<>(body, HttpStatus.NOT_FOUND); + return new ResponseEntity<>(body, ControllerHelpers.getResponseHeaders(), HttpStatus.NOT_FOUND); } public static ResponseEntity referenceNotFound(String message) { @@ -44,6 +44,6 @@ public static ResponseEntity referenceNotFound(String message) { .message(message) .build(); var body = TemplateUtils.fillTemplate(OPERATION_OUTCOME, model); - return new ResponseEntity<>(body, HttpStatus.NOT_FOUND); + return new ResponseEntity<>(body, ControllerHelpers.getResponseHeaders(), HttpStatus.NOT_FOUND); } } diff --git a/service/src/intTest/java/uk/nhs/adaptors/gpc/consumer/CloudGatewayRouteBaseTest.java b/service/src/intTest/java/uk/nhs/adaptors/gpc/consumer/CloudGatewayRouteBaseTest.java index 094b952c..e74faa24 100644 --- a/service/src/intTest/java/uk/nhs/adaptors/gpc/consumer/CloudGatewayRouteBaseTest.java +++ b/service/src/intTest/java/uk/nhs/adaptors/gpc/consumer/CloudGatewayRouteBaseTest.java @@ -62,16 +62,15 @@ protected WebTestClient.RequestBodySpec getWebTestClientForStandardPost(String r protected void When_GetRequestProducesSdsError_Expect_OperationOutcomeErrorResponse(String requestUri, String interactionId) { // Using Ssp-TraceID not a UUID to force an error from the SDS mock - webTestClient.get().uri(requestUri) + var responseSpec = webTestClient.get().uri(requestUri) .header(SSP_FROM_HEADER, ANY_STRING) .header(SSP_TO_HEADER, ANY_STRING) .header(SSP_INTERACTION_ID_HEADER, interactionId) .header(SSP_TRACE_ID_HEADER, "NotUUID") .header(HttpHeaders.AUTHORIZATION, ANYTOKEN) - .exchange() - .expectStatus() - .isEqualTo(HttpStatus.INTERNAL_SERVER_ERROR); - // TODO: NIAD-1165 GPCC should use the SDS API OperationOutcome here instead of a Spring default error response body + .exchange(); + + assertOperationOutcomeBadRequest(responseSpec); } protected WebTestClient.RequestHeadersSpec getWebTestClientForStandardGet(String requestUri, String interactionId) { @@ -84,4 +83,15 @@ protected WebTestClient.RequestHeadersSpec getWebTestClientForStandardGet(Str .header(HttpHeaders.AUTHORIZATION, ANYTOKEN); } + protected void assertOperationOutcomeBadRequest(WebTestClient.ResponseSpec responseSpec) { + responseSpec + .expectStatus().isEqualTo(HttpStatus.BAD_REQUEST) + .expectHeader().contentTypeCompatibleWith("application/json+fhir") + .expectBody() + .jsonPath("$.resourceType").isEqualTo("OperationOutcome") + .jsonPath("$.issue[0].code").isEqualTo("structure") + .jsonPath("$.issue[0].details.coding[0].code").isEqualTo("BAD_REQUEST") + .jsonPath("$.issue[0].details.coding[0].display").isEqualTo("BAD_REQUEST") + .jsonPath("$.issue[0].diagnostics").isEqualTo("X-Correlation-Id header must be a UUID"); + } } diff --git a/service/src/intTest/java/uk/nhs/adaptors/gpc/consumer/GetStructuredRecordRouteTest.java b/service/src/intTest/java/uk/nhs/adaptors/gpc/consumer/GetStructuredRecordRouteTest.java index f91ebdaa..a5ce9ade 100644 --- a/service/src/intTest/java/uk/nhs/adaptors/gpc/consumer/GetStructuredRecordRouteTest.java +++ b/service/src/intTest/java/uk/nhs/adaptors/gpc/consumer/GetStructuredRecordRouteTest.java @@ -5,7 +5,6 @@ import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.MethodSource; import org.springframework.http.HttpHeaders; -import org.springframework.http.HttpStatus; public class GetStructuredRecordRouteTest extends CloudGatewayRouteBaseTest { static final String REQUEST_URI_TEMPLATE = "/%s/STU3/1/gpconnect/structured/fhir/Patient/$gpc.getstructuredrecord"; @@ -50,17 +49,15 @@ public void When_SdsErrorOccursBeforeAccessStructuredRecord_Expect_ProxyResponse var nhsNumber = Fixtures.Patient.HAS_DOCUMENTS.getNhsNumber(); var requestBody = String.format(REQUEST_BODY_TEMPLATE, nhsNumber); - // Using Ssp-TraceID not a UUID to force an error from the SDS mock - getWebTestClient().post().uri(requestUri) + var responseSpec = getWebTestClient().post().uri(requestUri) .bodyValue(requestBody) .header(SSP_FROM_HEADER, ANY_STRING) .header(SSP_TO_HEADER, ANY_STRING) .header(SSP_INTERACTION_ID_HEADER, STRUCTURED_INTERACTION_ID) .header(SSP_TRACE_ID_HEADER, "NotUUID") .header(HttpHeaders.AUTHORIZATION, "anytoken") - .exchange() - .expectStatus() - .isEqualTo(HttpStatus.INTERNAL_SERVER_ERROR); - // TODO: NIAD-1165 GPCC should use the SDS API OperationOutcome here instead of a Spring default error response body + .exchange(); + + assertOperationOutcomeBadRequest(responseSpec); } } diff --git a/service/src/intTest/java/uk/nhs/adaptors/gpc/consumer/MigrateStructuredPatientRouteTest.java b/service/src/intTest/java/uk/nhs/adaptors/gpc/consumer/MigrateStructuredPatientRouteTest.java index d5273ab1..b062eb52 100644 --- a/service/src/intTest/java/uk/nhs/adaptors/gpc/consumer/MigrateStructuredPatientRouteTest.java +++ b/service/src/intTest/java/uk/nhs/adaptors/gpc/consumer/MigrateStructuredPatientRouteTest.java @@ -5,7 +5,6 @@ import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.MethodSource; import org.springframework.http.HttpHeaders; -import org.springframework.http.HttpStatus; public class MigrateStructuredPatientRouteTest extends CloudGatewayRouteBaseTest { static final String REQUEST_URI_TEMPLATE = "/%s/STU3/1/gpconnect/fhir/Patient/$gpc.migratestructuredrecord"; @@ -51,16 +50,15 @@ public void When_SdsErrorOccursBeforeAccessMigrateStructuredPatient_Expect_Proxy var requestBody = String.format(REQUEST_BODY_TEMPLATE, nhsNumber); // Using Ssp-TraceID not a UUID to force an error from the SDS mock - getWebTestClient().post().uri(requestUri) + var responseSpec = getWebTestClient().post().uri(requestUri) .bodyValue(requestBody) .header(SSP_FROM_HEADER, ANY_STRING) .header(SSP_TO_HEADER, ANY_STRING) .header(SSP_INTERACTION_ID_HEADER, MIGRATE_STRUCTURED_INTERACTION_ID) .header(SSP_TRACE_ID_HEADER, "NotUUID") .header(HttpHeaders.AUTHORIZATION, "anytoken") - .exchange() - .expectStatus() - .isEqualTo(HttpStatus.INTERNAL_SERVER_ERROR); - // TODO: NIAD-1165 GPCC should use the SDS API OperationOutcome here instead of a Spring default error response body + .exchange(); + + assertOperationOutcomeBadRequest(responseSpec); } } diff --git a/service/src/intTest/java/uk/nhs/adaptors/gpc/consumer/sds/SdsClientComponentTest.java b/service/src/intTest/java/uk/nhs/adaptors/gpc/consumer/sds/SdsClientComponentTest.java index 7be39ec8..40fd589a 100644 --- a/service/src/intTest/java/uk/nhs/adaptors/gpc/consumer/sds/SdsClientComponentTest.java +++ b/service/src/intTest/java/uk/nhs/adaptors/gpc/consumer/sds/SdsClientComponentTest.java @@ -49,7 +49,6 @@ import uk.nhs.adaptors.gpc.consumer.common.ResourceReader; import uk.nhs.adaptors.gpc.consumer.gpc.exception.GpConnectException; import uk.nhs.adaptors.gpc.consumer.sds.configuration.SdsConfiguration; -import uk.nhs.adaptors.gpc.consumer.sds.exception.SdsException; import uk.nhs.adaptors.gpc.consumer.testcontainers.WiremockExtension; @ExtendWith({SpringExtension.class, WiremockExtension.class}) @@ -281,7 +280,7 @@ public void When_SdsReturnsError_Expect_Exception() { stubSdsError(ENDPOINT); stubSdsError(DEVICE); assertThatThrownBy(() -> pair.getValue().apply(FROM_ODS_CODE, X_CORRELATION_ID).blockOptional()) - .isInstanceOf(SdsException.class); + .isInstanceOf(SdsPassthroughException.class); wireMockServer.resetAll(); }); } @@ -293,7 +292,7 @@ public void When_NoXCorrelationIdPresent_Expect_Exception() { stubSdsError(ENDPOINT); stubSdsError(DEVICE); assertThatThrownBy(() -> pair.getValue().apply(FROM_ODS_CODE, null).blockOptional()) - .isInstanceOf(SdsException.class); + .isInstanceOf(SdsPassthroughException.class); wireMockServer.resetAll(); }); } @@ -305,8 +304,9 @@ public void When_InvalidXCorrelationId_Expect_Exception() { stubSdsError(ENDPOINT); stubSdsError(DEVICE); assertThatThrownBy(() -> pair.getValue().apply(FROM_ODS_CODE, "not-UUID").blockOptional()) - .isInstanceOf(SdsException.class); + .isInstanceOf(SdsPassthroughException.class); wireMockServer.resetAll(); }); } } + diff --git a/service/src/main/java/uk/nhs/adaptors/gpc/consumer/sds/SdsClient.java b/service/src/main/java/uk/nhs/adaptors/gpc/consumer/sds/SdsClient.java index dbab9f3e..94c8f735 100644 --- a/service/src/main/java/uk/nhs/adaptors/gpc/consumer/sds/SdsClient.java +++ b/service/src/main/java/uk/nhs/adaptors/gpc/consumer/sds/SdsClient.java @@ -7,6 +7,8 @@ import org.jetbrains.annotations.NotNull; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.beans.factory.annotation.Value; +import org.springframework.http.HttpHeaders; +import org.springframework.http.HttpStatus; import org.springframework.stereotype.Component; import org.springframework.web.reactive.function.client.WebClient.RequestHeadersSpec; @@ -146,8 +148,22 @@ private String getAddressFromEndpoint(Endpoint endpoint) { } private Mono performRequest(RequestHeadersSpec> request) { - return request.retrieve() - .bodyToMono(String.class); + return request.exchangeToMono(clientResponse -> { + if (clientResponse.statusCode().is2xxSuccessful()) { + return clientResponse.bodyToMono(String.class); + } + + // Capture the status, headers and body, and allow to bubble up as a SdsPassthroughException + HttpStatus status = (HttpStatus) clientResponse.statusCode(); + HttpHeaders headers = clientResponse.headers().asHttpHeaders(); + + return clientResponse.bodyToMono(String.class) + .defaultIfEmpty("") + .flatMap(body -> { + LOGGER.warn("SDS returned error status: {}. Forwarding OperationOutcome.", status); + return Mono.error(new SdsPassthroughException(status, headers, body)); + }); + }); } @Builder diff --git a/service/src/main/java/uk/nhs/adaptors/gpc/consumer/sds/SdsPassthroughException.java b/service/src/main/java/uk/nhs/adaptors/gpc/consumer/sds/SdsPassthroughException.java new file mode 100644 index 00000000..19a7c131 --- /dev/null +++ b/service/src/main/java/uk/nhs/adaptors/gpc/consumer/sds/SdsPassthroughException.java @@ -0,0 +1,31 @@ +package uk.nhs.adaptors.gpc.consumer.sds; + +import lombok.Getter; +import lombok.extern.slf4j.Slf4j; +import org.springframework.http.HttpHeaders; +import org.springframework.http.HttpStatus; +import org.springframework.http.ResponseEntity; + +/** + * Signals that we should forward the SDS response (status + headers + body) unchanged to the caller. + */ +@Getter +@Slf4j +public class SdsPassthroughException extends RuntimeException { + private final HttpStatus status; + private final HttpHeaders headers; + private final String body; + + public SdsPassthroughException(HttpStatus status, HttpHeaders headers, String body) { + super("SDS passthrough " + status); + this.status = status; + this.headers = HttpHeaders.readOnlyHttpHeaders(headers); + this.body = body; + } + + public ResponseEntity toResponseEntity() { + HttpHeaders out = new HttpHeaders(); + out.putAll(headers); + return new ResponseEntity<>(body, out, status); + } +} diff --git a/service/src/main/java/uk/nhs/adaptors/gpc/consumer/web/SdsPassthroughExceptionHandler.java b/service/src/main/java/uk/nhs/adaptors/gpc/consumer/web/SdsPassthroughExceptionHandler.java new file mode 100644 index 00000000..a778f9d5 --- /dev/null +++ b/service/src/main/java/uk/nhs/adaptors/gpc/consumer/web/SdsPassthroughExceptionHandler.java @@ -0,0 +1,15 @@ +package uk.nhs.adaptors.gpc.consumer.web; + +import org.springframework.http.ResponseEntity; +import org.springframework.web.bind.annotation.ExceptionHandler; +import org.springframework.web.bind.annotation.RestControllerAdvice; +import uk.nhs.adaptors.gpc.consumer.sds.SdsPassthroughException; + +@RestControllerAdvice +public class SdsPassthroughExceptionHandler { + + @ExceptionHandler(SdsPassthroughException.class) + public ResponseEntity handleSds(SdsPassthroughException ex) { + return ex.toResponseEntity(); + } +} diff --git a/service/src/main/java/uk/nhs/adaptors/gpc/consumer/web/WebClientFilterService.java b/service/src/main/java/uk/nhs/adaptors/gpc/consumer/web/WebClientFilterService.java index 78e08e3c..d9a9021e 100644 --- a/service/src/main/java/uk/nhs/adaptors/gpc/consumer/web/WebClientFilterService.java +++ b/service/src/main/java/uk/nhs/adaptors/gpc/consumer/web/WebClientFilterService.java @@ -33,11 +33,22 @@ public ExchangeFilterFunction errorHandlingFilter(RequestType requestType, HttpS return ExchangeFilterFunction.ofResponseProcessor(clientResponse -> { clientResponse.statusCode(); if (clientResponse.statusCode().equals(httpStatus)) { - LOGGER.info(requestType + " request successful, status code: {}", clientResponse.statusCode()); + LOGGER.info("{} request successful, status code: {}", requestType, clientResponse.statusCode()); return Mono.just(clientResponse); - } else { - return getResponseError(clientResponse, requestType); } + + if (requestType == RequestType.SDS) { + LOGGER.warn("SDS returned {}, forwarding response unchanged", clientResponse.statusCode()); + return Mono.just(clientResponse); + } + + if (requestType == RequestType.GPC) { + LOGGER.warn("GPC returned {}, forwarding response unchanged", clientResponse.statusCode()); + return Mono.just(clientResponse); + } + + return getResponseError(clientResponse, requestType); + }); } diff --git a/service/src/main/resources/application.yml b/service/src/main/resources/application.yml index e93377ee..eee67592 100644 --- a/service/src/main/resources/application.yml +++ b/service/src/main/resources/application.yml @@ -34,5 +34,5 @@ gpc-consumer: sds: url: ${GPC_CONSUMER_SDS_URL:} apiKey: ${GPC_CONSUMER_SDS_APIKEY:} - supplierOdsCode: ${GPC_SUPPLIER_ODS_CODE:} + supplierOdsCode: ${GPC_SUPPLIER_ODS_CODE:XX111}