Opened 15 years ago
Last modified 8 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)
Change History (19)
by , 15 years ago
Attachment: | add_remove_querysets.diff added |
---|
comment:1 by , 15 years ago
Triage Stage: | Unreviewed → Ready for checkin |
---|
comment:2 by , 14 years ago
milestone: | → 1.3 |
---|---|
Status: | new → assigned |
comment:3 by , 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 , 14 years ago
Attachment: | add_remove_querysets_v2.diff added |
---|
new and improved version that handles iterables (not just querysets)
comment:4 by , 14 years ago
Patch needs improvement: | unset |
---|---|
Summary: | Modify RelatedManager and ManyRelatedManager add() and remove() to accept QuerySets → Modify RelatedManager and ManyRelatedManager add() and remove() to accept QuerySets and iterables |
Triage Stage: | Ready for checkin → 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 by , 14 years ago
Triage Stage: | Accepted → 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 by , 14 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 , 14 years ago
Patch needs improvement: | set |
---|---|
Triage Stage: | Ready for checkin → 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 by , 14 years ago
Indeed, an API that allowed for fewer queries is a good idea, but just implementing *args
doesn't help.
comment:9 by , 14 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 , 14 years ago
Type: | → New feature |
---|
comment:11 by , 14 years ago
Severity: | → Normal |
---|
comment:13 by , 10 years ago
Cc: | added |
---|
comment:14 by , 8 years ago
Owner: | removed |
---|---|
Status: | assigned → new |
comment:15 by , 8 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?
Bumping to 1.3 milestone.