Code

Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#2798 closed enhancement (wontfix)

[patch] make comments work on models with alphanummmeric PKs

Reported by: Maximillian dornseif <md@…> Owned by: adrian
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:

Description

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

http://c0re.23.nu/c0de/misc/django-r3797-comments_for_objects_with_non_integer_ids.diff

--- django/contrib/comments/models.py	(revision 3797)
+++ django/contrib/comments/models.py	(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)

Attachments (0)

Change History (4)

comment:1 Changed 8 years ago by mtredinnick

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 8 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 8 years ago by mtredinnick

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

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

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.

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.