quarkusio / quarkus

Quarkus: Supersonic Subatomic Java.
https://quarkus.io
Apache License 2.0
13.54k stars 2.61k forks source link

Support for long primitive in quarkus-redis when using kotlin #30842

Open tjakopan opened 1 year ago

tjakopan commented 1 year ago

Description

When trying to do a redis quide https://quarkus.io/guides/redis in kotlin I'm getting the following error java.lang.IllegalArgumentException: Unable to encode object of type class java.lang.Long when using ReactiveValueCommands with Long.

private val countCommands: ReactiveValueCommands<String, Long> = dataSource.value(Long::class.java)

The issue is that io.quarkus.redis.runtime.datasource.Marshaller constructor is receiving long as a class hint which then resolves to JsonCodec, but when Marshaller.codec(Class<?>) method is called in order to determine a codec, java.lang.Long is passed as a param. Ultimately the codec cannot be found because Marshaller.codecs map has a codec for long, not for java.lang.Long.

The solution is to do this: private val countCommands: ReactiveValueCommands<String, Long> = dataSource.value(Long::class.javaObjectType) instead of this: private val countCommands: ReactiveValueCommands<String, Long> = dataSource.value(Long::class.java)

It would be easier to use this from kotlin without needing to explicitly specify javaObjectType.

The reproducer is here in qs-redis module - https://github.com/tjakopan/quarkus-examples-kotlin. In order to reproduce the error, one can comment out line 13 and un-comment line 14 in IncrementService class and run the tests.

Implementation ideas

io.quarkus.redis.datasource.codecs.Codecs class could specify a LongCodec, similar to already existing codecs for double, int, String and byte[]. And then the Codecs.getDefaultCodecFor(Class<T>) method could be changed to:

    public static <T> Codec<T> getDefaultCodecFor(Class<T> clazz) {
        if (clazz.equals(Double.class) || clazz.equals(Double.TYPE)) {
            return (Codec<T>) DoubleCodec.INSTANCE;
        }
        if (clazz.equals(Integer.class) || clazz.equals(Integer.TYPE)) {
            return (Codec<T>) IntegerCodec.INSTANCE;
        }
       if (clazz.equals(Long.class) || clazz.equals(Long.TYPE)) {
            return (Codec<T>) LongCodec.INSTANCE;
        }
        if (clazz.equals(String.class)) {
            return (Codec<T>) StringCodec.INSTANCE;
        }
        if (clazz.equals(byte[].class)) {
            return (Codec<T>) ByteArrayCodec.INSTANCE;
        }
        // JSON by default
        return new JsonCodec<>(clazz);
    }

Willing to contribute with this if needed.

quarkus-bot[bot] commented 1 year ago

/cc @cescoffier (redis), @evanchooly (kotlin), @geoand (kotlin), @gsmet (redis), @machi1990 (redis)

cescoffier commented 1 year ago

Thanks for the report. Your approach looks good. Fancy a PR?

machi1990 commented 1 year ago

It might be worth to double check other primitives as well and apply the fix throughout and not just for long

tjakopan commented 1 year ago

Thanks for the report. Your approach looks good. Fancy a PR?

Sure, I'll do a PR, might be couple of days though.

machi1990 commented 1 year ago

Thanks for the report. Your approach looks good. Fancy a PR?

Sure, I'll do a PR, might be couple of days though.

Thanks @tjakopan shoutout if you need any help.

tjakopan commented 11 months ago

Thanks for the report. Your approach looks good. Fancy a PR?

Sure, I'll do a PR, might be couple of days though.

Couple of days turned into couple of months :-(. Previous company ended up not using quarkus and kotlin so completely forgot I have that PR almost ready.

I've upgraded the project to quarkus 3 and that issue is no longer there. With the changes to io.quarkus.redis.datasource.codecs.Codecs.JsonCodec supporting type references, Long/long now defaults to JsonCodec and that case is now working correctly. In fact, current IntegerCodec and DoubleCodec might not even be needed anymore.

I do have char, float, byte, short, long and boolean codecs implemented, if needed. @machi1990 @cescoffier

cescoffier commented 11 months ago

Definitely welcome!