From 54ce68d9512955194f1552825955d1529a0bb9a4 Mon Sep 17 00:00:00 2001 From: Ben Manes Date: Wed, 31 Aug 2016 23:10:14 -0700 Subject: [PATCH] Fix jsr107 CacheLoader adapter to handle null values (fixes #119) This is required by the specification (per JavaDoc) but the TCK does not validate it, so sadly I failed to handle that case. To get CI functioning again, moving back to a jdk8-only config. I don't know why the new config hangs with Guava's tests and think it might be the distro. Also fixed the misnamed method for DI factories. I'm going to argue it doesn't break semver since JCache is the API and this is an impl hook no one calls directly. --- .travis.yml | 21 ++-- .travis.yml.jdk9 | 37 ++++++ gradle/dependencies.gradle | 4 +- .../configuration/TypesafeConfigurator.java | 2 +- .../integration/JCacheLoaderAdapter.java | 4 + .../caffeine/jcache/AbstractJCacheTest.java | 19 +-- .../caffeine/jcache/JCacheGuiceTest.java | 2 +- .../jcache/integration/CacheLoaderTest.java | 111 ++++++++++++++++++ 8 files changed, 177 insertions(+), 23 deletions(-) create mode 100644 .travis.yml.jdk9 create mode 100644 jcache/src/test/java/com/github/benmanes/caffeine/jcache/integration/CacheLoaderTest.java diff --git a/.travis.yml b/.travis.yml index 40493ed6f4..b19858f819 100644 --- a/.travis.yml +++ b/.travis.yml @@ -1,21 +1,16 @@ language: java - -dist: trusty -group: edge +sudo: false jdk: - oraclejdk8 - - oraclejdk9 + +env: + - TERM=dumb before_install: - cp gradle.properties.ci gradle.properties - export JAVA_OPTS="-Xmx384m -XX:+UseG1GC -XX:SoftRefLRUPolicyMSPerMB=0 -noverify" -install: - - ./gradlew assemble --stacktrace - -before_script: - - export MAVEN_SKIP_RC=true script: - ./gradlew check - sh -c 'cd examples/write-behind-rxjava && mvn test' @@ -24,8 +19,12 @@ after_success: - ./gradlew coveralls uploadArchives matrix: - allow_failures: - - jdk: oraclejdk9 + fast_finish: true + +addons: + apt: + packages: + - oracle-java8-installer before_cache: - rm -f $HOME/.gradle/caches/modules-2/modules-2.lock diff --git a/.travis.yml.jdk9 b/.travis.yml.jdk9 new file mode 100644 index 0000000000..40493ed6f4 --- /dev/null +++ b/.travis.yml.jdk9 @@ -0,0 +1,37 @@ +language: java + +dist: trusty +group: edge + +jdk: + - oraclejdk8 + - oraclejdk9 + +before_install: + - cp gradle.properties.ci gradle.properties + - export JAVA_OPTS="-Xmx384m -XX:+UseG1GC -XX:SoftRefLRUPolicyMSPerMB=0 -noverify" + +install: + - ./gradlew assemble --stacktrace + +before_script: + - export MAVEN_SKIP_RC=true +script: + - ./gradlew check + - sh -c 'cd examples/write-behind-rxjava && mvn test' + +after_success: +- ./gradlew coveralls uploadArchives + +matrix: + allow_failures: + - jdk: oraclejdk9 + +before_cache: + - rm -f $HOME/.gradle/caches/modules-2/modules-2.lock + - rm -fr $HOME/.gradle/caches/*/plugin-resolution/ +cache: + directories: + - $HOME/.m2 + - $HOME/.gradle/caches/ + - $HOME/.gradle/wrapper/ diff --git a/gradle/dependencies.gradle b/gradle/dependencies.gradle index 725d072ef4..5bfbed958d 100644 --- a/gradle/dependencies.gradle +++ b/gradle/dependencies.gradle @@ -29,7 +29,7 @@ ext { commons_compress: '1.12', commons_lang3: '3.4', config: '1.3.0', - error_prone_annotations: '2.0.11', + error_prone_annotations: '2.0.12', flip_tables: '1.0.2', guava: '19.0', javapoet: '1.7.0', @@ -48,7 +48,7 @@ ext { jcache_tck: '1.0.1', jctools: '1.2.1', junit: '4.12', - mockito: '2.0.111-beta', + mockito: '2.1.0-beta.119', pax_exam: '4.9.1', testng: '6.9.12', truth: '0.24', diff --git a/jcache/src/main/java/com/github/benmanes/caffeine/jcache/configuration/TypesafeConfigurator.java b/jcache/src/main/java/com/github/benmanes/caffeine/jcache/configuration/TypesafeConfigurator.java index e64b0e3526..f7905cc0be 100644 --- a/jcache/src/main/java/com/github/benmanes/caffeine/jcache/configuration/TypesafeConfigurator.java +++ b/jcache/src/main/java/com/github/benmanes/caffeine/jcache/configuration/TypesafeConfigurator.java @@ -93,7 +93,7 @@ public static Optional> from(Config config, S * @param factoryCreator the strategy for creating a factory */ @Inject - public static void setFactoryBuilder(FactoryCreator factoryCreator) { + public static void setFactoryCreator(FactoryCreator factoryCreator) { TypesafeConfigurator.factoryCreator = requireNonNull(factoryCreator); } diff --git a/jcache/src/main/java/com/github/benmanes/caffeine/jcache/integration/JCacheLoaderAdapter.java b/jcache/src/main/java/com/github/benmanes/caffeine/jcache/integration/JCacheLoaderAdapter.java index 4e6e92149c..1e9a14f3a0 100644 --- a/jcache/src/main/java/com/github/benmanes/caffeine/jcache/integration/JCacheLoaderAdapter.java +++ b/jcache/src/main/java/com/github/benmanes/caffeine/jcache/integration/JCacheLoaderAdapter.java @@ -72,6 +72,10 @@ public Expirable load(K key) { long start = statsEnabled ? ticker.read() : 0L; V value = delegate.load(key); + if (value == null) { + return null; + } + dispatcher.publishCreated(cache, key, value); if (statsEnabled) { diff --git a/jcache/src/test/java/com/github/benmanes/caffeine/jcache/AbstractJCacheTest.java b/jcache/src/test/java/com/github/benmanes/caffeine/jcache/AbstractJCacheTest.java index 7c08c3f3f8..6932eea0a6 100644 --- a/jcache/src/test/java/com/github/benmanes/caffeine/jcache/AbstractJCacheTest.java +++ b/jcache/src/test/java/com/github/benmanes/caffeine/jcache/AbstractJCacheTest.java @@ -23,7 +23,6 @@ import javax.cache.CacheManager; import javax.cache.Caching; import javax.cache.integration.CacheLoader; -import javax.cache.integration.CacheLoaderException; import javax.cache.spi.CachingProvider; import org.testng.annotations.AfterMethod; @@ -33,7 +32,6 @@ import com.github.benmanes.caffeine.jcache.configuration.CaffeineConfiguration; import com.github.benmanes.caffeine.jcache.spi.CaffeineCachingProvider; -import com.google.common.base.Functions; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; import com.google.common.collect.Maps; @@ -108,15 +106,20 @@ protected final long currentTimeMillis() { /** The loading configuration used by the test. */ protected CaffeineConfiguration getLoadingConfiguration() { CaffeineConfiguration configuration = getConfiguration(); - configuration.setCacheLoaderFactory(() -> new CacheLoader() { - @Override public Integer load(Integer key) throws CacheLoaderException { + configuration.setCacheLoaderFactory(this::getCacheLoader); + configuration.setReadThrough(true); + return configuration; + } + + /** The cache loader used by the test. */ + protected CacheLoader getCacheLoader() { + return new CacheLoader() { + @Override public Integer load(Integer key) { return key; } @Override public Map loadAll(Iterable keys) { - return Maps.asMap(ImmutableSet.copyOf(keys), Functions.identity()); + return Maps.asMap(ImmutableSet.copyOf(keys), this::load); } - }); - configuration.setReadThrough(true); - return configuration; + }; } } diff --git a/jcache/src/test/java/com/github/benmanes/caffeine/jcache/JCacheGuiceTest.java b/jcache/src/test/java/com/github/benmanes/caffeine/jcache/JCacheGuiceTest.java index 23dc6d453d..c742bbe790 100644 --- a/jcache/src/test/java/com/github/benmanes/caffeine/jcache/JCacheGuiceTest.java +++ b/jcache/src/test/java/com/github/benmanes/caffeine/jcache/JCacheGuiceTest.java @@ -64,7 +64,7 @@ public void beforeMethod() { @AfterClass public void afterClass() { - TypesafeConfigurator.setFactoryBuilder(FactoryBuilder::factoryOf); + TypesafeConfigurator.setFactoryCreator(FactoryBuilder::factoryOf); } @Test diff --git a/jcache/src/test/java/com/github/benmanes/caffeine/jcache/integration/CacheLoaderTest.java b/jcache/src/test/java/com/github/benmanes/caffeine/jcache/integration/CacheLoaderTest.java new file mode 100644 index 0000000000..90d8484af3 --- /dev/null +++ b/jcache/src/test/java/com/github/benmanes/caffeine/jcache/integration/CacheLoaderTest.java @@ -0,0 +1,111 @@ +/* + * Copyright 2016 Ben Manes. All Rights Reserved. + * + * 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 com.github.benmanes.caffeine.jcache.integration; + +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.anEmptyMap; +import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.instanceOf; +import static org.hamcrest.Matchers.is; +import static org.hamcrest.Matchers.nullValue; + +import java.util.Collections; +import java.util.Map; +import java.util.function.Supplier; + +import javax.cache.integration.CacheLoader; +import javax.cache.integration.CacheLoaderException; + +import org.testng.Assert; +import org.testng.annotations.BeforeMethod; +import org.testng.annotations.Test; + +import com.github.benmanes.caffeine.jcache.AbstractJCacheTest; +import com.github.benmanes.caffeine.jcache.configuration.CaffeineConfiguration; +import com.google.common.collect.ImmutableMap; +import com.google.common.collect.ImmutableSet; + +/** + * @author ben.manes@gmail.com (Ben Manes) + */ +public final class CacheLoaderTest extends AbstractJCacheTest { + private Supplier> loadAllSupplier; + private Supplier loadSupplier; + + @BeforeMethod + public void beforeMethod() { + loadSupplier = () -> { throw new UnsupportedOperationException(); }; + loadAllSupplier = () -> { throw new UnsupportedOperationException(); }; + } + + @Test + public void load() { + loadSupplier = () -> -1; + assertThat(jcacheLoading.get(1), is(-1)); + } + + @Test + public void load_null() { + loadSupplier = () -> null; + assertThat(jcacheLoading.get(1), is(nullValue())); + } + + @Test + public void load_failure() { + try { + loadSupplier = () -> { throw new IllegalStateException(); }; + jcacheLoading.get(1); + Assert.fail(); + } catch (CacheLoaderException e) { + assertThat(e.getCause(), instanceOf(IllegalStateException.class)); + } + } + + @Test + public void loadAll() { + loadAllSupplier = () -> ImmutableMap.of(1, -1, 2, -2, 3, -3); + Map result = jcacheLoading.getAll(ImmutableSet.of(1, 2, 3)); + assertThat(result, is(equalTo(loadAllSupplier.get()))); + } + + @Test(expectedExceptions = CacheLoaderException.class) + public void loadAll_null() { + loadAllSupplier = () -> null; + jcacheLoading.getAll(ImmutableSet.of(1, 2, 3)); + } + + @Test + public void loadAll_nullMapping() { + loadAllSupplier = () -> Collections.singletonMap(1, null); + Map result = jcacheLoading.getAll(ImmutableSet.of(1, 2, 3)); + assertThat(result, is(anEmptyMap())); + } + + @Override protected CaffeineConfiguration getConfiguration() { + return new CaffeineConfiguration<>(); + } + + @Override protected CacheLoader getCacheLoader() { + return new CacheLoader() { + @Override public Integer load(Integer key) { + return loadSupplier.get(); + } + @Override public Map loadAll(Iterable keys) { + return loadAllSupplier.get(); + } + }; + } +}