From bc0602bfcb52dd9da8d61cac7b093f07ffff33d0 Mon Sep 17 00:00:00 2001
From: Houkime <>
Date: Mon, 12 Feb 2024 20:47:33 +0000
Subject: [PATCH] fix(backup): rework caching so that there are rarer api calls

---
 selfprivacy_api/backup/__init__.py | 35 +++++++++++++++++-------------
 selfprivacy_api/backup/tasks.py    |  2 +-
 tests/test_backup.py               | 14 ++++++++----
 3 files changed, 31 insertions(+), 20 deletions(-)

diff --git a/selfprivacy_api/backup/__init__.py b/selfprivacy_api/backup/__init__.py
index 1c4c4e0..0a6d250 100644
--- a/selfprivacy_api/backup/__init__.py
+++ b/selfprivacy_api/backup/__init__.py
@@ -254,7 +254,7 @@ class Backups:
                 reason=reason,
             )
 
-            Backups._store_last_snapshot(service_name, snapshot)
+            Backups._on_new_snapshot_created(service_name, snapshot)
             if reason == BackupReason.AUTO:
                 Backups._prune_auto_snaps(service)
             service.post_restore()
@@ -265,7 +265,16 @@ class Backups:
         Jobs.update(job, status=JobStatus.FINISHED)
         if reason in [BackupReason.AUTO, BackupReason.PRE_RESTORE]:
             Jobs.set_expiration(job, AUTOBACKUP_JOB_EXPIRATION_SECONDS)
-        return snapshot
+        return Backups.sync_date_from_cache(snapshot)
+
+    @staticmethod
+    def sync_date_from_cache(snapshot: Snapshot) -> Snapshot:
+        """
+        Our snapshot creation dates are different from those on server by a tiny amount.
+        This is a convenience, maybe it is better to write a special comparison
+        function for snapshots
+        """
+        return Storage.get_cached_snapshot_by_id(snapshot.id)
 
     @staticmethod
     def _auto_snaps(service):
@@ -523,13 +532,12 @@ class Backups:
     @staticmethod
     def get_all_snapshots() -> List[Snapshot]:
         """Returns all snapshots"""
-        cached_snapshots = Storage.get_cached_snapshots()
-        if cached_snapshots:
-            return cached_snapshots
-        # TODO: the oldest snapshots will get expired faster than the new ones.
-        # How to detect that the end is missing?
+        # When we refresh our cache:
+        # 1. Manually
+        # 2. On timer
+        # 3. On new snapshot
+        # 4. On snapshot deletion
 
-        Backups.force_snapshot_cache_reload()
         return Storage.get_cached_snapshots()
 
     @staticmethod
@@ -548,15 +556,13 @@ class Backups:
     @staticmethod
     def forget_snapshots(snapshots: List[Snapshot]) -> None:
         """
-        Deletes a batch of snapshots from the repo and from cache
+        Deletes a batch of snapshots from the repo and syncs cache
         Optimized
         """
         ids = [snapshot.id for snapshot in snapshots]
         Backups.provider().backupper.forget_snapshots(ids)
 
-        # less critical
-        for snapshot in snapshots:
-            Storage.delete_cached_snapshot(snapshot)
+        Backups.force_snapshot_cache_reload()
 
     @staticmethod
     def forget_snapshot(snapshot: Snapshot) -> None:
@@ -593,12 +599,11 @@ class Backups:
         )
 
     @staticmethod
-    def _store_last_snapshot(service_id: str, snapshot: Snapshot) -> None:
+    def _on_new_snapshot_created(service_id: str, snapshot: Snapshot) -> None:
         """What do we do with a snapshot that is just made?"""
         # non-expiring timestamp of the last
         Storage.store_last_timestamp(service_id, snapshot)
-        # expiring cache entry
-        Storage.cache_snapshot(snapshot)
+        Backups.force_snapshot_cache_reload()
 
     # Autobackup
 
diff --git a/selfprivacy_api/backup/tasks.py b/selfprivacy_api/backup/tasks.py
index 6520c70..465b1a8 100644
--- a/selfprivacy_api/backup/tasks.py
+++ b/selfprivacy_api/backup/tasks.py
@@ -82,6 +82,6 @@ def automatic_backup():
         start_backup(service, BackupReason.AUTO)
 
 
-@huey.periodic_task(crontab(hour=SNAPSHOT_CACHE_TTL_HOURS))
+@huey.periodic_task(crontab(hour="*/" + str(SNAPSHOT_CACHE_TTL_HOURS)))
 def reload_snapshot_cache():
     Backups.force_snapshot_cache_reload()
diff --git a/tests/test_backup.py b/tests/test_backup.py
index 23569eb..f2b34a8 100644
--- a/tests/test_backup.py
+++ b/tests/test_backup.py
@@ -165,7 +165,7 @@ def test_reinit_after_purge(backups):
     Backups.erase_repo()
     assert Backups.is_initted() is False
     with pytest.raises(ValueError):
-        Backups.get_all_snapshots()
+        Backups.force_snapshot_cache_reload()
 
     Backups.init_repo()
     assert Backups.is_initted() is True
@@ -209,7 +209,11 @@ def test_backup_returns_snapshot(backups, dummy_service):
     snapshot = provider.backupper.start_backup(service_folders, name)
 
     assert snapshot.id is not None
-    assert len(snapshot.id) == len(Backups.get_all_snapshots()[0].id)
+
+    snapshots = provider.backupper.get_snapshots()
+    assert snapshots != []
+
+    assert len(snapshot.id) == len(snapshots[0].id)
     assert Backups.get_snapshot_by_id(snapshot.id) is not None
     assert snapshot.service_name == name
     assert snapshot.created_at is not None
@@ -472,10 +476,12 @@ def test_snapshots_caching(backups, dummy_service):
     cached_snapshots = Storage.get_cached_snapshots()
     assert len(cached_snapshots) == 0
 
+    # We do not assume that no snapshots means we need to reload the cache
     snapshots = Backups.get_snapshots(dummy_service)
-    assert len(snapshots) == 1
+    assert len(snapshots) == 0
+    # No cache reload happened
     cached_snapshots = Storage.get_cached_snapshots()
-    assert len(cached_snapshots) == 1
+    assert len(cached_snapshots) == 0
 
 
 def lowlevel_forget(snapshot_id):