Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Datastore cleanup and bugfix #5994

Merged
merged 4 commits into from
Feb 11, 2023
Merged

Datastore cleanup and bugfix #5994

merged 4 commits into from
Feb 11, 2023

Conversation

rustyrussell
Copy link
Contributor

No description provided.

@rustyrussell
libplugin: don't spew datastore errors to LOG_DEBUG.
603e342 
People get upset, especially as our "not found" error can be a bit
hard to read!

Signed-off-by: Rusty Russell <[email protected]>
See-also: ElementsProject#5990
@rustyrussell rustyrussell added this to the v23.02 milestone Feb 10, 2023
@m-schmoock
Copy link
Collaborator

m-schmoock commented Feb 10, 2023 •
edited
Loading

@rustyrussell
Now the token->type of the string is not JSMN_STRING anymore but JSMN_PRIMITIVE which fails inside the param_escaped_string callback, as it expects it to also be a string?

@m-schmoock
Copy link
Collaborator

m-schmoock commented Feb 10, 2023 •
edited
Loading

This 'fixes' the testcases (e.g. tests/test_plugin.py::test_commando_rune), but without me knowing what I'm doing there and if that's correct :D

I could not find the code where the tok->type is actually set when we pass a param_escaped_string callback, thats why I change the accepting/expecting code in the callback which gets it now as JSMN_PRIMITIVE for whatever reason.

diff --git a/common/json_param.c b/common/json_param.c
index 7aa217464..e8e9bf3ff 100644
--- a/common/json_param.c
+++ b/common/json_param.c
@@ -417,7 +417,7 @@ struct command_result *param_escaped_string(struct command *cmd,
                                            const jsmntok_t *tok,
                                            const char **str)
 {
-       if (tok->type == JSMN_STRING) {
+       if (tok->type == JSMN_STRING || tok->type == JSMN_PRIMITIVE) {
                struct json_escape *esc;
                /* jsmn always gives us ~ well-formed strings. */
                esc = json_escape_string_(cmd, buffer + tok->start,

@m-schmoock m-schmoock force-pushed the guilt/datastore-cleanups branch from 8c4aa7f to 1dd29ea Compare February 10, 2023 23:00
@rustyrussell
Copy link
Contributor Author

rustyrussell commented Feb 11, 2023 •
edited
Loading

@rustyrussell Now the token->type of the string is not JSMN_STRING anymore but JSMN_PRIMITIVE which fails inside the param_escaped_string callback, as it expects it to also be a string?

Yes, the previous code would accept anything. But that's clearly a bug, we should insist that a parameter called "string" be a string?

Indeed, fixed.

rustyrussell and others added 3 commits February 11, 2023 12:03
@rustyrussell
commando: don't try putting an integer as the 'string' parameter to "…
52e3ef8 
…datastore".

This only worked because we handled the JSON raw: next patch prohibits this.

Signed-off-by: Rusty Russell <[email protected]>
@rustyrussell
lightningd: unescape JSON strings for db.
7b40edf 
We were feeding in the raw JSON, which escapes \".  Then we were
escaping *again* to return it.

Reported-by: @m-schmook
Signed-off-by: Rusty Russell <[email protected]>
Changelog-Fixed: JSON-RPC: `datastore` handles escapes in `string` parameter correctly.
@m-schmoock @rustyrussell
pytest: adds xfail test that shows datastore issues
6992e42 
When doing some plugin related work, I discovered that the datastore API
has two issues:

- Error messages on startup of plugins init method when the datastore is
  still completely empty: "Parsing '{datastore:[0:': token has no index 0: []"
- Data is escaped but not unwrapped again when sending and getting from
  the API.

[ Removed xfail, it now passes! --RR ]
Closes: ElementsProject#5990
@rustyrussell rustyrussell force-pushed the guilt/datastore-cleanups branch from 1dd29ea to 6992e42 Compare February 11, 2023 01:34
@m-schmoock
Copy link
Collaborator

Yes, the previous code would accept anything. But that's clearly a bug, we should insist that a parameter called "string" be a string?

Indeed, fixed.

Yes, thats much better. I din't see that this was caused by the commando plugin sending a u64 'string'.

@m-schmoock
Copy link
Collaborator

ACK 6992e42

endothermicdev
endothermicdev approved these changes Feb 11, 2023
View reviewed changes
Copy link
Collaborator

@endothermicdev endothermicdev left a comment

Choose a reason for hiding this comment

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

I like allowing the caller to handle the empty result case.

ACK 6992e42

endothermicdev
endothermicdev reviewed Feb 11, 2023
View reviewed changes
plugins/libplugin.h
@@ -310,17 +310,19 @@ void rpc_scan(struct plugin *plugin,
const char *guide,
...);

/* Helper to scan datastore: can only be used in init callback. *
Returns false if field does not exist. * path is /-separated. Final
arg is JSON_SCAN or JSON_SCAN_TAL.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wouldn't have thought a comment block could give me anxiety, but this one succeeded!

@endothermicdev endothermicdev merged commit 698eb04 into ElementsProject:master Feb 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@endothermicdev endothermicdev endothermicdev approved these changes

Assignees
No one assigned
Labels
plugin
Projects
None yet
Milestone
v23.02
Development

Successfully merging this pull request may close these issues.
3 participants
@rustyrussell @m-schmoock @endothermicdev