Opened 14 years ago

Last modified 7 years ago

#13240 new New feature

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

Reported by: Gabriel Hurley Owned by:
Component: Database layer (models, ORM) Version: dev
Severity: Normal Keywords:
Cc: cmawebsite@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

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.

Attachments (2)

add_remove_querysets.diff (5.8 KB ) - added by Gabriel Hurley 14 years ago.
add_remove_querysets_v2.diff (7.1 KB ) - added by Gabriel Hurley 13 years ago.
new and improved version that handles iterables (not just querysets)

Download all attachments as: .zip

Change History (19)

by Gabriel Hurley, 14 years ago

Attachment: add_remove_querysets.diff added

comment:1 by Russell Keith-Magee, 14 years ago

Triage Stage: UnreviewedReady for checkin

comment:2 by Gabriel Hurley, 14 years ago

milestone: 1.3
Status: newassigned

Bumping to 1.3 milestone.

comment:3 by modulatrix, 14 years ago

Patch needs improvement: set

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

by Gabriel Hurley, 13 years ago

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

comment:4 by Gabriel Hurley, 13 years ago

Patch needs improvement: unset
Summary: Modify RelatedManager and ManyRelatedManager add() and remove() to accept QuerySetsModify RelatedManager and ManyRelatedManager add() and remove() to accept QuerySets and iterables
Triage Stage: Ready for checkinAccepted

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!

comment:5 by Paul McMillan, 13 years ago

Triage Stage: AcceptedReady for checkin

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.

comment:6 by Alex Gaynor, 13 years ago

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

comment:7 by Russell Keith-Magee, 13 years ago

Patch needs improvement: set
Triage Stage: Ready for checkinAccepted

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.

comment:8 by Alex Gaynor, 13 years ago

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

comment:9 by Gabriel Hurley, 13 years ago

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.

comment:10 by Luke Plant, 13 years ago

Type: New feature

comment:11 by Luke Plant, 13 years ago

Severity: Normal

comment:12 by Jacob, 12 years ago

milestone: 1.3

Milestone 1.3 deleted

comment:11 by Aymeric Augustin, 12 years ago

UI/UX: unset

Change UI/UX from NULL to False.

comment:12 by Aymeric Augustin, 12 years ago

Easy pickings: unset

Change Easy pickings from NULL to False.

comment:13 by Collin Anderson, 9 years ago

Cc: cmawebsite@… added

comment:14 by Tim Graham, 7 years ago

Owner: Gabriel Hurley removed
Status: assignednew

comment:15 by Jimmy Merrild Krag, 7 years ago

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?

Note: See TracTickets for help on using tickets.
Back to Top