From fe75811812e284ab09fc64fbeb65c08684e6fb6a Mon Sep 17 00:00:00 2001 From: Oliver Gierke Date: Thu, 26 Jan 2017 07:39:54 +0100 Subject: [PATCH] DATAREST-944 - DomainObjectMerger now also merges associations for PUT. We now explicitly merge associations skipping the linkable ones. We also try to reuse the existing collections and maps if they're mutable falling back to a completely new one if not. Extracted PropertyHandler and AssociationHandler implementations. --- .../rest/webmvc/json/DomainObjectReader.java | 179 +++++++++++++----- .../json/DomainObjectReaderUnitTests.java | 40 ++++ 2 files changed, 172 insertions(+), 47 deletions(-) diff --git a/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/json/DomainObjectReader.java b/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/json/DomainObjectReader.java index 8935f415a..b25d1b97b 100644 --- a/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/json/DomainObjectReader.java +++ b/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/json/DomainObjectReader.java @@ -15,6 +15,7 @@ */ package org.springframework.data.rest.webmvc.json; +import lombok.Getter; import lombok.NonNull; import lombok.RequiredArgsConstructor; @@ -30,11 +31,12 @@ import java.util.Map.Entry; import org.springframework.beans.PropertyAccessor; import org.springframework.beans.PropertyAccessorFactory; import org.springframework.core.CollectionFactory; -import org.springframework.core.convert.ConversionService; import org.springframework.core.convert.support.DefaultConversionService; +import org.springframework.data.mapping.Association; import org.springframework.data.mapping.PersistentEntity; import org.springframework.data.mapping.PersistentProperty; import org.springframework.data.mapping.PersistentPropertyAccessor; +import org.springframework.data.mapping.SimpleAssociationHandler; import org.springframework.data.mapping.SimplePropertyHandler; import org.springframework.data.mapping.context.PersistentEntities; import org.springframework.data.mapping.model.ConvertingPropertyAccessor; @@ -127,7 +129,7 @@ public class DomainObjectReader { * @param mapper must not be {@literal null}. * @return */ - private T mergeForPut(T source, T target, final ObjectMapper mapper) { + T mergeForPut(T source, T target, final ObjectMapper mapper) { Assert.notNull(mapper, "ObjectMapper must not be null!"); @@ -137,7 +139,7 @@ public class DomainObjectReader { Class type = target.getClass(); - final PersistentEntity entity = entities.getPersistentEntity(type); + PersistentEntity entity = entities.getPersistentEntity(type); if (entity == null) { return source; @@ -145,52 +147,14 @@ public class DomainObjectReader { Assert.notNull(entity, "No PersistentEntity found for ".concat(type.getName()).concat("!")); - final MappedProperties properties = MappedProperties.fromJacksonProperties(entity, mapper); + MergingPropertyHandler propertyHandler = new MergingPropertyHandler(source, target, entity, mapper); - ConversionService conversionService = new DefaultConversionService(); - final PersistentPropertyAccessor targetAccessor = entity.getPropertyAccessor(target); - final ConvertingPropertyAccessor convertingAccessor = new ConvertingPropertyAccessor(targetAccessor, - conversionService); - final PersistentPropertyAccessor sourceAccessor = entity.getPropertyAccessor(source); - - entity.doWithProperties(new SimplePropertyHandler() { - - /* - * (non-Javadoc) - * @see org.springframework.data.mapping.SimplePropertyHandler#doWithPersistentProperty(org.springframework.data.mapping.PersistentProperty) - */ - @Override - public void doWithPersistentProperty(PersistentProperty property) { - - if (property.isIdProperty() || property.isVersionProperty() || !property.isWritable()) { - return; - } - - if (!properties.isMappedProperty(property)) { - return; - } - - Object sourceValue = sourceAccessor.getProperty(property); - Object targetValue = targetAccessor.getProperty(property); - Object result = null; - - if (property.isMap()) { - result = mergeMaps(property, sourceValue, targetValue, mapper); - } else if (property.isCollectionLike()) { - result = mergeCollections(property, sourceValue, targetValue, mapper); - } else if (property.isEntity()) { - result = mergeForPut(sourceValue, targetValue, mapper); - } else { - result = sourceValue; - } - - convertingAccessor.setProperty(property, result); - } - }); + entity.doWithProperties(propertyHandler); + entity.doWithAssociations(new LinkedAssociationSkippingAssociationHandler(associationLinks, propertyHandler)); // Need to copy unmapped properties as the PersistentProperty model currently does not contain any transient // properties - copyRemainingProperties(properties, source, target); + copyRemainingProperties(propertyHandler.getProperties(), source, target); return target; } @@ -460,7 +424,20 @@ public class DomainObjectReader { result.put(entry.getKey(), mergeForPut(entry.getValue(), targetValue, mapper)); } - return result; + if (targetMap == null) { + return result; + } + + try { + + targetMap.clear(); + targetMap.putAll(result); + + return targetMap; + + } catch (UnsupportedOperationException o_O) { + return result; + } } private Collection mergeCollections(PersistentProperty property, Object source, Object target, @@ -489,7 +466,20 @@ public class DomainObjectReader { result.add(mergeForPut(sourceElement, targetElement, mapper)); } - return result; + if (targetCollection == null) { + return result; + } + + try { + + targetCollection.clear(); + targetCollection.addAll(result); + + return targetCollection; + + } catch (UnsupportedOperationException o_O) { + return result; + } } @SuppressWarnings("unchecked") @@ -574,4 +564,99 @@ public class DomainObjectReader { return value.getClass().equals(type.getType()) ? type : ClassTypeInformation.from(value.getClass()); } + + /** + * {@link SimpleAssociationHandler} that skips linkable associations and forwards handling for all other ones to the + * delegate {@link SimplePropertyHandler}. + * + * @author Oliver Gierke + */ + @RequiredArgsConstructor + private final class LinkedAssociationSkippingAssociationHandler implements SimpleAssociationHandler { + + private final @NonNull Associations associations; + private final @NonNull SimplePropertyHandler delegate; + + /* + * (non-Javadoc) + * @see org.springframework.data.mapping.SimpleAssociationHandler#doWithAssociation(org.springframework.data.mapping.Association) + */ + @Override + public void doWithAssociation(Association> association) { + + if (associationLinks.isLinkableAssociation(association)) { + return; + } + + delegate.doWithPersistentProperty(association.getInverse()); + } + } + + /** + * {@link SimplePropertyHandler} to merge the states of the given objects. + * + * @author Oliver Gierke + */ + private class MergingPropertyHandler implements SimplePropertyHandler { + + private final @Getter MappedProperties properties; + private final PersistentPropertyAccessor targetAccessor; + private final PersistentPropertyAccessor sourceAccessor; + private final ObjectMapper mapper; + + /** + * Creates a new {@link MergingPropertyHandler} for the given source, target, {@link PersistentEntity} and + * {@link ObjectMapper}. + * + * @param source must not be {@literal null}. + * @param target must not be {@literal null}. + * @param entity must not be {@literal null}. + * @param mapper must not be {@literal null}. + */ + public MergingPropertyHandler(Object source, Object target, PersistentEntity entity, ObjectMapper mapper) { + + Assert.notNull(source, "Source instance must not be null!"); + Assert.notNull(target, "Target instance must not be null!"); + Assert.notNull(entity, "PersistentEntity must not be null!"); + Assert.notNull(mapper, "ObjectMapper must not be null!"); + + this.properties = MappedProperties.fromJacksonProperties(entity, mapper); + this.targetAccessor = new ConvertingPropertyAccessor(entity.getPropertyAccessor(target), + new DefaultConversionService()); + this.sourceAccessor = entity.getPropertyAccessor(source); + this.mapper = mapper; + } + + /* + * (non-Javadoc) + * @see org.springframework.data.mapping.SimplePropertyHandler#doWithPersistentProperty(org.springframework.data.mapping.PersistentProperty) + */ + @Override + public void doWithPersistentProperty(PersistentProperty property) { + + if (property.isIdProperty() || property.isVersionProperty() || !property.isWritable()) { + return; + } + + if (!properties.isMappedProperty(property)) { + return; + } + + Object sourceValue = sourceAccessor.getProperty(property); + Object targetValue = targetAccessor.getProperty(property); + Object result = null; + + if (property.isMap()) { + result = mergeMaps(property, sourceValue, targetValue, mapper); + } else if (property.isCollectionLike()) { + result = mergeCollections(property, sourceValue, targetValue, mapper); + } else if (property.isEntity()) { + result = mergeForPut(sourceValue, targetValue, mapper); + } else { + result = sourceValue; + } + + targetAccessor.setProperty(property, result); + } + } } diff --git a/spring-data-rest-webmvc/src/test/java/org/springframework/data/rest/webmvc/json/DomainObjectReaderUnitTests.java b/spring-data-rest-webmvc/src/test/java/org/springframework/data/rest/webmvc/json/DomainObjectReaderUnitTests.java index 03b09400f..2527ca50e 100644 --- a/spring-data-rest-webmvc/src/test/java/org/springframework/data/rest/webmvc/json/DomainObjectReaderUnitTests.java +++ b/spring-data-rest-webmvc/src/test/java/org/springframework/data/rest/webmvc/json/DomainObjectReaderUnitTests.java @@ -22,6 +22,7 @@ import static org.mockito.Mockito.*; import lombok.AllArgsConstructor; import lombok.EqualsAndHashCode; import lombok.NoArgsConstructor; +import lombok.Value; import java.io.ByteArrayInputStream; import java.util.ArrayList; @@ -45,6 +46,7 @@ import org.mockito.runners.MockitoJUnitRunner; import org.springframework.data.annotation.CreatedDate; import org.springframework.data.annotation.Id; import org.springframework.data.annotation.ReadOnlyProperty; +import org.springframework.data.annotation.Reference; import org.springframework.data.annotation.Transient; import org.springframework.data.annotation.Version; import org.springframework.data.keyvalue.core.mapping.context.KeyValueMappingContext; @@ -94,6 +96,7 @@ public class DomainObjectReaderUnitTests { mappingContext.getPersistentEntity(Product.class); mappingContext.getPersistentEntity(TransientReadOnlyProperty.class); mappingContext.getPersistentEntity(CollectionOfEnumWithMethods.class); + mappingContext.getPersistentEntity(SampleWithReference.class); mappingContext.afterPropertiesSet(); PersistentEntities entities = new PersistentEntities(Collections.singleton(mappingContext)); @@ -466,6 +469,33 @@ public class DomainObjectReaderUnitTests { assertThat(result.enums, contains(SampleEnum.SECOND, SampleEnum.FIRST)); } + @Test // DATAREST-944 + public void mergesAssociations() { + + List originalCollection = Arrays.asList(new Nested(2, 3)); + SampleWithReference source = new SampleWithReference(Arrays.asList(new Nested(1, 2), new Nested(2, 3))); + SampleWithReference target = new SampleWithReference(originalCollection); + + SampleWithReference result = reader.mergeForPut(source, target, new ObjectMapper()); + + assertThat(result.nested, is(source.nested)); + assertThat(result.nested == originalCollection, is(false)); + } + + @Test // DATAREST-944 + public void mergesAssociationsAndKeepsMutableCollection() { + + ArrayList originalCollection = new ArrayList(Arrays.asList(new Nested(2, 3))); + SampleWithReference source = new SampleWithReference( + new ArrayList(Arrays.asList(new Nested(1, 2), new Nested(2, 3)))); + SampleWithReference target = new SampleWithReference(originalCollection); + + SampleWithReference result = reader.mergeForPut(source, target, new ObjectMapper()); + + assertThat(result.nested, is(source.nested)); + assertThat(result.nested == originalCollection, is(true)); + } + @SuppressWarnings("unchecked") private static T as(Object source, Class type) { @@ -629,4 +659,14 @@ public class DomainObjectReaderUnitTests { static class CollectionOfEnumWithMethods { List enums = new ArrayList(); } + + @Value + static class SampleWithReference { + @Reference List nested; + } + + @Value + static class Nested { + int x, y; + } }