Fixes for findbugs issues in shindig/java/gadgets

Incremental fix for https://issues.apache.org/jira/browse/SHINDIG-1374, which addresses the following bugs:

Method org.apache.shindig.gadgets.uri.DefaultConcatUriManager.makeConcatUri(ConcatUriManager$ConcatUri, boolean, String) invokes inefficient new Integer(int) constructor; use Integer.valueOf(int) instead

Method org.apache.shindig.gadgets.uri.DefaultConcatUriManager.process(Uri) invokes inefficient new Integer(int) constructor; use Integer.valueOf(int) instead

org.apache.shindig.gadgets.servlet.HttpGadgetContext.getDebug(HttpServletRequest) has Boolean return type and returns explicit null

org.apache.shindig.gadgets.servlet.HttpGadgetContext.getIgnoreCache(HttpServletRequest) has Boolean return type and returns explicit null

org.apache.shindig.gadgets.servlet.ConcatProxyServlet.doFetchConcatResources(HttpServletResponse, ConcatUriManager$ConcatUri) may fail to close stream

Suspicious comparison of Boolean references in org.apache.shindig.gadgets.features.FeatureRegistry$FeatureCacheKey.equals(Object)

Call to equals() comparing different types in org.apache.shindig.gadgets.http.HttpResponseBuilder.setEncoding(Charset)	

Should org.apache.shindig.gadgets.servlet.ConcatProxyServlet$RequestContext be a _static_ inner class?	

org.apache.shindig.gadgets.uri.ProxyUriBase defines equals and uses Object.hashCode()	

org.apache.shindig.gadgets.uri.ProxyUriManager$ProxyUri defines equals and uses Object.hashCode()

[johnfargo, 2010-06-24 01:03:46]

http://codereview.appspot.com/1712045/diff/1/2
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponseBuilder.java
(right):

http://codereview.appspot.com/1712045/diff/1/2#newcode117
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponseBuilder.java:117:
values.add(newContentType);
FMI how is this impl any different?

[janluehe, 2010-06-24 01:16:46]

http://codereview.appspot.com/1712045/diff/1/2
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponseBuilder.java
(right):

http://codereview.appspot.com/1712045/diff/1/2#newcode117
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponseBuilder.java:117:
values.add(newContentType);
On 2010/06/24 01:03:46, johnfargo wrote:
> FMI how is this impl any different?

In the original code, "newContentType" is of type StringBuilder, and is being
passed to the equals method of "contentType", which is of type String, prompting
findbugs to log this warning:

  Call to equals() comparing different types in
org.apache.shindig.gadgets.http.HttpResponseBuilder.setEncoding(Charset)

In the new code, "newContentType" is of type String.

[johnfargo, 2010-06-24 01:32:06]

Ah, missed the non-diffed lines. Lgtm. Will commit if nobody else has by the
time I get to a workstation again.

Cheers
John 


On 2010/06/24 01:16:46, janluehe wrote:
> http://codereview.appspot.com/1712045/diff/1/2
> File
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponseBuilder.java
> (right):
> 
> http://codereview.appspot.com/1712045/diff/1/2#newcode117
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponseBuilder.java:117:
> values.add(newContentType);
> On 2010/06/24 01:03:46, johnfargo wrote:
> > FMI how is this impl any different?
> 
> In the original code, "newContentType" is of type StringBuilder, and is being
> passed to the equals method of "contentType", which is of type String,
prompting
> findbugs to log this warning:
> 
>   Call to equals() comparing different types in
> org.apache.shindig.gadgets.http.HttpResponseBuilder.setEncoding(Charset)
> 
> In the new code, "newContentType" is of type String.

[janluehe, 2010-06-24 17:59:19]

Thanks for committing, John!

I'm about to create Patch Set 2, which contains fixes for the following Findbugs
issues (I've also taken the liberty to fix all equals methods that were missing
a check for reference identity):

- Unread field:
org.apache.shindig.gadgets.oauth.testing.FakeOAuthServiceProvider$TokenState.consumer

- Suspicious comparison of a Integer reference to constant in
org.apache.shindig.gadgets.rewrite.old.ProxyingLinkRewriter.rewrite(String, Uri)

- org.apache.shindig.gadgets.uri.ProxyUriManager$ProxyUri defines equals but not
hashCode       

- Exception is caught when Exception is not thrown in new
org.apache.shindig.gadgets.parse.ParseModule$DOMImplementationProvider()

- Invocation of hashCode on array in
org.apache.shindig.gadgets.http.HttpRequest.hashCode()
  -> The code invokes hashCode on an array. Calling hashCode on an array returns
the same value as System.identityHashCode, and igores the contents and length of
the array. If you need a hashCode that depends on the contents of an array a,
use java.util.Arrays.hashCode(a). 

