enable testing experimental API versions in unit tests #3781

pwang200 · 2021-02-26T05:49:15Z

Motivation

As discussed in #3770, we need a way to unit test API version 2 RPC calls.

Context of Change

The API version that is available for public consumption is 1. But for unit tests, the API version 2 can be enabled.
To show how it works, a version 2 Version RPC call is also added.

Type of Change

Bug fix (non-breaking change which fixes an issue)

scottschurr

Nice job! I left a number of thoughts for your consideration. But I'm also aware that I might have missed the boat on any of them. Let me know what you think.

Once we're happy with the changes I'll see what happens if I put @Frassel's test on top of your changes and see how it works.

Thanks for digging into this!

src/ripple/rpc/impl/RPCHelpers.cpp

src/ripple/rpc/impl/Handler.cpp

scottschurr · 2021-03-04T22:03:37Z

src/ripple/rpc/impl/Handler.cpp

@@ -166,8 +166,13 @@ class HandlerTable
    template <std::size_t N>
    explicit HandlerTable(const Handler (&entries)[N])


I don't have a good place to leave this comment, but here goes.

Convince me of the value provided by member variable table_ being a...

std::array<std::map<std::string, Handler>, ApiNumberVersionSupported> table_;

As far as I can tell this array contains two identical std::map<std::string, Handler>.

I think I understand how this came about. People thought there would be different handlers for different versions. But I think, in reality, those handlers that distinguish between API versions do so internally. There is currently no change in the handler registration process based on supported API version.

Thoughts? Am I off base on this? Thanks.

Right, the current code allow different handlers for different versions, in addition to one handler dealing with different API versions internally. I think it is more flexible in case people want different handlers. But I don't against simplifying it. Love to hear what @cjcobb23 has to say.

@cjcobb23, what's your feeling on this? My opinion is still that keeping arrays of handlers for each API version is complexity that we don't see any need for yet. But I'm open to being persuaded otherwise.

I prefer that the handlers distinguish between different API versions internally. It's easier to reuse common logic within the handlers this way, especially when the version differences are small. For drastically different handlers, I think we would just do something similar to how some of the reporting mode handlers were implemented: https://github.com/ripple/rippled/blob/9d89d4c1885e13e3c0aa7cdb4a389c6fbfd3790a/src/ripple/rpc/handlers/Tx.cpp#L173

However, if the handlers themselves distinguish between different versions internally, I don't think there is a way for different versions of the handler to have different conditions. The condition check happens before actually calling the handler. I don't know how likely it is that different versions of the handlers have different conditions, but it's probably possible.

@pwang200 @scottschurr I think if we want to support the case where different versions of the handler have different conditions, we need to keep this table. However, maybe we should remove the table for now and add it back if/when we need different conditions for different versions of the handler. Thoughts?

Speaking only for myself, I'd rather remove the table now. We don't know whether we'll need this particular flexibility (or something else) in the future. I'd prefer to postpone the complexity until it's needed.

Thanks for the votes! Let's remove it.

src/ripple/rpc/impl/RPCHelpers.h

src/test/rpc/ApiExperiment.h

cjcobb23 · 2021-03-08T20:00:02Z

@pwang200 @scottschurr I know I'm coming in late on this. I am not sure that the problem the pull request is trying to solve is even a problem, though maybe I don't fully understand the situation.

I don't think I agree that "experimental" API versions should be inaccessible via actual RPC. It looks like this pull request is presenting a way to unit test new versions of RPC commands, but those new versions are not actually callable via websocket or HTTP. I don't think this should be the case. We need to be able to also test the calls via actual requests, and not just unit test. I don't think we can say something is ready to go without actually testing it over websocket and HTTP, as well as unit test. Just unit test is not sufficient.

Furthermore, why are we merging in code that is experimental, for which we are not confident it works? Developers test their changes, write unit tests,we have a release process and we do QA testing. I don't think we should be merging in changes that we don't want people to be able to even hit. What even is the purpose, since no one can even test the new functionality? All we have are the unit tests. What is going to occur during the time between when we merge in an experimental change, and when we decide that it's ready to go? How is QA even going to test the change, if it's not callable via actual websocket or RPC? Also, it seems strange to me that changes to an existing version of a handler don't need to go through this experimental phase, and instead are immediately callable by clients. Why does a new version of a handler need to go through some experimental phase, but (possibly substantial) changes to an existing version do not?

