Re: svn commit: r687214 [1/3] - in /incubator/shindig/trunk/java: common/src/main/java/org/apache/shindig/common/uri/ common/src/test/java/org/apache/shindig/common/uri/ gadgets/src/main/java/org/apache/shindig/gadgets/ gadgets/src/main/java/org/apac

[Brian Eaton <be...@google.com>]

Had a look through this, looks good.  The securityToken field in
HttpRequest seems to be dead code.  We should probably remove that
field from either the HttpRequest class or the signing fetcher
classes, having it in both is confusing.

Having the gadgetUrl available in the HttpRequest is a bit risky, that
value can't be trusted unless it is pulled out of the security token.

I have a slight preference for grouping parameters together into
smaller classes, the umpteen fields in HttpRequest are a bit
confusing.  For example, signOwner and signViewer could be combined
into SigningParams, etc...

On Tue, Aug 19, 2008 at 6:14 PM,  <et...@apache.org> wrote:
> Author: etnu
> Date: Tue Aug 19 18:14:33 2008
> New Revision: 687214
>
> URL: http://svn.apache.org/viewvc?rev=687214&view=rev
> Log:
> Refactored HttpRequest into a more useful construct in order to facilitate ongoing work for SHINDIG-523 and SHINDIG-517.
>
> This isn't really much of a change, it just touches a lot of files since it replaces most usage of java.net.URI.
>
> Some interfaces at the edge of this code may need to be modified slightly, especially if they looked at HttpRequest.getUri directly. We're now using org.apache.shindig.common.uri.Uri instead of java.net.URI. If existing code still wants a java.net.URI, some new utility methods have been added to Uri to facilitate converstion.
>
> Much more work remains to be done in the rendering and network request pipeline. The next step will be updating HttpResponse objects along the same lines as HttpRequest (with some notable significant differences, since responses must be immutable).  Additional cleanup to remove remaining use of java.net.URI would also be good, though not critical.

Re: svn commit: r687214 [1/3] - in /incubator/shindig/trunk/java: common/src/main/java/org/apache/shindig/common/uri/ common/src/test/java/org/apache/shindig/common/uri/ gadgets/src/main/java/org/apache/shindig/gadgets/ gadgets/src/main/java/org/apac

[Kevin Brown <et...@google.com>]

On Thu, Aug 21, 2008 at 10:02 AM, Brian Eaton <be...@google.com> wrote:

> Had a look through this, looks good.  The securityToken field in
> HttpRequest seems to be dead code.  We should probably remove that
> field from either the HttpRequest class or the signing fetcher
> classes, having it in both is confusing.


Yes, I plan on removing it from signing fetcher, but I didn't want the patch
to grow too large. One piece at a time.


> Having the gadgetUrl available in the HttpRequest is a bit risky, that
> value can't be trusted unless it is pulled out of the security token.


The gadget url is used for a variety of purposes, such as determining
rewriter rules or for fetchers that need to log stats. It was on the
existing HttpRequest object, so I left it there.

We definitely wouldn't want to pass security tokens through the open proxy,
as that would render the results uncacheable. It certainly shouldn't be used
for security purposes, though.


>
>
> I have a slight preference for grouping parameters together into
> smaller classes, the umpteen fields in HttpRequest are a bit
> confusing.  For example, signOwner and signViewer could be combined
> into SigningParams, etc...


So do I, but I also wanted the code changes to be as small as possible for
the parts that weren't functional changes.


>
>
> On Tue, Aug 19, 2008 at 6:14 PM,  <et...@apache.org> wrote:
> > Author: etnu
> > Date: Tue Aug 19 18:14:33 2008
> > New Revision: 687214
> >
> > URL: http://svn.apache.org/viewvc?rev=687214&view=rev
> > Log:
> > Refactored HttpRequest into a more useful construct in order to
> facilitate ongoing work for SHINDIG-523 and SHINDIG-517.
> >
> > This isn't really much of a change, it just touches a lot of files since
> it replaces most usage of java.net.URI.
> >
> > Some interfaces at the edge of this code may need to be modified
> slightly, especially if they looked at HttpRequest.getUri directly. We're
> now using org.apache.shindig.common.uri.Uri instead of java.net.URI. If
> existing code still wants a java.net.URI, some new utility methods have been
> added to Uri to facilitate converstion.
> >
> > Much more work remains to be done in the rendering and network request
> pipeline. The next step will be updating HttpResponse objects along the same
> lines as HttpRequest (with some notable significant differences, since
> responses must be immutable).  Additional cleanup to remove remaining use of
> java.net.URI would also be good, though not critical.
>