From 2b347daaab16899a404306e63912de0dd9048c16 Mon Sep 17 00:00:00 2001 From: Oliver Gierke Date: Tue, 11 Apr 2017 08:26:41 +0200 Subject: [PATCH] DATAREST-1050 - PUT for create now makes sure the resource id is set on the instance to create. So far, we handed the plain instance deserialized from the request body to the repository to persist it. That caused issues in PUT for create scenarios where the URI contains the identifier to be used for the aggregate to create and identifier generation being used in the backend. In that case the identifier submitted was never considered and subsequent requests would've created new instances, effectively breaking the idempotent nature of PUT. We now make sure the backend identifier derived from the resource is set on the aggregate instance about to be created, so that backend can either accept that situation (new entity + manually defined identifier) or reject it (in case it insists on identifier generation). --- ...ResourceHandlerMethodArgumentResolver.java | 39 +++--- ...andlerMethodArgumentResolverUnitTests.java | 117 ++++++++++++++++++ 2 files changed, 133 insertions(+), 23 deletions(-) create mode 100644 spring-data-rest-webmvc/src/test/java/org/springframework/data/rest/webmvc/config/PersistentEntityResourceHandlerMethodArgumentResolverUnitTests.java diff --git a/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/config/PersistentEntityResourceHandlerMethodArgumentResolver.java b/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/config/PersistentEntityResourceHandlerMethodArgumentResolver.java index 1535506dc..825703377 100644 --- a/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/config/PersistentEntityResourceHandlerMethodArgumentResolver.java +++ b/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/config/PersistentEntityResourceHandlerMethodArgumentResolver.java @@ -1,5 +1,5 @@ /* - * Copyright 2012-2014 the original author or authors. + * Copyright 2012-2017 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -23,7 +23,10 @@ import java.util.Optional; import javax.servlet.http.HttpServletRequest; import org.springframework.core.MethodParameter; +import org.springframework.core.convert.ConversionService; +import org.springframework.core.convert.support.DefaultConversionService; import org.springframework.data.mapping.PersistentEntity; +import org.springframework.data.mapping.model.ConvertingPropertyAccessor; import org.springframework.data.rest.webmvc.IncomingRequest; import org.springframework.data.rest.webmvc.PersistentEntityResource; import org.springframework.data.rest.webmvc.PersistentEntityResource.Builder; @@ -59,10 +62,10 @@ public class PersistentEntityResourceHandlerMethodArgumentResolver implements Ha private static final String NO_CONVERTER_FOUND = "No suitable HttpMessageConverter found to read request body into object of type %s from request with content type of %s!"; private final RootResourceInformationHandlerMethodArgumentResolver resourceInformationResolver; - private final BackendIdHandlerMethodArgumentResolver idResolver; private final DomainObjectReader reader; private final List> messageConverters; + private final ConversionService conversionService = new DefaultConversionService(); /** * Creates a new {@link PersistentEntityResourceHandlerMethodArgumentResolver} for the given @@ -123,8 +126,9 @@ public class PersistentEntityResourceHandlerMethodArgumentResolver implements Ha continue; } - Serializable id = idResolver.resolveArgument(parameter, mavContainer, webRequest, binderFactory); - Optional objectToUpdate = getObjectToUpdate(id, resourceInformation); + Optional id = Optional + .ofNullable(idResolver.resolveArgument(parameter, mavContainer, webRequest, binderFactory)); + Optional objectToUpdate = id.flatMap(it -> resourceInformation.getInvoker().invokeFindOne(it)); Object obj = read(resourceInformation, incoming, converter, objectToUpdate); @@ -137,9 +141,14 @@ public class PersistentEntityResourceHandlerMethodArgumentResolver implements Ha Optional entityIdentifier = objectToUpdate .flatMap(it -> entity.getIdentifierAccessor(it).getIdentifier()); - if (entityIdentifier.isPresent()) { - entity.getPropertyAccessor(obj).setProperty(entity.getRequiredIdProperty(), entityIdentifier); - } + entityIdentifier.ifPresent( + it -> entity.getPropertyAccessor(obj).setProperty(entity.getRequiredIdProperty(), entityIdentifier)); + + id.ifPresent(it -> { + ConvertingPropertyAccessor accessor = new ConvertingPropertyAccessor(entity.getPropertyAccessor(obj), + conversionService); + accessor.setProperty(entity.getRequiredIdProperty(), id); + }); Builder build = PersistentEntityResource.build(obj, entity); return forUpdate ? build.build() : build.forCreation(); @@ -224,20 +233,4 @@ public class PersistentEntityResourceHandlerMethodArgumentResolver implements Ha throw new HttpMessageNotReadableException(String.format(ERROR_MESSAGE, information.getDomainType()), o_O); } } - - /** - * Returns the object to be updated identified by the given id using the given {@link RootResourceInformation}. - * - * @param id can be {@literal null}. - * @param information must not be {@literal null}. - * @return - */ - private static Optional getObjectToUpdate(Serializable id, RootResourceInformation information) { - - if (id == null) { - return Optional.empty(); - } - - return information.getInvoker().invokeFindOne(id); - } } diff --git a/spring-data-rest-webmvc/src/test/java/org/springframework/data/rest/webmvc/config/PersistentEntityResourceHandlerMethodArgumentResolverUnitTests.java b/spring-data-rest-webmvc/src/test/java/org/springframework/data/rest/webmvc/config/PersistentEntityResourceHandlerMethodArgumentResolverUnitTests.java new file mode 100644 index 000000000..35dca6f31 --- /dev/null +++ b/spring-data-rest-webmvc/src/test/java/org/springframework/data/rest/webmvc/config/PersistentEntityResourceHandlerMethodArgumentResolverUnitTests.java @@ -0,0 +1,117 @@ +/* + * Copyright 2017 the original author or authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.springframework.data.rest.webmvc.config; + +import static org.assertj.core.api.Assertions.*; +import static org.mockito.ArgumentMatchers.*; +import static org.mockito.Mockito.*; + +import java.util.Arrays; + +import javax.servlet.http.HttpServletRequest; + +import org.junit.Before; +import org.junit.Test; +import org.mockito.Mockito; +import org.springframework.core.MethodParameter; +import org.springframework.data.annotation.Id; +import org.springframework.data.keyvalue.core.mapping.KeyValuePersistentEntity; +import org.springframework.data.keyvalue.core.mapping.context.KeyValueMappingContext; +import org.springframework.data.repository.support.RepositoryInvoker; +import org.springframework.data.rest.webmvc.PersistentEntityResource; +import org.springframework.data.rest.webmvc.RootResourceInformation; +import org.springframework.data.rest.webmvc.json.DomainObjectReader; +import org.springframework.data.rest.webmvc.support.BackendIdHandlerMethodArgumentResolver; +import org.springframework.http.HttpInputMessage; +import org.springframework.http.MediaType; +import org.springframework.http.converter.HttpMessageConverter; +import org.springframework.mock.web.MockHttpServletRequest; +import org.springframework.web.bind.support.WebDataBinderFactory; +import org.springframework.web.context.request.NativeWebRequest; +import org.springframework.web.context.request.ServletWebRequest; +import org.springframework.web.method.support.HandlerMethodArgumentResolver; +import org.springframework.web.method.support.ModelAndViewContainer; + +/** + * Unit tests for {@link PersistentEntityResourceHandlerMethodArgumentResolver}. + * + * @author Oliver Gierke + */ +public class PersistentEntityResourceHandlerMethodArgumentResolverUnitTests { + + HttpMessageConverter converter; + RootResourceInformationHandlerMethodArgumentResolver rootResourceResolver; + BackendIdHandlerMethodArgumentResolver backendIdResolver; + DomainObjectReader reader; + + @Before + public void setUp() throws Exception { + + this.converter = mock(HttpMessageConverter.class); + when(this.converter.canRead((Class) any(), (MediaType) any())).thenReturn(true); + + this.rootResourceResolver = mock(RootResourceInformationHandlerMethodArgumentResolver.class); + setupRootResourceInfoFor(Foo.class); + + this.backendIdResolver = mock(BackendIdHandlerMethodArgumentResolver.class); + this.reader = mock(DomainObjectReader.class); + } + + @Test // DATAREST-1050 + @SuppressWarnings("unchecked") + public void returnsAggregateInstanceWithIdentifierPopulatedForPutRequests() throws Exception { + + PersistentEntityResourceHandlerMethodArgumentResolver argumentResolver = new PersistentEntityResourceHandlerMethodArgumentResolver( + Arrays.> asList(converter), rootResourceResolver, backendIdResolver, reader); + + HttpServletRequest request = new MockHttpServletRequest("PUT", "/foo/4711"); + + doReturn(new Foo()).when(converter).read(Mockito.any(Class.class), Mockito.any(HttpInputMessage.class)); + mockInvocationOfResolver(backendIdResolver, "4711"); + + Object result = argumentResolver.resolveArgument(null, null, new ServletWebRequest(request), null); + + assertThat(result).isInstanceOfSatisfying(PersistentEntityResource.class, it -> { + assertThat(it.getContent()).isInstanceOfSatisfying(Foo.class, foo -> assertThat(foo.id).isEqualTo(4711L)); + }); + } + + private void setupRootResourceInfoFor(Class type) throws Exception { + + RootResourceInformation information = mock(RootResourceInformation.class); + + doReturn(type).when(information).getDomainType(); + mockInvocationOfResolver(rootResourceResolver, information); + + KeyValueMappingContext context = new KeyValueMappingContext<>(); + KeyValuePersistentEntity entity = context.getRequiredPersistentEntity(Foo.class); + + doReturn(entity).when(information).getPersistentEntity(); + doReturn(mock(RepositoryInvoker.class)).when(information).getInvoker(); + } + + private static void mockInvocationOfResolver(HandlerMethodArgumentResolver resolver, Object result) throws Exception { + + doReturn(result).when(resolver).resolveArgument((MethodParameter) any(), // + (ModelAndViewContainer) any(), // + (NativeWebRequest) any(), // + (WebDataBinderFactory) any()); + } + + static class Foo { + @Id Long id; + } +}