http: Add animal-sniffer plugin and fix reported errors

http: Add animal-sniffer plugin and fix reported errors
https://code.google.com/p/google-http-java-client/issues/detail?id=232

[yanivi, 2013-06-28 21:48:56]

https://codereview.appspot.com/10679049/diff/1/google-http-client-findbugs/po...
File google-http-client-findbugs/pom.xml (right):

https://codereview.appspot.com/10679049/diff/1/google-http-client-findbugs/po...
google-http-client-findbugs/pom.xml:26: <plugin>
Comment that we are suppressing this because we're OK with a Java 6 requirement
for the FindBugs plugin.

https://codereview.appspot.com/10679049/diff/1/google-http-client/src/main/ja...
File google-http-client/src/main/java/com/google/api/client/util/IOUtils.java
(right):

https://codereview.appspot.com/10679049/diff/1/google-http-client/src/main/ja...
google-http-client/src/main/java/com/google/api/client/util/IOUtils.java:238: *
{@link Beta} Attempts to set the given file's permissions such that it can only
be read,
<br/> after Beta to match style we're using

https://codereview.appspot.com/10679049/diff/1/google-http-client/src/main/ja...
google-http-client/src/main/java/com/google/api/client/util/IOUtils.java:242: *
Note that this method requires at least Java version 1.6
This method doesn't require 1.6 per se.  It's just that if you are running it
under Java 5 then doesn't do anything and just returns false.  I'd rather keep
the @return comment short and instead be detailed in the main section of the
method JavaDoc.

