Refactoring servlets to allow caching response

Refactoring servlets by adding additional methods:
HttpRequest buildHttpRequest(HttpServletRequest request);
HttpResponse processRequest(HttpRequest request);

This results in added logical separation of common tasks performed by servlets (parsing and generating an http request, fetching the undelying resources, processing the request and convering http response to http servlet response to be sent to user.

Also, this will allow us to cache responses served by these servlets more easily. Then we can add simple logic along the lines of:

- If precached, serve out previously served response directly.
- If not, generate the response for given request and then add to cache.

This can be useful to at least servlets like MakeRequestServlet, ConcatServlet, AccelServlet etc..

Comments welcome.
Created a jira issue for the same: https://issues.apache.org/jira/browse/SHINDIG-1381

PLEASE NOTE: This is a rough changelist. Its aim is to gather feedback on the suggested refactoring. It can be polished later if you feel the idea is good. So please focus on the API and interface changes in ProxyBase, AccelHandler, MakeRequestHandler and ProxyHandler.

[gagan.goku, 2010-07-11 20:21:18]

fixing accel servlet tests

[gagan.goku, 2010-07-12 06:57:27]

fixing tests

[vikaas.arora, 2010-07-14 06:10:24]

http://codereview.appspot.com/1770043/diff/13001/14001
File main/java/org/apache/shindig/gadgets/servlet/AccelHandler.java (left):

http://codereview.appspot.com/1770043/diff/13001/14001#oldcode211
main/java/org/apache/shindig/gadgets/servlet/AccelHandler.java:211: throws
IOException {
This method (and it's constituent method calls) wasn't throwing IOException.
Good to have this cleaned up.

http://codereview.appspot.com/1770043/diff/13001/14001
File main/java/org/apache/shindig/gadgets/servlet/AccelHandler.java (right):

http://codereview.appspot.com/1770043/diff/13001/14001#newcode86
main/java/org/apache/shindig/gadgets/servlet/AccelHandler.java:86:
IOUtils.copy(results.getResponse(), response.getOutputStream());
You can add another small function (10 odd lines) for block #75-#86 and name it
CreateResponse()

http://codereview.appspot.com/1770043/diff/13001/14001#newcode90
main/java/org/apache/shindig/gadgets/servlet/AccelHandler.java:90: protected
HttpRequest buildHttpRequest(HttpServletRequest request)
Move this overloaded method 'buildHttpRequest' near the other method (that takes
two args).

http://codereview.appspot.com/1770043/diff/13001/14001#newcode108
main/java/org/apache/shindig/gadgets/servlet/AccelHandler.java:108: // throw
appropriate gadget exception.
Are we throwing GadgetException ?
If not, revisit the comment.

http://codereview.appspot.com/1770043/diff/13001/14003
File main/java/org/apache/shindig/gadgets/servlet/MakeRequestHandler.java
(right):

http://codereview.appspot.com/1770043/diff/13001/14003#newcode100
main/java/org/apache/shindig/gadgets/servlet/MakeRequestHandler.java:100:
response.getWriter().write(UNPARSEABLE_CRUFT + output);
Block #91-#100 -> CreateResponse()

http://codereview.appspot.com/1770043/diff/13001/14004
File main/java/org/apache/shindig/gadgets/servlet/ProxyBase.java (right):

http://codereview.appspot.com/1770043/diff/13001/14004#newcode203
main/java/org/apache/shindig/gadgets/servlet/ProxyBase.java:203: protected
HttpRequest buildHttpRequest(HttpServletRequest request)
Don't you want to make this method abstract.
If for a particular case, this method implementation is null, let that specific
ProxyHandler define this will null body.

Otherwise, just don't declare this method in this abstract class (interface) (if
you feel that this is not generic enough. Also in that case, make this method
private in the other ProxyHandlers).

http://codereview.appspot.com/1770043/diff/13001/14005
File main/java/org/apache/shindig/gadgets/servlet/ProxyHandler.java (right):

http://codereview.appspot.com/1770043/diff/13001/14005#newcode175
main/java/org/apache/shindig/gadgets/servlet/ProxyHandler.java:175: HttpRequest
rcr = buildHttpRequest(request, proxyUri, proxyUri.getResource());
What does 'rcr' signify/mean ?

[vikaas.arora, 2010-07-14 06:22:11]

http://codereview.appspot.com/1770043/diff/13001/14005
File main/java/org/apache/shindig/gadgets/servlet/ProxyHandler.java (left):

http://codereview.appspot.com/1770043/diff/13001/14005#oldcode89
main/java/org/apache/shindig/gadgets/servlet/ProxyHandler.java:89: }
Shouldn't you retain this check as the first statement in the doFetch(). You
have the similar check in 'processRequest'

http://codereview.appspot.com/1770043/diff/13001/14005
File main/java/org/apache/shindig/gadgets/servlet/ProxyHandler.java (right):

http://codereview.appspot.com/1770043/diff/13001/14005#newcode105
main/java/org/apache/shindig/gadgets/servlet/ProxyHandler.java:105: }
Embed this block in a separate method (prepareRequest())

