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

Reply via email to