-
Notifications
You must be signed in to change notification settings - Fork 216
Replace texfields with jsonfield #487
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Replace texfields with jsonfield #487
Conversation
for more information, see https://pre-commit.ci
Hi @auvipy, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR replaces several TextField
usages with JSONField
in both the TaskResult
and ChordCounter
models and adjusts migrations to migrate existing data into the new fields, then remove the old ones.
- Models updated: swap out text-based JSON storage for native
JSONField
ontask_args
,task_kwargs
,meta
, andsub_tasks
. - New migrations (0015–0017) copy existing text data into new JSON columns; 0018 removes old fields and renames the new ones.
- Ensures data continuity via batch migrations and bulk updates.
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
django_celery_results/models.py | Replaced TextField definitions for task_args , task_kwargs , meta , sub_tasks with JSONField |
django_celery_results/migrations/0015_chordcounter_new_sub_tasks_taskresult_new_meta_and_more.py | Added new JSONField columns (new_sub_tasks , new_meta , new_task_args , new_task_kwargs ) |
django_celery_results/migrations/0016_make_copy_of_taskresult_textfields.py | Populates new TaskResult JSON fields from existing text data in batches |
django_celery_results/migrations/0017_make_copy_of_chordcounter_textfields.py | Populates new ChordCounter JSON field from existing text data in batches |
django_celery_results/migrations/0018_remove_chordcounter_new_sub_tasks_and_more.py | Drops old text fields and renames new JSONField columns to their original names |
Comments suppressed due to low confidence (2)
django_celery_results/models.py:42
- There's no existing test coverage to verify serialization and deserialization of the new JSONField
task_args
. Consider adding unit tests to ensure it correctly handles various JSON structures (arrays, nested objects, nulls, etc.).
task_args = models.JSONField(
django_celery_results/models.py:151
- [nitpick] The JSONField
sub_tasks
lacks averbose_name
argument, unlike the other fields (task_args
,task_kwargs
,meta
). Adding averbose_name
will improve admin UI consistency and clarity.
sub_tasks = models.JSONField(
@@ -148,7 +148,8 @@ class ChordCounter(models.Model): | |||
verbose_name=_("Group ID"), | |||
help_text=_("Celery ID for the Chord header group"), | |||
) | |||
sub_tasks = models.TextField( | |||
sub_tasks = models.JSONField( | |||
default=None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new JSONField for sub_tasks
sets a default of None
without null=True
, which may cause database constraint errors when existing records are null. Consider adding null=True
or providing a valid default (e.g., an empty list) to maintain compatibility.
default=None, | |
null=True, | |
default=list, |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why so many schema migration files? can we reduce them? Also the change is not backward compatible as far as I can understand
Hi @auvipy, There are total 4 migration files. |
that sounds nice. allow me some time to properly check and review |
Replacing TextField with JSONField.
Related Issue