Use a deterministic hash

sublime/adapters/filesystem/adapter.py

@@ -1,4 +1,7 @@
+import hashlib
+import json
 import logging
+from base64 import b64encode
 from dataclasses import asdict
 from datetime import datetime
 from pathlib import Path
@@ -54,6 +57,11 @@ class FilesystemAdapter(CachingAdapter):
     # =========================================================================
     can_service_requests: bool = True
 
+    # Data Helper Methods
+    # =========================================================================
+    def _params_hash(self, *params: Any) -> str:
+        return hashlib.sha1(bytes(json.dumps(params), 'utf8')).hexdigest()
+
     # Data Retrieval Methods
     # =========================================================================
     can_get_playlists: bool = True
@@ -87,7 +95,8 @@ class FilesystemAdapter(CachingAdapter):
         function_name = CachingAdapter.FunctionNames.GET_PLAYLIST_DETAILS
         cache_info = models.CacheInfo.get_or_none(
             models.CacheInfo.query_name == function_name,
-            params_hash=hash((playlist_id, ), ))
+            params_hash=self._params_hash(playlist_id),
+        )
         if not cache_info:
             raise CacheMissError(partial_data=playlist)
 
@@ -106,7 +115,7 @@ class FilesystemAdapter(CachingAdapter):
 
         models.CacheInfo.insert(
             query_name=function,
-            params_hash=hash(params),
+            params_hash=self._params_hash(*params),
             last_ingestion_time=datetime.now(),
         ).on_conflict_replace().execute()
 

sublime/adapters/filesystem/models.py

@@ -3,6 +3,7 @@ from typing import Any, Optional, Sequence
 
 from peewee import (
     BooleanField,
+    CompositeKey,
     DoubleField,
     ensure_tuple,
     ForeignKeyField,
@@ -23,14 +24,6 @@ database = SqliteDatabase(None)
 
 # Custom Fields
 # =============================================================================
-class DurationField(DoubleField):
-    def db_value(self, value: timedelta) -> Optional[float]:
-        return value.total_seconds() if value else None
-
-    def python_value(self, value: Optional[float]) -> Optional[timedelta]:
-        return timedelta(seconds=value) if value else None
-
-
 class CacheConstantsField(TextField):
     def db_value(self, value: CachingAdapter.FunctionNames) -> str:
         return value.value
@@ -39,6 +32,14 @@ class CacheConstantsField(TextField):
         return CachingAdapter.FunctionNames(value)
 
 
+class DurationField(DoubleField):
+    def db_value(self, value: timedelta) -> Optional[float]:
+        return value.total_seconds() if value else None
+
+    def python_value(self, value: Optional[float]) -> Optional[timedelta]:
+        return timedelta(seconds=value) if value else None
+
+
 class TzDateTimeField(TextField):
     def db_value(self, value: Optional[datetime]) -> Optional[str]:
         return value.isoformat() if value else None
@@ -204,10 +205,13 @@ class Song(BaseModel):
 
 
 class CacheInfo(BaseModel):
-    query_name = CacheConstantsField(unique=True, primary_key=True)
-    params_hash = IntegerField(null=False)
+    query_name = CacheConstantsField()
+    params_hash = TextField()
     last_ingestion_time = TzDateTimeField(null=False)
 
+    class Meta:
+        primary_key = CompositeKey('query_name', 'params_hash')
+
 
 class Playlist(BaseModel):
     id = TextField(unique=True, primary_key=True)

sublime/app.py

@@ -134,6 +134,9 @@ class SublimeMusicApp(Gtk.Application):
         self.window.show_all()
         self.window.present()
 
+        # Load the state for the server, if it exists.
+        self.app_config.load_state()
+
         # If there is no current server, show the dialog to select a server.
         if self.app_config.server is None:
             self.show_configure_servers_dialog()

sublime/config.py

@@ -1,4 +1,5 @@
 import hashlib
+import logging
 import os
 import pickle
 from dataclasses import asdict, dataclass, field, fields
@@ -68,7 +69,7 @@ class ServerConfiguration:
         >>> sc.strhash()
         '6df23dc03f9b54cc38a0fc1483df6e21'
         """
-        server_info = (self.name + self.server_address + self.username)
+        server_info = self.name + self.server_address + self.username
         return hashlib.md5(server_info.encode('utf-8')).hexdigest()
 
 
@@ -100,6 +101,7 @@ class AppConfiguration:
 
         config = AppConfiguration(**args)
         config.filename = filename
+
         return config
 
     def __post_init__(self):
@@ -136,16 +138,25 @@ class AppConfiguration:
         if not server:
             return UIState()
 
-        # If already retrieved, and the server hasn't changed, then return the
-        # state. Don't use strhash because that is much more expensive of an
-        # operation.
-        if self._current_server_hash != hash(server) or not self._state:
-            self._current_server_hash = hash(server)
+        # If the server has changed, then retrieve the new server's state.
+        # TODO: if things are slow, then use a different hash
+        if self._current_server_hash != server.strhash():
+            self.load_state()
+
+        return self._state
+
+    def load_state(self):
+        if not self.server:
+            return
+
+        self._current_server_hash = self.server.strhash()
         if self.state_file_location.exists():
             try:
                 with open(self.state_file_location, 'rb') as f:
                     self._state = UIState(**pickle.load(f))
             except Exception:
+                logging.warning(
+                    f"Couldn't load state from {self.state_file_location}")
                 # Just ignore any errors, it is only UI state.
                 self._state = UIState()
 
@@ -155,8 +166,6 @@ class AppConfiguration:
         CacheManager.reset(self)
         AdapterManager.reset(self)
 
-        return self._state
-
     @property
     def state_file_location(self) -> Path:
         assert self.server is not None

tests/adapter_tests/filesystem_adapter_tests.py

@@ -178,7 +178,7 @@ def test_caching_get_playlist_then_details(cache_adapter: FilesystemAdapter):
         FilesystemAdapter.FunctionNames.GET_PLAYLISTS,
         (),
         [
-            SubsonicAPI.Playlist('1', 'test1', comment='comment'),
+            SubsonicAPI.Playlist('1', 'test1'),
             SubsonicAPI.Playlist('2', 'test2'),
         ],
     )
@@ -192,3 +192,33 @@ def test_caching_get_playlist_then_details(cache_adapter: FilesystemAdapter):
         assert e.partial_data
         assert e.partial_data.id == '1'
         assert e.partial_data.name == 'test1'
+
+    # Simulate getting playlist details for id=1, then id=2
+    songs = [
+        SubsonicAPI.Song(
+            '3',
+            'Song 3',
+            parent='foo',
+            album='foo',
+            artist='foo',
+            duration=timedelta(seconds=10.2),
+            path='/foo/song3.mp3',
+        ),
+    ]
+    cache_adapter.ingest_new_data(
+        FilesystemAdapter.FunctionNames.GET_PLAYLIST_DETAILS,
+        ('1', ),
+        SubsonicAPI.PlaylistWithSongs('1', 'test1', songs=songs),
+    )
+
+    cache_adapter.ingest_new_data(
+        FilesystemAdapter.FunctionNames.GET_PLAYLIST_DETAILS,
+        ('2', ),
+        SubsonicAPI.PlaylistWithSongs('2', 'test2', songs=songs),
+    )
+
+    # Going back and getting playlist details for the first one should not
+    # cache miss.
+    playlist = cache_adapter.get_playlist_details('1')
+    assert playlist.id == '1'
+    assert playlist.name == 'test1'