- Invocation of hashCode on array in
org.apache.shindig.gadgets.http.HttpRequest.hashCode()
  -> See above for recommended fix

[johnfargo, 2010-06-24 18:17:11]

Looks like Paul got to it before I did -- happy to look at patch set 2 when it
comes about. 

Cheers,
John

On 2010/06/24 17:59:19, janluehe wrote:
> Thanks for committing, John!
> 
> I'm about to create Patch Set 2, which contains fixes for the following
Findbugs
> issues (I've also taken the liberty to fix all equals methods that were
missing
> a check for reference identity):
> 
> - Unread field:
>
org.apache.shindig.gadgets.oauth.testing.FakeOAuthServiceProvider$TokenState.consumer
> 
> - Suspicious comparison of a Integer reference to constant in
> org.apache.shindig.gadgets.rewrite.old.ProxyingLinkRewriter.rewrite(String,
Uri)
> 
> - org.apache.shindig.gadgets.uri.ProxyUriManager$ProxyUri defines equals but
not
> hashCode       
> 
> - Exception is caught when Exception is not thrown in new
> org.apache.shindig.gadgets.parse.ParseModule$DOMImplementationProvider()
> 
> - Invocation of hashCode on array in
> org.apache.shindig.gadgets.http.HttpRequest.hashCode()
>   -> The code invokes hashCode on an array. Calling hashCode on an array
returns
> the same value as System.identityHashCode, and igores the contents and length
of
> the array. If you need a hashCode that depends on the contents of an array a,
> use java.util.Arrays.hashCode(a). 
> 
> - Invocation of hashCode on array in
> org.apache.shindig.gadgets.http.HttpRequest.hashCode()
>   -> See above for recommended fix

[janluehe, 2010-06-24 18:46:55]

Second set of fixes

[plindner_linkedin.com, 2010-06-24 19:31:03]

lgtm... committing.


On Thu, Jun 24, 2010 at 11:46 AM, <janluehe@gmail.com> wrote:

> Second set of fixes
>
>
> http://codereview.appspot.com/1712045/show
>

[janluehe, 2010-06-25 04:36:43]

The remaining Findbugs issues complain about non-transient non-serializable
instance fields in the various Servlets of org.apache.shindig.gadgets.servlet
(see below for details).

