Opened 11 years ago

Closed 11 years ago

Last modified 10 years ago

#2798 closed enhancement (wontfix)

[patch] make comments work on models with alphanummmeric PKs

Reported by: Maximillian dornseif <md@…> Owned by: Adrian Holovaty
Component: Contrib apps Version: master
Severity: minor Keywords:
Cc: md@… Triage Stage: Unreviewed
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:


Comments don't work on Models using non-integer Primary Keys. The tiny patch below makes them work (with no observable preformance impact).

--- django/contrib/comments/	(revision 3797)
+++ django/contrib/comments/	(working copy)
@@ -64,7 +64,7 @@
 class Comment(models.Model):
     user = models.ForeignKey(User, raw_id_admin=True)
     content_type = models.ForeignKey(ContentType)
-    object_id = models.IntegerField(_('object ID'))
+    object_id = models.CharField(_('object ID'), maxlength=255)

Change History (4)

comment:1 Changed 11 years ago by Malcolm Tredinnick

This is a massively backwards-incompatible change (everybody using the comments system would have to alter their databases). I'm inclined not to include it for that reason and because comments are slated to get a rewrite prior to 1.0.

comment:2 Changed 11 years ago by md@…

I don't see how this is massively backwards-incompatible. The SQL Layer should handle mapping from intteger fields to char fields gracefully.
In fact I'm was running my installlation for two months or so with just changing the database structure to varchar(255) and not changing the model. This worked for numeric only PKs in a varchar field but not for alphanumeric PKs.

So this is not backwards incompatible on PostgreSQL and - although not tested - my experience with MySQL indicates that it should also work with MySQL. You don't have to change the database scheme.

comment:3 Changed 11 years ago by Malcolm Tredinnick

You can write an integer value into a varchar field in a database (which is what you were testing). You cannot always do the reverse. So if we change the Python model to allow character strings in that place, people should change the relevant database tables as well, otherwise there will be cases where they cannot write out the object_id if they try to use comments with a model that has a non-integer primary key. We cannot deliberately ship code that can cause database errors if you use it as documented.

So it is backwards incompatible because it requires a database change to work properly.

comment:4 Changed 11 years ago by anonymous

Resolution: wontfix
Status: newclosed

To my understanding you can always write numeric strincs into an integer field. Alphanumeric strincs should only occur at people having alphanumerik PKs and they can't use the comment framework without database changes anyway.

But "because comments are slated to get a rewrite prior to 1.0." I suggest we move on to more interesting tickets.

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