Code

Opened 4 years ago

Last modified 3 years ago

#13240 assigned New feature

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

Reported by: gabrielhurley Owned by: gabrielhurley
Component: Database layer (models, ORM) Version: master
Severity: Normal Keywords:
Cc: 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 gabrielhurley 4 years ago.
add_remove_querysets_v2.diff (7.1 KB) - added by gabrielhurley 4 years ago.
new and improved version that handles iterables (not just querysets)

Download all attachments as: .zip

Change History (16)

Changed 4 years ago by gabrielhurley

comment:1 Changed 4 years ago by russellm

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Ready for checkin

comment:2 Changed 4 years ago by gabrielhurley

  • milestone set to 1.3
  • Status changed from new to assigned

Bumping to 1.3 milestone.

comment:3 Changed 4 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 4 years ago by gabrielhurley

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

comment:4 Changed 4 years ago by gabrielhurley

  • Patch needs improvement unset
  • Summary changed from Modify RelatedManager and ManyRelatedManager add() and remove() to accept QuerySets to Modify RelatedManager and ManyRelatedManager add() and remove() to accept QuerySets and iterables
  • Triage Stage changed from Ready for checkin to Accepted

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 4 years ago by PaulM

  • Triage Stage changed from Accepted to Ready 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 4 years ago by Alex

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 4 years ago by russellm

  • Patch needs improvement set
  • Triage Stage changed from Ready for checkin to Accepted

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 4 years ago by Alex

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

comment:9 Changed 4 years ago by gabrielhurley

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 3 years ago by lukeplant

  • Type set to New feature

comment:11 Changed 3 years ago by lukeplant

  • Severity set to Normal

comment:12 Changed 3 years ago by jacob

  • milestone 1.3 deleted

Milestone 1.3 deleted

comment:11 Changed 2 years ago by aaugustin

  • UI/UX unset

Change UI/UX from NULL to False.

comment:12 Changed 2 years ago by aaugustin

  • Easy pickings unset

Change Easy pickings from NULL to False.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as assigned
The owner will be changed from gabrielhurley to anonymous. Next status will be 'assigned'
The ticket will be disowned. Next status will be 'new'
as The resolution will be set. Next status will be 'closed'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.