APIv2(ledger_entry): return "invalidParams" when fields missing

[arihantkothari]

High Level Overview of Change

Fix for #4303; Replacing unknownOption error with invalidParams in ledger_entry in API v2

Type of Change

• [ x] Bug fix (non-breaking change which fixes an issue)
• [ x] Tests (Updated the existing tests)

Before / After

Test Input Payload 1:
{ "method":"ledger_entry", "params":[ { "ledger_index":"validated" } ] }

Before:
{ "result":{ "error":"unknownOption", "ledger_hash":"CA38067ED8FCDCFDE8CAB00A7F83BD90FE0B75D4906D42CCA13EE02B15EC1D10", "ledger_index":29063797, "request":{ "command":"ledger_entry", "ledger_index":"validated" }, "status":"error", "validated":true } }

After: remains same as before.

Test Input Payload 2:
{ "method":"ledger_entry", "params":[ { "api_version": 2, "ledger_index":"validated" } ] }

Before: Not supported

After:
{"result":{"error":"invalidParams","ledger_hash":"2532DD58AF4FDC24EAD3F713280C91781C9A0C9CA1DB51EF9BA8946B1D3C1B4E","ledger_index":29343470,"request":{"api_version":2,"command":"ledger_entry","ledger_index":"validated"},"status":"error","validated":true}}

[shawnxie999]

wouldn't this be a small breaking change?

[arihantkothari]

How is it that so ? I have updated the existing unit test as well. I was told that we do not want unknownOption at all but the API currently returns it. I might be missing something, do you think I should be updating something else also to make it a non-breaking change if it is a breaking change ?

[shawnxie999]

Take it with a grain of salt, but it seems unknownOption have existed for quite some while for this API. I think this change would break the error handling for existing applications if the error message is suddenly changed from unknownOption to invalidParams

[shawnxie999]

But I think it would be okay if invalidParams is the intended behavior. I would check with @ckniffen if it's affecting anything.

[arihantkothari]

Do you think we should be deprecating the unknownOption error code ? Is that possible ? For a gradual transition from unknownOption to invalidParams .

[intelliot]

• Existing API requests should continue to return the same values
• New API requests which specify api_version: 2 should get the fixed error code
• You should update the rippled code so that api_version: 2 is accepted and the API response is changed accordingly

[arihantkothari]

Thanks @intelliot ! Here are the results of the changes made:

Input:
{ "method":"ledger_entry", "params":[ { "api_version": 2, "ledger_index":"validated" } ] }

Output:
{"result":{"error":"invalidParams","ledger_hash":"2532DD58AF4FDC24EAD3F713280C91781C9A0C9CA1DB51EF9BA8946B1D3C1B4E","ledger_index":29343470,"request":{"api_version":2,"command":"ledger_entry","ledger_index":"validated"},"status":"error","validated":true}}

Input:
{ "method":"ledger_entry", "params":[ { "ledger_index":"validated" } ] }

Output:
{"result":{"error":"unknownOption","ledger_hash":"BB17A4E96BB0D8411B36AA003AF6E25DC71B3B5E29E16C2FC2F2F88A8C17251B","ledger_index":29343475,"request":{"command":"ledger_entry","ledger_index":"validated"},"status":"error","validated":true}}

[arihantkothari]

Currently, a bunch of account_info unit tests fail (rippled/src/test/rpc/AccountInfo_test.cpp) because of setting ApiMaximumSupportedVersion to 2. It seems that there exists version specific tests in that and by default all tests are run on the max supported version. I am wondering if I should start a new issue regarding this or do the changes here ? I think starting a new issue might make more sense.

[intelliot]

Yes, a new issue (or better - a PR) makes sense.

[ximinez]

As I wrote over on #4571, once a version of rippled is released with apiMaximumSupportedVersion is set to 2, then we are saying that API v2 is Done. Once that happens we can not legitimately make any further changes to API version 2. To the best of my knowledge, API version 2 is still in beta, under active development, and still changing.

