[tor-bugs] #12651 [Onionoo]: Details documents of non-running nodes may contain "running":true

[Tor Bug Tracker & Wiki]

#12651: Details documents of non-running nodes may contain "running":true
-------------------------+--------------------------
     Reporter:  karsten  |      Owner:  karsten
         Type:  defect   |     Status:  needs_review
     Priority:  normal   |  Milestone:
    Component:  Onionoo  |    Version:
   Resolution:           |   Keywords:
Actual Points:           |  Parent ID:
       Points:           |
-------------------------+--------------------------

Comment (by iwakeh):

 You did do quite some testing on the mirror. So, it should be ok to merge
 to master and deploy.

 One issue I see is the "hostname-hack":
 Is there a way to avoid the unnecessary additional lookup (after an update
 to the new format) computationally rather than by changing the code after
 the first run?



 Other than that I only have some minor things and coding style issues
 which could be implemented at some point later.

 a) In NodeStatus:124
 {{{
      if (orAddressAndPort.contains(":") &&
             orAddressAndPort.length() > 0)
 }}}
 wouldn't
 {{{    if (orAddressAndPort.contains(":")) }}}
 suffice?

 b) NodeStatus:432
 {{{
       ...
       written = 0;
       for (String orAddressAndPort : this.orAddressesAndPorts) {
         sb.append((written++ > 0 ? "+" : "") + orAddressAndPort);
       }
       ...
 }}}

 As commons-lang3 is available, why not use StringUtils.join?
 {{{
       sb.append(StringUtils.join(this.exitAddresses, "+"));
 }}}
 This would apply to a few other places, too:
 {{{
 src/main/java/org/torproject/onionoo/docs/NodeStatus.java:433:
 sb.append((written++ > 0 ? "+" : "") + orAddressAndPort);
 src/main/java/org/torproject/onionoo/docs/NodeStatus.java:440:
 sb.append((written++ > 0 ? "+" : "") + exitAddress);
 src/main/java/org/torproject/onionoo/docs/NodeStatus.java:449:
 sb.append((written++ > 0 ? "," : "") + relayFlag);
 src/main/java/org/torproject/onionoo/docs/NodeStatus.java:482:
 sb.append((written++ > 0 ? ";" : "") + familyFingerprint);
 src/main/java/org/torproject/onionoo/docs/ClientsHistory.java:125:
 sb.append((written++ > 0 ? "," : "") + e.getKey() + "="
 src/main/java/org/torproject/onionoo/server/PerformanceMetrics.java:67:
 sb.append((written++ > 0 ? ", " : "") + string + " ("
 src/main/java/org/torproject/onionoo/server/PerformanceMetrics.java:100:
 sb.append((j > 0 ? ", " : "") + "." + permilles[j]
 src/main/java/org/torproject/onionoo/server/PerformanceMetrics.java:111:
 sb.append((j > 0 ? ", " : "") + "." + permilles[j] + "<null");
 src/main/java/org/torproject/onionoo/server/ResponseBuilder.java:154:
 addressesBuilder.append((written++ > 0 ? "," : "") + "\""
 }}}



 === Coding Style
 Well, when reading through code, the coding style also gets some focus.
 Thus, a few suggestions for the contributer guide:

 NodeDetailsStatusUpdater:236
 {{{
       int orPort = entry.getOrPort(), dirPort = entry.getDirPort();
 }}}
 Such declarations are a little less readable imho;
 and, they can trip up IDEs during refactoring.
 I'd prefer
 {{{
       int orPort = entry.getOrPort();
       int dirPort = entry.getDirPort();
 }}}
 instead.
 So, maybe we could add this to the contributing guide?

 In addition, there are many places where 'null' values are validated,
 e.g. {{{ if(someVariable == null) ... }}}.
 It might be better to always use {{{ if(null == someVariable) ...}}}
 in order to avoid accidental assigment by style?


 In quite a few places I encountered if-statements that lead to a silent
 method exit, e.g.
 {{{
    if(someBooleanExpression){
         return;
    }
 }}}
 A debug logging statement with a reason might be of value in these cases;
 for both reading the code and later on when programming maintenance or
 enhancements.

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/12651#comment:10>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online