Fix Deleting Attachments Via API#8073
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthrough
ChangesDeletion Flow Safety
🚥 Pre-merge checks | ✅ 4 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
emenslin
left a comment
emenslin
left a comment
There was a problem hiding this comment.
- Make sure the deletion is successful and no errors are raised through this deletion method
- Make sure the deletion is successful and no errors are raised for this deletion method
Looks good, I was able to delete attachments without any errors
bhumikaguptaa
left a comment
bhumikaguptaa
left a comment
There was a problem hiding this comment.
GUI deletion:
- Make sure the deletion is successful and no errors are raised through this deletion method
API deletion:
- Make sure the deletion is successful and no errors are raised for this deletion method
Works as expected, ran into no errors.
alesan99
left a comment
alesan99
left a comment
There was a problem hiding this comment.
- Make sure the deletion is successful and no errors are raised through this deletion method
- Make sure the deletion is successful and no errors are raised for this deletion method
The fix works, nice job! 👍
I just have some minor code comments
| def delete_obj(obj, deleter: Callable[[Any, Any], None] | None=None, version=None, parent_obj=None, clean_predelete=None) -> None: | ||
| # need to delete dependent -to-one records | ||
| # e.g. delete CollectionObjectAttribute when CollectionObject is deleted | ||
| # but have to delete the referring record first |
There was a problem hiding this comment.
I think these comments should be kept
| obj_id = obj.id | ||
|
|
||
| if deleter: | ||
| if deleter and (obj_id is not None): |
There was a problem hiding this comment.
From my testing, I think the fix works without this line? (And without the obj_id definition earlier). I may be wrong though.
| delete_obj(dep, deleter, version, parent_obj=obj, clean_predelete=clean_predelete) | ||
| dep_id = dep.id | ||
| if dep_id is not None: | ||
| delete_obj(dep, deleter, version, parent_obj=obj, clean_predelete=clean_predelete) |
There was a problem hiding this comment.
Nitpick, but the indentation is off here and in the comment before.
github-project-automation
Bot
moved this from 📋Back Log
to Dev Attention Needed
in General Tester Board
… its not non as its redundants, Fixed indentaiton on delete
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
specifyweb/specify/api/crud.py (1)
185-188:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't propagate the parent's
versioninto recursive dependent deletions.
versionbelongs to the top-level object being deleted (used bybump_versionfor optimistic locking). Passing it down means each dependent runsbump_version(dep, parent_version), which comparesparent_versionagainst the dependent's ownversioncolumn. Unless they happen to coincide,manager.filter(pk=dep.pk, version=parent_version).update(...)returns 0 and you'll get aStaleObjectExceptionon otherwise valid cascades (e.g. Attachment when deleting a{Table}Attachment).The previous behavior of recursing without
versionwas correct — dependents should not participate in the caller-driven optimistic-lock check.🛠️ Suggested fix
for dep in dependents_to_delete: dep_id = dep.id if dep_id is not None: - delete_obj(dep, deleter, version, parent_obj=obj, clean_predelete=clean_predelete) + delete_obj(dep, deleter, parent_obj=obj, clean_predelete=clean_predelete)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@specifyweb/specify/api/crud.py` around lines 185 - 188, The recursive deletion is incorrectly passing the caller's top-level `version` into dependent deletions, causing dependent `bump_version` checks to compare the wrong version; update the call site in the loop over `dependents_to_delete` (where `delete_obj(dep, deleter, version, parent_obj=obj, clean_predelete=clean_predelete)` is invoked) to omit the `version` argument so dependents are deleted with their own version checks (i.e., call `delete_obj` without forwarding `version`), leaving the top-level `version` handling intact for the originally requested object.
🧹 Nitpick comments (1)
specifyweb/specify/api/crud.py (1)
175-184: 💤 Low valueClean up the debug-era commented code and stale "temporary" note before merge.
Once the
deleterinvocation is restored with the correct null-id guard (see the critical comment), the "temporary disabled to tests its necessity…" note and the commented-out lines become dead weight. Removing them now keeps the function readable and avoids future confusion about whether the disablement was intentional.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@specifyweb/specify/api/crud.py` around lines 175 - 184, Remove the leftover debug comment block and stale "temporary disabled..." note and restore the original deleter invocation: capture obj.id into a local obj_id before calling obj.delete(), then call deleter(obj, parent_obj) only when deleter is truthy and obj_id is not None (e.g., use the guard that was commented out). Ensure the preceding explanatory comment about storing dependent ids is updated or removed so the block around obj.delete(), obj_id, deleter, and parent_obj is concise and clear.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@specifyweb/specify/api/crud.py`:
- Around line 175-182: Re-enable the deleter call in delete_obj so permission
checks and audit logging are preserved: if a deleter (from make_default_deleter)
exists, invoke deleter(obj, parent_obj) guarded by obj.id is not None and do it
before calling obj.delete(); this ensures check_table_permissions(..., "delete")
and auditlog.remove(...) run with a live id and avoids the null-id assertion on
cascaded dependents.
---
Duplicate comments:
In `@specifyweb/specify/api/crud.py`:
- Around line 185-188: The recursive deletion is incorrectly passing the
caller's top-level `version` into dependent deletions, causing dependent
`bump_version` checks to compare the wrong version; update the call site in the
loop over `dependents_to_delete` (where `delete_obj(dep, deleter, version,
parent_obj=obj, clean_predelete=clean_predelete)` is invoked) to omit the
`version` argument so dependents are deleted with their own version checks
(i.e., call `delete_obj` without forwarding `version`), leaving the top-level
`version` handling intact for the originally requested object.
---
Nitpick comments:
In `@specifyweb/specify/api/crud.py`:
- Around line 175-184: Remove the leftover debug comment block and stale
"temporary disabled..." note and restore the original deleter invocation:
capture obj.id into a local obj_id before calling obj.delete(), then call
deleter(obj, parent_obj) only when deleter is truthy and obj_id is not None
(e.g., use the guard that was commented out). Ensure the preceding explanatory
comment about storing dependent ids is updated or removed so the block around
obj.delete(), obj_id, deleter, and parent_obj is concise and clear.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 2cb0f7da-88c5-4576-b38c-cece8659cfb0
📒 Files selected for processing (1)
specifyweb/specify/api/crud.py
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
specifyweb/specify/api/crud.py (1)
157-182:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winGuard the unguarded
deleter(obj, parent_obj)call indelete_obj
delete_objstill acceptsdeleter: ... | None = None, but it callsdeleter(obj, parent_obj)unconditionally. There’s an in-tree caller that omitsdeleterentirely—specifyweb/backend/merge/record_merging.pycallsdelete_obj(..., clean_predelete=...)only—so this will raiseTypeError: 'NoneType' object is not callable.🛡️ Suggested fix
- deleter(obj, parent_obj) + if deleter is not None: + deleter(obj, parent_obj)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@specifyweb/specify/api/crud.py` around lines 157 - 182, The delete_obj function currently calls deleter(obj, parent_obj) unconditionally even though deleter may be None; update delete_obj to check whether deleter is provided before invoking it (e.g., if deleter: deleter(obj, parent_obj)) so callers that omit the deleter (like record_merging.py) won’t trigger a TypeError; keep the existing deleter parameter and only invoke the callable when it is not None.
🧹 Nitpick comments (2)
specifyweb/specify/api/crud.py (2)
184-188: 💤 Low valueTighten the comment and tiny readability nit on the dependents loop.
The fix itself is correct — Django's collector sets
pk=Noneon in-memory instances after cascade delete, so cachingdep.idand skippingNonecleanly avoids the "null id to audit log" assertion for already-cascaded dependents. Two small touch-ups would make this easier to follow:
- The comment says "before delete obj, store dep id" but
obj.delete()has already run on line 182. Reword to reflect thatobj.delete()may have cascaded the dependents topk=None.- Consider collapsing the cache/guard into the loop for readability.
♻️ Optional polish
- # before delete obj, store dep id to avoid accessing deleted obj in recursive delete calls - for dep in dependents_to_delete: - dep_id = dep.id - if dep_id is not None: - delete_obj(dep, deleter, version, parent_obj=obj, clean_predelete=clean_predelete) + # obj.delete() above may have cascade-deleted dependents, which leaves their + # in-memory pk as None. Skip those to avoid the null-id audit assertion. + for dep in dependents_to_delete: + if dep.pk is None: + continue + delete_obj(dep, deleter, version, parent_obj=obj, clean_predelete=clean_predelete)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@specifyweb/specify/api/crud.py` around lines 184 - 188, Comment and loop are slightly misleading: obj.delete() may have already cascaded dependents and set their pk to None, so we should update the comment to reflect that and inline the id-cache/None-guard for clarity. Change the comment to indicate "after obj.delete(), dependents may have been cascaded to pk=None" and rewrite the loop using a single-step pattern that reads dep_id = dep.id; if dep_id is None: continue; then call delete_obj(dep, deleter, version, parent_obj=obj, clean_predelete=clean_predelete). Keep references to dependents_to_delete, dep.id, and delete_obj to locate the change.
175-179: ⚡ Quick winPlease remove the exploratory commented-out code before merging.
Lines 175–179 carry stale debugging scaffolding — the commented
obj_id = obj.id, the "temporary disabled to tests its necessity" note, and the commentedif deleter and (obj_id is not None):guard. They no longer describe what the code does and will confuse future readers (especially since the now-active line 180 was previously the body of that commented-outif, so the indentation context is lost).♻️ Suggested cleanup (combine with the None-guard fix above)
- # store obj id and class before deletion to avoid accessing deleted obj - # obj_id = obj.id - # temporary disabled to tests its necessity since the cases in which its obj_id is missing are on the dependent objects - - # if deleter and (obj_id is not None): - deleter(obj, parent_obj) + if deleter is not None: + deleter(obj, parent_obj)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@specifyweb/specify/api/crud.py` around lines 175 - 179, Remove the exploratory commented-out lines (the commented "obj_id = obj.id", the note about "temporary disabled to tests its necessity", and the commented "if deleter and (obj_id is not None):") from the deletion logic in specify/api/crud.py; ensure the remaining code around the deletion continues to use a proper None-guard for obj.id (reintroduce or keep a check like "if deleter and obj.id is not None:" where appropriate) so the indentation/context of the now-active deletion body is correct and unambiguous.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@specifyweb/specify/api/crud.py`:
- Around line 157-182: The delete_obj function currently calls deleter(obj,
parent_obj) unconditionally even though deleter may be None; update delete_obj
to check whether deleter is provided before invoking it (e.g., if deleter:
deleter(obj, parent_obj)) so callers that omit the deleter (like
record_merging.py) won’t trigger a TypeError; keep the existing deleter
parameter and only invoke the callable when it is not None.
---
Nitpick comments:
In `@specifyweb/specify/api/crud.py`:
- Around line 184-188: Comment and loop are slightly misleading: obj.delete()
may have already cascaded dependents and set their pk to None, so we should
update the comment to reflect that and inline the id-cache/None-guard for
clarity. Change the comment to indicate "after obj.delete(), dependents may have
been cascaded to pk=None" and rewrite the loop using a single-step pattern that
reads dep_id = dep.id; if dep_id is None: continue; then call delete_obj(dep,
deleter, version, parent_obj=obj, clean_predelete=clean_predelete). Keep
references to dependents_to_delete, dep.id, and delete_obj to locate the change.
- Around line 175-179: Remove the exploratory commented-out lines (the commented
"obj_id = obj.id", the note about "temporary disabled to tests its necessity",
and the commented "if deleter and (obj_id is not None):") from the deletion
logic in specify/api/crud.py; ensure the remaining code around the deletion
continues to use a proper None-guard for obj.id (reintroduce or keep a check
like "if deleter and obj.id is not None:" where appropriate) so the
indentation/context of the now-active deletion body is correct and unambiguous.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: cf583a6e-a742-402b-b977-93c7dbce6a9d
📒 Files selected for processing (1)
specifyweb/specify/api/crud.py
emenslin
left a comment
emenslin
left a comment
There was a problem hiding this comment.
-
Make sure the deletion is successful and no errors are raised through this deletion method
-
Make sure the deletion is successful and no errors are raised for this deletion method
Looks good, I didn't run into any issues
alesan99
left a comment
alesan99
left a comment
There was a problem hiding this comment.
- Make sure the deletion is successful and no errors are raised through this deletion method
- Make sure the deletion is successful and no errors are raised for this deletion method
Works!
5 participants
Fixes #6859
Fixed an error that was caused by the delete_obj function attempting to audit an access obj.id's that were already deleted. This happened when attempting to delete an attachment or {table}attachment record via the API (both via direct API delete and using the "delete" button on the data entry form).
Checklist
self-explanatory (or properly documented)
Testing instructions
GUI deletion:
API deletion:
A query like this should help you get it:
Use the COA ID to Delete the attachment through the tables API page by using the DELETE request for Collectionobjectattachment( this page https://your specify instance/documentation/api/tables/)
Make sure the deletion is successful and no errors are raised for this deletion method
Summary by CodeRabbit