From 429706a77ebd3bc6101a16ac730bc081f0fd33d4 Mon Sep 17 00:00:00 2001 From: Oliver Gierke Date: Thu, 12 Jul 2018 10:30:54 +0200 Subject: [PATCH] DATAREST-1260 - Tweaked DomainObjectReader to support immutable entities. DomainObjectReader now aborts the recursive merges in case it encounters an entity that's described as @Immutable (just introduced in Spring Data Commons). Fixed some generics and removed DomainObjectMerger as it's unused. Related ticket: DATACMNS-1322. --- .../rest/core/support/DomainObjectMerger.java | 156 ------------------ .../data/rest/core/domain/Person.java | 10 +- .../core/support/DomainObjectMergerTests.java | 90 ---------- .../support/DomainObjectMergerUnitTests.java | 55 ------ .../data/rest/webmvc/HttpHeadersPreparer.java | 2 +- .../RepositoryRestMvcConfiguration.java | 12 -- .../rest/webmvc/json/DomainObjectReader.java | 34 ++-- .../json/DomainObjectReaderUnitTests.java | 8 +- 8 files changed, 37 insertions(+), 330 deletions(-) delete mode 100644 spring-data-rest-core/src/main/java/org/springframework/data/rest/core/support/DomainObjectMerger.java delete mode 100755 spring-data-rest-core/src/test/java/org/springframework/data/rest/core/support/DomainObjectMergerTests.java delete mode 100755 spring-data-rest-core/src/test/java/org/springframework/data/rest/core/support/DomainObjectMergerUnitTests.java diff --git a/spring-data-rest-core/src/main/java/org/springframework/data/rest/core/support/DomainObjectMerger.java b/spring-data-rest-core/src/main/java/org/springframework/data/rest/core/support/DomainObjectMerger.java deleted file mode 100644 index f804e881a..000000000 --- a/spring-data-rest-core/src/main/java/org/springframework/data/rest/core/support/DomainObjectMerger.java +++ /dev/null @@ -1,156 +0,0 @@ -/* - * Copyright 2012-2018 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.core.support; - -import static org.springframework.data.rest.core.support.DomainObjectMerger.NullHandlingPolicy.*; - -import org.springframework.beans.factory.annotation.Autowired; -import org.springframework.core.convert.ConversionService; -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.model.ConvertingPropertyAccessor; -import org.springframework.data.repository.support.Repositories; -import org.springframework.util.Assert; -import org.springframework.util.ObjectUtils; - -/** - * Component to be able to merge the first level of two objects. - * - * @author Jon Brisbin - * @author Oliver Gierke - * @author Willie Wheeler - */ -public class DomainObjectMerger { - - private final Repositories repositories; - private final ConversionService conversionService; - - /** - * Creates a new {@link DomainObjectMerger} for the given {@link Repositories} and {@link ConversionService}. - * - * @param repositories must not be {@literal null}. - * @param conversionService must not be {@literal null}. - */ - @Autowired - public DomainObjectMerger(Repositories repositories, ConversionService conversionService) { - - Assert.notNull(repositories, "Repositories must not be null!"); - Assert.notNull(conversionService, "ConversionService must not be null!"); - - this.repositories = repositories; - this.conversionService = conversionService; - } - - /** - * Merges the given target object into the source one. - * - * @param from can be {@literal null}. - * @param target can be {@literal null}. - * @param nullPolicy how to handle {@literal null} values in the source object. - */ - public void merge(Object from, Object target, final NullHandlingPolicy nullPolicy) { - - if (from == null || target == null) { - return; - } - - final PersistentEntity sourceEntity = repositories.getPersistentEntity(from.getClass()); - final PersistentPropertyAccessor sourceWrapper = new ConvertingPropertyAccessor( - sourceEntity.getPropertyAccessor(from), conversionService); - final PersistentEntity targetEntity = repositories.getPersistentEntity(from.getClass()); - final PersistentPropertyAccessor targetWrapper = new ConvertingPropertyAccessor( - targetEntity.getPropertyAccessor(target), conversionService); - - targetEntity.doWithProperties(new SimplePropertyHandler() { - - /* - * (non-Javadoc) - * @see org.springframework.data.mapping.SimplePropertyHandler#doWithPersistentProperty(org.springframework.data.mapping.PersistentProperty) - */ - @Override - public void doWithPersistentProperty(PersistentProperty persistentProperty) { - - Object sourceValue = sourceWrapper.getProperty(persistentProperty); - Object targetValue = targetWrapper.getProperty(persistentProperty); - - if (targetEntity.isIdProperty(persistentProperty)) { - return; - } - - if (sourceValue != null && sourceValue.equals(targetValue)) { - return; - } - - if (nullPolicy == APPLY_NULLS || sourceValue != null) { - targetWrapper.setProperty(persistentProperty, sourceValue); - } - } - }); - - targetEntity.doWithAssociations(new SimpleAssociationHandler() { - - /* - * (non-Javadoc) - * @see org.springframework.data.mapping.SimpleAssociationHandler#doWithAssociation(org.springframework.data.mapping.Association) - */ - @Override - public void doWithAssociation(Association> association) { - - PersistentProperty persistentProperty = association.getInverse(); - Object fromVal = sourceWrapper.getProperty(persistentProperty); - - if (!isNullOrEmpty(fromVal) && !fromVal.equals(targetWrapper.getProperty(persistentProperty))) { - targetWrapper.setProperty(persistentProperty, fromVal); - } - } - }); - } - - /** - * Returns whether the given source is {@literal null} or considered empty, which means it's an {@link Iterable} or - * array and doesn't have any elements. - * - * @param source can be {@literal null}. - * @return - */ - static boolean isNullOrEmpty(Object source) { - - if (source == null) { - return true; - } - - if (source instanceof Iterable) { - return !((Iterable) source).iterator().hasNext(); - } - - if (ObjectUtils.isArray(source)) { - return ObjectUtils.isEmpty((Object[]) source); - } - - return false; - } - - /** - * Strategy to express whether {@literal null} values should be ignored or set on the target domain object. - */ - public static enum NullHandlingPolicy { - APPLY_NULLS, IGNORE_NULLS; - } -} diff --git a/spring-data-rest-core/src/test/java/org/springframework/data/rest/core/domain/Person.java b/spring-data-rest-core/src/test/java/org/springframework/data/rest/core/domain/Person.java index fbb0802e8..4d6b53cb3 100644 --- a/spring-data-rest-core/src/test/java/org/springframework/data/rest/core/domain/Person.java +++ b/spring-data-rest-core/src/test/java/org/springframework/data/rest/core/domain/Person.java @@ -15,7 +15,9 @@ */ package org.springframework.data.rest.core.domain; +import lombok.AccessLevel; import lombok.Data; +import lombok.RequiredArgsConstructor; import java.util.ArrayList; import java.util.Calendar; @@ -24,6 +26,7 @@ import java.util.List; import java.util.UUID; import org.springframework.data.annotation.Id; +import org.springframework.data.annotation.PersistenceConstructor; import org.springframework.data.annotation.Reference; import org.springframework.data.rest.core.annotation.RestResource; @@ -34,11 +37,16 @@ import org.springframework.data.rest.core.annotation.RestResource; * @author Oliver Gierke */ @Data +@RequiredArgsConstructor(access = AccessLevel.PRIVATE, onConstructor = @__(@PersistenceConstructor)) public class Person { - private final @Id UUID id = UUID.randomUUID(); + private final @Id UUID id; private final String firstName, lastName; + public Person(String firstName, String lastName) { + this(UUID.randomUUID(), firstName, lastName); + } + private @Reference List siblings = new ArrayList(); private @RestResource(path = "father-mapped") @Reference Person father; private Date created = Calendar.getInstance().getTime(); diff --git a/spring-data-rest-core/src/test/java/org/springframework/data/rest/core/support/DomainObjectMergerTests.java b/spring-data-rest-core/src/test/java/org/springframework/data/rest/core/support/DomainObjectMergerTests.java deleted file mode 100755 index 7735942dc..000000000 --- a/spring-data-rest-core/src/test/java/org/springframework/data/rest/core/support/DomainObjectMergerTests.java +++ /dev/null @@ -1,90 +0,0 @@ -/* - * Copyright 2014-2018 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.core.support; - -import static org.assertj.core.api.Assertions.*; -import static org.springframework.data.rest.core.support.DomainObjectMerger.NullHandlingPolicy.*; - -import java.util.Arrays; - -import org.junit.Before; -import org.junit.Test; -import org.junit.runner.RunWith; -import org.springframework.beans.factory.annotation.Autowired; -import org.springframework.context.ConfigurableApplicationContext; -import org.springframework.core.convert.support.DefaultConversionService; -import org.springframework.data.repository.support.Repositories; -import org.springframework.data.rest.core.domain.JpaRepositoryConfig; -import org.springframework.data.rest.core.domain.Person; -import org.springframework.test.context.ContextConfiguration; -import org.springframework.test.context.junit4.SpringJUnit4ClassRunner; - -/** - * Integration tests for {@link DomainObjectMerger}. - * - * @author Greg Turnquist - * @author Oliver Gierke - */ -@RunWith(SpringJUnit4ClassRunner.class) -@ContextConfiguration(classes = JpaRepositoryConfig.class) -public class DomainObjectMergerTests { - - @Autowired ConfigurableApplicationContext context; - - DomainObjectMerger merger; - - @Before - public void setUp() { - this.merger = new DomainObjectMerger(new Repositories(context.getBeanFactory()), new DefaultConversionService()); - } - - @Test // DATAREST-130 - public void mergeNewValue() { - - Person incoming = new Person("Bilbo", "Baggins"); - Person existingDomainObject = new Person("Frodo", "Baggins"); - - merger.merge(incoming, existingDomainObject, APPLY_NULLS); - - assertThat(existingDomainObject.getFirstName()).isEqualTo(incoming.getFirstName()); - assertThat(existingDomainObject.getLastName()).isEqualTo(incoming.getLastName()); - } - - @Test // DATAREST-130 - public void mergeNullValue() { - - Person incoming = new Person(null, null); - Person existingDomainObject = new Person("Frodo", "Baggins"); - - merger.merge(incoming, existingDomainObject, APPLY_NULLS); - - assertThat(existingDomainObject.getFirstName()).isEqualTo(incoming.getFirstName()); - assertThat(existingDomainObject.getLastName()).isEqualTo(incoming.getLastName()); - } - - @Test // DATAREST-327 - public void doesNotMergeEmptyCollectionsForReferences() { - - Person bilbo = new Person("Bilbo", "Baggins"); - - Person frodo = new Person("Frodo", "Baggins"); - frodo.setSiblings(Arrays.asList(bilbo)); - - merger.merge(new Person("Sam", null), frodo, IGNORE_NULLS); - - assertThat(frodo.getSiblings()).isNotEmpty(); - } -} diff --git a/spring-data-rest-core/src/test/java/org/springframework/data/rest/core/support/DomainObjectMergerUnitTests.java b/spring-data-rest-core/src/test/java/org/springframework/data/rest/core/support/DomainObjectMergerUnitTests.java deleted file mode 100755 index 628f77627..000000000 --- a/spring-data-rest-core/src/test/java/org/springframework/data/rest/core/support/DomainObjectMergerUnitTests.java +++ /dev/null @@ -1,55 +0,0 @@ -/* - * Copyright 2014-2018 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.core.support; - -import static org.assertj.core.api.Assertions.*; -import static org.springframework.data.rest.core.support.DomainObjectMerger.*; - -import java.util.Collections; -import java.util.Iterator; - -import org.junit.Test; - -/** - * Unit tests for {@link DomainObjectMerger}. - * - * @author Oliver Gierke - */ -public class DomainObjectMergerUnitTests { - - @Test // DATAREST-327 - public void considersEmptyObjectsEmpty() { - - assertThat(isNullOrEmpty(null)).isTrue(); - assertThat(isNullOrEmpty(Collections.emptyList())).isTrue(); - assertThat(isNullOrEmpty(new Object[0])).isTrue(); - assertThat(isNullOrEmpty(new String[0])).isTrue(); - assertThat(isNullOrEmpty(new MyIterable())).isTrue(); - - assertThat(isNullOrEmpty(new Object())).isFalse(); - assertThat(isNullOrEmpty(Collections.singleton(new Object()))).isFalse(); - assertThat(isNullOrEmpty(new Object[] { "1" })).isFalse(); - assertThat(isNullOrEmpty(new String[] { "1" })).isFalse(); - } - - class MyIterable implements Iterable { - - @Override - public Iterator iterator() { - return Collections.emptyList().iterator(); - } - } -} diff --git a/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/HttpHeadersPreparer.java b/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/HttpHeadersPreparer.java index e5777d339..0f73274c6 100644 --- a/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/HttpHeadersPreparer.java +++ b/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/HttpHeadersPreparer.java @@ -112,7 +112,7 @@ public class HttpHeadersPreparer { * @param source can be {@literal null}. * @return */ - private Optional getAuditableBeanWrapper(Object source) { + private Optional> getAuditableBeanWrapper(Object source) { return auditableBeanWrapperFactory.getBeanWrapperFor(source); } diff --git a/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/config/RepositoryRestMvcConfiguration.java b/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/config/RepositoryRestMvcConfiguration.java index ed555780f..66064ca1a 100644 --- a/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/config/RepositoryRestMvcConfiguration.java +++ b/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/config/RepositoryRestMvcConfiguration.java @@ -71,7 +71,6 @@ import org.springframework.data.rest.core.mapping.RepositoryResourceMappings; import org.springframework.data.rest.core.mapping.ResourceDescription; import org.springframework.data.rest.core.mapping.ResourceMappings; import org.springframework.data.rest.core.support.DefaultSelfLinkProvider; -import org.springframework.data.rest.core.support.DomainObjectMerger; import org.springframework.data.rest.core.support.EntityLookup; import org.springframework.data.rest.core.support.RepositoryRelProvider; import org.springframework.data.rest.core.support.SelfLinkProvider; @@ -346,17 +345,6 @@ public class RepositoryRestMvcConfiguration extends HateoasAwareSpringDataWebCon return new AnnotatedEventHandlerInvoker(); } - /** - * For merging incoming objects materialized from JSON with existing domain objects loaded from the repository. - * - * @return - * @throws Exception - */ - @Bean - public DomainObjectMerger domainObjectMerger() throws Exception { - return new DomainObjectMerger(repositories(), defaultConversionService()); - } - /** * Turns an {@link javax.servlet.http.HttpServletRequest} into a * {@link org.springframework.http.server.ServerHttpRequest}. 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 d38c08d99..2899b9818 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 @@ -138,20 +138,22 @@ public class DomainObjectReader { Class type = target.getClass(); - return entities.getPersistentEntity(type).map(it -> { + return entities.getPersistentEntity(type) // + .filter(it -> !it.isImmutable()) // + .map(it -> { - MergingPropertyHandler propertyHandler = new MergingPropertyHandler(source, target, it, mapper); + MergingPropertyHandler propertyHandler = new MergingPropertyHandler(source, target, it, mapper); - it.doWithProperties(propertyHandler); - it.doWithAssociations(new LinkedAssociationSkippingAssociationHandler(associationLinks, propertyHandler)); + it.doWithProperties(propertyHandler); + it.doWithAssociations(new LinkedAssociationSkippingAssociationHandler(associationLinks, propertyHandler)); - // Need to copy unmapped properties as the PersistentProperty model currently does not contain any transient - // properties - copyRemainingProperties(propertyHandler.getProperties(), source, target); + // Need to copy unmapped properties as the PersistentProperty model currently does not contain any transient + // properties + copyRemainingProperties(propertyHandler.getProperties(), source, target); - return target; + return target; - }).orElse(source); + }).orElse(source); } /** @@ -229,7 +231,7 @@ public class DomainObjectReader { } PersistentProperty property = mappedProperties.getPersistentProperty(fieldName); - PersistentPropertyAccessor accessor = entity.getPropertyAccessor(target); + PersistentPropertyAccessor accessor = entity.getPropertyAccessor(target); Optional rawValue = Optional.ofNullable(accessor.getProperty(property)); if (!rawValue.isPresent() || associationLinks.isLinkableAssociation(property)) { @@ -596,8 +598,8 @@ public class DomainObjectReader { private class MergingPropertyHandler implements SimplePropertyHandler { private final @Getter MappedProperties properties; - private final PersistentPropertyAccessor targetAccessor; - private final PersistentPropertyAccessor sourceAccessor; + private final PersistentPropertyAccessor targetAccessor; + private final PersistentPropertyAccessor sourceAccessor; private final ObjectMapper mapper; /** @@ -617,7 +619,7 @@ public class DomainObjectReader { Assert.notNull(mapper, "ObjectMapper must not be null!"); this.properties = MappedProperties.fromJacksonProperties(entity, mapper); - this.targetAccessor = new ConvertingPropertyAccessor(entity.getPropertyAccessor(target), + this.targetAccessor = new ConvertingPropertyAccessor<>(entity.getPropertyAccessor(target), new DefaultConversionService()); this.sourceAccessor = entity.getPropertyAccessor(source); this.mapper = mapper; @@ -639,6 +641,12 @@ public class DomainObjectReader { } Optional sourceValue = Optional.ofNullable(sourceAccessor.getProperty(property)); + + if (property.isImmutable()) { + targetAccessor.setProperty(property, sourceValue.orElse(null)); + return; + } + Optional targetValue = Optional.ofNullable(targetAccessor.getProperty(property)); Optional result = Optional.empty(); 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 ff5723d71..6a7169ce4 100755 --- 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 @@ -23,6 +23,7 @@ import static org.mockito.Mockito.*; import lombok.AllArgsConstructor; import lombok.EqualsAndHashCode; +import lombok.Getter; import lombok.NoArgsConstructor; import lombok.RequiredArgsConstructor; import lombok.Value; @@ -51,6 +52,7 @@ import org.mockito.Mock; import org.mockito.junit.MockitoJUnitRunner; import org.springframework.data.annotation.CreatedDate; import org.springframework.data.annotation.Id; +import org.springframework.data.annotation.Immutable; import org.springframework.data.annotation.ReadOnlyProperty; import org.springframework.data.annotation.Reference; import org.springframework.data.annotation.Transient; @@ -734,11 +736,13 @@ public class DomainObjectReaderUnitTests { List enums = new ArrayList(); } - @Value + @EqualsAndHashCode + @AllArgsConstructor static class SampleWithReference { - @Reference List nested; + private @Getter @Reference List nested; } + @Immutable @Value static class Nested { int x, y;