Django 4.1 compatibility

[gabn88]

The requirement of the third party jsonfield does not play nice with the upgrade to Django 4.1.

I open this issue, since I think jsonfield can be removed in favor of the default django JSONfield. What do you think?

[selwin]

I'm ok with this. Ideally we can do this in a backwards compatible way. Mind opening a PR for this?

[selwin]

@gabn88 do you mind taking a look at this PR? https://github.com/ui/django-post_office/pull/414

This PR also replaces jsonfield with Django's built in JSONField.

[gabn88]

@selwin

I have seen multiple packages replace jsonfield.JSONField to models.JSONField (from django 3.1+). AFAIK it works fine, but it didn't test it thoroughly. Backwards-compatible shouldn't be a big problem, as Django 3.1 is already not receiving LTS anymore, and from django 3.1 onwards models.JSONField exists. As long as the old data from jsonfield is compatible.

EDIT: I've digged deeper into it and the encoder of the current implementation is here: https://github.com/rpkilby/jsonfield/blob/master/src/jsonfield/encoder.py It is different from this implementation, but since we are talking here about headers and context here, whereby headers are strings anyway.

I can see it go wrong for the context. Ideally, we would copy-paste the encoder code and use it as encoder/decoder on the JSONfield. That way it will be 100% compatible. Otherwise old emails might not render correctly (when encoding/decoding dates or datetimes, for example).

[selwin]

Ok, in that case, let’s do it 😀On Jan 25, 2023, at 1:43 AM, gabn88 @.***> wrote: @selwin I have seen multiple packages replace jsonfield.JSONField to models.JSONField (from django 3.1+). AFAIK it works fine, but it didn't test it thoroughly. Backwards-compatible shouldn't be a big problem, as Django 3.1 is already not receiving LTS anymore, and from django 3.1 onwards models.JSONField exists. As long as the old data from jsonfield is compatible.

—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: @.***>

[petrprikryl]

I can see it go wrong for the context. Ideally, we would copy-paste the encoder code and use it as encoder/decoder on the JSONfield. That way it will be 100% compatible. Otherwise old emails might not render correctly (when encoding/decoding dates or datetimes, for example).

Dates/datetimes are serialized into strings too. So the rendering will be using same data (context) after migration. If you look at https://github.com/rpkilby/jsonfield/blob/3.1.0/src/jsonfield/fields.py#L17 you will see that there isn't custom decoder class so date/datetimes aren't converted to Python objects in json.loads step.

[selwin]

I have decided on the way forward. The safest way would be to add a new JSON column using Django's built in JSONField and keep backward compatibility for a while before we can eventually drop the old column.

[gabn88]

This is actually a good idea. Let's do that as it gives people a time frame to do the migration themselves. I assume you would include a setting which column to use, where the default in this release is the old column and in the next major release will be the new column?

[selwin]

No, when delivering the email, we should check the new column, if it’s empty we also check the old one.On May 17, 2023, at 12:39 AM, gabn88 @.***> wrote: This is actually a good idea. Let's do that as it gives people a time frame to do the migration themselves. I assume you would include a setting which column to use, where the default in this release is the old column and in the next major release will be the new column?

—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: @.***>