Modify RelatedManager and ManyRelatedManager add() and remove() to accept QuerySets and iterables

[Gabriel Hurley]

I was surprised today to realize that the add() and remove() methods on ManyRelatedManager didn't work when passed a QuerySet. It seemed like they ought to, so I wrote up a patch.

The patch allows the *objs argument to RelatedManager.add(), RelatedManager.remove(), ManyRelatedManager.add(), and ManyRelatedManager.remove() to accept any number of either objects or querysets (of the appropriate model, of course).

The change passes the full test suite, includes new tests, and adds a note in the docs.

I understand if it's too late to make it into 1.2, but it'd be cool if it did. Mark the milestone accordingly.

[Gabriel Hurley]

Bumping to 1.3 milestone.

[modulatrix]

Agree that this should be fixed, but this patch seems incomplete - add() and remove() should accept any arbitrary sequence (e.g. a list or tuple) as well, not just QuerySets.

E.g., in 1.1:

# this works (extra args)
user_instance.groups.add(Group.objects.get(name="Group 1"), Group.objects.get(name="Group 2"))

# this doesn't work (list)
groups_to_add = [Group.objects.get(name="Group 1"), Group.objects.get(name="Group 2")]
user_instance.groups.add(groups_to_add) # Throws TypeError: unhashable type: 'list'

# this doesn't work (tuple)
groups_to_add = (Group.objects.get(name="Group 1"), Group.objects.get(name="Group 2"),)
user_instance.groups.add(groups_to_add) # Throws InterfaceError: Error binding parameter 1 - probably unsupported type

[Gabriel Hurley]

new and improved version that handles iterables (not just querysets)

[Gabriel Hurley]

I completely agree with modulatrix that any iterable that will yield appropriate objects should be accepted. In that spirit I completely re-wrote the patch and it now accepts exactly that.

It's now a much cleaner solution, and in the process it improves the fall-through case and provides a more useful error message when passed arguments that cannot be added.

The patch applies cleanly to trunk and passes the full test suite for me. Feel free to review and provide feedback!

[Paul McMillan]

This looks good to me, and it seems to pass all the tests on my system. The improved error message is definitely a nice touch.

[Alex Gaynor]

FTR this strikes me as a really bad API. There is no compelling reason, IMO, to prefer this form over the *args form, which will work with any iterable, and isn't confusing (randomly flattening 1-level of the *args would confuse me).

[Russell Keith-Magee]

Having a closer look at this when I'm not engaged in a whirlwind of triage, I agree with Alex.

I agree that there should be a way to support the adding/removing the contents of querysets to m2m relations, but making the arguments to add() 'item or iterable' isn't it.

[Alex Gaynor]

Indeed, an API that allowed for fewer queries is a good idea, but just implementing *args doesn't help.

[Gabriel Hurley]

My limiting factor here is backwards compatibility. I don't love this API either, but I don't see a simple way of improving it without breaking existing code. I'm all ears for suggestions.

However, the fact that the current system 1) naively assumes anything passed in that's not an *args of model instances is instead a set of pk's; and 2) that a queryset is not a valid input both seem quite broken to me. Number one there regularly produces confusing and frustrating errors as well.

So please, point me in the right direction. I'd like to see this fixed. But saying "this isn't it" doesn't go anywhere.

[Jacob]

Milestone 1.3 deleted

[Aymeric Augustin]

Change UI/UX from NULL to False.

[Aymeric Augustin]

Change Easy pickings from NULL to False.

[Jimmy Merrild Krag]

What's the status on this one? I am in a situation where this could be handy, but since it is not there, I have to loop.

Discussing the API seems to be what killed this. Would it be better if another function than add handled querysets and iterables?