Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Redis Cache: support more complex types #43320

Merged
merged 1 commit into from
Sep 24, 2024

Conversation

Ladicek
Copy link
Contributor

@Ladicek Ladicek commented Sep 16, 2024

This includes generic types, like List<String>, array types, like int[], and more.

The type parser in this commit is reasonably complete. Omitting type variables should not be an issue; nested types might, but adding support for them should be doable later.

Fixes #41301

@Ladicek
Copy link
Contributor Author

Ladicek commented Sep 16, 2024

Draft because there's one implementation TODO that I need to think about -- but the rest should be ready for review.

I'm parsing types at runtime and not at build time, because transferring types through the recorder mechanism is not exactly possible. I could easily copy the ArC code that turns org.jboss.jandex.Type into java.lang.reflect.Type, but that is fundamentally incompatible with the recorder mechanism and I don't see a way to circumvent that.

Copy link
Member

@cescoffier cescoffier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The parser is pretty neat!

@Ladicek
Copy link
Contributor Author

Ladicek commented Sep 17, 2024

2 things I just thought about:

It should be possible to construct the java.lang.reflect.Type at build time (because loading Classes is possible, and the rest is just a few interfaces that we can implement on our own, as demonstrated in this PR), and that should be straightforward to transfer to runtime via a recorder. Not sure if that would be a huge improvement, but a build-time failure is always preferable to a runtime failure, so I guess I'll work on that.

Maybe the API shouldn't accept Type, but jakarta.enterprise.util.TypeLiteral (or com.fasterxml.jackson.core.type.TypeReference, but I guess I'd prefer the CDI class, as it's more neutral). The implementation would still work on Types, but that would be hidden. Sounds like a small but neat improvement, I'll see how that works.

This includes generic types, like `List<String>`, array types,
like `int[]`, and more.

The type parser in this commit is reasonably complete. Omitting
type variables should not be an issue; nested types might, but
adding support for them should be doable later.
@Ladicek Ladicek marked this pull request as ready for review September 18, 2024 07:40
@Ladicek
Copy link
Contributor Author

Ladicek commented Sep 18, 2024

I think this is ready now. I've implemented both ideas from above, so the type parser is now in core/deployment (and public). I've also added some tests for it.

To the methods on RedisCache that accept Class, I added a variant that accepts TypeLiteral. This meant that the Class-less variants (inherited from Cache) can no longer be default; I had to move their implementation into RedisCacheImpl (which should be OK). This led me to question the usefulness of RedisCache.getDefaultValueType() -- I didn't find a use for that, except for implementing the default methods, so I marked that method as @Deprecated.

This comment has been minimized.

@Ladicek
Copy link
Contributor Author

Ladicek commented Sep 18, 2024

One more thing that perhaps needs @cescoffier's attention: I added a TODO to the implementation of getOrNull(), because that method accepts a type of the value, but doesn't use it. Instead, it always uses the default type of value. Seems wrong to me, but there's no documentation for the method, so who knows 🤷

Copy link

quarkus-bot bot commented Sep 18, 2024

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit bcc9bb7.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

You can consult the Develocity build scans.

enforceDefaultType();
byte[] encodedKey = marshaller.encode(computeActualKey(encodeKey(key)));
return withConnection(new Function<RedisConnection, Uni<V>>() {
@Override
public Uni<V> apply(RedisConnection redisConnection) {
return (Uni<V>) doGet(redisConnection, encodedKey, classOfValue, marshaller);
// TODO maybe use `type` (if non-null?) instead of `classOfValue`?
return doGet(redisConnection, encodedKey, classOfValue, marshaller);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's a bug 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have another commit that makes the RedisCache API overall more regular (all get* methods have 3 variants: without type, using the default value type, with Class type, and with TypeLiteral type). I'll submit that in another PR so we can discuss properly, if you agree.

@Ladicek
Copy link
Contributor Author

Ladicek commented Sep 24, 2024

@cescoffier Could you please approve if this looks OK? Alternatively, could you please share who would be the best reviewer here? Both for the core part and the redis-cache extension.

@cescoffier
Copy link
Member

Sorry, missed the notification. Let's merge!

Thanks!

@cescoffier cescoffier merged commit 3baeed1 into quarkusio:main Sep 24, 2024
67 checks passed
@quarkus-bot quarkus-bot bot added the kind/enhancement New feature or request label Sep 24, 2024
@quarkus-bot quarkus-bot bot added this to the 3.16 - main milestone Sep 24, 2024
@Ladicek Ladicek deleted the redis-cache-complex-types branch September 24, 2024 14:12
@Ladicek
Copy link
Contributor Author

Ladicek commented Sep 24, 2024

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Support for Caching List Results in Quarks Redis Cache
2 participants