Spring @Cacheable with a SpEL key containing a parameter list breaks silently in Spring Framework 6.1 in libraries compiled without -parameters flag

Description

Let's assume we have a library compiled without -parameters flag, that uses @Cacheable annotation with a SpEL expression containing a list, e.g.:

@Component
public class CachedDataLib {
    @Cacheable(value = "myCacheLib", key = "{ #param1, #param2 }")
    public String get(String param1, String param2) {
        return param1 + "/" + param2;
    }
}

It works correctly in Spring Framework < 6.1, but in 6.1 all the invocations after the first one will return the result of the first invocation, regardless of parameter values:

var firstResult = cachedDataLib.get("first", "first");
var secondResult = cachedDataLib.get("second", "second");
assertThat(firstResult).isEqualTo("first/first");
assertThat(secondResult).isEqualTo("second/second");    <--- this fails as secondResult = "first/first"

This can result in wrong data being silently returned after the update to Spring Framework 6.1.

Analysis and expected behavior

This is because LocalVariableTableParameterNameDiscoverer has been removed (as described in Spring Framework 6.1 upgrade guide), and there are no checks that the parameters haven't been resolved successfully, making CacheAspectSupport silently use a list with values [null,null] as its cache key for any parameter values.

There is a check in CacheAspectSupport that throws an exception when the whole key becomes null due to lack of -parameters flag, but it doesn't apply in this case, as the key is an ArrayList containing nulls.

Would it be possible to throw a similar exception when the object is a list containing at least one null value?

Also, it might be worth adding -parameters to the official Creating a Multi Module Project guide (Library Project disables org.springframework.boot plugin, so it does not use this flag).

Steps to reproduce

A minimal example reproducing the issue: https://github.com/mgocd/spring-cacheable-parameters-issue

See also https://github.com/spring-projects/spring-framework/issues/14021.

Comment From: snicoll

This can result in wrong data being silently returned after the update to Spring Framework 6.1.

One could argue that the SpEL expression could have been tested to validate that the generated key matches the expectation. Running your sample with Spring Framework 6.0 outputs the following:

2024-01-25T11:06:37.666+01:00  WARN 2460 --- [    Test worker] ocalVariableTableParameterNameDiscoverer : Using deprecated '-debug' fallback for parameter name resolution. Compile the affected code with '-parameters' instead or avoid its introspection: com.example.demo.mylib.CachedDataLib

Would it be possible to throw a similar exception when the object is a list containing at least one null value?

I am afraid not. While not ideal, having null value in part of the key looks valid to me. I think your analysis is wrong there. We do check that the key is not null but if you decide to work with a more complex type, we shouldn't interfere with that and doing so will break someone else.

Comment From: mgocd

@snicoll I understand, what's worth adding is that if a library was compiled with Spring Framework 6.0 with all tests passing (even if it had SpEL expression tests), an app using this library would silently break after updating to Spring Framework 6.1. The only warning this could happen is the log you mentioned.

Also, when using the @Cacheable as described in this issue in a new Spring Framework 6.1 app using the Creating a Multi Module Project guide, it will just not work without any warnings (and work if -parameters is added).

Is there a reason why SpEL evaluator couldn't throw an exception if it fails to evaluate an expression in this case?

Comment From: snicoll

if a library was compiled with Spring Framework 6.0 with all tests passing (even if it had SpEL expression tests), an app using this library would silently break after updating to Spring Framework 6.1.

Yes. I would expect an application that uses an underlying framework like Spring that publishes common libraries to be upgraded consistently.

So, if you don't look for warnings in the log, don't sanity test your SpEL expression or do that but with an outdated version of the framework, and don't read the release notes, you can be impacted. Unfortunately, that's the nature of using something like SpEL: we can't deprecate the code and or fail compilation due to its dynamic nature.

Also, when using the @Cacheable as described in this issue in a new Spring Framework 6.1 app using the Creating a Multi Module Project guide, it will just not work without any warnings (and work if -parameters is added).

I've created an issue for that but, as far as I can see, it's gradle specific as the library uses the boot parent as well and that configures the compiler plugin accordingly.

Is there a reason why SpEL evaluator couldn't throw an exception if it fails to evaluate an expression in this case?

That's just not how SpEL works. I understand that looking at that very specific use case you're thinking this is a good idea, but if it was possible we would have done that rather than logging a warning.