Opened 8 years ago

Closed 7 years ago

Last modified 5 years ago

#8554 closed (fixed)

Comment object_pk isn't correctly cast to text for lookups

Reported by: Florian Apolloner Owned by: nobody
Component: contrib.comments Version: master
Severity: Keywords:
Cc: Jannis Leidel, hanne.moa@… Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

The title says it all... (http://code.djangoproject.com/browser/django/trunk/django/contrib/comments/models.py#L22) Postgres throws a nice error:

   Caught an exception while rendering: operator does not exist: text = integer
LINE 1: ...public" = true  AND "django_comments"."object_pk" = 139  AND...
                                                             ^
HINT:  No operator matches the given name and argument type(s). You might need to add explicit type casts.

Attachments (3)

newcomments_fix.diff (596 bytes) - added by Florian Apolloner 8 years ago.
fix_comments.diff (539 bytes) - added by cmutel@… 8 years ago.
Explicitly cast id to string
cast_to_unicode.diff (780 bytes) - added by cmutel@… 8 years ago.
Uses smart_unicode() instead of str()

Download all attachments as: .zip

Change History (21)

Changed 8 years ago by Florian Apolloner

Attachment: newcomments_fix.diff added

comment:1 Changed 8 years ago by Alex Gaynor

Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset
Triage Stage: UnreviewedDesign decision needed

This was originally done to allow the generic foreign key to point to a model with a cusotm non integer fkey, marking as DDN.

comment:2 Changed 8 years ago by Florian Apolloner

If that's so we still need to fix it for Postgres.

comment:3 Changed 8 years ago by Jacob

Summary: Newcomments should use PositiveIntegerField instead of TextField for object_pkComment object_pk isn't correctly cast to text for lookups

The problem isn't the use of the textfield; it's that the value isn't getting cast correctly -- it's probably a bug in GenericForeignKey.

comment:4 Changed 8 years ago by Jacob

Component: Uncategorizeddjango.contrib.comments
milestone: 1.0
Triage Stage: Design decision neededAccepted

comment:5 Changed 8 years ago by Jacob

apollo13: can you post the entire traceback?

comment:6 Changed 8 years ago by Florian Apolloner

there you go: http://dpaste.com/73906/. I am still not convinced of using TextFields here, I think a PositiveIntegerField is something every comment model writer could live with...

comment:7 Changed 8 years ago by Jacob

... until they want to attach a comment to an object that doesn't use an integer as its primary key.

comment:8 Changed 8 years ago by Peter Baumgartner

#8565 was closed as duplicate, but just so it is noted here, the default everywhere else in Django is to use object_id for the generic relation field name. It would make sense that comments used that instead of object_pk

Changed 8 years ago by cmutel@…

Attachment: fix_comments.diff added

Explicitly cast id to string

comment:9 Changed 8 years ago by Chris Mutel

My attachment just explicitly performs the correct casting. I am no Python expert, but this works for me (on posgresql). I think this is only a problem with new versions of psycopg, which are quite strict about types.

comment:10 Changed 8 years ago by Malcolm Tredinnick

That fix looks like it should be roughly correct, although it should use smart_unicode() or smart_str(). It will currently break with non-ASCII data.

The problem, btw, is actually only caused by PostgreSQL 8.3, which has stopped auto-casting types. The error is coming from the database layer.

comment:11 Changed 8 years ago by Jannis Leidel

Cc: Jannis Leidel added

Changed 8 years ago by cmutel@…

Attachment: cast_to_unicode.diff added

Uses smart_unicode() instead of str()

comment:12 Changed 8 years ago by Chris Mutel

Added revised patch which uses smart_unicode()

Read more about how 8.3 has changed, and what one can do to get previous behaviour, here:

http://people.planetpostgresql.org/peter/index.php?/archives/18-Readding-implicit-casts-in-PostgreSQL-8.3.html

comment:13 Changed 8 years ago by anonymous

Cc: hanne.moa@… added

Hmh, Postgresql 8.3 is almost getting to be more trouble than it is worth... I'm also seeing this.

comment:14 Changed 8 years ago by Jacob

Resolution: fixed
Status: newclosed

Fixed in [8631]

comment:15 Changed 8 years ago by anonymous

That is [8641]

comment:16 Changed 7 years ago by martync

Resolution: fixed
Status: closedreopened

The same problem seems to occur on generic.GenericRelation

class Article(models.Model):

comments = generic.GenericRelation(Comment, object_id_field="object_pk")

comment:17 Changed 7 years ago by Karen Tracey

Resolution: fixed
Status: reopenedclosed

The commit message (though it cited the wrong ticket) stated the fix only applied to the specific problem noted here:

Fixed #8544: correctly cast Comment.object_pk to string when doing lookups. This really only papers over a bigger problem related to casting the RHS of GFKs, but that larger change can wait for a more systematic fix.

This problem is fixed. The larger problem needs its own ticket (if it doesn't have one already).

comment:18 Changed 5 years ago by Jacob

milestone: 1.0

Milestone 1.0 deleted

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