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

test(listconnectionwithtotalcount): check the number of SQL queries when only fetching totalCount #525

Conversation

euriostigue
Copy link
Contributor

@euriostigue euriostigue commented Apr 30, 2024

The ListConnectionWithTotalCount always requires 2 queries, even if the edges or pageInfo aren't queried. Ideally, this only requires 1 SQL query.

Description

The required fix will be in the strawberry library. I will be submitting a PR for that shortly. I have added tests here that will pass after those changes have merged and this library uses the latest release.

Types of Changes

  • Core
  • Bugfix
  • New feature
  • Enhancement/optimization
  • Documentation

Issues Fixed or Closed by This PR

#397

Checklist

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • I have tested the changes and verified that they work and don't break anything (as well as I can manage).

Copy link
Contributor

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

Hey @euriostigue - I've reviewed your changes and they look great!

Here's what I looked at during the review
  • 🟢 General issues: all looks good
  • 🟢 Security: all looks good
  • 🟢 Testing: all looks good
  • 🟢 Complexity: all looks good

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.

@bellini666
Copy link
Member

Hey @euriostigue ,

Yes, you got a point. We won't trigger an extra query for totalCount when only retrieving edges, but the opposite does happen.

Please mark me as a reviewer on strawberry's PR and I'll try to review it ASAP. Let me know if you need help with anything :)

@bellini666
Copy link
Member

Hey @euriostigue ,

Yes, you got a point. We won't trigger an extra query for totalCount when only retrieving edges, but the opposite does happen.

Please mark me as a reviewer on strawberry's PR and I'll try to review it ASAP. Let me know if you need help with anything :)

nvm my comment, already reviewed it in strawberry-graphql/strawberry#3480

@euriostigue euriostigue force-pushed the 397-test-list-connection-total-count-sql-queries branch from dd54954 to 7d6dd33 Compare May 9, 2024 14:54
@euriostigue
Copy link
Contributor Author

@bellini666 I upgraded the strawberry version and the added test passes now

@bellini666
Copy link
Member

@bellini666 I upgraded the strawberry version and the added test passes now

Nice, thank you! 😊

@bellini666 bellini666 merged commit f493237 into strawberry-graphql:main May 9, 2024
50 checks passed
@euriostigue euriostigue deleted the 397-test-list-connection-total-count-sql-queries branch May 9, 2024 16:51
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.

2 participants