Proxied image resizing

There are two patches squashed into one.  Since the second removes some extra work, it's ok to present them together.

---
    The code and cleanup to support proxied image resizing.
    
    Based on the initial work of Paul Lindner of Hi5.  His work in turn is
    based on further contributions, but I don't know at present which ones.
    
    New contributions are Copyright (c) 2009 Google.  Rest by Paul Lindner.
    
    o Reorganized the image rewriter implementation from Paul, and corrected
    a few bugs in the image dimensions computation.
    o A few readability changes.
    o Verified new behavior and added the tests to guard against accidental
    further behavior changes.
    o Modified the caching policy to take into account the resizing URL
    arguments required for resizing to work.
    o Factored the cache key building into a builder class that ensures
    backward compatibility of the generated cache keys, and makes building
    cache keys easier in general.
    o Added the tests to guard against accidental behavior changes in the
    cache key builder.
    o Fitted all the pieces back together again.

---

    Removed the text fixture that depends on the used JVM.
    
    The checksum-based tests turned out not to be portable across JVMs, so
    I had to exclude them, until I get a better idea on how to test the
    resize, without duplicating the actual resizing code.
    
    Changes Copyright (c) 2009 Google

[fmil, 2009-04-02 18:44:20]

This time with the new file CacheKeyBuilder.java and tests

[louiscryan, 2009-04-02 23:09:19]

Looks good, a few functional points.

http://codereview.appspot.com/32111/diff/19/1009
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/AbstractHttpCache.java
(right):

http://codereview.appspot.com/32111/diff/19/1009#newcode116
Line 116: * - the resize height parameter (sanitized)
...image resize...
also I think you can drop the (sanitized)

http://codereview.appspot.com/32111/diff/19/1009#newcode150
Line 150: // The old behavior in the code above was to return ownerId as-is. 
Since ownerId may be
I think you can make this a comment on the commit vs. a comment on the code. The
reason is fairly obvious.

http://codereview.appspot.com/32111/diff/19/1008
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/CacheKeyBuilder.java
(right):

