From 53a1fc217afc0e682ef75573e98e0d84e4acbd85 Mon Sep 17 00:00:00 2001 From: John Blum Date: Thu, 25 Feb 2016 12:16:02 -0800 Subject: [PATCH] SGF-474 - Fix the NPE in the MappingPdxSerializer --- .../gemfire/mapping/MappingPdxSerializer.java | 39 ++-- .../MappingPdxSerializerIntegrationTest.java | 25 ++- .../MappingPdxSerializerUnitTests.java | 177 +++++++++++++++--- .../gemfire/repository/sample/Person.java | 14 ++ 4 files changed, 195 insertions(+), 60 deletions(-) diff --git a/src/main/java/org/springframework/data/gemfire/mapping/MappingPdxSerializer.java b/src/main/java/org/springframework/data/gemfire/mapping/MappingPdxSerializer.java index 94303c8f..4bd1dde4 100644 --- a/src/main/java/org/springframework/data/gemfire/mapping/MappingPdxSerializer.java +++ b/src/main/java/org/springframework/data/gemfire/mapping/MappingPdxSerializer.java @@ -151,30 +151,26 @@ public class MappingPdxSerializer implements PdxSerializer, ApplicationContextAw new GemfirePropertyValueProvider(reader), null)); final PersistentPropertyAccessor accessor = new ConvertingPropertyAccessor(entity.getPropertyAccessor(instance), - getConversionService()); + getConversionService()); entity.doWithProperties(new PropertyHandler() { - @Override public void doWithPersistentProperty(GemfirePersistentProperty persistentProperty) { - if (entity.isConstructorArgument(persistentProperty)) { - return; - } + if (!entity.isConstructorArgument(persistentProperty)) { + PdxSerializer customSerializer = getCustomSerializer(persistentProperty.getType()); - PdxSerializer customSerializer = getCustomSerializer(persistentProperty.getType()); - Object value; + Object value = null; - if (customSerializer != null) { - value = customSerializer.fromData(persistentProperty.getType(), reader); - } - else { - value = reader.readField(persistentProperty.getName()); - } + try { + value = (customSerializer != null + ? customSerializer.fromData(persistentProperty.getType(), reader) + : reader.readField(persistentProperty.getName())); - try { - accessor.setProperty(persistentProperty, value); - } - catch (Exception e) { - throw new MappingException("Could not read value " + value.toString(), e); + accessor.setProperty(persistentProperty, value); + } + catch (Exception e) { + throw new MappingException(String.format("Could not read and set value [%1$s] for property [%2$s]", + value, persistentProperty.getName()), e); + } } } }); @@ -190,9 +186,8 @@ public class MappingPdxSerializer implements PdxSerializer, ApplicationContextAw */ @Override public boolean toData(Object value, final PdxWriter writer) { - GemfirePersistentEntity entity = getPersistentEntity(value.getClass()); - + final PersistentPropertyAccessor accessor = new ConvertingPropertyAccessor(entity.getPropertyAccessor(value), getConversionService()); @@ -214,8 +209,8 @@ public class MappingPdxSerializer implements PdxSerializer, ApplicationContextAw } } catch (Exception e) { - throw new MappingException(String.format("Could not write value for property %1$s", - persistentProperty), e); + throw new MappingException(String.format("Could not write value for property [%1$s]", + persistentProperty.getName()), e); } } }); diff --git a/src/test/java/org/springframework/data/gemfire/mapping/MappingPdxSerializerIntegrationTest.java b/src/test/java/org/springframework/data/gemfire/mapping/MappingPdxSerializerIntegrationTest.java index 6c1bd113..f3007704 100644 --- a/src/test/java/org/springframework/data/gemfire/mapping/MappingPdxSerializerIntegrationTest.java +++ b/src/test/java/org/springframework/data/gemfire/mapping/MappingPdxSerializerIntegrationTest.java @@ -37,7 +37,6 @@ import com.gemstone.gemfire.cache.Cache; import com.gemstone.gemfire.cache.CacheFactory; import com.gemstone.gemfire.cache.DataPolicy; import com.gemstone.gemfire.cache.Region; -import com.gemstone.gemfire.cache.RegionFactory; /** * Integration tests for {@link MappingPdxSerializer}. @@ -53,24 +52,24 @@ public class MappingPdxSerializerIntegrationTest { @BeforeClass public static void setUp() { - MappingPdxSerializer serializer = new MappingPdxSerializer(new GemfireMappingContext(), new DefaultConversionService()); - CacheFactory factory = new CacheFactory(); - factory.set("name", MappingPdxSerializer.class.getSimpleName()); - factory.set("mcast-port", "0"); - factory.set("log-level", "warning"); - factory.setPdxSerializer(serializer); - factory.setPdxPersistent(true); - cache = factory.create(); + cache = new CacheFactory() + .set("name", MappingPdxSerializerIntegrationTest.class.getSimpleName()) + .set("mcast-port", "0") + .set("log-level", "warning") + .setPdxSerializer(serializer) + .setPdxPersistent(true) + .create(); - RegionFactory regionFactory = cache.createRegionFactory(); - regionFactory.setDataPolicy(DataPolicy.PERSISTENT_REPLICATE); - region = regionFactory.create("foo"); + region = cache.createRegionFactory() + .setDataPolicy(DataPolicy.PERSISTENT_REPLICATE) + .create("foo"); } @AfterClass + @SuppressWarnings("all") public static void tearDown() { try { cache.close(); @@ -79,7 +78,6 @@ public class MappingPdxSerializerIntegrationTest { } finally { for (String name : new File(".").list(new FilenameFilter() { - @Override public boolean accept(File dir, String name) { return name.startsWith("BACKUP"); } @@ -185,4 +183,5 @@ public class MappingPdxSerializerIntegrationTest { } } + } diff --git a/src/test/java/org/springframework/data/gemfire/mapping/MappingPdxSerializerUnitTests.java b/src/test/java/org/springframework/data/gemfire/mapping/MappingPdxSerializerUnitTests.java index a84564c9..d067c08e 100644 --- a/src/test/java/org/springframework/data/gemfire/mapping/MappingPdxSerializerUnitTests.java +++ b/src/test/java/org/springframework/data/gemfire/mapping/MappingPdxSerializerUnitTests.java @@ -15,17 +15,26 @@ */ package org.springframework.data.gemfire.mapping; +import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.instanceOf; +import static org.hamcrest.Matchers.is; +import static org.hamcrest.Matchers.isA; +import static org.junit.Assert.assertThat; import static org.mockito.Matchers.any; +import static org.mockito.Matchers.anyString; import static org.mockito.Matchers.eq; +import static org.mockito.Mockito.atMost; +import static org.mockito.Mockito.never; import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; -import java.util.HashMap; -import java.util.Map; +import java.util.Collections; import org.junit.Before; +import org.junit.Rule; import org.junit.Test; +import org.junit.rules.ExpectedException; import org.junit.runner.RunWith; import org.mockito.Mock; import org.mockito.runners.MockitoJUnitRunner; @@ -34,15 +43,30 @@ import org.springframework.core.convert.support.GenericConversionService; import org.springframework.data.convert.EntityInstantiator; import org.springframework.data.gemfire.repository.sample.Address; import org.springframework.data.gemfire.repository.sample.Person; +import org.springframework.data.mapping.model.MappingException; import org.springframework.data.mapping.model.ParameterValueProvider; import com.gemstone.gemfire.pdx.PdxReader; import com.gemstone.gemfire.pdx.PdxSerializer; +import com.gemstone.gemfire.pdx.PdxWriter; /** * Unit tests for {@link MappingPdxSerializer}. * * @author Oliver Gierke + * @author John Blum + * @see org.junit.Rule + * @see org.junit.Test + * @see org.junit.rules.ExpectedException + * @see org.junit.runner.RunWith + * @see org.mockito.Mock + * @see org.mockito.Mockito + * @see org.mockito.runners.MockitoJUnitRunner + * @see org.springframework.core.convert.ConversionService + * @see org.springframework.data.gemfire.mapping.MappingPdxSerializer + * @see com.gemstone.gemfire.pdx.PdxReader + * @see com.gemstone.gemfire.pdx.PdxSerializer + * @see com.gemstone.gemfire.pdx.PdxWriter */ @RunWith(MockitoJUnitRunner.class) public class MappingPdxSerializerUnitTests { @@ -53,25 +77,28 @@ public class MappingPdxSerializerUnitTests { MappingPdxSerializer serializer; - @Mock - EntityInstantiator instantiator; + @Rule + public ExpectedException expectedException = ExpectedException.none(); @Mock - PdxReader reader; - + EntityInstantiator mockInstantiator; + @Mock - PdxSerializer addressSerializer; + PdxReader mockReader; + + @Mock + PdxSerializer mockAddressSerializer; + + @Mock + PdxWriter mockWriter; @Before public void setUp() { - conversionService = new GenericConversionService(); context = new GemfireMappingContext(); + conversionService = new GenericConversionService(); serializer = new MappingPdxSerializer(context, conversionService); - - Map,PdxSerializer> customSerializers = new HashMap, PdxSerializer>(); - - customSerializers.put(Address.class, addressSerializer); - serializer.setCustomSerializers(customSerializers); + serializer.setCustomSerializers(Collections., PdxSerializer>singletonMap( + Address.class, mockAddressSerializer)); } @Test @@ -84,22 +111,122 @@ public class MappingPdxSerializerUnitTests { Person person = new Person(1L, "Oliver", "Gierke"); person.address = address; - GemfirePersistentEntity entity = any(GemfirePersistentEntity.class); + when(mockInstantiator.createInstance(any(GemfirePersistentEntity.class), any(ParameterValueProvider.class))) + .thenReturn(person); - ParameterValueProvider provider = any(ParameterValueProvider.class); + serializer.setGemfireInstantiators(Collections., EntityInstantiator>singletonMap( + Person.class, mockInstantiator)); - when(instantiator.createInstance(entity, provider)).thenReturn(person); + serializer.fromData(Person.class, mockReader); - Map, EntityInstantiator> instantiators = new HashMap, EntityInstantiator>(); - - instantiators.put(Person.class, instantiator); - - serializer.setGemfireInstantiators(instantiators); - serializer.fromData(Person.class, reader); - - verify(instantiator, times(1)).createInstance(eq(context.getPersistentEntity(Person.class)), + verify(mockInstantiator, times(1)).createInstance(eq(context.getPersistentEntity(Person.class)), any(ParameterValueProvider.class)); - verify(addressSerializer,times(1)).fromData(eq(Address.class), any(PdxReader.class)); + verify(mockAddressSerializer, times(1)).fromData(eq(Address.class), any(PdxReader.class)); + } + + @Test + public void fromDataMapsPdxDataToApplicationDomainObject() { + Address expectedAddress = new Address(); + expectedAddress.city = "Portland"; + expectedAddress.zipCode = "12345"; + + when(mockInstantiator.createInstance(any(GemfirePersistentEntity.class), any(ParameterValueProvider.class))) + .thenReturn(new Person(null, null, null)); + when(mockReader.readField(eq("id"))).thenReturn(1l); + when(mockReader.readField(eq("firstname"))).thenReturn("Jon"); + when(mockReader.readField(eq("lastname"))).thenReturn("Doe"); + when(mockAddressSerializer.fromData(eq(Address.class), eq(mockReader))).thenReturn(expectedAddress); + + serializer.setGemfireInstantiators(Collections., EntityInstantiator>singletonMap( + Person.class, mockInstantiator)); + + Object obj = serializer.fromData(Person.class, mockReader); + + assertThat(obj, is(instanceOf(Person.class))); + + Person jonDoe = (Person) obj; + + assertThat(jonDoe.getAddress(), is(equalTo(expectedAddress))); + assertThat(jonDoe.getId(), is(equalTo(1l))); + assertThat(jonDoe.getFirstname(), is(equalTo("Jon"))); + assertThat(jonDoe.getLastname(), is(equalTo("Doe"))); + + verify(mockInstantiator, times(1)).createInstance(any(GemfirePersistentEntity.class), any(ParameterValueProvider.class)); + verify(mockReader, times(1)).readField(eq("id")); + verify(mockReader, times(1)).readField(eq("firstname")); + verify(mockReader, times(1)).readField(eq("lastname")); + verify(mockAddressSerializer, times(1)).fromData(eq(Address.class), eq(mockReader)); + } + + @Test + public void fromDataHandlesExceptionProperly() { + when(mockInstantiator.createInstance(any(GemfirePersistentEntity.class), any(ParameterValueProvider.class))) + .thenReturn(new Person(null, null, null)); + when(mockReader.readField(eq("id"))).thenThrow(new IllegalArgumentException("test")); + + serializer.setGemfireInstantiators(Collections., EntityInstantiator>singletonMap( + Person.class, mockInstantiator)); + + try { + expectedException.expect(MappingException.class); + expectedException.expectCause(isA(IllegalArgumentException.class)); + expectedException.expectMessage("Could not read and set value [null] for property [id]"); + + serializer.fromData(Person.class, mockReader); + } + finally { + verify(mockInstantiator, times(1)).createInstance(any(GemfirePersistentEntity.class), any(ParameterValueProvider.class)); + verify(mockReader, times(1)).readField(eq("id")); + } + } + + @Test + public void toDataSerializesApplicationDomainObjectToPdx() { + Address address = new Address(); + address.city = "Portland"; + address.zipCode = "12345"; + + Person jonDoe = new Person(1l, "Jon", "Doe"); + jonDoe.address = address; + + serializer.setCustomSerializers(Collections., PdxSerializer>singletonMap( + Address.class, mockAddressSerializer)); + + assertThat(serializer.toData(jonDoe, mockWriter), is(true)); + + verify(mockAddressSerializer, times(1)).toData(eq(address), eq(mockWriter)); + verify(mockWriter, times(1)).writeField(eq("id"), eq(1l), eq(Long.class)); + verify(mockWriter, times(1)).writeField(eq("firstname"), eq("Jon"), eq(String.class)); + verify(mockWriter, times(1)).writeField(eq("lastname"), eq("Doe"), eq(String.class)); + verify(mockWriter, times(1)).markIdentityField(eq("id")); + } + + @Test + public void toDataHandlesExceptionProperly() { + Address address = new Address(); + address.city = "Portland"; + address.zipCode = "12345"; + + Person jonDoe = new Person(1l, "Jon", "Doe"); + jonDoe.address = address; + + when(mockWriter.writeField(eq("address"), eq(address), eq(Address.class))) + .thenThrow(new IllegalArgumentException("test")); + + try { + expectedException.expect(MappingException.class); + expectedException.expectCause(isA(IllegalArgumentException.class)); + expectedException.expectMessage("Could not write value for property [address]"); + + new MappingPdxSerializer(context, conversionService).toData(jonDoe, mockWriter); + } + finally { + verify(mockWriter, atMost(1)).writeField(eq("id"), eq(1l), eq(Long.class)); + verify(mockWriter, atMost(1)).writeField(eq("firstname"), eq("Jon"), eq(String.class)); + verify(mockWriter, atMost(1)).writeField(eq("lastname"), eq("Doe"), eq(String.class)); + verify(mockWriter, times(1)).writeField(eq("address"), eq(address), eq(Address.class)); + verify(mockWriter, never()).markIdentityField(anyString()); + } } } diff --git a/src/test/java/org/springframework/data/gemfire/repository/sample/Person.java b/src/test/java/org/springframework/data/gemfire/repository/sample/Person.java index 755daa49..3a92fe82 100644 --- a/src/test/java/org/springframework/data/gemfire/repository/sample/Person.java +++ b/src/test/java/org/springframework/data/gemfire/repository/sample/Person.java @@ -18,6 +18,7 @@ package org.springframework.data.gemfire.repository.sample; import java.io.Serializable; import org.springframework.data.annotation.Id; +import org.springframework.data.annotation.PersistenceConstructor; import org.springframework.data.gemfire.mapping.Region; import org.springframework.util.ObjectUtils; @@ -44,6 +45,19 @@ public class Person implements Serializable { public String firstname; public String lastname; + @PersistenceConstructor + public Person() { + } + + public Person(Long id) { + this.id = id; + } + + public Person(String firstName, String lastName) { + this.firstname = firstName; + this.lastname = lastName; + } + public Person(Long id, String firstname, String lastname) { this.id = id; this.firstname = firstname;