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, <[email protected]> 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 >
