Write proper forAllApiVersions used in NetworkOPs.cpp

[Bronek]

High Level Overview of Change

We have forAllApiVersions in tests, but since #4820 we also need something similar in NetworkOPs.cpp. Write it properly so we can use it both in prod code and in tests

Context of Change

What's MultiApiJson and why do we need it (not this PR)

Class MultiApiJson was added in version 2.0, to enable us to publish JSON data from NetworkOPs.cpp (where we handle data subscriptions) to clients supporting different API version - that is, slightly different JSON objects, because of differences between API versions. A lazy (and very wasteful) solution would have been to build new JSON object for each subscription on each update, however that would cause a serious performance degradation. So instead, when sending publications to subscribed clients, I chose to expand on the existing (faster) solution, which is to build JSON once and then publish it to all clients. The trouble is, we need to send subtly different JSON depending on the API version supported by the client - the good news is that there are not that many supported API versions, and hopefully that number will stay low. This is where MultiApiJson comes in (technically, it is a specialization of MultivarJson). It is a very small (size equals to the number of valid API versions, 3 at this moment) array of distinct JSON objects, which are indexed by API version. All these JSON objects are created as a copy of a single JSON received in the class constructor - which should be the object that's closest to what we want to publish. After construction, each individual copy can be altered to accommodate variations between versions (this is best seen in the body of NetworkOPsImp::transJson), or all can be altered at the same time (there's a set method for this, which is not affected by this PR). The ownership and functions allowing for inspection and alteration of the individual JSON objects is the responsibility of MultiApiJson. This PR changes how we can inspect and alter these individual JSON objects.

Back to this PR

The purpose of this change is to enable the user of the MultiApiJson class to "visit" (that is, execute a piece of code, typically a lambda) a JSON object selected by API version. If the user wants to "visit" all API versions, the MultiApiJson in this PR is smart enough to know what specific versions that means. The best example is in the NetworkOPsImp::transJson function, where the following:

    MultiApiJson multiObj{jvObj};
    visit<RPC::apiMinimumSupportedVersion, RPC::apiMaximumValidVersion>(
        multiObj,  //
        [&](Json::Value& jvTx, unsigned int apiVersion) {
            RPC::insertDeliverMax(
                jvTx[jss::transaction], transaction->getTxnType(), apiVersion);

            if (apiVersion > 1)
              // . . .
        });

has changed to:

    MultiApiJson multiObj{jvObj};
    forAllApiVersions(
        multiObj.visit(),  //
        [&](Json::Value& jvTx, auto v) {
            constexpr unsigned version = decltype(v)::value;
            RPC::insertDeliverMax(
                jvTx[jss::transaction], transaction->getTxnType(), version);

            if constexpr (version > 1)
              // . . .
        });

This came about because a very similar code was also needed in #4820 , inside function NetworkOPsImp::pubValidation, and of course, many instances of forAllApiVersions inside tests. Such uses are likely to increase as we add new API versions. Hence it is important to establish good practice which minimises potential for bugs.

The approach adopted in this PR is to:

• factor out API versions from ripple/rpc/impl/RPCHelpers.h to a new file ripple/protocol/ApiVersion.h
• factor out a "policy" template parameter, used by MultivarJson for conversion from version to index in the array of JSON objects
• factor out MultiApiJson from ripple/json/MultivarJson.h to ApiVersion.h; this is where "policy" ApiVersionHelper (used to specialize MultivarJson) is defined
• redefine API version numbers as compile-time constants, using std::integral_constant<unsigned, ...> inside ApiVersion.h
• define new functions forAllApiVersions (removing equivalent from tests) and forApiVersions (similar to the previously defined visit template) inside ApiVersion.h
• replace MultivarJson::select with a more powerful MultivarJson::visit and a helper nested class visitor_t

The type change of API version from unsigned to std::integral_constant<unsigned, ...> has several consequences:

• it is now possible to validate during compilation the correctness of a hardcoded version number. Here's an example from NetworkOPsImp::pubValidation:

        multiObj.visit(
            RPC::apiVersion<1>,  // Invalid API version here will trigger compilation error
            [](Json::Value& jvTx) {
                // . . .
            });

