Code consistency (#358)

Several changes to make the code base more consistent in naming, typing,
and use of connection vs session.

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>

Author: PGijsbers

docs/development/tests.md

@@ -71,18 +71,58 @@ Some guidelines and things to keep in mind when writing tests:
  - Try to keep tests small, so that they fail for one particular reason only.
  - Mark tests that update the database in anyway with the `mut` marker (`@pytest.mark.mut`).
  - If the test is excessively slow (>0.1 sec) and does not connect to PHP, use a `slow` marker. Tests that include PHP always require roundtrips through other services which makes them slow by default. PHP tests can be filtered out with the automatically generated "php_api" marker.
- - Four common fixtures you might need when writing tests are:
-    - `py_api`: an async client for the Python based REST API
-    - `php_api`: an async client for the PHP based REST API
-    - `expdb_test`: an AsyncConnection to the "expdb" OpenML database.
-    - `user_test`: an AsyncConnection to the "openml" OpenML database.
- - Above fixtures have considerable per-test overhead. Use them only when you need them.
  - When writing assertions the expected value (a constant, or a php response) should be on the right (`assert response == expected`).
 
+### Fixtures
+There are a number of fixtures in `conftest.py`, here is a quick rundown of the most notable ones:
+
+    - `py_api` and `php_api`: an async client for the Python- and PHP-based REST APIs, respectively. The `py_api` client has its normal dependency injection for database connections patched to use the connection and session fixtures below.
+    - `expdb_connection` and `userdb_connection`: an AsyncConnection to the "expdb" OpenML database. This fixture is function-scoped and automatically starts a transaction which is rolled back as long as no `commit` is made.
+    - `expdb_session` and `userdb_session`: an AsyncSession that is bound to the respective connection. This means it also has automatic rollback. Any changes that need to be visible to the `py_api` fixture can be performed on this session (see below).
+
+The pseudocode below shows how you might combine these for a test of the new REST API, either as standalone or when compared to the PHP API:
+
+```python
+
+async def test_python(py_api: httpx.AsyncClient, expdb_session: AsyncSession) -> None:
+    await expdb_session.execute(text("INSERT INTO dataset ..."), params=...)  # Insert dataset with id 42
+
+    response = await py_api.get("/datasets/42")  # Since this call shares the session, it should retrieve this data
+
+    assert ...
+    # after the test is done, the fixture clean up will ensure the change is not committed to the database, no extra code needed
+
+async def test_python_and_php(py_api: httpx.AsyncClient, php_api: httpx.AsyncClient, expdb_connection: AsyncConnection) -> None:
+    await expdb_connection.execute(text("INSERT INTO dataset ..."), parameters=...)  # Insert dataset with id 42
+    await expdb_connection.commit()  # We need to persist the data in the database, because the PHP REST API cannot see our transaction
+
+    response = await php_api.get("/datasets/42")  # The PHP REST API can see the dataset, because it exists in the database
+    response = await py_api.get("/datasets/42")  # The Python REST API can see the dataset also
+
+    # We need to clean up after ourselves, otherwise the test has side effects.
+    # This isn't a great pattern, prefer instead the use of context managers which will execute the delete statements even if unexpected exceptions occur.
+    await expdb_connection.execute(text("DELETE FROM dataset ..."), parameters=...)
+    await expdb_connection.commit()
+
+```
+
+???- "Why not always use the `*_connection`?"
+
+    As this implementation will make more and more use of ORM models, it is more convenient to have access to an `AsyncSession` object which can deal with those models.
+    Eventually, when verification against PHP REST API output is no longer necessary, we do not even need the `AsyncConnection` objects anymore at all.
+
+
+Above fixtures have considerable per-test overhead. Use them only when you need them. More details in the next section.
+
 ### Writing Tests for an Endpoint
 Because the `py_api` and database fixtures provide considerable per-test overhead,
 follow these guidelines for writing a test suite for an endpoint.
 
+!!! warning "Code snippets may not work"
+
+    The code below is provided as a guide, but isn't automatically tested (yet). This means it may be out of sync.
+    For examples that work, reference our test suite.
+
 Include tests against `py_api` for input validation specific to that endpoint. Validation in reused components should be tested centrally (e.g., Pagination).
 ```python
 def test_get_dataset_identifier_validation(py_api: httpx.AsyncClient) -> None:
@@ -102,17 +142,17 @@ def test_get_dataset_success(py_api: httpx.AsyncClient) -> None:
 For all other tests, do not use `py_api` but call the implementing function directly. For example, do not call `client.get("/datasets/1")` but instead `get_dataset`:
 
 ```python
-async def test_get_dataset_private_success(expdb_test: AsyncConnection, user_test: AsyncConnection) -> None:
+async def test_get_dataset_private_success(expdb_session: AsyncSession, userdb_session: AsyncSession) -> None:
     private_dataset = 42
     owner_of_that_dataset = OWNER_USER
-    dataset = await get_dataset(dataset_id=42, user=owner_of_that_dataset, user_db=user_test, expdb_db=expdb_test)
+    dataset = await get_dataset(dataset_id=42, user=owner_of_that_dataset, userdb_session=userdb_session, expdb_session=expdb_session)
     assert dataset.id == private_dataset
 
-async def test_get_dataset_private_access_denied(expdb_test: AsyncConnection, user_test: AsyncConnection) -> None:
+async def test_get_dataset_private_access_denied(expdb_session: AsyncSession, userdb_session: AsyncSession) -> None:
     private_dataset = 42
     owner_of_that_dataset = SOME_USER  # Test User defined in a common file
     with pytest.raises(DatasetNoAccessError) as e:
-        await get_dataset(dataset_id=42, user=owner_of_that_dataset, user_db=user_test, expdb_db=expdb_test)
+        await get_dataset(dataset_id=42, user=owner_of_that_dataset, userdb_session=userdb_session, expdb_session=expdb_session)
     assert e.value.status_code == HTTPStatus.FORBIDDEN
 ```
 
@@ -164,7 +204,7 @@ def _assert_error_response_equal(py_response, php_response) -> None:
 You frequently need to write tests which include fetching from or writing to the database.
 There is a test database that is prepopulated with data available for use as defined in our `compose.yaml` file.
 
-The `expdb_test` and `user_test` connections automatically start a transaction during setup and perform a rollback during teardown.
+The `expdb_connection`, `userdb_connection`, `expdb_session`, and `userdb_session` fixtures automatically start a transaction during setup and perform a rollback during teardown.
 This means that as long as you do not `.commit()` any changes, the data will not persist.
 This is a good thing. We do not want our tests to have side effects, as it might lead to inconsistent behavior.
 

src/core/access.py

@@ -1,7 +1,7 @@
 from typing import TYPE_CHECKING, Any
 
 from database.users import User
-from schemas.datasets import Visibility
+from routers.schemas.datasets import Visibility
 
 if TYPE_CHECKING:
     from sqlalchemy.engine import Row

src/database/datasets.py

@@ -15,99 +15,105 @@
     ForeignKeyConstraintError,
 )
 from database.models.base import UntypedRow
-from schemas.datasets import DatasetStatus, Feature
+from routers.schemas.datasets import DatasetStatus, Feature
 
 if TYPE_CHECKING:
-    from sqlalchemy.ext.asyncio import AsyncConnection
+    from sqlalchemy.ext.asyncio import AsyncSession
 
 
-async def get(id_: Identifier, connection: AsyncConnection) -> UntypedRow | None:
-    row = await connection.execute(
+async def get(dataset_id: Identifier, session: AsyncSession) -> UntypedRow | None:
+    row = await session.execute(
         text(
             """
     SELECT *
     FROM dataset
     WHERE did = :dataset_id
     """,
         ),
-        parameters={"dataset_id": id_},
+        params={"dataset_id": dataset_id},
     )
     return row.one_or_none()
 
 
-async def get_file(*, file_id: Identifier, connection: AsyncConnection) -> UntypedRow | None:
-    row = await connection.execute(
+async def get_file(*, file_id: Identifier, session: AsyncSession) -> UntypedRow | None:
+    row = await session.execute(
         text(
             """
     SELECT *
     FROM file
     WHERE id = :file_id
     """,
         ),
-        parameters={"file_id": file_id},
+        params={"file_id": file_id},
     )
     return row.one_or_none()
 
 
 async def get_tag(
     dataset_id: Identifier,
     tag: TagString,
-    connection: AsyncConnection,
+    session: AsyncSession,
 ) -> UntypedRow | None:
     return (
-        await connection.execute(
+        await session.execute(
             text(
                 """
     SELECT *
     FROM dataset_tag
     WHERE id = :dataset_id AND tag = :tag
     """,
             ),
-            parameters={"dataset_id": dataset_id, "tag": tag},
+            params={"dataset_id": dataset_id, "tag": tag},
         )
     ).first()
 
 
-async def delete_tag(dataset_id: Identifier, tag: TagString, connection: AsyncConnection) -> None:
-    await connection.execute(
+async def delete_tag(dataset_id: Identifier, tag: TagString, session: AsyncSession) -> None:
+    await session.execute(
         text(
             """
     DELETE FROM dataset_tag
     WHERE id = :dataset_id AND tag = :tag
     """,
         ),
-        parameters={"dataset_id": dataset_id, "tag": tag},
+        params={"dataset_id": dataset_id, "tag": tag},
     )
 
 
-async def get_tags_for(id_: Identifier, connection: AsyncConnection) -> list[str]:
-    row = await connection.execute(
+async def get_tags_for(dataset_id: Identifier, session: AsyncSession) -> list[str]:
+    row = await session.execute(
         text(
             """
     SELECT *
     FROM dataset_tag
     WHERE id = :dataset_id
     """,
         ),
-        parameters={"dataset_id": id_},
+        params={"dataset_id": dataset_id},
     )
     rows = row.all()
     return [row.tag for row in rows]
 
 
-async def tag(id_: int, tag_: str, *, user_id: int, connection: AsyncConnection) -> None:
+async def tag(
+    dataset_id: Identifier,
+    tag: str,
+    *,
+    user_id: Identifier,
+    session: AsyncSession,
+) -> None:
     try:
-        await connection.execute(
+        await session.execute(
             text(
                 """
         INSERT INTO dataset_tag(`id`, `tag`, `uploader`)
         VALUES (:dataset_id, :tag, :user_id)
         """,
             ),
-            parameters={
-                "dataset_id": id_,
+            params={
+                "dataset_id": dataset_id,
                 "user_id": user_id,
-                "tag": tag_,
+                "tag": tag,
             },
         )
     except IntegrityError as e:
@@ -122,11 +128,11 @@ async def tag(id_: int, tag_: str, *, user_id: int, connection: AsyncConnection)
 
 
 async def get_description(
-    id_: Identifier,
-    connection: AsyncConnection,
+    dataset_id: Identifier,
+    session: AsyncSession,
 ) -> UntypedRow | None:
     """Get the most recent description for the dataset."""
-    row = await connection.execute(
+    row = await session.execute(
         text(
             """
     SELECT *
@@ -135,15 +141,15 @@ async def get_description(
     ORDER BY version DESC
     """,
         ),
-        parameters={"dataset_id": id_},
+        params={"dataset_id": dataset_id},
     )
     return row.first()
 
 
-async def get_status(id_: Identifier, connection: AsyncConnection) -> DatasetStatus:
+async def get_status(dataset_id: Identifier, session: AsyncSession) -> DatasetStatus:
     """Get most recent status for the dataset."""
     row = (
-        await connection.execute(
+        await session.execute(
             text(
                 """
     SELECT status
@@ -153,17 +159,17 @@ async def get_status(id_: Identifier, connection: AsyncConnection) -> DatasetSta
     LIMIT 1
     """,
             ),
-            parameters={"dataset_id": id_},
+            params={"dataset_id": dataset_id},
         )
     ).first()
     return DatasetStatus(row.status) if row else DatasetStatus.IN_PREPARATION
 
 
 async def get_latest_processing_update(
     dataset_id: Identifier,
-    connection: AsyncConnection,
+    session: AsyncSession,
 ) -> UntypedRow | None:
-    row = await connection.execute(
+    row = await session.execute(
         text(
             """
     SELECT *
@@ -172,13 +178,13 @@ async def get_latest_processing_update(
     ORDER BY processing_date DESC
     """,
         ),
-        parameters={"dataset_id": dataset_id},
+        params={"dataset_id": dataset_id},
     )
     return row.first()
 
 
-async def get_features(dataset_id: Identifier, connection: AsyncConnection) -> list[Feature]:
-    row = await connection.execute(
+async def get_features(dataset_id: Identifier, session: AsyncSession) -> list[Feature]:
+    row = await session.execute(
         text(
             """
             SELECT `index`,`name`,`data_type`,`is_target`,
@@ -187,25 +193,25 @@ async def get_features(dataset_id: Identifier, connection: AsyncConnection) -> l
             WHERE `did` = :dataset_id
             """,
         ),
-        parameters={"dataset_id": dataset_id},
+        params={"dataset_id": dataset_id},
     )
     rows = row.mappings().all()
     return [Feature(**row, nominal_values=None) for row in rows]
 
 
 async def get_feature_ontologies(
     dataset_id: Identifier,
-    connection: AsyncConnection,
+    session: AsyncSession,
 ) -> dict[int, list[str]]:
-    rows = await connection.execute(
+    rows = await session.execute(
         text(
             """
             SELECT `index`, `value`
             FROM data_feature_description
             WHERE `did` = :dataset_id AND `description_type` = 'ontology'
             """,
         ),
-        parameters={"dataset_id": dataset_id},
+        params={"dataset_id": dataset_id},
     )
     ontologies: dict[int, list[str]] = defaultdict(list)
     for row in rows.mappings():
@@ -217,17 +223,17 @@ async def get_feature_values(
     dataset_id: Identifier,
     *,
     feature_index: int,
-    connection: AsyncConnection,
+    session: AsyncSession,
 ) -> list[str]:
-    row = await connection.execute(
+    row = await session.execute(
         text(
             """
             SELECT `value`
             FROM data_feature_value
             WHERE `did` = :dataset_id AND `index` = :feature_index
             """,
         ),
-        parameters={"dataset_id": dataset_id, "feature_index": feature_index},
+        params={"dataset_id": dataset_id, "feature_index": feature_index},
     )
     rows = row.all()
     return [row.value for row in rows]
@@ -238,16 +244,16 @@ async def update_status(
     status: Literal[DatasetStatus.ACTIVE, DatasetStatus.DEACTIVATED],
     *,
     user_id: Identifier,
-    connection: AsyncConnection,
+    session: AsyncSession,
 ) -> None:
-    await connection.execute(
+    await session.execute(
         text(
             """
             INSERT INTO dataset_status(`did`,`status`,`status_date`,`user_id`)
             VALUES (:dataset, :status, :date, :user)
             """,
         ),
-        parameters={
+        params={
             "dataset": dataset_id,
             "status": status,
             "date": datetime.datetime.now(datetime.UTC),
@@ -256,13 +262,13 @@ async def update_status(
     )
 
 
-async def remove_deactivated_status(dataset_id: Identifier, connection: AsyncConnection) -> None:
-    await connection.execute(
+async def remove_deactivated_status(dataset_id: Identifier, session: AsyncSession) -> None:
+    await session.execute(
         text(
             """
             DELETE FROM dataset_status
             WHERE `did` = :data AND `status`='deactivated'
             """,
         ),
-        parameters={"data": dataset_id},
+        params={"data": dataset_id},
     )