django-auditlog
django-auditlog copied to clipboard
jazzband
Integrate jsonfield_compat
This commit integrates django-jsonfield-compat which aims to fix #71 .
After using this branch, add
USE_NATIVE_JSONFIELD = True
to your project's settings, and then run python manage.py migrate.
This will update auditlog's LogEntry model to be a native JSONField type. You can switch everything back by changing the above setting to False, just be sure to run migrations afterwards.
The package I uploaded to pypi originally was for python2 only. I've updated that now.
Great, it builds now as well :-)
I like to hear when you think it is mergable.
I finally got around to adding a wider range of tests to django-jsonfield-compat, and am now comfortable with this being used a by a wider audience.
While I still am in favor of a compatibility layer I do want to see your particular solution before accepting these changes. You will hear from me soon.
I do not really like the fact that you do some SQL "fixes", can you elaborate a bit more on your implementation?
I chose to use direct DB manipulation so a simple Django setting could be used instead of having to create a migration for every model that used a JSONField.
The register_app call adds a post-migrate handler which may update the column type of all fields defined as jsonfield_compat.JSONField.
First, the column types are determined by this query.
We then check whether the database field type should be TEXT or JSONB depending on the USE_NATIVE_JSONFIELD setting.
If the DB field type needs to change, it runs the update query.
Let me know if you want me to go into more depth.
I do not agree with you on the migrations part, I think having migrations for these kind of things is a good thing. Hacking in SQL should - in my opinion - never be done except in one-time, custom migrations where it is unavoidable.
I do not feel comfortable imposing your solution on the users of this library since I do not feel comfortable with it myself, so I will not merge this pull request.
Still, the issue you try to fix here is somewhat important. Another solution is therefore appreciated, for example just a wrapper around the two classes which imports and returns the right class for the environment.
What user experience would you want for implementers who have already integrated django-auditlog and now want to switch to using the native JSONField implementation?
Assume their projects also include include django_jsonfield via other common libraries (e.g. django-activity-stream and django-geojson), and directly in their custom apps.
There can be no mixing of the native JSONField with django_jsonfield, so all apps have to switch at once.
I'm motivated to build support to make this work, as our company is in the situation I mention above.
(We already have custom builds of the three libraries that make this change, but we don't want to maintain a fork forever.)
I think a settings switch is still permissable, but as stated above I am not a fan of executing SQL ALTER statements manually outside of migrations (it might very well be the only practical way though). I realize that creating a migration for an external library is not something you would want to do as well. Maintaining two versions is also not a very good solution. So you could say that I am conflicted.
FWIW, @kbussell PR working for us. So thanks!! We had been using Postgres native JSONField across the project, and are just bringing in auditlog now, which caused all of our JSON fields to become strings. Switching to this branch and adding USE_NATIVE_JSONFIELD = True was perfect for us.
However, my 2c: If I had been using TEXT json fields, I'd be pretty furious if adding auditlog had the secret side effect of converting my entire database schema's JSON fields from TEXT to JSONB. I think it would be nice if auditlog could detect that you're in this bad situation and tell you, and the USE_NATIVE_JSONFIELD switch is fine. But effecting that migration has to be VERY explicit and very intentional on the part of the user.
@kbussell @jjkester I'm seeing an issue with the diffing of JSON columns, which may be caused by this change. My JSON is coming up as "changed" but it's just that the keys changed orders. It seems auditlog.diff.get_field_value needs to be special-cased for JSON fields.
Hello people. Is there somebody who can look into this PR again, please? Native JSON fields of Postgres are pretty important nowadays and crucial for many libraries and projects.
Hey @eriktelepovsky , this PR came to a bit of stand-still because of how to proceed with the implementation, the core issue is that if we proceed with this implementation and you switch your database (e.g. sqlite -> postgres), the next time you run python manage.py migrate there would be sql statements run that alter your data to the new format, and could corrupt it or mess it up, and you would have no idea as a developer that it was going to happen.
@jjkester was suggesting that a settings switch like AUDITLOG_USE_NATIVE_JSONFIELD could be added instead so the developer has to make a conscious decision to run the SQL alterations and with the right docs should be able to make a smooth transition from a DB that doesn't support JSON to a DB that does (e.g. Postgres).
We would need to make or modify this PR to be based off the settings switch, and then do some testing where we take a Sqlite3 database using django-jsonfield and convert it to Postgres and see how it goes. We also need to ensure as @ses4j mentioned that the diff algorithm correctly handles both types in a sane manner, and then we need to write good docs about how you transition from a django-jsonfield db column to a native one. So I think due to the large scope, it just hasn't seen forward progress.
If people want to contribute to this issue I can help them along but I don't have the time to work on it right now and it is also not a problem I have in my own use-case for this library at work so I can't justify time at work to fixing it.
Hi @eriktelepovsky,
If you are in a hurry, use my fork: git+https://github.com/florencioq/django-auditlog.git
It works together with django-jsonfield-compat as a workaround for this issue.
Just replace 'django-auditlog' by 'git+https://github.com/florencioq/django-auditlog.git' in your requirements.txt.
Take care, José
Thank you @florencioq . Actually, @kbussell already did the same in his commit of his PR. I believe, the best possible solution would be a direct implementation of django-jsonfield-compat into django-jsonfield so any library using JSONField could gain from its purpose.
While this PR continues to stall, I made a v0.4.5 version of this if anyone wants it:
pip install git+git://github.com/mathandpencil/django-auditlog.git@44a653d13f9077b7684727347517ea46c5ed7c95#egg=django-auditlog==0.4.5
Please consider using models.JSONField since Django 3.1 that can be used on all supported database backends:
https://docs.djangoproject.com/en/3.1/releases/3.1/#jsonfield-for-all-supported-database-backends
Since the minimum supported version is django 3.2, there is no need for this pr. I vote we close it. @hramezani
