From 1c74a1a0fe8553223a635b5752eb2f174e3e6a38 Mon Sep 17 00:00:00 2001 From: Stephane Nicoll Date: Mon, 20 Feb 2017 15:58:04 +0100 Subject: [PATCH] Improve allowNullValue handling when a null value is provided This commit improves `AbstractValueAdaptingCache` to throw a dedicated exception if `allowNullValues` is `false` and a `null` value is provided anyway. This avoid a lower-level exception from the cache library that will miss some context. Issue: SPR-15173 --- .../caffeine/CaffeineCacheManagerTests.java | 9 +--- .../cache/caffeine/CaffeineCacheTests.java | 17 +++++-- .../cache/jcache/JCacheEhCacheApiTests.java | 18 +++++-- .../support/AbstractValueAdaptingCache.java | 11 ++-- .../cache/AbstractCacheTests.java | 4 +- .../AbstractValueAdaptingCacheTests.java | 51 +++++++++++++++++++ .../ConcurrentMapCacheManagerTests.java | 11 +--- .../concurrent/ConcurrentMapCacheTests.java | 20 ++++++-- 8 files changed, 109 insertions(+), 32 deletions(-) create mode 100644 spring-context/src/test/java/org/springframework/cache/AbstractValueAdaptingCacheTests.java diff --git a/spring-context-support/src/test/java/org/springframework/cache/caffeine/CaffeineCacheManagerTests.java b/spring-context-support/src/test/java/org/springframework/cache/caffeine/CaffeineCacheManagerTests.java index ca4fc2cedfb..59b6a0049e7 100644 --- a/spring-context-support/src/test/java/org/springframework/cache/caffeine/CaffeineCacheManagerTests.java +++ b/spring-context-support/src/test/java/org/springframework/cache/caffeine/CaffeineCacheManagerTests.java @@ -102,14 +102,7 @@ public class CaffeineCacheManagerTests { assertEquals("value1", cache1x.get("key1").get()); cache1x.put("key2", 2); assertEquals(2, cache1x.get("key2").get()); - try { - cache1x.put("key3", null); - fail("Should have thrown NullPointerException"); - } - catch (NullPointerException ex) { - // expected - } - + cm.setAllowNullValues(true); Cache cache1y = cm.getCache("c1"); diff --git a/spring-context-support/src/test/java/org/springframework/cache/caffeine/CaffeineCacheTests.java b/spring-context-support/src/test/java/org/springframework/cache/caffeine/CaffeineCacheTests.java index 5ede180c4fb..9dfed63728f 100644 --- a/spring-context-support/src/test/java/org/springframework/cache/caffeine/CaffeineCacheTests.java +++ b/spring-context-support/src/test/java/org/springframework/cache/caffeine/CaffeineCacheTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2015 the original author or authors. + * Copyright 2002-2017 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. @@ -21,6 +21,7 @@ import org.junit.Before; import org.junit.Test; import org.springframework.cache.AbstractCacheTests; +import org.springframework.cache.AbstractValueAdaptingCacheTests; import org.springframework.cache.Cache; import static org.junit.Assert.*; @@ -29,21 +30,31 @@ import static org.junit.Assert.*; * @author Ben Manes * @author Stephane Nicoll */ -public class CaffeineCacheTests extends AbstractCacheTests { +public class CaffeineCacheTests extends AbstractValueAdaptingCacheTests { private com.github.benmanes.caffeine.cache.Cache nativeCache; private CaffeineCache cache; + private CaffeineCache cacheNoNull; + @Before public void setUp() { nativeCache = Caffeine.newBuilder().build(); cache = new CaffeineCache(CACHE_NAME, nativeCache); + com.github.benmanes.caffeine.cache.Cache nativeCacheNoNull + = Caffeine.newBuilder().build(); + cacheNoNull = new CaffeineCache(CACHE_NAME_NO_NULL, nativeCacheNoNull, false); } @Override protected CaffeineCache getCache() { - return cache; + return getCache(true); + } + + @Override + protected CaffeineCache getCache(boolean allowNull) { + return allowNull ? this.cache : this.cacheNoNull; } @Override diff --git a/spring-context-support/src/test/java/org/springframework/cache/jcache/JCacheEhCacheApiTests.java b/spring-context-support/src/test/java/org/springframework/cache/jcache/JCacheEhCacheApiTests.java index bfb5bce39f7..0a495a493ea 100644 --- a/spring-context-support/src/test/java/org/springframework/cache/jcache/JCacheEhCacheApiTests.java +++ b/spring-context-support/src/test/java/org/springframework/cache/jcache/JCacheEhCacheApiTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2015 the original author or authors. + * Copyright 2002-2017 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. @@ -26,11 +26,12 @@ import org.junit.After; import org.junit.Before; import org.springframework.cache.AbstractCacheTests; +import org.springframework.cache.AbstractValueAdaptingCacheTests; /** * @author Stephane Nicoll */ -public class JCacheEhCacheApiTests extends AbstractCacheTests { +public class JCacheEhCacheApiTests extends AbstractValueAdaptingCacheTests { private CacheManager cacheManager; @@ -38,13 +39,19 @@ public class JCacheEhCacheApiTests extends AbstractCacheTests { private JCacheCache cache; + private JCacheCache cacheNoNull; + @Before public void setup() { this.cacheManager = getCachingProvider().getCacheManager(); this.cacheManager.createCache(CACHE_NAME, new MutableConfiguration<>()); + this.cacheManager.createCache(CACHE_NAME_NO_NULL, new MutableConfiguration<>()); this.nativeCache = this.cacheManager.getCache(CACHE_NAME); this.cache = new JCacheCache(this.nativeCache); + Cache nativeCacheNoNull = + this.cacheManager.getCache(CACHE_NAME_NO_NULL); + this.cacheNoNull = new JCacheCache(nativeCacheNoNull, false); } protected CachingProvider getCachingProvider() { @@ -58,10 +65,13 @@ public class JCacheEhCacheApiTests extends AbstractCacheTests { } } - @Override protected JCacheCache getCache() { - return this.cache; + return getCache(true); + } + + @Override protected JCacheCache getCache(boolean allowNull) { + return allowNull ? this.cache : this.cacheNoNull; } @Override diff --git a/spring-context/src/main/java/org/springframework/cache/support/AbstractValueAdaptingCache.java b/spring-context/src/main/java/org/springframework/cache/support/AbstractValueAdaptingCache.java index 82fbd99e83c..a197831c5e5 100644 --- a/spring-context/src/main/java/org/springframework/cache/support/AbstractValueAdaptingCache.java +++ b/spring-context/src/main/java/org/springframework/cache/support/AbstractValueAdaptingCache.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2015 the original author or authors. + * Copyright 2002-2017 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. @@ -95,8 +95,13 @@ public abstract class AbstractValueAdaptingCache implements Cache { * @return the value to store */ protected Object toStoreValue(Object userValue) { - if (this.allowNullValues && userValue == null) { - return NullValue.INSTANCE; + if (userValue == null) { + if (this.allowNullValues) { + return NullValue.INSTANCE; + } + throw new IllegalArgumentException( + String.format("Cache '%s' is configured to not allow null " + + "values but null was provided", getName())); } return userValue; } diff --git a/spring-context/src/test/java/org/springframework/cache/AbstractCacheTests.java b/spring-context/src/test/java/org/springframework/cache/AbstractCacheTests.java index c71f133d790..e72d2f1f85a 100644 --- a/spring-context/src/test/java/org/springframework/cache/AbstractCacheTests.java +++ b/spring-context/src/test/java/org/springframework/cache/AbstractCacheTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2015 the original author or authors. + * Copyright 2002-2017 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. @@ -26,6 +26,8 @@ import org.junit.Rule; import org.junit.Test; import org.junit.rules.ExpectedException; +import org.springframework.cache.support.AbstractValueAdaptingCache; + import static org.hamcrest.core.Is.*; import static org.junit.Assert.*; diff --git a/spring-context/src/test/java/org/springframework/cache/AbstractValueAdaptingCacheTests.java b/spring-context/src/test/java/org/springframework/cache/AbstractValueAdaptingCacheTests.java new file mode 100644 index 00000000000..7d397590ad1 --- /dev/null +++ b/spring-context/src/test/java/org/springframework/cache/AbstractValueAdaptingCacheTests.java @@ -0,0 +1,51 @@ +/* + * Copyright 2002-2017 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.cache; + +import org.junit.Rule; +import org.junit.Test; +import org.junit.rules.ExpectedException; + +import org.springframework.cache.support.AbstractValueAdaptingCache; + +/** + * + * @author Stephane Nicoll + */ +public abstract class AbstractValueAdaptingCacheTests + extends AbstractCacheTests { + + @Rule + public ExpectedException thrown = ExpectedException.none(); + + protected final static String CACHE_NAME_NO_NULL = "testCacheNoNull"; + + protected abstract T getCache(boolean allowNull); + + @Test + public void testCachePutNullValueAllowNullFalse() { + T cache = getCache(false); + String key = createRandomKey(); + + this.thrown.expect(IllegalArgumentException.class); + this.thrown.expectMessage(CACHE_NAME_NO_NULL); + this.thrown.expectMessage( + "is configured to not allow null values but null was provided"); + cache.put(key, null); + } + +} diff --git a/spring-context/src/test/java/org/springframework/cache/concurrent/ConcurrentMapCacheManagerTests.java b/spring-context/src/test/java/org/springframework/cache/concurrent/ConcurrentMapCacheManagerTests.java index 1598b0fa390..92e8754b750 100644 --- a/spring-context/src/test/java/org/springframework/cache/concurrent/ConcurrentMapCacheManagerTests.java +++ b/spring-context/src/test/java/org/springframework/cache/concurrent/ConcurrentMapCacheManagerTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2015 the original author or authors. + * Copyright 2002-2017 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. @@ -104,14 +104,7 @@ public class ConcurrentMapCacheManagerTests { assertEquals("value1", cache1x.get("key1").get()); cache1x.put("key2", 2); assertEquals(2, cache1x.get("key2").get()); - try { - cache1x.put("key3", null); - fail("Should have thrown NullPointerException"); - } - catch (NullPointerException ex) { - // expected - } - + cm.setAllowNullValues(true); Cache cache1y = cm.getCache("c1"); diff --git a/spring-context/src/test/java/org/springframework/cache/concurrent/ConcurrentMapCacheTests.java b/spring-context/src/test/java/org/springframework/cache/concurrent/ConcurrentMapCacheTests.java index 58e44a4cc62..a27ed954350 100644 --- a/spring-context/src/test/java/org/springframework/cache/concurrent/ConcurrentMapCacheTests.java +++ b/spring-context/src/test/java/org/springframework/cache/concurrent/ConcurrentMapCacheTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2016 the original author or authors. + * Copyright 2002-2017 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. @@ -25,7 +25,7 @@ import java.util.concurrent.ConcurrentMap; import org.junit.Before; import org.junit.Test; -import org.springframework.cache.AbstractCacheTests; +import org.springframework.cache.AbstractValueAdaptingCacheTests; import org.springframework.core.serializer.support.SerializationDelegate; import static org.junit.Assert.*; @@ -35,23 +35,35 @@ import static org.junit.Assert.*; * @author Juergen Hoeller * @author Stephane Nicoll */ -public class ConcurrentMapCacheTests extends AbstractCacheTests { +public class ConcurrentMapCacheTests + extends AbstractValueAdaptingCacheTests { protected ConcurrentMap nativeCache; protected ConcurrentMapCache cache; + protected ConcurrentMap nativeCacheNoNull; + + protected ConcurrentMapCache cacheNoNull; + @Before public void setUp() throws Exception { this.nativeCache = new ConcurrentHashMap<>(); this.cache = new ConcurrentMapCache(CACHE_NAME, this.nativeCache, true); + this.nativeCacheNoNull = new ConcurrentHashMap<>(); + this.cacheNoNull = new ConcurrentMapCache(CACHE_NAME_NO_NULL, + this.nativeCacheNoNull, false); this.cache.clear(); } @Override protected ConcurrentMapCache getCache() { - return this.cache; + return getCache(true); + } + + @Override protected ConcurrentMapCache getCache(boolean allowNull) { + return allowNull ? this.cache : this.cacheNoNull; } @Override