Fix handling of multiple items (pinned recordings) having same mbid/msid #1831
Conversation
|
Hello @amCap1712! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:
Comment last updated at 2022-02-01 15:29:54 UTC |
listenbrainz/db/msid_mbid_mapping.py
Outdated
| @@ -79,47 +80,49 @@ def fetch_track_metadata_for_items(items: List[ModelT]) -> List[ModelT]: | |||
| Returns: | |||
| The given list of MsidMbidModel objects with updated track_metadata. | |||
| """ | |||
| msid_item_map, mbid_item_map = {}, {} | |||
| # it is possible to that multiple items have same msid/mbid. for example, pinning the | |||
There was a problem hiding this comment.
this is a bit unclear. Need to remove a stray "to"?
| "artist_name": metadata["payload"]["artist"], | ||
| "additional_info": { | ||
| "recording_msid": msid | ||
| for item in msid_item_map[msid]: |
There was a problem hiding this comment.
When I iterate one item via another, I always consider what happens if it gets out of sync for some reason - e.g. what if msid_item_map[msid] doesn't exist? One solution would be to use for item in msd_item_map.get(msid, [])
That being said, I see that similar code has been in use before this, so probably not a great issue.
There was a problem hiding this comment.
Nothing is deleted from MsB ever so don't think this should be an issue. Also, if this is missing we need to fix code elsewhere as well because other places in codebase assume presence of title, artist name.
|
|
||
| mapping_mbid_metadata, mapping_msid_metadata = load_recordings_from_mapping(mbid_item_map.keys(), msid_item_map.keys()) | ||
| _update_items_from_map(mbid_item_map, mapping_mbid_metadata) | ||
| _update_items_from_map(msid_item_map, mapping_msid_metadata) | ||
| return items | ||
|
|
||
|
|
||
| def _update_items_from_map(models: Dict[str, ModelT], metadatas: Dict) -> Dict[str, ModelT]: | ||
| for _id, item in models.items(): | ||
| def _update_items_from_map(models: Dict[str, Iterable[ModelT]], metadatas: Dict): |
There was a problem hiding this comment.
comment saying that it updates models in place would be helpful
| "track_name": metadata["title"], | ||
| "artist_name": metadata["artist"], | ||
| "release_name": metadata["release"], | ||
| "additional_info": { | ||
| "recording_msid": metadata["recording_msid"], | ||
| "recording_mbid": metadata["recording_mbid"], | ||
| "release_mbid": metadata["release_mbid"], | ||
| "artist_mbids": metadata["artist_mbids"] |
There was a problem hiding this comment.
not directly related to this PR, but are we always going to be sure that the metadata dict has all of these fields?
There was a problem hiding this comment.
Yes, these are the exact fields selected in load_recordings_from_mapping (above in this file) and this function is only called with data from that query.
It is possible to that multiple items have same msid/mbid. for example, pinning the same recording twice. Since the dict is keyed by mbid/msid the corresponding value should be a iterable of all items having that mbid so that such items are handled. Added test for the same.