feat: HTTP REST API: Filter support v2 #1890

NagyZoltanPeter · 2023-08-07T09:50:36Z

Description

WIP PR to allow pre-check.

Add support for Waku Filter v2 specification on Rest API interface.
Integrate use of v2 filter into node.

Changes

Updated openapi.yaml doc for v1 and v2 support
Separation of legacy filter rest api
Add client interface for v2 filter rest api
Add handler for processing v2 rest requests
Integrate v2 filter into node
Add tests for v2
Extend tests with failure cases
Rebased to latest master with applied autoshard feat on FilterV2 subscribe/unsubscribe

Tracks

#1872

NagyZoltanPeter · 2023-08-07T09:51:59Z

@jm-clius : Please check if the interface is ok.

Also a remark to https://rfc.vac.dev/spec/12/
RequestId is mandatory part of the communication, but can't really see the use of it.
Rather I would add a subscription key that is generated by the peer and can identify it when sending subsequent updates or deletes to its subscription. So to avoid unintended modification (and discovery) to peers subscription data by knowing only the peer_id. Such a key could be stored on service side. What do you think?

jm-clius

Added some comments :)

waku/v2/node/rest/filter/openapi.yaml

NagyZoltanPeter · 2023-08-29T09:13:20Z

Hi, @jm-clius, @Ivansete-status, @SionoiS

Please review this PR, I intend to be the finalized Filter V2 - Rest API interface.
Although there are number of changes in the protocol level, but mainly about conversion of types.

@SionoiS I would like to ask your special attention on Autoshard part of legacy filter and filter v2 adaptation. I hope I can apply it carefully while rebasing/merging. Also I would like to discuss to apply some more test cases on sub/unsub using sharding. WDYT?

github-actions · 2023-08-29T09:16:07Z

You can find the image built from this PR at

quay.io/wakuorg/nwaku-pr:1890

Built from dbdc197

Ivansete-status

Thanks for the PR!
I'm adding a bunch of reviews so that you can start checking it. Tomorrow morning I will study this PR again, and potentially add more comments
Cheers

Makefile

apps/wakunode2/app.nim

waku/node/rest/filter/handlers.nim

waku/node/waku_node.nim

SionoiS

LGTM

See comments but nothing major.

tests/wakunode_rest/test_rest_filter.nim

waku/node/rest/filter/openapi.yaml

waku/node/waku_node.nim

NagyZoltanPeter · 2023-08-29T14:31:08Z

Sorry, it was my bad to screw up zerokit submodule commit while doing the rebase. Not it is fixed and align with master.
Thanks for pointing out @Ivansete-status

jm-clius

Not approving yet because of comment:

blocking: question below re need for internal and external caches or message handlers. I think we can simplify this PR and the node logic if we remove this for now.

Another non-blocking comment, as this really relates to autosharding:
have commented on the autosharding PRs, but I think we need to simplify what we require of the node's API here. For example, I see loads of replication in the logic between the subscribe and unsubscribe - such as mapping contentTopics to pubsubTopic, contentTopic pairs using the parseSharding method, which I imagine can be handled as helper methods within the sharding modules. I think we need to follow this up with PRs that simplify waku_node.nim (perhaps the async queue work that @SionoiS has opened does exactly this - will still review).

Makefile

waku/node/waku_node.nim

jm-clius · 2023-08-29T17:21:38Z

Btw, especially if a PR remains open for quite some time, it's helpful to "resolve" older comments that are no longer relevant or that you've addressed. This way when a reviewer comes back to the PR it's easy to navigate through the history and see what has been resolved and what is still to be addressed. :)

Ivansete-status · 2023-08-30T10:35:16Z

waku/node/waku_node.nim

+
+  let remotePeerRes = parsePeerInfo(peer)
+  if remotePeerRes.isErr():
+    error "Couldn't parse the peer info properly", error = remotePeerRes.error


Something that we should enhance in the future is to make the waku_node trace the minimum possible (or nothing ideally)

From my point of view, we should always deal with Result[T] types and leave the traces for the upper layers. There shouldn't be any trace/log in waku_node. wdyt about this statement @jm-clius ?

SionoiS · 2023-08-30T20:09:55Z

waku/node/rest/filter/handlers.nim

+
+    let req: FilterUnsubscribeRequest = decodedBody.value()
+
+    let peerOpt = node.peerManager.selectPeer(WakuFilterCodec)


Doesn't this select a random peer?

When unsubscribing we should use the same peer than for subscribing no?

NagyZoltanPeter · 2023-08-31T02:27:45Z

@jm-clius , @Ivansete-status , @SionoiS
Hereby I would like to come to conclusion on approach mounting
Filter V2 client vs. legacy one.

All of you found this part questionable. I expected about it but would like to come up with some sort of solution.
Mostly because it derives from the same question the led to SP's cache handling proposal.

Unlike realy's and legacy filte's approach where we add message callback per subscription, FilterV2 changes it to one global handler to be provided by its user at creation. So it makes it a bit more difficult to provide it at the stage of creation. Maybe less flexible. So for easy use, I gave an option to handle it internally for the app level and give a generic caching of received messages from the push by the filter service peer.

I feel both option a bit limiting the app level possibilities and not that consistent.
Maybe a bit better abstraction could be more handy / happy to discuss /.

But for shorthand solution we may think it is enough that app level provides a handler for the messages pushed and do what it wants about it.

Noticing here the discussion just recently popped out on discord that it may needs a bit wider thinking where - what layer - to provide any guaranties over messages delivered to the light client.

I agree it is clumsy as now it is, so some fix is necessary.

Wdyt?

jm-clius · 2023-08-31T10:55:34Z

FilterV2 changes it to one global handler to be provided by its user at creation.

Right, I see your point. I still wouldn't add an intermediate handler and cache though, as it provides functionality which is only really needed by the API. In this case the API has some application-like requirements of how the pushed messages should be handled, but cannot effect this as it is not responsible for creating the filterClient. This indeed points out a flaw in the architecture. My suggestion here would be to change the filter client to have a list of global push handlers rather than just one set at creation and to expose an addPushHandler()-like method. This way applications can add handlers to an already-mounted filter client. The API should then add a push handler that populates a cache and expose a read function that returns the cached messages to REST clients (and clears the cache). This is to provide a polling mechanism via REST, similar to what we have for JSON-RPC API. WDYT?

SionoiS · 2023-08-31T12:16:38Z

Seams like the simplest thing to do for now would be what @jm-clius wrote.

Allow registering handlers in filterClient. Rest API register handlers for caches. APPs can also register handlers.

NagyZoltanPeter · 2023-09-05T10:36:32Z

@Ivansete-status @jm-clius
I've done with fixing the findings (do hope).
Please re-check it as the filter push handling involved quite number of change also in interfaces.

Note: I tried different approaches, but was not happy with them. So I went back the approach used with Filter v1, and providing callback at subscribe. So does rest interface is extended as well with get message for V2.

Note2.: I will see if ci goes ok and check if needs any further action.

Note3.: I did not rebased again for ease your review now for better compare against previous.

github-actions · 2023-09-05T10:37:49Z

You can find the experimental image built from this PR at

quay.io/wakuorg/nwaku-pr:1890-experimental

jm-clius

Right, I would really like to approve and thanks for your patience with this, but I don't quite see the benefit of introducing complex subscription management logic into the filter protocol. What particular benefit does this provide above the far simpler approach of adding handlers to the client object itself? Perhaps I'm missing part of the picture here, so happy to set a call to clear this up.

tests/wakunode_rest/test_rest_filter.nim

NagyZoltanPeter · 2023-09-06T12:40:10Z

@Ivansete-status @jm-clius
Although I'm now finished with all fixes and catch ups where tests / examples / chat app were broken I have put back to draft while I will make agreed changes with Hanno.
We discussed to shoot for a simpler less restrictive interface on FilterPushHandling, while keep the possibility of poll messages through REST API if.
It means, it will be not necessary to add handler per subscription but apart from that.
I will notice you when ready. But /health endpoint has now higher prio first.

Ivansete-status

It looks really great in general! Good job!
However, I've added a few nitpicks and a comment where I suggest to submit a separate PR to refactor the waku/waku_filter/client.nim file.

examples/filter_subscriber.nim

tests/wakunode_rest/test_rest_filter.nim

waku/node/rest/filter/client.nim

waku/node/rest/filter/handlers.nim

waku/node/waku_node.nim

waku/waku_core/subscription.nim

jm-clius

I've requested some changes. After they are implemented I'll be ready to approve the rest of this PR. :) I think it will be too much overhead to change this now, but there could have been a great many individual PRs here. For complex PRs, even basic refactoring (such as changing line lengths) should be split into separate PRs. The API handlers and types could be a PR separate from the actual integration with the node, etc. I know this has been mentioned before, but since then the PR has still grown quite a bit.

waku/node/waku_node.nim

jm-clius · 2023-09-12T16:45:28Z

waku/node/waku_node.nim

-    return
+proc filterUnsubscribe*(node: WakuNode,
+                          pubsubTopic: Option[PubsubTopic],
+                          contentTopics: Option[seq[ContentTopic]],


I don't think this should be an Option. seq[ContentTopic] can be empty, but that would result in an err returned by the underlying client protocol. We don't want to build much logic into waku_node belonging to underlying protocols - it simply exists to mount, integrate and expose a common API for underlying protocols. The exception here is that we do provide autosharding "middleware" in the waku_node - in other words, logic to populate the pubsubTopic if absent from API calls. But even this should probably be extracted to another module. The waku_node should in theory just provide a filterSubscribe, filterUnsubscribe and filterUnsubscribeAll- not interpret certain API calls as one or the other based on added logic.

waku/node/waku_node.nim

NagyZoltanPeter · 2023-09-13T04:24:45Z

I needed to rebase to latest master to kick in ci checks, hence I did a squash (that would be done anyway by GH).

NagyZoltanPeter · 2023-09-14T06:09:50Z

@jm-clius, @Ivansete-status :
Please look at the final stage of this long long running PR.
Last changes:

Addressed interface change on waku_node, introduced filterUnsubscribeAll.
It involves changes on Rest interface, new endpoint added: /filter/v2/subscriptions/all with HTTP DELETE.

jm-clius

Happy with the changes now. Good job. :) Let's get this merged! Thanks for your patience.

I do think all relevant observations are answered or fixed. I'm not pretty sure which one is signed by GH as change requested though.

Ivansete-status

LGTM! Thanks for it and for the patience! 💯

…eeded Filter rest api documentation updated with v1 and v2 interface support. Separated legacy filter rest interface Fix code and tests of v2 Filter rest api Filter v2 message push test added Small enhancment on Makefile. 'make list' will list all targets defined Applied autoshard to Filter V2 Fix of PR review comments - removed unnecessary defaults, long lines, added rest filter tests to all tests Fix PR findings: Redesigned FilterPushHandling, code style, catch up apps and tests with filter v2 interface changes Fixed broken interface effect in exemples, warning elimination Rename of FilterSubscriptionsRequest (Filter Rest API schema type) to FilterSubscribeRequest Rename of FilterV1SubscriptionsRequest to FilterLegacySubscribeRequest, fix broken chat2 app, fix tests Changed Filter v2 push handler subscription to simple register Fix PR styling foundings

…upon review

NagyZoltanPeter requested review from jm-clius and Ivansete-status August 7, 2023 09:50

NagyZoltanPeter self-assigned this Aug 7, 2023

NagyZoltanPeter linked an issue Aug 7, 2023 that may be closed by this pull request

feat: HTTP REST API: Filter support v2 #1872

Closed

3 tasks

NagyZoltanPeter changed the title ~~HTTP REST API: Filter support v2 - WIP~~ feat: HTTP REST API: Filter support v2 - WIP Aug 7, 2023

jm-clius reviewed Aug 16, 2023

View reviewed changes

NagyZoltanPeter force-pushed the feat-1851-http-rest-api-filter-v2 branch from ede943b to 80158ee Compare August 23, 2023 13:52

NagyZoltanPeter changed the title ~~feat: HTTP REST API: Filter support v2 - WIP~~ feat: HTTP REST API: Filter support v2 Aug 25, 2023

NagyZoltanPeter force-pushed the feat-1851-http-rest-api-filter-v2 branch 2 times, most recently from 652cf8f to 18c2953 Compare August 29, 2023 09:06

NagyZoltanPeter requested review from jm-clius and SionoiS August 29, 2023 09:08

NagyZoltanPeter marked this pull request as ready for review August 29, 2023 09:33

Ivansete-status previously requested changes Aug 29, 2023

View reviewed changes

SionoiS approved these changes Aug 29, 2023

View reviewed changes

jm-clius reviewed Aug 29, 2023

View reviewed changes

Makefile Outdated Show resolved Hide resolved

waku/node/waku_node.nim Outdated Show resolved Hide resolved

waku/node/waku_node.nim Outdated Show resolved Hide resolved

waku/node/waku_node.nim Outdated Show resolved Hide resolved

Ivansete-status reviewed Aug 30, 2023

View reviewed changes

SionoiS reviewed Aug 30, 2023

View reviewed changes

NagyZoltanPeter requested review from jm-clius and Ivansete-status September 5, 2023 10:32

jm-clius reviewed Sep 6, 2023

View reviewed changes

SionoiS reviewed Sep 6, 2023

View reviewed changes

tests/wakunode_rest/test_rest_filter.nim Outdated Show resolved Hide resolved

NagyZoltanPeter marked this pull request as draft September 6, 2023 12:35

NagyZoltanPeter mentioned this pull request Sep 11, 2023

feat: HTTP REST API: Filter support v2 #1872

Closed

3 tasks

