[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:               |
-----------------------------+----------------------------
Changes (by karsten):

 * status:  needs_review => needs_revision


Comment:

 Whee, long patch is long.  Here's a first round of comments:
  - With the changed `build.xml` we now distinguish between server version
 and API/client version.  Wouldn't it be easier to just have a single
 version for both, where any API version x.y.* understands the protocol by
 server x.y.*?  Or am I oversimplifying things?
  - I assume the changes to `DetailsDocument` are mostly unrelated to this
 ticket and refer to the code comment `"TODO Maybe there's a more elegant
 way (more maintainable, more efficient, etc.) to implement this?"`?  I
 like the idea, but I have some concerns:
    - Does Gson still (de-)serialize details documents correctly with this
 new code?
    - Is there an easy way to preserve static type safety with the new
 approach?
    - This change contracts the statement in package-info.java: `This
 package does NOT implement the interfaces in {@link
 org.torproject.onionoo.protocol.docs}!`.
    - This change should really go into a separate commit, and ideally we'd
 discuss this in its own ticket.
  - It seems that `FieldValue` is specific to details documents.  If so,
 should it be renamed to something with "detail" in it?  And should there
 be enums for the other documents?
  - Should `BridgeSummary` and `RelaySummary` extend `Summary`?  The same
 applies to `RelayDetails` and `BridgeDetails`.
  - Should there be a `RelayBase` and `BridgeBase` for relay-specific and
 bridge-specific fields?
  - Do you know an easy way to generate class diagram for this review?
  - Can we remove underscores from method names and use camel case there?
 The only reason for using underscores in attribute names is that Gson uses
 them for (de-)serialization.
  - Should we rename CharacterValidator to PrintableAsciiValidator to be
 extra precise?  Just a thought.
  - 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.
  - Can we undo the change to `HttpServletResponseWrapper`, unless it's
 used by anything in this commit that I overlooked?
  - Can we undo the whitespace fixes in `RequestHandler`?
  - Should `RequestHandler` parse the days parameter using one of the
 validators, rather than adding a new `parseDays()` method?
  - `Constants.SLASH` and `Constants.ONIONOO_BASE` are mostly Tomcat-
 specific and should go away as soon as we switch to an embedded servlet
 engine.  We could as well define them in `ResourceServlet` for now.  Not
 sure where to put the "UNNAMED" constant, but I doubt that it justifies
 its own `Constants` class.
  - Why don't you import `org.slf4j.Logger` in `ResponseBuilder`?  And what
 about static importing `CLIENTS` in the same class?
  - 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?

 Thanks for working hard on this ticket and patch, and thanks for your
 patience!

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