Making the underlying classes serializable is impossible. On the other hand,
simply marking these fields as transient will result in partially initialized
Servlets upon deserialization (we can't perform Guice injection for these fields
during readObject, because we won't have access to the Injector).

A possible solution would be to remove the problematic instance fields from the
Servlet definitions. Instead, we'll inject them into a new
GadgetServletContextListener, which extends GuiceServletContextListener. This
new ServletContextListener then makes the injected fields available to the
Servlets as ServletContext attributes.

I'm about to submit a patch for your review (Patch Set 3). If the patch is
approved, I'll provide an additional patch for the affected unit tests.

Findbugs errors:

Class org.apache.shindig.gadgets.servlet.GadgetRenderingServlet defines
non-transient non-serializable instance field renderer

Class org.apache.shindig.gadgets.servlet.HtmlAccelServlet defines non-transient
non-serializable instance field accelHandler

Class org.apache.shindig.gadgets.servlet.JsServlet defines non-transient
non-serializable instance field registry       

Class org.apache.shindig.gadgets.servlet.MakeRequestServlet defines
non-transient non-serializable instance field makeRequestHandler    

Class org.apache.shindig.gadgets.servlet.ProxyServlet defines non-transient
non-serializable instance field proxyHandler

Class org.apache.shindig.gadgets.servlet.RpcServlet defines non-transient
non-serializable instance field jsonHandler   

Thanks,

Jan

[janluehe, 2010-06-25 04:37:46]

Patch which address non-transient non-serializable Servlet instance fields

[plindner_linkedin.com, 2010-06-26 00:44:29]

hmm.  I'm not a big fan of how this is implemented.

I think it might be better to inject a Provider<foo> and then just call
get() on that.  That would allow us to make those fields final variables.

On Thu, Jun 24, 2010 at 9:37 PM, <janluehe@gmail.com> wrote:

> Patch which address non-transient non-serializable Servlet instance
> fields
>
>
> http://codereview.appspot.com/1712045/show
>

[janluehe, 2010-06-26 00:56:07]

Thanks, Paul. How would this work during deserialization, though?

On 2010/06/26 00:44:29, plindner_linkedin.com wrote:
> hmm.  I'm not a big fan of how this is implemented.
> 
> I think it might be better to inject a Provider<foo> and then just call
> get() on that.  That would allow us to make those fields final variables.
> 
> On Thu, Jun 24, 2010 at 9:37 PM, <mailto:janluehe@gmail.com> wrote:
> 
> > Patch which address non-transient non-serializable Servlet instance
> > fields
> >
> >
> > http://codereview.appspot.com/1712045/show
> >
>

[Paul Lindner, 2010-06-26 01:29:51]

Hrm, that's true...  Also note that the injector itself is stored in
InjectedServlet, that would also need to move to the ServletContext too..

tricky...



On 2010/06/26 00:56:07, janluehe wrote:
> Thanks, Paul. How would this work during deserialization, though?
> 
> On 2010/06/26 00:44:29, http://plindner_linkedin.com wrote:
> > hmm.  I'm not a big fan of how this is implemented.
> > 
> > I think it might be better to inject a Provider<foo> and then just call
> > get() on that.  That would allow us to make those fields final variables.
> > 
> > On Thu, Jun 24, 2010 at 9:37 PM, <mailto:janluehe@gmail.com> wrote:
> > 
> > > Patch which address non-transient non-serializable Servlet instance
> > > fields
> > >
> > >
> > > http://codereview.appspot.com/1712045/show
> > >
> >

[janluehe, 2010-06-26 02:36:01]

I actually don't see any need for Injector to be an instance field of
InjectedServlet: I think it would be sufficient for it to be a local variable
scoped to InjectedServlet#init, because subclasses of InjectedServlet should
have all their injections performed no later than at init time.

Note that Injector is already stored in ServletContext.

On 2010/06/26 01:29:51, Paul Lindner wrote:
> Hrm, that's true...  Also note that the injector itself is stored in
> InjectedServlet, that would also need to move to the ServletContext too..
> 
> tricky...
> 
> 
> 
> On 2010/06/26 00:56:07, janluehe wrote:
> > Thanks, Paul. How would this work during deserialization, though?
> > 
> > On 2010/06/26 00:44:29, http://plindner_linkedin.com wrote:
> > > hmm.  I'm not a big fan of how this is implemented.
> > > 
> > > I think it might be better to inject a Provider<foo> and then just call
> > > get() on that.  That would allow us to make those fields final variables.
> > > 
> > > On Thu, Jun 24, 2010 at 9:37 PM, <mailto:janluehe@gmail.com> wrote:
> > > 
> > > > Patch which address non-transient non-serializable Servlet instance
> > > > fields
> > > >
> > > >
> > > > http://codereview.appspot.com/1712045/show
> > > >
> > >

[janluehe, 2010-07-01 17:57:18]

I thought about this some more and reworked the patch:

Any Servlet that is serialized and deserialized cannot be simply put back into
service unless its init method was called. This is because the ServletConfig
instance variable inherited from javax.servlet.GenericServlet is marked as
transient:

    private transient ServletConfig config;

meaning any Servlet that was deserialized won't have any access to its
ServletConfig (and from it, its ServletContext) unless its init method was
called, which stores the passed in ServletConfig in the "config" field.

Therefore, since we must rely on init being called, we may also rely on
InjectedServlet#init performing any dependency injection.

The updated patch declares all the injected instance fields as transient, adds
the required serialVersionUID, and also prevents a Servlet from being mutated
after it has been initialized.


On 2010/06/26 02:36:01, janluehe wrote:
> I actually don't see any need for Injector to be an instance field of
> InjectedServlet: I think it would be sufficient for it to be a local variable
> scoped to InjectedServlet#init, because subclasses of InjectedServlet should
> have all their injections performed no later than at init time.
> 
> Note that Injector is already stored in ServletContext.
> 
> On 2010/06/26 01:29:51, Paul Lindner wrote:
> > Hrm, that's true...  Also note that the injector itself is stored in
> > InjectedServlet, that would also need to move to the ServletContext too..
> > 
> > tricky...
> > 
> > 
> > 
> > On 2010/06/26 00:56:07, janluehe wrote:
> > > Thanks, Paul. How would this work during deserialization, though?
> > > 
> > > On 2010/06/26 00:44:29, http://plindner_linkedin.com wrote:
> > > > hmm.  I'm not a big fan of how this is implemented.
> > > > 
> > > > I think it might be better to inject a Provider<foo> and then just call
> > > > get() on that.  That would allow us to make those fields final
variables.
> > > > 
> > > > On Thu, Jun 24, 2010 at 9:37 PM, <mailto:janluehe@gmail.com> wrote:
> > > > 
> > > > > Patch which address non-transient non-serializable Servlet instance
> > > > > fields
> > > > >
> > > > >
> > > > > http://codereview.appspot.com/1712045/show
> > > > >
> > > >

[janluehe, 2010-07-01 17:58:52]

Simplified patch for serialization issues flagged by Findbugs

[Paul Lindner, 2010-07-02 00:50:50]

lgtm - patch applied.