Opened 16 years ago

Closed 14 years ago

Last modified 12 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: dev
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: no UI/UX: no

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 16 years ago.
fix_comments.diff (539 bytes ) - added by cmutel@… 16 years ago.
Explicitly cast id to string
cast_to_unicode.diff (780 bytes ) - added by cmutel@… 16 years ago.
Uses smart_unicode() instead of str()

Download all attachments as: .zip

Change History (21)

by Florian Apolloner, 16 years ago

Attachment: newcomments_fix.diff added

comment:1 by Alex Gaynor, 16 years ago

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 by Florian Apolloner, 16 years ago

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

comment:3 by Jacob, 16 years ago

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 by Jacob, 16 years ago

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

comment:5 by Jacob, 16 years ago

apollo13: can you post the entire traceback?

comment:6 by Florian Apolloner, 16 years ago

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 by Jacob, 16 years ago

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

comment:8 by Peter Baumgartner, 16 years ago

#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

by cmutel@…, 16 years ago

Attachment: fix_comments.diff added

Explicitly cast id to string

comment:9 by Chris Mutel, 16 years ago

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 by Malcolm Tredinnick, 16 years ago

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 by Jannis Leidel, 16 years ago

Cc: Jannis Leidel added

by cmutel@…, 16 years ago

Attachment: cast_to_unicode.diff added

Uses smart_unicode() instead of str()

comment:12 by Chris Mutel, 16 years ago

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 by anonymous, 16 years ago

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 by Jacob, 16 years ago

Resolution: fixed
Status: newclosed

Fixed in [8631]

comment:15 by anonymous, 16 years ago

That is [8641]

comment:16 by martync, 14 years ago

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 by Karen Tracey, 14 years ago

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 by Jacob, 12 years ago

milestone: 1.0

Milestone 1.0 deleted

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