[tor-bugs] #12944 [Onionoo]: onionoo protocol/client api and base implementaion

[Tor Bug Tracker & Wiki]

#12944: onionoo protocol/client api and base implementaion
-----------------------------+----------------------------
     Reporter:  iwakeh       |      Owner:  iwakeh
         Type:  enhancement  |     Status:  needs_revision
     Priority:  normal       |  Milestone:
    Component:  Onionoo      |    Version:
   Resolution:               |   Keywords:
Actual Points:               |  Parent ID:
       Points:               |
-----------------------------+----------------------------

Comment (by iwakeh):

 Second part and a new patch attached:

 >  - Is the request "/SUMMARY" now valid?
 > If so, let's update the test rather than remove it.
 > Do we need to update any user documentation for that?
 > And should this change take place in its own commit?
 Update the test in the same commit seems better, otherwise there'd be a
 'commit' that fails the tests.
 No need to advertise this change, though. It'll be documented in the
 changed test.
 The other changes to this test class only provide more information
 on test failure, but are no structural changes.


 >  - It seems that `FieldValue` is specific to details documents.  If so,
 should it be renamed to something with "detail" in it?

 Good suggestion, I renamed it.

 I introduced this enum in order to enable the DetailsDocuments changes (to
 be added in #13334).
 Without that the GivenValuesValidator and a String array
 with all the field names would suffice. Hence, the other documents do not
 have enums.

 >  - Can we remove underscores from method names and use camel case there?
 > ...
 Of course! I intended to use camel-case all over.
 I missed Weights and Bandwidth, now all the ugly underscores should be
 gone.


 >  - Should `BridgeSummary` and `RelaySummary` extend `Summary`?
 Yes, thanks for catching that.

 >  The same applies to `RelayDetails` and `BridgeDetails`.
 There are no such interfaces, as I didn't notice a difference.
 Is there any difference?


 >  - Should there be a `RelayBase` and `BridgeBase` for relay-specific and
 bridge-specific fields?
 No, b/c Base refers to Onionoo's basic structure, which is reflected in
 Base.

 >  - Do you know an easy way to generate class diagram for this review?
 Hmm, not really.

 >  - Can we undo the change to `HttpServletResponseWrapper`, unless it's
 used by anything in this commit that I overlooked?
 I removed these changes.

 >  - Can we undo the whitespace fixes in `RequestHandler`?
 I undid the whitespace changes.

 >  - Should `RequestHandler` parse the days parameter using one of the
 validators,
 > rather than adding a new `parseDays()` method?
 The 'parseDays() method parses input and returns an array.
 The validators do not operate on their input, they just verify values.


 >  - `Constants.SLASH` and `Constants.ONIONOO_BASE` ...
 These are now part of ResourceServlet and unnamed is removed, as it was
 only used once
 in ResponseBuilder.

 >  - Why don't you import `org.slf4j.Logger` in `ResponseBuilder`?
 >  And what about static importing `CLIENTS` in the same class?
 That was due to an automated IDE decision I didn't notice.
 All changed now.

 >  ...  - This change contra_di_cts the statement in package-info.java:
 > `This package does NOT implement the interfaces in {@link
 org.torproject.onionoo.protocol.docs}!`.
 Well, to me the term "package A implements another package B" means
 that each interface of B is implemented in A.
 This is not the case currently with docs.impl and protocol.docs.


 >  - How about we flatten the package structure a bit?
 >  We could have `reqs` and `reqs/impl` packages for everything to put
 together and validate requests (document type, parameters),
 > and `docs` and `docs/impl` packages for everything to create and parse
 documents.
 > And in the future there could be another package called `client` that
 uses an HTTP
 > client library to make requests to onionoo servers, cache responses,
 etc.
 It's good not have a too complicated package hierarchy.

 Preliminaries:
 The parameters and the validators are more than just request related, b/c
 the validation
 could and should be used also when creating documents.

 Currently, 'docs.impl' is not an implementation of 'protocol.docs' (see
 above).
 Maybe rename the current 'docs' to something like 'creator' and
 'creator.impl' or 'processor' or ...?

 I sort of view the api structure from outside of the onionoo application,
 i.e.
 the design base is only the Onionoo protocol as defined in
 'protocol.html'.
 If there is a change in the Onionoo protocol, we first adapt
 the protocol packages. These will also be the contents of the api-jar.
 After that, the change is 'defined' in java, too, and everything depending
 on
 the protocol-api will signal differences during compile and test.
 So changes are made easier.
 The docs package should not have an implementaion sub-package.
 These could go into 'client' or be done in some other structure
 depending on the implementer even outside 'org.torproject.onionoo'.

 Flattened structure:
 {{{
 onionoo-+
         |
         +- creator +
         |          |
         |          +  impl  (for the current docs and docs.impl packages)
         |
         +- protocol -+
         |            |
         |            +- validation
         |            |
         |            +- docs (docs for the current protocol.docs)
         |
         +- client (the future client as you suggested)
         |
         +- cron  (unchanged)
         |
         +- server   (unchanged)
         |
         +- updater   (unchanged)
         |
         +- util  (unchanged)
         |
         +- writer   (unchanged)
 }}}


 I attached another patch as replacement for the older patch.
 I didn't change the package structure yet.
 Feel free to rename and rearrange.

 PS: Tests should be defined after we agree on the new structure.
 Then the tests in ResourceServletTest can be sorted out, too.

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