• it is possible to pass a strongly typed API version to the user function, which in turn enables it to perform dispatch via overloading or if constexpr (instead of branching instruction), as seen in NetworkOPsImp::transJson
• few existing uses of the API version require an explicit cast to unsigned (in ServerHandler.cpp and RPCHelpers.h)
• since the API client connection handling does not create different types for requests for different API versions, we still need to support all uses of API version as unsigned (this could in time change, but the cost in terms of code churn is not worth it, at this time).
• MultivarJson::visitor_t has to support both variants of API version selection:
  • with a std::integral_constant<unsigned, ...> (which enforces version correctness via static_assert)
  • with an unsigned (which enforces version correctness via normal assert)

Additionally, the switch from select to visit reverses the calling convention used to publish JSON inside NetworkOPs.cpp, from:

	p->send(
	    multiObj.select(apiVersionSelector(p->getApiVersion())),
	    true);

to:

        multiObj.visit(
            p->getApiVersion(),
            [&](Json::Value const& jv) { p->send(jv, true); });

As for the complexity of change, the implementation of MultivarJson::visitor_t is probably the most challenging, mostly due to the number of defined overloads. The class itself is a stateless niebloid which should make the reasoning about it slightly easier. The number of overloads inside this class also explains the size of unit tests, both MultivarJson_test.cpp and ApiVersion_test.cpp.

Additionally, the newly added visit function inside MultivarJson has two slightly different meanings depending on the parameters:

• the overload taking no parameters just returns the visitor object for the lazy execution (wrapped in a tiny lambda to provide the *this object); this object is next called by forAllApiVersions etc.
• the overload taking one or more parameters just calls the visitor directly, forwarding both *this and all the provided parameters, for the eager execution

I realise that this approach might be foreign to some readers, but I feel it is consistent with the paradigms of functional programming, where function evaluation is lazy when possible and eager when needed.

There is also a small drive-by fix in LedgerToJson.cpp where I removed the superfluous (fill.context->apiVersion > 1) conditional inside the scope of else if (fill.context->apiVersion > 1) section (starting at the line 140).

Type of Change

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• Refactor (non-breaking change that only restructures code)
• Tests (you added tests for code that already exists, or your new feature included in this PR)
• Documentation update
• Chore (no impact to binary, e.g. .gitignore, formatting, dropping support for older tooling)
• Release

Perf impact not expected: This does not change any algorithms. The binary will be little different for NetworkOPs compared to before the change, but not in a way that would change anything perceptible. It gives the compiler slightly more space to apply optimisations when building JSON and when publishing but this is very minor.

[thejohnfreeman]

Before I dive into the code, can you please share in an English comment (a) what problem this is meant to solve and (b) the general design / intuition of this solution?

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 94.81481% with 14 lines in your changes are missing coverage. Please review.

Project coverage is 76.97%. Comparing base (47c8cc2) to head (6c78c73).

Files	Patch %	Lines
src/ripple/app/misc/NetworkOPs.cpp	70.37%	7 Missing and 1 partial ⚠️
src/test/protocol/MultiApiJson_test.cpp	98.12%	0 Missing and 3 partials ⚠️
src/ripple/protocol/MultiApiJson.h	95.45%	0 Missing and 2 partials ⚠️
src/ripple/app/ledger/impl/LedgerToJson.cpp	0.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #4833      +/-   ##
===========================================
+ Coverage    76.96%   76.97%   +0.01%     
===========================================
  Files         1127     1129       +2     
  Lines       131768   131858      +90     
  Branches     39699    39662      -37     
===========================================
+ Hits        101417   101503      +86     
+ Misses       24494    24475      -19     
- Partials      5857     5880      +23     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[intelliot]

@Bronek - any preference on whether this goes in 2.1 or 2.2? Given it is a refactor, I'm not sure if there is some urgency to getting it merged/released.

[Bronek]

@intelliot After 2.1 release is fine.

[Bronek]

from the coverage report:

src/ripple/app/misc/NetworkOPs.cpp

• old tests of that same part, which do not have (and did not have) full coverage

src/ripple/protocol/MultiApiJson.h

• two asserts in new code which are impossible to test

src/ripple/app/ledger/impl/LedgerToJson.cpp

• old tests of that same part, which do not have (and did not have) full coverage

so basically these gaps are either unavoidable, or old.