search

django / django-contrib-comments

BSD 3-Clause "New" or "Revised" License
614 stars 196 forks source link

Add db_index=True to object_pk and is_removed fields #159

Closed atodorov closed 3 years ago

atodorov commented 4 years ago

TextField with db_index will fail on MySQL/MariaDB b/c there's no fixed size specified. Works fine on Postgres. In a different library that was solved by changing object_pk to CharField(max_length=64). See https://github.com/bartTC/django-attachments/pull/81

Any objections to do the same here ?

Note2: the reported PASS result means there's no testing with MySQL here so that also needs to be added. I can work on it if the CharField change is accepted.

atodorov commented 3 years ago

TextField with db_index will fail on MySQL/MariaDB b/c there's no fixed size specified. Works fine on Postgres. In a different library that was solved by changing object_pk to CharField(max_length=64). See bartTC/django-attachments#81

Any objections to do the same here ?

@claudep ^^^

claudep commented 3 years ago

I think that transforming TextField to CharField(max_length=64) for object_id should be fine for a vast majority of use cases. We could even describe how to keep the TextField object_id by a custom Comment model if really needed. @carltongibson, @felixxm, do you think this would be an acceptable backwards incompatibility? My hopes of getting auto-limited MySQL index sizes (in the spirit of https://github.com/django/django/pull/8848/files) are now very thin.

atodorov commented 3 years ago

I've converted object_pk field to CharField and updated the history in case you decide this looks good enough and want to merge. Thanks for reviewing.

felixxm commented 3 years ago

@claudep I think it's fine. Docs changes are missing, e.g. https://github.com/django/django-contrib-comments/blob/ab3e0e6afa0af0a478ce024f3f398134da7d778f/docs/models.txt#L29-L33

claudep commented 3 years ago

@atodorov, thanks for your patient work on this. I think it's good to go.