Add serialized object field #412

sum-rock · 2022-08-08T14:00:31Z

This work creates the basic implementation of the LogEntry.serialized_object field. This field tracks the state of an object following a create, update, or delete action. A manager method is also added to retrieve the value of an object's field at a given date.

Efforts were taken to make sure this addition is not disruptive to current installations of django-auditlog. Specifically, this is an opt-in feature per model. Users are able to opt in by adding the serialize_data=True kwarg to the model registration.

This relates to issue 410.

codecov · 2022-08-08T14:04:00Z

Codecov Report

Merging #412 (509d0ee) into master (a00d2c2) will increase coverage by 0.44%.
The diff coverage is 94.91%.

@@            Coverage Diff             @@
##           master     #412      +/-   ##
==========================================
+ Coverage   91.79%   92.24%   +0.44%     
==========================================
  Files          23       24       +1     
  Lines         646      709      +63     
==========================================
+ Hits          593      654      +61     
- Misses         53       55       +2

Impacted Files	Coverage Δ
auditlog/models.py	`87.12% <93.61%> (+1.64%)`	⬆️
...ditlog/migrations/0011_logentry_serialized_data.py	`100.00% <100.00%> (ø)`
auditlog/registry.py	`98.51% <100.00%> (+0.09%)`	⬆️
auditlog/middleware.py	`100.00% <0.00%> (+7.14%)`	⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

sum-rock · 2022-08-08T14:07:02Z

This PR contains a migration. If the wonderful maintainers of django-auditlog would help my team out tremendously by adding this to their project, we would need to be mindful of any other PR's with migrations that might also make it into the release. For example, #407 also contains a migration and we'd need to coordinate that with this one so that they don't have shared dependencies.

I'm also aware that I'd need to add something to the change log here. Let me know what you all think of this PR and if you're feeling positive about it, I'll write something up.

hramezani · 2022-08-08T14:38:25Z

Thanks @sum-rock for the PR 👍

I am still not sure about the idea of the feature.
I will back to the PR later. Unfortunately, I don't have time to check it now.

@alieh-rymasheuski @Linkid It would be great if you have time to take a look at this PR.

A field was added to the LogEntry model to store a serialized dictionary of key value pairs representing the state of the object after the create, update, or delete action. This field is populated optionally using an `auditlog.regester` kwarg.

auditlog/models.py

sum-rock · 2022-08-10T01:38:11Z

Given some of the feedback here, I've made a few adjustments. I'm still optimistic that you all may still be favorable to including the serialized_data field in the auditlog project. When I think about the typical use cases of an audit log, I think that having an option to access a full account of an objects state, either prior to or following a change, would be helpful in understanding not only what changes were made, but also why they might have been made. This is a concept that is embraced by other audit logging systems such as Papertrail. However, there isn't a good option for Django that we are aware of. Django auditlog is so darn close to having everything that anyone would ever need in an audit log! We just want to help put the cherry on top of the Sunday here.

I can understand if the contents of the HistoricalStateLookupManagerMixin is too heavy handed (although I think people would use it to avoid boilerplate code and more easily find the object or field states they are looking for). So if that is not in keeping with the vision of django auditlog, I can definitely extract it from this PR.

Please let me know if you all have any other concerns that could be mitigated.

hramezani · 2022-08-10T06:53:09Z

Thanks @sum-rock for the update. I will take a look whenever I have time.

auditlog/history.py

hramezani · 2022-08-10T15:43:08Z

To me, the change has two parts

Adding a JSON field to store the JSON repr of the object. (sounds good)
The HistoricalStateLookupManagerMixin class, is about how we use the stored data. there are two methods there for finding the log record and finding a field value from the log at a specific time.

I think we can remove the HistoricalStateLookupManagerMixin from this path because:

I think the idea of this library is to store the changes, not how to use them.
Finding a log entry at a given timestamp is not a big thing and users of the library can easily do it
The currently added functions are not complete. we only have a filter for one timestamp with __lte and maybe we need another filter for __gte, __lt, __gt, ... . we may want to get multiple fields value not just one field value. we may want to apply filters based on user and action as well. In the end I would say the functions are not generic enough to cover all the needs

I would like to know what do you think @sum-rock and @rposborne?

sum-rock · 2022-08-10T16:54:30Z

To me, the change has two parts

Adding a JSON field to store the JSON repr of the object. (sounds good)

The HistoricalStateLookupManagerMixin class, is about how we use the stored data. there are two methods there for finding the log record and finding a field value from the log at a specific time.

