API versioning #3155

pwang200 · 2019-11-12T20:35:21Z

The requirements are:
https://github.com/pwang200/RippledRPCDesign/blob/API_versioning/requirement/requirements.md
The design document is:
https://github.com/pwang200/RippledRPCDesign/blob/API_versioning/design/design.md

Note that the documentation is for informational only. We will consolidate the documentation and create another PR after both API Versioning and gRPC are merged.

MarkusTeufelberger · 2019-11-12T22:31:11Z

How about a versioning scheme similar to Kubernetes objects where also a readiness is encoded (Alpha, Beta...)? Just an integer is a bit weak...

intelliot · 2019-11-12T23:00:25Z

In my opinion, any API version that appears in a stable release of rippled is “ready”. I don’t think there’s a need for the additional complexity that comes with alpha/beta distinctions.

codecov-io · 2019-11-13T06:12:57Z

Codecov Report

Merging #3155 into develop will increase coverage by 0.06%.
The diff coverage is 94.11%.

@@            Coverage Diff             @@
##           develop   #3155      +/-   ##
==========================================
+ Coverage    68.94%     69%   +0.06%     
==========================================
  Files          688     688              
  Lines        55743   55799      +56     
==========================================
+ Hits         38432   38506      +74     
+ Misses       17311   17293      -18

Impacted Files	Coverage Δ
src/ripple/rpc/impl/RPCHelpers.h	`100% <ø> (+100%)`	⬆️
src/ripple/rpc/impl/Handler.h	`0% <ø> (ø)`	⬆️
src/ripple/app/main/Application.cpp	`59% <0%> (-0.07%)`	⬇️
src/ripple/rpc/impl/RPCHandler.cpp	`75.29% <100%> (ø)`	⬆️
src/ripple/net/impl/RPCCall.cpp	`94.69% <100%> (+0.22%)`	⬆️
src/ripple/rpc/impl/RPCHelpers.cpp	`87.96% <100%> (+0.42%)`	⬆️
src/ripple/rpc/impl/ServerHandlerImp.cpp	`87.45% <93.33%> (+1.36%)`	⬆️
src/ripple/rpc/impl/Handler.cpp	`96.49% <96.55%> (-1.43%)`	⬇️
src/ripple/core/Stoppable.h	`93.75% <0%> (-6.25%)`	⬇️
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 232975b...f4661e0. Read the comment docs.

nbougalis

Looks good. Left some comments, mostly nits that are at your discretion. I'll give you a chance to consider the feedback and come back to give this a second look and sign-off on it tomorrow.

src/ripple/rpc/impl/RPCHelpers.h

nbougalis · 2019-11-20T22:24:27Z

src/ripple/rpc/impl/RPCHelpers.cpp

+            }
+        }
+        return RPC::APIInvalidVersion;
+    } else


This takes advantage of code that already exists in Json::Value. I like it but I'm not sure if I like it enough, especially when you consider that we will be eventually be migrating to this when it's merged into Boost. Anyways, something for you to consider:

unsigned int getAPIVersionNumber(Json::Value const& jv) { static Json::Value const minVersion (RPC::APIVersionSupportedRangeLow); static Json::Value const maxVersion (RPC::APIVersionSupportedRangeHigh); static Json::Value const invalidVersion (RPC::APIInvalidVersion); auto requestedVersion = jv.get (jss::api_version, Json::Value(RPC::APIVersionIfUnspecified)); if(requestedVersion < minVersion || requestedVersion > maxVersion) v = invalidVersion; return requestedVersion.asUInt(); }

Actually my simplifcation doesn't quite work. It needs a further tweak.

I modified it and added a unit test.

nbougalis · 2019-11-20T22:25:36Z

src/ripple/rpc/Context.h

@@ -55,6 +55,7 @@ struct Context
    LedgerMaster& ledgerMaster;
    Resource::Consumer& consumer;
    Role role;
+    unsigned apiVersion;


This should be unsigned int (not just here, but everywhere).

Thanks. I changed unsigned to unsigned int in all files in this PR.

src/ripple/rpc/impl/Handler.cpp

nbougalis · 2019-11-20T22:41:57Z

src/ripple/rpc/impl/RPCHelpers.h

+
+constexpr unsigned APIInvalidVersion = 0;
+constexpr unsigned APIFirstVersion = 1;
+constexpr unsigned APIVersionSupportedRangeLow = 1;


Consider:

APIVersionSupportedRangeLow -> ApiMinimumSupportedVersion
APIVersionSupportedRangeHigh -> ApiMaximumSupportedVersion
APIVersionIfUnspecified -> ApiDefaultVersion