I have a hunch that we are drawing inspiration from the amendment system. However, the amendment system is for changes to transaction processing that would cause a different ledger to be computed. RPCs do not need to be treated the same way. When we make changes to transaction processing and the amendment is enabled, everyone must upgrade and the old way (version) of processing those transactions can no longer be used. For RPCs though, we can call v1 or v2 of a handler. We don't have to call v2; we can always fallback to v1.

I think API versioning itself solves this problem. If we release something, and discover there is an issue with a new version of a handler, clients can call the older version. If we really want to be explicit that something is experimental, we can just have the latest API version be considered the beta version. Then the changes are actually callable, but we have a way of saying "this is a beta version. Use with caution", or something like that. We could also leverage the config file as well, such that users must explicitly enable the latest version of the API. I'm not convinced that we actually need this though. Maybe for some changes we are really unsure about, we can use beta versioning. But for other changes that are minor, including the PR that motivated this change, I think the new version can be callable right away, and we don't need to put any sort of beta label on it.

EDIT: Sorry for the noise if I'm misunderstanding something.

scottschurr · 2021-03-11T20:39:00Z

@cjcobb23, I'm not ignoring your comment. I just felt that a response from @pwang200 would be better since, as I understand it, he implemented the API versioning we have in place today.

pwang200 · 2021-03-12T01:06:50Z

@cjcobb23 @scottschurr sorry for the delay, I thought I could get to it today. I will work on the comments soon.

pwang200 · 2021-03-13T02:54:55Z

@cjcobb23 thanks for the comment! As @scottschurr pointed out (in the other PR): "If we're eventually going to support V2 of the API, how do we test additions to the V2 API before that support is enabled for public consumption?" Is it a legitimate concern? I realize it is, because of the need of accumulation of code. What the existing API versioning can do is to accumulate API issues, and when we have enough we code them together and release a new version. This does not allow accumulation of changes in the code base. Because the API versioning (or at least the current setup) is kind of horizontal. When we say V2 API, it intuitively means for the whole set of API functions. If some API functions need changes in V2, then we change them. The rest of the functions are brought to V2 without changes. So if there is one function that we release as V2 and the rest of the functions (without changing) are also release as V2 as well. If we want to change another function in the next release, it will be V3. Let me know if I misunderstood something.

We have several options:

we don't do anything special to support test. We use our existing API versioning and use release process to test. This is fine except code cannot be merged till we accumulate enough for V2.
we enable testing, unit tests only (i.e. this PR). Code can be merged and unit tested. I expect the developers do websocket and HTTP tests (they need to do it anyway). The downside is people need to modify the code to up the API version number to test. It can be done once manually, definitely not ideal. When we want to release V2, however, the API version number is adjusted and people don't need to modify code to test. So in a sense, websocket and HTTP are not skipped. They are tested by the developers (only once though) and their automated QA are delayed.
we find a way of saying "this is a beta version. Use with caution". This is fine also. I feel it maybe over killing.

Let me know what you think and there could be better options. @cjcobb23 @scottschurr

cjcobb23 · 2021-03-15T14:37:15Z

@pwang200 Got it, thanks for the explanation. I understand the situation better now. The idea is we accumulate many changes over time into a new version of the API, and then release those changes as one batch. This makes sense to me.

I do think it's a bit cumbersome that for developers to test their changes over websocket or HTTP, they need to manually modify the code and rebuild. I feel that having a beta version of the API that is marked as unstable but is still callable over websocket and HTTP (maybe protected by a special config flag as well) would be better. However, that is a somewhat more significant lift than this PR, so I think the changes in this PR are a good solution to the problem for the time being.

scottschurr · 2021-03-15T15:48:51Z

@cjcobb23, I think you have an excellent point that folks need to be able to test the interfaces over RPC, not just with unit tests.

