Caching Bug

[2e0byo]

Whilst finishing the test suite I think I've found a bug. The duplicate cache check in _lookup_track can be reached, since the album might have been cached before hand:

tracks = self._album_cache.get(album_uri)
if tracks is None:
    tracks = self._get_album_tracks(session, album_id)
track = [t for t in tracks if t.id == int(parts[4])][0]

library.py

The return value of _get_album_tracks is a list of Tidal tracks. The return value of _album_cache.get() is a list of Mopidy tracks. Mopidy tracks don't have an id, but they do have a uri, so we can check against that. Additionally the remainder of the code would try to make a mopidy track from a mopidy track if there's a cache hit; that might work but isn't needed.

AFAICS this bug is currently reachable; it must just be that nobody's tried to lookup a track for an album they already have (or they haven't noticed).

Additionally, the current setup will often cache the same track twice---once as a track, the other time as an album. But it will only do this if the album is looked up, despite looking up the album every time in order to get the track. My PR has an xfailed test for looking up two different tracks on the same album, which could be cached if we moved the cache/lookup into get_album_tracks. Since @BlackLight moved all the caching into the calling funciton I don't want to just move it back to handle this edge case. Thus I can see a few solutions for this bug:

1. drop the album cache check and just lookup again for tracks. It must be quite a rare occurance, since nobody's complained yet and it's currently broken if the album has been looked up before.
2. fix the cache check by comparing uris and return if matched
3. fix the cache check and add caching for the album as well.

For now I'll do #2 over in my PR.

[2e0byo]

This bug is in error; I'd missed the explicit update of the track cache [and managed to reach the code by a broken mock]. Apologies. Closing and removing the unreachable caching logic.

[blacklight]

Thanks for spotting this. I couldn't recall why I had the explicit logic to manage tidal:track:0:0:0 in playlists, but after a couple of tests I recalled why I did that: Iris needs a playlists properly populated with track objects in order to infer the number of tracks, but if we retrieve the playlist metadata only (without the tracks) we don't have that info. That's the reasoning behind populating playlists with mock tracks - as many as the actual number of tracks that is supposed to be there.

However, when we actually retrieve the playlist tracks, we should check that the cached playlist doesn't have mock track URIs - if that's the case, then we have to retrieve the tracks and overwrite the cache, otherwise Iris would show a bunch of invalid tracks.

[2e0byo]

Right, thanks! I blindly tested that but did wonder why it was done. I'll have a look about making it more explicit in the tests.

That said this particular issue was in error I think---I think that line really was unreachable. I'll fire up iris and do a few tests in a mo.

Many thanks for all the help as I work out how this codebase works btw.