http://codereview.appspot.com/1712043/diff/5001/6003 File java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/AccelHandler.java (right):
http://codereview.appspot.com/1712043/diff/5001/6003#newcode123 java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/AccelHandler.java:123: Uri normalizedUri = Uri.parse(proxiedUri.getQueryParameter( On 2010/06/21 22:55:13, johnfargo wrote:
surround this with try/catch(UriException) for error handling.
Our error handling strategy should be 2 fold in this case: 1) In case there is a problem parsing the uri, or fetching content for a given uri, there is not much we can do about it. I guess throwing 404+ is our only option there. Or we can try and redirect the guy to original page, but lets not think of that case now. 2) In case we were able to fetch the content for the given url, we should definitely return this content in case of any errors. So we make sure our rewriting pipeline doesn't mess up. If it does, we fallback to this content. currently this method already throws GadgetException. Plus, the errors encountered in this function seem to be non recoverable for now. how do you want it refactored ? http://codereview.appspot.com/1712043/diff/5001/6005 File java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/AccelUriManager.java (right): http://codereview.appspot.com/1712043/diff/5001/6005#newcode50 java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/AccelUriManager.java:50: public Uri parseAndNormalize(Gadget gadget, Uri requestUri) throws GadgetException { On 2010/06/21 22:55:13, johnfargo wrote:
While we're at it, we should refactor this class a bit to better
reflect the
needs of Accel: 1. parse method should take an HttpRequest rather than a Gadget.
but i need the gadget param as non null to be able to generate valid ProxyUri's :( This is the only way i could figure out to send a non null value for gadget when generating a proxy uri for say: www.example.org/a.html
then perhaps (these can be in a followup as well) 2. Don't extend DefaultProxyUriManager, just @Inject a ProxyUriManager
to which
you delegate when useful. 3. [optional, but fits the pattern] Make AccelUriManager an interface,
with
DefaultAccelUriManager implementing it. ^^ these two help isolate AccelUriManager from potential future issues
having to
do w/ DefaultProxyUriManager impl changes.
Done. http://codereview.appspot.com/1712043/diff/5001/6005#newcode65 java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/AccelUriManager.java:65: String accelPath = config.getString(container, PROXY_PATH_PARAM); On 2010/06/21 21:50:25, zhoresh wrote:
Shouldn't it be accel_path?
you mean ACCEL_PATH ? http://codereview.appspot.com/1712043/diff/5001/6005#newcode65 java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/AccelUriManager.java:65: String accelPath = config.getString(container, PROXY_PATH_PARAM); On 2010/06/21 22:55:13, johnfargo wrote:
You should also just calculate these once in the ctor.
Done. http://codereview.appspot.com/1712043/diff/5001/6005#newcode67 java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/AccelUriManager.java:67: return accelHost.equals(requestUri.getAuthority()) && On 2010/06/21 21:50:25, zhoresh wrote:
I don't think we can promise that the authority will always match
for now this is a hack because i have only 1 accel host. later when i have multiple, il check for something like this: accelHosts.contains(requestUri.getAuthority()) http://codereview.appspot.com/1712043/diff/5001/6004 File java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultProxyUriManager.java (right): http://codereview.appspot.com/1712043/diff/5001/6004#newcode66 java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultProxyUriManager.java:66: protected final ContainerConfig config; On 2010/06/21 22:55:13, johnfargo wrote:
not needed; just store your own ContainerConfig reference in
AccelUriManager. Done. http://codereview.appspot.com/1712043/show
