diff --git a/sublime/adapters/filesystem/adapter.py b/sublime/adapters/filesystem/adapter.py index f352366..b8f5a1d 100644 --- a/sublime/adapters/filesystem/adapter.py +++ b/sublime/adapters/filesystem/adapter.py @@ -155,15 +155,33 @@ class FilesystemAdapter(CachingAdapter): return obj + def _compute_song_filename(self, cache_info: models.CacheInfo) -> Path: + if path_str := cache_info.path: + # Make sure that the path is somewhere in the cache directory and a + # malicious server (or MITM attacker) isn't trying to override files in + # other parts of the system. + path = self.music_dir.joinpath(path_str) + if self.music_dir in path.parents: + return path + + # Fall back to using the song file hash as the filename. This shouldn't happen + # with good servers, but just to be safe. + return self.music_dir.joinpath(cache_info.file_hash) + # Data Retrieval Methods # ================================================================================== def get_cached_status(self, song: API.Song) -> SongCacheStatus: try: song_model = self.get_song_details(song.id) file = song_model.file - if file.valid and self.music_dir.joinpath(file.file_hash).exists(): - # TODO (#74): check if path is permanently cached - return SongCacheStatus.CACHED + if self._compute_song_filename(file).exists(): + if file.valid: + if file.cache_permanently: + return SongCacheStatus.PERMANENTLY_CACHED + return SongCacheStatus.CACHED + + # The file is on disk, but marked as stale. + return SongCacheStatus.CACHED_STALE except Exception: pass @@ -204,7 +222,7 @@ class FilesystemAdapter(CachingAdapter): try: if (song_file := song.file) and ( - filename := self.music_dir.joinpath(str(song_file.file_hash)) + filename := self._compute_song_filename(song_file) ): if song_file.valid and filename.exists(): return str(filename) @@ -513,10 +531,10 @@ class FilesystemAdapter(CachingAdapter): song_data = { "id": api_song.id, "title": api_song.title, - "path": getattr(api_song, "path", None), "track": getattr(api_song, "track", None), "year": getattr(api_song, "year", None), "duration": getattr(api_song, "duration", None), + "parent_id": api_song.parent_id, # Ingest the FKs. "genre": ingest_genre_data(g) if (g := api_song.genre) else None, "artist": ingest_artist_data(ar) if (ar := api_song.artist) else None, @@ -526,7 +544,11 @@ class FilesystemAdapter(CachingAdapter): ) if api_song.cover_art else None, - "parent_id": api_song.parent_id, + "file": self._do_ingest_new_data( + KEYS.SONG_FILE, params=(api_song.id,), data=(api_song.path, None) + ) + if api_song.path + else None, } song, created = models.Song.get_or_create( @@ -662,13 +684,6 @@ class FilesystemAdapter(CachingAdapter): elif data_key == KEYS.SONG_FILE: cache_info_extra["file_id"] = params[0] - if data is not None: - file_hash = compute_file_hash(data) - cache_info_extra["file_hash"] = file_hash - - # Copy the actual cover art file - shutil.copy(str(data), str(self.music_dir.joinpath(file_hash))) - elif data_key == KEYS.SONG_FILE_PERMANENT: cache_info_extra["cache_permanently"] = True @@ -697,9 +712,25 @@ class FilesystemAdapter(CachingAdapter): # Special handling for Song if data_key == KEYS.SONG_FILE: - song = models.Song.get_by_id(params[0]) - song.file = cache_info - song.save() + path, buffer_filename = data + + if path: + cache_info.path = path + + if buffer_filename: + cache_info.file_hash = compute_file_hash(buffer_filename) + + # Copy the actual song file from the download buffer dir to the cache + # dir. + filename = self._compute_song_filename(cache_info) + filename.parent.mkdir(parents=True, exist_ok=True) + shutil.copy(str(buffer_filename), str(filename)) + + cache_info.save() + + # song = models.Song.get_by_id(params[0]) + # song.file = cache_info + # song.save() return return_val if return_val is not None else cache_info @@ -752,16 +783,16 @@ class FilesystemAdapter(CachingAdapter): logging.debug( f"_do_delete_data params={params} params_hash={params_hash} data_key={data_key}" # noqa: 502 ) + cache_info = models.CacheInfo.get_or_none( + models.CacheInfo.cache_key == data_key, + models.CacheInfo.params_hash == params_hash, + ) if data_key == CachingAdapter.CachedDataKey.COVER_ART_FILE: - cache_info = models.CacheInfo.get_or_none( - models.CacheInfo.cache_key == data_key, - models.CacheInfo.params_hash == params_hash, - ) if cache_info: - cover_art_file = self.cover_art_dir.joinpath(str(cache_info.file_hash)) - cover_art_file.unlink(missing_ok=True) - cache_info.delete() + self.cover_art_dir.joinpath(str(cache_info.file_hash)).unlink( + missing_ok=True + ) elif data_key == CachingAdapter.CachedDataKey.PLAYLIST_DETAILS: # Delete the playlist and corresponding cover art. @@ -774,16 +805,7 @@ class FilesystemAdapter(CachingAdapter): playlist.delete_instance() elif data_key == CachingAdapter.CachedDataKey.SONG_FILE: - cache_info = models.CacheInfo.get_or_none( - models.CacheInfo.cache_key == data_key, - models.CacheInfo.params_hash == params_hash, - ) if cache_info: - cover_art_file = self.music_dir.joinpath(str(cache_info.file_hash)) - cover_art_file.unlink(missing_ok=True) - cache_info.delete() + self._compute_song_filename(cache_info).unlink(missing_ok=True) - models.CacheInfo.delete().where( - models.CacheInfo.cache_key == data_key, - models.CacheInfo.params_hash == params_hash, - ).execute() + cache_info.delete_instance() diff --git a/sublime/adapters/filesystem/models.py b/sublime/adapters/filesystem/models.py index fcb438d..a4e65b7 100644 --- a/sublime/adapters/filesystem/models.py +++ b/sublime/adapters/filesystem/models.py @@ -33,17 +33,18 @@ class CacheInfo(BaseModel): valid = BooleanField(default=False) cache_key = CacheConstantsField() params_hash = TextField() + # TODO (#2) actually use this for cache expiry. last_ingestion_time = TzDateTimeField(null=False) - file_id = TextField(null=True) - file_hash = TextField(null=True) - # TODO store path - cache_permanently = BooleanField(null=True) - - # TODO some sort of expiry? class Meta: indexes = ((("cache_key", "params_hash"), True),) + # Used for cached files. + file_id = TextField(null=True) + file_hash = TextField(null=True) + path = TextField(null=True) + cache_permanently = BooleanField(null=True) + class Genre(BaseModel): name = TextField(unique=True, primary_key=True) @@ -132,9 +133,6 @@ class Song(BaseModel): title = TextField() duration = DurationField(null=True) - # TODO move path to file foreign key - path = TextField(null=True) - parent_id = TextField(null=True) album = ForeignKeyField(Album, null=True, backref="songs") artist = ForeignKeyField(Artist, null=True) @@ -143,6 +141,13 @@ class Song(BaseModel): # figure out how to deal with different transcodings, etc. file = ForeignKeyField(CacheInfo, null=True) + @property + def path(self) -> Optional[str]: + try: + return self.file.path + except Exception: + return None + _cover_art = ForeignKeyField(CacheInfo, null=True) @property diff --git a/sublime/adapters/manager.py b/sublime/adapters/manager.py index 2ce6510..85b7e30 100644 --- a/sublime/adapters/manager.py +++ b/sublime/adapters/manager.py @@ -803,7 +803,7 @@ class AdapterManager: AdapterManager._instance.caching_adapter.ingest_new_data( CachingAdapter.CachedDataKey.SONG_FILE, (song_id,), - song_tmp_filename, + (None, song_tmp_filename), ) on_song_download_complete(song_id) diff --git a/sublime/util.py b/sublime/util.py index 6300673..1ee07be 100644 --- a/sublime/util.py +++ b/sublime/util.py @@ -6,6 +6,7 @@ from sublime.adapters import AlbumSearchQuery def params_hash(*params: Any) -> str: + # TODO determine if we ever have more than one parameter. # Special handling for AlbumSearchQuery objects. # TODO figure out if I can optimize this if len(params) > 0 and isinstance(params[0], AlbumSearchQuery): diff --git a/tests/adapter_tests/filesystem_adapter_tests.py b/tests/adapter_tests/filesystem_adapter_tests.py index 47fe367..5b97a84 100644 --- a/tests/adapter_tests/filesystem_adapter_tests.py +++ b/tests/adapter_tests/filesystem_adapter_tests.py @@ -341,8 +341,8 @@ def test_invalidate_song_file(cache_adapter: FilesystemAdapter): cache_adapter.ingest_new_data( KEYS.COVER_ART_FILE, ("s1", "song"), MOCK_ALBUM_ART, ) - cache_adapter.ingest_new_data(KEYS.SONG_FILE, ("1",), MOCK_SONG_FILE) - cache_adapter.ingest_new_data(KEYS.SONG_FILE, ("2",), MOCK_SONG_FILE2) + cache_adapter.ingest_new_data(KEYS.SONG_FILE, ("1",), (None, MOCK_SONG_FILE)) + cache_adapter.ingest_new_data(KEYS.SONG_FILE, ("2",), (None, MOCK_SONG_FILE2)) cache_adapter.invalidate_data(KEYS.SONG_FILE, ("1",)) cache_adapter.invalidate_data(KEYS.COVER_ART_FILE, ("s1", "song")) @@ -354,7 +354,24 @@ def test_invalidate_song_file(cache_adapter: FilesystemAdapter): cache_adapter.get_cover_art_uri("s1", "file") # Make sure it didn't delete the other song. - assert cache_adapter.get_song_uri("2", "file").endswith(MOCK_SONG_FILE2_HASH) + assert cache_adapter.get_song_uri("2", "file").endswith("song2.mp3") + + +def test_malformed_song_path(cache_adapter: FilesystemAdapter): + cache_adapter.ingest_new_data(KEYS.SONG, ("1",), MOCK_SUBSONIC_SONGS[1]) + cache_adapter.ingest_new_data(KEYS.SONG, ("2",), MOCK_SUBSONIC_SONGS[0]) + cache_adapter.ingest_new_data( + KEYS.SONG_FILE, ("1",), ("/malformed/path", MOCK_SONG_FILE) + ) + cache_adapter.ingest_new_data( + KEYS.SONG_FILE, ("2",), ("fine/path/song2.mp3", MOCK_SONG_FILE2) + ) + + song_uri = cache_adapter.get_song_uri("1", "file") + assert song_uri.endswith(f"/music/{MOCK_SONG_FILE_HASH}") + + song_uri2 = cache_adapter.get_song_uri("2", "file") + assert song_uri2.endswith("fine/path/song2.mp3") def test_delete_playlists(cache_adapter: FilesystemAdapter): @@ -401,7 +418,7 @@ def test_delete_playlists(cache_adapter: FilesystemAdapter): def test_delete_song_data(cache_adapter: FilesystemAdapter): cache_adapter.ingest_new_data(KEYS.SONG, ("1",), MOCK_SUBSONIC_SONGS[1]) - cache_adapter.ingest_new_data(KEYS.SONG_FILE, ("1",), MOCK_SONG_FILE) + cache_adapter.ingest_new_data(KEYS.SONG_FILE, ("1",), (None, MOCK_SONG_FILE)) cache_adapter.ingest_new_data( KEYS.COVER_ART_FILE, ("s1",), MOCK_ALBUM_ART, )