Opened 13 years ago

Closed 11 months ago

Last modified 11 months ago

#16055 closed Bug (fixed)

Filtering over generic relations with TextField/CharField object_id breaks in postgres

Reported by: anonymous Owned by: David Wobrock
Component: contrib.contenttypes Version: 3.2
Severity: Normal Keywords:
Cc: victor.van.den.elzen@…, joeri@…, mmitar@…, brian@…, Kye Russell, Kevin Wiliarty, Marc DEBUREAUX, Sage Abdullah, David Wobrock Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

When you have a generic foreign key with TextField / CharField object_id, and try to use it in a filter with a model that has an integer primary key, PostgreSQL 9.0.4 errors out with this:

DatabaseError: operator does not exist: integer = text

HINT:  No operator matches the given name and argument type(s). You might need to add explicit type casts.

A small example:

class Tag(models.Model):
    tag = models.SlugField()
    content_type = models.ForeignKey(ContentType)
    object_id = models.TextField()
    content_object = generic.GenericForeignKey()

class Animal(models.Model):
    name = models.TextField()
    tags = generic.GenericRelation(Tag)

print Animal.objects.filter(tags__tag='mammal')

I have a patch that changes Django's generic foreign key tests to also try everything with a textual object_id, which exhibits this problem.

A workaround is to create this cast as implicit in postgres:

CREATE CAST (integer AS text) WITH INOUT AS IMPLICIT

Attachments (3)

django_generic_text_id_test.diff (10.4 KB ) - added by victor.van.den.elzen@… 13 years ago.
patch for tests
16055_tests.diff (2.2 KB ) - added by Ramiro Morales 13 years ago.
Minimal patch for current existing tests
16055-test.diff (1.7 KB ) - added by Tim Graham 8 years ago.

Download all attachments as: .zip

Change History (31)

by victor.van.den.elzen@…, 13 years ago

patch for tests

comment:1 by victor.van.den.elzen@…, 13 years ago

Cc: victor.van.den.elzen@… added

comment:2 by joeri@…, 13 years ago

Cc: joeri@… added

in reply to:  description comment:3 by Ramiro Morales, 13 years ago

Triage Stage: UnreviewedAccepted

Replying to anonymous:

When you have a generic foreign key with TextField / CharField object_id, and try to use it in a filter with a model that has an integer primary key, PostgreSQL 9.0.4 errors out with this:

DatabaseError: operator does not exist: integer = text

HINT:  No operator matches the given name and argument type(s). You might need to add explicit type casts.

A small example:

I've verified this specific test case by modifying tests already present in the test suite (added in [12353]). See the attached patch.

I see no failure with Postgres 8.3 (the version in which cast behavior changes were introduced). Can you please test it with Postgres 9 and post the results?

by Ramiro Morales, 13 years ago

Attachment: 16055_tests.diff added

Minimal patch for current existing tests

comment:4 by Aymeric Augustin, 12 years ago

UI/UX: unset

Change UI/UX from NULL to False.

comment:5 by Tim Graham, 8 years ago

Component: Database layer (models, ORM)contrib.contenttypes
Summary: Filtering over funky generic relations breaks in postgresFiltering over generic relations with TextField/CharField object_id breaks in postgres

I updated the tests to apply cleanly to master and verified the failure on PostgreSQL 9.4 at 58c7ff39fb265754fb17ab8d7f8a1401b355777b (Django 1.10.dev).

