Add audit backend #1886

waxlamp · 2024-03-06T17:38:23Z

This PR implements the backend portion of the audit design doc with the following changes:

a new model, AuditRecord, that captures audit event information in a general, extensible way
a collection of static methods on AuditRecord that construct several audit record types
invocation of those static methods at the points in the codebase where the associated actions are carried out

This PR should not be merged outside of the operations plan described in the design doc, so this will be a draft PR until we're ready to carry out that plan.

edit by @yarikoptic: Closes #61

waxlamp

Some points where I'm looking for specific feedback in my "self-review". Otherwise:

@mvandenburgh, I thought about cutting the Asset.previous field out in this PR, but that is technically out of scope now that I think about it. Once we capture a DB snapshot and put this into production, you can remove that field in your GC work. Does that sound right?
I'm deliberately deferring frontend support to a future PR. I am leaning towards creating a new branch named audit and basing this PR to that branch so that the frontend work can land there as well, enabling us to merge a feature complete branch into production when we're ready. Looking for feedback on this idea.
I'd also like to add tests, but not sure how. I'd love to work with someone (@jjnesbitt?) to figure that out. I read this article but did not fully grok it.
- I tested this PR manually by carrying out each significant action and then observing the creation of AuditRecords in the admin console. Everything seems to work but of course I am not confident I tested every edge and corner case.
What to do about these lint errors? I think both represent essential complexity, so I just put an ignore on them. Thoughts?
- dandiapi/api/services/asset/init.py:108:5: PLR0913 Too many arguments in function definition (6 > 5)
- dandiapi/api/views/dandiset.py:375:9: C901 users is too complex (11 > 10)

dandiapi/api/services/embargo/__init__.py

dandiapi/api/views/dandiset.py

dandiapi/api/views/version.py

dandiapi/api/models/audit.py

jjnesbitt

A few suggestions but otherwise I like this implementation a lot! It seems fairly lightweight, and I like the specific attention to ensuring the audit records are created in the same transaction as the objects they are tracking.

dandiapi/api/models/audit.py

dandiapi/api/tests/test_asset_paths.py

dandiapi/api/views/dandiset.py

dandiapi/api/models/audit.py

dandiapi/api/views/version.py

dandiapi/api/services/asset/__init__.py

mvandenburgh · 2024-03-15T14:16:56Z

@mvandenburgh, I thought about cutting the Asset.previous field out in this PR, but that is technically out of scope now that I think about it. Once we capture a DB snapshot and put this into production, you can remove that field in your GC work. Does that sound right?

I agree, I think removing the Asset.previous field falls more into the garbage collection scope (although there is some crossover with audit).

I'm deliberately deferring frontend support to a future PR. I am leaning towards creating a new branch named audit and basing this PR to that branch so that the frontend work can land there as well, enabling us to merge a feature complete branch into production when we're ready. Looking for feedback on this idea.

I agree frontend should be a future PR. I don't have any strong feelings towards the feature branch idea. An alternative approach could be just merging both of them piecemeal into master, without cutting a release. Then once we're ready, we can cut a release and both will go to prod at the same time. The only thought I have is because we have so many large PRs flying around lately, merging two smaller PRs into master at seperate times might be less painful then waiting to merge one huge one.

What to do about these lint errors? I think both represent essential complexity, so I just put an ignore on them. Thoughts?

dandiapi/api/services/asset/init.py:108:5: PLR0913 Too many arguments in function definition (6 > 5)

dandiapi/api/views/dandiset.py:375:9: C901 users is too complex (11 > 10)

I kind of agree with ruff that the users function is too complex. But I don't think your PR is adding any additional complexity to what is already there, so I think ignoring it is fine. And maybe we can defer a refactor to a later time.

The change_asset service function is somewhat complex, but then again, "changing" an asset is a complex operation due to our domain model and all the intricacies like de-duplication, publishing, etc. Maybe that would be worth revisting and possibly break up, but again I'm not sure this PR is the right place for that.

dandibot · 2024-08-14T18:24:15Z