So another approach to consider would be to have a flag that goes in the config file to enable the supported API level. Certain unit tests already use changes in the config file to enable specific changes. We could follow that pattern. That would also allow folks to set the config flag in a local rippled instance to test the interface using RPC.

This would mean replacing the RAII ApiExperiment flag with a config flag.

Thoughts?

cjcobb23 · 2021-03-15T15:56:48Z

@scottschurr I think using a config flag would be ideal, since we can then easily test the interface using RPC. And you are right that we have other unit tests that already manipulate the config settings, so we could just follow that pattern, instead of having to do something new.

pwang200 · 2021-03-16T15:36:51Z

@cjcobb23 @scottschurr I added a config flag.
One thing to discuss is the forwarding to peers. A config flag is a local decision, but I think if the beta API version is enable and if forwarding is needed, it should be forwarded to allow say testing in a test network.

cjcobb23 · 2021-03-16T15:40:04Z

@cjcobb23 @scottschurr I added a config flag.
One thing to discuss is the forwarding to peers. A config flag is a local decision, but I think if the beta API version is enable and if forwarding is needed, it should be forwarded to allow say testing in a test network.

@pwang200 what exactly do you mean by forwarding to peers?

pwang200 · 2021-03-16T16:12:46Z

@cjcobb23 probably only for reporting mode?
https://github.com/ripple/rippled/blob/9d89d4c1885e13e3c0aa7cdb4a389c6fbfd3790a/src/ripple/app/reporting/P2pProxy.h#L33-L43

cjcobb23 · 2021-03-16T16:30:17Z

@pwang200 I see. I think it should be up to the user to make sure both the p2p node and the reporting node have the flag set. I don't think that's incredibly difficult, and if they mess it up, the RPC response clearly indicates the problem.

scottschurr

Looks good! I like the way that enabling the beta API with the config file turned out. I also tried out your changes with the test that @Frassle put together in #3770 It worked great!

I'm holding off approval until @cjcobb23 weighs in on whether we need multiple arrays of handlers.

scottschurr · 2021-03-22T18:24:13Z

cfg/rippled-example.cfg

@@ -1502,6 +1502,15 @@
 #       Enable or disable access to /vl requests. Default is '1' which
 #       enables access.
 #
+# [beta_api]


I'm wondering whether this should be beta_rpc_api? Currently there are RPC, web socket, and gRPC interfaces. Do we believe we can guarantee that all three of those interfaces switch at the same time? I kinda hope that's the case. But I felt like I should at least ask the question.

Apologies if I'm missing context here, but it seems odd for this to be 0 or 1. In the future, there may be multiple API versions, so why not specify the desired (default?) API version here?

The idea is that the latest changes that are not yet part of a specific version are just part of the beta version. Whether these changes are available for consumption via RPC is controlled by this config value.

I agree with what @cjcobb23 said.

Right, changed.

scottschurr · 2021-03-22T22:22:30Z

src/ripple/rpc/impl/Handler.cpp

+        if (version < RPC::apiMinimumSupportedVersion ||
+            (betaEnabled && version > RPC::apiBetaVersion) ||
+            (!betaEnabled && version > RPC::apiMaximumSupportedVersion))


I wonder if this reformulation is a little more robust since it only checks betaEnabled once...

if (version < RPC::apiMinimumSupportedVersion || version > (betaEnabled ? RPC::apiBetaVersion : RPC::apiMaximumSupportedVersion))

Just for your consideration...

Thanks, changed.

scottschurr · 2021-03-22T23:15:28Z

src/test/rpc/Version_test.cpp

+            std::string toLoad(R"rippleConfig(
+[beta_api]
+1
+)rippleConfig");
+            c.loadFromString(toLoad);


