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

fix: invalid cursor returning messages #2724

Merged
merged 2 commits into from
May 27, 2024
Merged

fix: invalid cursor returning messages #2724

merged 2 commits into from
May 27, 2024

Conversation

SionoiS
Copy link
Contributor

@SionoiS SionoiS commented May 23, 2024

Store v3 queries with invalid cursors will no longer return messages. I also added more/better tests.

Fixing the issue for real this time 💯 % pinky promise, I swear! 🤣

Copy link

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.

Copy link

github-actions bot commented May 23, 2024

You can find the image built from this PR at

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

Built from 9494293

Copy link

github-actions bot commented May 23, 2024

You can find the image built from this PR at

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

Built from 9494293

@gabrielmer
Copy link
Contributor

There's something I don't quite understand. In 3 different tests/files, sending an invalid cursor returns different things.

  • In tests/waku_archive/test_driver_postgres_query.nim ok is returned but with no messages
  • In tests/waku_archive/test_driver_queue_query.nim the error invalid_cursor is returned
  • In tests/waku_archive/test_driver_sqlite_query.nim returns an error with cursor not found

Why the different behavior in those cases? Especially the one that doesn't return an error, I guess that the error message depends on the driver

@SionoiS
Copy link
Contributor Author

SionoiS commented May 23, 2024

There's something I don't quite understand. In 3 different tests/files, sending an invalid cursor returns different things.

  • In tests/waku_archive/test_driver_postgres_query.nim ok is returned but with no messages
  • In tests/waku_archive/test_driver_queue_query.nim the error invalid_cursor is returned
  • In tests/waku_archive/test_driver_sqlite_query.nim returns an error with cursor not found

Why the different behavior in those cases? Especially the one that doesn't return an error, I guess that the error message depends on the driver

Yes that is a good point that will be addressed by my driver refactor. I plan to consolidate driver error handling.

Copy link
Contributor

@gabrielmer gabrielmer left a comment

Choose a reason for hiding this comment

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

Thanks so much!

Copy link
Collaborator

@Ivansete-status Ivansete-status left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for it! 💯

response.data.messages.len == 0

await restServer.stop()
await restServer.closeWait()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Interesting. Did you need that to avoid a possible crash?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure. It's just a copy pasta of another test.

waku/waku_api/rest/store/types.nim Outdated Show resolved Hide resolved
Co-authored-by: Ivan FB <128452529+Ivansete-status@users.noreply.github.com>
@SionoiS SionoiS merged commit a65b13f into master May 27, 2024
9 of 13 checks passed
@SionoiS SionoiS deleted the fix--store-v3-cursor branch May 27, 2024 14:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants