Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Re-prefetch related objects after updating#8043

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
auvipy merged 5 commits intoencode:masterfromyuekui:master
Jan 11, 2023
Merged

Conversation

@yuekui
Copy link
Contributor

Instead of re-loadingget_object() or not using any prefetch after updating, we could keep the instance and re-prefetch related objects for the updated instance.

refs:
#4553
#4668
#4661

@stale
Copy link

stalebot commentedApr 24, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stalestalebot added the stale labelApr 24, 2022
@dr-ftvkun
Copy link

dr-ftvkun commentedJun 3, 2022
edited
Loading

TLDR; this PR seems to be a good compromise between re-invokingget_object() and full (buggy, see below) invalidation of prefetched objects' cache during updates.@kevin-brown is it going to be merged?

So, about the bug.

Let we have a viewset with the following get_queryset():

defget_queryset(self):returnEntity.objects.prefetch_related(Prefetch("related_objects",queryset=RelatedObject.objects.filter(is_removed=False).order_by("order")        )    )

Expected:

  • basically there shouldn't be differences between result of PATCH/PUT and repeated GET.
    In particular, GET /enitities/{pk}/ and PATCH /entities/{pk}/ {} should return the same object.

Actual:

  • if we use GET (list or retrieve), the prefetched objects will be in expected order and state. But if we use PATCH/PUT, the UpdateModelMixin's invalidation breaks both things and uses justinstance.related_objects.all() from scratch which leads to appear unexpected objects (those having is_removed=True) in api response in an unpredictable order.

So in general I think just re-prefetching them would do the right job.

fjsj and aj4ayushjain reacted with thumbs up emoji

@lovelydinosaur
Copy link
Contributor

Okay. I can't say that I know those bits of private implementation, but I'm okay with this.

Marking it as a priority, so I can verify to myself that the change does fix the behaviour as described above. Once that's confirmed I think we should be okay to merge this.

dr-ftvkun reacted with thumbs up emoji

@stale
Copy link

stalebot commentedAug 12, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@fjsj
Copy link

As this is buggy in current main/master branch, as explained by@dr-ftvkun above, I also think this should be merged.

@stale
Copy link

stalebot commentedOct 29, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stalestalebot added the stale labelOct 29, 2022
@stalestalebot closed thisNov 23, 2022
@yuekui
Copy link
ContributorAuthor

Can we re-open it?

dr-ftvkun and richardmarktl reacted with thumbs up emoji

@dr-ftvkun
Copy link

@tomchristie could you please re-open it?

Hoping one day someone (mb me?) will have enough energy to push it to become a part of DRF or be rejected by a good reason (not just because it got stale).

yuekui reacted with thumbs up emoji

@auvipyauvipy reopened thisNov 23, 2022
@stalestalebot removed the stale labelNov 23, 2022
@auvipy
Copy link
Collaborator

Hey good people, I'm a new maintainer here. you can ping me anytime for reviewing and insights for direction. if we are unsure about anything we then ping him for final call. I have already reviewed and merged some old pr. so in coming weeks this project will have more active maintainers available.

dr-ftvkun and yuekui reacted with rocket emoji

@dr-ftvkun
Copy link

Hey good people, I'm a new maintainer here. you can ping me anytime for reviewing and insights for direction. if we are unsure about anything we then ping him for final call. I have already reviewed and merged some old pr. so in coming weeks this project will have more active maintainers available.

Hey@auvipy! Great news!

I think this PR at least needs tests for the cases i providedabove.

But still, could you also look at it to check the general idea?

@auvipy
Copy link
Collaborator

I will review this thoroughly

@auvipyauvipy self-requested a reviewNovember 23, 2022 13:49
@yuekui
Copy link
ContributorAuthor

Hey good people, I'm a new maintainer here. you can ping me anytime for reviewing and insights for direction. if we are unsure about anything we then ping him for final call. I have already reviewed and merged some old pr. so in coming weeks this project will have more active maintainers available.

Hey@auvipy! Great news!

I think this PR at least needs tests for the cases i providedabove.

But still, could you also look at it to check the general idea?

Thanks@dr-ftvkun , I'll add tests for that.

Copy link
Collaborator

@auvipyauvipy left a comment

Choose a reason for hiding this comment

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

I like the general idea, but I want to validate myself more looking into the django pvt stuffs. that may be first of next week

@yuekuiyuekui requested a review fromauvipyDecember 2, 2022 22:47
@yuekui
Copy link
ContributorAuthor

Hi@auvipy please let me know if there's any improvement we could do for this PR.

@auvipy
Copy link
Collaborator

we need to verify the usage of django internals here. so need little bit more time then expected. please allow us some time to get back to you for this.

yuekui and dr-ftvkun reacted with thumbs up emojiyuekui and dr-ftvkun reacted with heart emoji

@auvipyauvipy added this to the3.15 milestoneJan 11, 2023
@auvipy
Copy link
Collaborator

we got a regression report, also a possible fix, can you check?#9314

@yuekui
Copy link
ContributorAuthor

we got a regression report, also a possible fix, can you check?#9314

Yeah that should fix the case that if only get_object() was defined, it looks good to me.

auvipy added a commit that referenced this pull requestMar 21, 2024
lovelydinosaur pushed a commit that referenced this pull requestMar 21, 2024
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@auvipyauvipyauvipy approved these changes

+1 more reviewer

@kevin-brownkevin-brownkevin-brown left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

3.15

Development

Successfully merging this pull request may close these issues.

6 participants

@yuekui@dr-ftvkun@lovelydinosaur@fjsj@auvipy@kevin-brown

[8]ページ先頭

©2009-2025 Movatter.jp