APIv2(ledger_entry) : check error #4630

PeterChen13579 · 2023-07-19T15:33:44Z

High Level Overview of Change

Fix issue #4550 on ledger entry

Context of Change

"check" field is verified to be a string. (Check the above Github issue link for more info.) Everything is under API Version 2 as it is a breaking change.

Type of Change

[X ] Breaking change (fix or feature that would cause existing functionality to not work as expected)
[ X] Tests (You added tests for code that already exists, or your new feature included in this PR)

Added own unit test for API Version 2;

src/ripple/rpc/handlers/LedgerEntry.cpp

connorjchen · 2023-07-19T21:29:22Z

src/test/rpc/LedgerRPC_test.cpp

+            Json::Value const jrr = env.rpc(
+                "json", "ledger_entry", to_string(jvParams))[jss::result];
+
+            if (apiVersion < 2u)


nit: why is apiVersion checked with < 2u in this file compared > 1u? seems better to keep consistent throughout codebase (if most files have already established some standard)

In the test file, it is specifically checking results for apiVersion < 2u and apiVersion other than that (>=2) , while in non-test files, the new error codes only execute if apiVersion is greater than 1. There is a guideline already established here:

Don't have access to that doc, but thanks for elaboration. If you don't mind sharing it, my email is [email protected]

Is > 1u in non-test file equivalent to >= 2u in test file? These seem to always be equivalent expressions since it is impossible to have decimal (1.5) here, right?

yea > 1u in non-test file is equivalent to >= 2u in test file.

…rams

arihantkothari

LGTM, but comments can be improved.

arihantkothari · 2023-07-24T21:20:38Z

src/ripple/rpc/handlers/LedgerEntry.cpp

