Change default_auto_field to BigAutoField #426
Conversation
|
I don't understand the test failure, they seem unrelated with my changes... Are they broken due to an external factor or am I missing something obvious? |
|
@browniebroke They're failing because Pytest was unconstrained and breaking changes in Pytest 8 (released in Jan) are blowing up the tests. My PR #425 to add Django 5 to CI also places a constraint on pytest. Hoping it might be able to get reviewed/merged soon. |
|
Thanks a lot! Will wait for a bit then. |
| @@ -12,4 +12,4 @@ class CeleryResultConfig(AppConfig): | |||
| name = 'django_celery_results' | |||
| label = 'django_celery_results' | |||
| verbose_name = _('Celery Results') | |||
| default_auto_field = 'django.db.models.AutoField' | |||
| default_auto_field = 'django.db.models.BigAutoField' | |||
There was a problem hiding this comment.
I'm curious, if this change is backward compatible?
There was a problem hiding this comment.
I think it's backwards compatible. It's changing the DB type from an integer field to a 64-bit integer, increasing its range.
However, I have a few concerns for users of the library:
- I'm not sure if there are any performances considerations when applying this migration on a table with a large number of rows. Would the
migratescript takes a while to run? What kind of locks does it need? How does it varies between databases? - The django docs have a large note about the change, and if folks have a many to many field that link to one of the tables, they'll need to do an extra migration:
Unfortunately, the primary keys of existing auto-created through tables cannot currently be updated by the migrations framework.
So it might be worthy of a major bump to signal these pitfalls...
Fix #307
Or at least make the issue much less frequent...