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.

Sign up for GitHub

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
Merged

Allow customization of @CacheFor cache key #1166

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
811a235
Allow customization of @CacheFor cache key #1133
tahseenarticle Jun 19, 2017
50355d9
Merge remote-tracking branch 'upstream/master'
tahseenarticle Aug 15, 2017
2a64d8d
Merge remote-tracking branch 'upstream/master'
tahseenarticle Oct 12, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions framework/src/play/cache/CacheFor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,4 +17,5 @@
public @interface CacheFor {
String value() default "1h";
String id() default "";
Class<? extends CacheKeyGenerator> generator() default DefaultCacheKeyGenerator.class;
}
10 changes: 10 additions & 0 deletions framework/src/play/cache/CacheKeyGenerator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
package play.cache;

import play.mvc.Http.Request;

/**
* Allow custom cache key to be used by applications.
*/
public interface CacheKeyGenerator {
public String generate(Request request);
}
13 changes: 13 additions & 0 deletions framework/src/play/cache/DefaultCacheKeyGenerator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
package play.cache;

import play.mvc.Http.Request;

/**
* The Default Cache Key Generator
*/
public class DefaultCacheKeyGenerator implements CacheKeyGenerator {
@Override
public String generate(Request request) {
return "urlcache:" + request.url + request.querystring;
}
}
12 changes: 8 additions & 4 deletions framework/src/play/mvc/ActionInvoker.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,11 +147,15 @@ public static void invoke(Http.Request request, Http.Response response) {

// Check the cache (only for GET or HEAD)
if ((request.method.equals("GET") || request.method.equals("HEAD")) && actionMethod.isAnnotationPresent(CacheFor.class)) {
cacheKey = actionMethod.getAnnotation(CacheFor.class).id();
CacheFor cacheFor = actionMethod.getAnnotation(CacheFor.class);;
cacheKey = cacheFor.id();
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 •
edited
Loading

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 •
edited
Loading

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.

}
if(cacheKey != null && !"".equals(cacheKey)) {
actionResult = (Result) Cache.get(cacheKey);
}
actionResult = (Result) Cache.get(cacheKey);
}

if (actionResult == null) {
Expand All @@ -161,7 +165,7 @@ public static void invoke(Http.Request request, Http.Response response) {
} catch (Result result) {
actionResult = result;
// Cache it if needed
if (cacheKey != null) {
if (cacheKey != null && !"".equals(cacheKey)) {
Cache.set(cacheKey, actionResult, actionMethod.getAnnotation(CacheFor.class).value());
}
} catch (JavaExecutionException e) {
Expand Down