Skip to content

Commit

Permalink
Po 749 747 exception paths (#609)
Browse files Browse the repository at this point in the history 
* merge conflicts

* exception pathways for put and patch

* unused import
  • Loading branch information
@TomReed
TomReed authored Oct 24, 2024
1 parent 2ea6e32 commit c4e5cc1
Show file tree
Hide file tree
Showing 6 changed files with 172 additions and 27 deletions.
160 changes: 156 additions & 4 deletions ...grationTest/java/uk/gov/hmcts/opal/controllers/DraftAccountControllerIntegrationTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,9 @@
import com.fasterxml.jackson.databind.ObjectMapper;
import jakarta.persistence.QueryTimeoutException;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.Arguments;
import org.junit.jupiter.params.provider.MethodSource;
import org.mockito.stubbing.OngoingStubbing;
import org.postgresql.util.PSQLException;
import org.postgresql.util.PSQLState;
Expand All @@ -11,6 +14,7 @@
import org.springframework.boot.test.autoconfigure.web.servlet.WebMvcTest;
import org.springframework.boot.test.mock.mockito.MockBean;
import org.springframework.boot.test.mock.mockito.SpyBean;
import org.springframework.http.HttpMethod;
import org.springframework.http.MediaType;
import org.springframework.test.context.ActiveProfiles;
import org.springframework.test.context.ContextConfiguration;
Expand All @@ -21,6 +25,7 @@
import uk.gov.hmcts.opal.authentication.service.AccessTokenService;
import uk.gov.hmcts.opal.authorisation.model.Permissions;
import uk.gov.hmcts.opal.authorisation.model.UserState;
import uk.gov.hmcts.opal.config.WebConfig;
import uk.gov.hmcts.opal.controllers.advice.GlobalExceptionHandler;
import uk.gov.hmcts.opal.dto.AddDraftAccountRequestDto;
import uk.gov.hmcts.opal.dto.ReplaceDraftAccountRequestDto;
Expand All @@ -38,6 +43,7 @@
import java.time.LocalDate;
import java.time.LocalDateTime;
import java.util.logging.Logger;
import java.util.stream.Stream;

import static java.util.Collections.singletonList;
import static org.hamcrest.Matchers.containsString;
Expand All @@ -46,6 +52,10 @@
import static org.mockito.ArgumentMatchers.eq;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;
import static org.springframework.http.HttpMethod.GET;
import static org.springframework.http.HttpMethod.PATCH;
import static org.springframework.http.HttpMethod.POST;
import static org.springframework.http.HttpMethod.PUT;
import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.delete;
import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.get;
import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.patch;
Expand All @@ -60,7 +70,7 @@


@WebMvcTest
@ContextConfiguration(classes = {DraftAccountController.class, GlobalExceptionHandler.class})
@ContextConfiguration(classes = {DraftAccountController.class, GlobalExceptionHandler.class, WebConfig.class})
@ActiveProfiles({"integration"})
class DraftAccountControllerIntegrationTest {
private static final Logger logger = Logger.getLogger(DraftAccountControllerIntegrationTest.class.getSimpleName());
Expand Down Expand Up @@ -637,7 +647,7 @@ void shouldReturn503WhenDownstreamServiceIsUnavailable(MockHttpServletRequestBui
}"""));
}

private String validCreateRequestBody() {
private static String validCreateRequestBody() {
return """
{
"account": {
Expand All @@ -662,7 +672,7 @@ private String validCreateRequestBody() {
}""";
}

private String invalidCreateRequestBody() {
private static String invalidCreateRequestBody() {
return """
{
"invalid_field": "This field shouldn't be here",
Expand All @@ -683,7 +693,7 @@ private String invalidCreateRequestBody() {
}""";
}

private String validUpdateRequestBody() {
private static String validUpdateRequestBody() {
return """
{
"account_status": "PENDING",
Expand All @@ -706,4 +716,146 @@ private DraftAccountEntity toEntity(AddDraftAccountRequestDto dto, LocalDateTim
.build();
}

//CEP 1 CEP1 - Invalid Request Payload (400)
@ParameterizedTest
@MethodSource("endpointsWithInvalidBodiesProvider")
void methodsShouldReturn400WhenRequestPayloadIsInvalid(HttpMethod method, String fullPath, String requestBody)
throws Exception {
MockHttpServletRequestBuilder requestBuilder = switch (method.name()) {
case "GET" -> get(fullPath);
case "POST" -> post(fullPath);
case "PUT" -> put(fullPath);
case "PATCH" -> patch(fullPath);
default -> throw new IllegalArgumentException("Unsupported HTTP method: " + method);
};

when(userStateService.checkForAuthorisedUser(any())).thenReturn(allPermissionsUser());

mockMvc.perform(requestBuilder
.header("Authorization", "Bearer some_value")
.header("Accept", "application/json")
.contentType(MediaType.APPLICATION_JSON)
.content(requestBody))
.andExpect(status().isBadRequest());
}

private static Stream<Arguments> endpointsWithInvalidBodiesProvider() {
return Stream.of(Arguments.of(POST, "/draft-accounts",invalidCreateRequestBody()),
Arguments.of(PUT, "/draft-accounts/1",invalidCreateRequestBody()),
Arguments.of(PATCH, "/draft-accounts/1",invalidCreateRequestBody())
);
}

//CEP2 - Invalid or No Access Token (401) - Security Context required - test elsewhere

//CEP3 - Not Authorised to perform the requested action (403)
@ParameterizedTest
@MethodSource("testCasesRequiringAuthorizationProvider")
void methodsShouldReturn403WhenUserLacksPermission(HttpMethod method, String fullPath, String requestBody)
throws Exception {
MockHttpServletRequestBuilder requestBuilder = switch (method.name()) {
case "GET" -> get(fullPath);
case "POST" -> post(fullPath);
case "PUT" -> put(fullPath);
case "PATCH" -> patch(fullPath);
default -> throw new IllegalArgumentException("Unsupported HTTP method: " + method);
};

when(userStateService.checkForAuthorisedUser(any())).thenReturn(noPermissionsUser());

mockMvc.perform(requestBuilder
.header("Authorization", "Bearer some_value")
.contentType(MediaType.APPLICATION_JSON)
.content(requestBody))
.andExpect(status().isForbidden())
.andExpect(content().contentType(MediaType.APPLICATION_JSON))
.andExpect(jsonPath("$.error").value("Forbidden"));
}

private static Stream<Arguments> testCasesRequiringAuthorizationProvider() {
return Stream.of(
Arguments.of(POST, "/draft-accounts", validCreateRequestBody()),
Arguments.of(PUT, "/draft-accounts/1", validCreateRequestBody()),
Arguments.of(PATCH, "/draft-accounts/1", validUpdateRequestBody()),
Arguments.of(GET, "/draft-accounts", "") // GET endpoints with empty body
);
}

//CEP4 - Resource Not Found (404) - applies to GET PUT PATCH & DELETE
@ParameterizedTest
@MethodSource("testCasesForResourceNotFoundProvider")
void methodsShouldReturn404WhenResourceNotFound(HttpMethod method, String fullPath, String requestBody)
throws Exception {
// Set up a non-existent ID
long nonExistentId = 999L;

MockHttpServletRequestBuilder requestBuilder = switch (method.name()) {
case "GET" -> get(fullPath);
case "PUT" -> put(fullPath);
case "PATCH" -> patch(fullPath);
case "DELETE" -> delete(fullPath);
default -> throw new IllegalArgumentException("Unsupported HTTP method: " + method);
};

// Mock the service behavior
when(userStateService.checkForAuthorisedUser(any())).thenReturn(allPermissionsUser());

// For GET return null
when(draftAccountService.getDraftAccount(nonExistentId)).thenReturn(null);

// For PUT, throw EntityNotFoundException
when(draftAccountService.replaceDraftAccount(eq(nonExistentId), any(ReplaceDraftAccountRequestDto.class)))
.thenThrow(new jakarta.persistence.EntityNotFoundException("Draft Account not found with id: "
+ nonExistentId));
// For PATCH, throw EntityNotFoundException
when(draftAccountService.updateDraftAccount(eq(nonExistentId), any(UpdateDraftAccountRequestDto.class)))
.thenThrow(new jakarta.persistence.EntityNotFoundException("Draft Account not found with id: "
+ nonExistentId));
mockMvc.perform(requestBuilder
.header("Authorization", "Bearer some_value")
.contentType(MediaType.APPLICATION_JSON)
.content(requestBody))
.andExpect(status().isNotFound());
}

private static Stream<Arguments> testCasesForResourceNotFoundProvider() {
return Stream.of(
Arguments.of(GET, "/draft-accounts/999", ""),
Arguments.of(PUT, "/draft-accounts/999", validCreateRequestBody()),
Arguments.of(PATCH, "/draft-accounts/999", validUpdateRequestBody())
);
}

//CEP5 - Unsupported Content Type for Response (406)
@ParameterizedTest
@MethodSource("testCasesWithValidBodiesProvider")
void methodsShouldReturn406WhenAcceptHeaderIsNotSupported(HttpMethod method, String fullPath, String requestBody)
throws Exception {
MockHttpServletRequestBuilder requestBuilder = switch (method.name()) {
case "GET" -> get(fullPath);
case "POST" -> post(fullPath);
case "PUT" -> put(fullPath);
case "PATCH" -> patch(fullPath);
default -> throw new IllegalArgumentException("Unsupported HTTP method: " + method);
};

when(userStateService.checkForAuthorisedUser(any())).thenReturn(allPermissionsUser());

mockMvc.perform(requestBuilder
.header("Authorization", "Bearer some_value")
.header("Accept", "application/xml")
.contentType(MediaType.APPLICATION_JSON)
.content(requestBody))
.andExpect(status().isNotAcceptable());
}

private static Stream<Arguments> testCasesWithValidBodiesProvider() {
return Stream.of(Arguments.of(POST, "/draft-accounts",validCreateRequestBody()),
Arguments.of(PUT, "/draft-accounts/1","{}"),
Arguments.of(PATCH, "/draft-accounts/1","{}"),
Arguments.of(GET, "/draft-accounts/1","{}")
);
}


}
8 changes: 6 additions & 2 deletions src/main/java/uk/gov/hmcts/opal/controllers/DraftAccountController.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@ public DraftAccountController(UserStateService userStateService, DraftAccountSer
}

@GetMapping(value = "/{draftAccountId}")
@CheckAcceptHeader
@Operation(summary = "Returns the Draft Account for the given draftAccountId.")
public ResponseEntity<DraftAccountResponseDto> getDraftAccountById(
@PathVariable Long draftAccountId,
Expand All @@ -86,6 +87,7 @@ public ResponseEntity<DraftAccountResponseDto> getDraftAccountById(
}

@GetMapping()
@CheckAcceptHeader
@Operation(summary = "Returns a collection of draft accounts summaries for the given user.")
public ResponseEntity<DraftAccountsResponseDto> getDraftAccountSummaries(
@RequestParam(value = "business_unit") Optional<List<Short>> optionalBusinessUnitIds,
Expand Down Expand Up @@ -176,18 +178,19 @@ public ResponseEntity<String> deleteDraftAccountById(

@PutMapping(value = "/{draftAccountId}", consumes = MediaType.APPLICATION_JSON_VALUE)
@Operation(summary = "Replaces an existing Draft Account Entity in the DB with data in request body")
@CheckAcceptHeader
public ResponseEntity<DraftAccountResponseDto> replaceDraftAccount(
@PathVariable Long draftAccountId,
@RequestBody ReplaceDraftAccountRequestDto dto,
@RequestHeader(value = "Authorization", required = false) String authHeaderValue) {

UserState userState = userStateService.checkForAuthorisedUser(authHeaderValue);
jsonSchemaValidationService.validateOrError(dto.toJson(), REPLACE_DRAFT_ACCOUNT_REQUEST_JSON);

if (userState.hasBusinessUnitUserWithPermission(dto.getBusinessUnitId(),
Permissions.CREATE_MANAGE_DRAFT_ACCOUNTS)) {
log.info(":PUT:replaceDraftAccount: replacing draft account entity with ID: {} and data: {}",
draftAccountId, dto);
jsonSchemaValidationService.validateOrError(dto.toJson(), REPLACE_DRAFT_ACCOUNT_REQUEST_JSON);

DraftAccountEntity replacedEntity = draftAccountService.replaceDraftAccount(draftAccountId, dto);

Expand All @@ -199,12 +202,14 @@ public ResponseEntity<DraftAccountResponseDto> replaceDraftAccount(

@PatchMapping(value = "/{draftAccountId}", consumes = MediaType.APPLICATION_JSON_VALUE)
@Operation(summary = "Updates an existing Draft Account Entity in the DB with data in request body")
@CheckAcceptHeader
public ResponseEntity<DraftAccountResponseDto> updateDraftAccount(
@PathVariable Long draftAccountId,
@RequestBody UpdateDraftAccountRequestDto dto,
@RequestHeader(value = "Authorization", required = false) String authHeaderValue) {

UserState userState = userStateService.checkForAuthorisedUser(authHeaderValue);
jsonSchemaValidationService.validateOrError(dto.toJson(), UPDATE_DRAFT_ACCOUNT_REQUEST_JSON);

if (userState.hasBusinessUnitUserWithPermission(dto.getBusinessUnitId(),
Permissions.CREATE_MANAGE_DRAFT_ACCOUNTS)) {
Expand All @@ -213,7 +218,6 @@ public ResponseEntity<DraftAccountResponseDto> updateDraftAccount(
draftAccountId, dto
);

jsonSchemaValidationService.validateOrError(dto.toJson(), UPDATE_DRAFT_ACCOUNT_REQUEST_JSON);

DraftAccountEntity updatedEntity = draftAccountService.updateDraftAccount(draftAccountId, dto);

Expand Down
2 changes: 1 addition & 1 deletion src/main/java/uk/gov/hmcts/opal/interceptor/AcceptHeaderInterceptor.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,6 @@ public boolean preHandle(HttpServletRequest request, HttpServletResponse respons
}

private boolean isAcceptableMediaType(String acceptHeader) {
return acceptHeader != null && (acceptHeader.contains("application/json") || acceptHeader.contains("*/*"));
return acceptHeader == null || (acceptHeader.contains("application/json") || acceptHeader.contains("*/*"));
}
}
5 changes: 3 additions & 2 deletions src/main/java/uk/gov/hmcts/opal/service/opal/DraftAccountService.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
import com.fasterxml.jackson.core.JsonProcessingException;
import com.fasterxml.jackson.databind.ObjectMapper;
import com.fasterxml.jackson.databind.node.ObjectNode;
import jakarta.persistence.EntityNotFoundException;
import lombok.RequiredArgsConstructor;
import lombok.extern.slf4j.Slf4j;
import org.springframework.beans.factory.annotation.Qualifier;
Expand Down Expand Up @@ -100,7 +101,7 @@ public DraftAccountEntity submitDraftAccount(AddDraftAccountRequestDto dto) {

public DraftAccountEntity replaceDraftAccount(Long draftAccountId, ReplaceDraftAccountRequestDto dto) {
DraftAccountEntity existingAccount = draftAccountRepository.findById(draftAccountId)
.orElseThrow(() -> new RuntimeException("Draft Account not found with id: " + draftAccountId));
.orElseThrow(() -> new EntityNotFoundException("Draft Account not found with id: " + draftAccountId));

BusinessUnitEntity businessUnit = businessUnitRepository.findById(dto.getBusinessUnitId())
.orElseThrow(() -> new RuntimeException("Business Unit not found with id: " + dto.getBusinessUnitId()));
Expand Down Expand Up @@ -133,7 +134,7 @@ public DraftAccountEntity replaceDraftAccount(Long draftAccountId, ReplaceDraftA

public DraftAccountEntity updateDraftAccount(Long draftAccountId, UpdateDraftAccountRequestDto dto) {
DraftAccountEntity existingAccount = draftAccountRepository.findById(draftAccountId)
.orElseThrow(() -> new RuntimeException("Draft Account not found with id: " + draftAccountId));
.orElseThrow(() -> new EntityNotFoundException("Draft Account not found with id: " + draftAccountId));

if (!(existingAccount.getBusinessUnit().getBusinessUnitId().equals(dto.getBusinessUnitId()))) {
log.info("DTO BU does not match entity for draft account with ID: {}", draftAccountId);
Expand Down
7 changes: 6 additions & 1 deletion src/main/resources/jsonSchemas/updateDraftAccountRequest.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,5 +20,10 @@
"type": "object",
"description": "Status changes to the Draft Account in chronological order (JSON Array) - System generated (UI)"
}
}
},
"required": [
"business_unit_id",
"validated_by",
"account_status"
]
}
17 changes: 0 additions & 17 deletions src/test/java/uk/gov/hmcts/opal/interceptor/AcceptHeaderInterceptorTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -78,23 +78,6 @@ void preHandle_WithCheckAcceptHeaderAnnotationAndInvalidAcceptHeader_ShouldRetur
assertTrue(stringWriter.toString().contains("\"message\":\"The requested media type is not supported\""));
}

@Test
void preHandle_WithCheckAcceptHeaderAnnotationAndNullAcceptHeader_ShouldReturnFalse() throws Exception {
when(handlerMethod.hasMethodAnnotation(CheckAcceptHeader.class)).thenReturn(true);
when(request.getHeader("Accept")).thenReturn(null);

StringWriter stringWriter = new StringWriter();
PrintWriter writer = new PrintWriter(stringWriter);
when(response.getWriter()).thenReturn(writer);

boolean result = interceptor.preHandle(request, response, handlerMethod);

assertFalse(result);
verify(response).setStatus(HttpServletResponse.SC_NOT_ACCEPTABLE);
writer.flush();
assertTrue(stringWriter.toString().contains("\"error\":\"Not Acceptable\""));
assertTrue(stringWriter.toString().contains("\"message\":\"The requested media type is not supported\""));
}

@Test
void preHandle_WithoutCheckAcceptHeaderAnnotation_ShouldReturnTrue() throws Exception {
Expand Down

0 comments on commit c4e5cc1

Please sign in to comment.