Opened 5 years ago

Closed 3 years ago

#12944 closed enhancement (wontfix)

onionoo protocol/client api and base implementaion

Reported by: iwakeh Owned by: iwakeh
Priority: Medium Milestone:
Component: Metrics/Onionoo Version:
Severity: Normal Keywords:
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

derived from the discussion in #12869:
this will be the support base for future onionoo java clients

  • separate the protocol from current onionoo implementation, so it can be reused in the client.
  • design onionoo client api and base implementation.

Child Tickets

TicketTypeStatusOwnerSummary
#13334enhancementclosedimprove DetailsDocument

Attachments (3)

0001-client-protocol-api-draft-1.patch (10 bytes) - added by iwakeh 5 years ago.
obsolete; I'll set up a public fork, as I have more changes already.
0001-task-12944-protocol-separation-validator-package-so.patch (93.5 KB) - added by iwakeh 5 years ago.
here's the patch for review
0001-second-api-suggestion.patch (67.2 KB) - added by iwakeh 5 years ago.
second patch based on your suggestions

Download all attachments as: .zip

Change History (12)

comment:1 Changed 5 years ago by iwakeh

Status: newassigned

comment:2 Changed 5 years ago by iwakeh

Here's the patch.
https://trac.torproject.org/projects/tor/attachment/ticket/12944/0001-task-12944-protocol-separation-validator-package-so.patch

Goals and Assumptions

The protocol package should contain all of onionoo's protocol information:

  • resource types,
  • parameters,
  • fields values, and
  • all validation requirements.

Changes to onionoo's protocol will be reflected in the code of this package.
For example, removal of a parameter can be signaled via deprecating the corresponding enum value.

Classes of server have to "react" to protocol package changes,
i.e. provide a new document etc.

  • ResourceServlet validates the incoming requests.
  • Hence, ResourceHandler and ResponseBuilder can rely on valid input for all methods called from ResourceServlet.

Structure and Verification

Clients that use user input (like koninoo) or some internal
calculations for querying Onionoo need to verify the input
or calculated values in order to avoid server errors.

This first part provides this functionality. The new protocol
package contains a ResourceType enum, a Parameter enum, and a
Constants class. The latter is not huge, but I couldn't find a better place.
In order to keep track of the many fields in details docs
I created an enum FieldValue.

The subpackage validation provides the classes that implement
ParameterValidator.

I adapted the server package and the DetailsDocument,
in order to avoid
code duplication and make the protocol package the main place
for protocol changes:

A new resource has to be added to the Resource enum,
a new parameter has to be added to the Parameter enum together
with its ParameterValidator.
If some parameter or resource is going to be removed, it first
could be marked as deprecated in these enums and later removed from them.

(Of course, the RequestHandler still has to be adapted, too, but its
functionality is not part of the protocol.)

There are only client interfaces currently. Smaller steps are better here,
and the implementation will differ quite a bit from the onionoo.docs package.

Please review.

Last edited 5 years ago by iwakeh (previous) (diff)

Changed 5 years ago by iwakeh

obsolete; I'll set up a public fork, as I have more changes already.

Changed 5 years ago by iwakeh

here's the patch for review

comment:3 Changed 5 years ago by iwakeh

Status: assignedneeds_review

comment:4 Changed 5 years ago by karsten

FYI, I started reviewing this patch but am not done yet. I'll get to this ASAP, which may not be today or tomorrow. This is just to let you know that I'm not ignoring this ticket. Thanks!

comment:5 Changed 5 years ago by karsten

Status: needs_reviewneeds_revision

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!

comment:6 Changed 5 years ago by iwakeh

Thanks for carefully reading through all that code!

Here some first answers and some questions (without patch).
The other suggestions/questions I'll reply to later.

  • 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 cannot recall why I introduced the longer version pattern.
Your suggestion ought to be fine.

  • 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?

New ticket #13334

  • This change contracts the statement in package-info.java:

This package does NOT implement the interfaces in {@link org.torproject.onionoo.protocol.docs}!.

Did you mean 'contradicts'?

  • Should we rename CharacterValidator to PrintableAsciiValidator to be extra precise?

Good idea. I'll rename it.

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

comment:7 Changed 5 years ago by karsten

Sorry, yes, 'contradicts' is what I meant.

Changed 5 years ago by iwakeh

second patch based on your suggestions

comment:8 Changed 5 years ago 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.

comment:9 Changed 3 years ago by iwakeh

Resolution: wontfix
Severity: Normal
Status: needs_revisionclosed

This would be a major refactoring task and seems outdated (in parts) by now.

Closing as it doesn't fit into the current plans.

Re-open, if it should serve as a reminder.

Note: See TracTickets for help on using tickets.