From d02d1bb0c14853444f12f55cc85293a46ab511ae Mon Sep 17 00:00:00 2001 From: Oliver Drotbohm Date: Tue, 21 Feb 2023 17:25:28 +0100 Subject: [PATCH] Support for returning response bodies on deletion of item resources. RepositoryRestConfiguration now allows to configure to return a response body for the deletion of item resources. The controller implementation follows the same patter we have already established for creation and updates: unless explicitly enabled or disabled we now consider the presence of an accept header as indicator of whether a response body should be rendered. This could be a "breaking" change for clients having explicitly expected 204 until now even for requests with an Accept header. If that's an issue, those should either explicitly disable the setting, do not submit an Accept header or loosen their expectations to expect either 200 or 2xx as indicator of success in general. Fixes #2225. --- .../config/RepositoryRestConfiguration.java | 25 +++++++++++ ...itoryEntityControllerIntegrationTests.java | 44 ++++++++++++++++++- .../security/SecurityIntegrationTests.java | 7 +-- .../webmvc/RepositoryEntityController.java | 8 +++- 4 files changed, 76 insertions(+), 8 deletions(-) diff --git a/spring-data-rest-core/src/main/java/org/springframework/data/rest/core/config/RepositoryRestConfiguration.java b/spring-data-rest-core/src/main/java/org/springframework/data/rest/core/config/RepositoryRestConfiguration.java index f72bd5744..3b54f72c1 100644 --- a/spring-data-rest-core/src/main/java/org/springframework/data/rest/core/config/RepositoryRestConfiguration.java +++ b/spring-data-rest-core/src/main/java/org/springframework/data/rest/core/config/RepositoryRestConfiguration.java @@ -61,6 +61,7 @@ public class RepositoryRestConfiguration { private boolean useHalAsDefaultJsonMediaType = true; private Boolean returnBodyOnCreate = null; private Boolean returnBodyOnUpdate = null; + private Boolean returnBodyOnDelete = null; private List> exposeIdsFor = new ArrayList>(); private ResourceMappingConfiguration domainMappings = new ResourceMappingConfiguration(); private ResourceMappingConfiguration repoMappings = new ResourceMappingConfiguration(); @@ -375,6 +376,30 @@ public class RepositoryRestConfiguration { return this; } + /** + * Whether to return a response body after deleting an entity considering the given accept header. + * + * @param acceptHeader can be {@literal null} or empty. + * @return + * @since 4.1 + */ + public boolean returnBodyOnDelete(String acceptHeader) { + return returnBodyOnDelete == null ? StringUtils.hasText(acceptHeader) : returnBodyOnDelete; + } + + /** + * Set whether to return a response body after deleting an entity. + * + * @param returnBodyOnUpdate can be {@literal null}, expressing the decision shall be derived from the presence of an + * {@code Accept} header in the request. + * @return {@literal this} + * @since 4.1 + */ + public RepositoryRestConfiguration setReturnBodyOnDelete(Boolean returnBodyOnDelete) { + this.returnBodyOnDelete = returnBodyOnDelete; + return this; + } + /** * Start configuration a {@link ResourceMapping} for a specific domain type. * diff --git a/spring-data-rest-tests/spring-data-rest-tests-jpa/src/test/java/org/springframework/data/rest/webmvc/RepositoryEntityControllerIntegrationTests.java b/spring-data-rest-tests/spring-data-rest-tests-jpa/src/test/java/org/springframework/data/rest/webmvc/RepositoryEntityControllerIntegrationTests.java index 499432399..40cac3290 100755 --- a/spring-data-rest-tests/spring-data-rest-tests-jpa/src/test/java/org/springframework/data/rest/webmvc/RepositoryEntityControllerIntegrationTests.java +++ b/spring-data-rest-tests/spring-data-rest-tests-jpa/src/test/java/org/springframework/data/rest/webmvc/RepositoryEntityControllerIntegrationTests.java @@ -22,8 +22,11 @@ import static org.springframework.http.HttpMethod.*; import java.util.List; import java.util.Optional; +import java.util.stream.Stream; +import org.junit.jupiter.api.DynamicTest; import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.TestFactory; import org.mockito.Mockito; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.context.annotation.Bean; @@ -266,7 +269,7 @@ class RepositoryEntityControllerIntegrationTests extends AbstractControllerInteg RootResourceInformation informationSpy = Mockito.spy(resourceInformation); doReturn(invoker).when(informationSpy).getInvoker(); - controller.deleteItemResource(informationSpy, "foo", ETag.from("0")); + controller.deleteItemResource(informationSpy, "foo", ETag.from("0"), assembler, MediaType.ALL_VALUE); assertThat(repository.findById(address.id)).isEmpty(); } @@ -283,5 +286,44 @@ class RepositoryEntityControllerIntegrationTests extends AbstractControllerInteg MediaType.APPLICATION_JSON_VALUE)); } + @TestFactory // #2225 + Stream returnsResponseBodyForDeleteForAcceptHeaderOrConfig() throws Exception { + + record Fixture(String description, Boolean activateReturnBodyOnDelete, String acceptHeader, + HttpStatus expectedStatusCode) { + public String description() { + return description.formatted(expectedStatusCode); + } + } + + var fixtures = Stream.of( + new Fixture("No config, no header -> %s", null, null, HttpStatus.NO_CONTENT), + new Fixture("No config, but header -> %s", null, MediaType.ALL_VALUE, HttpStatus.OK), + new Fixture("Enabled, no header -> %s", true, null, HttpStatus.OK), + new Fixture("Enabled, and header -> %s", true, MediaType.ALL_VALUE, HttpStatus.OK), + new Fixture("Disabled, no header -> %s", false, null, HttpStatus.NO_CONTENT), + new Fixture("Disabled, and header -> %s", false, MediaType.ALL_VALUE, HttpStatus.NO_CONTENT)); + + return DynamicTest.stream(fixtures, Fixture::description, it -> { + + try { + + // Apply configuration + configuration.setReturnBodyOnDelete(it.activateReturnBodyOnDelete()); + + var address = repository.save(new Address()); + var response = controller.deleteItemResource(getResourceInformation(Address.class), address.id, + ETag.NO_ETAG, assembler, it.acceptHeader()); + + assertThat(response.getStatusCode()).isEqualTo(it.expectedStatusCode()); + + } finally { + + // Reset configuration + configuration.setReturnBodyOnDelete(null); + } + }); + } + interface AddressProjection {} } diff --git a/spring-data-rest-tests/spring-data-rest-tests-security/src/test/java/org/springframework/data/rest/tests/security/SecurityIntegrationTests.java b/spring-data-rest-tests/spring-data-rest-tests-security/src/test/java/org/springframework/data/rest/tests/security/SecurityIntegrationTests.java index 9c978d8ba..c22288766 100755 --- a/spring-data-rest-tests/spring-data-rest-tests-security/src/test/java/org/springframework/data/rest/tests/security/SecurityIntegrationTests.java +++ b/spring-data-rest-tests/spring-data-rest-tests-security/src/test/java/org/springframework/data/rest/tests/security/SecurityIntegrationTests.java @@ -29,7 +29,6 @@ import org.springframework.data.map.repository.config.EnableMapRepositories; import org.springframework.data.rest.tests.AbstractWebIntegrationTests; import org.springframework.data.rest.tests.TestMvcClient; import org.springframework.data.rest.webmvc.config.RepositoryRestMvcConfiguration; -import org.springframework.http.HttpStatus; import org.springframework.mock.web.MockHttpServletResponse; import org.springframework.security.access.intercept.aopalliance.MethodSecurityInterceptor; import org.springframework.security.authentication.UsernamePasswordAuthenticationToken; @@ -37,7 +36,6 @@ import org.springframework.security.core.authority.AuthorityUtils; import org.springframework.security.core.context.SecurityContextHolder; import org.springframework.test.context.ContextConfiguration; import org.springframework.test.context.junit.jupiter.SpringExtension; -import org.springframework.test.web.servlet.result.MockMvcResultHandlers; import org.springframework.test.web.servlet.setup.MockMvcBuilders; import org.springframework.web.context.WebApplicationContext; @@ -140,7 +138,7 @@ class SecurityIntegrationTests extends AbstractWebIntegrationTests { SecurityContextHolder.clearContext(); mvc.perform(delete(href).with(user("user").roles("USER", "ADMIN"))) - .andExpect(status().is(HttpStatus.NO_CONTENT.value())); + .andExpect(status().is2xxSuccessful()); } @Test // DATAREST-327 @@ -205,7 +203,7 @@ class SecurityIntegrationTests extends AbstractWebIntegrationTests { SecurityContextHolder.clearContext(); mvc.perform(delete(href).with(user("user").roles("USER", "ADMIN"))) - .andExpect(status().is(HttpStatus.NO_CONTENT.value())); + .andExpect(status().is2xxSuccessful()); } @Test // DATAREST-327 @@ -240,7 +238,6 @@ class SecurityIntegrationTests extends AbstractWebIntegrationTests { String href = assertHasJsonPathValue("$._embedded.orders[0]._links.self.href", response); mvc.perform(get(href).with(user("user").roles("USER"))) - .andDo(MockMvcResultHandlers.print()) .andExpect(status().isForbidden()); } } diff --git a/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/RepositoryEntityController.java b/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/RepositoryEntityController.java index 088bca734..e7df909ab 100644 --- a/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/RepositoryEntityController.java +++ b/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/RepositoryEntityController.java @@ -402,7 +402,9 @@ class RepositoryEntityController extends AbstractRepositoryRestController implem */ @RequestMapping(value = BASE_MAPPING + "/{id}", method = RequestMethod.DELETE) public ResponseEntity deleteItemResource(RootResourceInformation resourceInformation, @BackendId Serializable id, - ETag eTag) throws ResourceNotFoundException, HttpRequestMethodNotSupportedException { + ETag eTag, PersistentEntityResourceAssembler assembler, + @RequestHeader(value = ACCEPT_HEADER, required = false) String acceptHeader) + throws ResourceNotFoundException, HttpRequestMethodNotSupportedException { resourceInformation.verifySupportedMethod(HttpMethod.DELETE, ResourceType.ITEM); @@ -419,7 +421,9 @@ class RepositoryEntityController extends AbstractRepositoryRestController implem invoker.invokeDeleteById(entity.getIdentifierAccessor(it).getIdentifier()); publisher.publishEvent(new AfterDeleteEvent(it)); - return new ResponseEntity(HttpStatus.NO_CONTENT); + return config.returnBodyOnDelete(acceptHeader) + ? ResponseEntity.ok(assembler.toFullResource(it)) + : new ResponseEntity(HttpStatus.NO_CONTENT); }).orElseThrow(() -> new ResourceNotFoundException()); }