https://codereview.appspot.com/10679049/diff/1/google-http-client/src/main/ja...
google-http-client/src/main/java/com/google/api/client/util/IOUtils.java:252:
public static boolean setPermissionsToOwnerOnly(File file) throws IOException {
do we really need to return a boolean?  maybe make it void?

https://codereview.appspot.com/10679049/diff/1/google-http-client/src/main/ja...
google-http-client/src/main/java/com/google/api/client/util/IOUtils.java:260:
return ((Boolean) setReadable.invoke(file, false, false)
Problem is that the || short-cuts attempts to run the others the moment one
fails.  In particular, if we cannot make it not readable by everyone, we still
want to try to make it readable by owner.  Notice the previous code was doing
that, so we should try to retain that behavior.

https://codereview.appspot.com/10679049/diff/1/google-http-client/src/main/ja...
File
google-http-client/src/main/java/com/google/api/client/util/store/FileDataStoreFactory.java
(right):

https://codereview.appspot.com/10679049/diff/1/google-http-client/src/main/ja...
google-http-client/src/main/java/com/google/api/client/util/store/FileDataStoreFactory.java:64:
LOGGER.warning("Unable to change some permissions for: " + dataDirectory);
why do we need this?  we're already logging it inside the setPermissions method.

[ngmiceli, 2013-07-01 16:10:04]

https://codereview.appspot.com/10679049/diff/1/google-http-client-findbugs/po...
File google-http-client-findbugs/pom.xml (right):

https://codereview.appspot.com/10679049/diff/1/google-http-client-findbugs/po...
google-http-client-findbugs/pom.xml:26: <plugin>
On 2013/06/28 21:48:57, yanivi wrote:
> Comment that we are suppressing this because we're OK with a Java 6
requirement
> for the FindBugs plugin.

Done.

https://codereview.appspot.com/10679049/diff/1/google-http-client/src/main/ja...
File google-http-client/src/main/java/com/google/api/client/util/IOUtils.java
(right):

https://codereview.appspot.com/10679049/diff/1/google-http-client/src/main/ja...
google-http-client/src/main/java/com/google/api/client/util/IOUtils.java:238: *
{@link Beta} Attempts to set the given file's permissions such that it can only
be read,
On 2013/06/28 21:48:57, yanivi wrote:
> <br/> after Beta to match style we're using

Done.

https://codereview.appspot.com/10679049/diff/1/google-http-client/src/main/ja...
google-http-client/src/main/java/com/google/api/client/util/IOUtils.java:242: *
Note that this method requires at least Java version 1.6
On 2013/06/28 21:48:57, yanivi wrote:
> This method doesn't require 1.6 per se.  It's just that if you are running it
> under Java 5 then doesn't do anything and just returns false.  I'd rather keep
> the @return comment short and instead be detailed in the main section of the
> method JavaDoc.

Done.

https://codereview.appspot.com/10679049/diff/1/google-http-client/src/main/ja...
google-http-client/src/main/java/com/google/api/client/util/IOUtils.java:252:
public static boolean setPermissionsToOwnerOnly(File file) throws IOException {
On 2013/06/28 21:48:57, yanivi wrote:
> do we really need to return a boolean?  maybe make it void?

I think this is a more intuitive API. Let the caller decide what it means if the
permissions couldn't be changed.

https://codereview.appspot.com/10679049/diff/1/google-http-client/src/main/ja...
google-http-client/src/main/java/com/google/api/client/util/IOUtils.java:260:
return ((Boolean) setReadable.invoke(file, false, false)
On 2013/06/28 21:48:57, yanivi wrote:
> Problem is that the || short-cuts attempts to run the others the moment one
> fails.  In particular, if we cannot make it not readable by everyone, we still
> want to try to make it readable by owner.  Notice the previous code was doing
> that, so we should try to retain that behavior.

Done.

https://codereview.appspot.com/10679049/diff/1/google-http-client/src/main/ja...
File
google-http-client/src/main/java/com/google/api/client/util/store/FileDataStoreFactory.java
(right):

https://codereview.appspot.com/10679049/diff/1/google-http-client/src/main/ja...
google-http-client/src/main/java/com/google/api/client/util/store/FileDataStoreFactory.java:64:
LOGGER.warning("Unable to change some permissions for: " + dataDirectory);
On 2013/06/28 21:48:57, yanivi wrote:
> why do we need this?  we're already logging it inside the setPermissions
method.

Done.

[yanivi, 2013-07-01 16:41:46]

https://codereview.appspot.com/10679049/diff/13001/google-http-client-findbug...
File google-http-client-findbugs/pom.xml (right):

https://codereview.appspot.com/10679049/diff/13001/google-http-client-findbug...
google-http-client-findbugs/pom.xml:27: not part of the library but a tool used
in development of it.-->
Don't suppress it. Instead specify java16 is the requirement for the FindBugs
plugin.

and remove the incorrect comment about FindBugs only being "used in development
of it".  It is used by any developer that wants to enforce a no-Beta
requirement.  Basically we're saying if you're a developer you need at least
Java 6 for development, and that's not unreasonable (I think).

we should change the JDK requirement for google-http-client-appengine to be
java16 also

https://codereview.appspot.com/10679049/diff/13001/google-http-client/src/mai...
File google-http-client/src/main/java/com/google/api/client/util/IOUtils.java
(right):

https://codereview.appspot.com/10679049/diff/13001/google-http-client/src/mai...
google-http-client/src/main/java/com/google/api/client/util/IOUtils.java:259:
failure = !(Boolean) setReadable.invoke(file, true, true)
add a comment to explain why you broke it up into two parts

https://codereview.appspot.com/10679049/diff/16002/google-http-client/src/mai...
File google-http-client/src/main/java/com/google/api/client/util/IOUtils.java
(right):

https://codereview.appspot.com/10679049/diff/16002/google-http-client/src/mai...
google-http-client/src/main/java/com/google/api/client/util/IOUtils.java:249:
public static boolean setPermissionsToOwnerOnly(File file) throws IOException {
The more I look at this the more I think we shouldn't try to make a
general-purpose utility out of it. I really think for 1.16 it will be much
easier to move this to FileDataStoreFactory.

https://codereview.appspot.com/10679049/diff/16002/google-http-client/src/mai...
google-http-client/src/main/java/com/google/api/client/util/IOUtils.java:260:
failure = !(Boolean) setReadable.invoke(file, true, true)
would be nice to log a slightly different warning if permissions weren't changed
for everybody vs. changed for yourself, like the implementation currently does.

https://codereview.appspot.com/10679049/diff/16002/google-http-client/src/mai...
google-http-client/src/main/java/com/google/api/client/util/IOUtils.java:271: }
catch (NoSuchMethodException exception) {
[optional] would be nice to log a warning when running under Java 5 that
specifies the permissions weren't changed because you're running under Java 5.

[ngmiceli, 2013-07-01 17:00:37]

https://codereview.appspot.com/10679049/diff/16002/google-http-client/src/mai...
File google-http-client/src/main/java/com/google/api/client/util/IOUtils.java
(right):

https://codereview.appspot.com/10679049/diff/16002/google-http-client/src/mai...
google-http-client/src/main/java/com/google/api/client/util/IOUtils.java:249:
public static boolean setPermissionsToOwnerOnly(File file) throws IOException {
On 2013/07/01 16:41:46, yanivi wrote:
> The more I look at this the more I think we shouldn't try to make a
> general-purpose utility out of it. I really think for 1.16 it will be much
> easier to move this to FileDataStoreFactory.

Done.

https://codereview.appspot.com/10679049/diff/16002/google-http-client/src/mai...
google-http-client/src/main/java/com/google/api/client/util/IOUtils.java:260:
failure = !(Boolean) setReadable.invoke(file, true, true)
On 2013/07/01 16:41:46, yanivi wrote:
> would be nice to log a slightly different warning if permissions weren't
changed
> for everybody vs. changed for yourself, like the implementation currently
does.

Done.

https://codereview.appspot.com/10679049/diff/16002/google-http-client/src/mai...
google-http-client/src/main/java/com/google/api/client/util/IOUtils.java:271: }
catch (NoSuchMethodException exception) {
On 2013/07/01 16:41:46, yanivi wrote:
> [optional] would be nice to log a warning when running under Java 5 that
> specifies the permissions weren't changed because you're running under Java 5.

Done.

[yanivi, 2013-07-01 17:15:15]

https://codereview.appspot.com/10679049/diff/13001/google-http-client-findbug...
File google-http-client-findbugs/pom.xml (right):

https://codereview.appspot.com/10679049/diff/13001/google-http-client-findbug...
google-http-client-findbugs/pom.xml:27: not part of the library but a tool used
in development of it.-->
On 2013/07/01 16:41:46, yanivi wrote:
> Don't suppress it. Instead specify java16 is the requirement for the FindBugs
> plugin.
> 
> and remove the incorrect comment about FindBugs only being "used in
development
> of it".  It is used by any developer that wants to enforce a no-Beta
> requirement.  Basically we're saying if you're a developer you need at least
> Java 6 for development, and that's not unreasonable (I think).
> 
> we should change the JDK requirement for google-http-client-appengine to be
> java16 also

ping

https://codereview.appspot.com/10679049/diff/24001/google-http-client/src/mai...
File
google-http-client/src/main/java/com/google/api/client/util/store/FileDataStoreFactory.java
(right):

https://codereview.appspot.com/10679049/diff/24001/google-http-client/src/mai...
google-http-client/src/main/java/com/google/api/client/util/store/FileDataStoreFactory.java:122:
* {@link Beta} <br/>
remove Beta

https://codereview.appspot.com/10679049/diff/24001/google-http-client/src/mai...
google-http-client/src/main/java/com/google/api/client/util/store/FileDataStoreFactory.java:127:
* Note that Java 1.5 does not support manipulating file permissions. Because of
that, this method
remove this paragraph

https://codereview.appspot.com/10679049/diff/24001/google-http-client/src/mai...
google-http-client/src/main/java/com/google/api/client/util/store/FileDataStoreFactory.java:158:
} catch (ClassNotFoundException exception) {
[optional] // ignored

similarly below

[ngmiceli, 2013-07-01 19:00:29]

The animal-sniffer plugin crashes on FindBugs. This appears have been a known
problem once http://jira.codehaus.org/browse/SONARPLUGINS-506a but it supposedly
was fixed in 1.6 (we're using 1.9). Nonetheless, a Google search shows we're not
the only ones to still be experiencing this problem. It seems to have something
to do with the specific version of icu4j: 2.6.1.

https://codereview.appspot.com/10679049/diff/24001/google-http-client/src/mai...
File
google-http-client/src/main/java/com/google/api/client/util/store/FileDataStoreFactory.java
(right):

https://codereview.appspot.com/10679049/diff/24001/google-http-client/src/mai...
google-http-client/src/main/java/com/google/api/client/util/store/FileDataStoreFactory.java:122:
* {@link Beta} <br/>
On 2013/07/01 17:15:15, yanivi wrote:
> remove Beta

Done.

https://codereview.appspot.com/10679049/diff/24001/google-http-client/src/mai...
google-http-client/src/main/java/com/google/api/client/util/store/FileDataStoreFactory.java:127:
* Note that Java 1.5 does not support manipulating file permissions. Because of
that, this method
On 2013/07/01 17:15:15, yanivi wrote:
> remove this paragraph

Done.

https://codereview.appspot.com/10679049/diff/24001/google-http-client/src/mai...
google-http-client/src/main/java/com/google/api/client/util/store/FileDataStoreFactory.java:158:
} catch (ClassNotFoundException exception) {
On 2013/07/01 17:15:15, yanivi wrote:
> [optional] // ignored
> 
> similarly below

Done.

[yanivi, 2013-07-01 22:04:14]

LGTM

but assuming you add this one snippet into the pom.xml...

https://codereview.appspot.com/10679049/diff/16003/pom.xml
File pom.xml (right):

https://codereview.appspot.com/10679049/diff/16003/pom.xml#newcode552
pom.xml:552: </plugin>
please add:

        <executions>
          <execution>
            <goals>
              <goal>check</goal>
            </goals>
          </execution>
        </executions>

to make sure it is actually run during a normal mvn install (technically
process-classes)

[ngmiceli, 2013-07-02 14:13:29]

https://codereview.appspot.com/10679049/diff/16003/pom.xml
File pom.xml (right):

https://codereview.appspot.com/10679049/diff/16003/pom.xml#newcode552
pom.xml:552: </plugin>
On 2013/07/01 22:04:15, yanivi wrote:
> please add:
> 
>         <executions>
>           <execution>
>             <goals>
>               <goal>check</goal>
>             </goals>
>           </execution>
>         </executions>
> 
> to make sure it is actually run during a normal mvn install (technically
> process-classes)

Done.

[yanivi, 2013-07-02 15:11:08]

https://codereview.appspot.com/10679049/diff/56001/google-http-client/src/mai...
File
google-http-client/src/main/java/com/google/api/client/util/store/FileDataStoreFactory.java
(right):

https://codereview.appspot.com/10679049/diff/56001/google-http-client/src/mai...
google-http-client/src/main/java/com/google/api/client/util/store/FileDataStoreFactory.java:132:
Class<?> fileClass = File.class;
      Method setReadable = File.class.getMethod("setReadable", boolean.class,
boolean.class);
      Method setWritable = File.class.getMethod("setWritable", boolean.class,
boolean.class);
      Method setExecutable = File.class.getMethod("setExecutable",
boolean.class, boolean.class);

[ngmiceli, 2013-07-02 15:34:38]

https://codereview.appspot.com/10679049/diff/56001/google-http-client/src/mai...
File
google-http-client/src/main/java/com/google/api/client/util/store/FileDataStoreFactory.java
(right):

https://codereview.appspot.com/10679049/diff/56001/google-http-client/src/mai...
google-http-client/src/main/java/com/google/api/client/util/store/FileDataStoreFactory.java:132:
Class<?> fileClass = File.class;
On 2013/07/02 15:11:08, yanivi wrote:
>       Method setReadable = File.class.getMethod("setReadable", boolean.class,
> boolean.class);
>       Method setWritable = File.class.getMethod("setWritable", boolean.class,
> boolean.class);
>       Method setExecutable = File.class.getMethod("setExecutable",
> boolean.class, boolean.class);

Done.

[yanivi, 2013-07-02 15:35:07]

LGTM