Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix #1781: compact_items expects dict but sometimes passed list #1862

Merged
merged 3 commits into from
Feb 14, 2024

Conversation

jacobswe
Copy link
Contributor

@jacobswe jacobswe commented Feb 1, 2024

This pull request fixes #1781 by implementing a simple type check before invoking compact_items. I wasn't able to find any docs on preferences for contributing, so please shoot this back to me if any changes are needed!

Summary:
With Spotify connected, running the mass.search service causes an error to occur if track is passed as a media_type. This occurs because the compact_items function in mass/services.py expects and is type hinted to exclusively accept a dictionary, but when called from handle_search or recursively from within compact_items, no check is performed to ensure the correct type is passed. This usually does not cause errors, but in some circumstances lists are passed, in the case of the below example for the external_ids field.

Recreation YAML:

service: mass.search
data:
  limit: 5
  name: Pink Floyd
  media_type:
    - track

Error:
The error TypeError: pop expected at most 1 argument, got 2 is thrown because list.pop expects a single index whereas dict.pop expects a key and optionally a default value. This causes improper arguments to be passed when a list is given to compact_items.

Working Local Demonstration:
image

@jacobswe
Copy link
Contributor Author

jacobswe commented Feb 8, 2024

Hey HA music wizards, just wondering if I can do anything on this. Totally get you're probably busy but let me know! It would be cool to get this enabled on the public release.

@@ -59,14 +59,16 @@ def compact_item(item: dict[str, Any]) -> dict[str, Any]:
item[key] = compact_item(value)
elif isinstance(value, list):
for subitem in value:
compact_item(subitem)
if isinstance(subitem, dict):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's always better to guard and breakout early. So in this case:

if not isinstance(subitem, dict):
continue

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

btw: maybe we also want to catch lists here ?

@jacobswe
Copy link
Contributor Author

jacobswe commented Feb 9, 2024

Thanks, happy to make that update, and I'll think about how to capture lists as well. I'll have time to push some changes on Sunday probably. Appreciate the review!

@jacobswe
Copy link
Contributor Author

Pushed the guard clauses and verified working. I don't think anything is needed for lists here, they should just work I think? If required, can we do in separate merge?

@marcelveldt
Copy link
Member

Thanks!

@marcelveldt marcelveldt merged commit e610731 into music-assistant:main Feb 14, 2024
2 of 3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

mass.search service crashes when mass server response contains tracks from spotify
2 participants