I think all of this `loadFromString stuff can be reduced to a single line and still be readable. Like this:

c.loadFromString("\n[beta_api]\n1\n");

Just for your consideration.

Thanks, changed.

scottschurr · 2021-03-22T23:43:02Z

src/ripple/rpc/impl/Handler.cpp

@@ -166,8 +166,13 @@ class HandlerTable
    template <std::size_t N>
    explicit HandlerTable(const Handler (&entries)[N])


@cjcobb23, what's your feeling on this? My opinion is still that keeping arrays of handlers for each API version is complexity that we don't see any need for yet. But I'm open to being persuaded otherwise.

scottschurr

👍 Looks great to me. I think you have a good solution. Thanks for the work and for your flexibility.

cjcobb23

LGTM

pwang200 requested a review from scottschurr February 26, 2021 05:50

pwang200 assigned scottschurr Feb 26, 2021

scottschurr reviewed Mar 5, 2021

View reviewed changes

scottschurr requested a review from cjcobb23 March 9, 2021 02:38

scottschurr assigned cjcobb23 Mar 9, 2021

scottschurr reviewed Mar 22, 2021

View reviewed changes

scottschurr approved these changes Mar 27, 2021

View reviewed changes

cjcobb23 approved these changes Apr 1, 2021

View reviewed changes

Enable testing beta RPC API version with config

2b4aefd

pwang200 force-pushed the ApiVersionParameter branch from 2a41b3b to 2b4aefd Compare April 2, 2021 20:50

pwang200 added API Change Ready to merge *PR author* thinks it's ready to merge. Has passed code review. Perf sign-off may still be required. labels Apr 3, 2021

scottschurr mentioned this pull request Apr 12, 2021

Move signer_list in JSON response for api_version 2 #3770

Closed

1 task

This was referenced Jun 2, 2021

Proposed 1.8.0-b3 #3857

Closed

Proposed 1.8.0-b3 #3858

Merged

manojsdoshi closed this in #3858 Jun 4, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

enable testing experimental API versions in unit tests #3781

enable testing experimental API versions in unit tests #3781

pwang200 commented Feb 26, 2021

scottschurr left a comment

scottschurr Mar 4, 2021

pwang200 Mar 16, 2021

scottschurr Mar 22, 2021

cjcobb23 Mar 23, 2021

cjcobb23 Mar 23, 2021

cjcobb23 Mar 23, 2021

scottschurr Mar 23, 2021

pwang200 Mar 23, 2021

pwang200 Mar 25, 2021

cjcobb23 commented Mar 8, 2021 •

edited

Loading

scottschurr commented Mar 11, 2021

pwang200 commented Mar 12, 2021

pwang200 commented Mar 13, 2021

cjcobb23 commented Mar 15, 2021

scottschurr commented Mar 15, 2021

cjcobb23 commented Mar 15, 2021

pwang200 commented Mar 16, 2021

cjcobb23 commented Mar 16, 2021

pwang200 commented Mar 16, 2021 •

edited

Loading

cjcobb23 commented Mar 16, 2021

scottschurr left a comment

scottschurr Mar 22, 2021

intelliot Mar 23, 2021

cjcobb23 Mar 23, 2021

pwang200 Mar 23, 2021

pwang200 Mar 25, 2021

scottschurr Mar 22, 2021

pwang200 Mar 25, 2021

scottschurr Mar 22, 2021

pwang200 Mar 25, 2021

scottschurr Mar 22, 2021

scottschurr left a comment

cjcobb23 left a comment

		@@ -166,8 +166,13 @@ class HandlerTable
		template <std::size_t N>
		explicit HandlerTable(const Handler (&entries)[N])

enable testing experimental API versions in unit tests #3781

enable testing experimental API versions in unit tests #3781

Conversation

pwang200 commented Feb 26, 2021

Motivation

Context of Change

Type of Change

scottschurr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjcobb23 commented Mar 8, 2021 • edited Loading

scottschurr commented Mar 11, 2021

pwang200 commented Mar 12, 2021

pwang200 commented Mar 13, 2021

cjcobb23 commented Mar 15, 2021

scottschurr commented Mar 15, 2021

cjcobb23 commented Mar 15, 2021

pwang200 commented Mar 16, 2021

cjcobb23 commented Mar 16, 2021

pwang200 commented Mar 16, 2021 • edited Loading

cjcobb23 commented Mar 16, 2021

scottschurr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scottschurr left a comment

Choose a reason for hiding this comment

cjcobb23 left a comment

Choose a reason for hiding this comment

cjcobb23 commented Mar 8, 2021 •

edited

Loading

pwang200 commented Mar 16, 2021 •

edited

Loading