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

GeneratedCacheKey implementation should be part of spec #313

Closed
beikov opened this issue Apr 14, 2015 · 10 comments
Closed

GeneratedCacheKey implementation should be part of spec #313

beikov opened this issue Apr 14, 2015 · 10 comments
Assignees
Labels

Comments

@beikov
Copy link

beikov commented Apr 14, 2015

I recently faced a situation where I use @CacheResult on a method with a string parameter. Another method which is annotated with @CachePut or @CacheRemove that uses an object as parameter will immediately bring up problems.
The cache put/remove case will need a custom CacheKeyGenerator that extracts the string parameter of the object. The problem now is, that the cache key generator class has to return a GeneratedCacheKey instance which will be used to find a cache entry. Since the implementation that is used in the cache result case is not necessarily known, we also have to create our own implementations for CacheKeyGenerator and GeneratedCacheKey and use the generator class everywhere instead of the default in order to have a compatible cache key for the cache put/remove case.

I see 3 possible solutions

  • Move the DefaultGeneratedCacheKey implementation to the spec and specify that every implementation should use that class as default
  • Expose the parameters somehow in the interface and demand that the equals-hashCode implementations must only make use of parameters array
  • Make it possible to create an instance of the default type via a factory
@gregrluck gregrluck added this to the 1.1 milestone Oct 26, 2015
@gregrluck
Copy link
Member

Assigned this Eric.

Can you write a test to illustrate.

Would like to get this fixed for 1.1.

@beikov
Copy link
Author

beikov commented Oct 28, 2015

Here is the test case showing that I have to use the GeneratedCacheKey implementation of the cdi integration module to actually retrieve the value from the cache: https://github.com/beikov/jsr107-313

@gregrluck gregrluck assigned cruftex and unassigned edalquist May 14, 2016
@cruftex
Copy link
Member

cruftex commented May 14, 2016

I am not really an expert here and try to get into this, since nobody else seem to. So bare with me.

Another method which is annotated with @CachePut or @CacheRemove that uses an object as parameter will immediately bring up problems.

Does that mean one time you use String and one time you use Object as key type?

Since the implementation that is used in the cache result case is not necessarily known

As far as I interpret it, either you specify it via the annotation cacheKeyGenerator or it isn't known.

Your test case only includes the CacheResult annotation, but your initial statement was about the interaction of the different annotations. Can you give a more complete example?

@cruftex cruftex removed the Defect label May 14, 2016
@cruftex cruftex removed this from the 1.1 milestone May 14, 2016
@cruftex
Copy link
Member

cruftex commented May 14, 2016

Updated issue to "Question" since we first need to see what is affected here.

@beikov
Copy link
Author

beikov commented May 14, 2016

Look, if you call a method annotated with @CacheResult as in my example, the interceptor puts a value into the underlying cache with a key of type org.jsr107.ri.annotations.DefaultGeneratedCacheKey. Now if you want to manually alter the cache or have a custom cache key generator for a different method, you actually MUST use the same GeneratedCacheKey implementation because of the implementation of the equals method of org.jsr107.ri.annotations.DefaultGeneratedCacheKey.

@cruftex
Copy link
Member

cruftex commented May 14, 2016

Look, if you call a method annotated with @CacheResult as in my example, the interceptor puts a value into the underlying cache with a key of type org.jsr107.ri.annotations.DefaultGeneratedCacheKey.

We are talking about the Spec. The Spec defines:

Default CacheKeyGenerator Rules:

  1. Create an Object[] using CacheInvocationParameter.getValue() from the array
    returned by CacheKeyInvocationContext.getKeyParameters()
  2. Create a CacheKey instance that wraps the Object[] and uses Arrays.deepHashCode to
    calculate its hashCode and Arrays.deepEquals for comparison to other keys.

As far as I understand it right now, if you do not specify cacheKeyGenerator on the CacheResult annotation you do not have a guarantee what class implementation you get.

So if you want to have a specific implementation, why don't you specify cacheKeyGenerator on the CacheResult annotation?

@cruftex
Copy link
Member

cruftex commented May 14, 2016

Ah, actually you said that by yourself:

We also have to create our own implementations for CacheKeyGenerator and GeneratedCacheKey and use the generator class everywhere instead of the default in order to have a compatible cache key for the cache put/remove case.

Sorry, about that.

@cruftex
Copy link
Member

cruftex commented May 14, 2016

I think your point is relevant, in general. There are a couple of things popping up, that all have to do with the interaction of the imperative API and annotations. Furthermore, there are always way to improve.

However, JSR107 is in maintenance, we cannot change at this point to make it "nicer" which might break compatibility.

If I did overlook something and some general things are wrong with the KeyGenerator that make things unusable, please reopen. Closing.

@cruftex cruftex closed this as completed May 14, 2016
@beikov
Copy link
Author

beikov commented May 15, 2016

How is adding DefaultGeneratedCacheKey to the spec afterwards and requiring implementations to use that as default implementation backwards incompatible?
This is in my opinion something that is not only relevant, but essential and makes the interceptors like they are implemented unusable just because of this little issue.
If it stays like that, people will sooner or later have to implement their own interceptors or will have to pollute all usages with their own default KeyGenerator.

@chrisknoll
Copy link

Late to the party, but IMO, the CacheKeyGenerator should have the specified 'generateCacheKey' return Object for the same reasons that java.util.HashMap.get(Object k) takes an Object as the paramter: the only thing that matters is that the object passed in object's .hashCode() and .equals() matches the item attempting to be found. The specific type doesn't matter.

Now what I have to do is implement my own DefaultCacheKey implementation just like everyone else in the world. Which is code cruft which sorta makes me sad.

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

No branches or pull requests

5 participants