Improve JSON Patch implementation.

Refactor JSON Patch application implementation to improve the property detection for which values are supposed to be set.

Fixes #2177.

Author: odrotbohm

spring-data-rest-tests/spring-data-rest-tests-mongodb/src/test/java/org/springframework/data/rest/webmvc/config/JsonPatchHandlerUnitTests.java

@@ -19,6 +19,8 @@
 import static org.mockito.Mockito.*;
 import static org.springframework.data.rest.tests.mongodb.TestUtils.*;
 
+import lombok.Data;
+
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collections;
@@ -36,21 +38,28 @@
 import org.springframework.data.rest.tests.mongodb.Address;
 import org.springframework.data.rest.tests.mongodb.User;
 import org.springframework.data.rest.webmvc.RestMediaTypes;
+import org.springframework.data.rest.webmvc.json.BindContextFactory;
 import org.springframework.data.rest.webmvc.json.DomainObjectReader;
+import org.springframework.data.rest.webmvc.json.PersistentEntitiesBindContextFactory;
+import org.springframework.data.rest.webmvc.json.patch.PatchException;
 import org.springframework.data.rest.webmvc.mapping.Associations;
 import org.springframework.http.converter.HttpMessageNotReadableException;
 
+import com.fasterxml.jackson.annotation.JsonIgnore;
+import com.fasterxml.jackson.annotation.JsonIgnoreProperties;
 import com.fasterxml.jackson.databind.ObjectMapper;
 
 /**
  * Unit tests for {@link JsonPatchHandler}.
  *
  * @author Oliver Gierke
+ * @author Mark Paluch
  */
 @ExtendWith(MockitoExtension.class)
 class JsonPatchHandlerUnitTests {
 
 	JsonPatchHandler handler;
+	ObjectMapper mapper = new ObjectMapper();
 	User user;
 
 	@Mock ResourceMappings mappings;
@@ -63,12 +72,13 @@ void setUp() {
 		MongoMappingContext context = new MongoMappingContext();
 		context.setSimpleTypeHolder(conversions.getSimpleTypeHolder());
 		context.getPersistentEntity(User.class);
+		context.getPersistentEntity(WithIgnoredProperties.class);
 
 		PersistentEntities entities = new PersistentEntities(Arrays.asList(context));
-
 		Associations associations = new Associations(mappings, mock(RepositoryRestConfiguration.class));
+		BindContextFactory factory = new PersistentEntitiesBindContextFactory(entities);
 
-		this.handler = new JsonPatchHandler(new ObjectMapper(), new DomainObjectReader(entities, associations));
+		this.handler = new JsonPatchHandler(factory, new DomainObjectReader(entities, associations));
 
 		Address address = new Address();
 		address.street = "Foo";
@@ -86,7 +96,7 @@ void appliesRemoveOperationCorrectly() throws Exception {
 		String input = "[{ \"op\": \"replace\", \"path\": \"/address/zipCode\", \"value\": \"ZIP\" },"
 				+ "{ \"op\": \"remove\", \"path\": \"/lastname\" }]";
 
-		User result = handler.applyPatch(asStream(input), user);
+		User result = handler.applyPatch(asStream(input), user, mapper);
 
 		assertThat(result.lastname).isNull();
 		assertThat(result.address.zipCode).isEqualTo("ZIP");
@@ -97,7 +107,7 @@ void appliesMergePatchCorrectly() throws Exception {
 
 		String input = "{ \"address\" : { \"zipCode\" : \"ZIP\"}, \"lastname\" : null }";
 
-		User result = handler.applyMergePatch(asStream(input), user);
+		User result = handler.applyMergePatch(asStream(input), user, mapper);
 
 		assertThat(result.lastname).isNull();
 		assertThat(result.address.zipCode).isEqualTo("ZIP");
@@ -119,7 +129,7 @@ void removesArrayItemCorrectly() throws Exception {
 
 		String input = "[{ \"op\": \"remove\", \"path\": \"/colleagues/0\" }]";
 
-		handler.applyPatch(asStream(input), user);
+		handler.applyPatch(asStream(input), user, mapper);
 
 		assertThat(user.colleagues).hasSize(1);
 		assertThat(user.colleagues.get(0).firstname).isEqualTo(christoph.firstname);
@@ -129,7 +139,49 @@ void removesArrayItemCorrectly() throws Exception {
 	void hintsToMediaTypeIfBodyCantBeRead() throws Exception {
 
 		assertThatExceptionOfType(HttpMessageNotReadableException.class)
-				.isThrownBy(() -> handler.applyPatch(asStream("{ \"foo\" : \"bar\" }"), new User()))
+				.isThrownBy(() -> handler.applyPatch(asStream("{ \"foo\" : \"bar\" }"), new User(), mapper))
 				.withMessageContaining(RestMediaTypes.JSON_PATCH_JSON.toString());
 	}
+
+	@Test
+	void skipsReplaceConditionally() throws Exception {
+
+		WithIgnoredProperties object = new WithIgnoredProperties();
+		assertThatExceptionOfType(PatchException.class).isThrownBy(() -> {
+			handler.applyPatch(asStream("[{ \"op\": \"replace\", \"path\": \"/password\", \"value\": \"hello\" }]"), object,
+					mapper);
+		});
+
+		WithIgnoredProperties result = handler
+				.applyPatch(asStream("[{ \"op\": \"replace\", \"path\": \"/name\", \"value\": \"hello\" }]"), object, mapper);
+
+		assertThat(result.name).isEqualTo("hello");
+	}
+
+	@Test
+	void skipsCopyConditionally() throws Exception {
+
+		WithIgnoredProperties object = new WithIgnoredProperties();
+		object.setName("hello");
+
+		assertThatExceptionOfType(PatchException.class).isThrownBy(() -> {
+			handler.applyPatch(asStream("[{ \"op\": \"copy\", \"path\": \"/password\", \"from\": \"/name\" }]"), object,
+					mapper);
+		});
+
+		WithIgnoredProperties result = handler
+				.applyPatch(asStream("[{ \"op\": \"copy\", \"path\": \"/lastname\", \"from\": \"/name\" }]"), object, mapper);
+
+		assertThat(result.lastname).isEqualTo("hello");
+	}
+
+	@JsonIgnoreProperties("password")
+	@Data
+	static class WithIgnoredProperties {
+
+		String name, lastname, password;
+
+		@JsonIgnore String ssn;
+
+	}
 }

spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/config/JsonPatchHandler.java

@@ -19,14 +19,15 @@
 
 import org.springframework.data.rest.webmvc.IncomingRequest;
 import org.springframework.data.rest.webmvc.RestMediaTypes;
+import org.springframework.data.rest.webmvc.json.BindContextFactory;
 import org.springframework.data.rest.webmvc.json.DomainObjectReader;
+import org.springframework.data.rest.webmvc.json.patch.BindContext;
 import org.springframework.data.rest.webmvc.json.patch.JsonPatchPatchConverter;
 import org.springframework.data.rest.webmvc.json.patch.Patch;
 import org.springframework.data.rest.webmvc.util.InputStreamHttpInputMessage;
 import org.springframework.http.converter.HttpMessageNotReadableException;
 import org.springframework.util.Assert;
 
-import com.fasterxml.jackson.annotation.JsonInclude.Include;
 import com.fasterxml.jackson.databind.ObjectMapper;
 import com.fasterxml.jackson.databind.node.ObjectNode;
 
@@ -44,26 +45,23 @@
  */
 class JsonPatchHandler {
 
-	private final ObjectMapper mapper;
-	private final ObjectMapper sourceMapper;
+	private final BindContextFactory factory;
 	private final DomainObjectReader reader;
 
 	/**
-	 * Creates a new {@link JsonPatchHandler} with the given {@link ObjectMapper} and {@link DomainObjectReader}.
+	 * Creates a new {@link JsonPatchHandler} with the given {@link JacksonBindContextFactory} and
+	 * {@link DomainObjectReader}.
 	 *
-	 * @param mapper must not be {@literal null}.
+	 * @param factory must not be {@literal null}.
 	 * @param reader must not be {@literal null}.
 	 */
-	public JsonPatchHandler(ObjectMapper mapper, DomainObjectReader reader) {
+	public JsonPatchHandler(BindContextFactory factory, DomainObjectReader reader) {
 
-		Assert.notNull(mapper, "ObjectMapper must not be null!");
-		Assert.notNull(reader, "DomainObjectReader must not be null!");
+		Assert.notNull(factory, "BindContextFactory must not be null");
+		Assert.notNull(reader, "DomainObjectReader must not be null");
 
-		this.mapper = mapper;
+		this.factory = factory;
 		this.reader = reader;
-
-		this.sourceMapper = mapper.copy();
-		this.sourceMapper.setSerializationInclusion(Include.NON_NULL);
 	}
 
 	/**
@@ -74,43 +72,48 @@ public JsonPatchHandler(ObjectMapper mapper, DomainObjectReader reader) {
 	 * @return
 	 * @throws Exception
 	 */
-	public <T> T apply(IncomingRequest request, T target) throws Exception {
+	public <T> T apply(IncomingRequest request, T target, ObjectMapper mapper) throws Exception {
 
 		Assert.notNull(request, "Request must not be null!");
 		Assert.isTrue(request.isPatchRequest(), "Cannot handle non-PATCH request!");
 		Assert.notNull(target, "Target must not be null!");
 
 		if (request.isJsonPatchRequest()) {
-			return applyPatch(request.getBody(), target);
+			return applyPatch(request.getBody(), target, mapper);
 		} else {
-			return applyMergePatch(request.getBody(), target);
+			return applyMergePatch(request.getBody(), target, mapper);
 		}
 	}
 
 	@SuppressWarnings("unchecked")
-	<T> T applyPatch(InputStream source, T target) throws Exception {
-		return getPatchOperations(source).apply(target, (Class<T>) target.getClass());
+	<T> T applyPatch(InputStream source, T target, ObjectMapper mapper) throws Exception {
+
+		Class<?> type = target.getClass();
+		BindContext context = factory.getBindContextFor(mapper);
+
+		return getPatchOperations(source, mapper, context).apply(target, (Class<T>) target.getClass());
 	}
 
-	<T> T applyMergePatch(InputStream source, T existingObject) throws Exception {
+	<T> T applyMergePatch(InputStream source, T existingObject, ObjectMapper mapper) throws Exception {
 		return reader.read(source, existingObject, mapper);
 	}
 
-	<T> T applyPut(ObjectNode source, T existingObject) throws Exception {
+	<T> T applyPut(ObjectNode source, T existingObject, ObjectMapper mapper) throws Exception {
 		return reader.readPut(source, existingObject, mapper);
 	}
 
 	/**
 	 * Returns all {@link JsonPatchOperation}s to be applied.
 	 *
 	 * @param source must not be {@literal null}.
+	 * @param mapper must not be {@literal null}.
 	 * @return
 	 * @throws HttpMessageNotReadableException in case the payload can't be read.
 	 */
-	private Patch getPatchOperations(InputStream source) {
+	private Patch getPatchOperations(InputStream source, ObjectMapper mapper, BindContext context) {
 
 		try {
-			return new JsonPatchPatchConverter(mapper).convert(mapper.readTree(source));
+			return new JsonPatchPatchConverter(mapper, context).convert(mapper.readTree(source));
 		} catch (Exception o_O) {
 			throw new HttpMessageNotReadableException(
 					String.format("Could not read PATCH operations! Expected %s!", RestMediaTypes.JSON_PATCH_JSON), o_O,

spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/config/PersistentEntityResourceHandlerMethodArgumentResolver.java

@@ -36,6 +36,7 @@
 import org.springframework.data.rest.webmvc.PersistentEntityResource.Builder;
 import org.springframework.data.rest.webmvc.ResourceNotFoundException;
 import org.springframework.data.rest.webmvc.RootResourceInformation;
+import org.springframework.data.rest.webmvc.json.BindContextFactory;
 import org.springframework.data.rest.webmvc.json.DomainObjectReader;
 import org.springframework.data.rest.webmvc.support.BackendIdHandlerMethodArgumentResolver;
 import org.springframework.http.MediaType;
@@ -69,15 +70,15 @@ public class PersistentEntityResourceHandlerMethodArgumentResolver implements Ha
 	private final List<HttpMessageConverter<?>> messageConverters;
 	private final RootResourceInformationHandlerMethodArgumentResolver resourceInformationResolver;
 	private final BackendIdHandlerMethodArgumentResolver idResolver;
-	private final DomainObjectReader reader;
 	private final PluginRegistry<EntityLookup<?>, Class<?>> lookups;
 	private final ConversionService conversionService = new DefaultConversionService();
+	private final JsonPatchHandler jsonPatchHandler;
 
 	public PersistentEntityResourceHandlerMethodArgumentResolver(
 			List<HttpMessageConverter<?>> messageConverters,
 			RootResourceInformationHandlerMethodArgumentResolver resourceInformationResolver,
 			BackendIdHandlerMethodArgumentResolver idResolver, DomainObjectReader reader,
-			PluginRegistry<EntityLookup<?>, Class<?>> lookups) {
+			PluginRegistry<EntityLookup<?>, Class<?>> lookups, BindContextFactory factory) {
 
 		Assert.notNull(messageConverters, "HttpMessageConverters must not be null!");
 		Assert.notNull(resourceInformationResolver, "RootResourceInformation resolver must not be null!");
@@ -88,8 +89,8 @@ public PersistentEntityResourceHandlerMethodArgumentResolver(
 		this.messageConverters = messageConverters;
 		this.resourceInformationResolver = resourceInformationResolver;
 		this.idResolver = idResolver;
-		this.reader = reader;
 		this.lookups = lookups;
+		this.jsonPatchHandler = new JsonPatchHandler(mapper -> factory.getBindContextFor(mapper), reader);
 	}
 
 	/*
@@ -210,8 +211,7 @@ private Object readPatch(IncomingRequest request, ObjectMapper mapper, Object ex
 
 		try {
 
-			JsonPatchHandler handler = new JsonPatchHandler(mapper, reader);
-			return handler.apply(request, existingObject);
+			return jsonPatchHandler.apply(request, existingObject, mapper);
 
 		} catch (Exception o_O) {
 
@@ -228,10 +228,9 @@ private Object readPutForUpdate(IncomingRequest request, ObjectMapper mapper, Ob
 
 		try {
 
-			JsonPatchHandler handler = new JsonPatchHandler(mapper, reader);
 			JsonNode jsonNode = mapper.readTree(request.getBody());
 
-			return handler.applyPut((ObjectNode) jsonNode, existingObject);
+			return jsonPatchHandler.applyPut((ObjectNode) jsonNode, existingObject, mapper);
 
 		} catch (Exception o_O) {
 			throw new HttpMessageNotReadableException(String.format(ERROR_MESSAGE, existingObject.getClass()), o_O,

spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/config/RepositoryRestMvcConfiguration.java

@@ -495,13 +495,15 @@ public PersistentEntityResourceHandlerMethodArgumentResolver persistentEntityArg
 			@Qualifier("defaultMessageConverters") List<HttpMessageConverter<?>> defaultMessageConverters,
 			RootResourceInformationHandlerMethodArgumentResolver repoRequestArgumentResolver, Associations associationLinks,
 			BackendIdHandlerMethodArgumentResolver backendIdHandlerMethodArgumentResolver,
-			PersistentEntities persistentEntities) {
+			PersistentEntities entities) {
 
 		PluginRegistry<EntityLookup<?>, Class<?>> lookups = PluginRegistry.of(getEntityLookups());
+		DomainObjectReader reader = new DomainObjectReader(entities, associationLinks);
+		BindContextFactory factory = new PersistentEntitiesBindContextFactory(entities);
 
 		return new PersistentEntityResourceHandlerMethodArgumentResolver(defaultMessageConverters,
 				repoRequestArgumentResolver, backendIdHandlerMethodArgumentResolver,
-				new DomainObjectReader(persistentEntities, associationLinks), lookups);
+				reader, lookups, factory);
 	}
 
 	/**

spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/json/BindContextFactory.java

@@ -0,0 +1,36 @@
+/*
+ * Copyright 2022 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
+ *
+ *      https://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.json;
+
+import org.springframework.data.rest.webmvc.json.patch.BindContext;
+
+import com.fasterxml.jackson.databind.ObjectMapper;
+
+/**
+ * Factory to create {@link BindContext} instances.
+ *
+ * @author Oliver Drotbohm
+ */
+public interface BindContextFactory {
+
+	/**
+	 * Creates a {@link BindContext} for the given {@link ObjectMapper}.
+	 *
+	 * @param mapper must not be {@literal null}.
+	 * @return will never be {@literal null}.
+	 */
+	BindContext getBindContextFor(ObjectMapper mapper);
+}

spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/json/DomainObjectReader.java

@@ -246,7 +246,7 @@ <T> T doMerge(ObjectNode root, T target, ObjectMapper mapper) throws Exception {
 			JsonNode child = entry.getValue();
 			String fieldName = entry.getKey();
 
-			if (!mappedProperties.isWritableProperty(fieldName)) {
+			if (!mappedProperties.isWritableField(fieldName)) {
 
 				i.remove();
 				continue;