Rename copyResponseToServlet in ServletUtils

Since the copyResponseToServlet does more than just copy the response into servlet(it overrides caching headers), renaming the function to copyToServletResponseAndOverrideCacheHeaders to make this clear. 
For example, with this function, a response with Cache-Control=private,max-age=100 is transformed to
Cache-Control=no-store.
We also add another function called copyToServletResponse which does not override cache headers, but simply copies the response into the servlet response instance.

[gagan.goku, 2011-03-09 12:50:37]

LGMT
Thanks for cleaning this up.

http://codereview.appspot.com/4260059/diff/4008/java/gadgets/src/test/java/or...
File
java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/ServletUtilTest.java
(right):

http://codereview.appspot.com/4260059/diff/4008/java/gadgets/src/test/java/or...
java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/ServletUtilTest.java:239:
.addHeader("h2", "v2").addHeader("Cache-Control", "private,max-age=10")
add no-store also

[nikhilmadan23, 2011-03-09 12:54:11]

http://codereview.appspot.com/4260059/diff/4008/java/gadgets/src/test/java/or...
File
java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/ServletUtilTest.java
(right):

http://codereview.appspot.com/4260059/diff/4008/java/gadgets/src/test/java/or...
java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/ServletUtilTest.java:239:
.addHeader("h2", "v2").addHeader("Cache-Control", "private,max-age=10")
On 2011/03/09 12:50:37, gagan.goku wrote:
> add no-store also

Done.

[anupama.dutta, 2011-03-09 14:43:18]

http://codereview.appspot.com/4260059/diff/1002/java/gadgets/src/main/java/or...
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ServletUtil.java
(right):

