From 50e03b4a56d02ce69b1d2e5e01dcac6827415237 Mon Sep 17 00:00:00 2001
From: Carlos Herrero <26092748+hbcarlos@users.noreply.github.com>
Date: Mon, 9 Oct 2023 18:09:22 +0200
Subject: [PATCH] Review

---
 tests/test_file_store.py             |  57 ++++++-----
 tests/test_sqlite_store.py           | 146 ++++++++++-----------------
 ypy_websocket/stores/__init__.py     |   2 +-
 ypy_websocket/stores/base_store.py   |   5 +-
 ypy_websocket/stores/file_store.py   |  10 +-
 ypy_websocket/stores/sqlite_store.py |  11 +-
 ypy_websocket/stores/utils.py        |   2 +-
 7 files changed, 105 insertions(+), 128 deletions(-)

diff --git a/tests/test_file_store.py b/tests/test_file_store.py
index 1f4709e..defe8bf 100644
--- a/tests/test_file_store.py
+++ b/tests/test_file_store.py
@@ -1,3 +1,5 @@
+from __future__ import annotations
+
 import struct
 import time
 from pathlib import Path
@@ -5,7 +7,7 @@
 import anyio
 import pytest
 
-from ypy_websocket.stores import DocExists, FileYStore
+from ypy_websocket.stores import FileYStore, YDocExists, YDocNotFound
 from ypy_websocket.yutils import Decoder, write_var_uint
 
 
@@ -23,21 +25,23 @@ async def _inner(path: str, version: int) -> None:
 
 @pytest.fixture
 def add_document():
-    async def _inner(path: str, doc_path: str, version: int, data: bytes = b"") -> None:
+    async def _inner(path: str, doc_path: str, version: int, data: bytes | None = None) -> None:
         file_path = Path(path, (doc_path + ".y"))
         await anyio.Path(file_path.parent).mkdir(parents=True, exist_ok=True)
 
         async with await anyio.open_file(file_path, "ab") as f:
             version_bytes = f"VERSION:{version}\n".encode()
             await f.write(version_bytes)
-            data_len = write_var_uint(len(data))
-            await f.write(data_len + data)
-            metadata = b""
-            metadata_len = write_var_uint(len(metadata))
-            await f.write(metadata_len + metadata)
-            timestamp = struct.pack("<d", time.time())
-            timestamp_len = write_var_uint(len(timestamp))
-            await f.write(timestamp_len + timestamp)
+
+            if data is not None:
+                data_len = write_var_uint(len(data))
+                await f.write(data_len + data)
+                metadata = b""
+                metadata_len = write_var_uint(len(metadata))
+                await f.write(metadata_len + metadata)
+                timestamp = struct.pack("<d", time.time())
+                timestamp_len = write_var_uint(len(timestamp))
+                await f.write(timestamp_len + timestamp)
 
     return _inner
 
@@ -54,7 +58,7 @@ async def test_initialization(tmp_path):
     version_path = Path(path / "__version__")
     async with await anyio.open_file(version_path, "rb") as f:
         version = int(await f.readline())
-        assert FileYStore.version == version
+    assert FileYStore.version == version
 
 
 @pytest.mark.anyio
@@ -73,7 +77,7 @@ async def test_initialization_with_old_store(tmp_path, create_store):
     version_path = Path(path / "__version__")
     async with await anyio.open_file(version_path, "rb") as f:
         version = int(await f.readline())
-        assert FileYStore.version == version
+    assert FileYStore.version == version
 
 
 @pytest.mark.anyio
