Add serialized object field

[sum-rock]

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[bot]]

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%)	:arrow_up:
...ditlog/migrations/0011_logentry_serialized_data.py	100.00% <100.00%> (ø)
auditlog/registry.py	98.51% <100.00%> (+0.09%)	:arrow_up:
auditlog/middleware.py	100.00% <0.00%> (+7.14%)	:arrow_up:

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

[sum-rock]

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]

Thanks @sum-rock for the PR :+1:

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.

[sum-rock]

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]

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

[hramezani]

To me, the change has two parts

1. Adding a JSON field to store the JSON repr of the object. (sounds good)
2. 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]

To me, the change has two parts

1. Adding a JSON field to store the JSON repr of the object. (sounds good)
2. 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]

Thanks for your understanding @sum-rock

[hramezani]

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]

@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]

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

[sum-rock]

@hramezani @alieh-rymasheuski Thank you for the code review. I appreciate your feedback and I've made effort to make sure I have addressed each of your comments. There are a few things you may want to take a look at. In particular, the changes around mask_fields and my responses to the comments about what fields to include in the serialized data and my thoughts about serializing reverse FK relationships.

Again, I really appreciate the time you both spend in maintaining this project and the diligence you show in your efforts. Enjoy the weekend!

[hramezani]

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

[aleh-rymasheuski]

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

[hramezani]

@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]

@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.

[sum-rock]

@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.

[aleh-rymasheuski]

@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]

@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

[sum-rock]

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]

Thanks @sum-rock for your effort!