Fix cache compatibility with rest-client #13991

gwenneg · 2020-12-20T14:02:41Z

This PR:

Should not change anything in terms of extension behavior (except for the bug it fixes of course): none of the runtime tests were changed.
Removes the interceptor bindings previously added using AnnotationsTransformer because that's what prevented the extension from being compatible with MP REST Client. The former API annotations (@CacheResult, @CacheInvalidate and @CacheInvalidateAll) are now the new interceptor bindings.
Contains a full refactor of all build time validations.

Quarkus (Arc) currently supports repeated caching interceptor bindings (like @CacheInvalidateAll) but this PR does not allow repeated interceptor bindings on a method from a MP REST Client bean. The interceptors implementation from MP REST Client simply ignores repeated interceptor bindings. That's why there's a dedicated check at build time to forbid that combination.

gwenneg · 2020-12-20T14:05:18Z

extensions/cache/runtime/src/main/java/io/quarkus/cache/runtime/CacheInterceptor.java

+            List<Short> positions = new ArrayList<>();
+            for (short i = 0; i < parameters.length; i++) {
+                if (parameters[i].isAnnotationPresent(CacheKey.class)) {
+                    positions.add(i);
+                }
+            }
+            short[] positionsArray = new short[positions.size()];
+            for (int i = 0; i < positions.size(); i++) {
+                positionsArray[i] = positions.get(i);
+            }


I know this is not pretty, but there's a reason behind that: avoid lambdas in runtime code.

gwenneg · 2020-12-20T14:07:40Z

...ache/deployment/src/main/java/io/quarkus/spring/cache/SpringCacheAnnotationsTransformer.java

 public class SpringCacheAnnotationsTransformer implements AnnotationsTransformer {

    private static final Logger LOGGER = Logger.getLogger(SpringCacheAnnotationsTransformer.class);

    private static final DotName CACHE_RESULT_INTERCEPTOR_BINDING = DotName
-            .createSimple(CacheResultInterceptorBinding.class.getName());
+            .createSimple(CacheResult.class.getName());
    private static final DotName CACHE_INVALIDATE_INTERCEPTOR_BINDING = DotName
-            .createSimple(CacheInvalidateInterceptorBinding.class.getName());
+            .createSimple(CacheInvalidate.class.getName());
    private static final DotName CACHE_INVALIDATE_ALL_INTERCEPTOR_BINDING = DotName
-            .createSimple(CacheInvalidateAllInterceptorBinding.class.getName());
+            .createSimple(CacheInvalidateAll.class.getName());


gwenneg · 2020-12-20T15:08:15Z

...ache/deployment/src/test/java/io/quarkus/cache/test/deployment/DeploymentExceptionsTest.java

+                assertUnknownCacheNameException(t, UNKNOWN_CACHE_1);
+                assertUnknownCacheNameException(t, UNKNOWN_CACHE_2);
+                assertUnknownCacheNameException(t, UNKNOWN_CACHE_3);
+            });


Here's a preview of what would be logged if the exceptions were not "caught" by the test:

[...] Caused by: javax.enterprise.inject.spi.DeploymentException: Found 10 deployment problems: [1] @CacheResult is not allowed on a method returning void [class=io.quarkus.cache.test.deployment.DeploymentExceptionsTest$TestResource, method=showThrowVoidReturnTypeTargetException] [2] Caching annotations are not allowed on a class [class=io.quarkus.cache.test.deployment.DeploymentExceptionsTest$TestResource, annotation=io.quarkus.cache.CacheInvalidate] [3] Caching annotations are not allowed on a private method [class=io.quarkus.cache.test.deployment.DeploymentExceptionsTest$TestResource, method=shouldThrowPrivateMethodTargetException, annotation=io.quarkus.cache.CacheInvalidateAll] [4] Caching annotations are not allowed on a private method [class=io.quarkus.cache.test.deployment.DeploymentExceptionsTest$TestResource, method=shouldAlsoThrowPrivateMethodTargetException, annotation=io.quarkus.cache.CacheInvalidate] [5] Caching annotations are not allowed on a private method [class=io.quarkus.cache.test.deployment.DeploymentExceptionsTest$TestResource, method=shouldAlsoThrowPrivateMethodTargetException, annotation=io.quarkus.cache.CacheInvalidate] [6] Caching annotations are not allowed on a class [class=io.quarkus.cache.test.deployment.DeploymentExceptionsTest$TestBean, annotation=io.quarkus.cache.CacheInvalidateAll] [7] Caching annotations are not allowed on a class [class=io.quarkus.cache.test.deployment.DeploymentExceptionsTest$TestBean, annotation=io.quarkus.cache.CacheInvalidateAll] [8] A field or method parameter is annotated with a @CacheName annotation referencing an unknown cache name [class=io.quarkus.cache.test.deployment.DeploymentExceptionsTest$TestBean, cacheName=unknown-cache-1] [9] A field or method parameter is annotated with a @CacheName annotation referencing an unknown cache name [class=io.quarkus.cache.test.deployment.DeploymentExceptionsTest$TestBean, cacheName=unknown-cache-2] [10] A field or method parameter is annotated with a @CacheName annotation referencing an unknown cache name [class=io.quarkus.cache.test.deployment.DeploymentExceptionsTest$TestBean, cacheName=unknown-cache-3]

