importsource: Catch importer crash when skipping #6203
Conversation
Catches a crash when "skip" is selected in the importer and
task.imported_items() runs into a condition branch that supposedly
should never be reached:
File "beets/beets/importer/tasks.py", line 254, in imported_items
assert False
|
Thank you for the PR! The changelog has not been updated, so here is a friendly reminder to check if you need to add an entry. |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- Catching a bare AssertionError here risks hiding unrelated bugs in imported_items; consider guarding this logic based on task state (e.g., task choice/skip status) or checking for the presence of imported items instead of relying on the assertion.
- Swallowing the AssertionError silently makes debugging harder if this path is hit unexpectedly; it would be helpful to at least log a debug message when the exception is caught so that it’s visible during troubleshooting.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Catching a bare AssertionError here risks hiding unrelated bugs in imported_items; consider guarding this logic based on task state (e.g., task choice/skip status) or checking for the presence of imported items instead of relying on the assertion.
- Swallowing the AssertionError silently makes debugging harder if this path is hit unexpectedly; it would be helpful to at least log a debug message when the exception is caught so that it’s visible during troubleshooting.
## Individual Comments
### Comment 1
<location> `beetsplug/importsource.py:42-47` </location>
<code_context>
- for item in task.imported_items():
- if "mb_albumid" in item:
- self.stop_suggestions_for_albums.add(item.mb_albumid)
+ try:
+ for item in task.imported_items():
+ if "mb_albumid" in item:
+ self.stop_suggestions_for_albums.add(item.mb_albumid)
+ except AssertionError:
+ # No imported items - nothing to do
+ pass
</code_context>
<issue_to_address>
**issue (bug_risk):** Catching AssertionError around the whole loop risks masking unrelated bugs.
Because the `try`/`except AssertionError` wraps the whole loop, any assertion triggered inside `task.imported_items()`, the membership check, or `add()` will be swallowed, not just the “no imported items” case. Please limit the `try` to the precise call that can raise for the empty-items condition (e.g. only `task.imported_items()`) or, if possible, detect “no imported items” without relying on catching `AssertionError`, so real programming errors are not suppressed.
</issue_to_address>
### Comment 2
<location> `beetsplug/importsource.py:46-48` </location>
<code_context>
+ for item in task.imported_items():
+ if "mb_albumid" in item:
+ self.stop_suggestions_for_albums.add(item.mb_albumid)
+ except AssertionError:
+ # No imported items - nothing to do
+ pass
def import_stage(self, _, task):
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Silently ignoring AssertionError may hinder debugging when unexpected conditions occur.
Catching `AssertionError` and doing nothing can mask genuine issues (e.g., unexpected `task`/`item` state). If the "no imported items" scenario is expected, consider narrowing the condition (e.g., checking for that case explicitly) or at least logging the exception at debug level so other assertion failures are visible.
Suggested implementation:
```python
def prevent_suggest_removal(self, session, task):
try:
for item in task.imported_items():
if "mb_albumid" in item:
self.stop_suggestions_for_albums.add(item.mb_albumid)
except AssertionError as exc:
# No imported items - nothing to do; log in case this masks a real issue.
log.debug(
"AssertionError while processing imported items for task %r: %s",
task,
exc,
exc_info=True,
)
```
If `log` is not already defined in `beetsplug/importsource.py`, you should:
1. Import the logging facility used by other beets plugins (for example, `from beets import logging`).
2. Initialize a module-level logger consistent with the rest of the codebase, e.g. `log = logging.getLogger(__name__)` or whatever convention other plugins in this project follow.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Pull request overview
This PR fixes a crash in the importsource plugin when the user selects "skip" during import, which previously caused an AssertionError when task.imported_items() was called with an unsupported action.
Key Changes:
- Added try-except block around
task.imported_items()call inprevent_suggest_removalmethod to catch the AssertionError when tasks are skipped
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| try: | ||
| for item in task.imported_items(): | ||
| if "mb_albumid" in item: | ||
| self.stop_suggestions_for_albums.add(item.mb_albumid) | ||
| except AssertionError: | ||
| # No imported items - nothing to do | ||
| pass |
There was a problem hiding this comment.
Catching bare AssertionError is problematic because:
- It's too broad and could mask other assertion failures unrelated to the skip condition
- It relies on implementation details of
imported_items() - It doesn't explicitly check the actual condition (task being skipped)
A better approach would be to check if the task should be skipped before calling imported_items():
def prevent_suggest_removal(self, session, task):
if task.skip:
return
for item in task.imported_items():
if "mb_albumid" in item:
self.stop_suggestions_for_albums.add(item.mb_albumid)This is more explicit, follows the same pattern used in other stages (e.g., plugin_stage in beets/importer/stages.py), and doesn't catch unexpected assertion failures.
| try: | |
| for item in task.imported_items(): | |
| if "mb_albumid" in item: | |
| self.stop_suggestions_for_albums.add(item.mb_albumid) | |
| except AssertionError: | |
| # No imported items - nothing to do | |
| pass | |
| if getattr(task, "skip", False): | |
| return | |
| for item in task.imported_items(): | |
| if "mb_albumid" in item: | |
| self.stop_suggestions_for_albums.add(item.mb_albumid) |
There was a problem hiding this comment.
This looks promision. Should I try it this way?
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6203 +/- ##
==========================================
- Coverage 67.93% 67.92% -0.01%
==========================================
Files 137 137
Lines 18677 18680 +3
Branches 3155 3155
==========================================
+ Hits 12688 12689 +1
- Misses 5324 5326 +2
Partials 665 665
🚀 New features to boost your workflow:
|
|
Personally, I don't think we should have any I think we should either raise a dedicated exception with debugging details so we can identify which assumption is false, or else have that function return a |
|
@JOJ0 Do you have the full trace-back here? How do we even run into this? This is a quite old part of the codebase, how was this never an issue beforehand?
I agree fully here but I would like to figure out how and why we are now running into this issue now. We did not run into this for the previous 12 years afaik. (Empty list or none makes sense in my opinion) |
Sure. Here it is: |
|
There might be some design-flaws that slipped through my initial review of this plugin. One thing is that the listener here is of no use if the configuration of the plugin does not want to use that feature anyway: But still if we'd prevent registering the listener when not configured, it does not cure the problem that Should I try to include an "empty list or none fix" as you both suggested within this PR? |
Catches a crash when "skip" is selected in the importer and task.imported_items() runs into a condition branch that supposedly should never be reached:
To Do
Documentation.