Re: Introduce ProxyUriManager (issue217110)

[zh...@gmail.com]

Here are few comments.
I still think that both the manager and the versioner don't need to work
on a list but rather on one request at a time.

I think you will need a separate service to group multiple versions
requests for a batch requests.
You can have walker that first scn all resources and send one batch
request to get all resources metadata.

Also in the validation you need to check the version against the actual
resource that is going to be served (meaning that when creating the
proxy url you can use a separate cache of versions for resources, but
when verifying you should check verify the actual served resource).




http://codereview.appspot.com/217110/diff/1/3
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultProxyUriManager.java
(right):

http://codereview.appspot.com/217110/diff/1/3#newcode50
java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultProxyUriManager.java:50:
*
http://www.example.com/gadgets/proxy/refresh=1&.../http://www.foo.com/img.gif
Add start and end beacon in the example.
btw - is that a new feature? where is it currently implemented?

http://codereview.appspot.com/217110/diff/1/3#newcode87
java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultProxyUriManager.java:87:
versions.put(resource, versionList.get(i++));
use iterator instead of index

http://codereview.appspot.com/217110/diff/1/3#newcode141
java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultProxyUriManager.java:141:
public ProxyUri validateProxiedUri(Uri uriIn) throws GadgetException {
It actualy parsing the uri not just validating, so I would rename this
to parseProxiedUri (which apply through out the UriManager pattern)

http://codereview.appspot.com/217110/diff/1/3#newcode151
java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultProxyUriManager.java:151:
config.getString(container,
PROXY_PATH_PARAM).equalsIgnoreCase(uriIn.getPath())) {
This is a suggestion that might be too late if this feature is out
already as is.
This check limit remote sources in using container parameter.
I think we have more power on limiting the value of the proxy prefix.
Force chain path to always contain '/chain/' in it (and no chained
should not).

http://codereview.appspot.com/217110/diff/1/2
File
java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/DefaultProxyUriManagerTest.java
(right):

http://codereview.appspot.com/217110/diff/1/2#newcode131
java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/DefaultProxyUriManagerTest.java:131:
checkValidate("/proxy/" + DefaultProxyUriManager.CHAINED_PARAMS_TOKEN +
"/path",
Add a test with CHAINED_PARAMS_TOKEN at the end of the path (code check
for split result == 2)

http://codereview.appspot.com/217110/show

Re: Introduce ProxyUriManager (issue217110)

[John Hjelmstad <jo...@gmail.com>]

Thanks as always. In addition to the comments below, I'm rejiggering the
makeProxyUri context object to be a ProxyUri context object itself, with the
latter taking a Gadget rather than origUri in its constructor. This is in
response to your note that got dropped from shindig.remailer@ on the To:
line. I completely agree this would A) promote much more "true" symmetry in
the API (a pattern which I'll backport where useful to the other
UriManagers), and B) ever so slowly start ridding our code of
Gadget-as-Context-object.

On Wed, Feb 24, 2010 at 4:45 PM, <zh...@gmail.com> wrote:

> Here are few comments.
> I still think that both the manager and the versioner don't need to work
> on a list but rather on one request at a time.
>
> I think you will need a separate service to group multiple versions
> requests for a batch requests.
> You can have walker that first scn all resources and send one batch
> request to get all resources metadata.
>

Agreed that's possible and in most situations would remove the need to make
multiple (even when batched) requests. That said, computing/retrieving
metadata/version info for all resources for every request may be expensive,
and worse yet, unused in many situations. Also, even when all such
info *is* used,
encapsulation suffers: a rewriter/versioner thus requires another pass to
run before it, and versioning logic won't be context-specific at that point
(though admittedly, I'm not sure if it will have to be). Lastly, I don't
have a use case for this right now but "compound operations" - where the
results of one versioning call could be modified by a pass running before it
- could be affected (this is sort of the same thing as less encapsulation
when it comes down to it).

I'd like to explore, in later CLs, the possibility that we introduce
something like a third pass to DomWalker.Visitor eg. "finish()" which could
enable multiple visitors' modifications to run in parallel, along w/ the
underlying versioning/metadata operations done on them. IMO that's a bit
heavyweight though :)


>
> Also in the validation you need to check the version against the actual
> resource that is going to be served (meaning that when creating the
> proxy url you can use a separate cache of versions for resources, but
> when verifying you should check verify the actual served resource).
>

Agreed, I suspect this will be the concrete implementation provided under
the Versioner.verify(...) method(s) in use.


>
>
>
>
> http://codereview.appspot.com/217110/diff/1/3
> File
>
>
> java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultProxyUriManager.java
> (right):
>
> http://codereview.appspot.com/217110/diff/1/3#newcode50
>
> java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultProxyUriManager.java:50:
> *
>
> http://www.example.com/gadgets/proxy/refresh=1&.../http://www.foo.com/img.gif
> Add start and end beacon in the example.
> btw - is that a new feature? where is it currently implemented?
>

It's new. I'm adding a beacon in order to deterministically differentiate
between chained proxy syntax and query-style syntax. To verify the URL I
need the container, which in turn keys the proxy format, but it's the proxy
format itself that has the container param (the target URL itself may have a
?container param that could confuse things).


>
> http://codereview.appspot.com/217110/diff/1/3#newcode87
>
> java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultProxyUriManager.java:87:
> versions.put(resource, versionList.get(i++));
> use iterator instead of index
>

Done.


>
> http://codereview.appspot.com/217110/diff/1/3#newcode141
>
> java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultProxyUriManager.java:141:
> public ProxyUri validateProxiedUri(Uri uriIn) throws GadgetException {
> It actualy parsing the uri not just validating, so I would rename this
> to parseProxiedUri (which apply through out the UriManager pattern)
>

Sounds good to me. Mind if I do this as a single CL after finishing all of
them? IMO it'd be easier to review and involve fewer conflicts.


>
> http://codereview.appspot.com/217110/diff/1/3#newcode151
>
> java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultProxyUriManager.java:151:
> config.getString(container,
> PROXY_PATH_PARAM).equalsIgnoreCase(uriIn.getPath())) {
> This is a suggestion that might be too late if this feature is out
> already as is.
> This check limit remote sources in using container parameter.
> I think we have more power on limiting the value of the proxy prefix.
> Force chain path to always contain '/chain/' in it (and no chained
> should not).
>

Alas, these semantics are already deployed. "/chain/" would be a nice
direction to migrate though...


>
> http://codereview.appspot.com/217110/diff/1/2
> File
>
>
> java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/DefaultProxyUriManagerTest.java
> (right):
>
> http://codereview.appspot.com/217110/diff/1/2#newcode131
>
> java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/DefaultProxyUriManagerTest.java:131:
> checkValidate("/proxy/" + DefaultProxyUriManager.CHAINED_PARAMS_TOKEN +
> "/path",
> Add a test with CHAINED_PARAMS_TOKEN at the end of the path (code check
> for split result == 2)


Great idea. Added.


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