http://codereview.appspot.com/32111/diff/19/1008#newcode29
Line 29: public class CacheKeyBuilder {
I think this is fine but in the long run I hate how we build keys, I'd rather
see something more like a name=value delimited string

http://codereview.appspot.com/32111/diff/19/1010
File java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpRequest.java
(right):

http://codereview.appspot.com/32111/diff/19/1010#newcode53
Line 53: private HttpServletRequest parentRequest;
rather than delegating to the original http request I think I'd rather create an
attribute map and have the values explicitly copied into it.

http://codereview.appspot.com/32111/diff/19/1010#newcode421
Line 421: public Integer getResizeWidth() {
I dont think going forward we want these methods here. Instead provide a public
get/setAttribute(key, value)

http://codereview.appspot.com/32111/diff/19/1017
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/BasicImageRewriter.java
(right):

http://codereview.appspot.com/32111/diff/19/1017#newcode121
Line 121: if (!isResizeRequested && response.getContentLength() <
config.getMinThresholdBytes()) {
This is a potential DOS attack. Given that this is an open service we have to
set limits even for the resize functionality.

http://codereview.appspot.com/32111/diff/19/1017#newcode134
Line 134: if (!isResizeRequested && isImageTooLarge(imageInfo)) {
Again a DOS vector.

http://codereview.appspot.com/32111/diff/19/1016
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/ImageRewriter.java
(right):

http://codereview.appspot.com/32111/diff/19/1016#newcode28
Line 28: @ImplementedBy(BasicImageRewriter.class)
I dont think we want this on by default, it should be opt in

[fmil, 2009-04-03 21:57:42]

I handled most of the issues you raised.  

The patch update will follow soon.

http://codereview.appspot.com/32111/diff/19/1009
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/AbstractHttpCache.java
(right):

http://codereview.appspot.com/32111/diff/19/1009#newcode116
Line 116: * - the resize height parameter (sanitized)
On 2009/04/02 23:09:20, louiscryan wrote:
> ...image resize...
> also I think you can drop the (sanitized)

Done.

http://codereview.appspot.com/32111/diff/19/1009#newcode150
Line 150: // The old behavior in the code above was to return ownerId as-is. 
Since ownerId may be
On 2009/04/02 23:09:20, louiscryan wrote:
> I think you can make this a comment on the commit vs. a comment on the code.
The
> reason is fairly obvious.

Done.

http://codereview.appspot.com/32111/diff/19/1008
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/CacheKeyBuilder.java
(right):

http://codereview.appspot.com/32111/diff/19/1008#newcode29
Line 29: public class CacheKeyBuilder {
On 2009/04/02 23:09:20, louiscryan wrote:
> I think this is fine but in the long run I hate how we build keys, I'd rather
> see something more like a name=value delimited string

Changed it.

The cache key had to remain backward-compatible so only the additions are now
keyvalue pairs.

http://codereview.appspot.com/32111/diff/19/1010
File java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpRequest.java
(right):

http://codereview.appspot.com/32111/diff/19/1010#newcode53
Line 53: private HttpServletRequest parentRequest;
On 2009/04/02 23:09:20, louiscryan wrote:
> rather than delegating to the original http request I think I'd rather create
an
> attribute map and have the values explicitly copied into it.

Done.

http://codereview.appspot.com/32111/diff/19/1010#newcode421
Line 421: public Integer getResizeWidth() {
On 2009/04/02 23:09:20, louiscryan wrote:
> I dont think going forward we want these methods here. Instead provide a
public
> get/setAttribute(key, value) 

Done.  I opted for typechecked variants though since we need to sanitize user
input.  There may be other more 'shindigy' ways to do it, please point out if
you know of them.

http://codereview.appspot.com/32111/diff/19/1017
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/BasicImageRewriter.java
(right):

http://codereview.appspot.com/32111/diff/19/1017#newcode121
Line 121: if (!isResizeRequested && response.getContentLength() <
config.getMinThresholdBytes()) {
On 2009/04/02 23:09:20, louiscryan wrote:
> This is a potential DOS attack. Given that this is an open service we have to
> set limits even for the resize functionality.

What to do here?

http://codereview.appspot.com/32111/diff/19/1017#newcode134
Line 134: if (!isResizeRequested && isImageTooLarge(imageInfo)) {
On 2009/04/02 23:09:20, louiscryan wrote:
> Again a DOS vector.

What to do here?

http://codereview.appspot.com/32111/diff/19/1016
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/ImageRewriter.java
(right):

http://codereview.appspot.com/32111/diff/19/1016#newcode28
Line 28: @ImplementedBy(BasicImageRewriter.class)
On 2009/04/02 23:09:20, louiscryan wrote:
> I dont think we want this on by default, it should be opt in

Done.

[fmil, 2009-04-07 02:53:59]

Updated after review comments.

[fmil, 2009-04-07 17:48:02]

Reinforces the image resizing tests

[fmil, 2009-04-07 17:55:05]

Removed stale comment

[fmil, 2009-04-07 17:57:49]

Intermittent commit increased the diff size, resending.

[fmil, 2009-04-07 17:59:39]

Readding new files

[etnu00, 2009-04-08 04:41:31]

http://codereview.appspot.com/32111/diff/1061/1069
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/AbstractHttpCache.java
(right):

http://codereview.appspot.com/32111/diff/1061/1069#newcode144
Line 144: .setResizeQuality(request.getParamAsInteger(PARAM_RESIZE_QUALITY));
The cache having explicit knowledge of image manipulation is really not
appropriate.

It would be a lot better to simply take all of those extra params that getParam*
are returning and append those.

The cache key can then become a set of key / value pairs (a map), without any
explicit knowledge of image processing.

http://codereview.appspot.com/32111/diff/1061/1068
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/CacheKeyBuilder.java
(right):

http://codereview.appspot.com/32111/diff/1061/1068#newcode33
Line 33: public class CacheKeyBuilder {
The strongly typed nature of this class is really overkill, and it creates a
situation where one class (this one) has to have explicit knowledge of many
parts of the system.

Just using the sorted map to build cache keys avoids this problem.

http://codereview.appspot.com/32111/diff/1061/1070
File java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpRequest.java
(right):

http://codereview.appspot.com/32111/diff/1061/1070#newcode55
Line 55: private final Map<String, String> params = Maps.newHashMap();
When adding this, all of the existing ad hoc params should be included as well.
This is probably best handled with a follow up patch for now though, to keep the
size of this change small.

http://codereview.appspot.com/32111/diff/1061/1070#newcode396
Line 396: String value = params.get(paramName);
This is a redundant hash lookup. Use params.get and perform a null check on that
instead of using containsKey.

http://codereview.appspot.com/32111/diff/1061/1070#newcode401
Line 401: params.put(paramName, paramValue.toString());
String.valueOf is a bit more robust here.

http://codereview.appspot.com/32111/diff/1061/1075
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/ImageUtils.java
(right):

http://codereview.appspot.com/32111/diff/1061/1075#newcode156
Line 156: {
This should be on the same line as the closing paren above.

http://codereview.appspot.com/32111/diff/1061/1071
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyBase.java
(right):

http://codereview.appspot.com/32111/diff/1061/1071#newcode129
Line 129: // additional referrer checks.
The HTTP spec actually spells this 'referer'. It' stupid, but it's not a mistake
here.

http://codereview.appspot.com/32111/diff/1061/1071#newcode148
Line 148: return true;
This makes it so that resized images will never be cached. I don't think that's
the behavior you actually want.

http://codereview.appspot.com/32111/diff/1061/1072
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyHandler.java
(right):

http://codereview.appspot.com/32111/diff/1061/1072#newcode69
Line 69: private HttpRequest buildHttpRequest(HttpServletRequest servletRequest)
throws GadgetException {
Please don't rename parameters or variables unless it's actually material to the
code in question. It complicates diffs unnecessarily.

[fmil, 2009-04-09 00:40:42]

Here's the answer to your comments.  

Can you please continue the review at:
http://codereview.appspot.com/32150

Thanks,
f

http://codereview.appspot.com/32111/diff/1061/1069
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/AbstractHttpCache.java
(right):

http://codereview.appspot.com/32111/diff/1061/1069#newcode144
Line 144: .setResizeQuality(request.getParamAsInteger(PARAM_RESIZE_QUALITY));
On 2009/04/08 04:41:31, etnu00 wrote:
> The cache having explicit knowledge of image manipulation is really not
> appropriate.
> It would be a lot better to simply take all of those extra params that
getParam*
> are returning and append those.

Done.

> The cache key can then become a set of key / value pairs (a map), without any
> explicit knowledge of image processing.

I understand that it is not possible to convert the cache key to a (sorted) map
in one go, as this would invalidate persistent caches.  

The current cache key builder takes care that the keys don't change for the URLs
that don't use the extra parameters.

http://codereview.appspot.com/32111/diff/1061/1068
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/CacheKeyBuilder.java
(right):

http://codereview.appspot.com/32111/diff/1061/1068#newcode33
Line 33: public class CacheKeyBuilder {
On 2009/04/08 04:41:31, etnu00 wrote:
> The strongly typed nature of this class is really overkill, and it creates a
> situation where one class (this one) has to have explicit knowledge of many
> parts of the system.
> 
> Just using the sorted map to build cache keys avoids this problem.

Done, modulo the issue with the key backward compatibility.

http://codereview.appspot.com/32111/diff/1061/1070
File java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpRequest.java
(right):

http://codereview.appspot.com/32111/diff/1061/1070#newcode55
Line 55: private final Map<String, String> params = Maps.newHashMap();
On 2009/04/08 04:41:31, etnu00 wrote:
> When adding this, all of the existing ad hoc params should be included as
well.
> This is probably best handled with a follow up patch for now though, to keep
the
> size of this change small.

Yes please, I prefer separating this.

http://codereview.appspot.com/32111/diff/1061/1070#newcode396
Line 396: String value = params.get(paramName);
On 2009/04/08 04:41:31, etnu00 wrote:
> This is a redundant hash lookup. Use params.get and perform a null check on
that
> instead of using containsKey.

Done.

http://codereview.appspot.com/32111/diff/1061/1070#newcode401
Line 401: params.put(paramName, paramValue.toString());
On 2009/04/08 04:41:31, etnu00 wrote:
> String.valueOf is a bit more robust here.

Done.

http://codereview.appspot.com/32111/diff/1061/1075
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/ImageUtils.java
(right):

http://codereview.appspot.com/32111/diff/1061/1075#newcode156
Line 156: {
On 2009/04/08 04:41:31, etnu00 wrote:
> This should be on the same line as the closing paren above.

Done.

http://codereview.appspot.com/32111/diff/1061/1071
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyBase.java
(right):

http://codereview.appspot.com/32111/diff/1061/1071#newcode129
Line 129: // additional referrer checks.
On 2009/04/08 04:41:31, etnu00 wrote:
> The HTTP spec actually spells this 'referer'. It' stupid, but it's not a
mistake
> here.

Done, pardon the wrongful exercise of OCD.

http://codereview.appspot.com/32111/diff/1061/1071#newcode148
Line 148: return true;
On 2009/04/08 04:41:31, etnu00 wrote:
> This makes it so that resized images will never be cached. I don't think
that's
> the behavior you actually want.

Done.

http://codereview.appspot.com/32111/diff/1061/1072
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyHandler.java
(right):

http://codereview.appspot.com/32111/diff/1061/1072#newcode69
Line 69: private HttpRequest buildHttpRequest(HttpServletRequest servletRequest)
throws GadgetException {
On 2009/04/08 04:41:31, etnu00 wrote:
> Please don't rename parameters or variables unless it's actually material to
the
> code in question. It complicates diffs unnecessarily.

Done.