http://codereview.appspot.com/4260059/diff/1002/java/gadgets/src/main/java/or...
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ServletUtil.java:105:
public static void copyResponseIntoServlet(
Are you sure this is a good idea? There can be usages of the old method which
won't give an error because of this name change - but the behavior would have
internally changed. (We have one such usage inside Google itself which could
have gone unnoticed)

[anupama.dutta, 2011-03-09 15:18:33]

LGTM.

http://codereview.appspot.com/4260059/diff/1002/java/gadgets/src/main/java/or...
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ServletUtil.java
(right):

http://codereview.appspot.com/4260059/diff/1002/java/gadgets/src/main/java/or...
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ServletUtil.java:105:
public static void copyResponseIntoServlet(
On 2011/03/09 14:43:18, anupama.dutta wrote:
> Are you sure this is a good idea? There can be usages of the old method which
> won't give an error because of this name change - but the behavior would have
> internally changed. (We have one such usage inside Google itself which could
> have gone unnoticed)

Missed that the function name has a tiny change from "To" to "Into". Do consider
making this copyResponseIntoServletResponse or some such thing to make it more
meaningful.

[nikhilmadan23, 2011-03-09 15:55:21]

http://codereview.appspot.com/4260059/diff/1002/java/gadgets/src/main/java/or...
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ServletUtil.java
(right):

http://codereview.appspot.com/4260059/diff/1002/java/gadgets/src/main/java/or...
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ServletUtil.java:105:
public static void copyResponseIntoServlet(
On 2011/03/09 15:18:33, anupama.dutta wrote:
> On 2011/03/09 14:43:18, anupama.dutta wrote:
> > Are you sure this is a good idea? There can be usages of the old method
which
> > won't give an error because of this name change - but the behavior would
have
> > internally changed. (We have one such usage inside Google itself which could
> > have gone unnoticed)
> 
> Missed that the function name has a tiny change from "To" to "Into". Do
consider
> making this copyResponseIntoServletResponse or some such thing to make it more
> meaningful.

Done.

[nikhilmadan23, 2011-03-10 08:40:01]

Fix import order

[gagan.goku, 2011-03-10 08:57:46]

http://codereview.appspot.com/4260059/diff/16001/java/gadgets/src/main/java/o...
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ServletUtil.java
(right):

http://codereview.appspot.com/4260059/diff/16001/java/gadgets/src/main/java/o...
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ServletUtil.java:102:
HttpUtil.setCachingHeaders(servletResponse, (int)(response.getCacheTtl() /
1000L));
this wont work, because the response has already been written out and possibly
flushed to the user.
Sorry for not noticing earlier.

[nikhilmadan23, 2011-03-10 10:16:45]

http://codereview.appspot.com/4260059/diff/16001/java/gadgets/src/main/java/o...
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ServletUtil.java
(right):

http://codereview.appspot.com/4260059/diff/16001/java/gadgets/src/main/java/o...
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ServletUtil.java:102:
HttpUtil.setCachingHeaders(servletResponse, (int)(response.getCacheTtl() /
1000L));
On 2011/03/10 08:57:46, gagan.goku wrote:
> this wont work, because the response has already been written out and possibly
> flushed to the user.
> Sorry for not noticing earlier.

Good catch. Fixed.

[gagan.goku, 2011-03-10 12:18:16]

http://codereview.appspot.com/4260059/diff/3011/java/gadgets/src/main/java/or...
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ServletUtil.java
(right):

http://codereview.appspot.com/4260059/diff/3011/java/gadgets/src/main/java/or...
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ServletUtil.java:98:
public static void copyResponseToServletResponseAndOverrideCacheHeaders(
can we change this to copyToServletResponseAndOverrideCacheHeaders ?

http://codereview.appspot.com/4260059/diff/3011/java/gadgets/src/main/java/or...
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ServletUtil.java:106:
public static void copyResponseToServletResponse(
can we change this to copyToServletResponse

http://codereview.appspot.com/4260059/diff/3011/java/gadgets/src/main/java/or...
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ServletUtil.java:112:
private static void copyResponseContentToServletResponse(
copyContentToServletResponse

http://codereview.appspot.com/4260059/diff/3011/java/gadgets/src/main/java/or...
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ServletUtil.java:117:
private static void copyResponseHeadersAndStatusToServletResponse(
copyHeadersAndStatusToServletResponse

http://codereview.appspot.com/4260059/diff/3011/java/gadgets/src/main/java/or...
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ServletUtil.java:120:
servletResponse.setContentLength(response.getContentLength());
move this line to copyContentToServletResponse()

http://codereview.appspot.com/4260059/diff/3011/java/gadgets/src/test/java/or...
File
java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/ServletUtilTest.java
(right):

http://codereview.appspot.com/4260059/diff/3011/java/gadgets/src/test/java/or...
java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/ServletUtilTest.java:216:
public void testCopyResponseToServletResponseAndOverrideCacheHeaders() throws
Exception {
add the forPublic in test name

[nikhilmadan23, 2011-03-10 12:26:34]

http://codereview.appspot.com/4260059/diff/3011/java/gadgets/src/main/java/or...
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ServletUtil.java
(right):

http://codereview.appspot.com/4260059/diff/3011/java/gadgets/src/main/java/or...
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ServletUtil.java:98:
public static void copyResponseToServletResponseAndOverrideCacheHeaders(
On 2011/03/10 12:18:16, gagan.goku wrote:
> can we change this to copyToServletResponseAndOverrideCacheHeaders ?

Done.

http://codereview.appspot.com/4260059/diff/3011/java/gadgets/src/main/java/or...
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ServletUtil.java:106:
public static void copyResponseToServletResponse(
On 2011/03/10 12:18:16, gagan.goku wrote:
> can we change this to copyToServletResponse

Done.

http://codereview.appspot.com/4260059/diff/3011/java/gadgets/src/main/java/or...
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ServletUtil.java:112:
private static void copyResponseContentToServletResponse(
On 2011/03/10 12:18:16, gagan.goku wrote:
> copyContentToServletResponse

Done.

http://codereview.appspot.com/4260059/diff/3011/java/gadgets/src/main/java/or...
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ServletUtil.java:120:
servletResponse.setContentLength(response.getContentLength());
On 2011/03/10 12:18:16, gagan.goku wrote:
> move this line to copyContentToServletResponse()

Done.

http://codereview.appspot.com/4260059/diff/3011/java/gadgets/src/test/java/or...
File
java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/ServletUtilTest.java
(right):

http://codereview.appspot.com/4260059/diff/3011/java/gadgets/src/test/java/or...
java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/ServletUtilTest.java:216:
public void testCopyResponseToServletResponseAndOverrideCacheHeaders() throws
Exception {
On 2011/03/10 12:18:16, gagan.goku wrote:
> add the forPublic in test name

Done.

[anupama.dutta, 2011-03-10 12:40:54]

Minor comment below. LGTM otherwise.

http://codereview.appspot.com/4260059/diff/18004/java/gadgets/src/test/java/o...
File
java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/ServletUtilTest.java
(right):

http://codereview.appspot.com/4260059/diff/18004/java/gadgets/src/test/java/o...
java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/ServletUtilTest.java:277:
assertGreater(testStartTime - 2000, expires);
Won't this make this test flaky? Should we not make use of the FakeTimeSource
object that we have here and call incrementSeconds or some such thing to make
the comparison deterministic?

[nikhilmadan23, 2011-03-10 13:23:59]

http://codereview.appspot.com/4260059/diff/18004/java/gadgets/src/test/java/o...
File
java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/ServletUtilTest.java
(right):

http://codereview.appspot.com/4260059/diff/18004/java/gadgets/src/test/java/o...
java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/ServletUtilTest.java:277:
assertGreater(testStartTime - 2000, expires);
On 2011/03/10 12:40:54, anupama.dutta wrote:
> Won't this make this test flaky? Should we not make use of the FakeTimeSource
> object that we have here and call incrementSeconds or some such thing to make
> the comparison deterministic?

As discussed, this test is based on the tests in HttpUtilTest

[anupama.dutta, 2011-03-10 13:26:58]

LGTM.

[gagan.goku, 2011-03-10 14:45:33]

LGTM. Committing this now with small modification:

http://codereview.appspot.com/4260059/diff/21006/java/gadgets/src/main/java/o...
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ServletUtil.java
(right):

http://codereview.appspot.com/4260059/diff/21006/java/gadgets/src/main/java/o...
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ServletUtil.java:118:
private static void copyHeadersAndStatusToServletResponse(
change to public, since this is ServletUtil, these methods might prove useful
later.

[gagan.goku, 2011-03-10 19:02:25]

Build looks good.
Committed as r1080320.