@@ -81,7 +85,7 @@ async def test_initialization_with_existing_store(tmp_path, create_store, add_do
     path = tmp_path / "tmp"
     doc_path = "test.txt"
 
-    # Create a store with an old version
+    # Create a store
     await create_store(path, FileYStore.version)
     await add_document(path, doc_path, 0)
 
@@ -105,7 +109,7 @@ async def test_exists(tmp_path, create_store, add_document):
     path = tmp_path / "tmp"
     doc_path = "test.txt"
 
-    # Create a store with an old version
+    # Create a store
     await create_store(path, FileYStore.version)
     await add_document(path, doc_path, 0)
 
@@ -140,7 +144,7 @@ async def test_list(tmp_path, create_store, add_document):
     count = 0
     async for doc in store.list():
         count += 1
-        # assert doc in [doc1, doc2]
+        assert doc in [doc1, doc2]
 
     assert count == 2
 
@@ -150,7 +154,7 @@ async def test_get(tmp_path, create_store, add_document):
     path = tmp_path / "tmp"
     doc_path = "test.txt"
 
-    # Create a store with an old version
+    # Create a store
     await create_store(path, FileYStore.version)
     await add_document(path, doc_path, 0)
 
@@ -173,7 +177,7 @@ async def test_create(tmp_path, create_store, add_document):
     path = tmp_path / "tmp"
     doc_path = "test.txt"
 
-    # Create a store with an old version
+    # Create a store
     await create_store(path, FileYStore.version)
     await add_document(path, doc_path, 0)
 
@@ -194,7 +198,7 @@ async def test_create(tmp_path, create_store, add_document):
         version = int(await f.readline())
         assert version == 0
 
-    with pytest.raises(DocExists) as e:
+    with pytest.raises(YDocExists) as e:
         await store.create(doc_path, 0)
     assert str(e.value) == f"The document {doc_path} already exists."
 
@@ -204,7 +208,7 @@ async def test_remove(tmp_path, create_store, add_document):
     path = tmp_path / "tmp"
     doc_path = "test.txt"
 
-    # Create a store with an old version
+    # Create a store
     await create_store(path, FileYStore.version)
     await add_document(path, doc_path, 0)
 
@@ -214,13 +218,15 @@ async def test_remove(tmp_path, create_store, add_document):
 
     assert store.initialized
 
+    assert await store.exists(doc_path)
     await store.remove(doc_path)
     assert not await store.exists(doc_path)
 
     new_doc = "new_doc.path"
     assert not await store.exists(new_doc)
-
-    await store.remove(new_doc)
+    with pytest.raises(YDocNotFound) as e:
+        await store.remove(new_doc)
+    assert str(e.value) == f"The document {new_doc} doesn't exists."
     assert not await store.exists(new_doc)
 
 
@@ -230,7 +236,7 @@ async def test_read(tmp_path, create_store, add_document):
     doc_path = "test.txt"
     update = b"foo"
 
-    # Create a store with an old version
+    # Create a store
     await create_store(path, FileYStore.version)
     await add_document(path, doc_path, 0, update)
 
@@ -253,7 +259,7 @@ async def test_write(tmp_path, create_store, add_document):
     path = tmp_path / "tmp"
     doc_path = "test.txt"
 
-    # Create a store with an old version
+    # Create a store
     await create_store(path, FileYStore.version)
     await add_document(path, doc_path, 0)
 
@@ -281,8 +287,7 @@ async def test_write(tmp_path, create_store, add_document):
         for u in Decoder(data).read_messages():
             if i == 0:
                 count += 1
-                # The fixture add_document inserts an empty update
-                assert u in [b"", update]
+                assert u == update
             i = (i + 1) % 3
 
-        assert count == 2
+        assert count == 1
diff --git a/tests/test_sqlite_store.py b/tests/test_sqlite_store.py
index b636aee..2abb1c9 100644
--- a/tests/test_sqlite_store.py
+++ b/tests/test_sqlite_store.py
@@ -1,9 +1,11 @@
+from __future__ import annotations
+
 import time
 
 import aiosqlite
 import pytest
 
-from ypy_websocket.stores import DocExists, SQLiteYStore
+from ypy_websocket.stores import SQLiteYStore, YDocExists, YDocNotFound
 
 
 @pytest.fixture
@@ -28,16 +30,17 @@ async def _inner(path: str, version: int, tables: bool = True) -> None:
 
 @pytest.fixture
 def add_document():
-    async def _inner(path: str, doc_path: str, version: int, data: bytes = b"") -> None:
+    async def _inner(path: str, doc_path: str, version: int, data: bytes | None = None) -> None:
         async with aiosqlite.connect(path) as db:
             await db.execute(
                 "INSERT INTO documents VALUES (?, ?)",
                 (doc_path, version),
             )
-            await db.execute(
-                "INSERT INTO yupdates VALUES (?, ?, ?, ?)",
-                (doc_path, data, b"", time.time()),
-            )
+            if data is not None:
+                await db.execute(
+                    "INSERT INTO yupdates VALUES (?, ?, ?, ?)",
+                    (doc_path, data, b"", time.time()),
+                )
             await db.commit()
 
     return _inner
@@ -52,22 +55,7 @@ async def test_initialization(tmp_path):
 
     assert store.initialized
 
-    async with aiosqlite.connect(path) as db:
-        cursor = await db.execute("pragma user_version")
-        version = (await cursor.fetchone())[0]
-        assert store.version == version
-
-        cursor = await db.execute(
-            "SELECT count(*) FROM sqlite_master WHERE type='table' AND name='documents'"
-        )
-        res = await cursor.fetchone()
-        assert res[0] == 1
-
-        cursor = await db.execute(
-            "SELECT count(*) FROM sqlite_master WHERE type='table' AND name='yupdates'"
-        )
-        res = await cursor.fetchone()
-        assert res[0] == 1
+    await _check_db(path, store)
 
 
 @pytest.mark.anyio
@@ -83,29 +71,14 @@ async def test_initialization_with_old_database(tmp_path, create_database):
 
     assert store.initialized
 
-    async with aiosqlite.connect(path) as db:
-        cursor = await db.execute("pragma user_version")
-        version = (await cursor.fetchone())[0]
-        assert store.version == version
-
-        cursor = await db.execute(
-            "SELECT count(*) FROM sqlite_master WHERE type='table' AND name='documents'"
-        )
-        res = await cursor.fetchone()
-        assert res[0] == 1
-
-        cursor = await db.execute(
-            "SELECT count(*) FROM sqlite_master WHERE type='table' AND name='yupdates'"
-        )
-        res = await cursor.fetchone()
-        assert res[0] == 1
+    await _check_db(path, store)
 
 
 @pytest.mark.anyio
 async def test_initialization_with_empty_database(tmp_path, create_database):
     path = tmp_path / "tmp.db"
 
-    # Create a database with an old version
+    # Create a database
     await create_database(path, SQLiteYStore.version, False)
 
     store = SQLiteYStore(str(path))
@@ -114,22 +87,7 @@ async def test_initialization_with_empty_database(tmp_path, create_database):
 
     assert store.initialized
 
-    async with aiosqlite.connect(path) as db:
-        cursor = await db.execute("pragma user_version")
-        version = (await cursor.fetchone())[0]
-        assert store.version == version
-
-        cursor = await db.execute(
-            "SELECT count(*) FROM sqlite_master WHERE type='table' AND name='documents'"
-        )
-        res = await cursor.fetchone()
-        assert res[0] == 1
-
-        cursor = await db.execute(
-            "SELECT count(*) FROM sqlite_master WHERE type='table' AND name='yupdates'"
-        )
-        res = await cursor.fetchone()
-        assert res[0] == 1
+    await _check_db(path, store)
 
 
 @pytest.mark.anyio
@@ -137,7 +95,7 @@ async def test_initialization_with_existing_database(tmp_path, create_database,
     path = tmp_path / "tmp.db"
     doc_path = "test.txt"
 
-    # Create a database with an old version
+    # Create a database
     await create_database(path, SQLiteYStore.version)
     await add_document(path, doc_path, 0)
 
@@ -147,30 +105,7 @@ async def test_initialization_with_existing_database(tmp_path, create_database,
 
     assert store.initialized
 
-    async with aiosqlite.connect(path) as db:
-        cursor = await db.execute("pragma user_version")
-        version = (await cursor.fetchone())[0]
-        assert store.version == version
-
-        cursor = await db.execute(
-            "SELECT count(*) FROM sqlite_master WHERE type='table' AND name='documents'"
-        )
-        res = await cursor.fetchone()
-        assert res[0] == 1
-
-        cursor = await db.execute(
-            "SELECT count(*) FROM sqlite_master WHERE type='table' AND name='yupdates'"
-        )
-        res = await cursor.fetchone()
-        assert res[0] == 1
-
-        cursor = await db.execute(
-            "SELECT path, version FROM documents WHERE path = ?",
-            (doc_path,),
-        )
-        res = await cursor.fetchone()
-        assert res[0] == doc_path
-        assert res[1] == 0
+    await _check_db(path, store, doc_path)
 
 
 @pytest.mark.anyio
@@ -223,7 +158,7 @@ async def test_get(tmp_path, create_database, add_document):
     path = tmp_path / "tmp.db"
     doc_path = "test.txt"
 
-    # Create a database with an old version
+    # Create a database
     await create_database(path, SQLiteYStore.version)
     await add_document(path, doc_path, 0)
 
@@ -246,7 +181,7 @@ async def test_create(tmp_path, create_database, add_document):
     path = tmp_path / "tmp.db"
     doc_path = "test.txt"
 
-    # Create a database with an old version
+    # Create a database
     await create_database(path, SQLiteYStore.version)
     await add_document(path, doc_path, 0)
 
@@ -267,7 +202,7 @@ async def test_create(tmp_path, create_database, add_document):
         assert res[0] == new_doc
         assert res[1] == 0
 
-    with pytest.raises(DocExists) as e:
+    with pytest.raises(YDocExists) as e:
         await store.create(doc_path, 0)
     assert str(e.value) == f"The document {doc_path} already exists."
 
@@ -277,7 +212,7 @@ async def test_remove(tmp_path, create_database, add_document):
     path = tmp_path / "tmp.db"
     doc_path = "test.txt"
 
-    # Create a database with an old version
+    # Create a database
     await create_database(path, SQLiteYStore.version)
     await add_document(path, doc_path, 0)
 
@@ -287,13 +222,15 @@ async def test_remove(tmp_path, create_database, add_document):
 
     assert store.initialized
 
+    assert await store.exists(doc_path)
     await store.remove(doc_path)
     assert not await store.exists(doc_path)
 
     new_doc = "new_doc.path"
     assert not await store.exists(new_doc)
-
-    await store.remove(new_doc)
+    with pytest.raises(YDocNotFound) as e:
+        await store.remove(new_doc)
+    assert str(e.value) == f"The document {new_doc} doesn't exists."
     assert not await store.exists(new_doc)
 
 
@@ -303,7 +240,7 @@ async def test_read(tmp_path, create_database, add_document):
     doc_path = "test.txt"
     update = b"foo"
 
-    # Create a database with an old version
+    # Create a database
     await create_database(path, SQLiteYStore.version)
     await add_document(path, doc_path, 0, update)
 
@@ -326,7 +263,7 @@ async def test_write(tmp_path, create_database, add_document):
     path = tmp_path / "tmp.db"
     doc_path = "test.txt"
 
-    # Create a database with an old version
+    # Create a database
     await create_database(path, SQLiteYStore.version)
     await add_document(path, doc_path, 0)
 
@@ -344,6 +281,33 @@ async def test_write(tmp_path, create_database, add_document):
             count = 0
             async for u, in cursor:
                 count += 1
-                # The fixture add_document inserts an empty update
-                assert u in [b"", update]
-            assert count == 2
+                assert u == update
+            assert count == 1
+
+
+async def _check_db(path: str, store: SQLiteYStore, doc_path: str | None = None):
+    async with aiosqlite.connect(path) as db:
+        cursor = await db.execute("pragma user_version")
+        version = (await cursor.fetchone())[0]
+        assert store.version == version
+
+        cursor = await db.execute(
+            "SELECT count(*) FROM sqlite_master WHERE type='table' AND name='documents'"
+        )
+        res = await cursor.fetchone()
+        assert res[0] == 1
+
+        cursor = await db.execute(
+            "SELECT count(*) FROM sqlite_master WHERE type='table' AND name='yupdates'"
+        )
+        res = await cursor.fetchone()
+        assert res[0] == 1
+
+        if doc_path is not None:
+            cursor = await db.execute(
+                "SELECT path, version FROM documents WHERE path = ?",
+                (doc_path,),
+            )
+            res = await cursor.fetchone()
+            assert res[0] == doc_path
+            assert res[1] == 0
diff --git a/ypy_websocket/stores/__init__.py b/ypy_websocket/stores/__init__.py
index 7666179..4bbaf63 100644
--- a/ypy_websocket/stores/__init__.py
+++ b/ypy_websocket/stores/__init__.py
@@ -1,4 +1,4 @@
 from .base_store import BaseYStore  # noqa
 from .file_store import FileYStore, TempFileYStore  # noqa
 from .sqlite_store import SQLiteYStore  # noqa
-from .utils import DocExists, YDocNotFound  # noqa
+from .utils import YDocExists, YDocNotFound  # noqa
diff --git a/ypy_websocket/stores/base_store.py b/ypy_websocket/stores/base_store.py
index fe9f90a..99c4ffc 100644
--- a/ypy_websocket/stores/base_store.py
+++ b/ypy_websocket/stores/base_store.py
@@ -32,7 +32,7 @@ def __init__(
         Initialize the object.
 
         Arguments:
-            path: The path where the store will be located or the prefix for file-based stores.
+            path: The path where the store will be located.
             metadata_callback: An optional callback to call to get the metadata.
             log: An optional logger.
         """
@@ -122,8 +122,7 @@ async def read(self, path: str) -> AsyncIterator[tuple[bytes, bytes]]:
     def initialized(self) -> bool:
         if self._initialized is not None:
             return self._initialized.is_set()
-        else:
-            return False
+        return False
 
     @property
     def started(self) -> Event:
diff --git a/ypy_websocket/stores/file_store.py b/ypy_websocket/stores/file_store.py
index 40d415e..89452a0 100644
--- a/ypy_websocket/stores/file_store.py
+++ b/ypy_websocket/stores/file_store.py
@@ -13,7 +13,7 @@
 
 from ..yutils import Decoder, get_new_path, write_var_uint
 from .base_store import BaseYStore
-from .utils import DocExists, YDocNotFound
+from .utils import YDocExists, YDocNotFound
 
 
 class FileYStore(BaseYStore):
@@ -102,7 +102,7 @@ async def list(self) -> AsyncIterator[str]:  # type: ignore[override]
         await self._initialized.wait()
 
         async for child in anyio.Path(self._store_path).glob("**/*.y"):
-            yield str(child.relative_to(self._store_path))
+            yield str(child.relative_to(self._store_path).with_suffix(""))
 
     async def get(self, path: str, updates: bool = False) -> dict | None:
         """
@@ -148,7 +148,7 @@ async def create(self, path: str, version: int) -> None:
 
         file_path = self._get_document_path(path)
         if await anyio.Path(file_path).exists():
-            raise DocExists(f"The document {path} already exists.")
+            raise YDocExists(f"The document {path} already exists.")
 
         else:
             await anyio.Path(file_path.parent).mkdir(parents=True, exist_ok=True)
@@ -170,6 +170,8 @@ async def remove(self, path: str) -> None:
         file_path = self._get_document_path(path)
         if await anyio.Path(file_path).exists():
             await anyio.Path(file_path).unlink(missing_ok=False)
+        else:
+            raise YDocNotFound(f"The document {path} doesn't exists.")
 
     async def read(self, path: str) -> AsyncIterator[tuple[bytes, bytes, float]]:  # type: ignore
         """Async iterator for reading the store content.
@@ -246,7 +248,7 @@ def _get_document_path(self, path: str) -> Path:
         return Path(self._store_path, path + ".y")
 
 
-@deprecated
+@deprecated(reason="Use FileYStore instead")
 class TempFileYStore(FileYStore):
     """
     A YStore which uses the system's temporary directory.
diff --git a/ypy_websocket/stores/sqlite_store.py b/ypy_websocket/stores/sqlite_store.py
index 90b3393..151cd50 100644
--- a/ypy_websocket/stores/sqlite_store.py
+++ b/ypy_websocket/stores/sqlite_store.py
@@ -12,7 +12,7 @@
 
 from ..yutils import get_new_path
 from .base_store import BaseYStore
-from .utils import DocExists, YDocNotFound
+from .utils import YDocExists, YDocNotFound
 
 
 class SQLiteYStore(BaseYStore):
@@ -198,7 +198,7 @@ async def create(self, path: str, version: int) -> None:
                     )
                     await db.commit()
             except aiosqlite.IntegrityError:
-                raise DocExists(f"The document {path} already exists.")
+                raise YDocExists(f"The document {path} already exists.")
 
     async def remove(self, path: str) -> None:
         """
@@ -213,6 +213,13 @@ async def remove(self, path: str) -> None:
 
         async with self._lock:
             async with aiosqlite.connect(self._store_path) as db:
+                cursor = await db.execute(
+                    "SELECT path, version FROM documents WHERE path = ?",
+                    (path,),
+                )
+                if (await cursor.fetchone()) is None:
+                    raise YDocNotFound(f"The document {path} doesn't exists.")
+
                 await db.execute(
                     "DELETE FROM documents WHERE path = ?",
                     (path,),
diff --git a/ypy_websocket/stores/utils.py b/ypy_websocket/stores/utils.py
index a73240f..6c5bf76 100644
--- a/ypy_websocket/stores/utils.py
+++ b/ypy_websocket/stores/utils.py
@@ -2,5 +2,5 @@ class YDocNotFound(Exception):
     pass
 
 
-class DocExists(Exception):
+class YDocExists(Exception):
     pass