Code review for SHINDIG-801

See https://issues.apache.org/jira/browse/SHINDIG-801

[louiscryan, 2008-12-19 08:49:25]

I feel lighter just reading it....

http://codereview.appspot.com/11250/diff/1/29
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/AbstractHttpCache.java
(right):

http://codereview.appspot.com/11250/diff/1/29#newcode28
Line 28: public static final char KEY_SEPARATOR = ':';
Use a non-URI safe separator to prevent collisions with opensocial id types
which are all URL safe by definition. viewer id can easily be 0:0. How about | ?

http://codereview.appspot.com/11250/diff/1/29#newcode64
Line 64: private static boolean isCacheable(HttpResponse response) {
should be protected and non-static to facilitate overrides.

http://codereview.appspot.com/11250/diff/1/29#newcode68
Line 68: private static boolean isCacheable(HttpRequest request) {
protected non-static

http://codereview.appspot.com/11250/diff/1/29#newcode97
Line 97: protected static String createKey(HttpRequest request) {
protected non-static.

http://codereview.appspot.com/11250/diff/1/29#newcode120
Line 120: private static String getOwnerId(HttpRequest request) {
these and all below should be protected static for re-use

http://codereview.appspot.com/11250/diff/1/29#newcode182
Line 182: public HttpResponse removeResponse(HttpRequest request) {
move above isCacheable

http://codereview.appspot.com/11250/diff/1/29#newcode200
Line 200: return response.getCacheExpiration() > System.currentTimeMillis();
someday we'll use clocks :(

http://codereview.appspot.com/11250/diff/1/25
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/BasicHttpFetcher.java
(right):

http://codereview.appspot.com/11250/diff/1/25#newcode42
Line 42: * Implementation of a {@code RemoteObjectFetcher} using standard
java.net
Can you clean up this comment. Its complete junk.

http://codereview.appspot.com/11250/diff/1/25#newcode151
Line 151: if (!"GET".equals(request.getMethod())) {
I dont think we really ever want to use the HttpURLConnection cache.

http://codereview.appspot.com/11250/diff/1/25#newcode162
Line 162: if (e instanceof java.net.SocketTimeoutException ||
log?

http://codereview.appspot.com/11250/diff/1/37
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth/OAuthModule.java
(right):

http://codereview.appspot.com/11250/diff/1/37#newcode33
Line 33: import org.apache.commons.io.IOUtils;
I think these go above com.google

http://codereview.appspot.com/11250/diff/1/43
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/preload/HttpPreloader.java
(right):

http://codereview.appspot.com/11250/diff/1/43#newcode52
Line 52: // TODO: This needs to be fixed.
TODO is TODONE

http://codereview.appspot.com/11250/diff/1/44
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/render/HtmlRenderer.java
(right):

http://codereview.appspot.com/11250/diff/1/44#newcode173
Line 173: // POST the preloaded content, with a method override of GET
in this code path the original render request is the one used to cache and not
the proxy one so while I think its good practice to set the override header its
technically not necessary here.

Can we make our custom headers constants on HttpRequest.

http://codereview.appspot.com/11250/diff/1/44#newcode177
Line 177: .setPostBody(UTF8.encode(postContent).array());
set content-type and charset.

http://codereview.appspot.com/11250/diff/1/23
File
java/gadgets/src/test/java/org/apache/shindig/gadgets/JsFeatureLoaderTest.java
(right):

http://codereview.appspot.com/11250/diff/1/23#newcode20
Line 20: import static org.easymock.EasyMock.eq;
import order

http://codereview.appspot.com/11250/diff/1/5
File
java/gadgets/src/test/java/org/apache/shindig/gadgets/http/DefaultHttpCacheTest.java
(right):

http://codereview.appspot.com/11250/diff/1/5#newcode75
Line 75: }
Several of the removed tests exercise code that isnt exercised elsewhere as an
individual unit. i.e expiry testing, error negative caching ... 

Should probably just be checked with mock request/response objects.

[louiscryan, 2008-12-19 08:51:38]

Updated

[etnu00, 2008-12-19 09:57:37]

http://codereview.appspot.com/11250/diff/1/29
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/AbstractHttpCache.java
(right):

http://codereview.appspot.com/11250/diff/1/29#newcode28
Line 28: public static final char KEY_SEPARATOR = ':';
On 2008/12/19 08:49:25, louiscryan wrote:
> Use a non-URI safe separator to prevent collisions with opensocial id types
> which are all URL safe by definition. viewer id can easily be 0:0. How about |
?

This doesn't actually matter. If the viewer id was 0:0, the value would just be 

<url>:SIGNED:<owner id>:0:0:<other stuff>
instead of
<url>:SIGNED:<owner id>:0:<other stuff>

There is no decoding or encoding, and the key match must always be a complete
sequence, so there's no chance of collision.

http://codereview.appspot.com/11250/diff/1/29#newcode64
Line 64: private static boolean isCacheable(HttpResponse response) {
On 2008/12/19 08:49:25, louiscryan wrote:
> should be protected and non-static to facilitate overrides.

Agreed...documented explicit inheritance model as well.

http://codereview.appspot.com/11250/diff/1/29#newcode97
Line 97: protected static String createKey(HttpRequest request) {
On 2008/12/19 08:49:25, louiscryan wrote:
> protected non-static.

Done.

http://codereview.appspot.com/11250/diff/1/29#newcode120
Line 120: private static String getOwnerId(HttpRequest request) {
On 2008/12/19 08:49:25, louiscryan wrote:
> these and all below should be protected static for re-use

Done.

http://codereview.appspot.com/11250/diff/1/29#newcode182
Line 182: public HttpResponse removeResponse(HttpRequest request) {
On 2008/12/19 08:49:25, louiscryan wrote:
> move above isCacheable

Done.

http://codereview.appspot.com/11250/diff/1/29#newcode200
Line 200: return response.getCacheExpiration() > System.currentTimeMillis();
On 2008/12/19 08:49:25, louiscryan wrote:
> someday we'll use clocks :(

Brian Eaton actually did check in a TimeSource class a while back. I'll go ahead
and make this use that now.

http://codereview.appspot.com/11250/diff/1/25
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/BasicHttpFetcher.java
(right):

http://codereview.appspot.com/11250/diff/1/25#newcode42
Line 42: * Implementation of a {@code RemoteObjectFetcher} using standard
java.net
On 2008/12/19 08:49:25, louiscryan wrote:
> Can you clean up this comment. Its complete junk.

Done.

http://codereview.appspot.com/11250/diff/1/25#newcode151
Line 151: if (!"GET".equals(request.getMethod())) {
On 2008/12/19 08:49:25, louiscryan wrote:
> I dont think we really ever want to use the HttpURLConnection cache. 

Probably not, but I don't want this change to get any bigger than it already is.
Fixing BasicHttpFetcher is a large task unto itself.

http://codereview.appspot.com/11250/diff/1/37
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth/OAuthModule.java
(right):

http://codereview.appspot.com/11250/diff/1/37#newcode33
Line 33: import org.apache.commons.io.IOUtils;
On 2008/12/19 08:49:25, louiscryan wrote:
> I think these go above com.google

That's not consistent with the imports elsewhere; only org.apache.shindig gets
any special treatment. After that it's all alpha order. I don't think it makes
sense to treat other apache projects as special here, given that we have no
influence over or direct interaction with those projects.

http://codereview.appspot.com/11250/diff/1/43
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/preload/HttpPreloader.java
(right):

http://codereview.appspot.com/11250/diff/1/43#newcode52
Line 52: // TODO: This needs to be fixed.
On 2008/12/19 08:49:25, louiscryan wrote:
> TODO is TODONE

Done.

http://codereview.appspot.com/11250/diff/1/44
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/render/HtmlRenderer.java
(right):

http://codereview.appspot.com/11250/diff/1/44#newcode173
Line 173: // POST the preloaded content, with a method override of GET
On 2008/12/19 08:49:25, louiscryan wrote:
> in this code path the original render request is the one used to cache and not
> the proxy one so while I think its good practice to set the override header
its
> technically not necessary here.
> 
> Can we make our custom headers constants on HttpRequest.

X-Method-Override isn't custom; it's a standards-track header popularized by
REST clients.

With this fix, though, we don't need it anymore anyway.

http://codereview.appspot.com/11250/diff/1/44#newcode177
Line 177: .setPostBody(UTF8.encode(postContent).array());
On 2008/12/19 08:49:25, louiscryan wrote:
> set content-type and charset.

I could have sworn that Adam did this in his patch. Weird. I'll fix it.

http://codereview.appspot.com/11250/diff/1/23
File
java/gadgets/src/test/java/org/apache/shindig/gadgets/JsFeatureLoaderTest.java
(right):

http://codereview.appspot.com/11250/diff/1/23#newcode20
Line 20: import static org.easymock.EasyMock.eq;
On 2008/12/19 08:49:25, louiscryan wrote:
> import order

This is the same as in most of the other tests. We seem to have a mixture of
'static first' and 'static sorted with others'. This is probably due to the use
of a mixture of eclipse and intellij. I don't care what we use one way or the
other, but it should be made consistent.

http://codereview.appspot.com/11250/diff/1/5
File
java/gadgets/src/test/java/org/apache/shindig/gadgets/http/DefaultHttpCacheTest.java
(right):

http://codereview.appspot.com/11250/diff/1/5#newcode75
Line 75: }
On 2008/12/19 08:49:25, louiscryan wrote:
> Several of the removed tests exercise code that isnt exercised elsewhere as an
> individual unit. i.e expiry testing, error negative caching ... 
> 
> Should probably just be checked with mock request/response objects.

All of those tests were either:

- Actually tests on HttpRequest/ HttpResponse objects and had no business being
exercised in an HttpCache unit test

- Are exercised by AbstractHttpCacheTest

The only code in DefaultHttpCache is the delegation to the CacheProvider cache,
so that's all the tests exercise now.

It might be worth creating an HttpCacheTest that all HttpCache implementations
inherit from, but that would probably just involve moving tests out of
AbstractHttpCacheTest.