Field filtering: limit what fields will be returned from (almost) any JSON command. #5681

rustyrussell · 2022-10-31T06:07:37Z

Sometimes you only care about a single field from a request: this especially makes this faster if you're running remotely (RTL, commando, etc). So we add a simple _filter field, which is a template of the same form as the request, but only fields mentioned in this template will be included.

In future, commands may examine the filter to avoid calculations altogether. And pyln may also be enhanced to support it (plugins which don't support it have it stripped, so no change for them!).

jb55 · 2022-10-31T21:10:15Z

very nice

doc/PLUGINS.md

cdecker

Very cool idea. Apart from a minor suggestion to locate the result post-processing separately from the arguments to the method, I think this is good to go 👍

doc/PLUGINS.md

cdecker · 2022-11-01T17:20:46Z

doc/lightningd-rpc.7.md

+lightningd-rpc -- Lightning Daemon RPC Protocols
+================================================


Good idea to add this to the docs 👍

jb55 · 2022-11-01T21:38:53Z

On Tue, Nov 01, 2022 at 10:21:59AM -0700, Christian Decker wrote: An alternative to this would have been to slightly break with the JSON-RPC spec and allow us to add meta-parameters in the top-level dict, instead of in the `payload` keyed dict. This is would have been nice since the filtering, and eventually paging, is orthogonal to the actual operations we're asking `lightningd` to perform.

I originally assumed this is how it would have been implemented but I'm ok with the current implementation as well.

rustyrussell · 2022-11-01T23:30:28Z

Hmm, good point re: a separate field being neater. However, that also means every layer above needs to know about it.

lightning-cli needs a --filter argument
pyln.client and grpc need updating either way since we already need to either wrap them all or have a "set_filter" function.
commando needs to handle this somehow.
c-lightning-rest needs updating.

I'd like @ShahanaFarooqui input on this!

Signed-off-by: Rusty Russell <[email protected]>

ShahanaFarooqui · 2022-11-01T23:50:51Z

Keeping it as a separate field looks more logical and neat solution, even though it will require some top layer updates initially.

Regarding c-lightning-rest, this update should not take much time to implement.

Since the "struct command" is different from plugins and lightningd, we need an accessor for this to work (the plugin one is a dummy for now!). Signed-off-by: Rusty Russell <[email protected]>

rustyrussell · 2022-11-04T04:10:31Z

OK, this reworks it to use a (non-standard) "filter" object in the request. This is simpler, though we now need to have an explicit lightning-cli --filter option, and a context manager for pyln to set it.

cdecker

ACK 1ae3a47

cdecker · 2022-11-07T17:01:49Z

contrib/pyln-client/pyln/client/lightning.py

+        ```python
+        with rpc.reply_filter({"transactions": [{"outputs": [{"amount_msat": true, "type": true}]}]}):
+            rpc.listtransactions()
+        ```


This is very cool ^^

I stole it from your context code I think!!

cdecker · 2022-11-07T17:07:46Z

Source code checker choked on duplicate include. Fixed and force-pushed.

cdecker · 2022-11-08T13:40:57Z

Hm, still fails, this time because with clang + fuzzing we have a reference to the json_filter infrastructure that doesn't get linked in. I tried adding it to the linker, but then we start slowly leaking out into the lightningd-specific code, which we likely don't want at all. To isolate the fuzz targets from lightningd may take some more time and a bit of rework.

Two options:

Disable the fuzzing targets that reference those symbols, and keep this PR in the release
Defer this PR until the next release.

What do you think @rustyrussell ?

No tests currently use it, and if they do we'll want to do some per-test objects. Otherwise, we are about it introduce a dependency on common/json_filter.o, which is a can of worms. Signed-off-by: Rusty Russell <[email protected]>

rustyrussell · 2022-11-09T03:11:07Z

Found a trivial fix: remove the other JSON common objects from the fuzz makefile!

Ack 3bbdd35

Signed-off-by: Rusty Russell <[email protected]>

Signed-off-by: Rusty Russell <[email protected]> Changelog-Added: JSON-RPC: `filter` object allows reduction of JSON response to (most) commands.

We suppress schema reply checking when filter is set: we could just remove all the `required` fields in the JSON schema. Signed-off-by: Rusty Russell <[email protected]>

Signed-off-by: Rusty Russell <[email protected]> Changelog-Added: pyln: LightningRpc has new `reply_filter` context manager for reducing output of RPC commands.

Signed-off-by: Rusty Russell <[email protected]>

This documents how to communicate with lightningd over RPC, including use of the `filter` object. Signed-off-by: Rusty Russell <[email protected]> Changelog-Added: Documentation: `lightningd-rpc` manual page describes details of our JSON-RPC interface, including compatibility and filtering.

Signed-off-by: Rusty Russell <[email protected]> Changelog-Added: cli: new `--filter` parameter to reduce JSON output.

rustyrussell · 2022-11-09T09:55:41Z

Fuzz fix was trivial (stop including other JSON files).

Ack a6f38a2

rustyrussell added json-rpc plugin Optech Make Me Famous! Look! Look! Look! COOL NEW FEATURE! labels Oct 31, 2022

rustyrussell added this to the v22.11 milestone Oct 31, 2022

rustyrussell requested review from jb55 and ShahanaFarooqui October 31, 2022 06:07

rustyrussell requested a review from cdecker as a code owner October 31, 2022 06:07

jb55 reviewed Oct 31, 2022

View reviewed changes

doc/PLUGINS.md Outdated Show resolved Hide resolved

cdecker approved these changes Nov 1, 2022

View reviewed changes

common/json_filter: routines for json filtering.

0ebb56c

Signed-off-by: Rusty Russell <[email protected]>

rustyrussell force-pushed the guilt/field-filter branch from e3252e2 to f451c08 Compare November 1, 2022 23:32

common/json_filter: routine to turn "filter" JSON into a filter.

1a3da10

Since the "struct command" is different from plugins and lightningd, we need an accessor for this to work (the plugin one is a dummy for now!). Signed-off-by: Rusty Russell <[email protected]>

rustyrussell force-pushed the guilt/field-filter branch 2 times, most recently from 8632d98 to ebbc324 Compare November 4, 2022 04:08

cdecker approved these changes Nov 7, 2022

View reviewed changes

cdecker force-pushed the guilt/field-filter branch from ebbc324 to 1ae3a47 Compare November 7, 2022 17:07

tests/fuzz: don't pull in JSON common at all.

46925e4

No tests currently use it, and if they do we'll want to do some per-test objects. Otherwise, we are about it introduce a dependency on common/json_filter.o, which is a can of worms. Signed-off-by: Rusty Russell <[email protected]>

rustyrussell force-pushed the guilt/field-filter branch from 1ae3a47 to 3bbdd35 Compare November 9, 2022 03:11

rustyrussell added 4 commits November 9, 2022 14:10

common/json_stream: support filtering don't print fields not allowed.

9c5533a

Signed-off-by: Rusty Russell <[email protected]>

common/test: add unit tests for JSON filtering.

b064389

Signed-off-by: Rusty Russell <[email protected]>

lightningd: set filter when we see 'filter' object.

12f237c

Signed-off-by: Rusty Russell <[email protected]> Changelog-Added: JSON-RPC: `filter` object allows reduction of JSON response to (most) commands.

pytest: add filter tests.

a5077f3

We suppress schema reply checking when filter is set: we could just remove all the `required` fields in the JSON schema. Signed-off-by: Rusty Russell <[email protected]>

rustyrussell added 4 commits November 9, 2022 14:10

pyln: add context manager to simpify filter use.

ee3517f

Signed-off-by: Rusty Russell <[email protected]> Changelog-Added: pyln: LightningRpc has new `reply_filter` context manager for reducing output of RPC commands.

libplugin: support filters.

221018d

Signed-off-by: Rusty Russell <[email protected]>

lightning-cli: support --filter parameter.

a6f38a2

Signed-off-by: Rusty Russell <[email protected]> Changelog-Added: cli: new `--filter` parameter to reduce JSON output.

rustyrussell force-pushed the guilt/field-filter branch from 3bbdd35 to a6f38a2 Compare November 9, 2022 03:41

rustyrussell merged commit ae3550c into ElementsProject:master Nov 9, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Field filtering: limit what fields will be returned from (almost) any JSON command. #5681

Field filtering: limit what fields will be returned from (almost) any JSON command. #5681

rustyrussell commented Oct 31, 2022

jb55 commented Oct 31, 2022

cdecker left a comment

cdecker Nov 1, 2022

jb55 commented Nov 1, 2022 via email

rustyrussell commented Nov 1, 2022

ShahanaFarooqui commented Nov 1, 2022

rustyrussell commented Nov 4, 2022

cdecker left a comment •

edited

Loading

cdecker Nov 7, 2022

rustyrussell Nov 9, 2022

cdecker commented Nov 7, 2022

cdecker commented Nov 8, 2022

rustyrussell commented Nov 9, 2022 •

edited

Loading

rustyrussell commented Nov 9, 2022

		lightningd-rpc -- Lightning Daemon RPC Protocols
		================================================

Field filtering: limit what fields will be returned from (almost) any JSON command. #5681

Field filtering: limit what fields will be returned from (almost) any JSON command. #5681

Conversation

rustyrussell commented Oct 31, 2022

jb55 commented Oct 31, 2022

cdecker left a comment

Choose a reason for hiding this comment

cdecker Nov 1, 2022

Choose a reason for hiding this comment

jb55 commented Nov 1, 2022 via email

rustyrussell commented Nov 1, 2022

ShahanaFarooqui commented Nov 1, 2022

rustyrussell commented Nov 4, 2022

cdecker left a comment • edited Loading

Choose a reason for hiding this comment

cdecker Nov 7, 2022

Choose a reason for hiding this comment

rustyrussell Nov 9, 2022

Choose a reason for hiding this comment

cdecker commented Nov 7, 2022

cdecker commented Nov 8, 2022

rustyrussell commented Nov 9, 2022 • edited Loading

rustyrussell commented Nov 9, 2022

cdecker left a comment •

edited

Loading

rustyrussell commented Nov 9, 2022 •

edited

Loading