http://codereview.appspot.com/1770043/diff/13001/14008
File test/java/org/apache/shindig/gadgets/servlet/ProxyHandlerTest.java (right):

http://codereview.appspot.com/1770043/diff/13001/14008#newcode79
test/java/org/apache/shindig/gadgets/servlet/ProxyHandlerTest.java:79: //   
expect(request.getHeaders("Host")).andReturn(makeEnumeration(host));
Remove this commented code from the test.

[gagan.goku, 2010-07-14 14:59:49]

addressing comments

[gagan.goku, 2010-07-14 15:00:21]

http://codereview.appspot.com/1770043/diff/13001/14001
File main/java/org/apache/shindig/gadgets/servlet/AccelHandler.java (left):

http://codereview.appspot.com/1770043/diff/13001/14001#oldcode211
main/java/org/apache/shindig/gadgets/servlet/AccelHandler.java:211: throws
IOException {
On 2010/07/14 06:10:25, vikaas.arora wrote:
> This method (and it's constituent method calls) wasn't throwing IOException.
> Good to have this cleaned up.

Thanks

http://codereview.appspot.com/1770043/diff/13001/14001
File main/java/org/apache/shindig/gadgets/servlet/AccelHandler.java (right):

http://codereview.appspot.com/1770043/diff/13001/14001#newcode86
main/java/org/apache/shindig/gadgets/servlet/AccelHandler.java:86:
IOUtils.copy(results.getResponse(), response.getOutputStream());
On 2010/07/14 06:10:25, vikaas.arora wrote:
> You can add another small function (10 odd lines) for block #75-#86 and name
it
> CreateResponse()

Done.

http://codereview.appspot.com/1770043/diff/13001/14001#newcode90
main/java/org/apache/shindig/gadgets/servlet/AccelHandler.java:90: protected
HttpRequest buildHttpRequest(HttpServletRequest request)
On 2010/07/14 06:10:25, vikaas.arora wrote:
> Move this overloaded method 'buildHttpRequest' near the other method (that
takes
> two args).

Done.

http://codereview.appspot.com/1770043/diff/13001/14001#newcode108
main/java/org/apache/shindig/gadgets/servlet/AccelHandler.java:108: // throw
appropriate gadget exception.
On 2010/07/14 06:10:25, vikaas.arora wrote:
> Are we throwing GadgetException ?
> If not, revisit the comment.

Done.

http://codereview.appspot.com/1770043/diff/13001/14003
File main/java/org/apache/shindig/gadgets/servlet/MakeRequestHandler.java
(right):

http://codereview.appspot.com/1770043/diff/13001/14003#newcode100
main/java/org/apache/shindig/gadgets/servlet/MakeRequestHandler.java:100:
response.getWriter().write(UNPARSEABLE_CRUFT + output);
On 2010/07/14 06:10:25, vikaas.arora wrote:
> Block #91-#100 -> CreateResponse()

I am currently adding a todo for the same as moving that out will require a lot
of refactoring as it uses the httpservletrequest.
Will fix later if ppl like the idea.

http://codereview.appspot.com/1770043/diff/13001/14004
File main/java/org/apache/shindig/gadgets/servlet/ProxyBase.java (right):

http://codereview.appspot.com/1770043/diff/13001/14004#newcode203
main/java/org/apache/shindig/gadgets/servlet/ProxyBase.java:203: protected
HttpRequest buildHttpRequest(HttpServletRequest request)
On 2010/07/14 06:10:25, vikaas.arora wrote:
> Don't you want to make this method abstract.
> If for a particular case, this method implementation is null, let that
specific
> ProxyHandler define this will null body.
> 
> Otherwise, just don't declare this method in this abstract class (interface)
(if
> you feel that this is not generic enough. Also in that case, make this method
> private in the other ProxyHandlers).

made abstract.

http://codereview.appspot.com/1770043/diff/13001/14005
File main/java/org/apache/shindig/gadgets/servlet/ProxyHandler.java (left):

http://codereview.appspot.com/1770043/diff/13001/14005#oldcode89
main/java/org/apache/shindig/gadgets/servlet/ProxyHandler.java:89: }
On 2010/07/14 06:22:12, vikaas.arora wrote:
> Shouldn't you retain this check as the first statement in the doFetch(). You
> have the similar check in 'processRequest'

I can make it the first statement, but then it doesn't fit in well with the
current refactoring.
So i though it'd be better to move it to processRequest.

http://codereview.appspot.com/1770043/diff/13001/14005
File main/java/org/apache/shindig/gadgets/servlet/ProxyHandler.java (right):

http://codereview.appspot.com/1770043/diff/13001/14005#newcode105
main/java/org/apache/shindig/gadgets/servlet/ProxyHandler.java:105: }
On 2010/07/14 06:22:12, vikaas.arora wrote:
> Embed this block in a separate method (prepareRequest())

Done.

