search

waku-org / nwaku

Waku node and protocol.
Other
183 stars 46 forks source link

fix: store v3 validate cursor & remove messages #2636

Closed SionoiS closed 2 weeks ago

SionoiS commented 3 weeks ago

Fixes https://github.com/waku-org/nwaku/issues/2635 and https://github.com/waku-org/nwaku/issues/2634

github-actions[bot] commented 3 weeks ago

You can find the image built from this PR at

quay.io/wakuorg/nwaku-pr:2636-rln-v2

Built from 6f4da99e5532d7b6d900a2fcc28556cbe81264da

github-actions[bot] commented 3 weeks ago

You can find the image built from this PR at

quay.io/wakuorg/nwaku-pr:2636-rln-v1

Built from 6f4da99e5532d7b6d900a2fcc28556cbe81264da

richard-ramos commented 3 weeks ago

What should happen if I send a request with a valid cursor that does not exist? i.e. 0x0000000000000000000000000000000000000000000000000000000000000000:


{"request_id":"ec560c592484291ca6dfa1491507a45dba55b238c2b271cf0cb81ce1e0590fce","include_data":true,"pubsub_topic":"/waku/2/default-waku/proto","content_topics":["test"],"time_start":1714155079182460704,"time_end":1714155079285053447,"pagination_cursor":"AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA=","pagination_forward":true,"pagination_limit":20}

Currently it is returning all the records but it seems to me that this behavior is not valid, and perhaps it should return either no results, or an invalid_cursor error. I think we should do the later, and probably do a SELECT EXISTS(SELECT 1 FROM messages WHERE message_hash=?) to determine if the cursor is valid. Since the message_hash is the primary key of the table I imagine this should execute fast with no major impact on the total store query response time

github-actions[bot] commented 2 weeks ago

This PR may contain changes to database schema of one of the drivers.

If you are introducing any changes to the schema, make sure the upgrade from the latest release to this change passes without any errors/issues.

Please make sure the label release-notes is added to make sure upgrade instructions properly highlight this change.

SionoiS commented 2 weeks ago

Currently it is returning all the records but it seems to me that this behavior is not valid, and perhaps it should return either no results, or an invalid_cursor error. I think we should do the later, and probably do a SELECT EXISTS(SELECT 1 FROM messages WHERE message_hash=?) to determine if the cursor is valid. Since the message_hash is the primary key of the table I imagine this should execute fast with no major impact on the total store query response time

Seams like the old code never validated the cursor, it's only used to order the results. I agree that we should do it though.

richard-ramos commented 2 weeks ago

Seams like the old code never validated the cursor

I see! It can be done later in a separate PR then. Will open an issue so we don't lose track of this :) I just tested this PR with my set of tests in go-waku and works fine!

-- EDIT: Tracking issue in here https://github.com/waku-org/nwaku/issues/2653