ApiMinimumSupportedVersion and ApiMaximumSupportedVersion are better names. I will change to them.

I used "ApiDefaultVersion" originally in the code too. But I feel we have two "Default Versions", (1) a network RPC request without specify a version which should default to version 1, (2) a command line request which I think should default to ApiMaximumSupportedVersion.

ApiMinimumSupportedVersion and ApiMaximumSupportedVersion are changed.
APIVersionIfUnspecified is not changed.

src/ripple/rpc/impl/RPCHelpers.h

nbougalis · 2019-11-20T23:46:09Z

src/ripple/rpc/impl/RPCHelpers.h

+constexpr unsigned APIVersionSupportedRangeHigh = 1;
+constexpr unsigned APIVersionIfUnspecified = APIFirstVersion;
+constexpr unsigned APIVersionCommandLine = APIVersionSupportedRangeHigh;
+constexpr unsigned APIVersionRippledStartUp = APIVersionSupportedRangeHigh;


Considering that these two values are always likely to use a fixed version of the API, I wonder if defining values for them makes sense. We should probably just use RPC::APIVersionSupportedRangeLow directly in the places where this is used and remove APIVersionCommandLine and APIVersionRippledStartUp.

I will remove APIVersionCommandLine and APIVersionRippledStartUp.

Just to confirm, we will use APIVersionSupportedRangeLow as the command line request version. I can see the benefit of both "high" and "low.

I can argue for either low or high. I'm fine with using whatever is appropriate and leave that up to your judgement.

I removed APIVersionCommandLine and APIVersionRippledStartUp, and used "high" for command line requests' versions.

pwang200 · 2019-11-21T14:03:55Z

Thanks, I will work on it.

…

On Thu, Nov 21, 2019, 1:24 AM Nik Bougalis ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/ripple/rpc/impl/RPCHelpers.cpp <#3155 (comment)>: > @@ -716,5 +716,23 @@ beast::SemanticVersion const firstVersion("1.0.0"); beast::SemanticVersion const goodVersion("1.0.0"); beast::SemanticVersion const lastVersion("1.0.0"); +unsigned getAPIVersionNumber(const Json::Value & jv) +{ + if (jv.isMember(jss::api_version)) + { + if(jv[jss::api_version].isInt()) + { + unsigned apiVersion = jv[jss::api_version].asInt(); + if(apiVersion >= RPC::APIVersionSupportedRangeLow && + apiVersion <= RPC::APIVersionSupportedRangeHigh) + { + return apiVersion; + } + } + return RPC::APIInvalidVersion; + } else Actually my simplifcation doesn't *quite* work. It needs a further tweak. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#3155>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACWTI7PSGCLGA3VISDNGL3QUYSRHANCNFSM4JMIVZLQ> .

mellery451

👍

mellery451 · 2019-11-21T22:56:30Z

src/ripple/net/impl/RPCCall.cpp

+    if(jvRequest.isObject())
+        insert_api_version(jvRequest);
+    else if(jvRequest.isArray()) {
+        for (Json::UInt j = 0; j < jvRequest.size(); ++j) {


the json arrays support range-based iteration, FWIW. Furthermore, since isMember already checks isObject, you can just just call std::for_each(jvRequest.begin(), jvRequest.end(), insert_api_version);

Thanks, I changed to std::for_each(jvRequest.begin(), jvRequest.end(), insert_api_version);

I modified insert_api_version() to include isObject(). I think that is still needed. Otherwise, Value::operator[] could be called on a non-object.

mellery451 · 2019-11-22T01:01:19Z

src/ripple/rpc/impl/RPCHelpers.h

+ * Note that APIInvalidVersion will be returned if
+ * 1) the version number field has a wrong format
+ * 2) the version number retrieved is out of the supported range
+ * 3) the version number is unspecified and


seems like case 3 should be covered by the static checks that @nbougalis suggested above

