SimpleKeyGenerator does not handle Collections the same way it handles Arrays

[rlubbat]

Affects: spring-context 5.1.6.RELEASE

---

Issue:

Using the @Cacheable annotation on a method with a single argument that is a Collection or Map while using a RedisCache often fails to serialize the argument as the cache key when no keyGenerator is specified (defaults to SimpleKeyGenerator).

Example:

Using a RedisCache with configuration defaults except for using JSON serialization for values:
RedisCacheConfiguration.defaultCacheConfig().serializeValuesWith(RedisSerializationContext.SerializationPair.fromSerializer(RedisSerializer.json()));

@Cacheable
public ResultObject findSomething(final List<Something> somethings) {
    // do the expensive operation with the somethings list to return a ResultObject
    return doExpensiveOperation(somethings);
}

This results in a serialization error generating a cache key because there is no conversion service to convert List<Something> to String.

The reason List<Something> is being used to convert to String is because SimpleKeyGenerator has a condition when the params array is length 1. If the single param in the array (in our example, the List<Something>) is not null and is not an array, then SimpleKeyGenerator returns that single param to use as the key. If there is more than 1 item in the params array, then a SimpleKey is returned to use as the key.

If SimpleKeyGenerator were changed to handle Collection and Map as well as array in the case where the params array is length 1, then the example method would work fine since the key would be a SimpleKey instead of the actual List<Something>.

I propose changing the SimpleKeyGenerator.generateKey(Object... params) method to this:

	public static Object generateKey(Object... params) {
            if (params.length == 0) {
                return SimpleKey.EMPTY;
            }
            if (params.length == 1) {
                final Object param = params[0];
                if (param != null && !param.getClass().isArray()
                        // this is the part that differs from SimpleKeyGenerator
                        && !Collection.class.isAssignableFrom(param.getClass())
                        && !Map.class.isAssignableFrom(param.getClass())) {
                    return param;
                }
            }
            return new SimpleKey(params);
        }

[snicoll]

@rlubbat seems sensible, thanks for reporting it.

[mentallurg]

@snicoll, @rlubbat: Why do you want to have it in SimpleKeyGenerator? It was implemented to cover the basic simple cases. For other cases other generators should be used (and implemented, if not yet exist). This is a wrong approach to extend simple implementation with new cases.

@snicoll: If decide to include it into Spring, please do it at least in a separate class. Don't flood simple implementation with new complex cases. Try to keep Spring code clean.

[snicoll]

@rlubbat I overlooked something. We can apply this to the Map use case as both the key and the value is part of the key. Using only the key of the single entry of the map to identify the cache entry sounds like a bad idea to me.

@mentallurg thanks for sharing your opinion but the only reason why I considered it is for consistency. If we handle a single entry array, not handing a single collection is inconsistent.

[snicoll]

I read the original description backwards so please ignore my comment on Map. The special check on array was added as part of #16130 and I am wondering if it would be worth to add it for Collection as well.

@rlubbat the fact that Redis can't serialize the key if it's not a SimpleKey is not something that should be fixed here. Either your configuration is wrong or there is an issue in the Redis implementation.

@jhoeller what are your thoughts to extend that check to Collection ?

[rlubbat]

This isn't about Redis specifically, it was only my example case. If you refer to the reported issue, I noted that the Redis cache configuration in my example was using the defaults. I was not doing any special configuration for key serialization so I don't think it is a problem with my configuration. If it is a problem with the Redis implementation, I can't seem to find where in the Spring code the problem would be.

This is more about #16130, as you mentioned. It is natural to add Collection for the same reason array was added. I could see leaving Map out of it, but I think the same rationale for array and Collection could also apply to Map.

Regarding the question of simplicity, I don't think extending the rationale for array to Collection makes this class any less simple. In fact, it makes it so that developers don't have to do any more complex work like creating a new class and configuring its use on each method that has a Collection as its sole argument. Spring is supposed to help developers by reducing the plumbing code and this request is right in line with that ideal.

Thank you both for looking into the issue.

[snicoll]

This isn't about Redis specifically, it was only my example case.

I don't really understand how you claim this can't be related to Redis considering your original description states:

This results in a serialization error generating a cache key because there is no conversion service to convert List to String.

I'd like us to take a step back now. I've created a sample project which is my understanding of your setup. With that setup and both redis or simple cache, things are working as expected.

Even though I think using a List<Something> as the key of an entity using the default key generation is an odd setup, there shouldn't be any problem with it and I haven't been able to find one so far.

I don't understand why the ConversionService would try to translate List<Something> to String either. Perhaps there is something in your configuration that you're not showing? If you want us to investigate this any further, please submit a PR to my sample project that showcases the problem you are experiencing. You can also provide your own github repo if that's any easier. Thanks!

[rlubbat]

Hi @snicoll. You're right, it is related to the conversion service. I've been on vacation this week and let slip the details from my mind :-) I will take a look at the sample project you created and see if I can get it to fail the way I'm experiencing the issue. I get back in a couple of days, so please don't interpret my lack of action as a lack of interest. Thanks!

[rlubbat]

Hello. I was able to reproduce the problem, but I don’t have permissions to your sample project to commit the code. I’ve attached a ZIP of the changes. - I created a new SomethingTwo object. The difference between it and the Something object is that SomethingTwo has two fields (name and hash). - I created a bean with my proposed key generator enhancement. - I created two new services, SomeServiceTwo and SomeServiceTwoWithKeyGenerator. They have the same findSomething() method as SomeService except they take a List of SomethingTwo objects. - SomeServiceTwoWithKeyGenerator annotates the method with @Cacheable and specifies the use of the enhanced key generator. This works. - SomeServiceTwo annotates the method with @Cacheable alone so that the only difference between it and SomeService is the List element type. This reproduces the error I am seeing. I know a ZIP is annoying. If you give me permissions I will commit and push the changes and send you a PR. You’re help is greatly appreciated. Thanks!

[rlubbat]

Sorry, forgot the ZIP attachment.

…

On Sep 2, 2019, at 5:45 PM, Ramsey Lubbat ***@***.***> wrote: Hello. I was able to reproduce the problem, but I don’t have permissions to your sample project to commit the code. I’ve attached a ZIP of the changes. - I created a new SomethingTwo object. The difference between it and the Something object is that SomethingTwo has two fields (name and hash). - I created a bean with my proposed key generator enhancement. - I created two new services, SomeServiceTwo and SomeServiceTwoWithKeyGenerator. They have the same findSomething() method as SomeService except they take a List of SomethingTwo objects. - SomeServiceTwoWithKeyGenerator annotates the method with @Cacheable and specifies the use of the enhanced key generator. This works. - SomeServiceTwo annotates the method with @Cacheable alone so that the only difference between it and SomeService is the List element type. This reproduces the error I am seeing. I know a ZIP is annoying. If you give me permissions I will commit and push the changes and send you a PR. You’re help is greatly appreciated. Thanks!

[rlubbat]

Ok, never mind the ZIP attachment. I just forked the repo and created a PR. Please let me know how else I can help. Thanks!

[snicoll]

Thanks for following up. This is, as I suspected, a Redis-specific problem. Redis has an optimization for SimpleKey that ensures that such instance can be serialized to a String. If you try to use an argument that isn't a SimpleKey, a proper converter should be in place to get a String out of it.

IMO, your setup is a little brittle. If you want to use Something as a component of the cache, I think it's important to configure how Something contributes to the identity of the entry. Regardless, I think the situation in Redis can be improved. If you want to pursue this, please open an issue against the Spring Data Redis issue tracker.

[christophstrobl]

Created DATAREDIS-1032 to see what we can do about this.