======================================================================
ERROR: test_charlink_filter (generic_relations_regress.tests.GenericRelationTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/tim/code/django/tests/generic_relations_regress/tests.py", line 57, in test_charlink_filter
    list(OddRelation1.objects.filter(clinks__title='title'))
  File "/home/tim/code/django/django/db/models/query.py", line 258, in __iter__
    self._fetch_all()
  File "/home/tim/code/django/django/db/models/query.py", line 1074, in _fetch_all
    self._result_cache = list(self.iterator())
  File "/home/tim/code/django/django/db/models/query.py", line 52, in __iter__
    results = compiler.execute_sql()
  File "/home/tim/code/django/django/db/models/sql/compiler.py", line 839, in execute_sql
    cursor.execute(sql, params)
  File "/home/tim/code/django/django/db/backends/utils.py", line 64, in execute
    return self.cursor.execute(sql, params)
  File "/home/tim/code/django/django/db/utils.py", line 92, in __exit__
    six.reraise(dj_exc_type, dj_exc_value, traceback)
  File "/home/tim/code/django/django/db/backends/utils.py", line 64, in execute
    return self.cursor.execute(sql, params)
ProgrammingError: operator does not exist: integer = character varying
LINE 1: ...ON ("generic_relations_regress_oddrelation1"."id" = "generic...
                                                             ^
HINT:  No operator matches the given name and argument type(s). You might need to add explicit type casts.

by Tim Graham, 8 years ago

Attachment: 16055-test.diff added

comment:6 by Tim Graham, 8 years ago

#20271 is a duplicate with an alternate test.

comment:7 by Mitar, 8 years ago

Cc: mmitar@… added

comment:8 by Brian May, 8 years ago

Cc: brian@… added

comment:9 by Kye Russell, 6 years ago

Cc: Kye Russell added

comment:10 by Kevin Wiliarty, 4 years ago

Cc: Kevin Wiliarty added

comment:11 by Simon Charette, 4 years ago

If anyone is interested in addressing this issue it should be doable by adjusting Join.as_sql to add the appropriate cast when lhs_col and rhs_col's .db_type(connection) doesn't match.

https://github.com/django/django/blob/129583a0d3cf69b08d058cd751d777588801b7ad/django/db/models/sql/datastructures.py#L70-L77

Ideally most of the Cast logic would be moved to the database backend operations so Join.as_sql can rely on it instead of having to depend on db.models stuff.

A less invasive solution would be to make GenericRelation's join_field return an empty get_joining_columns and include the adapted condition in get_extra_restriction instead but that seems like this should be solved at the Join level instead since we don't offer a low level way of completely configuring join conditions; we only allow to augment them using FilteredRelation.

comment:12 by Nekmo, 4 years ago

I have made a hack for Django-guardian. In my case, this query is the one that fails:

from customers.models import CustomerMemberObjectPermission

CustomerMemberObjectPermission.objects.filter(service_api__service__service_name='foo')  # Raise DatabaseError

My models:

class CustomerMemberObjectPermission(UserObjectPermission):
    objects = CustomerMemberObjectPermissionManager()


class ServiceApi(Model):
    customer_permissions = GenericRelation(CustomerMemberObjectPermission, object_id_field='object_pk',
                                           related_query_name='service_api')

This is the wrong sql:

SELECT "guardian_userobjectpermission"."id", "guardian_userobjectpermission"."permission_id", "guardian_userobjectpermission"."content_type_id", "guardian_userobjectpermission"."object_pk", "guardian_userobjectpermission"."user_id" FROM "guardian_userobjectpermission" INNER JOIN "services_serviceapi" ON ("guardian_userobjectpermission"."object_pk" = "services_serviceapi"."id" AND ("guardian_userobjectpermission"."content_type_id" = 22)) INNER JOIN "services_service" ON ("services_serviceapi"."service_id" = "services_service"."id") INNER JOIN "customers_customer" ON ("services_service"."customer_id" = "customers_customer"."id") WHERE "customers_customer"."customer_code" = BCL

The error is in the inner join. Django-guardian uses a field of type string for object_pk. But my id is a integer type:

"guardian_userobjectpermission"."object_pk" = "services_serviceapi"."id"

The solution is to use a cast:

CAST("guardian_userobjectpermission"."object_pk" AS integer) =  "services_serviceapi"."id"

Although I know it is not the best, my solution has been this hack thanks to Simon Charette:

class CustomJoin(Join):
    def as_sql(self, compiler, connection):
        sql, params = super(CustomJoin, self).as_sql(compiler, connection)
        sql = sql.replace('"guardian_userobjectpermission"."object_pk"',
                          'CAST("guardian_userobjectpermission"."object_pk" AS integer)')
        return sql, params


class CustomSQLCompiler(SQLCompiler):
    def compile(self, node, select_format=False):
        if isinstance(node, Join):
            node = CustomJoin(
                node.table_name, node.parent_alias, node.table_alias, node.join_type,
                node.join_field, node.nullable, node.filtered_relation
            )
        return super(CustomSQLCompiler, self).compile(node, select_format)


class CustomQuery(Query):
    def get_compiler(self, using=None, connection=None):
        original_compiler = super(CustomQuery, self).get_compiler(using=using, connection=connection)
        return CustomSQLCompiler(original_compiler.query, original_compiler.connection, original_compiler.using)
        # return super(CustomQuery, self).get_compiler(using=using, connection=connection)


class CustomerMemberObjectPermissionQuerySet(CustomerQuerySet):
    def __init__(self, model=None, query=None, using=None, hints=None):
        if not query:
            query = CustomQuery(model)
        super(CustomerMemberObjectPermissionQuerySet, self).__init__(
            model=model, query=query, using=using, hints=hints
        )


class CustomerMemberObjectPermissionManager(CustomerManager, UserObjectPermissionManager):
    def get_queryset(self):
        return CustomerMemberObjectPermissionQuerySet(self.model, using=self._db)

comment:13 by Simon Charette, 3 years ago

Note that another elegant solution to this problem could be to add support to transforms to ForeignObject.to_fields and .from_fields so you could do something along these lines

class Animal(models.Model):
    name = models.TextField()
    tags = generic.GenericRelation(Tag, from_field='id__text')

(assuming we'd register a __text lookup for IntegerField)

comment:14 by Marc DEBUREAUX, 3 years ago

Manage to create something based on Simon Charette's suggestion.

Creating a custom GenericRelation with the following:

  • get_joining_columns returning an empty tuple
  • get_extra_restriction building the whole where clause with content type AND primary key join with Cast

Here's the result:

class CustomGenericRelation(GenericRelation):
    def get_joining_columns(self, reverse_join=False):
        return ()

    def get_extra_restriction(self, where_class, alias, remote_alias):
        cond = super().get_extra_restriction(where_class, alias, remote_alias)
        from_field = self.model._meta.pk
        to_field = self.remote_field.model._meta.get_field(self.object_id_field_name)
        lookup = from_field.get_lookup('exact')(
            Cast(from_field.get_col(alias), output_field=models.TextField()),
            to_field.get_col(remote_alias))
        cond.add(lookup, 'AND')
        return cond

Giving the following:

  • a model common.History having a GenericForeignKey with object_id as text
  • a model generic.Twitter having a CustomGenericRelation and a numeric primary key
  • and the queryset:
q = Twitter.objects.values('id').filter(id=1).annotate(count=Count('histories'))
print(q.query)

It gives the following SQL query:

SELECT 
    "generic_twitter"."id", 
    COUNT("common_history"."id") AS "count" 
FROM "generic_twitter" 
LEFT OUTER JOIN "common_history" ON 
(((
    "common_history"."content_type_id" = 29 AND 
    CAST("generic_twitter"."id" AS text) = "common_history"."object_id"
))) 
WHERE "generic_twitter"."id" = 1 
GROUP BY "generic_twitter"."id"

Instead of this one (normal behaviour):

SELECT 
    "generic_twitter"."id", 
    COUNT("common_history"."id") AS "count" 
FROM "generic_twitter" 
LEFT OUTER JOIN "common_history" ON 
(
    "generic_twitter"."id" = "common_history"."object_id" AND 
    ("common_history"."content_type_id" = 29)
) 
WHERE "generic_twitter"."id" = 1 
GROUP BY "generic_twitter"."id"
Last edited 3 years ago by Marc DEBUREAUX (previous) (diff)

comment:15 by Marc DEBUREAUX, 3 years ago

Cc: Marc DEBUREAUX added
Version: 1.33.2

Change Django version number for visibility purposes.

comment:16 by Sage Abdullah, 15 months ago

Cc: Sage Abdullah added

comment:17 by David Wobrock, 13 months ago

Cc: David Wobrock added
Has patch: set
Owner: changed from nobody to David Wobrock
Status: newassigned

comment:18 by Mariusz Felisiak, 13 months ago

Patch needs improvement: set

Per Simon's comments.

comment:19 by David Wobrock, 13 months ago

Patch needs improvement: unset

comment:20 by Mariusz Felisiak, 12 months ago

Patch needs improvement: set

comment:21 by David Wobrock, 12 months ago

Patch needs improvement: unset

comment:22 by Mariusz Felisiak, 12 months ago

Needs tests: set
Patch needs improvement: set

comment:23 by David Wobrock, 11 months ago

Needs tests: unset
Patch needs improvement: unset

comment:24 by Mariusz Felisiak, 11 months ago

Needs tests: set
Patch needs improvement: set

comment:25 by David Wobrock, 11 months ago

Needs tests: unset
Patch needs improvement: unset

comment:26 by Mariusz Felisiak, 11 months ago

Triage Stage: AcceptedReady for checkin

comment:27 by Mariusz Felisiak <felisiak.mariusz@…>, 11 months ago

Resolution: fixed
Status: assignedclosed

In 9bbf97bc:

Fixed #16055 -- Fixed crash when filtering against char/text GenericRelation relation on PostgreSQL.

comment:28 by Mariusz Felisiak <felisiak.mariusz@…>, 11 months ago

In 8b1ff0da:

Refs #16055 -- Deprecated get_joining_columns()/get_reverse_joining_columns() methods.

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