Now, if you (and #4571 and #4568) are saying that version 2 is done, let's talk about version 2 being done.

If you want a node to properly handle API v2 requests, the correct way to do that, for now, is with the [beta_rpc_api] config option. And if you want to do it in unit tests, set it up in the config similar to how it's done in https://github.com/XRPLF/rippled/blob/develop/src/test/rpc/AccountInfo_test.cpp#L209-L212, or just by setting cfg.BETA_RPC_API=true; directly.

[arihantkothari]

Thanks for the feedback. I'll fix this.

[ximinez]

Thanks. Please see also #4573.

[arihantkothari]

Perfect, I'll have a look at it!

[arihantkothari]

Hi @ximinez , this seems a bit problematic. In the test, api_version 2 is specified in the payload which means that when let's say api_version 3 is released (ofc, not soon but whenever it does) - the test will no longer run on api_version 3 unless this particular behaviour changes and we rewrite the test for it. However ideally, would we not want the new test to run for all versions >= 2 unless the particular behaviour is changed for the next versions ofc ? I may be wrong, but I just wanted to clarify.

[arihantkothari]

Also now since we are using beta_version and not changing the max supported version, it does not matter for this PR. But I found a weird behaviour when I changed apiMaximumSupportedVersion version to 2. It immediately failed a bunch of tests because the unit-tests started running on apiMaximumSupportedVersion by default which is inconsistent with this. I found this. It is not important right now probably, but this might be a bug and we might need to fix this when api_version 2 is done?

[thejohnfreeman]

This does not give me a good impression of our versioning scheme. Makes me think this is why version 2 has not released.

Do we have guidelines that answer these questions?

• How should we write code that is conditional on version and robust in the face of future versions?
• What do we call the set of versions that is tested? What do we call the set of versions that is enabled during operation?
• How do we configure which versions are in these sets?
• How can we ensure that all to-be-tested versions are actually tested? How can we ensure that code that is not conditional on version maintains its behavior across versions?

[arihantkothari]

Tagging @intelliot to see if he has any views on this. :)

[intelliot]

It's the same as the way APIs are versioned in other API servers, unrelated to rippled.

You write code that is conditional by using conditions. When future versions are created, you have to update the code. Where updates are required depends on the changes being made in the API. The name for the set of versions that is tested is decided the same way as all other names in the code: propose something, and let other people propose alternatives. The same applies for the set of versions that are enabled during operation: you give it a name that makes sense to you, and is the best name you can think of, given limited time and knowledge. Similarly, you propose code that configures which versions are in these sets. And you have to write code that ensures all available versions are actually tested. The purpose of unit tests is to catch breakages when the API version changes, so it is good (and it is a sign of unit tests serving their intended purpose) when tests break in the face of breaking changes in the API. At that point, you update the tests so that they test the previous API version, and the current one.

If you have any other answers to your questions, go ahead and and share them.

[thejohnfreeman]

@ximinez gave some good answers in our internal Slack:

2. You mean like a name? I have no idea. As I said above, the tests should be over minimum to beta, and the set that's enabled during operation (by default) is minimum to maximum.
3. They are defined by the variables in RPCHelpers.h.
4. My thinking is that once #4573 is merged, any tests that care about versions should loop from apiMinimumSupportedVersion to apiBetaVersion and run the tests across all versions. The tests would then have to be written with conditional code to handle the expected results across all versions. This is different than how a lot of the existing tests are written where there's one function for v1 and one for v2, which I agree with you is not sustainable.

He did not answer the first question in my opinion. I think it should be done with zero version equalities, only version inequalities. In fact, I think we can prescribe a pattern to be strictly-enforced:

if (version < FEATURE_X_VERSION) {
  // old behavior
} else {
  // new behavior
}

if (version >= FEATURE_X_VERSION) {
  // new behavior where old behavior was just "do nothing"
}

if (version < FEATURE_X_VERSION) {
  // old behavior where new behavior is "stop doing that"
}