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).

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<HttpMessageConverter<?>> 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<Object> objectToUpdate = getObjectToUpdate(id, resourceInformation);
+			Optional<Serializable> id = Optional
+					.ofNullable(idResolver.resolveArgument(parameter, mavContainer, webRequest, binderFactory));
+			Optional<Object> 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<Object> 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<Object> getObjectToUpdate(Serializable id, RootResourceInformation information) {
-
-		if (id == null) {
-			return Optional.empty();
-		}
-
-		return information.getInvoker().invokeFindOne(id);
-	}
 }

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.<HttpMessageConverter<?>> 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;
+	}
+}