From 74ea61c51c225c346af04c4d628f1bb27fca2e58 Mon Sep 17 00:00:00 2001 From: Christoph Strobl Date: Wed, 19 Aug 2015 14:45:44 +0200 Subject: [PATCH] DATAREDIS-421 - Fix NPE when using HashMapper implementations. We now make sure the available HashMapper implementations avoid exposing null values. Prior to this null values would have been added to the converted Hash causing trouble when saving those to redis since hashes are expected to not contain any null values --- build.gradle | 3 +- .../data/redis/hash/BeanUtilsHashMapper.java | 26 +++++++++++-- .../data/redis/hash/JacksonHashMapper.java | 16 ++++++-- .../redis/mapping/AbstractHashMapperTest.java | 39 +++++++++++++++---- .../mapping/BeanUtilsHashMapperTest.java | 7 ++-- .../redis/mapping/JacksonHashMapperTest.java | 12 ++++-- 6 files changed, 81 insertions(+), 22 deletions(-) diff --git a/build.gradle b/build.gradle index 605977a9d..03cffa61f 100644 --- a/build.gradle +++ b/build.gradle @@ -89,7 +89,7 @@ dependencies { // Mappers compile("org.codehaus.jackson:jackson-mapper-asl:$jacksonVersion", optional) - compile("commons-beanutils:commons-beanutils-core:1.8.3", optional) + compile("commons-beanutils:commons-beanutils:1.9.2", optional) compile("com.fasterxml.jackson.core:jackson-core:$fasterXmlJacksonVersion", optional) compile("com.fasterxml.jackson.core:jackson-databind:$fasterXmlJacksonDatabindVersion", optional) @@ -100,6 +100,7 @@ dependencies { testCompile "junit:junit:$junitVersion" testCompile "org.springframework:spring-test:$springVersion" testCompile "org.springframework:spring-jdbc:$springVersion" + testCompile 'org.testinfected.hamcrest-matchers:core-matchers:1.8' testCompile "org.mockito:mockito-core:$mockitoVersion" testCompile("javax.annotation:jsr250-api:1.0", optional) testCompile("com.thoughtworks.xstream:xstream:1.4.4", optional) diff --git a/src/main/java/org/springframework/data/redis/hash/BeanUtilsHashMapper.java b/src/main/java/org/springframework/data/redis/hash/BeanUtilsHashMapper.java index d74602fea..9e87109ae 100644 --- a/src/main/java/org/springframework/data/redis/hash/BeanUtilsHashMapper.java +++ b/src/main/java/org/springframework/data/redis/hash/BeanUtilsHashMapper.java @@ -1,5 +1,5 @@ /* - * Copyright 2011-2013 the original author or authors. + * Copyright 2011-2015 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. @@ -15,7 +15,9 @@ */ package org.springframework.data.redis.hash; +import java.util.LinkedHashMap; import java.util.Map; +import java.util.Map.Entry; import org.apache.commons.beanutils.BeanUtils; @@ -23,6 +25,7 @@ import org.apache.commons.beanutils.BeanUtils; * HashMapper based on Apache Commons BeanUtils project. Does NOT supports nested properties. * * @author Costin Leau + * @author Christoph Strobl */ public class BeanUtilsHashMapper implements HashMapper { @@ -33,6 +36,7 @@ public class BeanUtilsHashMapper implements HashMapper { } public T fromHash(Map hash) { + T instance = org.springframework.beans.BeanUtils.instantiate(type); try { BeanUtils.populate(instance, hash); @@ -42,11 +46,27 @@ public class BeanUtilsHashMapper implements HashMapper { return instance; } + /* + * (non-Javadoc) + * @see org.springframework.data.redis.hash.HashMapper#toHash(java.lang.Object) + */ + @Override public Map toHash(T object) { try { - return BeanUtils.describe(object); + + Map map = BeanUtils.describe(object); + + Map result = new LinkedHashMap(); + for (Entry entry : map.entrySet()) { + if (entry.getValue() != null) { + result.put(entry.getKey(), entry.getValue()); + } + } + + return result; + } catch (Exception ex) { - throw new IllegalArgumentException("Cannot describe object " + object); + throw new IllegalArgumentException("Cannot describe object " + object, ex); } } } diff --git a/src/main/java/org/springframework/data/redis/hash/JacksonHashMapper.java b/src/main/java/org/springframework/data/redis/hash/JacksonHashMapper.java index 0a67d341b..ee62a4e6b 100644 --- a/src/main/java/org/springframework/data/redis/hash/JacksonHashMapper.java +++ b/src/main/java/org/springframework/data/redis/hash/JacksonHashMapper.java @@ -15,17 +15,19 @@ */ package org.springframework.data.redis.hash; +import java.util.Map; + import org.codehaus.jackson.map.ObjectMapper; +import org.codehaus.jackson.map.annotate.JsonSerialize.Inclusion; import org.codehaus.jackson.map.type.TypeFactory; import org.codehaus.jackson.type.JavaType; -import java.util.Map; - /** - * Mapper based on Jackson library. Supports nested properties (rich objects). + * {@link HashMapper} based on Jackson library. Supports nested properties (rich objects). * * @author Costin Leau * @author Thomas Darimont + * @author Christoph Strobl */ public class JacksonHashMapper implements HashMapper { @@ -34,11 +36,19 @@ public class JacksonHashMapper implements HashMapper { private final JavaType mapType = TypeFactory.defaultInstance() .constructMapType(Map.class, String.class, Object.class); + /** + * Creates new {@link JacksonHashMapper}. + * + * @param type + */ public JacksonHashMapper(Class type) { + this(type, new ObjectMapper()); + mapper.getSerializationConfig().setSerializationInclusion(Inclusion.NON_NULL); } public JacksonHashMapper(Class type, ObjectMapper mapper) { + this.mapper = mapper; this.userType = TypeFactory.defaultInstance().constructType(type); } diff --git a/src/test/java/org/springframework/data/redis/mapping/AbstractHashMapperTest.java b/src/test/java/org/springframework/data/redis/mapping/AbstractHashMapperTest.java index 26f89a27d..9474ea9cf 100644 --- a/src/test/java/org/springframework/data/redis/mapping/AbstractHashMapperTest.java +++ b/src/test/java/org/springframework/data/redis/mapping/AbstractHashMapperTest.java @@ -1,5 +1,5 @@ /* - * Copyright 2011-2013 the original author or authors. + * Copyright 2011-2015 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. @@ -19,6 +19,10 @@ import static org.junit.Assert.*; import java.util.Map; +import org.hamcrest.core.IsCollectionContaining; +import org.hamcrest.core.IsEqual; +import org.hamcrest.core.IsNot; +import org.hamcrest.core.IsNull; import org.junit.Test; import org.springframework.data.redis.Address; import org.springframework.data.redis.Person; @@ -26,24 +30,43 @@ import org.springframework.data.redis.hash.HashMapper; /** * @author Costin Leau + * @author Christoph Strobl */ public abstract class AbstractHashMapperTest { - protected abstract HashMapper mapperFor(Class t); - private void test(Object o) { - HashMapper mapper = mapperFor(o.getClass()); + @SuppressWarnings("rawtypes") + protected abstract HashMapper mapperFor(Class t); + + @SuppressWarnings({ "rawtypes", "unchecked" }) + protected void assertBackAndForwardMapping(Object o) { + + HashMapper mapper = mapperFor(o.getClass()); Map hash = mapper.toHash(o); - System.out.println("object hash " + hash.size() + " is " + hash); - assertEquals(o, mapper.fromHash(hash)); + assertThat(mapper.fromHash(hash), IsEqual.equalTo(o)); } @Test public void testSimpleBean() throws Exception { - test(new Address("Broadway", 1)); + assertBackAndForwardMapping(new Address("Broadway", 1)); } @Test public void testNestedBean() throws Exception { - test(new Person("George", "Enescu", 74, new Address("liveni", 19))); + assertBackAndForwardMapping(new Person("George", "Enescu", 74, new Address("liveni", 19))); + } + + /** + * @see DATAREDIS-421 + */ + @Test + public void toHashShouldTreatNullValuesCorrectly() { + + Person source = new Person("rand", null, 19); + + assertBackAndForwardMapping(source); + + assertThat((Iterable) mapperFor(Person.class).toHash(source).values(), + IsNot.not(IsCollectionContaining.hasItems(IsNull.nullValue()))); + } } diff --git a/src/test/java/org/springframework/data/redis/mapping/BeanUtilsHashMapperTest.java b/src/test/java/org/springframework/data/redis/mapping/BeanUtilsHashMapperTest.java index 2dcf4e9ac..235a56c7b 100644 --- a/src/test/java/org/springframework/data/redis/mapping/BeanUtilsHashMapperTest.java +++ b/src/test/java/org/springframework/data/redis/mapping/BeanUtilsHashMapperTest.java @@ -17,19 +17,20 @@ package org.springframework.data.redis.mapping; import org.junit.Test; import org.springframework.data.redis.hash.BeanUtilsHashMapper; -import org.springframework.data.redis.hash.HashMapper; /** * @author Costin Leau + * @author Christoph Strobl */ public class BeanUtilsHashMapperTest extends AbstractHashMapperTest { - protected HashMapper mapperFor(Class t) { - return new BeanUtilsHashMapper(t); + protected BeanUtilsHashMapper mapperFor(Class t) { + return new BeanUtilsHashMapper(t); } @Test(expected = Exception.class) public void testNestedBean() throws Exception { super.testNestedBean(); } + } diff --git a/src/test/java/org/springframework/data/redis/mapping/JacksonHashMapperTest.java b/src/test/java/org/springframework/data/redis/mapping/JacksonHashMapperTest.java index 6cfeca159..50ac5f3a0 100644 --- a/src/test/java/org/springframework/data/redis/mapping/JacksonHashMapperTest.java +++ b/src/test/java/org/springframework/data/redis/mapping/JacksonHashMapperTest.java @@ -1,5 +1,5 @@ /* - * Copyright 2011-2013 the original author or authors. + * Copyright 2011-2015 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. @@ -15,12 +15,16 @@ */ package org.springframework.data.redis.mapping; -import org.springframework.data.redis.hash.HashMapper; import org.springframework.data.redis.hash.JacksonHashMapper; +/** + * @author Costin Leau + * @author Christoph Strobl + */ public class JacksonHashMapperTest extends AbstractHashMapperTest { - protected HashMapper mapperFor(Class t) { - return new JacksonHashMapper(t); + protected JacksonHashMapper mapperFor(Class t) { + return new JacksonHashMapper(t); } + }