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: first_or_none no longer mutates FindMany object (#1116) #1117

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

eac0de
Copy link

@eac0de eac0de commented Jan 31, 2025

Fixes #1116

Copy link
Contributor

@staticxterm staticxterm left a comment

Choose a reason for hiding this comment

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

Thank you for creating the PR. Just one small comment, please if you can use your original proposal to retain the "limit".

if not res:
return None
return res[0]
result = await self.to_list(1)
Copy link
Contributor

Choose a reason for hiding this comment

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

While the performance impact may be negligible, I would prefer if the query would be as efficient as possible, so that Mongo only has to return 1 result.
Please write this the old way (use your initial proposal), using self.limit(1).to_list() and resetting self.limit_number to the previous value after result is retrieved.

Copy link
Author

@eac0de eac0de Feb 1, 2025

Choose a reason for hiding this comment

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

Sorry, I didn't realise that motor returns the whole list when to_list(1) is called and just truncates it. I've looked into it now and saw that. Thanks, I made an amend to follow the ‘one commit’ rule.

@staticxterm staticxterm requested a review from a team January 31, 2025 21:34
@eac0de eac0de force-pushed the fix-first-or-none branch from de92754 to cd36684 Compare February 1, 2025 07:20
Copy link
Contributor

@staticxterm staticxterm left a comment

Choose a reason for hiding this comment

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

Looks good. Thank you!

@staticxterm staticxterm requested a review from a team February 1, 2025 20:42
Copy link
Contributor

@CAPITAINMARVEL CAPITAINMARVEL left a comment

Choose a reason for hiding this comment

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

LGTM

@staticxterm staticxterm requested a review from a team February 2, 2025 14:40
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.

[BUG] After first_or_none of class FindMany, limit_number == 1 remains
3 participants