Add remove_orphaned_netbox_attachments management command (#22)#109
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
netbox_attachments/management/orphan_cleanup.py (1)
23-25: 💤 Low valueOptional: precompile masks to avoid recompiling per call.
matches_any_maskis invoked per disk file insideselect_orphaned_files, recompiling every mask's regex on each call (O(files × masks) compilations). For larger trees you could compile the exclude masks once and pass the compiled patterns in. Functionally fine as-is.🤖 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 `@netbox_attachments/management/orphan_cleanup.py` around lines 23 - 25, The current matches_any_mask repeatedly recompiles masks via mask_to_regex on every call (called from select_orphaned_files), so precompile masks once and use those compiled regexes: add a helper (e.g., precompile_masks or modify select_orphaned_files) to transform masks into compiled patterns via mask_to_regex and pass those compiled patterns into matches_any_mask (or add a new matches_any_compiled that uses pattern.match), update calls in select_orphaned_files to use the compiled list to avoid O(files × masks) recompilation.netbox_attachments/version.py (1)
1-1: ⚡ Quick winConsider minor version bump for new features.
The project states it "follows Semantic Versioning" (CHANGELOG.md line 6), which reserves patch versions for bug fixes and requires minor version bumps for new functionality. A management command is new functionality, suggesting
11.3.0rather than11.2.3. However, given the release history and that 11.2.2 is already skipped, changing now would be disruptive.For future releases, consider aligning version bumps with the stated semver policy to avoid confusion for automated dependency tools.
🤖 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 `@netbox_attachments/version.py` at line 1, The package version in netbox_attachments/version.py is set to a patch-level value while a new management command was added; update the __version__ constant from "11.2.3" to "11.3.0" to reflect a minor-version bump per Semantic Versioning and the project's policy, and ensure the CHANGELOG.md entry and release notes reflect this minor release decision so automated tools and consumers see the correct intent.
🤖 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
`@netbox_attachments/management/commands/remove_orphaned_netbox_attachments.py`:
- Around line 283-288: The loop over orphaned_files calls Path(abspath).stat()
unconditionally which can raise FileNotFoundError if the file was deleted after
scanning; wrap the stat() call in a try/except that catches FileNotFoundError
(and optionally PermissionError), skip that entry when stat fails (do not add to
total_size), and optionally emit a debug/info note that the file vanished;
update the loop that references orphaned_files, total_size,
Path(abspath).stat().st_size, and the self.debug/self.info calls so only
successfully stat-ed files contribute to total_size and are reported.
- Around line 236-238: The loop that collects disk_files using
attachment_dir.rglob("*") calls path.stat() which can raise FileNotFoundError if
the file vanishes (TOCTOU); update the for-block around
attachment_dir.rglob("*") in remove_orphaned_netbox_attachments.py so that the
path.is_file() branch wraps the path.stat() access (the code that builds the
tuple with path.stat().st_mtime and appends to disk_files) in a try/except
catching FileNotFoundError (and optionally OSError) and simply continue/skip
that path when the file is gone, so the scan doesn’t crash on transient
deletions.
---
Nitpick comments:
In `@netbox_attachments/management/orphan_cleanup.py`:
- Around line 23-25: The current matches_any_mask repeatedly recompiles masks
via mask_to_regex on every call (called from select_orphaned_files), so
precompile masks once and use those compiled regexes: add a helper (e.g.,
precompile_masks or modify select_orphaned_files) to transform masks into
compiled patterns via mask_to_regex and pass those compiled patterns into
matches_any_mask (or add a new matches_any_compiled that uses pattern.match),
update calls in select_orphaned_files to use the compiled list to avoid O(files
× masks) recompilation.
In `@netbox_attachments/version.py`:
- Line 1: The package version in netbox_attachments/version.py is set to a
patch-level value while a new management command was added; update the
__version__ constant from "11.2.3" to "11.3.0" to reflect a minor-version bump
per Semantic Versioning and the project's policy, and ensure the CHANGELOG.md
entry and release notes reflect this minor release decision so automated tools
and consumers see the correct intent.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: e3e94291-26fe-4cb0-b5b9-32b8a48eb364
📒 Files selected for processing (8)
CHANGELOG.mddocs/usage.mdnetbox_attachments/management/__init__.pynetbox_attachments/management/commands/__init__.pynetbox_attachments/management/commands/remove_orphaned_netbox_attachments.pynetbox_attachments/management/orphan_cleanup.pynetbox_attachments/tests/test_orphan_cleanup.pynetbox_attachments/version.py
A file under MEDIA_ROOT can be removed by the running NetBox app between the directory walk and the stat() call. The unguarded stat() in _scan_files (st_mtime) and _report (st_size) would then raise FileNotFoundError and abort the whole command with a traceback -- even on the default dry-run/cron path the command is marketed as safe to run. Wrap both in try/except OSError and skip the vanished file, matching the error handling already used in _delete (unlink). --min-age does not cover this: it skips recently-modified files, not concurrent deletions of already-old orphans. Addresses CodeRabbit review comments on PR #109.
37ce319 to
1f1e719
Compare
Provide a maintenance command to report and clean up the two kinds of orphan that accumulate over time: - Orphaned files on disk under MEDIA_ROOT/netbox-attachments/ that no NetBoxAttachment references (leftovers from failed deletes and from renaming/overwriting an attachment's file). - Orphaned attachment records: NetBoxAttachment rows with no assignments. Since 11.0.0 unlinking the last assignment no longer deletes the attachment, so unused records (and their files) build up. Safety-first design (cf. issue #22 discussion): dry-run by default, deletion requires --delete and an interactive confirmation (--no-input to skip). A --min-age guard skips freshly written files to avoid racing in-flight uploads; -e/--exclude masks and --files-only/--records-only scope the run. Verbose mode lists each item plus a per-object-type assignment breakdown. Broken attachments (record present, file missing) are reported but never deleted. Pure diffing logic lives in management/orphan_cleanup.py (no model imports) so it is unit-tested without a live Django environment. - Add management command + orphan_cleanup helpers and unit tests - Document the command in docs/usage.md - Bump version to 11.2.3 and update CHANGELOG
Make explicit that the cleanup command keys off database rows, so an attachment whose linked object belongs to a disabled or uninstalled plugin is never treated as orphaned (its record, assignment row, and file all remain). Document this guarantee in docs/usage.md and the command docstring. Add a report-only --list-broken flag that lists attachments tied to disabled/uninstalled plugins, using the same "broken assignment" definition as the has_broken_assignments filter (ObjectType.model_class() is None). These are surfaced for manual review and never deleted.
A file under MEDIA_ROOT can be removed by the running NetBox app between the directory walk and the stat() call. The unguarded stat() in _scan_files (st_mtime) and _report (st_size) would then raise FileNotFoundError and abort the whole command with a traceback -- even on the default dry-run/cron path the command is marketed as safe to run. Wrap both in try/except OSError and skip the vanished file, matching the error handling already used in _delete (unlink). --min-age does not cover this: it skips recently-modified files, not concurrent deletions of already-old orphans. Addresses CodeRabbit review comments on PR #109.
1f1e719 to
29cfc6d
Compare
Summary
Addresses #22 ("Deleting attachments"). Adds a maintenance management command that reports and optionally cleans up orphaned attachment data — a self-contained, current-codebase recreation of the
5.1.4-b1remove_orphaned_netbox_attachmentscommand and thecleanup_orphaned_files.pyscript shared in the thread (the old code relied ondjango-unused-mediahelpers andsix, which are gone).Two kinds of orphan handled
MEDIA_ROOT/netbox-attachments/with noNetBoxAttachment.filereference. Covers leftovers from failed deletes and from renaming/overwriting an attachment's file (the old file is not removed automatically — @llamafilm's point).NetBoxAttachmentrows with no assignments. Since 11.0.0, unlinking the last assignment no longer deletes the attachment, so unused records (and their files) accumulate (the same set surfaced by?has_assignments=false).Safety-first design
Echoing @mjf's caution in the thread:
--deleteand an interactive confirmation prompt (--no-inputto skip, e.g. for cron).--min-age SECONDS(default 60) skips freshly written files to avoid racing in-flight uploads.--files-only/--records-onlyscope the run;-e/--excludemasks skip paths.Implementation notes
select_orphaned_files,select_missing_files, mask matching) lives inmanagement/orphan_cleanup.py, which imports no models, so it is unit-tested without a live Django environment (matchingtests/test_signals.py). All DB/FS I/O stays in the command.management/__init__.pyandmanagement/commands/__init__.py— required, because[tool.setuptools.packages.find]uses the classic finder and would otherwise drop the package from the built wheel.Testing
pytest— 71 passed (12 new unit tests for the diffing/age/exclude logic).ruff checkclean on new files.--help, default dry-run, verbose report, and a real--delete --no-inputround-trip against a throwaway orphan file (detected with--min-age 0, deleted, confirmed gone).Version
Bumped to
11.2.3(assumes #108 /11.2.2merges first) and updated CHANGELOG +docs/usage.md.Summary by CodeRabbit
Release Notes - Version 11.2.3
New Features
Documentation