[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 karsten):

 Replying to [comment:5 iwakeh]:
 > I didn't do test runs, but finally got some time for reading through the
 changes.

 Thanks for reviewing!

 > Three things I want to mention:
 >
 > === Naming
 > The purpose of method {{{UpdateStatus.fromDocumentString}}}
 > would be easier to infer if it was named {{{setFromDocumentString}}}.
 >
 > Without the prefix 'set' I would expect to see some return value other
 than {{{void}}}.

 Indeed.  I just changed this in a new branch based on master, because it's
 a good idea.  I deployed it on the server and will push to master if it
 doesn't break within the next hour.

 > === Equals and Gson Output
 > The {{{equals}}} method in {{{SummaryDocument}}} relies on the
 "deterministic"
 > output of {{{Gson.toJson}}}.
 > As the Json protocol does not impose a particular order of fields, this
 behavior is not guaranteed.
 > It could change with the current implementation depending on
 circumstances,
 > that might not be obvioulsly related, or it could change with some
 > future update. When the output of {{{Gson.toJson}}} differs for the
 > same input we could see weird consequences in
 {{{DocumentStore.storeSummaryDocument}}}.
 >
 > I would prefer a real equals method that compares all the relevant
 fields.
 > This might be also performing better than the Gson-approach. Any decent
 > IDE should generate these equals methods, so not too much typing ;-)
 > (Well, but of course the short solution looks more elegant.)
 >
 > It seems currently {{{Gson.toJson}}} reliably outputs the same
 > Json String for the same input. So, if there is some reason for having
 > an equals method like this, there should at least be some test cases in
 > order to confirm this behavior.

 Huh, good point.  Fortunately, in a later commit that I didn't push yet, I
 took out the equals() comparison and did something even smarter.  But
 you're indeed right that implementing equals() like I did here was not a
 good idea.

 > === Testing
 > Are there any explicit test cases for the new functionality?
 > I did only find adaptions, but no new tests.

 Tests are the reason why I didn't push the other changes I'm talking about
 above. :)  The other reason is that I wanted to merge #13673 first.  Now
 that this is done, I should rebase what I have to master, look into unit
 tests again, and push the result for another review round.  This might not
 happen this week, but I'm working on it.

 Again, thanks for the review!  Much appreciated.

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