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

["Tor Bug Tracker & Wiki" <blackhole@xxxxxxxxxxxxxx>]

#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):

 I didn't do test runs, but finally got some time for reading through the
 changes.

 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}}}.


 === 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.

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

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/12651#comment:5>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online
_______________________________________________
tor-bugs mailing list
tor-bugs@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs