From c1ad9b73ba399814bd438d49ede8d24eef37e289 Mon Sep 17 00:00:00 2001 From: Stephane Nicoll Date: Wed, 31 Jan 2018 17:48:55 +0100 Subject: [PATCH] Allow caching for an Endpoint operation with optional arguments This commit makes sure that caching is enabled if an operation has nullable parameters and the actual invocation provides null values. Closes gh-11795 --- .../cache/CachingOperationInvoker.java | 15 +++++++++ .../cache/CachingOperationInvokerAdvisor.java | 12 ++++++- .../CachingOperationInvokerAdvisorTests.java | 26 +++++++++++++-- .../cache/CachingOperationInvokerTests.java | 32 +++++++++++++++++-- 4 files changed, 78 insertions(+), 7 deletions(-) diff --git a/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/endpoint/invoker/cache/CachingOperationInvoker.java b/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/endpoint/invoker/cache/CachingOperationInvoker.java index 96960b75a4..c284f940e8 100644 --- a/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/endpoint/invoker/cache/CachingOperationInvoker.java +++ b/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/endpoint/invoker/cache/CachingOperationInvoker.java @@ -20,6 +20,7 @@ import java.util.Map; import org.springframework.boot.actuate.endpoint.invoke.OperationInvoker; import org.springframework.util.Assert; +import org.springframework.util.ObjectUtils; /** * An {@link OperationInvoker} that caches the response of an operation with a @@ -58,6 +59,9 @@ public class CachingOperationInvoker implements OperationInvoker { @Override public Object invoke(Map arguments) { + if (hasArgument(arguments)) { + return this.invoker.invoke(arguments); + } long accessTime = System.currentTimeMillis(); CachedResponse cached = this.cachedResponse; if (cached == null || cached.isStale(accessTime, this.timeToLive)) { @@ -68,6 +72,17 @@ public class CachingOperationInvoker implements OperationInvoker { return cached.getResponse(); } + private boolean hasArgument(Map arguments) { + if (!ObjectUtils.isEmpty(arguments)) { + for (Object value : arguments.values()) { + if (value != null) { + return true; + } + } + } + return false; + } + /** * Apply caching configuration when appropriate to the given invoker. * @param invoker the invoker to wrap diff --git a/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/endpoint/invoker/cache/CachingOperationInvokerAdvisor.java b/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/endpoint/invoker/cache/CachingOperationInvokerAdvisor.java index b0f02f6095..639930472d 100644 --- a/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/endpoint/invoker/cache/CachingOperationInvokerAdvisor.java +++ b/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/endpoint/invoker/cache/CachingOperationInvokerAdvisor.java @@ -21,6 +21,7 @@ import java.util.function.Function; import org.springframework.boot.actuate.endpoint.OperationType; import org.springframework.boot.actuate.endpoint.invoke.OperationInvoker; import org.springframework.boot.actuate.endpoint.invoke.OperationInvokerAdvisor; +import org.springframework.boot.actuate.endpoint.invoke.OperationParameter; import org.springframework.boot.actuate.endpoint.invoke.OperationParameters; /** @@ -40,7 +41,7 @@ public class CachingOperationInvokerAdvisor implements OperationInvokerAdvisor { @Override public OperationInvoker apply(String endpointId, OperationType operationType, OperationParameters parameters, OperationInvoker invoker) { - if (operationType == OperationType.READ && !parameters.hasParameters()) { + if (operationType == OperationType.READ && !hasMandatoryParameter(parameters)) { Long timeToLive = this.endpointIdTimeToLive.apply(endpointId); if (timeToLive != null && timeToLive > 0) { return new CachingOperationInvoker(invoker, timeToLive); @@ -49,4 +50,13 @@ public class CachingOperationInvokerAdvisor implements OperationInvokerAdvisor { return invoker; } + private boolean hasMandatoryParameter(OperationParameters parameters) { + for (OperationParameter parameter : parameters) { + if (!parameter.isNullable()) { + return true; + } + } + return false; + } + } diff --git a/spring-boot-project/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/endpoint/invoker/cache/CachingOperationInvokerAdvisorTests.java b/spring-boot-project/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/endpoint/invoker/cache/CachingOperationInvokerAdvisorTests.java index cbedccee0a..6811ecf3ea 100644 --- a/spring-boot-project/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/endpoint/invoker/cache/CachingOperationInvokerAdvisorTests.java +++ b/spring-boot-project/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/endpoint/invoker/cache/CachingOperationInvokerAdvisorTests.java @@ -28,6 +28,7 @@ import org.springframework.boot.actuate.endpoint.OperationType; import org.springframework.boot.actuate.endpoint.invoke.OperationInvoker; import org.springframework.boot.actuate.endpoint.invoke.OperationParameters; import org.springframework.boot.actuate.endpoint.invoke.reflect.OperationMethod; +import org.springframework.lang.Nullable; import org.springframework.test.util.ReflectionTestUtils; import org.springframework.util.ReflectionUtils; @@ -40,6 +41,7 @@ import static org.mockito.Mockito.verify; * Tests for {@link CachingOperationInvokerAdvisor}. * * @author Phillip Webb + * @author Stephane Nicoll */ public class CachingOperationInvokerAdvisorTests { @@ -66,8 +68,9 @@ public class CachingOperationInvokerAdvisorTests { } @Test - public void applyWhenHasParametersShouldNotAddAdvise() { - OperationParameters parameters = getParameters("getWithParameter", String.class); + public void applyWhenHasAtLeaseOneMandatoryParameterShouldNotAddAdvise() { + OperationParameters parameters = getParameters("getWithParameter", String.class, + String.class); OperationInvoker advised = this.advisor.apply("foo", OperationType.READ, parameters, this.invoker); assertThat(advised).isSameAs(this.invoker); @@ -97,6 +100,18 @@ public class CachingOperationInvokerAdvisorTests { public void applyShouldAddCacheAdvise() { OperationParameters parameters = getParameters("get"); given(this.timeToLive.apply(any())).willReturn(100L); + assertAdviseIsApplied(parameters); + } + + @Test + public void applyWithAllOptionalParameterShouldAddAdvise() { + OperationParameters parameters = getParameters("getWithAllOptionalParameters", + String.class, String.class); + given(this.timeToLive.apply(any())).willReturn(100L); + assertAdviseIsApplied(parameters); + } + + private void assertAdviseIsApplied(OperationParameters parameters) { OperationInvoker advised = this.advisor.apply("foo", OperationType.READ, parameters, this.invoker); assertThat(advised).isInstanceOf(CachingOperationInvoker.class); @@ -123,7 +138,12 @@ public class CachingOperationInvokerAdvisorTests { return ""; } - public String getWithParameter(String foo) { + public String getWithParameter(@Nullable String foo, String bar) { + return ""; + } + + public String getWithAllOptionalParameters(@Nullable String foo, + @Nullable String bar) { return ""; } diff --git a/spring-boot-project/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/endpoint/invoker/cache/CachingOperationInvokerTests.java b/spring-boot-project/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/endpoint/invoker/cache/CachingOperationInvokerTests.java index dbfe0eb052..d8a37c228c 100644 --- a/spring-boot-project/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/endpoint/invoker/cache/CachingOperationInvokerTests.java +++ b/spring-boot-project/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/endpoint/invoker/cache/CachingOperationInvokerTests.java @@ -16,6 +16,7 @@ package org.springframework.boot.actuate.endpoint.invoker.cache; +import java.util.Collections; import java.util.HashMap; import java.util.Map; @@ -50,10 +51,21 @@ public class CachingOperationInvokerTests { } @Test - public void cacheInTtlRange() { - Object expected = new Object(); - OperationInvoker target = mock(OperationInvoker.class); + public void cacheInTtlRangeWithNoParameter() { + assertCacheIsUsed(Collections.emptyMap()); + } + + @Test + public void cacheInTtlWithNullParameters() { Map parameters = new HashMap<>(); + parameters.put("first", null); + parameters.put("second", null); + assertCacheIsUsed(parameters); + } + + private void assertCacheIsUsed(Map parameters) { + OperationInvoker target = mock(OperationInvoker.class); + Object expected = new Object(); given(target.invoke(parameters)).willReturn(expected); CachingOperationInvoker invoker = new CachingOperationInvoker(target, 500L); Object response = invoker.invoke(parameters); @@ -64,6 +76,20 @@ public class CachingOperationInvokerTests { verifyNoMoreInteractions(target); } + @Test + public void targetAlwaysInvokedWithArguments() { + OperationInvoker target = mock(OperationInvoker.class); + Map parameters = new HashMap<>(); + parameters.put("test", "value"); + parameters.put("something", null); + given(target.invoke(parameters)).willReturn(new Object()); + CachingOperationInvoker invoker = new CachingOperationInvoker(target, 500L); + invoker.invoke(parameters); + invoker.invoke(parameters); + invoker.invoke(parameters); + verify(target, times(3)).invoke(parameters); + } + @Test public void targetInvokedWhenCacheExpires() throws InterruptedException { OperationInvoker target = mock(OperationInvoker.class);