Opened 17 years ago

Closed 6 years ago

#3006 closed New feature (wontfix)

generic relations do not act as expected in a filter/get

Reported by: jeroen_at_lbvd.nl Owned by: Robert Myers
Component: Database layer (models, ORM) Version: dev
Severity: Normal Keywords: generic relations
Cc: tom@…, fridrik@… Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

If I have a model like this:

class SecurityContext(models.Model):
    title = models.CharField(maxlength=100)
    description = models.TextField()

class AccessList(models.Model):
    principal_id = models.PositiveIntegerField()
    principal_ct = models.ForeignKey(ContentType)

    security_context = models.ForeignKey(SecurityContext)
    principal = models.GenericForeignKey(fk_field="principal_id", ct_field="principal_ct")
    permission = models.ForeignKey(Permission)
    class Admin:
        pass

class MyModel(models.Model):
    security_context = models.ForeignKey(SecurityContext)
    owner = models.ForeignKey(User)

    my_field = models.CharField(maxlength=50)

    class Admin:
        pass


Then I expect to be able to do something like:

my_model_instance.security_context.accesslist_set.filter(principal=user_object)

and have it return a list of all accesslist entries for the given principal (similar to ForeignKeys or M2M relations).
Instead I get an error stating: Cannot resolve keyword 'principal' into field.

The following does work however:

my_model_instance.security_context.accesslist_set.filter(principal_id=user_object.id)

Attachments (1)

3006.patch (638 bytes ) - added by Robert Myers 16 years ago.
documentation patch added

Download all attachments as: .zip

Change History (25)

comment:1 by anonymous, 16 years ago

If nothing can be done to fix this, the fact that it cannot be done should at least be documented.

comment:2 by Jacob, 16 years ago

Triage Stage: UnreviewedAccepted
Version: SVN

comment:3 by Thomas Steinacher <tom@…>, 16 years ago

Cc: tom@… added

comment:4 by mrts, 16 years ago

milestone: 1.0

In scope for 1.0.

comment:5 by Alex Gaynor, 16 years ago

Keywords: generic relations added
Version: SVN

comment:6 by Robert Myers, 16 years ago

Owner: changed from nobody to Robert Myers

comment:7 by Robert Myers, 16 years ago

I believe this functionality will not be easy to create. Basically a generic foreign key is not a field, so it can not be used in a the ORM like normal fields. It would need to be coverted to a field subclass and all the methods that work with the db will need to be overridden. I question whether it would be a good idea to make this field overly complicated just to get this functionality.

by Robert Myers, 16 years ago

Attachment: 3006.patch added

documentation patch added

comment:8 by Robert Myers, 16 years ago

Has patch: set

comment:9 by Malcolm Tredinnick, 16 years ago

milestone: 1.0post-1.0
Triage Stage: AcceptedSomeday/Maybe

It's probably possible to do this eventually, although it needs #5937 to be fixed first. Agree that it's fiddly, though. Marking for post-1.0. It's never been reasonable to expect this to work. We can make this someday/maybe in case somebody comes up with a good implementation in the future.

I'll drop in your docs patch in a moment to clarify things for 1.0. Thanks for doing the investigative work on this one.

comment:10 by Malcolm Tredinnick, 16 years ago

(In [8417]) Documented that GenericForeignKey fields can't be used transparently in
filters. Refs #3006. Patch from rmyers.

comment:11 by (none), 15 years ago

milestone: post-1.0

Milestone post-1.0 deleted

comment:12 by anonymous, 15 years ago

Cc: fridrik@… added

comment:13 by Łukasz Rekucki, 13 years ago

Severity: normalNormal
Type: defectBug

comment:14 by anonymous, 13 years ago

Type: BugNew feature

comment:15 by Aymeric Augustin, 12 years ago

UI/UX: unset

Change UI/UX from NULL to False.

comment:16 by Aymeric Augustin, 12 years ago

Easy pickings: unset

Change Easy pickings from NULL to False.

comment:17 by coleifer, 12 years ago

I'd like to work up a patch to merge in some of the functionality i've written for dealing with GFKs -- including filter, annotate and aggregate:

https://github.com/coleifer/django-generic-aggregation/blob/master/generic_aggregation/utils.py

in reply to:  17 comment:18 by Asif Saifuddin Auvi, 8 years ago

Replying to coleifer:

I'd like to work up a patch to merge in some of the functionality i've written for dealing with GFKs -- including filter, annotate and aggregate:

https://github.com/coleifer/django-generic-aggregation/blob/master/generic_aggregation/utils.py

go for it if you are still interested!!!

comment:19 by Karolis Ryselis, 6 years ago

I have created a pull request for this at https://github.com/django/django/pull/9748. This special-cases generic relations when filters are added to query. It only works for simple lookups as nested lookups make no sense, i.e.,

TaggedItem.objects.filter(content_object=obj)

works and

TaggedItem.objects.filter(content_object__name="name")

will be caught by the old checks


comment:20 by Asif Saifuddin Auvi, 6 years ago

Triage Stage: Someday/MaybeAccepted

can we conditionally accept this to move forward?

comment:21 by Carlton Gibson, 6 years ago

Patch needs improvement: set

comment:22 by Carlton Gibson, 6 years ago

Triage Stage: AcceptedReady for checkin

Patch looks good. Not 100% sure it's needed given GenericRelation is already available. (Alternative is to close as wontfix.)

Last edited 6 years ago by Carlton Gibson (previous) (diff)

comment:23 by Tim Graham, 6 years ago

I'd lean toward a wontfix. In particular, making changes in django/db/models/sql/query.py for a contrib app doesn't look like a good separation of concerns. Is there a downside to the alternative of adding the GenericRelation?

comment:24 by Tim Graham, 6 years ago

Resolution: wontfix
Status: newclosed

Feel free to continue the discussion on django-developers if there's some motivation for this.

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