Re-prefetch related objects after updating

[yuekui]

Instead of re-loading get_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]

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.

[dr-ftvkun]

TLDR; this PR seems to be a good compromise between re-invoking get_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():

def get_queryset(self):
     return Entity.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 just instance.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.

[tomchristie]

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.

[stale]

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]

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

[stale]

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.

[yuekui]

Can we re-open it?

[dr-ftvkun]

@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).

[auvipy]

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]

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 provided above.

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

[auvipy]

I will review this thoroughly

[yuekui]

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 provided above.

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

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

[auvipy]

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

[yuekui]

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

[auvipy]

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.

[auvipy]

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

[yuekui]

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.