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

Added support to fetch latest object from mongoengine queryset #2757

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

Conversation

ArpitSachan
Copy link

Solving: #2756

@gowthamvbhat
Copy link

How come you are approving your own PRs?

@idoshr
Copy link
Contributor

idoshr commented Sep 21, 2023

I'm not sure that it will work good in case of sort that already created on the queryset
recommended to add tester to that case

@bagerard
Copy link
Collaborator

I'm not sure about the use case for such method but given that it's present in django, it's something that I'm ok to consider.

That being said, I see multiple issues with this proposal

  1. in case no order_by is present, existing first method will rely on no-sorting (i.e natural sorting). If last would rely on primary key sorting silently, it would mean that you could get inconsistencies (e.g get the same record) if you use first() and last().

  2. if an existing sorting is specified on the queryset, it should be re-used and not overwritten by the primary key ordering.

Fixing 2 can be easily done in this PR but if we want to tackle 1) correctly, we would have to add the same default ordering on first which I'm not necessarily keen to do...

What would be acceptable (and less disruptive) to do is the following:

  • add last but solely rely on the existing order_by (no letting it take any argument)
  • if no order_by is set on the queryset, raise an error to avoid any inconsistent behavior

@ArpitSachan
Copy link
Author

@bagerard Thanks for suggestions. Have handled the comments.

Update tests

Handle comment to address the incosistent behaviour
@ArpitSachan ArpitSachan force-pushed the update/add_last_method_on_queryset_cls branch from a75ac3f to c9fd9ff Compare August 22, 2024 15:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants