Better Optimization of AttachmentInline

[petrprikryl]

Preventing iteration over all attachments in database.

I think that this PR is better than #413 but in cost of replacing https://pypi.org/project/jsonfield/ in favor of Django's native JSONField that could be breaking somewhere.

[selwin]

Hey, I do want to move to use Django's native JSONField, but will do this in a major release (soon after the next bugfix release).

Would the switch to Django's JSONField require migration (as in the database would need to actually change it's field type and locks the database if table size is big)?

[selwin]

Mind fixing the conflict so we can prepare for the next major version of post office @petrprikryl ?

[petrprikryl]

Mind fixing the conflict so we can prepare for the next major version of post office @petrprikryl ?

Yes, I will look at it soon.

Would the switch to Django's JSONField require migration (as in the database would need to actually change it's field type and locks the database if table size is big)?

I will test it again against our DB and bring some data. I think it will lock but it should be quite a smooth and quick one.

[petrprikryl]

Some migration metrics for our production DB:

-- 65k rows: 0.1s
ALTER TABLE "post_office_attachment" ALTER COLUMN "headers" TYPE jsonb USING "headers"::jsonb;
-- 200k rows: 9s
ALTER TABLE "post_office_email" ALTER COLUMN "context" TYPE jsonb USING "context"::jsonb;
-- 200k rows: 10s
ALTER TABLE "post_office_email" ALTER COLUMN "headers" TYPE jsonb USING "headers"::jsonb;

I have also bumped the minimal Django version to 3.1 because lower versions don't have generic JSONField. It shouldn't be a big deal because it is almost one year after the end of extended support for 3.1.

[gabn88]

Ideally we need to add the currently used encoding, to make sure we don't break other peoples code.

I have added this (copy / paste) from jsonfield in the file default_encoder.py

Then we need to import this encoder and use it in the necessary places.

Such as the migrations file (0012) and the models.

Also the context_field_class settings is overridden with this PR.

[selwin]

@petrprikryl mind fixing the conflict so I can merge this in?

[selwin]

Hey, sorry but is there a reason why we're changing past migrations?

[petrprikryl]

https://github.com/rpkilby/jsonfield/blob/master/src/jsonfield/fields.py#L97 Old JSONField inherits from TextField so it is text type in DB on existing (migrated) projects. In this commit https://github.com/ui/django-post_office/pull/438/files#diff-2db5629c948ee3989f24ffc66174cb1c66aeb90d8ae1b01322084ea7987f9144 the JSONFields have been replaced. But the DB types of columns were not changed in DB because there is no new migration. And because Django's JSONField has different type in DB we need migrate the columns https://github.com/django/django/blob/4.2b1/django/db/backends/postgresql/base.py#L113. So new migration is required (0012). And without change in previous migrations, Django will skip this DB operations because "there are no model changes between migrations". You can try it. So you need to force Django via field changing to run the column altering. Btw it is more backward compatible because of old JSONField was text DB type.

Next, this problem wouldn't occur on new projects which are migrated completely from scratch.

[jrief]

This approach doesn't feel right to me. How about adding a SQL migration which changes the type of that field by force.
The problem with backward compatibility is, that migration 0008_attachment_headers already ran on the affected installations, so wouldn't have any effect anyway.

[petrprikryl]

Then we would need write custom SQL for each database that Django supports? :-/

[selwin]

This approach doesn't feel right to me. How about adding a SQL migration which changes the type of that field by force.

I agree with @jrief here.

How about this? We'll just add a new columns using Django's built in JSONField in this release and start reading from the new field, with fallback to the old one.

In a future version, we'll drop the old JSON field altogether.

[petrprikryl]

I think it is resolved by #438 so I have removed all migrations from this PR.