ZOOKEEPER-2628: Fix findbug warnings. #102

hanm · 2016-11-07T22:11:55Z

This PR fixed 19 find bug warnings and disabled one find bug warning:

Malicious code vulnerability Warnings
org.apache.zookeeper.ZooDefs$Ids.OPEN_ACL_UNSAFE is a mutable collection
Bug type MS_MUTABLE_COLLECTION (click for details)
In class org.apache.zookeeper.ZooDefs$Ids
Field org.apache.zookeeper.ZooDefs$Ids.OPEN_ACL_UNSAFE
At ZooDefs.java:[line 116]

We will use ZOOKEEPER-1362 for fixing this specific warning, which might require change to ZooDefs interface.

hanm · 2016-11-07T22:37:00Z

src/java/main/org/apache/jute/compiler/CGenerator.java

+                    }
+                }
+            }
+            if (c != null) {


It's sad that we have to close c twice (one here, one in the finally block earlier.). Findbugs is not happy if either of them is missing. It could be that the flow sensitive engine of findbug need an improvement.

i think try with resources would clean this up. we are on java7 now right?

Yeah I thought about using try / with - for some reasons I had the impression that we also need to support JDK 1.6 (that's probably only for 3.4.x). I'll update with try / with.

breed

here is my initial quick review

breed · 2016-11-08T00:42:08Z

src/java/main/org/apache/jute/compiler/CGenerator.java

+                    }
+                }
+            }
+            if (c != null) {


i think try with resources would clean this up. we are on java7 now right?

breed · 2016-11-08T00:44:01Z

src/java/main/org/apache/zookeeper/cli/DeleteCommand.java

@@ -59,11 +59,6 @@ private void retainCompatibility(String[] cmdArgs) throws CliParseException {
        // delete path [version]
        if (args.length > 2) {
            // rewrite to option


you can remove the comment too :)

breed · 2016-11-08T00:48:11Z

src/java/main/org/apache/zookeeper/version/util/VerGen.java

@@ -93,6 +93,14 @@ public static void generateFile(File outputDir, Version version, int rev, String
        } catch (IOException e) {
            System.out.println("Unable to generate version.Info file: "
                    + e.getMessage());
+            if (w != null) {


wow this is a weird one... we have it in the finally... try with resources would make this a bit nicer too...

Findbug complains here because we use System.exit in catch block - in that case it is not guaranteed that the finally block will be executed.

hanm · 2016-11-08T05:56:01Z

Updated PR to address review comments from @breed.

eribeiro · 2016-11-08T16:25:49Z

src/java/main/org/apache/zookeeper/version/util/VerGen.java

-        try {
-            w = new FileWriter(file);
+
+        try (FileWriter w = new FileWriter(new File(pkgdir, TYPE_NAME + ".java"));) {


nit: spurious ";"

eribeiro · 2016-11-08T16:29:44Z

src/java/main/org/apache/zookeeper/server/quorum/flexible/QuorumHierarchical.java

-            if(expansion.get(gid) > (groupWeight.get(gid) / 2) )
+        for (Entry<Long, Long> entry : expansion.entrySet()) {
+            Long gid = entry.getKey();
+            LOG.debug("Group info: " + entry.getValue() + ", " + gid + ", " + groupWeight.get(gid));


We could use modern LOG printing here:

LOG.debug("Group info: {}, {}, {}", entry.getValue(), gid, groupWeight.get(gid));

eribeiro · 2016-11-08T16:30:18Z

src/java/main/org/apache/zookeeper/server/quorum/flexible/QuorumHierarchical.java

+        for (Entry<Long, Long> entry : expansion.entrySet()) {
+            Long gid = entry.getKey();
+            LOG.debug("Group info: " + entry.getValue() + ", " + gid + ", " + groupWeight.get(gid));
+            if(entry.getValue() > (groupWeight.get(gid) / 2) )


nit: add space after the if and remove space before the last parenthesis.

eribeiro · 2016-11-08T16:33:38Z

src/java/main/org/apache/zookeeper/server/DataTree.java

            pwriter.println(":");
-            HashSet<String> tmp = ephemerals.get(k);
+            HashSet<String> tmp = entry.getValue();


Could we define tmp as Set instead of HashSet?

Is there any benefit of doing so? It's sad we don't have auto keyword in Java.

Usually, prefer to use the interface if we only use the methods defined in the interface, which makes it flexible (or less change needed) in case we want to use another Set implementation.

There is a best practice defined in Effective Java 2nd Edition, Item 52: Refer to objects by their interfaces:
If appropriate interface types exist, then parameters, return values, and fields should all be declared using interface types. If you get into the habit of using interface types, your program will be much more flexible. It is entirely appropriate to refer to an object by a class if no appropriate interface exists.

@lvfangmin Thanks for the reference. It makes sense to me. There are a couple of other places in code base that could be refactored, so rather than mixing this change with this PR I created ZOOKEEPER-2630 for the refactoring task.

Ya @lvfangmin, one of my favourite books on Java, and was the reference to my comment above. Thanks pointing out. 😃

Agree with @hanm, better to create a separate issue for dealing with this legacy technical debt. 👍

eribeiro · 2016-11-08T17:19:34Z

src/java/main/org/apache/jute/compiler/JRecord.java

+        } catch (IOException e) {
+            throw e;
+        } finally {
+            if (h != null) {


Just speculating here: couldn't we write this snippet as:

private IOException maybeClose(FileWriter file) { IOException t = null; if (file != null) { try { file.close(); } catch (IOException ex) { t = ex; } } return t; } (...) } finally { IOException e1 = maybeClose(h); IOException e2 = maybeClose(c); if (e1 != null) { throw e1; } if (e2 != null) { throw e2; } }

updated with a more concise version that findbug is happy with.

hanm · 2016-11-08T20:22:48Z

Updated PR to address review comments from @eribeiro.

lvfangmin · 2016-11-09T19:51:17Z

src/java/main/org/apache/zookeeper/server/DataTree.java

            pwriter.println(":");
-            HashSet<String> tmp = ephemerals.get(k);
+            HashSet<String> tmp = entry.getValue();


Usually, prefer to use the interface if we only use the methods defined in the interface, which makes it flexible (or less change needed) in case we want to use another Set implementation.

There is a best practice defined in Effective Java 2nd Edition, Item 52: Refer to objects by their interfaces:
If appropriate interface types exist, then parameters, return values, and fields should all be declared using interface types. If you get into the habit of using interface types, your program will be much more flexible. It is entirely appropriate to refer to an object by a class if no appropriate interface exists.

lvfangmin · 2016-11-09T19:53:29Z

src/java/main/org/apache/zookeeper/server/command/FourLetterCommands.java

@@ -151,9 +153,13 @@
     */
    public final static int telnetCloseCmd = 0xfff4fffd;

-    public final static HashMap<Integer, String> cmd2String =
+    final static HashMap<Integer, String> cmd2String =


Same here, we can also define this as Map to keep consistent with the others.

We just need to make sure this doesn't break any client exposed public field. Looks like it is not this case (that is, we can make it a Map), but unfortunately I guess not all concrete declarations in ZK codebase can be changed to interfaces without breaking backward compatibility. It is not that we are not going to change it, but probably only at major release version.

If we can remove this public, then I think we should. Also agree with the consistent Map declaration comment.

hanm · 2016-11-15T23:14:47Z

Update: disable "Malicious code vulnerability Warnings" appertains to org.apache.zookeeper.ZooDefs$Ids.OPEN_ACL_UNSAFE, and we will use ZOOKEEPER-1362 for this work.

Can we get this merged? Would be good to have findbug clean again for pre-commit builds.

fpj · 2016-11-23T07:16:04Z

src/java/main/org/apache/jute/compiler/JRecord.java

            for (Iterator<JField> i = mFields.iterator(); i.hasNext(); fIdx++) {
                JField jf = i.next();
-                jj.write(jf.genJavaCompareTo());
-                jj.write("    if (ret != 0) return ret;\n");
+                jj.write(jf.genJavaEquals());


What's the reason for changing to genJavaEquals?

Similarly, there is no actual change here.

fpj · 2016-11-23T07:19:45Z

src/java/main/org/apache/jute/compiler/JRecord.java

+                jj.write("    int ret = 0;\n");
+                for (Iterator<JField> i = mFields.iterator(); i.hasNext(); fIdx++) {
+                    JField jf = i.next();
+                    jj.write(jf.genJavaCompareTo());


Why are we changing to genJavaEquals below and not here?

This diff presented by git is not very accurate to reflect the state of the changes. There is no literal changes to these code blocks except indentations. We can compare the vanilla source file in master branch and in my working branch where the PR is created to double check:
JRecord.java in master: https://github.com/apache/zookeeper/blob/master/src/java/main/org/apache/jute/compiler/JRecord.java#L525
JRecord.java in PR branch:
https://github.com/hanm/zookeeper/blob/ZOOKEEPER-2628/src/java/main/org/apache/jute/compiler/JRecord.java#L530
Starting from these lines, compare both file side by side, they are the same. I am not sure why the diff was formatted this way.

fpj · 2016-11-23T10:00:04Z

src/java/main/org/apache/zookeeper/cli/DeleteCommand.java

        if (args.length > 2) {
-            // rewrite to option
-            String [] newCmd = new String[4];


Why are we removing this rewrite?

These are dead code. newCmd was declared but not used so it's better to remove them.

fpj · 2016-11-23T10:01:31Z

src/java/main/org/apache/zookeeper/server/command/FourLetterCommands.java

@@ -151,9 +153,13 @@
     */
    public final static int telnetCloseCmd = 0xfff4fffd;

-    public final static HashMap<Integer, String> cmd2String =
+    final static HashMap<Integer, String> cmd2String =


If we can remove this public, then I think we should. Also agree with the consistent Map declaration comment.

fpj · 2016-11-23T10:04:42Z

src/java/test/config/findbugsExcludeFile.xml

@@ -144,4 +144,10 @@
    <Bug pattern="DM_DEFAULT_ENCODING" />
  </Match>

+  <!-- Disable 'Malicious code vulnerability warnings' due to mutable collection types in interface.


We probably want to leave a note in ZOOKEEPER-1362 to remind ourselves to do it.

Gonna say that! At least a TODO mark I guess...

Comments added to ZOOKEEPER-1362.

fpj · 2016-11-23T10:12:43Z

src/java/main/org/apache/jute/compiler/JRecord.java

            for (Iterator<JField> i = mFields.iterator(); i.hasNext(); fIdx++) {
                JField jf = i.next();
-                cs.write(jf.genCsharpCompareTo());
-                cs.write("    if (ret != 0) return ret;\n");
+                cs.write(jf.genCsharpEquals());


What's the reason for changing to genCsharpEquals? I suppose is is the same reason for changing to genJavaEquals...

Similarly, there is no actual change here.

fpj · 2016-11-23T10:15:13Z

@hanm I've left a few comments, but in general looks pretty good. I noticed that there are also unaddressed comments from @lvfangmin and @eribeiro . Could you take care of those so that we can check this in, please?

hanm · 2016-11-23T16:58:37Z

@fpj Thank you for taking time to double check the patch.

I noticed that there are also unaddressed comments from @lvfangmin and @eribeiro .

The comments were about using Interface type instead of implementation type, and I addressed the comments by creating ZOOKEEPER-2630 because this issue is not a regression and is not specifically about findbug warnings, and I think it might be better to address them separately.

I've left replies on your other comments in PR, please take a look.

This PR fixed 19 find bug warnings and disabled one find bug warning: Malicious code vulnerability Warnings org.apache.zookeeper.ZooDefs$Ids.OPEN_ACL_UNSAFE is a mutable collection Bug type MS_MUTABLE_COLLECTION (click for details) In class org.apache.zookeeper.ZooDefs$Ids Field org.apache.zookeeper.ZooDefs$Ids.OPEN_ACL_UNSAFE At ZooDefs.java:[line 116] We will use ZOOKEEPER-1362 for fixing this specific warning, which might require change to ZooDefs interface. Author: Michael Han <hanm@cloudera.com> Reviewers: fpj <fpj@apache.org>, breed <breed@apache.org>, Edward Ribeiro <edward.ribeiro@gmail.com>, Allan Lyu <lvfangmin@gmail.com> Closes #102 from hanm/ZOOKEEPER-2628 (cherry picked from commit b9beabf) Signed-off-by: fpj <fpj@apache.org>

This PR fixed 19 find bug warnings and disabled one find bug warning: Malicious code vulnerability Warnings org.apache.zookeeper.ZooDefs$Ids.OPEN_ACL_UNSAFE is a mutable collection Bug type MS_MUTABLE_COLLECTION (click for details) In class org.apache.zookeeper.ZooDefs$Ids Field org.apache.zookeeper.ZooDefs$Ids.OPEN_ACL_UNSAFE At ZooDefs.java:[line 116] We will use ZOOKEEPER-1362 for fixing this specific warning, which might require change to ZooDefs interface. Author: Michael Han <hanm@cloudera.com> Reviewers: fpj <fpj@apache.org>, breed <breed@apache.org>, Edward Ribeiro <edward.ribeiro@gmail.com>, Allan Lyu <lvfangmin@gmail.com> Closes apache#102 from hanm/ZOOKEEPER-2628

Creating ci.yaml file to run CI for ZK ACL dev branch

hanm commented Nov 7, 2016

View reviewed changes

breed reviewed Nov 8, 2016

View reviewed changes

hanm force-pushed the ZOOKEEPER-2628 branch from d6e059e to cd1d69b Compare November 8, 2016 05:53

eribeiro reviewed Nov 8, 2016

View reviewed changes

hanm force-pushed the ZOOKEEPER-2628 branch from cd1d69b to 95e8731 Compare November 8, 2016 20:18

lvfangmin reviewed Nov 9, 2016

View reviewed changes

hanm closed this Nov 15, 2016

hanm force-pushed the ZOOKEEPER-2628 branch from 95e8731 to 440e092 Compare November 15, 2016 22:58

ZOOKEEPER-2628: Fix findbug warnings.

fbb0957

hanm reopened this Nov 15, 2016

fpj requested changes Nov 23, 2016

View reviewed changes

asfgit closed this in b9beabf Nov 24, 2016

mgao0 pushed a commit to mgao0/zookeeper that referenced this pull request Dec 8, 2022

Create ci.yaml (apache#102)

bab3fee

Creating ci.yaml file to run CI for ZK ACL dev branch

ZOOKEEPER-2628: Fix findbug warnings. #102

ZOOKEEPER-2628: Fix findbug warnings. #102

Conversation

hanm commented Nov 7, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

breed left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hanm commented Nov 8, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hanm commented Nov 8, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hanm commented Nov 15, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fpj commented Nov 23, 2016

hanm commented Nov 23, 2016 • edited Loading

hanm commented Nov 7, 2016 •

edited

Loading

hanm commented Nov 23, 2016 •

edited

Loading