Support chain style paramaters fror accel servlet

To support relative urls in an accelerate page we should support chained style paramaters.
For example:
http://shindig.org/gadgets/accel//http://www.cnn.com/

It does not resolve the issue of no host but absolute pass.
I also left open the support of added params to the servlet

[johnfargo, 2010-04-21 22:47:48]

http://codereview.appspot.com/969041/diff/1/3
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/HtmlAccelServlet.java
(right):

http://codereview.appspot.com/969041/diff/1/3#newcode237
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/HtmlAccelServlet.java:237:
String path = req.getRequestURI();
slight preference to use the UriBuilder(HttpServletRequest) mechanism to build a
Uri, then pull it apart. Avoids split logic dealing w/ HSR <-> URI parsing.

http://codereview.appspot.com/969041/diff/1/3#newcode241
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/HtmlAccelServlet.java:241:
if (path.startsWith(accelServletPrefix + "/")) {
it's been a while since I've used this, so I'd defer to others with more recent
experience as to the reliability of this method -- but it seems:
http://java.sun.com/products/servlet/2.2/javadoc/javax/servlet/http/HttpServl...

..may provide accelServletPrefix without any additional configuration.

http://codereview.appspot.com/969041/diff/1/3#newcode250
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/HtmlAccelServlet.java:250:
if (accelUrl == null){
s/null check/!StringUtils.isEmptyOrWhitespace(...)/

[zhoresh, 2010-04-22 00:13:11]

On Wed, Apr 21, 2010 at 3:47 PM, <johnfargo@gmail.com> wrote:

>
> http://codereview.appspot.com/969041/diff/1/3
> File
>
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/HtmlAccelServlet.java
> (right):
>
> http://codereview.appspot.com/969041/diff/1/3#newcode237
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/HtmlAccelServlet.java:237:
> String path = req.getRequestURI();
> slight preference to use the UriBuilder(HttpServletRequest) mechanism to
> build a Uri, then pull it apart. Avoids split logic dealing w/ HSR <->
> URI parsing.
>
> http://codereview.appspot.com/969041/diff/1/3#newcode241
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/HtmlAccelServlet.java:241:
> if (path.startsWith(accelServletPrefix + "/")) {
> it's been a while since I've used this, so I'd defer to others with more
> recent experience as to the reliability of this method -- but it seems:
>
>
http://java.sun.com/products/servlet/2.2/javadoc/javax/servlet/http/HttpServl...
>
> ..may provide accelServletPrefix without any additional configuration.
>
I am not crazy about this either, but I didn't find a better way.
Basically I need the full path here not just the servlet part in order to
figure out if it is parametrized or chained.
 I will try to see if it work or there is a better way.


> http://codereview.appspot.com/969041/diff/1/3#newcode250
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/HtmlAccelServlet.java:250:
> if (accelUrl == null){
> s/null check/!StringUtils.isEmptyOrWhitespace(...)/
>
>
> http://codereview.appspot.com/969041/show
>

[Paul Lindner, 2010-04-22 00:38:43]

If we agree to move to servlet-api-2.5 we can use getContextPath()

http://java.sun.com/javaee/5/docs/api/javax/servlet/ServletContext.html#getCo...

On Apr 21, 2010, at 5:12 PM, Ziv Horesh wrote:

> On Wed, Apr 21, 2010 at 3:47 PM, <johnfargo@gmail.com> wrote:
> 
>> 
>> http://codereview.appspot.com/969041/diff/1/3
>> File
>> 
>> 
>>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/HtmlAccelServlet.java
>> (right):
>> 
>> http://codereview.appspot.com/969041/diff/1/3#newcode237
>> 
>>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/HtmlAccelServlet.java:237:
>> String path = req.getRequestURI();
>> slight preference to use the UriBuilder(HttpServletRequest) mechanism to
>> build a Uri, then pull it apart. Avoids split logic dealing w/ HSR <->
>> URI parsing.
>> 
>> http://codereview.appspot.com/969041/diff/1/3#newcode241
>> 
>>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/HtmlAccelServlet.java:241:
>> if (path.startsWith(accelServletPrefix + "/")) {
>> it's been a while since I've used this, so I'd defer to others with more
>> recent experience as to the reliability of this method -- but it seems:
>> 
>>
http://java.sun.com/products/servlet/2.2/javadoc/javax/servlet/http/HttpServl...
>> 
>> ..may provide accelServletPrefix without any additional configuration.
>> 
> I am not crazy about this either, but I didn't find a better way.
> Basically I need the full path here not just the servlet part in order to
> figure out if it is parametrized or chained.
> I will try to see if it work or there is a better way.
> 
> 
>> http://codereview.appspot.com/969041/diff/1/3#newcode250
>> 
>>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/HtmlAccelServlet.java:250:
>> if (accelUrl == null){
>> s/null check/!StringUtils.isEmptyOrWhitespace(...)/
>> 
>> 
>> http://codereview.appspot.com/969041/show
>> 

[zhoresh, 2010-04-22 00:44:07]

On Wed, Apr 21, 2010 at 5:38 PM, Paul Lindner <lindner@inuus.com> wrote:

> If we agree to move to servlet-api-2.5 we can use getContextPath()
>

I actually started with it, but it rreturned empty string in current setup
:-)


>
>
>
http://java.sun.com/javaee/5/docs/api/javax/servlet/ServletContext.html#getCo...
>
> On Apr 21, 2010, at 5:12 PM, Ziv Horesh wrote:
>
> > On Wed, Apr 21, 2010 at 3:47 PM, <johnfargo@gmail.com> wrote:
> >
> >>
> >> http://codereview.appspot.com/969041/diff/1/3
> >> File
> >>
> >>
> >>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/HtmlAccelServlet.java
> >> (right):
> >>
> >> http://codereview.appspot.com/969041/diff/1/3#newcode237
> >>
> >>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/HtmlAccelServlet.java:237:
> >> String path = req.getRequestURI();
> >> slight preference to use the UriBuilder(HttpServletRequest) mechanism to
> >> build a Uri, then pull it apart. Avoids split logic dealing w/ HSR <->
> >> URI parsing.
> >>
> >> http://codereview.appspot.com/969041/diff/1/3#newcode241
> >>
> >>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/HtmlAccelServlet.java:241:
> >> if (path.startsWith(accelServletPrefix + "/")) {
> >> it's been a while since I've used this, so I'd defer to others with more
> >> recent experience as to the reliability of this method -- but it seems:
> >>
> >>
>
http://java.sun.com/products/servlet/2.2/javadoc/javax/servlet/http/HttpServl...
> <
>
http://java.sun.com/products/servlet/2.2/javadoc/javax/servlet/http/HttpServl...
> >
> >>
> >> ..may provide accelServletPrefix without any additional configuration.
> >>
> > I am not crazy about this either, but I didn't find a better way.
> > Basically I need the full path here not just the servlet part in order to
> > figure out if it is parametrized or chained.
> > I will try to see if it work or there is a better way.
> >
> >
> >> http://codereview.appspot.com/969041/diff/1/3#newcode250
> >>
> >>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/HtmlAccelServlet.java:250:
> >> if (accelUrl == null){
> >> s/null check/!StringUtils.isEmptyOrWhitespace(...)/
> >>
> >>
> >> http://codereview.appspot.com/969041/show
> >>
>
>

[zhoresh, 2010-04-23 01:43:23]

Use getServletPath as suggested

[zhoresh, 2010-04-23 03:26:58]

Use UriBuilder to parse the request

[johnfargo, 2010-04-23 03:52:28]

Looks pretty good; one last question.

http://codereview.appspot.com/969041/diff/11001/12002
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/HtmlAccelServlet.java
(right):

http://codereview.appspot.com/969041/diff/11001/12002#newcode114
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/HtmlAccelServlet.java:114:
data = fetch(uri, DEFAULT_CONTAINER);
this drops any custom container info, and importantly, ignoreCache behavior from
the request. intended?

[johnfargo, 2010-04-23 08:11:57]

one more nit while I'm at it.

http://codereview.appspot.com/969041/diff/11001/12002
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/HtmlAccelServlet.java
(right):

http://codereview.appspot.com/969041/diff/11001/12002#newcode229
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/HtmlAccelServlet.java:229:
private Uri getAccelRequest(HttpServletRequest req) throws GadgetException {
nit: suggest renaming to getAccelUri, and make protected since it will likely
prove a useful helper method for subclassers.

[zhoresh, 2010-04-23 16:47:19]

The change remove the support for refresh and container intentionally. I
don't think it add much value at this point.
The getAccelRequest suppose to also return the parameters at one point.
So I think instead of renaming I will just implement it correctly with
paramater parsing.
It will be called getAccelParams, which return map of parameters. It will
parse chain or parametrized style,
fill in the url, and some defaults such as a container.

Attachment later today.


On Fri, Apr 23, 2010 at 1:11 AM, <johnfargo@gmail.com> wrote:

> one more nit while I'm at it.
>
>
>
> http://codereview.appspot.com/969041/diff/11001/12002
> File
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/HtmlAccelServlet.java
> (right):
>
> http://codereview.appspot.com/969041/diff/11001/12002#newcode229
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/HtmlAccelServlet.java:229:
> private Uri getAccelRequest(HttpServletRequest req) throws
> GadgetException {
> nit: suggest renaming to getAccelUri, and make protected since it will
> likely prove a useful helper method for subclassers.
>
>
> http://codereview.appspot.com/969041/show
>

[zhoresh, 2010-04-23 18:24:31]

Add parsing of paramaters in chain style (also fix cache control time)

[johnfargo, 2010-04-29 22:23:17]

Final nits and minor suggestions. LGTM in general; fix what you wish and submit
(first CL submitted I believe!)

http://codereview.appspot.com/969041/diff/16002/18002
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/HtmlAccelServlet.java
(right):

http://codereview.appspot.com/969041/diff/16002/18002#newcode112
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/HtmlAccelServlet.java:112:
// Create request to hanlde parsed params
nit: s/hanlde/handle/

http://codereview.appspot.com/969041/diff/16002/18002#newcode117
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/HtmlAccelServlet.java:117:
return requestParams.get(name).get(0);
nit: this does a double-lookup. I'd just do:
List<String> values = reqParams.get(name);
if (values != null && values.size() > 0) return values.get(0);

http://codereview.appspot.com/969041/diff/16002/18002#newcode170
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/HtmlAccelServlet.java:170:
return requestParams.get(name).get(0);
could just use dataWrapper.getParameter(name)

[zhoresh, 2010-04-30 16:34:34]

All done and submitted!

On 2010/04/29 22:23:17, johnfargo wrote:
> Final nits and minor suggestions. LGTM in general; fix what you wish and
submit
> (first CL submitted I believe!)
> 
> http://codereview.appspot.com/969041/diff/16002/18002
> File
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/HtmlAccelServlet.java
> (right):
> 
> http://codereview.appspot.com/969041/diff/16002/18002#newcode112
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/HtmlAccelServlet.java:112:
> // Create request to hanlde parsed params
> nit: s/hanlde/handle/
> 
> http://codereview.appspot.com/969041/diff/16002/18002#newcode117
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/HtmlAccelServlet.java:117:
> return requestParams.get(name).get(0);
> nit: this does a double-lookup. I'd just do:
> List<String> values = reqParams.get(name);
> if (values != null && values.size() > 0) return values.get(0);
> 
> http://codereview.appspot.com/969041/diff/16002/18002#newcode170
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/HtmlAccelServlet.java:170:
> return requestParams.get(name).get(0);
> could just use dataWrapper.getParameter(name)