Opened 7 years ago

Last modified 2 months 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: master
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 7 years ago.
add_remove_querysets_v2.diff (7.1 KB) - added by Gabriel Hurley 6 years ago.
new and improved version that handles iterables (not just querysets)

Download all attachments as: .zip

Change History (18)

Changed 7 years ago by Gabriel Hurley

Attachment: add_remove_querysets.diff added

comment:1 Changed 7 years ago by Russell Keith-Magee

Triage Stage: UnreviewedReady for checkin

comment:2 Changed 6 years ago by Gabriel Hurley

milestone: 1.3
Status: newassigned

Bumping to 1.3 milestone.

comment:3 Changed 6 years ago by modulatrix

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

Changed 6 years ago by Gabriel Hurley

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

comment:4 Changed 6 years ago by Gabriel Hurley

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 Changed 6 years ago by Paul McMillan

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 Changed 6 years ago by 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).

comment:7 Changed 6 years ago by Russell Keith-Magee

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 Changed 6 years ago by Alex Gaynor

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

comment:9 Changed 6 years ago by 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.

comment:10 Changed 6 years ago by Luke Plant

Type: New feature

comment:11 Changed 6 years ago by Luke Plant

Severity: Normal

comment:12 Changed 5 years ago by Jacob

milestone: 1.3

Milestone 1.3 deleted

comment:11 Changed 5 years ago by Aymeric Augustin

UI/UX: unset

Change UI/UX from NULL to False.

comment:12 Changed 5 years ago by Aymeric Augustin

Easy pickings: unset

Change Easy pickings from NULL to False.

comment:13 Changed 22 months ago by Collin Anderson

Cc: cmawebsite@… added

comment:14 Changed 2 months ago by Tim Graham

Owner: Gabriel Hurley deleted
Status: assignednew
Note: See TracTickets for help on using tickets.
Back to Top