http://codereview.appspot.com/1770043/diff/13001/14005#newcode175
main/java/org/apache/shindig/gadgets/servlet/ProxyHandler.java:175: HttpRequest
rcr = buildHttpRequest(request, proxyUri, proxyUri.getResource());
On 2010/07/14 06:10:25, vikaas.arora wrote:
> What does 'rcr' signify/mean ?

no idea. I just kept the same variable name :)
Changed to httpRequest.

http://codereview.appspot.com/1770043/diff/13001/14008
File test/java/org/apache/shindig/gadgets/servlet/ProxyHandlerTest.java (right):

http://codereview.appspot.com/1770043/diff/13001/14008#newcode79
test/java/org/apache/shindig/gadgets/servlet/ProxyHandlerTest.java:79: //   
expect(request.getHeaders("Host")).andReturn(makeEnumeration(host));
On 2010/07/14 06:22:12, vikaas.arora wrote:
> Remove this commented code from the test.

Done.

[gagan.goku, 2010-07-14 15:32:52]

fixing bad things

[gagan.goku, 2010-07-15 19:23:20]

PTAL.

On Wed, Jul 14, 2010 at 9:02 PM, <gagan.goku@gmail.com> wrote:

> fixing bad things
>
>
> http://codereview.appspot.com/1770043/show
>

[anupama.dutta, 2010-08-04 11:28:17]

Let us discuss this approach in more detail, try to break this into smaller
patches and then proceed on this review.

http://codereview.appspot.com/1770043/diff/78001/79008
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/CachedResponseServletWrapper.java
(right):

http://codereview.appspot.com/1770043/diff/78001/79008#newcode2
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/CachedResponseServletWrapper.java:2:
* Licensed to the Apache Software Foundation (ASF) under one
Though this class indicates why the refactoring of handlers is being done, I
think this is quite independent in its functionality. Let us move this to
another patch so that the review is easier.

[satya3656, 2010-08-14 11:04:15]

Few minor comments

http://codereview.appspot.com/1770043/diff/78001/79008
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/CachedResponseServletWrapper.java
(right):

http://codereview.appspot.com/1770043/diff/78001/79008#newcode3
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/CachedResponseServletWrapper.java:3:
* or more contributor license agreements. See the NOTICE file
+1 to move caching part to a separate CL

http://codereview.appspot.com/1770043/diff/78001/79007
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/MakeRequestHandler.java
(right):

http://codereview.appspot.com/1770043/diff/78001/79007#newcode89
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/MakeRequestHandler.java:89:
// TODO: Move code for sending out response to sendResponse.
I think this TODO must be done now to make the refactoring complete.

http://codereview.appspot.com/1770043/diff/78001/79006
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyHandler.java
(right):

http://codereview.appspot.com/1770043/diff/78001/79006#newcode100
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyHandler.java:100:
protected void sendResponse(HttpRequest req, HttpResponse results,
Reodering the function definitions to
buildHttpRequest(request)
 processRequest(req);
sendResponse(req, results, response);

across all handlers will make it more readable.

[gagan.goku, 2010-08-16 04:32:56]

http://codereview.appspot.com/1770043/diff/78001/79008
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/CachedResponseServletWrapper.java
(right):

http://codereview.appspot.com/1770043/diff/78001/79008#newcode2
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/CachedResponseServletWrapper.java:2:
* Licensed to the Apache Software Foundation (ASF) under one
On 2010/08/04 11:28:18, anupama.dutta wrote:
> Though this class indicates why the refactoring of handlers is being done, I
> think this is quite independent in its functionality. Let us move this to
> another patch so that the review is easier.

Done.

http://codereview.appspot.com/1770043/diff/78001/79008#newcode3
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/CachedResponseServletWrapper.java:3:
* or more contributor license agreements. See the NOTICE file
On 2010/08/14 11:04:16, satya3656 wrote:
> +1 to move caching part to a separate CL

Done.

http://codereview.appspot.com/1770043/diff/78001/79007
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/MakeRequestHandler.java
(right):

http://codereview.appspot.com/1770043/diff/78001/79007#newcode89
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/MakeRequestHandler.java:89:
// TODO: Move code for sending out response to sendResponse.
On 2010/08/14 11:04:16, satya3656 wrote:
> I think this TODO must be done now to make the refactoring complete.

Done.

http://codereview.appspot.com/1770043/diff/78001/79006
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyHandler.java
(right):

http://codereview.appspot.com/1770043/diff/78001/79006#newcode100
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyHandler.java:100:
protected void sendResponse(HttpRequest req, HttpResponse results,
On 2010/08/14 11:04:16, satya3656 wrote:
> Reodering the function definitions to
> buildHttpRequest(request)
>  processRequest(req);
> sendResponse(req, results, response);
> 
> across all handlers will make it more readable.

Done.

[satya3656, 2010-08-16 11:20:16]

http://codereview.appspot.com/1770043/diff/90009/93006
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/MakeRequestHandler.java
(right):

http://codereview.appspot.com/1770043/diff/90009/93006#newcode288
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/MakeRequestHandler.java:288:
protected static String getParameter(HttpServletRequest request, String key,
String defaultValue) {
Is is possible to to remove this function?