rpc: add check command #2123

wythe · 2018-11-29T21:49:57Z

The check command will check the parameters of a command without running it; e.g.:

#cli/lightning-cli check invoice ant hello description
{ "code" : -32602, "message" : "'msatoshi' should be a positive number or 'any', not 'ant'" }

# cli/lightning-cli check invoice any hello description
{
  "command": "invoice", 
  "parameters": "ok"
}

And yes, you can check the check command:

# cli/lightning-cli check check
{ "code" : -32602, "message" : "missing required parameter: 'command_to_check'" }

The first commit might be a bit overkill but it made for easy implementation.

While playing with this I thought it would be nice if the error case would present the usage string for
the command also. But that can be saved for later.

rustyrussell

This is an awesome idea! Some minor comments, and please add a CHANGELOG.md entry under Added:

JSON API: New command check checks the validity of a JSON API call without running it.

rustyrussell · 2018-11-29T23:04:34Z

common/json.c

+{
+	const jsmntok_t *first = params;
+	const jsmntok_t *last = json_next(params);
+	printf("size: %d, count: %ld\n", params->size, last - first);


"count: %td" (ptrdiff_t) BTW.

common/json.c

rustyrussell · 2018-11-29T23:10:28Z

common/json.c

+		*dest++ = *first++;
+
+	return arr;
+}


I think this is almost a 1-liner?

if (!tok) return NULL; return tal_dup_arr(ctx, jsmntok_t, tok, json_next(tok) - tok, 0);

I took this approach at first, but it turns out the tok is not a tal allocated array. It's an offset into a tal allocated array. Its setup in parse_request (jsonrpc.c:506).

I think it should still work... I'll try it!

Ok you're right. I tried a getting the tal_length of tok and it didn't work, so I bailed out. My bad.

common/json.c

rustyrussell · 2018-11-29T23:13:15Z

common/test/run-json_remove.c

+			*first = '"';
+		first++;
+	}
+}


Cute trick :)

rustyrussell · 2018-11-29T23:15:10Z

lightningd/jsonrpc.c

+	}
+
+	if (!param(cmd, buffer, mod_params,
+		   p_req("command_to_check", json_tok_command, &name_tok),


I would remove "_to_check" here, since it makes the JSON API neater (the context is pretty clear).

... or just call it 'check'?

command seems better to me.

I discussed my reason for choosing such an awkward name in the commit message for e45dd7f. Basically, it has to be unique to all other parameter names for all commands. But I'm fine with changing it to whatever you are comfortable with. I can also add a developer mode check to make sure it is not used again. Note @ZmnSCPxj, we already are using command for the help command.

Note that you can't make it unique, since 'check check' is supposed to work :) But clashing with some other command would be painful, so happy to stop bikeshedding now :)

lightningd/param.c

rustyrussell · 2018-11-29T23:31:03Z

lightningd/param.c

@@ -79,7 +79,7 @@ static bool parse_by_position(struct command *cmd,
 	}

 	/* check for unexpected trailing params */
-	if (tok != end) {
+	if (!cmd->allow_unused && tok != end) {
 		command_fail(cmd, JSONRPC2_INVALID_PARAMS,
 			     "too many parameters:"
 			     " got %u, expected %zu",


Can we modify parse_by_name similarly?

Currently:

$ ./cli/lightning-cli check command_to_check=listpeers id=xxx { "code" : -32602, "message" : "unknown parameter: 'id'" }

wythe · 2018-12-04T18:26:35Z

I incorporated your suggested changes and rebased. This feature was oddly way more involved than I originally anticipated! But we did get a cool json_tok_print function out of it.

rustyrussell

Ack 83fe4d4

rustyrussell · 2018-12-05T01:30:16Z

common/json.c

+		*dest++ = *first++;
+
+	return arr;
+}


I think it should still work... I'll try it!

rustyrussell · 2018-12-05T02:06:55Z

OK, so I've got a few of additions if you want them in https://github.com/rustyrussell/lightning/commits/pr-2123 but they can be a separate PR and I don't want to hold this one up any longer!

wythe · 2018-12-05T22:23:11Z

Your additions look great. I will make a new PR for them (unless you want to). I also started on the man page.

Needed for check command. I left the print function in since it was so convenient for debugging purposes. Signed-off-by: Mark Beckwith <wythe@intrig.com>

We can now set a flag to have param() ignore unexpected parameters. Normally unexpected parameters are considered errors. Needed by the check command. Signed-off-by: Mark Beckwith <wythe@intrig.com>

The check command allows us to check the parameters of a command without running it. Example: lightning-cli check invoice 234 foo desc We do this by removing the "command_to_check" parameter and then using the remaining parameters as-is. I chose the parameter name "command_to_check" instead of just "command" because it must be unique to all other parameter names for all other commands. Why? Because it may be ambiguous in the case of a json object, where the parameters are not necessary ordered. We don't know which one is the command to check and which one is a parameter. Signed-off-by: Mark Beckwith <wythe@intrig.com>

Signed-off-by: Mark Beckwith <wythe@intrig.com>

cdecker · 2018-12-05T22:50:19Z

Rebased on top of master, checked locally.

Will merge once CI is happy.

ACK 8a361b4

rustyrussell · 2018-12-06T02:05:58Z

Your additions look great. I will make a new PR for them (unless you want to). I also started on the man page.

You steal them :)

Thanks!

rustyrussell reviewed Nov 29, 2018

View reviewed changes

wythe force-pushed the rpc-check branch 2 times, most recently from f62d406 to 83fe4d4 Compare December 4, 2018 17:36

rustyrussell approved these changes Dec 5, 2018

View reviewed changes

common/json.c

*dest++ = *first++;

return arr;

}

Copy link

Contributor

rustyrussell Dec 5, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it should still work... I'll try it!

wythe added 4 commits December 5, 2018 23:38

json: add print, copy, and remove functions

305d4c8

Needed for check command. I left the print function in since it was so convenient for debugging purposes. Signed-off-by: Mark Beckwith <wythe@intrig.com>

param: add support for unused parameters

34bbb61

We can now set a flag to have param() ignore unexpected parameters. Normally unexpected parameters are considered errors. Needed by the check command. Signed-off-by: Mark Beckwith <wythe@intrig.com>

test: add json_tok_remove unit tests

8a361b4

Signed-off-by: Mark Beckwith <wythe@intrig.com>

cdecker force-pushed the rpc-check branch from 83fe4d4 to 8a361b4 Compare December 5, 2018 22:49

rustyrussell merged commit 164c764 into ElementsProject:master Dec 6, 2018

rpc: add check command #2123

rpc: add check command #2123

Uh oh!

Conversation

wythe commented Nov 29, 2018

Uh oh!

rustyrussell left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wythe commented Dec 4, 2018

Uh oh!

rustyrussell left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rustyrussell commented Dec 5, 2018

Uh oh!

wythe commented Dec 5, 2018

Uh oh!

cdecker commented Dec 5, 2018

Uh oh!

rustyrussell commented Dec 6, 2018

Uh oh!

Uh oh!