@@ -51,7 +51,15 @@ doLedgerEntry(RPC::JsonContext& context)

    if (context.params.isMember(jss::index))
    {
-        if (!uNodeIndex.parseHex(context.params[jss::index].asString()))
+        // Document states "index" field is a string. check for this apiVersion


What document are you referring to ? xrpl.org ? You can make this clear in the comments and remove ambiguity.

godexsoft

LGTM. Feel free to ignore my minor comments or apply the suggestions - up to you 👍

godexsoft · 2023-08-14T18:02:10Z

src/ripple/rpc/handlers/LedgerEntry.cpp

+            // apiVersion 2 onwards. invalidParam error is thrown if the above
+            // condition is false
+            if (!context.params[jss::nft_page].isString() &&
+                context.apiVersion > 1u)


minor: You could switch the checks around to save some unnecessary work in case apiVersion is 1

godexsoft · 2023-08-14T18:04:16Z

src/ripple/rpc/handlers/LedgerEntry.cpp

+        // XRPL Doc states "index" field is a string. check for this apiVersion
+        // 2 onwards. invalidParam error is thrown if the above condition is
+        // false
+        if (!context.params[jss::index].isString() && context.apiVersion > 1u)


minor: same thing with switching params around

intelliot · 2023-08-16T05:54:34Z

@godexsoft @arihantkothari to review new changes

godexsoft

👍

arihantkothari

lgtm 👍

ximinez

You made changes to the cases for jss::index, jss::check, jss::directory, and jss::nft_page, but you only added a test case for jss::check. As noted below, the change for nft_page is unnecessary. That aside, you also need to test index and directory. However, the directory case knows how to handle a json object, so you also need to test with something it can't handle: arrays.

ximinez · 2023-08-16T17:13:18Z

src/ripple/rpc/handlers/LedgerEntry.cpp

+        // 2 onwards. invalidParam error is thrown if the above condition is
+        // false


To be precise, the error is not thrown, it's returned. I think it's worth rewriting this and all the other comments to say, "invalidParam error is returned if the above condition is false.", or even simpler, "invalidParam error is returned if not."

ximinez · 2023-08-16T17:23:14Z

src/test/rpc/LedgerRPC_test.cpp

+            Json::Value checkParams;
+            checkParams[jss::owner] = "rhigTLJJyXXSRUyRCQtqi1NoAZZzZnS4KU";
+            checkParams[jss::ledger_index] = "validated";
+            jvParams[jss::check] = checkParams;


You're reusing the jvParams from the previous test here, so that contains features, etc. To guarantee that you're testing what you expect to be testing, start with a new jvParams.

ximinez · 2023-08-16T18:57:00Z

src/ripple/rpc/handlers/LedgerEntry.cpp

        if (context.params[jss::nft_page].isString())
        {
+            // XRPL Doc states "nft_page" field is a string. check for this
+            // apiVersion 2 onwards. invalidParam error is thrown if the above
+            // condition is false
+            if (context.apiVersion > 1u &&
+                !context.params[jss::nft_page].isString())
+            {
+                uNodeIndex = beast::zero;
+                jvResult[jss::error] = "invalidParams";
+            }


Line 359 is checking whether nft_page is a string, so the check on line 365 that it's a string can never fail. In other words, this block is unnecessary.

ximinez

After posting my previous review, I dug a little further, and ended up going down a rabbit hole.

The original issue that this PR fixes is #4550. The basic problem is that the request is returning an "Internal error" for some bad inputs. The goal is to change that to return "invalidParams".

The question is why is "Internal error" being returned in the first place. Long story short, the Json::Value::asString() function throws a Json::error if the value is actually a Json object or array.

So I went through doLedgerEntry and looked for every unchecked call to asString. There were actually about a dozen more that were not handled in the original PR. I also expanded the unit tests to check all of those cases, including verifying that the checked calls to asString returned something other than "Internal error". That commit is here. ximinez@4c9bb17 (Feel free to cherry-pick that onto your branch.)

But then I looked over the whole change set and was really unhappy with all of these piecemeal checks.

There's a much cleaner way to do it, which eliminates the worry of missing any cases and is future-proof (in case later lookups neglect some of those checks). Just catch the thrown Json::error once. Here's that commit: dbe7c5c (Feel free to cherry-pick that one, too, but you'll have to grab the first one to merge cleanly. Or just merge the branch.)

It's much prettier if you hide whitespace changes.

PeterChen13579 · 2023-08-17T20:36:21Z

@ximinez Thank you for all the insight! 😄. I pretty much cherry-picked the try-catch and unittest. I agree it's a LOT cleaner this way and prevents future errors to occur.

ximinez

Looks good

intelliot · 2023-08-23T17:02:26Z

@godexsoft @arihantkothari - this PR has changed since the time you reviewed it. Could you have a look at the latest?

godexsoft · 2023-08-23T17:52:32Z

The diff generated by github for this PR is horrible, very hard to understand what the actual changes are.

My 2c:

It looks like the change does simplify the code a bit - nice
However, relying on exceptions to handle logic is not a very good practice (this is verifying user input)
Although this PR does not introduce this behaviour - it was already there in one way or another. Sometimes exceptions are a necessary evil.

Since @ximinez is happy with the change I have no objections.

ximinez · 2023-08-23T18:24:58Z

The diff generated by github for this PR is horrible, very hard to understand what the actual changes are.

Click the gear at the top of the diff, and check "Hide whitespace", then click "Apply and reload". Makes a huge difference on diffs like this.

However, relying on exceptions to handle logic is not a very good practice (this is verifying user input)

Sometimes it's a great practice, especially when you're dealing with user input, because you never know what kind of wacky input a user is going to send you, and trying to guess everything a user can do wrong can make the code a lot more complex and hard to read. Compare that to a catch that handles any unexpected behavior...

Besides, like you said, we already have exception handling - that's what returns the "Internal error" in the current version. This change just puts a prettier error on it.

godexsoft · 2023-08-23T22:59:04Z

@ximinez Thanks for the nifty trick to make the diff more readable!

In my opinion, user input is not a good place for using exceptions but as i said, they are sometimes a necessary evil, especially if your codebase is already relying on them.
A better solution in my opinion would be a wrapper that hides all the verification and conversions away and returns either values or errors thru std::expected (or ripple::Expected in case of rippled atm.).
I totally understand that one can't simply change the way things are working so I have no objections in this case, just stating my opinion.

My point is - the way to simplify code is thru abstractions that can be tested separately and hide irrelevant details.
Of course i can't disagree with the fact that catching one specific exception is better than "pokemon exception handling" :)

intelliot · 2023-08-29T16:55:00Z

note: updates rippled (with api_version 2) to behave as Clio already does

) - Verify "check", used to retrieve a Check object, is a string. - Verify "nft_page", used to retrieve an NFT Page, is a string. - Verify "index", used to retrieve any type of ledger object by its unique ID, is a string. - Verify "directory", used to retrieve a DirectoryNode, is a string or an object. This change only impacts api_version 2 since it is a breaking change. https://xrpl.org/ledger_entry.html Fix XRPLF#4550

PeterChen13579 and others added 2 commits July 19, 2023 11:02

Fix "check" field to be string as per doc

ea60e8c

Merge branch 'develop' into ledgerEntryInvParams

190d750

PeterChen13579 changed the title ~~APIv2(update check field)~~ APIv2(ledger_entry) : error message Jul 19, 2023

PeterChen13579 changed the title ~~APIv2(ledger_entry) : error message~~ APIv2(ledger_entry) : check error Jul 19, 2023

connorjchen reviewed Jul 19, 2023

View reviewed changes

src/ripple/rpc/handlers/LedgerEntry.cpp Outdated Show resolved Hide resolved

connorjchen reviewed Jul 19, 2023

View reviewed changes

intelliot added API Change Testable Will Need Documentation labels Jul 20, 2023

PeterChen13579 added 2 commits July 20, 2023 11:27

Merge remote-tracking branch 'upstream/develop' into ledgerEntryInvPa…

467f170

…rams

Added check for "nft_page", "index" "directory"

1669834

intelliot requested review from godexsoft and arihantkothari July 24, 2023 16:17

arihantkothari approved these changes Jul 24, 2023

View reviewed changes

add changes in comments

faafa99

godexsoft approved these changes Aug 14, 2023

View reviewed changes

quick fix

5d05d44

intelliot requested review from godexsoft and arihantkothari August 16, 2023 05:54

intelliot added this to the 1.12 milestone Aug 16, 2023

godexsoft approved these changes Aug 16, 2023

View reviewed changes

arihantkothari approved these changes Aug 16, 2023

View reviewed changes

ximinez requested changes Aug 16, 2023

View reviewed changes

Change to try/catch block

65b1274

PeterChen13579 requested a review from ximinez August 17, 2023 20:36

ximinez approved these changes Aug 22, 2023

View reviewed changes

intelliot requested a review from connorjchen August 23, 2023 17:01

intelliot requested review from godexsoft and arihantkothari August 23, 2023 17:02

arihantkothari approved these changes Aug 23, 2023

View reviewed changes

godexsoft approved these changes Aug 23, 2023

View reviewed changes

intelliot modified the milestones: 1.12, 1.13 Aug 23, 2023

intelliot added the Clio Reviewed label Aug 29, 2023

intelliot added 2 commits September 7, 2023 11:45

Merge branch 'develop' into ledgerEntryInvParams

13c09b3

Merge branch 'develop' into ledgerEntryInvParams

1c8748f

intelliot linked an issue Sep 7, 2023 that may be closed by this pull request

[ledger_entry with invalid combination of parameters return internal error] (Version: [1.11.0-rc2]]) #4550

Closed

intelliot merged commit c6f6375 into XRPLF:develop Sep 8, 2023

intelliot added the API V2 Testing label Oct 20, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

APIv2(ledger_entry) : check error #4630

APIv2(ledger_entry) : check error #4630

PeterChen13579 commented Jul 19, 2023

connorjchen Jul 19, 2023

PeterChen13579 Jul 20, 2023

connorjchen Jul 21, 2023

PeterChen13579 Jul 24, 2023

arihantkothari left a comment

arihantkothari Jul 24, 2023

godexsoft left a comment

godexsoft Aug 14, 2023

godexsoft Aug 14, 2023

intelliot commented Aug 16, 2023

godexsoft left a comment

arihantkothari left a comment

ximinez left a comment

ximinez Aug 16, 2023

ximinez Aug 16, 2023

ximinez Aug 16, 2023

ximinez left a comment

PeterChen13579 commented Aug 17, 2023

ximinez left a comment

intelliot commented Aug 23, 2023

godexsoft commented Aug 23, 2023

ximinez commented Aug 23, 2023

godexsoft commented Aug 23, 2023

intelliot commented Aug 29, 2023

		// 2 onwards. invalidParam error is thrown if the above condition is
		// false

APIv2(ledger_entry) : check error #4630

APIv2(ledger_entry) : check error #4630

Conversation

PeterChen13579 commented Jul 19, 2023

High Level Overview of Change

Context of Change

Type of Change

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

arihantkothari left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

godexsoft left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

intelliot commented Aug 16, 2023

godexsoft left a comment

Choose a reason for hiding this comment

arihantkothari left a comment

Choose a reason for hiding this comment

ximinez 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

ximinez left a comment

Choose a reason for hiding this comment

PeterChen13579 commented Aug 17, 2023

ximinez left a comment

Choose a reason for hiding this comment

intelliot commented Aug 23, 2023

godexsoft commented Aug 23, 2023

ximinez commented Aug 23, 2023

godexsoft commented Aug 23, 2023

intelliot commented Aug 29, 2023