Absolutely. I wanted to decouple these things as much as possible while keeping them in the same PR. (In hind sight, maybe that wasn't the correct approach).

I think we can remove the HistoricalStateLookupManagerMixin from this path because:

I think the idea of this library is to store the changes, not how to use them.

Finding a log entry at a given timestamp is not a big thing and users of the library can easily do it

I generally agree with this point. I wasn't going to add anything for that PR initially but decided to attempt to incorporate something to both show the value of the serialized_data field and to hopefully reduce an end user's boiler plate code. The manager methods also handle the cases of missing data which is less trivial but, yes, it's not a huge lift for end users to implement themselves either.

The currently added functions are not complete. we only have a filter for one timestamp with __lte and maybe we need another filter for __gte, __lt, __gt, ... . we may want to get multiple fields value not just one field value. we may want to apply filters based on user and action as well. In the end I would say the functions are not generic enough to cover all the needs

I'm not sure about needing a separate method for the different query filters because we are looking for the state of an object or field at a given time and that's accomplished with __lte. But I do hear you that the historical lookup methods aren't as full featured as some users may want or need them to be. If you commit to including these retrieval methods, you may find yourself on a slippery slope with end users who want more.

Overall, I think I'm in alignment with your points here @hramezani. You'd like auditlog to focus on recording actions and changes rather than on having full featured queries for the retrieval of those records. I think that is completely valid. I'm going to remove the HistoricalStateLookupManagerMixin from this PR and that should reduce both the surface area of change and burden of maintenance greatly.

hramezani · 2022-08-10T17:01:19Z

Thanks for your understanding @sum-rock

Test cases were added to assert the serialized data field is functioning as intended and that the registration kwargs are implemented as written.

hramezani · 2022-08-10T22:19:24Z

Why did you add the changes from the other PR here?
It makes it hard to review. We can first review this one and it can be merged to master because it is backward compatible. Then you could refresh the other PR to fix the migration dependency

sum-rock · 2022-08-10T22:34:24Z

@hramezani I really apologize for that. I had some git madness on my end and unintentionally made that happen. It's fixed now. I was hoping it would be fixed before you'd notice 🙈 .

hramezani · 2022-08-12T09:00:00Z

Please add documentation into usage after Many-to-many fields section.

auditlog/registry.py

auditlog_tests/tests.py

auditlog/registry.py

CHANGELOG.md

auditlog/models.py

A few redundant lines were able to be elimited from the set logic. Statment now takes the set of included fields (or all fields if no included list was found) and removes the fields present in the excluded list.

hramezani · 2022-08-16T07:17:27Z

@alieh-rymasheuski as you left some comments on this PR, It would be great if you check the updates here.

auditlog/models.py

The `_mask_serialized_fields` method had been receiving a all model field options but only required the `mask_fields` value. The relative import for `mask_str` function was also changed to be an absolute import.

aleh-rymasheuski · 2022-08-16T14:51:55Z

@hramezani, passing this pull request back to you. I don't have objections to merging this pull request.

hramezani · 2022-08-16T14:56:41Z

@hramezani, passing this pull request back to you. I don't have objections to merging this pull request.

Thanks @alieh-rymasheuski for the review. I will take a final look later and will merge it.

sum-rock · 2022-08-16T15:15:10Z

@hramezani Before you guys merge. I'm looking into something that's coming up with implementing this in our system. The issue occurs if datetimes are written as strings on in-memory model fields. Without serialize_data=True, the DB will type cast successfully. However when updating, auditlog is sending a pre-save signal and the serializer will attempt to serialize a string as if it is a datetime and thus fail. Please standby as I am currently trouble shooting.

It may be advisable to add a cls=LazyEncoder to the serializer by default? This could be something that that requires solutions all on our side and auditlog with this PR is fine. I'll know more within a few hours.

The Django core serializer assumes that the values on object fields are correctly typed to their respective fields. Updates made to an object's in-memory state may not meet this assumption. To prevent this violation, values are typed by calling `to_python` from the field object, the result is set on a copy of the instance and the copy is sent to the serializer. For example, a datetime parsed as a string and set on a `models.DateTimeField()` field is normally written to the database as a string and set to a python datetime object when read on the model. Therefore, no error is raised on save when a datetime field contains a string formatted datetime. The serializer, however, expects a datetime object and will attempt to call `isoformat()` to convert the datetime object to a string which then raises an attribute error.

sum-rock · 2022-08-17T02:19:43Z

@hramezani @alieh-rymasheuski I was running our testing suite with these changes this morning and noticed a few unexpected errors. After fully untangling the stacktrace this afternoon I believe I've found the issue and put together a correction. I do think it is best to put the correction in this PR rather than relying on developers to correct in their own code. Avoiding any disruption, even if users need to opt into this new feature, is in everyone's interest.

Take a look at what I've written and let me know your thoughts. I've tried to include an explanation in the doc string and commit message so I'll avoid repeating it here. One thing I might add is that the try/except around the deepcopy was done to avoid raising TypeErrors for missing arguments that are required when attempting to copy some objects. The instance_copy is preferred because I'd like to avoid mutating something before saving it possible. However, I don't think it's critical because the model save and model instantiation process cycle will do a similar type transformation in the same way anyway.

Thanks again for your time and dedication, our entire team is very grateful.

auditlog/models.py

aleh-rymasheuski · 2022-08-17T12:03:52Z

@hramezani why milestone 2.2? This seems to be a backwards-compatible change so it can be include in a 1.X feature release.

UPD: please ignore this message, I forgot that auditlog version was already 2.X.

hramezani · 2022-08-17T12:15:24Z

@hramezani why milestone 2.2? This seems to be a backwards-compatible change so it can be include in a 1.X feature release.

@alieh-rymasheuski why 1.x? Latest version for django-auditlog is 2.1.1 https://pypi.org/project/django-auditlog/
I would like to include this change in the next release 2.2

Removed an unnecessary call to copy the data sent to the mask serialize data method. An in method import was also changed to be absolute rather than relative.

With this strategy, a value is not require to determine if the field should be skipped when performing the ``to_python`` call. This is preferred because there are a few edge cases around creating log entries as a result of a cascade delete. In those cases, there are situations when the system attempts to get a value that has already been deleted.

sum-rock · 2022-08-18T14:52:00Z

Added a two line change due to an edge case. This wasn't happening in normal cascade delete circumstances but it showed up in "real world" use cases where there are lots of interdependencies. We're rolling this out into production here within the next few days so feel confident that if there are any other bugs, I'll be on to squash them.

hramezani · 2022-08-21T19:45:12Z

Thanks @sum-rock for your effort!

Add serialized_data field to LogEntry

5394cf8

A field was added to the LogEntry model to store a serialized dictionary of key value pairs representing the state of the object after the create, update, or delete action. This field is populated optionally using an `auditlog.regester` kwarg.

sum-rock force-pushed the add-serialized-object-field branch 2 times, most recently from e0641e5 to d0ec818 Compare August 10, 2022 01:17

sum-rock commented Aug 10, 2022

View reviewed changes

auditlog/models.py Outdated Show resolved Hide resolved

hramezani reviewed Aug 10, 2022

View reviewed changes

auditlog/history.py Outdated Show resolved Hide resolved

hramezani reviewed Aug 10, 2022

View reviewed changes

auditlog/history.py Outdated Show resolved Hide resolved

hramezani reviewed Aug 10, 2022

View reviewed changes

auditlog/history.py Outdated Show resolved Hide resolved

sum-rock added 3 commits August 10, 2022 16:54

Migrations for serialized_data field

bc74dc3

Tests for serialized_data field

622d8d8

Test cases were added to assert the serialized data field is functioning as intended and that the registration kwargs are implemented as written.

Add line to CHANGELOG for new feature

b860d59

sum-rock force-pushed the add-serialized-object-field branch from d0ec818 to b860d59 Compare August 10, 2022 21:55

sum-rock requested a review from hramezani August 10, 2022 21:57

sum-rock force-pushed the add-serialized-object-field branch from 48953c8 to b860d59 Compare August 10, 2022 22:17

hramezani reviewed Aug 12, 2022

View reviewed changes

auditlog/registry.py Outdated Show resolved Hide resolved

hramezani reviewed Aug 12, 2022

View reviewed changes

auditlog/registry.py Outdated Show resolved Hide resolved

hramezani reviewed Aug 12, 2022

View reviewed changes

auditlog_tests/tests.py Outdated Show resolved Hide resolved

hramezani reviewed Aug 12, 2022

View reviewed changes

auditlog/registry.py Outdated Show resolved Hide resolved

aleh-rymasheuski reviewed Aug 12, 2022

View reviewed changes

Specify dictionary type content

23c7277

hramezani reviewed Aug 15, 2022

View reviewed changes

auditlog/models.py Outdated Show resolved Hide resolved

Simplify applicable model field method

932be49

A few redundant lines were able to be elimited from the set logic. Statment now takes the set of included fields (or all fields if no included list was found) and removes the fields present in the excluded list.

sum-rock requested a review from hramezani August 15, 2022 19:45

aleh-rymasheuski reviewed Aug 16, 2022

View reviewed changes

auditlog/models.py Outdated Show resolved Hide resolved

auditlog/models.py Show resolved Hide resolved

auditlog/models.py Outdated Show resolved Hide resolved

aleh-rymasheuski reviewed Aug 16, 2022

View reviewed changes

auditlog/models.py Outdated Show resolved Hide resolved

auditlog/models.py Show resolved Hide resolved

Cleaned up the mask fields method

a9dfa78

The `_mask_serialized_fields` method had been receiving a all model field options but only required the `mask_fields` value. The relative import for `mask_str` function was also changed to be an absolute import.

sum-rock requested a review from aleh-rymasheuski August 16, 2022 14:41

hramezani reviewed Aug 17, 2022

View reviewed changes

auditlog/models.py Outdated Show resolved Hide resolved

hramezani reviewed Aug 17, 2022

View reviewed changes

auditlog/models.py Outdated Show resolved Hide resolved

hramezani reviewed Aug 17, 2022

View reviewed changes

auditlog/models.py Outdated Show resolved Hide resolved

hramezani added this to the 2.2 milestone Aug 17, 2022

hramezani removed this from the 2.2 milestone Aug 17, 2022

Remove call to copy masked_data

4745d4a

Removed an unnecessary call to copy the data sent to the mask serialize data method. An in method import was also changed to be absolute rather than relative.

sum-rock requested a review from hramezani August 17, 2022 14:06

hramezani closed this Aug 21, 2022

hramezani reopened this Aug 21, 2022

hramezani approved these changes Aug 21, 2022

View reviewed changes

hramezani merged commit 777bd53 into jazzband:master Aug 21, 2022

2ykwang mentioned this pull request Nov 15, 2022

Update serialized object field docs #452

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add serialized object field #412

Add serialized object field #412

sum-rock commented Aug 8, 2022 •

edited

Loading

codecov bot commented Aug 8, 2022 •

edited

Loading

sum-rock commented Aug 8, 2022

hramezani commented Aug 8, 2022

sum-rock commented Aug 10, 2022

hramezani commented Aug 10, 2022

hramezani commented Aug 10, 2022

sum-rock commented Aug 10, 2022

hramezani commented Aug 10, 2022

hramezani commented Aug 10, 2022

sum-rock commented Aug 10, 2022

hramezani commented Aug 12, 2022

hramezani commented Aug 16, 2022

aleh-rymasheuski commented Aug 16, 2022

hramezani commented Aug 16, 2022

sum-rock commented Aug 16, 2022 •

edited

Loading

sum-rock commented Aug 17, 2022 •

edited

Loading

aleh-rymasheuski commented Aug 17, 2022 •

edited

Loading

hramezani commented Aug 17, 2022

sum-rock commented Aug 18, 2022 •

edited

Loading

hramezani commented Aug 21, 2022

Add serialized object field #412

Add serialized object field #412

Conversation

sum-rock commented Aug 8, 2022 • edited Loading

codecov bot commented Aug 8, 2022 • edited Loading

Codecov Report

sum-rock commented Aug 8, 2022

hramezani commented Aug 8, 2022

sum-rock commented Aug 10, 2022

hramezani commented Aug 10, 2022

hramezani commented Aug 10, 2022

sum-rock commented Aug 10, 2022

hramezani commented Aug 10, 2022

hramezani commented Aug 10, 2022

sum-rock commented Aug 10, 2022

hramezani commented Aug 12, 2022

hramezani commented Aug 16, 2022

aleh-rymasheuski commented Aug 16, 2022

hramezani commented Aug 16, 2022

sum-rock commented Aug 16, 2022 • edited Loading

sum-rock commented Aug 17, 2022 • edited Loading

aleh-rymasheuski commented Aug 17, 2022 • edited Loading

hramezani commented Aug 17, 2022

sum-rock commented Aug 18, 2022 • edited Loading

hramezani commented Aug 21, 2022

sum-rock commented Aug 8, 2022 •

edited

Loading

codecov bot commented Aug 8, 2022 •

edited

Loading

sum-rock commented Aug 16, 2022 •

edited

Loading

sum-rock commented Aug 17, 2022 •

edited

Loading

aleh-rymasheuski commented Aug 17, 2022 •

edited

Loading

sum-rock commented Aug 18, 2022 •

edited

Loading