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 index ac31b6591..a2fee3d95 100644 --- 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 @@ -112,13 +112,37 @@ public class DomainObjectMerger { PersistentProperty persistentProperty = association.getInverse(); Object fromVal = fromWrapper.getProperty(persistentProperty); - if (fromVal != null && !fromVal.equals(targetWrapper.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. */ 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 index 9307c6335..a5e4842f1 100644 --- 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 @@ -19,16 +19,17 @@ import static org.hamcrest.Matchers.*; import static org.junit.Assert.*; 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.ConversionService; import org.springframework.core.convert.support.DefaultConversionService; import org.springframework.data.repository.support.Repositories; import org.springframework.data.rest.core.domain.jpa.JpaRepositoryConfig; import org.springframework.data.rest.core.domain.jpa.Person; -import org.springframework.data.rest.core.domain.jpa.PersonRepository; import org.springframework.test.context.ContextConfiguration; import org.springframework.test.context.junit4.SpringJUnit4ClassRunner; @@ -42,26 +43,28 @@ import org.springframework.test.context.junit4.SpringJUnit4ClassRunner; @ContextConfiguration(classes = JpaRepositoryConfig.class) public class DomainObjectMergerTests { - @Autowired PersonRepository personRepository; @Autowired ConfigurableApplicationContext context; + DomainObjectMerger merger; + + @Before + public void setUp() { + this.merger = new DomainObjectMerger(new Repositories(context.getBeanFactory()), new DefaultConversionService()); + } + /** * @see DATAREST-130 */ @Test public void mergeNewValue() { - Repositories repositories = new Repositories(context.getBeanFactory()); - ConversionService conversionService = new DefaultConversionService(); - Person incoming = new Person("Bilbo", "Baggins"); Person existingDomainObject = new Person("Frodo", "Baggins"); - DomainObjectMerger merger = new DomainObjectMerger(repositories, conversionService); merger.merge(incoming, existingDomainObject, APPLY_NULLS); - assertThat(existingDomainObject.getFirstName(), equalTo(incoming.getFirstName())); - assertThat(existingDomainObject.getLastName(), equalTo(incoming.getLastName())); + assertThat(existingDomainObject.getFirstName(), is(incoming.getFirstName())); + assertThat(existingDomainObject.getLastName(), is(incoming.getLastName())); } /** @@ -70,16 +73,28 @@ public class DomainObjectMergerTests { @Test public void mergeNullValue() { - Repositories repositories = new Repositories(context.getBeanFactory()); - ConversionService conversionService = new DefaultConversionService(); - Person incoming = new Person(null, null); Person existingDomainObject = new Person("Frodo", "Baggins"); - DomainObjectMerger merger = new DomainObjectMerger(repositories, conversionService); merger.merge(incoming, existingDomainObject, APPLY_NULLS); - assertThat(existingDomainObject.getFirstName(), equalTo(incoming.getFirstName())); - assertThat(existingDomainObject.getLastName(), equalTo(incoming.getLastName())); + assertThat(existingDomainObject.getFirstName(), is(incoming.getFirstName())); + assertThat(existingDomainObject.getLastName(), is(incoming.getLastName())); + } + + /** + * @see DATAREST-327 + */ + @Test + 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(), is(not(emptyIterable()))); } } 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 new file mode 100644 index 000000000..5153b2acd --- /dev/null +++ b/spring-data-rest-core/src/test/java/org/springframework/data/rest/core/support/DomainObjectMergerUnitTests.java @@ -0,0 +1,59 @@ +/* + * Copyright 2014 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.hamcrest.Matchers.*; +import static org.junit.Assert.*; +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 { + + /** + * @see DATAREST-327 + */ + @Test + public void considersEmptyObjectsEmpty() { + + assertThat(isNullOrEmpty(null), is(true)); + assertThat(isNullOrEmpty(Collections.emptyList()), is(true)); + assertThat(isNullOrEmpty(new Object[0]), is(true)); + assertThat(isNullOrEmpty(new String[0]), is(true)); + assertThat(isNullOrEmpty(new MyIterable()), is(true)); + + assertThat(isNullOrEmpty(new Object()), is(false)); + assertThat(isNullOrEmpty(Collections.singleton(new Object())), is(false)); + assertThat(isNullOrEmpty(new Object[] { "1" }), is(false)); + assertThat(isNullOrEmpty(new String[] { "1" }), is(false)); + } + + class MyIterable implements Iterable { + + @Override + public Iterator iterator() { + return Collections.emptyList().iterator(); + } + } +}