Introduce ProxyUriManager

Introduces interface ProxytUriManager, to be used in the content proxy. Like other UriManager classes, it provides a well-encapsulated API between URI generation and consumption.

DefaultProxyUriManager is provided as a default implementation of same.
It provides a base implementation of ConcatUriManager that is conceptually
consistent with all the other UriManager implementations provided. In
particular:
* Defines its own ContainerConfig "gadgets.uri.proxy" namespace with "host" and
"path" values used for Host: and path assignment, respectively.
* Provides a symmetric makeUri/validateUri pair
* Provides a Versioner interface, in this case supporting batching, for
versioning URLs or batches of same. No default Versioner implementation is
provided in this CL.

Neither the interface nor implementation are used in this CL, as the intent is
to keep CLs as small as possible. A follow-up CL will change ProxyServlet
to use the new interface, along with attendant rewriter changes.

[zhoresh, 2010-02-25 00:45:07]

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)

[johnfargo, 2010-02-26 22:41:12]

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, <zhoresh@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
>

[johnfargo, 2010-02-27 00:25:51]

(also)Improving symmetry of make/validate APIs. Ridding API of Gadget context
object. Other updates.

[johnfargo, 2010-03-02 01:14:08]

Symmetric APIs. Some unused params, at the benefit of fewer class types.

[zhoresh, 2010-03-02 17:00:56]

LGTM
I see common code in the manager and the concat manager.
I think you can create a base manager for that code.

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

http://codereview.appspot.com/217110/diff/3009/3010#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 the start and end section markers in the example

http://codereview.appspot.com/217110/diff/3009/3010#newcode120
java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultProxyUriManager.java:120:
}
Maybe put common code in basic manager: getBaseUriBuilder()

http://codereview.appspot.com/217110/diff/3009/3010#newcode227
java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultProxyUriManager.java:227:
private String getReqConfig(String container, String key) {
This seems like a repeated code, maybe create a base uri manager.

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

http://codereview.appspot.com/217110/diff/3009/3012#newcode79
java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/DefaultProxyUriManagerTest.java:79:
String path = "/proxy/" + DefaultProxyUriManager.CHAINED_PARAMS_TOKEN + "/path";
Test also the case that CHAINED_PARAM_TOKEN is at the end of the path.

[johnfargo, 2010-03-03 19:08:18]

Committing this CL; cleanup soonish.

On Tue, Mar 2, 2010 at 9:00 AM, <zhoresh@gmail.com> wrote:

> LGTM
> I see common code in the manager and the concat manager.
> I think you can create a base manager for that code.
>
>
>
> http://codereview.appspot.com/217110/diff/3009/3010
>
> File
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultProxyUriManager.java
> (right):
>
> http://codereview.appspot.com/217110/diff/3009/3010#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 the start and end section markers in the example
>

Added.


>
> http://codereview.appspot.com/217110/diff/3009/3010#newcode120
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultProxyUriManager.java:120:
> }
> Maybe put common code in basic manager: getBaseUriBuilder()
>

Excellent idea; will do in cleanup.


>
> http://codereview.appspot.com/217110/diff/3009/3010#newcode227
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultProxyUriManager.java:227:
> private String getReqConfig(String container, String key) {
> This seems like a repeated code, maybe create a base uri manager.
>

Same.


>
> http://codereview.appspot.com/217110/diff/3009/3012
>
> File
>
>
java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/DefaultProxyUriManagerTest.java
> (right):
>
> http://codereview.appspot.com/217110/diff/3009/3012#newcode79
>
>
java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/DefaultProxyUriManagerTest.java:79:
> String path = "/proxy/" + DefaultProxyUriManager.CHAINED_PARAMS_TOKEN +
> "/path";
> Test also the case that CHAINED_PARAM_TOKEN is at the end of the path.


Provided @line86 and following.


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