From dcb4b629b054891847173a937611017e178b1571 Mon Sep 17 00:00:00 2001 From: MartinWheelerMT Date: Tue, 8 Sep 2026 14:07:35 +0100 Subject: [PATCH] Refactor `SdsFilter` for readabilty * Moved SSP Header names into `HeaderConstants`. * Refactor `SdsFilterTest` to use headers from `HeaderConstants` * Renamed method to better reflect their usage. * Refactor code to use more modern switch with pattern matching to improve readability. * Refactor `extractOdsCode` to fluent methods. * Refactor `appendSspHeaderWhenAbsent` to handle the header array values better by using `getFirst` on the HttpHeader object. --- .../gpc/consumer/filters/SdsFilter.java | 165 ++++++++---------- .../gpc/consumer/utils/HeaderConstants.java | 3 + .../gpc/consumer/gpc/SdsFilterTest.java | 3 +- 3 files changed, 79 insertions(+), 92 deletions(-) diff --git a/service/src/main/java/uk/nhs/adaptors/gpc/consumer/filters/SdsFilter.java b/service/src/main/java/uk/nhs/adaptors/gpc/consumer/filters/SdsFilter.java index 05b00eb5..179e4f78 100644 --- a/service/src/main/java/uk/nhs/adaptors/gpc/consumer/filters/SdsFilter.java +++ b/service/src/main/java/uk/nhs/adaptors/gpc/consumer/filters/SdsFilter.java @@ -1,29 +1,8 @@ package uk.nhs.adaptors.gpc.consumer.filters; -import org.springframework.http.HttpHeaders; -import org.springframework.http.HttpStatus; -import org.springframework.http.MediaType; -import org.springframework.http.ResponseEntity; -import static uk.nhs.adaptors.gpc.consumer.gpc.InteractionIds.DOCUMENT_MIGRATE_ID; -import static uk.nhs.adaptors.gpc.consumer.gpc.InteractionIds.DOCUMENT_READ_ID; -import static uk.nhs.adaptors.gpc.consumer.gpc.InteractionIds.DOCUMENT_SEARCH_ID; -import static uk.nhs.adaptors.gpc.consumer.gpc.InteractionIds.MIGRATE_STRUCTURED_ID; -import static uk.nhs.adaptors.gpc.consumer.gpc.InteractionIds.PATIENT_SEARCH_ID; -import static uk.nhs.adaptors.gpc.consumer.gpc.InteractionIds.STRUCTURED_ID; -import static uk.nhs.adaptors.gpc.consumer.utils.HeaderConstants.SSP_TRACE_ID; -import static uk.nhs.adaptors.gpc.consumer.utils.OperationOutcomes.buildErrorResponse; - -import java.net.URI; -import java.nio.charset.StandardCharsets; -import java.util.List; -import java.util.Map; -import java.util.Objects; -import java.util.Optional; -import java.util.function.BiFunction; -import java.util.stream.Collectors; - import jakarta.annotation.PostConstruct; - +import lombok.RequiredArgsConstructor; +import lombok.extern.slf4j.Slf4j; import org.jetbrains.annotations.NotNull; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.cloud.gateway.filter.GatewayFilterChain; @@ -32,6 +11,10 @@ import org.springframework.cloud.gateway.support.ServerWebExchangeUtils; import org.springframework.core.Ordered; import org.springframework.core.io.buffer.DataBuffer; +import org.springframework.http.HttpHeaders; +import org.springframework.http.HttpStatus; +import org.springframework.http.MediaType; +import org.springframework.http.ResponseEntity; import org.springframework.http.server.PathContainer; import org.springframework.http.server.RequestPath; import org.springframework.http.server.reactive.ServerHttpRequest; @@ -42,25 +25,39 @@ import org.springframework.web.reactive.function.client.WebClientResponseException; import org.springframework.web.server.ServerWebExchange; import org.springframework.web.util.UriComponentsBuilder; - -import lombok.RequiredArgsConstructor; -import lombok.extern.slf4j.Slf4j; import reactor.core.publisher.Mono; import uk.nhs.adaptors.gpc.consumer.filters.exception.SdsFilterException; import uk.nhs.adaptors.gpc.consumer.sds.SdsClient; import uk.nhs.adaptors.gpc.consumer.sds.exception.SdsException; import uk.nhs.adaptors.gpc.consumer.utils.LoggingUtil; +import uk.nhs.adaptors.gpc.consumer.utils.OperationOutcomes; import uk.nhs.adaptors.gpc.consumer.utils.QueryParamsEncoder; +import java.net.URI; +import java.nio.charset.StandardCharsets; +import java.util.Map; +import java.util.Objects; +import java.util.Optional; +import java.util.function.BiFunction; + +import static uk.nhs.adaptors.gpc.consumer.gpc.InteractionIds.DOCUMENT_MIGRATE_ID; +import static uk.nhs.adaptors.gpc.consumer.gpc.InteractionIds.DOCUMENT_READ_ID; +import static uk.nhs.adaptors.gpc.consumer.gpc.InteractionIds.DOCUMENT_SEARCH_ID; +import static uk.nhs.adaptors.gpc.consumer.gpc.InteractionIds.MIGRATE_STRUCTURED_ID; +import static uk.nhs.adaptors.gpc.consumer.gpc.InteractionIds.PATIENT_SEARCH_ID; +import static uk.nhs.adaptors.gpc.consumer.gpc.InteractionIds.STRUCTURED_ID; +import static uk.nhs.adaptors.gpc.consumer.utils.HeaderConstants.SSP_FROM; +import static uk.nhs.adaptors.gpc.consumer.utils.HeaderConstants.SSP_INTERACTION_ID; +import static uk.nhs.adaptors.gpc.consumer.utils.HeaderConstants.SSP_TO; +import static uk.nhs.adaptors.gpc.consumer.utils.HeaderConstants.SSP_TRACE_ID; + @Component @Slf4j @RequiredArgsConstructor(onConstructor = @__(@Autowired)) public class SdsFilter implements GlobalFilter, Ordered { public static final int SDS_FILTER_ORDER = RouteToRequestUrlFilter.ROUTE_TO_URL_FILTER_ORDER + 1; - public static final String SSP_INTERACTION_ID = "Ssp-InteractionID"; private static final String DOCUMENT_REFERENCE_SUFFIX = "/DocumentReference"; - public static final String OPERATION_OUTCOME = "operationOutcome"; public static final String INTERNAL_SERVER_ERROR = "INTERNAL_SERVER_ERROR"; public static final String EXCEPTION = "exception"; public static final String STRUCTURE = "structure"; @@ -68,7 +65,6 @@ public class SdsFilter implements GlobalFilter, Ordered { public static final String BAD_GATEWAY = "BAD_GATEWAY"; public static final String BAD_REQUEST = "BAD_REQUEST"; public static final String PATIENT_NOT_FOUND = "PATIENT_NOT_FOUND"; - public static final String NO_ENDPOINT_AVAILABLE = "NO_ENDPOINT_AVAILABLE"; public static final String MISSING_HEADER_EXCEPTION_MESSAGE = "Missing or empty %s Header Value for SDS Request"; private final SdsClient sdsClient; @@ -77,12 +73,12 @@ public class SdsFilter implements GlobalFilter, Ordered { @Override public Mono filter(ServerWebExchange exchange, GatewayFilterChain chain) { return getGpcProviderEndpointDetails(exchange) - .flatMap(gpcProviderEndpointDetails -> appendSspHeadersToExchangeIfRequired( - exchange, - chain, - gpcProviderEndpointDetails) - ) - .onErrorResume(Exception.class, e -> errorResponse(exchange, e)); + .flatMap(gpcProviderEndpointDetails -> appendSspHeadersToExchangeIfRequired( + exchange, + chain, + gpcProviderEndpointDetails) + ) + .onErrorResume(Exception.class, e -> buildErrorResponse(exchange, e)); } private Mono getGpcProviderEndpointDetails(ServerWebExchange exchange) { @@ -138,10 +134,10 @@ private Mono appendSspHeadersToExchangeIfRequired( ? chain.filter(exchange) : getGpcConsumerAsid(exchange) .flatMap(gpcConsumerAsid -> addMissingSspHeaders( - gpcConsumerAsid, - exchange, - chain, - gpcProviderEndpointDetails + gpcConsumerAsid, + exchange, + chain, + gpcProviderEndpointDetails )); } @@ -151,21 +147,17 @@ private Mono addMissingSspHeaders( GatewayFilterChain chain, SdsClient.SdsResponseData gpcProviderEndpointDetails ) { - var mutatedExchange = appendSspHeaderWhenAbsent( - exchange, - gpcProviderEndpointDetails.getNhsSpineAsid(), - "Ssp-To" - ); - mutatedExchange = appendSspHeaderWhenAbsent(mutatedExchange, gpcConsumerAsid, "Ssp-From"); + var mutatedExchange = appendSspHeaderWhenAbsent(exchange, gpcProviderEndpointDetails.getNhsSpineAsid(), SSP_TO); + mutatedExchange = appendSspHeaderWhenAbsent(mutatedExchange, gpcConsumerAsid, SSP_FROM); return chain.filter(mutatedExchange); } - private static @NotNull Mono errorResponse(ServerWebExchange exchange, Exception e) { + private static @NotNull Mono buildErrorResponse(ServerWebExchange exchange, Exception e) { HttpStatus status = mapExceptionToHttpStatus(e); String spineCode = mapWebClientExceptionToSpineCode(e); String fhirCode = mapSpineCodeToFhirCode(spineCode); - ResponseEntity errorResponse = buildErrorResponse(status, spineCode, fhirCode, e.getMessage()); + ResponseEntity errorResponse = OperationOutcomes.buildErrorResponse(status, spineCode, fhirCode, e.getMessage()); ServerHttpResponse response = exchange.getResponse(); response.setStatusCode(errorResponse.getStatusCode()); response.getHeaders().setContentType(MediaType.APPLICATION_JSON); @@ -176,13 +168,12 @@ private Mono addMissingSspHeaders( } private static HttpStatus mapExceptionToHttpStatus(Exception e) { - if (e instanceof WebClientRequestException) { - return HttpStatus.BAD_GATEWAY; - } - if (e instanceof WebClientResponseException webClientResponseEx) { - return HttpStatus.resolve(webClientResponseEx.getStatusCode().value()); - } - return HttpStatus.INTERNAL_SERVER_ERROR; + return switch (e) { + case WebClientRequestException ignored -> HttpStatus.BAD_GATEWAY; + case WebClientResponseException webClientResponseEx -> + HttpStatus.resolve(webClientResponseEx.getStatusCode().value()); + default -> HttpStatus.INTERNAL_SERVER_ERROR; + }; } private static String mapWebClientExceptionToSpineCode(Exception e) { @@ -216,32 +207,20 @@ private Mono getGpcConsumerAsid(ServerWebExchange exchange) { } private String extractOdsCode(RequestPath requestPath) { - Optional odsCodeElement = requestPath.elements() - .stream() + return requestPath.elements().stream() .skip(1) - .findFirst(); - - if (odsCodeElement.isPresent()) { - return odsCodeElement.get().value(); - } - - throw new IllegalArgumentException("URL does not contain ODS code in its second element"); + .findFirst() + .map(PathContainer.Element::value) + .orElseThrow(() -> new IllegalArgumentException("URL does not contain ODS code in its second element")); } @NotNull private ServerWebExchange appendSspHeaderWhenAbsent(ServerWebExchange exchange, String asid, String sspHeader) { + String ssp = Optional.ofNullable(exchange.getRequest().getHeaders().getFirst(sspHeader)) + .filter(StringUtils::hasText) + .orElse(asid); - List incomingSspHeaderValue = exchange.getRequest().getHeaders().get(sspHeader); - String ssp = asid; - - if (incomingSspHeaderValue != null) { - ssp = incomingSspHeaderValue.stream().findFirst().orElse(asid); - } - - ServerHttpRequest mutateRequest = exchange.getRequest() - .mutate() - .header(sspHeader, ssp) - .build(); + ServerHttpRequest mutateRequest = exchange.getRequest().mutate().header(sspHeader, ssp).build(); return exchange.mutate().request(mutateRequest).build(); } @@ -256,22 +235,26 @@ public int getOrder() { @SuppressWarnings("unused") public void initializeSdsRequestFunctions() { sdsRequestFunctions = Map.of( - STRUCTURED_ID, sdsClient::callForGetStructuredRecord, - PATIENT_SEARCH_ID, sdsClient::callForPatientSearchAccessDocument, - DOCUMENT_SEARCH_ID, sdsClient::callForSearchForDocumentRecord, - DOCUMENT_READ_ID, sdsClient::callForRetrieveDocumentRecord, - DOCUMENT_MIGRATE_ID, sdsClient::callForMigrateDocumentRecord, - MIGRATE_STRUCTURED_ID, sdsClient::callForMigrateStructuredRecord + STRUCTURED_ID, sdsClient::callForGetStructuredRecord, + PATIENT_SEARCH_ID, sdsClient::callForPatientSearchAccessDocument, + DOCUMENT_SEARCH_ID, sdsClient::callForSearchForDocumentRecord, + DOCUMENT_READ_ID, sdsClient::callForRetrieveDocumentRecord, + DOCUMENT_MIGRATE_ID, sdsClient::callForMigrateDocumentRecord, + MIGRATE_STRUCTURED_ID, sdsClient::callForMigrateStructuredRecord ); } - private Mono performRequestAccordingToInteractionId(String interactionId, - String organisation, String sspTraceId, ServerWebExchange exchange) { + private Mono performRequestAccordingToInteractionId( + String interactionId, + String organisation, + String sspTraceId, + ServerWebExchange exchange + ) { if (sdsRequestFunctions.containsKey(interactionId)) { LoggingUtil.info(LOGGER, exchange, "Performing request with organisation \"{}\" and NHS service endpoint id \"{}\"", - organisation, interactionId); + organisation, interactionId); return sdsRequestFunctions.get(interactionId) - .apply(organisation, sspTraceId); + .apply(organisation, sspTraceId); } throw new IllegalArgumentException(String.format("Not recognised InteractionId %s", interactionId)); } @@ -279,8 +262,8 @@ private Mono performRequestAccordingToInteractionId(S private Optional prepareLookupUri(String serviceRootUrl, ServerHttpRequest originalRequest) { var originalRequestPath = originalRequest.getPath(); var originalRequestPathValues = originalRequestPath.elements().stream() - .map(PathContainer.Element::value) - .collect(Collectors.toList()); + .map(PathContainer.Element::value) + .toList(); int indexOfPatientInFhirPath = originalRequestPathValues.lastIndexOf("Patient"); int indexOfBinaryInFhirPath = originalRequestPathValues.lastIndexOf("Binary"); int indexOfStartOfFhirPath = Math.max(indexOfPatientInFhirPath, indexOfBinaryInFhirPath); @@ -288,12 +271,12 @@ private Optional prepareLookupUri(String serviceRootUrl, ServerHttpRequest throw new SdsFilterException("Unable to detect a supported FHIR path in the original request"); } String fhirRequestPathPart = originalRequest.getPath().subPath(indexOfStartOfFhirPath - 1) - .toString(); + .toString(); String uriWithoutQueryParameters = serviceRootUrl + fhirRequestPathPart; URI constructedUri = UriComponentsBuilder.fromUriString(uriWithoutQueryParameters) - .queryParams(originalRequest.getQueryParams()) - .build() - .toUri(); + .queryParams(originalRequest.getQueryParams()) + .build() + .toUri(); return Optional.of(constructedUri); } -} +} \ No newline at end of file diff --git a/service/src/main/java/uk/nhs/adaptors/gpc/consumer/utils/HeaderConstants.java b/service/src/main/java/uk/nhs/adaptors/gpc/consumer/utils/HeaderConstants.java index 235438ea..1fcab811 100644 --- a/service/src/main/java/uk/nhs/adaptors/gpc/consumer/utils/HeaderConstants.java +++ b/service/src/main/java/uk/nhs/adaptors/gpc/consumer/utils/HeaderConstants.java @@ -2,6 +2,9 @@ public final class HeaderConstants { public static final String SSP_TRACE_ID = "Ssp-TraceID"; + public static final String SSP_INTERACTION_ID = "Ssp-InteractionID"; + public static final String SSP_FROM = "Ssp-From"; + public static final String SSP_TO = "Ssp-To"; private HeaderConstants() { } } diff --git a/service/src/test/java/uk/nhs/adaptors/gpc/consumer/gpc/SdsFilterTest.java b/service/src/test/java/uk/nhs/adaptors/gpc/consumer/gpc/SdsFilterTest.java index 3e9d1709..ac8a8165 100644 --- a/service/src/test/java/uk/nhs/adaptors/gpc/consumer/gpc/SdsFilterTest.java +++ b/service/src/test/java/uk/nhs/adaptors/gpc/consumer/gpc/SdsFilterTest.java @@ -32,9 +32,10 @@ import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.when; import reactor.test.StepVerifier; -import static uk.nhs.adaptors.gpc.consumer.filters.SdsFilter.SSP_INTERACTION_ID; + import static uk.nhs.adaptors.gpc.consumer.gpc.InteractionIds.DOCUMENT_READ_ID; import static uk.nhs.adaptors.gpc.consumer.gpc.InteractionIds.STRUCTURED_ID; +import static uk.nhs.adaptors.gpc.consumer.utils.HeaderConstants.SSP_INTERACTION_ID; import static uk.nhs.adaptors.gpc.consumer.utils.HeaderConstants.SSP_TRACE_ID; import static org.assertj.core.api.Assertions.assertThat;