Code

Opened 6 years ago

Closed 5 years ago

Last modified 3 years ago

#8554 closed (fixed)

Comment object_pk isn't correctly cast to text for lookups

Reported by: apollo13 Owned by: nobody
Component: contrib.comments Version: master
Severity: Keywords:
Cc: jezdez, 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 apollo13 6 years ago.
fix_comments.diff (539 bytes) - added by cmutel@… 6 years ago.
Explicitly cast id to string
cast_to_unicode.diff (780 bytes) - added by cmutel@… 6 years ago.
Uses smart_unicode() instead of str()

Download all attachments as: .zip

Change History (21)

Changed 6 years ago by apollo13

comment:1 Changed 6 years ago by Alex

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Design 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 6 years ago by apollo13

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

comment:3 Changed 6 years ago by jacob

  • Summary changed from Newcomments should use PositiveIntegerField instead of TextField for object_pk to Comment 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 6 years ago by jacob

  • Component changed from Uncategorized to django.contrib.comments
  • milestone set to 1.0
  • Triage Stage changed from Design decision needed to Accepted

comment:5 Changed 6 years ago by jacob

apollo13: can you post the entire traceback?

comment:6 Changed 6 years ago by apollo13

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 6 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 6 years ago by baumer1122

#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 6 years ago by cmutel@…

Explicitly cast id to string

comment:9 Changed 6 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 6 years ago by mtredinnick

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 6 years ago by jezdez

  • Cc jezdez added

Changed 6 years ago by cmutel@…

Uses smart_unicode() instead of str()

comment:12 Changed 6 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 6 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 6 years ago by jacob

  • Resolution set to fixed
  • Status changed from new to closed

Fixed in [8631]

comment:15 Changed 6 years ago by anonymous

That is [8641]

comment:16 Changed 5 years ago by martync

  • Resolution fixed deleted
  • Status changed from closed to reopened

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 5 years ago by kmtracey

  • Resolution set to fixed
  • Status changed from reopened to closed

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 3 years ago by jacob

  • milestone 1.0 deleted

Milestone 1.0 deleted

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.