New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

rpc: add check command #2123

Merged
merged 4 commits into from Dec 6, 2018

Conversation

Projects
None yet
4 participants
@wythe
Copy link
Collaborator

wythe commented Nov 29, 2018

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
Copy link
Contributor

rustyrussell left a comment

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.
{
const jsmntok_t *first = params;
const jsmntok_t *last = json_next(params);
printf("size: %d, count: %ld\n", params->size, last - first);

This comment has been minimized.

@rustyrussell

rustyrussell Nov 29, 2018

Contributor

"count: %td" (ptrdiff_t) BTW.

Show resolved Hide resolved common/json.c
*dest++ = *first++;

return arr;
}

This comment has been minimized.

@rustyrussell

rustyrussell Nov 29, 2018

Contributor

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);

This comment has been minimized.

@wythe

wythe Nov 30, 2018

Collaborator

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).

This comment has been minimized.

@rustyrussell

rustyrussell Dec 5, 2018

Contributor

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

This comment has been minimized.

@wythe

wythe Dec 5, 2018

Collaborator

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

Show resolved Hide resolved common/json.c Outdated
*first = '"';
first++;
}
}

This comment has been minimized.

@rustyrussell

rustyrussell Nov 29, 2018

Contributor

Cute trick :)

}

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

This comment has been minimized.

@rustyrussell

rustyrussell Nov 29, 2018

Contributor

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

This comment has been minimized.

@rustyrussell

rustyrussell Nov 29, 2018

Contributor

... or just call it 'check'?

This comment has been minimized.

@ZmnSCPxj

ZmnSCPxj Nov 30, 2018

Collaborator

command seems better to me.

This comment has been minimized.

@wythe

wythe Nov 30, 2018

Collaborator

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.

This comment has been minimized.

@rustyrussell

rustyrussell Dec 2, 2018

Contributor

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 :)

Show resolved Hide resolved 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",

This comment has been minimized.

@rustyrussell

rustyrussell Nov 29, 2018

Contributor

Can we modify parse_by_name similarly?

Currently:

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

This comment has been minimized.

@wythe

wythe Dec 3, 2018

Collaborator

oops.

@wythe wythe force-pushed the wythe:rpc-check branch 2 times, most recently from f62d406 to 83fe4d4 Dec 3, 2018

@wythe

This comment has been minimized.

Copy link
Collaborator

wythe commented Dec 4, 2018

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
Copy link
Contributor

rustyrussell left a comment

Ack 83fe4d4

*dest++ = *first++;

return arr;
}

This comment has been minimized.

@rustyrussell

rustyrussell Dec 5, 2018

Contributor

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

@rustyrussell

This comment has been minimized.

Copy link
Contributor

rustyrussell commented Dec 5, 2018

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

This comment has been minimized.

Copy link
Collaborator

wythe commented Dec 5, 2018

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

wythe added some commits Nov 21, 2018

json: add print, copy, and remove functions
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
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>
rpc: add check command
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>
test: add json_tok_remove unit tests
Signed-off-by: Mark Beckwith <wythe@intrig.com>

@cdecker cdecker force-pushed the wythe:rpc-check branch from 83fe4d4 to 8a361b4 Dec 5, 2018

@cdecker

This comment has been minimized.

Copy link
Member

cdecker commented Dec 5, 2018

Rebased on top of master, checked locally.

Will merge once CI is happy.

ACK 8a361b4

@rustyrussell

This comment has been minimized.

Copy link
Contributor

rustyrussell commented Dec 6, 2018

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 rustyrussell merged commit 164c764 into ElementsProject:master Dec 6, 2018

2 checks passed

ackbot PR ack'd by cdecker
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment