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.

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

bug: PUT /filter/v2/subscriptions doesn't exist #969

Closed
fbarbu15 opened this issue Dec 21, 2023 · 5 comments
Closed

bug: PUT /filter/v2/subscriptions doesn't exist #969

fbarbu15 opened this issue Dec 21, 2023 · 5 comments
Assignees
Labels
bug Something isn't working

Comments

@fbarbu15
Copy link

This is more of a question than a bug.
On nwaku there is this method as described here https://waku-org.github.io/waku-rest-api/#put-/filter/v2/subscriptions
If it's not needed on go-waku please close this bug

@fbarbu15 fbarbu15 added the bug Something isn't working label Dec 21, 2023
@chaitanyaprem
Copy link
Collaborator

This API doesn't seem right.
AS per HTTP PUT means it replaces existing resource with a new one, but as Filter protocol does not support replacing an existing subscription, rather only adding to it I don't think it is correct to have a PUT method.

cc @NagyZoltanPeter , this needs to be fixed in nwaku as well.

@chaitanyaprem chaitanyaprem self-assigned this Dec 26, 2023
@NagyZoltanPeter
Copy link

This API doesn't seem right. AS per HTTP PUT means it replaces existing resource with a new one, but as Filter protocol does not support replacing an existing subscription, rather only adding to it I don't think it is correct to have a PUT method.

cc @NagyZoltanPeter , this needs to be fixed in nwaku as well.

I agree the current interface is not pretty clear. @jm-clius WDYT?
But the interface is not wrong on REST API level but limited on the filter protocol level.
What do we achieve if we remove PUT from the API level and the client will constantly call POST filter subscribe?
As I remember client must fully aware of its current subscriptions, but seems the only way for the client to keep it consistent is to unsubscribe and fully subscribe again.
I can imagine a solution that still having PUT on API level but will handle request as an always complete one subscription one and would do a full replace on subscribed topics in place.
Thinking of client uses DELETE and than full new POST requests can lead to miss some of the messages in between.

@jm-clius
Copy link

jm-clius commented Jan 2, 2024

Indeed, the filter protocol only allows for modification not replacement of existing subscriptions. This, however, is a server-side limitation and not on the client-side. Clients can always "replace" one of their subscriptions by doing an "unsubscribe all" followed by a new "subscribe request". Since this is a client-side API, why not have the PUT method follow exactly this chain of events - i.e. publish an "unsubscribe all" and, on success, publish a new filter request? This seems to me to be a useful endpoint and consistent with idiomatic HTTP usage. WDYT?

@NagyZoltanPeter
Copy link

Indeed, the filter protocol only allows for modification not replacement of existing subscriptions. This, however, is a server-side limitation and not on the client-side. Clients can always "replace" one of their subscriptions by doing an "unsubscribe all" followed by a new "subscribe request". Since this is a client-side API, why not have the PUT method follow exactly this chain of events - i.e. publish an "unsubscribe all" and, on success, publish a new filter request? This seems to me to be a useful endpoint and consistent with idiomatic HTTP usage. WDYT?

Well, I cannot fully agree with it in this form or we should explicitly tell the operators to always use full subscription definition in API calls. This discrepancy from the protocol usage - that is likely to be examined by an operator - can lead to misunderstanding.
In this way it might be better to remove PUT as @chaitanyaprem suggested than to over engineer behind the scene.
The only drawback not supporting instant update of the subscriptions that client may miss messages in between unsubs and re-subs requests.

@chair28980
Copy link
Contributor

@waku-org/nwaku-developers please create an issue in the nwaku repo to remove the REST api referenced here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Archived in project
Development

No branches or pull requests

5 participants