You are right. But I think we need both the static check (checking we don't make a mistake in coding) and the check in the function (checking network requests are acceptable).

mellery451 · 2019-11-22T01:07:10Z

src/ripple/rpc/impl/ServerHandlerImp.cpp

+        if (jsonRPC.isMember(jss::params) &&
+            jsonRPC[jss::params].isArray() &&
+            jsonRPC[jss::params].size() > 0 &&
+            jsonRPC[jss::params][Json::UInt(0)].isObject())


jsonRPC[jss::params][0u].... might be little more concise (in general for indexing of json arrays with static values)

Thanks! I changed the code.

mellery451 · 2019-11-22T01:23:21Z

src/test/rpc/RPCCall_test.cpp

@@ -77,6 +79,7 @@ static RPCCallTestData const rpcCallTestArray [] =
    "method" : "account_channels",
    "params" : [
      {
+         "api_version" : 1,


if I understand correctly, this version text/field is always replaced with the max supported (?). If that's the case, I would prefer just a token string here (e.g. %MAX_API_VER%....) and then you search/replace that token in the update method (it gives a better sense of what's actually happening). If I've misunderstood what's going on, please ignore.

Great, thanks! I changed the code.

scottschurr · 2019-12-02T17:10:52Z

@pwang200, FWIW, I'm recusing myself from this code review.

I have misgivings about the on-going indefinite maintenance effort that this change represents versus the benefit to users. Based on the conversations I've seen in other pull requests, I do not expect my concerns to be considered seriously at this point in time. I find myself unable to focus only on whether "an implementation has successfully realized its associated design" when I have reservations about the design itself.

Rather than increase noise with no expected change in outcome I'll simply recuse myself.

intelliot · 2021-08-11T04:30:31Z

Note that the documentation is for informational only. We will consolidate the documentation and create another PR after both API Versioning and gRPC are merged.

@pwang200 Has the consolidated documentation been created yet?

pwang200 added 3 commits November 12, 2019 14:42

unit tests work

d1193b0

unit tested, tested command line, websocket, and json-rpc

da69178

code clean

9da2555

pwang200 changed the title ~~Api versioning~~ API versioning Nov 12, 2019

add RPCHelpers.h to ServerHandlerImp.cpp

a122f4f

RPCHelpers.h to test files

f51bc3a

nbougalis requested review from mellery451 and scottschurr and removed request for mellery451 November 19, 2019 20:20

scottschurr assigned mellery451 and scottschurr Nov 20, 2019

nbougalis reviewed Nov 20, 2019

View reviewed changes

requested changes for PR comments

e3deece

mellery451 approved these changes Nov 22, 2019

View reviewed changes

requested changes for PR comments 2

f4661e0

MarkusTeufelberger mentioned this pull request Nov 25, 2019

gRPC support #3159

Merged

scottschurr removed their request for review December 2, 2019 17:11

scottschurr removed their assignment Dec 2, 2019

intelliot mentioned this pull request Dec 3, 2019

Test and make use of rippled API versioning XRPLF/xrpl.js#1105

Closed

nbougalis mentioned this pull request Dec 31, 2019

Proposed 1.5.0-b2 #3199

Merged

nbougalis closed this in #3199 Jan 2, 2020

intelliot referenced this pull request Jun 8, 2023

Support API versioning

2aa11fa

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

API versioning #3155

API versioning #3155

pwang200 commented Nov 12, 2019 •

edited

Loading

MarkusTeufelberger commented Nov 12, 2019

intelliot commented Nov 12, 2019

codecov-io commented Nov 13, 2019 •

edited

Loading

nbougalis left a comment

nbougalis Nov 20, 2019

nbougalis Nov 21, 2019

pwang200 Nov 22, 2019

nbougalis Nov 20, 2019

pwang200 Nov 22, 2019

nbougalis Nov 20, 2019

pwang200 Nov 21, 2019

pwang200 Nov 22, 2019

nbougalis Nov 20, 2019

pwang200 Nov 21, 2019

nbougalis Nov 21, 2019

pwang200 Nov 22, 2019

pwang200 commented Nov 21, 2019 via email

mellery451 left a comment

mellery451 Nov 21, 2019

pwang200 Nov 22, 2019

mellery451 Nov 22, 2019

pwang200 Nov 22, 2019

mellery451 Nov 22, 2019

pwang200 Nov 22, 2019

mellery451 Nov 22, 2019

pwang200 Nov 22, 2019

scottschurr commented Dec 2, 2019

intelliot commented Aug 11, 2021

API versioning #3155

API versioning #3155

Conversation

pwang200 commented Nov 12, 2019 • edited Loading

MarkusTeufelberger commented Nov 12, 2019

intelliot commented Nov 12, 2019

codecov-io commented Nov 13, 2019 • edited Loading

Codecov Report

nbougalis 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pwang200 commented Nov 21, 2019 via email

mellery451 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

scottschurr commented Dec 2, 2019

intelliot commented Aug 11, 2021

pwang200 commented Nov 12, 2019 •

edited

Loading

codecov-io commented Nov 13, 2019 •

edited

Loading