hmpf / easydmp

MIT License
7 stars 2 forks source link

Convert to jsonb #158

Closed hmpf closed 3 years ago

hmpf commented 3 years ago

Closes #82.

This is the first step to get rid of the dependency "jsonfield".

It has been replaced with django.contrib.postgres.fields.JSONField.

In order to finally get rid of the dependency, it will be necessary to spread it over two releases. This one gets rid of it in running code, while the next will get rid of it completely in the migration history, most easily by squashed migrations.

codecov[bot] commented 3 years ago

Codecov Report

Merging #158 (fbc58f3) into master (25d9c74) will increase coverage by 0.10%. The diff coverage is 73.01%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #158      +/-   ##
==========================================
+ Coverage   55.17%   55.28%   +0.10%     
==========================================
  Files          97      100       +3     
  Lines        5870     5922      +52     
==========================================
+ Hits         3239     3274      +35     
- Misses       2631     2648      +17     
Impacted Files Coverage Δ
...c/easydmp/plan/migrations/0008_convert_to_jsonb.py 55.00% <55.00%> (ø)
...asydmp/eestore/migrations/0002_convert_to_jsonb.py 73.33% <73.33%> (ø)
...sydmp/eventlog/migrations/0002_convert_to_jsonb.py 73.33% <73.33%> (ø)
...igrations/0001_squashed_0002_update_django_meta.py 100.00% <100.00%> (ø)
src/easydmp/eestore/models.py 53.46% <100.00%> (+0.46%) :arrow_up:
src/easydmp/eventlog/migrations/0001_initial.py 100.00% <100.00%> (ø)
src/easydmp/eventlog/models.py 81.33% <100.00%> (+0.12%) :arrow_up:
...s/0001_squashed_0024_cleanup_plan_editor_groups.py 100.00% <100.00%> (ø)
src/easydmp/plan/models.py 42.82% <100.00%> (+0.12%) :arrow_up:
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 25d9c74...fbc58f3. Read the comment docs.

hmpf commented 3 years ago

We're very fortunate here that the new jsonfield can contain both strings and dicts. That makes it possible to cheat quite a bit in the migrations. For other types of changing field types, the following procedure would be needed:

  1. Add a field with a temporary name with the new type, and migrate
  2. Have a datamigration copy data from old to new. Don't migrate!
  3. Add a migration-file to rename old to something temporary. Don't migrate!
  4. Add a migration-file to rename the new to the proper name. Don't migrate!
  5. Add a migration-file to drop the old field. Don't migrate!
  6. Combine the migration files in steps 2 to 5, in that exact order, into a single migration-file to ensure it all is in the same transaction. NOW you can migrate. The first migration cannot be combined with the rest.
hmpf commented 3 years ago

Ugh this reminds me, the way the data migration is written this'll update the Plan.modified field. Give me a sec.

hmpf commented 3 years ago

There: Plan.modified not modified. Gotta remove the "auto_now" from it..

I timed these migrations on a copy of the prod data:

bash cmd-local.sh migrate eestore 1,03s user 0,07s system 91% cpu 1,198 total bash cmd-local.sh migrate eventlog 2,09s user 0,13s system 70% cpu 3,153 total bash cmd-local.sh migrate plan 1,09s user 0,09s system 87% cpu 1,347 total

Not bad. The non-cheaty way took forever on eventlog.

hmpf commented 3 years ago

I've manually tested it backwards and forwards on a copy of prod data so I think we're good. But I'll take an extra backup of prod anyway, before I deploy =)

hmpf commented 3 years ago

Rebased+merged manually.