NagyZoltanPeter requested review from jm-clius and SionoiS September 12, 2023 00:17

NagyZoltanPeter marked this pull request as ready for review September 12, 2023 08:21

Ivansete-status reviewed Sep 12, 2023

View reviewed changes

NagyZoltanPeter mentioned this pull request Sep 12, 2023

chore: move SubscriptionManager under waku_core #2025

Merged

jm-clius requested changes Sep 12, 2023

View reviewed changes

NagyZoltanPeter force-pushed the feat-1851-http-rest-api-filter-v2 branch from 77f88f8 to 1734cf0 Compare September 13, 2023 04:08

NagyZoltanPeter requested review from jm-clius and Ivansete-status September 14, 2023 05:59

jm-clius approved these changes Sep 14, 2023

View reviewed changes

Ivansete-status approved these changes Sep 14, 2023

View reviewed changes

NagyZoltanPeter and others added 3 commits September 14, 2023 20:32

Fix broken chat2bridge

d318972

Separate node's filterUnsubscrube and unsubscrube all, various fixes …

880d018

…upon review

NagyZoltanPeter force-pushed the feat-1851-http-rest-api-filter-v2 branch from 8791875 to 880d018 Compare September 14, 2023 18:43

NagyZoltanPeter merged commit dac072f into master Sep 14, 2023
16 checks passed

NagyZoltanPeter deleted the feat-1851-http-rest-api-filter-v2 branch September 14, 2023 19:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: HTTP REST API: Filter support v2 #1890

feat: HTTP REST API: Filter support v2 #1890

NagyZoltanPeter commented Aug 7, 2023 •

edited

NagyZoltanPeter commented Aug 7, 2023 •

edited

jm-clius left a comment

NagyZoltanPeter commented Aug 29, 2023

github-actions bot commented Aug 29, 2023 •

edited

Ivansete-status left a comment

SionoiS left a comment

NagyZoltanPeter commented Aug 29, 2023

jm-clius left a comment

jm-clius commented Aug 29, 2023

Ivansete-status Aug 30, 2023

SionoiS Aug 30, 2023

NagyZoltanPeter commented Aug 31, 2023

jm-clius commented Aug 31, 2023

SionoiS commented Aug 31, 2023

NagyZoltanPeter commented Sep 5, 2023

github-actions bot commented Sep 5, 2023

jm-clius left a comment

NagyZoltanPeter commented Sep 6, 2023

Ivansete-status left a comment

jm-clius left a comment

jm-clius Sep 12, 2023

NagyZoltanPeter commented Sep 13, 2023

NagyZoltanPeter commented Sep 14, 2023

jm-clius left a comment

Ivansete-status left a comment


		let req: FilterUnsubscribeRequest = decodedBody.value()

		let peerOpt = node.peerManager.selectPeer(WakuFilterCodec)

feat: HTTP REST API: Filter support v2 #1890

feat: HTTP REST API: Filter support v2 #1890

Conversation

NagyZoltanPeter commented Aug 7, 2023 • edited

Description

Changes

Tracks

NagyZoltanPeter commented Aug 7, 2023 • edited

jm-clius left a comment

Choose a reason for hiding this comment

NagyZoltanPeter commented Aug 29, 2023

github-actions bot commented Aug 29, 2023 • edited

Ivansete-status left a comment

Choose a reason for hiding this comment

SionoiS left a comment

Choose a reason for hiding this comment

NagyZoltanPeter commented Aug 29, 2023

jm-clius left a comment

Choose a reason for hiding this comment

jm-clius commented Aug 29, 2023

Ivansete-status Aug 30, 2023

Choose a reason for hiding this comment

SionoiS Aug 30, 2023

Choose a reason for hiding this comment

NagyZoltanPeter commented Aug 31, 2023

jm-clius commented Aug 31, 2023

SionoiS commented Aug 31, 2023

NagyZoltanPeter commented Sep 5, 2023

github-actions bot commented Sep 5, 2023

jm-clius left a comment

Choose a reason for hiding this comment

NagyZoltanPeter commented Sep 6, 2023

Ivansete-status left a comment

Choose a reason for hiding this comment

jm-clius left a comment

Choose a reason for hiding this comment

jm-clius Sep 12, 2023

Choose a reason for hiding this comment

NagyZoltanPeter commented Sep 13, 2023

NagyZoltanPeter commented Sep 14, 2023

jm-clius left a comment

Choose a reason for hiding this comment

Ivansete-status left a comment

Choose a reason for hiding this comment

NagyZoltanPeter commented Aug 7, 2023 •

edited

NagyZoltanPeter commented Aug 7, 2023 •

edited

github-actions bot commented Aug 29, 2023 •

edited