Handling post requests for Accel servlet

[gagan.goku, 2010-07-13 02:20:28]

adding tests

[gagan.goku, 2010-07-13 03:07:36]

beautification

[anupama.dutta, 2010-07-13 15:48:50]

http://codereview.appspot.com/1822041/diff/9001/10004
File main/java/org/apache/shindig/gadgets/uri/UriUtils.java (right):

http://codereview.appspot.com/1822041/diff/9001/10004#newcode70
main/java/org/apache/shindig/gadgets/uri/UriUtils.java:70: // Headers that the
fetcher itself would like to fill.
This comment seems slightly cryptic to me - could you give a few more details as
to why the fetcher needs to fill this. Could you also add a comment (similar to
38-42) just before the enum to explain what this enum is meant for?

http://codereview.appspot.com/1822041/diff/9001/10004#newcode71
main/java/org/apache/shindig/gadgets/uri/UriUtils.java:71:
POST_INCOMPATIBLE_HEADERS(ImmutableSet.of("content-length"));
In order to use consistent terminology, would POST_INCOMPATIBLE_DIRECTIVES be a
better name here?

http://codereview.appspot.com/1822041/diff/9001/10004#newcode154
main/java/org/apache/shindig/gadgets/uri/UriUtils.java:154: public static void
copyRequestHeaders(HttpServletRequest request,
Similar to the previous method, using "data" for this parameter might make this
more readable.

http://codereview.appspot.com/1822041/diff/9001/10004#newcode158
main/java/org/apache/shindig/gadgets/uri/UriUtils.java:158: for
(DisallowedRequestHeaders h : disallowedResponseHeaders) {
disallowedResponseHeaders -> disallowedRequestHeaders.

http://codereview.appspot.com/1822041/diff/9001/10004#newcode162
main/java/org/apache/shindig/gadgets/uri/UriUtils.java:162: Enumeration<String>
headerNames = request.getHeaderNames();
Wouldn't using a block similar to 136-141 work here as well (with a small change
that uses addHeader instead of setHeader)?

http://codereview.appspot.com/1822041/diff/9001/10004#newcode190
main/java/org/apache/shindig/gadgets/uri/UriUtils.java:190: Enumeration<String>
paramNames = request.getParameterNames();
Just wondering - Isn't there a shorter Map.Entry<String, String> kind of block
that can be used here?

[gagan.goku, 2010-07-13 17:25:54]

addressing comments

[gagan.goku, 2010-07-13 17:26:24]

http://codereview.appspot.com/1822041/diff/9001/10004
File main/java/org/apache/shindig/gadgets/uri/UriUtils.java (right):

http://codereview.appspot.com/1822041/diff/9001/10004#newcode70
main/java/org/apache/shindig/gadgets/uri/UriUtils.java:70: // Headers that the
fetcher itself would like to fill.
On 2010/07/13 15:48:50, anupama.dutta wrote:
> This comment seems slightly cryptic to me - could you give a few more details
as
> to why the fetcher needs to fill this. Could you also add a comment (similar
to
> 38-42) just before the enum to explain what this enum is meant for?

Done.

http://codereview.appspot.com/1822041/diff/9001/10004#newcode71
main/java/org/apache/shindig/gadgets/uri/UriUtils.java:71:
POST_INCOMPATIBLE_HEADERS(ImmutableSet.of("content-length"));
On 2010/07/13 15:48:50, anupama.dutta wrote:
> In order to use consistent terminology, would POST_INCOMPATIBLE_DIRECTIVES be
a
> better name here?

Done.

http://codereview.appspot.com/1822041/diff/9001/10004#newcode154
main/java/org/apache/shindig/gadgets/uri/UriUtils.java:154: public static void
copyRequestHeaders(HttpServletRequest request,
On 2010/07/13 15:48:50, anupama.dutta wrote:
> Similar to the previous method, using "data" for this parameter might make
this
> more readable.

Done.

http://codereview.appspot.com/1822041/diff/9001/10004#newcode158
main/java/org/apache/shindig/gadgets/uri/UriUtils.java:158: for
(DisallowedRequestHeaders h : disallowedResponseHeaders) {
On 2010/07/13 15:48:50, anupama.dutta wrote:
> disallowedResponseHeaders -> disallowedRequestHeaders.

Done.

http://codereview.appspot.com/1822041/diff/9001/10004#newcode162
main/java/org/apache/shindig/gadgets/uri/UriUtils.java:162: Enumeration<String>
headerNames = request.getHeaderNames();
On 2010/07/13 15:48:50, anupama.dutta wrote:
> Wouldn't using a block similar to 136-141 work here as well (with a small
change
> that uses addHeader instead of setHeader)?

I couldnt find an easy way of getting headers from HttpServletRequest. As far as
i know, this is the only way.
In that case, this block is way too different from the previous one :(

http://codereview.appspot.com/1822041/diff/9001/10004#newcode190
main/java/org/apache/shindig/gadgets/uri/UriUtils.java:190: Enumeration<String>
paramNames = request.getParameterNames();
On 2010/07/13 15:48:50, anupama.dutta wrote:
> Just wondering - Isn't there a shorter Map.Entry<String, String> kind of block
> that can be used here?

There is a getParameterMap function in ServletRequest which seems to return Map,
but not Map<String, String[]>

I dont know how to safely convert it to Map<String, String[]>
Also, that block will probably be the same number of lines.

[gagan.goku, 2010-07-13 18:31:50]

reverting basichttpfetcher

[gagan.goku, 2010-07-13 18:39:44]

fixing comment

[gagan.goku, 2010-07-14 10:43:54]

setting follow redirect to false for accel

[johnfargo, 2010-07-14 20:39:37]

Somehow the patch got out of sync w/ the current Shindig (chunk mismatch) --
give it a quick update. Thx

http://codereview.appspot.com/1822041/diff/33001/34001
File main/java/org/apache/shindig/gadgets/http/HttpRequest.java (right):

http://codereview.appspot.com/1822041/diff/33001/34001#newcode54
main/java/org/apache/shindig/gadgets/http/HttpRequest.java:54: // TODO: Convert
to Map<String, List<String>> to allow multiple values for a key.
@see comment in UriUtils. These are internal params, not resource URI params, so
the String, String mapping is intentional.

http://codereview.appspot.com/1822041/diff/33001/34002
File main/java/org/apache/shindig/gadgets/rewrite/ProxyingContentRewriter.java
(right):

http://codereview.appspot.com/1822041/diff/33001/34002#newcode53
main/java/org/apache/shindig/gadgets/rewrite/ProxyingContentRewriter.java:53: //
       new ConcatVisitor.Css(config, concatUriManager),
Why commenting these out? That there are 3 visitors here is key to the
rewriter's functionality. It ensures that concatenated resources don't later get
proxied.

http://codereview.appspot.com/1822041/diff/33001/34003
File main/java/org/apache/shindig/gadgets/servlet/AccelHandler.java (right):

http://codereview.appspot.com/1822041/diff/33001/34003#newcode182
main/java/org/apache/shindig/gadgets/servlet/AccelHandler.java:182: // Copy the
post body.
if it exists, presumably

http://codereview.appspot.com/1822041/diff/33001/34005
File main/java/org/apache/shindig/gadgets/uri/UriUtils.java (right):

http://codereview.appspot.com/1822041/diff/33001/34005#newcode76
main/java/org/apache/shindig/gadgets/uri/UriUtils.java:76: public enum
DisallowedRequestHeaders {
could just simplify this by calling it DisallowedHeaders. Whether they're used
in Request or Response context can be a matter of convention (naming) and/or
simple use.

http://codereview.appspot.com/1822041/diff/33001/34005#newcode146
main/java/org/apache/shindig/gadgets/uri/UriUtils.java:146:
resp.addHeader(entry.getKey(), entry.getValue());
I'd argue that set is still appropriate, since the semantics of
copyResponseHeadersAndStatusCode have been to fully replace the response's
headers w/ those provided.

http://codereview.appspot.com/1822041/diff/33001/34005#newcode181
main/java/org/apache/shindig/gadgets/uri/UriUtils.java:181: if (headerValues !=
null && isValidHeaderName(header) &&
probably want to do a headerValues.hasMoreElements() check.

http://codereview.appspot.com/1822041/diff/33001/34005#newcode213
main/java/org/apache/shindig/gadgets/uri/UriUtils.java:213:
req.setParam(paramName, value);
This underscores a really confusing note in our API. HttpRequest.set/getParam()
isn't a representation of URI params -- it's a representation of
"internal"/contextual params used by the system.

Specifically, get/setParam() is only used for image processing, as a way of
sending resizing parameters to the rewriter mechanism. That's because these
params appear in the proxy URI, *not* the resource URI.

In the case of Accel, params should appear in the resource URI, I presume. Or
did you intend this for passing custom internal config params to accel's
handlers?

[gagan.goku, 2010-07-15 06:19:58]

addressing comments

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

http://codereview.appspot.com/1822041/diff/33001/34001
File main/java/org/apache/shindig/gadgets/http/HttpRequest.java (right):

http://codereview.appspot.com/1822041/diff/33001/34001#newcode54
main/java/org/apache/shindig/gadgets/http/HttpRequest.java:54: // TODO: Convert
to Map<String, List<String>> to allow multiple values for a key.
On 2010/07/14 20:39:37, johnfargo wrote:
> @see comment in UriUtils. These are internal params, not resource URI params,
so
> the String, String mapping is intentional.

Makes sense then. Reverting this now.

http://codereview.appspot.com/1822041/diff/33001/34002
File main/java/org/apache/shindig/gadgets/rewrite/ProxyingContentRewriter.java
(right):

http://codereview.appspot.com/1822041/diff/33001/34002#newcode53
main/java/org/apache/shindig/gadgets/rewrite/ProxyingContentRewriter.java:53: //
       new ConcatVisitor.Css(config, concatUriManager),
On 2010/07/14 20:39:37, johnfargo wrote:
> Why commenting these out? That there are 3 visitors here is key to the
> rewriter's functionality. It ensures that concatenated resources don't later
get
> proxied.

Sorry about that. I commented these out for internal testing and forgot to
revert this file.
Really sorry.

http://codereview.appspot.com/1822041/diff/33001/34003
File main/java/org/apache/shindig/gadgets/servlet/AccelHandler.java (right):

http://codereview.appspot.com/1822041/diff/33001/34003#newcode182
main/java/org/apache/shindig/gadgets/servlet/AccelHandler.java:182: // Copy the
post body.
On 2010/07/14 20:39:37, johnfargo wrote:
> if it exists, presumably

Done.

http://codereview.appspot.com/1822041/diff/33001/34005
File main/java/org/apache/shindig/gadgets/uri/UriUtils.java (right):

http://codereview.appspot.com/1822041/diff/33001/34005#newcode76
main/java/org/apache/shindig/gadgets/uri/UriUtils.java:76: public enum
DisallowedRequestHeaders {
On 2010/07/14 20:39:37, johnfargo wrote:
> could just simplify this by calling it DisallowedHeaders. Whether they're used
> in Request or Response context can be a matter of convention (naming) and/or
> simple use.

Wow. Now that you mention it, it really sounds so simple.
Done.

http://codereview.appspot.com/1822041/diff/33001/34005#newcode146
main/java/org/apache/shindig/gadgets/uri/UriUtils.java:146:
resp.addHeader(entry.getKey(), entry.getValue());
On 2010/07/14 20:39:37, johnfargo wrote:
> I'd argue that set is still appropriate, since the semantics of
> copyResponseHeadersAndStatusCode have been to fully replace the response's
> headers w/ those provided.

But there could be multiple header values for the same header, like:
Set-Cookie: NAME=value; domain=hello
Set-Cookie: NAME2=value2; domain=buffalo

Set header would just retain the last one (i hope thats why HttpServletResponse
has addHeader and setHeader functions)

http://codereview.appspot.com/1822041/diff/33001/34005#newcode181
main/java/org/apache/shindig/gadgets/uri/UriUtils.java:181: if (headerValues !=
null && isValidHeaderName(header) &&
On 2010/07/14 20:39:37, johnfargo wrote:
> probably want to do a headerValues.hasMoreElements() check.

Done.

http://codereview.appspot.com/1822041/diff/33001/34005#newcode213
main/java/org/apache/shindig/gadgets/uri/UriUtils.java:213:
req.setParam(paramName, value);
On 2010/07/14 20:39:37, johnfargo wrote:
> This underscores a really confusing note in our API.
HttpRequest.set/getParam()
> isn't a representation of URI params -- it's a representation of
> "internal"/contextual params used by the system.
> 
> Specifically, get/setParam() is only used for image processing, as a way of
> sending resizing parameters to the rewriter mechanism. That's because these
> params appear in the proxy URI, *not* the resource URI.
> 
> In the case of Accel, params should appear in the resource URI, I presume. Or
> did you intend this for passing custom internal config params to accel's
> handlers?

I had thought that it was the equivalent of HttpServletRequest params, which are
get/post parameter values.
Might make sense to add a comment on the param variable so its more clear.

[johnfargo, 2010-07-15 22:46:23]

Hey Gagan:

Thanks for the responses. Brief comments inline, then continuing the review.

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

>
> http://codereview.appspot.com/1822041/diff/33001/34001
> File main/java/org/apache/shindig/gadgets/http/HttpRequest.java (right):
>
> http://codereview.appspot.com/1822041/diff/33001/34001#newcode54
> main/java/org/apache/shindig/gadgets/http/HttpRequest.java:54: // TODO:
> Convert to Map<String, List<String>> to allow multiple values for a key.
> On 2010/07/14 20:39:37, johnfargo wrote:
>
>> @see comment in UriUtils. These are internal params, not resource URI
>>
> params, so
>
>> the String, String mapping is intentional.
>>
>
> Makes sense then. Reverting this now.
>
>
> http://codereview.appspot.com/1822041/diff/33001/34002
> File
> main/java/org/apache/shindig/gadgets/rewrite/ProxyingContentRewriter.java
> (right):
>
> http://codereview.appspot.com/1822041/diff/33001/34002#newcode53
>
> main/java/org/apache/shindig/gadgets/rewrite/ProxyingContentRewriter.java:53:
> //        new ConcatVisitor.Css(config, concatUriManager),
> On 2010/07/14 20:39:37, johnfargo wrote:
>
>> Why commenting these out? That there are 3 visitors here is key to the
>> rewriter's functionality. It ensures that concatenated resources don't
>>
> later get
>
>> proxied.
>>
>
> Sorry about that. I commented these out for internal testing and forgot
> to revert this file.
> Really sorry.


No worries, I once accidentally committed test code that pointed to my local
directory structure.


>
>
> http://codereview.appspot.com/1822041/diff/33001/34003
> File main/java/org/apache/shindig/gadgets/servlet/AccelHandler.java
> (right):
>
> http://codereview.appspot.com/1822041/diff/33001/34003#newcode182
> main/java/org/apache/shindig/gadgets/servlet/AccelHandler.java:182: //
> Copy the post body.
> On 2010/07/14 20:39:37, johnfargo wrote:
>
>> if it exists, presumably
>>
>
> Done.
>
>
> http://codereview.appspot.com/1822041/diff/33001/34005
> File main/java/org/apache/shindig/gadgets/uri/UriUtils.java (right):
>
> http://codereview.appspot.com/1822041/diff/33001/34005#newcode76
> main/java/org/apache/shindig/gadgets/uri/UriUtils.java:76: public enum
> DisallowedRequestHeaders {
> On 2010/07/14 20:39:37, johnfargo wrote:
>
>> could just simplify this by calling it DisallowedHeaders. Whether
>>
> they're used
>
>> in Request or Response context can be a matter of convention (naming)
>>
> and/or
>
>> simple use.
>>
>
> Wow. Now that you mention it, it really sounds so simple.
> Done.
>
>
> http://codereview.appspot.com/1822041/diff/33001/34005#newcode146
> main/java/org/apache/shindig/gadgets/uri/UriUtils.java:146:
> resp.addHeader(entry.getKey(), entry.getValue());
> On 2010/07/14 20:39:37, johnfargo wrote:
>
>> I'd argue that set is still appropriate, since the semantics of
>> copyResponseHeadersAndStatusCode have been to fully replace the
>>
> response's
>
>> headers w/ those provided.
>>
>
> But there could be multiple header values for the same header, like:
> Set-Cookie: NAME=value; domain=hello
> Set-Cookie: NAME2=value2; domain=buffalo
>
> Set header would just retain the last one (i hope thats why
> HttpServletResponse has addHeader and setHeader functions)


Agreed w/ the rationale; I'm just suggesting not changing the API's
semantics out of an abundance of caution. Perhaps it's over-paranoid, but
I'd prefer to see a new API added that does addHeader rather than setHeader.
The two could be differentiated on a boolean, w/ both deferring to a helper
method (basically this one, w/ a new bool applied).


>
>
> http://codereview.appspot.com/1822041/diff/33001/34005#newcode181
> main/java/org/apache/shindig/gadgets/uri/UriUtils.java:181: if
> (headerValues != null && isValidHeaderName(header) &&
> On 2010/07/14 20:39:37, johnfargo wrote:
>
>> probably want to do a headerValues.hasMoreElements() check.
>>
>
> Done.
>
>
> http://codereview.appspot.com/1822041/diff/33001/34005#newcode213
> main/java/org/apache/shindig/gadgets/uri/UriUtils.java:213:
> req.setParam(paramName, value);
> On 2010/07/14 20:39:37, johnfargo wrote:
>
>> This underscores a really confusing note in our API.
>>
> HttpRequest.set/getParam()
>
>> isn't a representation of URI params -- it's a representation of
>> "internal"/contextual params used by the system.
>>
>
>  Specifically, get/setParam() is only used for image processing, as a
>>
> way of
>
>> sending resizing parameters to the rewriter mechanism. That's because
>>
> these
>
>> params appear in the proxy URI, *not* the resource URI.
>>
>
>  In the case of Accel, params should appear in the resource URI, I
>>
> presume. Or
>
>> did you intend this for passing custom internal config params to
>>
> accel's
>
>> handlers?
>>
>
> I had thought that it was the equivalent of HttpServletRequest params,
> which are get/post parameter values.
> Might make sense to add a comment on the param variable so its more
> clear.


Agreed, feel free to add one. I did a triple-take when reading this CL
myself due to this existing confusion.


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

[gagan.goku, 2010-07-16 19:55:46]

>> But there could be multiple header values for the same header, like:
>> Set-Cookie: NAME=value; domain=hello
>> Set-Cookie: NAME2=value2; domain=buffalo
>>
>> Set header would just retain the last one (i hope thats why
>> HttpServletResponse has addHeader and setHeader functions)
>
>
> Agreed w/ the rationale; I'm just suggesting not changing the API's
> semantics out of an abundance of caution. Perhaps it's over-paranoid, but
> I'd prefer to see a new API added that does addHeader rather than setHeader.
> The two could be differentiated on a boolean, w/ both deferring to a helper
> method (basically this one, w/ a new bool applied).

Added a boolean for the same. Not making 2 functions as currently there is only
1 usage of copyResponseHeadersAndStatusCode.

[gagan.goku, 2010-07-16 19:57:28]

addressing comments

[johnfargo, 2010-07-19 17:58:26]

LGTM, looks great!

Patch applied, tested, and committed.

On Fri, Jul 16, 2010 at 12:57 PM, <gagan.goku@gmail.com> wrote:

> addressing comments
>
>
> http://codereview.appspot.com/1822041/show
>

[gagan.goku, 2010-07-20 01:29:20]

Thanks John.

On Mon, Jul 19, 2010 at 11:28 PM, John Hjelmstad <johnfargo@gmail.com>wrote:

> LGTM, looks great!
>
> Patch applied, tested, and committed.
>
>
> On Fri, Jul 16, 2010 at 12:57 PM, <gagan.goku@gmail.com> wrote:
>
>> addressing comments
>>
>>
>> http://codereview.appspot.com/1822041/show
>>
>
>