Force json commands to be objects #2319
Conversation
* Null json values can be objects or arrays. * json arrays are now interpreted as batch commands. * json objects are single commands. * null jsons are ambiguous as to whether they are single or batch commands and should be avoided.
Jenkins Build SummaryBuilt from this commit Built at 20180108 - 21:09:27 Test Results
|
There was a problem hiding this comment.
I would be happier if these changes had some unit tests. It could be something along the lines of
Env env;
auto result = env.rpc("wallet_propose");
BEAST_ASSERT(result["result"]["status"] == "success");
Edited: Removed the seed in the example because providing a seed resolves the problem.
|
Good suggestion, thanks Ed. Done. |
|
The travis failure is meaningless. It is an "out of time" error, and I've already restarted it once. |
There was a problem hiding this comment.
Looks good to me. Just for grins I also audited other uses of Json::Value in RPCCall.cpp. It looks like you caught all of them. The only one I had any question about was on line 1302. I think that one's okay, but the usage is not very straightforward. So it may deserve a last look.
👍
| Env env{*this}; | ||
| auto result = env.rpc("validation_create"); | ||
| BEAST_EXPECT(result.isMember(jss::result) && | ||
| result[jss::result][jss::status] == "success"); |
There was a problem hiding this comment.
I'd be in favor of getting more coverage with an additional test with the optional seed parameter (which @ximinez may have originally suggested) even though that case worked before and after this fix.
There was a problem hiding this comment.
I'll add that test.
|
@scottschurr I think I'll leave the one at line 1302 alone for now. This war is limited to just the use of |
HowardHinnant
added
the
Passed
|
In 0.90.0-b3 |
commands and should be avoided.
This fixes a regression introduced with 0.90 by the batch json support.