#4917 - Remove O(n) queries in m2m updates

[orf]

Re: #4917

Lets see if the tests pass.

[tomchristie]

Not looked into this in any detail, but the tests are failing against hyperlinked relationships. Which can also use the pk only optimisation... https://github.com/orf/django-rest-framework/blob/1d990eaba10d542978cbf35d57497858e6f0c3a7/rest_framework/relations.py#L284

[orf]

I removed that check and added an isinstance check for PrimaryKeyRelatedField. I think for this case it's safe to do, and I think that would be a common case. To support hyperlinked relationships it would take a lot of work.

[carltongibson]

Why not use values_list('pk', flat=True) here to just fetch the ids and skip object instantiation?

[orf]

Because we then return the values below (line 507), which need to be full model instances.

[EmreSoner]

If you use set for unique list, why not use distinct on values filter?

[orf]

We are not bothered about distinct values, Model.objects.filter(pk__in=[1, 1]) will only return 1 model AFAIK. We use set() here to just easily find any differences between the returned list of primary keys and the given list of primary keys.

[carltongibson]

I like the idea here but not that it just solves the issue for PrimaryKeyRelatedField.

The if isinstance(self.child_relation, PrimaryKeyRelatedField) special case looks to me like it's queuing up work for later.

to_internal_value is performing two tasks: mapping from the wire representation to lookup parameters and the lookup itself.

In the many case the lookup is always an __in on whatever the lookup field was, so it should be pretty generic.

Even HyperlinkRelatedField has two clearly separate phases: the Map and the lookup

If these were separately callableManyRelatedField should be able to build the lookup (for each RelatedField subclass) for a single fetch, as per the intention.

[orf]

I agree that it's a special case @carltongibson, but doing anything else would be a major breaking change. Third party fields would rely on to_internal_value returning a whole 'thing' and changing that may be a fair bit of work.

Perhaps we could add another method that converts from the wire API and returns something we can use as a filter (perhaps a Q object would be best?) and have the base to_internal_value call that and do the .get() to maintain compatibility. But that wouldn't help with third party fields anyway, so... I don't know.

Again, queuing up work isn't a bad thing unless nobody wants to do the work. This MR doesn't solve it for the general case, but it solves it for the a common case. I think that's worthwhile until a better solution is implemented.

And its backwards compatible 👍

[carltongibson]

@orf I'm not suggesting we break to_internal_value.

If it were refactored to look a bit like this:

def to_internal_value(...):
   # get lookup
   # use that to get object

... then ManyRelatedField could do use those methods to do a little bit of magic such as yours to avoid the O(n) separate lookups.

[orf]

oh right, so perhaps a to_lookup function that returns a Q object perhaps? Then we can do a hasattr(child, 'to_lookup') and use that if present? I like that idea.

[carltongibson]

@orf I added a Proof of Concept for HyperlinkedRelatedField in #5150. (I've just added code to HyperlinkedRelatedField there, obviously it would need refactoring properly but it should communicate the idea.)

[carltongibson]

Closing as a known limitation in line with #5150. (Full featured PRs welcomed.)