Hi folks,
A cursory look at the new Findbug issues highlighted mostly trivial, and a
subtle but drastic change (IMHO):
1. Dodgy code warnings
* 3/4 of the cases spotted are due to the File.listFiles() method that
could return null if the abstract pathname does not denote a directory or
if an I/O error occurs. Probably it requires a:
if (file.listFiles() != null etc) {
More code verbosity, but doable, imo.
* the remaining fourth case is at
DeleteCommand.retainCompatibility(String[]) (line 62) where
'
newCmd' is created but never used. Trivial.
-----------------------------------------------------------------------------------------------------------------------------------
2. Performance warnings
All of the cases are to replace this idiom
for (K key : map.keySet()) {
V value = map.getKey(key)
with this one:
for (Entry<K,V> e: map.entrySet()) {
therefore, trivial change.
-----------------------------------------------------------------------------------------------------------------------------------
3.
Malicious code vulnerability Warnings
Findbugs is complaining because public or protected fields are mutable
collections as bellow:
public static final HashMap<Integer,String> map = new HashMap<>();
The solution would be something along the lines of Map<Integer, String> map
= Collections.unmodifiableMap(new HashMap<>());
BUT this would break compatibility with ZooDefs.Ids.OPEN_ACL_UNSAFE and
ZooDefs.Ids.READ_ACL_UNSAFE :((((
The other mutable collections are used internally so I don't see any
further problem using an non modifiable collection, but those ZooDefs.Ids
fields can potentially break ZK clients into the wild. In fact, I commented
about this a couple years ago:
https://issues.apache.org/jira/browse/ZOOKEEPER-1362
-----------------------------------------------------------------------------------------------------------------------------------
4. Experimental Warnings
Okay, I am totally unsure what we could alter here, but as far as I could
understand it is complaining that an I/O error could potentially leak
FileWriter, that is, it wants a:
try {
/.../
}
finally {
if (file != null) {
file.close();
}
}
As far as I understood.
-----------------------------------------------------------------------------------------------------------------------------------
So, all in all, my question is: what to do about
ZooDefs.Ids.OPEN_ACL_UNSAFE and ZooDefs.Ids.OPEN_ACL_UNSAFE and
ZooDefs.Ids.READ_ACL_UNSAFE, aka ZOOKEEPER-1362?
Cheers,
Edward
On Sat, Nov 5, 2016 at 2:01 PM, Jordan Zimmerman <[email protected]>
wrote:
>
> Might as well fix the issues, merge to master and merge into the existing
PRs. Then it will be done. Thoughts?
>
> -Jordan