geoand · 2020-12-21T10:38:17Z

Thanks a lot for this work @gwenneg!

I'd like @mkouba to take a look

gwenneg · 2020-12-30T20:57:21Z

Conflicts solved.

mkouba · 2021-01-04T08:53:58Z

extensions/cache/runtime/src/main/java/io/quarkus/cache/runtime/CacheInterceptionContext.java

+import java.util.ArrayList;
+import java.util.List;
+
+public class CacheInterceptionContext<T extends Annotation> {


It would be nice if this class was immutable... WDYT?

I agree, thanks for the suggestion. It took care of it in #14111.

mkouba · 2021-01-04T08:56:27Z

I added one minor comment, otherwise looks good to me.

gsmet

OK, looks like both @geoand and @mkouba are ok with it so let's merge.

The CI failure is a concurrency issue Gwenneg solved in another PR.

gwenneg · 2021-01-04T16:55:01Z

I was actually planning on applying @mkouba's suggestion about immutability and ask for another review tonight 😃 I'll do it in a separate PR.

gsmet · 2021-01-04T17:31:58Z

Ah I saw you pushed another commit, I thought you fixed it. Sorry :/

gwenneg · 2021-01-04T19:26:58Z

It's no big deal, we're only talking about a code improvement, master isn't broken :)

Thanks @geoand and @mkouba for the reviews!

ghost added area/cache area/spring Issues relating to the Spring integration labels Dec 20, 2020

gwenneg commented Dec 20, 2020

View reviewed changes

gwenneg force-pushed the issue-8221-restclient-cache-compatibility branch from c176af2 to de9cf89 Compare December 20, 2020 15:05

gwenneg commented Dec 20, 2020

View reviewed changes

gwenneg force-pushed the issue-8221-restclient-cache-compatibility branch 2 times, most recently from cbd22e3 to abab11a Compare December 20, 2020 15:50

gwenneg force-pushed the issue-8221-restclient-cache-compatibility branch from abab11a to 17cf713 Compare December 30, 2020 20:56

gwenneg requested a review from mkouba January 3, 2021 09:06

mkouba reviewed Jan 4, 2021

View reviewed changes

gwenneg force-pushed the issue-8221-restclient-cache-compatibility branch from 17cf713 to 981e7b9 Compare January 4, 2021 11:08

Fix cache compatibility with rest-client

1da99b6

gwenneg force-pushed the issue-8221-restclient-cache-compatibility branch from 981e7b9 to 1da99b6 Compare January 4, 2021 11:10

gsmet approved these changes Jan 4, 2021

View reviewed changes

gsmet merged commit 23146f0 into quarkusio:master Jan 4, 2021

ghost added this to the 1.11 - master milestone Jan 4, 2021

gwenneg deleted the issue-8221-restclient-cache-compatibility branch January 4, 2021 19:27

gwenneg mentioned this pull request Jan 5, 2021

Make CacheInterceptionContext immutable #14111

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix cache compatibility with rest-client #13991

Fix cache compatibility with rest-client #13991

gwenneg commented Dec 20, 2020 •

edited

Loading

gwenneg Dec 20, 2020

gwenneg Dec 20, 2020

geoand Dec 21, 2020

gwenneg Dec 20, 2020 •

edited

Loading

geoand commented Dec 21, 2020

gwenneg commented Dec 30, 2020

mkouba Jan 4, 2021

gwenneg Jan 5, 2021

mkouba commented Jan 4, 2021

gsmet left a comment

gwenneg commented Jan 4, 2021

gsmet commented Jan 4, 2021

gwenneg commented Jan 4, 2021

Fix cache compatibility with rest-client #13991

Fix cache compatibility with rest-client #13991

Conversation

gwenneg commented Dec 20, 2020 • edited Loading

gwenneg Dec 20, 2020

Choose a reason for hiding this comment

gwenneg Dec 20, 2020

Choose a reason for hiding this comment

geoand Dec 21, 2020

Choose a reason for hiding this comment

gwenneg Dec 20, 2020 • edited Loading

Choose a reason for hiding this comment

geoand commented Dec 21, 2020

gwenneg commented Dec 30, 2020

mkouba Jan 4, 2021

Choose a reason for hiding this comment

gwenneg Jan 5, 2021

Choose a reason for hiding this comment

mkouba commented Jan 4, 2021

gsmet left a comment

Choose a reason for hiding this comment

gwenneg commented Jan 4, 2021

gsmet commented Jan 4, 2021

gwenneg commented Jan 4, 2021

gwenneg commented Dec 20, 2020 •

edited

Loading

gwenneg Dec 20, 2020 •

edited

Loading