DATAREST-1440 - Revisited removal of fields in incoming payloads.

We now only remove fields from the payload in case there's no @JsonAnySetter on the target entity.

Author: odrotbohm

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

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

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

@@ -26,17 +26,16 @@
 import java.util.Map;
 import java.util.Optional;
 import java.util.Set;
-import java.util.function.Predicate;
 
 import org.springframework.data.mapping.PersistentEntity;
 import org.springframework.data.mapping.PersistentProperty;
 import org.springframework.util.Assert;
 
+import com.fasterxml.jackson.annotation.JsonAnySetter;
 import com.fasterxml.jackson.databind.BeanDescription;
 import com.fasterxml.jackson.databind.DeserializationConfig;
 import com.fasterxml.jackson.databind.ObjectMapper;
 import com.fasterxml.jackson.databind.SerializationConfig;
-import com.fasterxml.jackson.databind.introspect.BasicClassIntrospector;
 import com.fasterxml.jackson.databind.introspect.BeanPropertyDefinition;
 import com.fasterxml.jackson.databind.introspect.ClassIntrospector;
 
@@ -50,39 +49,45 @@
 @RequiredArgsConstructor(access = AccessLevel.PRIVATE)
 class MappedProperties {
 
-	private static final ClassIntrospector INTROSPECTOR = new BasicClassIntrospector();
-
 	private final Map<PersistentProperty<?>, BeanPropertyDefinition> propertyToFieldName;
 	private final Map<String, PersistentProperty<?>> fieldNameToProperty;
 	private final Set<BeanPropertyDefinition> unmappedProperties;
+	private final Set<String> ignoredPropertyNames;
+	private final boolean anySetterFound;
 
 	/**
 	 * Creates a new {@link MappedProperties} instance for the given {@link PersistentEntity} and {@link BeanDescription}.
 	 *
 	 * @param entity must not be {@literal null}.
 	 * @param description must not be {@literal null}.
 	 */
-	private MappedProperties(PersistentEntity<?, ? extends PersistentProperty<?>> entity, BeanDescription description,
-			Predicate<PersistentProperty<?>> filter) {
+	private MappedProperties(PersistentEntity<?, ? extends PersistentProperty<?>> entity, BeanDescription description) {
 
 		Assert.notNull(entity, "Entity must not be null!");
 		Assert.notNull(description, "BeanDescription must not be null!");
 
-		this.propertyToFieldName = new HashMap<PersistentProperty<?>, BeanPropertyDefinition>();
-		this.fieldNameToProperty = new HashMap<String, PersistentProperty<?>>();
-		this.unmappedProperties = new HashSet<BeanPropertyDefinition>();
+		this.propertyToFieldName = new HashMap<>();
+		this.fieldNameToProperty = new HashMap<>();
+		this.unmappedProperties = new HashSet<>();
+
+		this.anySetterFound = description.findAnySetterAccessor() != null;
+
+		// We need to call this method after findAnySetterAccessor above as that triggers the
+		// collection of ignored properties in the first place. See
+		// https://github.com/FasterXML/jackson-databind/issues/2531
+
+		this.ignoredPropertyNames = description.getIgnoredPropertyNames();
 
 		for (BeanPropertyDefinition property : description.findProperties()) {
 
-			if (description.getIgnoredPropertyNames().contains(property.getName())) {
+			if (ignoredPropertyNames.contains(property.getName())) {
 				continue;
 			}
 
 			Optional<? extends PersistentProperty<?>> persistentProperty = //
 					Optional.ofNullable(entity.getPersistentProperty(property.getInternalName()));
 
-			persistentProperty//
-					.filter(filter) //
+			persistentProperty //
 					.ifPresent(it -> {
 						propertyToFieldName.put(it, property);
 						fieldNameToProperty.put(property.getName(), it);
@@ -105,10 +110,11 @@ private MappedProperties(PersistentEntity<?, ? extends PersistentProperty<?>> en
 	public static MappedProperties forDeserialization(PersistentEntity<?, ?> entity, ObjectMapper mapper) {
 
 		DeserializationConfig config = mapper.getDeserializationConfig();
-		BeanDescription description = INTROSPECTOR.forDeserialization(config, mapper.constructType(entity.getType()),
+		ClassIntrospector introspector = config.getClassIntrospector();
+		BeanDescription description = introspector.forDeserialization(config, mapper.constructType(entity.getType()),
 				config);
 
-		return new MappedProperties(entity, description, it -> it.isWritable());
+		return new MappedProperties(entity, description);
 	}
 
 	/**
@@ -122,13 +128,15 @@ public static MappedProperties forDeserialization(PersistentEntity<?, ?> entity,
 	public static MappedProperties forSerialization(PersistentEntity<?, ?> entity, ObjectMapper mapper) {
 
 		SerializationConfig config = mapper.getSerializationConfig();
-		BeanDescription description = INTROSPECTOR.forSerialization(config, mapper.constructType(entity.getType()), config);
+		ClassIntrospector introspector = config.getClassIntrospector();
+		BeanDescription description = introspector.forSerialization(config, mapper.constructType(entity.getType()), config);
 
-		return new MappedProperties(entity, description, it -> true);
+		return new MappedProperties(entity, description);
 	}
 
 	public static MappedProperties none() {
-		return new MappedProperties(Collections.emptyMap(), Collections.emptyMap(), Collections.emptySet());
+		return new MappedProperties(Collections.emptyMap(), Collections.emptyMap(), Collections.emptySet(),
+				Collections.emptySet(), false);
 	}
 
 	/**
@@ -196,4 +204,24 @@ public boolean isMappedProperty(PersistentProperty<?> property) {
 
 		return propertyToFieldName.containsKey(property);
 	}
+
+	/**
+	 * Returns whether the property is actually writable. I.e. whether there's a non-read-only property on the target type
+	 * or there's a catch all method annotated with {@link JsonAnySetter}.
+	 *
+	 * @param name must not be {@literal null} or empty.
+	 * @return
+	 */
+	public boolean isWritableProperty(String name) {
+
+		Assert.hasText(name, "Property name must not be null or empty!");
+
+		if (ignoredPropertyNames.contains(name)) {
+			return false;
+		}
+
+		PersistentProperty<?> property = fieldNameToProperty.get(name);
+
+		return property != null ? property.isWritable() : anySetterFound;
+	}
 }

spring-data-rest-webmvc/src/test/java/org/springframework/data/rest/webmvc/json/MappedPropertiesUnitTests.java

@@ -23,6 +23,7 @@
 import org.springframework.data.keyvalue.core.mapping.context.KeyValueMappingContext;
 import org.springframework.data.mapping.PersistentEntity;
 
+import com.fasterxml.jackson.annotation.JsonAnySetter;
 import com.fasterxml.jackson.annotation.JsonIgnore;
 import com.fasterxml.jackson.annotation.JsonProperty;
 import com.fasterxml.jackson.annotation.JsonProperty.Access;
@@ -90,7 +91,7 @@ public void doesNotRegardReadOnlyPropertyForDeserialization() {
 
 		MappedProperties properties = MappedProperties.forDeserialization(entity, mapper);
 
-		assertThat(properties.hasPersistentPropertyForField("anotherReadOnlyProperty")).isFalse();
+		assertThat(properties.isWritableProperty("anotherReadOnlyProperty")).isFalse();
 		assertThat(properties.getPersistentProperty("readOnlyProperty")).isNull();
 
 		properties = MappedProperties.forSerialization(entity, mapper);
@@ -99,6 +100,21 @@ public void doesNotRegardReadOnlyPropertyForDeserialization() {
 		assertThat(properties.getPersistentProperty("readOnlyProperty")).isNotNull();
 	}
 
+	@Test // DATAREST-1440
+	public void exposesExistanceOfCatchAllMethod() {
+
+		PersistentEntity<?, ?> entity = context.getRequiredPersistentEntity(SampleWithJsonAnySetter.class);
+
+		MappedProperties properties = MappedProperties.forDeserialization(entity, mapper);
+
+		assertThat(properties.isWritableProperty("someProperty")).isTrue();
+		assertThat(properties.isWritableProperty("readOnlyProperty")).isFalse();
+		assertThat(properties.isWritableProperty("anotherReadOnlyProperty")).isFalse();
+
+		// Due to @JsonAnySetter
+		assertThat(properties.isWritableProperty("someRandomProperty")).isTrue();
+	}
+
 	static class Sample {
 
 		public @Transient String notExposedBySpringData;
@@ -108,4 +124,14 @@ static class Sample {
 		public @JsonProperty(access = Access.READ_ONLY) String readOnlyProperty;
 		public @ReadOnlyProperty String anotherReadOnlyProperty;
 	}
+
+	static class SampleWithJsonAnySetter {
+
+		public String someProperty;
+		public @JsonProperty(access = Access.READ_ONLY) String readOnlyProperty;
+		public @ReadOnlyProperty String anotherReadOnlyProperty;
+
+		@JsonAnySetter
+		public void set(String key, String value) {}
+	}
 }