From 0bdcd2ee677d7234450ed16f698df6824671d5e5 Mon Sep 17 00:00:00 2001 From: dreis2211 Date: Tue, 12 Nov 2019 11:34:11 +0100 Subject: [PATCH] Handle ApiVersion in CachingOperationInvoker Prior to this commit, ApiVersion was treated as a mandatory parameter in CachingOperationInvokerAdvisor and thus prevented the CachingOperationInvoker to kick in. By skipping ApiVersion in the same way we're skipping SecurityContext we can avoid this. In order to not return the same cached response, this commit also changes the cache handling in CachingOperationInvoker to account for different ApiVersions being passed. See gh-18961 --- .../cache/CachingOperationInvoker.java | 10 ++++++--- .../cache/CachingOperationInvokerAdvisor.java | 4 +++- .../CachingOperationInvokerAdvisorTests.java | 12 +++++++++++ .../cache/CachingOperationInvokerTests.java | 21 +++++++++++++++++++ 4 files changed, 43 insertions(+), 4 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 e19d68763b1c..51842bfe3775 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 @@ -19,11 +19,13 @@ import java.time.Duration; import java.util.Map; import java.util.Objects; +import java.util.concurrent.ConcurrentHashMap; import reactor.core.publisher.Flux; import reactor.core.publisher.Mono; import org.springframework.boot.actuate.endpoint.InvocationContext; +import org.springframework.boot.actuate.endpoint.http.ApiVersion; import org.springframework.boot.actuate.endpoint.invoke.OperationInvoker; import org.springframework.util.Assert; import org.springframework.util.ClassUtils; @@ -46,7 +48,7 @@ public class CachingOperationInvoker implements OperationInvoker { private final long timeToLive; - private volatile CachedResponse cachedResponse; + private final Map cachedResponses; /** * Create a new instance with the target {@link OperationInvoker} to use to compute @@ -58,6 +60,7 @@ public class CachingOperationInvoker implements OperationInvoker { Assert.isTrue(timeToLive > 0, "TimeToLive must be strictly positive"); this.invoker = invoker; this.timeToLive = timeToLive; + this.cachedResponses = new ConcurrentHashMap<>(); } /** @@ -74,11 +77,12 @@ public Object invoke(InvocationContext context) { return this.invoker.invoke(context); } long accessTime = System.currentTimeMillis(); - CachedResponse cached = this.cachedResponse; + ApiVersion contextApiVersion = context.getApiVersion(); + CachedResponse cached = this.cachedResponses.get(contextApiVersion); if (cached == null || cached.isStale(accessTime, this.timeToLive)) { Object response = this.invoker.invoke(context); cached = createCachedResponse(response, accessTime); - this.cachedResponse = cached; + this.cachedResponses.put(contextApiVersion, cached); } return cached.getResponse(); } 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 b62320b89333..7b0a70da911b 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 org.springframework.boot.actuate.endpoint.EndpointId; import org.springframework.boot.actuate.endpoint.OperationType; import org.springframework.boot.actuate.endpoint.SecurityContext; +import org.springframework.boot.actuate.endpoint.http.ApiVersion; import org.springframework.boot.actuate.endpoint.invoke.OperationInvoker; import org.springframework.boot.actuate.endpoint.invoke.OperationInvokerAdvisor; import org.springframework.boot.actuate.endpoint.invoke.OperationParameter; @@ -54,7 +55,8 @@ public OperationInvoker apply(EndpointId endpointId, OperationType operationType private boolean hasMandatoryParameter(OperationParameters parameters) { for (OperationParameter parameter : parameters) { - if (parameter.isMandatory() && !SecurityContext.class.isAssignableFrom(parameter.getType())) { + if (parameter.isMandatory() && !ApiVersion.class.isAssignableFrom(parameter.getType()) + && !SecurityContext.class.isAssignableFrom(parameter.getType())) { return true; } } 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 9ac20f5da67f..c70db883f023 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 @@ -27,6 +27,7 @@ import org.springframework.boot.actuate.endpoint.EndpointId; import org.springframework.boot.actuate.endpoint.OperationType; import org.springframework.boot.actuate.endpoint.SecurityContext; +import org.springframework.boot.actuate.endpoint.http.ApiVersion; 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; @@ -117,6 +118,13 @@ void applyWithSecurityContextShouldAddAdvise() { assertAdviseIsApplied(parameters); } + @Test + void applyWithApiVersionShouldAddAdvise() { + OperationParameters parameters = getParameters("getWithApiVersion", ApiVersion.class, String.class); + given(this.timeToLive.apply(any())).willReturn(100L); + assertAdviseIsApplied(parameters); + } + private void assertAdviseIsApplied(OperationParameters parameters) { OperationInvoker advised = this.advisor.apply(EndpointId.of("foo"), OperationType.READ, parameters, this.invoker); @@ -152,6 +160,10 @@ String getWithSecurityContext(SecurityContext securityContext, @Nullable String return ""; } + String getWithApiVersion(ApiVersion apiVersion, @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 fabefe5c5d67..94733976cec9 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 @@ -28,6 +28,7 @@ import org.springframework.boot.actuate.endpoint.InvocationContext; import org.springframework.boot.actuate.endpoint.SecurityContext; +import org.springframework.boot.actuate.endpoint.http.ApiVersion; import org.springframework.boot.actuate.endpoint.invoke.MissingParametersException; import org.springframework.boot.actuate.endpoint.invoke.OperationInvoker; @@ -152,6 +153,26 @@ void targetInvokedWhenCacheExpires() throws InterruptedException { verify(target, times(2)).invoke(context); } + @Test + void targetInvokedWithDifferentApiVersion() { + OperationInvoker target = mock(OperationInvoker.class); + Object expectedV2 = new Object(); + Object expectedV3 = new Object(); + InvocationContext contextV2 = new InvocationContext(ApiVersion.V2, mock(SecurityContext.class), + Collections.emptyMap()); + InvocationContext contextV3 = new InvocationContext(ApiVersion.V3, mock(SecurityContext.class), + Collections.emptyMap()); + given(target.invoke(contextV2)).willReturn(expectedV2); + given(target.invoke(contextV3)).willReturn(expectedV3); + CachingOperationInvoker invoker = new CachingOperationInvoker(target, 500L); + Object response = invoker.invoke(contextV2); + assertThat(response).isSameAs(expectedV2); + verify(target, times(1)).invoke(contextV2); + Object cachedResponse = invoker.invoke(contextV3); + assertThat(cachedResponse).isNotSameAs(response); + verify(target, times(1)).invoke(contextV3); + } + private static class MonoOperationInvoker implements OperationInvoker { static int invocations;