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

Allow customization of @CacheFor cache key #1166

Merged
merged 3 commits into from
Oct 16, 2017

Conversation

tahseenarticle
Copy link
Contributor

Please see issue #1133

At the moment when you annotate an Controller method with CacheFor. Play framework generate the cache key as follow in ActionInvoker.java line 152:

cacheKey = "urlcache:" + request.url + request.querystring;

CacheFor annotation does not allow customisation of this cache key. This is useful in case you your controller shows different page based on the session. For example if you viewing different language version of the website the URL will be the same but the locale information is in the session. Having a way to customise this cache key helps in lot different cases of caching.

My proposal is to extend the @CacheFor as follow:

@Retention(RetentionPolicy.RUNTIME)
@Target(ElementType.METHOD)
public @interface CacheFor {
    String value() default "1h";
    String id() default "";
    Class<? extends CacheKeyGenerator> generator() default DefaultCacheKeyGenerator.class;
}

Where CacheKeyGenerator is a simple interface that generate the cache key.

public interface CacheKeyGenerator {
	public String generate(Request request);
}
public class DefaultCacheKeyGenerator implements CacheKeyGenerator {
	@Override
	public String generate(Request request) {
		return "urlcache:" + request.url + request.querystring;
	}
}

Then we can modify the InvokerAction.invoke to generate the cache key based on the CacheKeyGenerator.

You can use a custom CacheKeyGenerator as follow:

@CacheFor(value = "1h", generator=SessionBasedCacheKeyGenerator.class)

If you do not specify the generator the default key generator will be used as usual.

@CacheFor(value = "1h")

@asolntsev asolntsev self-assigned this Sep 18, 2017
if ("".equals(cacheKey)) {
cacheKey = "urlcache:" + request.url + request.querystring;
// Generate a cache key for this request
cacheKey = cacheFor.generator().newInstance().generate(request);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't it cause a bigger memory footprint because of creating a new instance of generator every time?

Copy link
Contributor Author

@tahseenarticle tahseenarticle Sep 18, 2017

Choose a reason for hiding this comment

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

There two options here:

  1. We can maintain a cache of CacheKeyGenerator instances in the ActionInvoker.
  2. Push the creation of generator to be application developer responsibility by added a get() method to the CacheKeyGenerator interface.
public interface CacheKeyGenerator {
	public String generate(Request request);
        public CacheKeyGenerator get();
}

Line 154 will can rewitten as:

cacheKey = cacheFor.generator().get().generate(request);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

DefaultCacheKeyGenerator can implement a singleton

public class DefaultCacheKeyGenerator implements CacheKeyGenerator {
        private final static CacheKeyGenerator instance;
	@Override
	public String generate(Request request) {
		return "urlcache:" + request.url + request.querystring;
	}

       public CacheKeyGenerator get() {
               if(instance == null) {
                        instance = new DefaultCacheKeyGenerator();
               }
               return  instance;
       }
}

Copy link
Contributor Author

@tahseenarticle tahseenarticle Sep 18, 2017

Choose a reason for hiding this comment

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

I can make quick these changes if this is what is required to get this pull request out of the door. I am also all ears for any other solutions.

Copy link
Contributor

Choose a reason for hiding this comment

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

@tahseenarticle I am not really sure if we need to change anything. I just asked :)
Probably it would be a premature optimization.
What I actually am waiting is @xael-fry review.

@asolntsev
Copy link
Contributor

@xael-fry Please review the PR. From a code quality, it seems ok. Though, probably a little bit overkill... Or not?

@asolntsev asolntsev added this to the 1.5.1 milestone Oct 16, 2017
@asolntsev asolntsev merged commit 62e2ea7 into playframework:master Oct 16, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants