Calculate response Hash value

This feature will be used by the Uri rewritters to create versions for resources.
The request pipeline calculates the md5 for each response and store it in the HttpResponse metadata map.

The added class HttpResponseMetadataHelper will support updating the metadata for other values, such as image dimensions to supoprt img link rewritter (will be done if next CL).

[johnfargo, 2010-03-11 00:48:26]

http://codereview.appspot.com/391042/diff/1/2
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/DefaultRequestPipeline.java
(right):

http://codereview.appspot.com/391042/diff/1/2#newcode48
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/DefaultRequestPipeline.java:48:
HttpResponseMetadataHelper metadataHelper) {
may want to make this @Nullable, and allow the hash to be omitted when not
present.

http://codereview.appspot.com/391042/diff/1/6
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponseMetadataHelper.java
(right):

http://codereview.appspot.com/391042/diff/1/6#newcode48
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponseMetadataHelper.java:48:
public static HttpResponse updateMetadata(HttpResponse response, Map<String,
String> values) {
will this method ever be used with more than 1 k/v pair? or by any calling
context that's not within this class itself?

http://codereview.appspot.com/391042/diff/1/6#newcode61
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponseMetadataHelper.java:61:
public HttpResponse updateHash(HttpResponse response) {
this method doesn't really use any class state, so you could either:
A) make it static as well
B) refactor the top of this method into String getHash(HttpResponse response),
which in turn can be mocked (or overridden) in the DefaultRequestPipeline test
so you don't have to calculate data hash to evaluate the test.

I'd lean toward the latter.

[zhoresh, 2010-03-11 01:20:55]

On Wed, Mar 10, 2010 at 4:48 PM, <johnfargo@gmail.com> wrote:

>
> http://codereview.appspot.com/391042/diff/1/2
> File
>
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/DefaultRequestPipeline.java
> (right):
>
> http://codereview.appspot.com/391042/diff/1/2#newcode48
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/DefaultRequestPipeline.java:48:
> HttpResponseMetadataHelper metadataHelper) {
> may want to make this @Nullable, and allow the hash to be omitted when
> not present.
>
> http://codereview.appspot.com/391042/diff/1/6
> File
>
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponseMetadataHelper.java
> (right):
>
> http://codereview.appspot.com/391042/diff/1/6#newcode48
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponseMetadataHelper.java:48:
> public static HttpResponse updateMetadata(HttpResponse response,
> Map<String, String> values) {
> will this method ever be used with more than 1 k/v pair? or by any
> calling context that's not within this class itself?
>

I was thinking that the BasicImageRewriter will use that function to update
both width and height using this function.


> http://codereview.appspot.com/391042/diff/1/6#newcode61
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponseMetadataHelper.java:61:
> public HttpResponse updateHash(HttpResponse response) {
> this method doesn't really use any class state, so you could either:
> A) make it static as well
> B) refactor the top of this method into String getHash(HttpResponse
> response), which in turn can be mocked (or overridden) in the
> DefaultRequestPipeline test so you don't have to calculate data hash to
> evaluate the test.
>
> I'd lean toward the latter.


I will refactor according to B and do a unit test instead of integration
just for you  ;-)


>
> http://codereview.appspot.com/391042/show
>

[zhoresh, 2010-03-11 01:21:59]

Update according to john comments

[johnfargo, 2010-03-11 01:26:05]

Sounds good but don't get rid of the integration test, I'd just like to see the
unit test added :)

[zhoresh, 2010-03-11 01:45:05]

One more time with integration test

[johnfargo, 2010-03-11 02:34:24]

LGTM, committed.

[chirag, 2010-03-11 02:44:21]

http://codereview.appspot.com/391042/diff/12002/25005
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponseMetadataHelper.java
(right):

http://codereview.appspot.com/391042/diff/12002/25005#newcode50
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponseMetadataHelper.java:50:
// metadata.putAll(values);
Just curious, what was wrong with putAll?

[zhoresh, 2010-03-11 03:58:54]

On Mar 10, 2010, at 6:44 PM, chiragshah1@gmail.com wrote:

>
> http://codereview.appspot.com/391042/diff/12002/25005
> File
> java/gadgets/src/main/java/org/apache/shindig/gadgets/http/ 
> HttpResponseMetadataHelper.java
> (right):
>
> http://codereview.appspot.com/391042/diff/12002/25005#newcode50
> java/gadgets/src/main/java/org/apache/shindig/gadgets/http/ 
> HttpResponseMetadataHelper.java:50:
> // metadata.putAll(values);
> Just curious, what was wrong with putAll?

I had a problem with a test and I tried multiple things, this was just  
as part of my trail and error.
I should have put back the putAll

>
> http://codereview.appspot.com/391042/show