-
Notifications
You must be signed in to change notification settings - Fork 410
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
Modify change
field to be a json field.
#407
Modify change
field to be a json field.
#407
Conversation
Codecov Report
@@ Coverage Diff @@
## master #407 +/- ##
==========================================
+ Coverage 93.55% 94.01% +0.46%
==========================================
Files 29 30 +1
Lines 869 869
==========================================
+ Hits 813 817 +4
+ Misses 56 52 -4
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
0997c30
to
f325b97
Compare
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.
Good catch. But in fact, I'm agree with @hramezani.
f325b97
to
5b1ccc6
Compare
@hramezani @Linkid Thanks for the review. I added a migration and updated the changelog. Sorry about the missing migration! I'm not experienced in contributing to Django packages with migrations. I managed to make it happen in the python console with the following... Out of curiosity, is there a better way to do that? import os
import django
from django.core.management import call_command
os.environ["DJANGO_SETTINGS_MODULE"] = "auditlog_tests.test_settings"
django.setup()
call_command('makemigrations') |
Thanks @sum-rock for your update. |
58a89b7
to
4442147
Compare
@hramezani I noticed in the migration file that you suggested it changed the (Also, that last force push was done to squash a commit that formatted the migration file with black since this failed the auto-checks.) |
I like the idea of this pull request. I've got just two major concerns:
And a lesser concern: this is a breaking change because it changes the existing schema (== the API of the package) so it will require us to bump the semver to |
Sorry for that. |
@alieh-rymasheuski
As all official DB backends in Django support
This is my concern as well. I was about to test locally and see what's happening. It would be great if you @sum-rock and @alieh-rymasheuski do it as well. |
@sum-rock @alieh-rymasheuski I've tested the change on On Here is the
On Here is the
|
4442147
to
ffa6f02
Compare
@hramezani I created 502,000 I pulled this branch and installed the update. When I ran |
Thanks @sum-rock The patch seems good to me now. |
Thanks everyone for the collaboration! I felt like this was a natural progression to the code and fits within the vision for your project. I'm glad you guys agree! I'm in the process of creating another PR that would include a different breaking change. However, this next one is more opinionated so you guys might not want it. It's necessary for our project though. |
I let @hramezani approve the PR to unlock the merge, but I'm ok with this PR.
I would suggest first create an issue for that. Then we can discuss there |
@sum-rock we are preparing |
@hramezani Absolutely. I'll take a look next week, after the holiday. |
5be7d58
to
410121c
Compare
Storing the object changes as a json is preferred because it allows SQL queries to access the change values. This work moves the burden of handling json objects from an implementation of python's json library in this package and puts it instead onto the ORM. Ultimately, having the text field store the changes was leaving them less accessible to external systems and code that is written outside the scope of the django auditlog. This change was accomplished by updating the field type on the model and then removing the JSON dumps invocations on write and JSON loads invocations on read. Test were updated to assert equality of dictionaries rather than equality of JSON parsable text. Separately, it was asserted that postgres will make these changes to existing data. Therefore, existing postgres installations should update the type of existing field values without issue.
The "Modify change field to be a json field" commit reduced test coverage on the mixins.py file by 0.03%. The reduction in coverage was the result of reducing the number of operations required to achieve the desired state. An additional test was added to increase previously uncovered code. The net effect is an increase in test case coverage.
Better markdown formatting Co-authored-by: Hasan Ramezani <hasan.r67@gmail.com>
More specific language in the improvement section regarding `LogEntry.change` Co-authored-by: Hasan Ramezani <hasan.r67@gmail.com>
Co-authored-by: Hasan Ramezani <hasan.r67@gmail.com>
Running the migration to update the field type of `LogEntry.change` is a breaking change. Co-authored-by: Hasan Ramezani <hasan.r67@gmail.com>
The create log method on the LogEntry manager required an additional kwarg for a call to create an instance regardless of a change or not. This felt brittle anyway. The reason it had worked prior to these changes was that the `change` kwarg was sending a string "null" and not a None when there were no changes.
410121c
to
56f9da0
Compare
def log_create(self, instance, **kwargs): | ||
def log_create(self, instance, force_log: bool = False, **kwargs): |
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.
@hramezani This is a recent addition. When the receivers were calling this method, the changes
kwarg would equal "null"
when changes were None
. This transformation was caused by the json.dumps
method being called on None
.
Because we don't need to transform with the json library anymore, we need a way to signal that a log should be created when there are no changes. Specifically this is required for the new ACCESS
action type. I extended the idea of passing along the force_log
flag in those cases.
@hramezani The branch is now refreshed. |
Any update on this? |
Storing the object changes as a json is preferred because it allows SQL
queries to access the change values. This work moves the burden of
handling json objects from an implementation of python's json library in
this package and puts it instead onto the ORM. Ultimately, having the
text field store the changes was leaving them less accessible to external
systems and code that is written outside the scope of the django
auditlog.
This change was accomplished by updating the field type on the model and
then removing the JSON dumps invocations on write and JSON loads
invocations on read. Test were updated to assert equality of
dictionaries rather than equality of JSON parsable text.
Separately, it was asserted that postgres will make these changes to
existing data. Therefore, existing postgres installations should update the
type of existing field values without issue.