Merge ~r00ta/maas:refactoring-baserepository-interface into maas:master

[Jacopo Rota]

refactoring: improve BaseRepository interface removing dependencies from the request models

This MP aims to remove the dependency from the request models from the db layer.

Since the ID of the resources comes from the undelying database sequence, we can retrieve the value from the service in order to properly build the model that the repository has to store.

[MAAS Lander (maas-lander)]

UNIT TESTS
-b refactoring-baserepository-interface lp:~r00ta/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: SUCCESS
COMMIT: 1de447f43e4367f1eb9e0c0cef2c57e70136f670

review: Approve

[Jacopo Rota (r00ta)]

jenkins: !test

[MAAS Lander (maas-lander)]

UNIT TESTS
-b refactoring-baserepository-interface lp:~r00ta/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: FAILED
LOG: http://maas-ci.internal:8080/job/maas-tester/5507/console
COMMIT: 1de447f43e4367f1eb9e0c0cef2c57e70136f670

review: Needs Fixing

[Wyatt Rees (wyattrees)]

jenkins: !test

[MAAS Lander (maas-lander)]

UNIT TESTS
-b refactoring-baserepository-interface lp:~r00ta/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: FAILED
LOG: http://maas-ci.internal:8080/job/maas-tester/5508/console
COMMIT: 1de447f43e4367f1eb9e0c0cef2c57e70136f670

review: Needs Fixing

Unmerged commits

1de447f... by Jacopo Rota on 2024-04-23

refactoring: improve BaseRepository interface removing dependencies from the request models

diff --git a/src/maasapiserver/common/db/sequences.py b/src/maasapiserver/common/db/sequences.py
new file mode 100644
index 0000000..963a7c6
--- /dev/null
+++ b/src/maasapiserver/common/db/sequences.py
@@ -0,0 +1,4 @@
+from sqlalchemy import Sequence
+
+ZoneIdSequence = Sequence("maasserver_zone_id_seq")
+ResourcePoolIdSequence = Sequence("maasserver_resourcepool_id_seq")
diff --git a/src/maasapiserver/v3/db/base.py b/src/maasapiserver/v3/db/base.py
index 21ff3cf..26cea78 100644
--- a/src/maasapiserver/v3/db/base.py
+++ b/src/maasapiserver/v3/db/base.py
@@ -8,15 +8,21 @@ from maasapiserver.v3.models.base import ListResult
 
 T = TypeVar("T")
 
-K = TypeVar("K")
 
-
-class BaseRepository(ABC, Generic[T, K]):
+class BaseRepository(ABC, Generic[T]):
     def __init__(self, connection: AsyncConnection):
         self.connection = connection
 
     @abstractmethod
-    async def create(self, request: K) -> T:
+    async def get_next_id(self) -> int:
+        """
+        Get the next ID for a new resource. Usually, this is useful because the ID comes from a sequence defined in the
+        database.
+        """
+        pass
+
+    @abstractmethod
+    async def create(self, request: T) -> T:
         pass
 
     @abstractmethod
diff --git a/src/maasapiserver/v3/db/bmc.py b/src/maasapiserver/v3/db/bmc.py
index 7dee695..df07a0a 100644
--- a/src/maasapiserver/v3/db/bmc.py
+++ b/src/maasapiserver/v3/db/bmc.py
@@ -2,15 +2,17 @@ from sqlalchemy import update
 from sqlalchemy.sql.operators import eq
 
 from maasapiserver.common.db.tables import BMCTable
-from maasapiserver.v3.api.models.requests.bmc import BmcRequest
 from maasapiserver.v3.api.models.requests.query import PaginationParams
 from maasapiserver.v3.db.base import BaseRepository
 from maasapiserver.v3.models.base import ListResult
 from maasapiserver.v3.models.bmc import Bmc
 
 
-class BmcRepository(BaseRepository[Bmc, BmcRequest]):
-    async def create(self, request: BmcRequest) -> Bmc:
+class BmcRepository(BaseRepository[Bmc]):
+    async def get_next_id(self) -> int:
+        raise Exception("Not implemented yet.")
+
+    async def create(self, resource: Bmc) -> Bmc:
         raise Exception("Not implemented yet.")
 
     async def find_by_id(self, id: int) -> Bmc | None:
diff --git a/src/maasapiserver/v3/db/machines.py b/src/maasapiserver/v3/db/machines.py
index 3002fb5..85205aa 100644
--- a/src/maasapiserver/v3/db/machines.py
+++ b/src/maasapiserver/v3/db/machines.py
@@ -11,7 +11,6 @@ from maasapiserver.common.db.tables import (
     NodeTable,
     UserTable,
 )
-from maasapiserver.v3.api.models.requests.machines import MachineRequest
 from maasapiserver.v3.api.models.requests.query import PaginationParams
 from maasapiserver.v3.db.base import BaseRepository
 from maasapiserver.v3.models.base import ListResult
@@ -19,8 +18,11 @@ from maasapiserver.v3.models.machines import Machine
 from maasserver.enum import NODE_TYPE
 
 
-class MachinesRepository(BaseRepository[Machine, MachineRequest]):
-    async def create(self, request: MachineRequest) -> Machine:
+class MachinesRepository(BaseRepository[Machine]):
+    async def get_next_id(self) -> int:
+        raise Exception("Not implemented yet.")
+
+    async def create(self, machine: Machine) -> Machine:
         raise Exception("Not implemented yet.")
 
     async def find_by_id(self, id: int) -> Machine | None:
diff --git a/src/maasapiserver/v3/db/nodes.py b/src/maasapiserver/v3/db/nodes.py
index 2686a7a..1bf1b71 100644
--- a/src/maasapiserver/v3/db/nodes.py
+++ b/src/maasapiserver/v3/db/nodes.py
@@ -2,15 +2,17 @@ from sqlalchemy import update
 from sqlalchemy.sql.operators import eq
 
 from maasapiserver.common.db.tables import NodeTable
-from maasapiserver.v3.api.models.requests.nodes import NodeRequest
 from maasapiserver.v3.api.models.requests.query import PaginationParams
 from maasapiserver.v3.db.base import BaseRepository
 from maasapiserver.v3.models.base import ListResult
 from maasapiserver.v3.models.nodes import Node
 
 
-class NodesRepository(BaseRepository[Node, NodeRequest]):
-    async def create(self, request: NodeRequest) -> Node:
+class NodesRepository(BaseRepository[Node]):
+    async def get_next_id(self) -> int:
+        raise Exception("Not implemented yet.")
+
+    async def create(self, resource: Node) -> Node:
         raise Exception("Not implemented yet.")
 
     async def find_by_id(self, id: int) -> Node | None:
diff --git a/src/maasapiserver/v3/db/resource_pools.py b/src/maasapiserver/v3/db/resource_pools.py
index 33640ae..525847b 100644
--- a/src/maasapiserver/v3/db/resource_pools.py
+++ b/src/maasapiserver/v3/db/resource_pools.py
@@ -6,6 +6,7 @@ from sqlalchemy.exc import IntegrityError
 from sqlalchemy.sql.functions import count
 from sqlalchemy.sql.operators import eq
 
+from maasapiserver.common.db.sequences import ResourcePoolIdSequence
 from maasapiserver.common.db.tables import ResourcePoolTable
 from maasapiserver.common.models.constants import (
     UNIQUE_CONSTRAINT_VIOLATION_TYPE,
@@ -15,9 +16,6 @@ from maasapiserver.common.models.exceptions import (
     BaseExceptionDetail,
 )
 from maasapiserver.v3.api.models.requests.query import PaginationParams
-from maasapiserver.v3.api.models.requests.resource_pools import (
-    ResourcePoolRequest,
-)
 from maasapiserver.v3.db.base import BaseRepository
 from maasapiserver.v3.models.base import ListResult
 from maasapiserver.v3.models.resource_pools import ResourcePool
@@ -31,9 +29,11 @@ RESOURCE_POOLS_FIELDS = (
 )
 
 
-class ResourcePoolRepository(
-    BaseRepository[ResourcePool, ResourcePoolRequest]
-):
+class ResourcePoolRepository(BaseRepository[ResourcePool]):
+    async def get_next_id(self) -> int:
+        stmt = select(ResourcePoolIdSequence.next_value())
+        return (await self.connection.execute(stmt)).scalar()
+
     async def find_by_id(self, id: int) -> Optional[ResourcePool]:
         stmt = self._select_all_statement().where(
             eq(ResourcePoolTable.c.id, id)
@@ -43,24 +43,23 @@ class ResourcePoolRepository(
                 return ResourcePool(**resource_pools._asdict())
         return None
 
-    async def create(self, request: ResourcePoolRequest) -> ResourcePool:
-        now = datetime.utcnow()
+    async def create(self, resource_pool: ResourcePool) -> ResourcePool:
         stmt = (
             insert(ResourcePoolTable)
             .returning(*RESOURCE_POOLS_FIELDS)
             .values(
-                name=request.name,
-                description=request.description,
-                updated=now,
-                created=now,
+                name=resource_pool.name,
+                description=resource_pool.description,
+                updated=resource_pool.updated,
+                created=resource_pool.created,
             )
         )
         try:
             result = await self.connection.execute(stmt)
         except IntegrityError:
-            self._raise_constraint_violation(request.name)
-        resource_pools = result.one()
-        return ResourcePool(**resource_pools._asdict())
+            self._raise_constraint_violation(resource_pool.name)
+        created_resource_pools = result.one()
+        return ResourcePool(**created_resource_pools._asdict())
 
     async def list(
         self, pagination_params: PaginationParams
diff --git a/src/maasapiserver/v3/db/users.py b/src/maasapiserver/v3/db/users.py
index 573b790..097c475 100644
--- a/src/maasapiserver/v3/db/users.py
+++ b/src/maasapiserver/v3/db/users.py
@@ -3,14 +3,16 @@ from sqlalchemy.sql.operators import eq
 
 from maasapiserver.common.db.tables import UserTable
 from maasapiserver.v3.api.models.requests.query import PaginationParams
-from maasapiserver.v3.api.models.requests.users import UserRequest
 from maasapiserver.v3.db.base import BaseRepository
 from maasapiserver.v3.models.base import ListResult
 from maasapiserver.v3.models.users import User
 
 
-class UsersRepository(BaseRepository[User, UserRequest]):
-    async def create(self, request: UserRequest) -> User:
+class UsersRepository(BaseRepository[User]):
+    async def get_next_id(self) -> int:
+        raise Exception("Not implemented yet.")
+
+    async def create(self, resource: User) -> User:
         raise Exception("Not implemented yet.")
 
     async def find_by_id(self, id: int) -> User | None:
diff --git a/src/maasapiserver/v3/db/vmcluster.py b/src/maasapiserver/v3/db/vmcluster.py
index b0a2bbf..9153d95 100644
--- a/src/maasapiserver/v3/db/vmcluster.py
+++ b/src/maasapiserver/v3/db/vmcluster.py
@@ -3,14 +3,16 @@ from sqlalchemy.sql.operators import eq
 
 from maasapiserver.common.db.tables import VmClusterTable
 from maasapiserver.v3.api.models.requests.query import PaginationParams
-from maasapiserver.v3.api.models.requests.vmcluster import VmClusterRequest
 from maasapiserver.v3.db.base import BaseRepository
 from maasapiserver.v3.models.base import ListResult
 from maasapiserver.v3.models.vmcluster import VmCluster
 
 
-class VmClustersRepository(BaseRepository[VmCluster, VmClusterRequest]):
-    async def create(self, request: VmClusterRequest) -> VmCluster:
+class VmClustersRepository(BaseRepository[VmCluster]):
+    async def get_next_id(self) -> int:
+        raise Exception("Not implemented yet.")
+
+    async def create(self, resource: VmCluster) -> VmCluster:
         raise Exception("Not implemented yet.")
 
     async def find_by_id(self, id: int) -> VmCluster | None:
diff --git a/src/maasapiserver/v3/db/zones.py b/src/maasapiserver/v3/db/zones.py
index 5325896..417e553 100644
--- a/src/maasapiserver/v3/db/zones.py
+++ b/src/maasapiserver/v3/db/zones.py
@@ -1,10 +1,10 @@
-from datetime import datetime
 from typing import Any
 
 from sqlalchemy import delete, desc, insert, select, Select
 from sqlalchemy.sql.functions import count
 from sqlalchemy.sql.operators import eq
 
+from maasapiserver.common.db.sequences import ZoneIdSequence
 from maasapiserver.common.db.tables import DefaultResourceTable, ZoneTable
 from maasapiserver.common.models.constants import (
     UNIQUE_CONSTRAINT_VIOLATION_TYPE,
@@ -14,18 +14,21 @@ from maasapiserver.common.models.exceptions import (
     BaseExceptionDetail,
 )
 from maasapiserver.v3.api.models.requests.query import PaginationParams
-from maasapiserver.v3.api.models.requests.zones import ZoneRequest
 from maasapiserver.v3.db.base import BaseRepository
 from maasapiserver.v3.models.base import ListResult
 from maasapiserver.v3.models.zones import Zone
 
 
-class ZonesRepository(BaseRepository[Zone, ZoneRequest]):
-    async def create(self, request: ZoneRequest) -> Zone:
+class ZonesRepository(BaseRepository[Zone]):
+    async def get_next_id(self) -> int:
+        stmt = select(ZoneIdSequence.next_value())
+        return (await self.connection.execute(stmt)).scalar()
+
+    async def create(self, zone: Zone) -> Zone:
         check_integrity_stmt = (
             select(ZoneTable.c.id)
             .select_from(ZoneTable)
-            .where(eq(ZoneTable.c.name, request.name))
+            .where(eq(ZoneTable.c.name, zone.name))
             .limit(1)
         )
         existing_entity = (
@@ -36,12 +39,11 @@ class ZonesRepository(BaseRepository[Zone, ZoneRequest]):
                 details=[
                     BaseExceptionDetail(
                         type=UNIQUE_CONSTRAINT_VIOLATION_TYPE,
-                        message=f"An entity with name '{request.name}' already exists. Its id is '{existing_entity.id}'.",
+                        message=f"An entity with name '{zone.name}' already exists. Its id is '{existing_entity.id}'.",
                     )
                 ]
             )
 
-        now = datetime.utcnow()
         stmt = (
             insert(ZoneTable)
             .returning(
@@ -52,15 +54,16 @@ class ZonesRepository(BaseRepository[Zone, ZoneRequest]):
                 ZoneTable.c.updated,
             )
             .values(
-                name=request.name,
-                description=request.description,
-                updated=now,
-                created=now,
+                id=zone.id,
+                name=zone.name,
+                description=zone.description,
+                updated=zone.updated,
+                created=zone.created,
             )
         )
         result = await self.connection.execute(stmt)
-        zone = result.one()
-        return Zone(**zone._asdict())
+        created_zone = result.one()
+        return Zone(**created_zone._asdict())
 
     async def find_by_id(self, id: int) -> Zone | None:
         stmt = self._select_all_statement().filter(eq(ZoneTable.c.id, id))
diff --git a/src/maasapiserver/v3/services/resource_pools.py b/src/maasapiserver/v3/services/resource_pools.py
index 6862bdc..4388b31 100644
--- a/src/maasapiserver/v3/services/resource_pools.py
+++ b/src/maasapiserver/v3/services/resource_pools.py
@@ -1,3 +1,4 @@
+from datetime import datetime
 from typing import Optional
 
 from sqlalchemy.ext.asyncio import AsyncConnection
@@ -34,9 +35,16 @@ class ResourcePoolsService(Service):
     async def create(
         self, resource_pool_request: ResourcePoolRequest
     ) -> ResourcePool:
-        return await self.resource_pools_repository.create(
-            resource_pool_request
+        now = datetime.utcnow()
+        resource_pool_id = await self.resource_pools_repository.get_next_id()
+        resource_pool = ResourcePool(
+            id=resource_pool_id,
+            name=resource_pool_request.name,
+            description=resource_pool_request.description,
+            updated=now,
+            created=now,
         )
+        return await self.resource_pools_repository.create(resource_pool)
 
     async def get_by_id(self, id: int) -> Optional[ResourcePool]:
         return await self.resource_pools_repository.find_by_id(id)
diff --git a/src/maasapiserver/v3/services/zones.py b/src/maasapiserver/v3/services/zones.py
index 8b6e5f3..f40f419 100644
--- a/src/maasapiserver/v3/services/zones.py
+++ b/src/maasapiserver/v3/services/zones.py
@@ -1,3 +1,4 @@
+from datetime import datetime
 from typing import Optional
 
 from sqlalchemy.ext.asyncio import AsyncConnection
@@ -50,7 +51,16 @@ class ZonesService(Service):
         )
 
     async def create(self, zone_request: ZoneRequest) -> Zone:
-        return await self.zones_repository.create(zone_request)
+        zone_id = await self.zones_repository.get_next_id()
+        now = datetime.utcnow()
+        zone = Zone(
+            id=zone_id,
+            name=zone_request.name,
+            description=zone_request.description,
+            updated=now,
+            created=now,
+        )
+        return await self.zones_repository.create(zone)
 
     async def get_by_id(self, id: int) -> Optional[Zone]:
         return await self.zones_repository.find_by_id(id)
diff --git a/src/tests/maasapiserver/v3/db/test_resource_pools.py b/src/tests/maasapiserver/v3/db/test_resource_pools.py
index 6ad5feb..da0e994 100644
--- a/src/tests/maasapiserver/v3/db/test_resource_pools.py
+++ b/src/tests/maasapiserver/v3/db/test_resource_pools.py
@@ -7,9 +7,6 @@ from sqlalchemy.orm.exc import NoResultFound
 
 from maasapiserver.common.models.exceptions import AlreadyExistsException
 from maasapiserver.v3.api.models.requests.query import PaginationParams
-from maasapiserver.v3.api.models.requests.resource_pools import (
-    ResourcePoolRequest,
-)
 from maasapiserver.v3.db.resource_pools import ResourcePoolRepository
 from maasapiserver.v3.models.resource_pools import ResourcePool
 from tests.fixtures.factories.resource_pools import (
@@ -22,35 +19,45 @@ from tests.maasapiserver.fixtures.db import Fixture
 @pytest.mark.usefixtures("ensuremaasdb")
 @pytest.mark.asyncio
 class TestResourcePoolRepository:
+    async def test_get_next_id(self, db_connection: AsyncConnection) -> None:
+        resource_pools_repository = ResourcePoolRepository(db_connection)
+        first = await resource_pools_repository.get_next_id()
+        second = await resource_pools_repository.get_next_id()
+        assert first < second
+
     async def test_create(self, db_connection: AsyncConnection) -> None:
-        now = datetime.utcnow()
+        date = datetime.fromisocalendar(2024, 1, 1).astimezone(timezone.utc)
         resource_pools_repository = ResourcePoolRepository(db_connection)
         created_resource_pools = await resource_pools_repository.create(
-            ResourcePoolRequest(
-                name="my_resource_pool", description="my description"
+            ResourcePool(
+                id=2,
+                name="my_resource_pool",
+                description="my description",
+                created=date,
+                updated=date,
             )
         )
         assert created_resource_pools.id
         assert created_resource_pools.name == "my_resource_pool"
         assert created_resource_pools.description == "my description"
-        assert created_resource_pools.created.astimezone(
-            timezone.utc
-        ) >= now.astimezone(timezone.utc)
-        assert created_resource_pools.updated.astimezone(
-            timezone.utc
-        ) >= now.astimezone(timezone.utc)
+        assert created_resource_pools.created.astimezone(timezone.utc) == date
+        assert created_resource_pools.updated.astimezone(timezone.utc) == date
 
     async def test_create_duplicated(
         self, db_connection: AsyncConnection, fixture: Fixture
     ) -> None:
+        date = datetime.fromisocalendar(2024, 1, 1).astimezone(timezone.utc)
         resource_pools_repository = ResourcePoolRepository(db_connection)
         created_resource_pools = await create_test_resource_pool(fixture)
 
         with pytest.raises(AlreadyExistsException):
             await resource_pools_repository.create(
-                ResourcePoolRequest(
+                ResourcePool(
+                    id=3,
                     name=created_resource_pools.name,
                     description=created_resource_pools.description,
+                    created=date,
+                    updated=date,
                 )
             )
 
diff --git a/src/tests/maasapiserver/v3/db/test_zones.py b/src/tests/maasapiserver/v3/db/test_zones.py
index 1104182..55367c9 100644
--- a/src/tests/maasapiserver/v3/db/test_zones.py
+++ b/src/tests/maasapiserver/v3/db/test_zones.py
@@ -11,6 +11,7 @@ from maasapiserver.v3.api.models.requests.query import PaginationParams
 from maasapiserver.v3.api.models.requests.zones import ZoneRequest
 from maasapiserver.v3.constants import DEFAULT_ZONE_NAME
 from maasapiserver.v3.db.zones import ZonesRepository
+from maasapiserver.v3.models.zones import Zone
 from tests.fixtures.factories.zone import create_test_zone
 from tests.maasapiserver.fixtures.db import Fixture
 
@@ -18,21 +19,29 @@ from tests.maasapiserver.fixtures.db import Fixture
 @pytest.mark.usefixtures("ensuremaasdb")
 @pytest.mark.asyncio
 class TestZonesRepository:
+    async def test_get_next_id(self, db_connection: AsyncConnection) -> None:
+        zones_repository = ZonesRepository(db_connection)
+        first = await zones_repository.get_next_id()
+        second = await zones_repository.get_next_id()
+        assert first < second
+
     async def test_create(self, db_connection: AsyncConnection) -> None:
-        now = datetime.utcnow()
+        date = datetime.fromisocalendar(2024, 1, 1).astimezone(timezone.utc)
         zones_repository = ZonesRepository(db_connection)
         created_zone = await zones_repository.create(
-            ZoneRequest(name="my_zone", description="my description")
+            Zone(
+                id=2,
+                name="my_zone",
+                description="my description",
+                created=date,
+                updated=date,
+            )
         )
         assert created_zone.id > 1
         assert created_zone.name == "my_zone"
         assert created_zone.description == "my description"
-        assert created_zone.created.astimezone(timezone.utc) >= now.astimezone(
-            timezone.utc
-        )
-        assert created_zone.updated.astimezone(timezone.utc) >= now.astimezone(
-            timezone.utc
-        )
+        assert created_zone.created.astimezone(timezone.utc) == date
+        assert created_zone.updated.astimezone(timezone.utc) == date
 
     async def test_create_duplicated(
         self, db_connection: AsyncConnection, fixture: Fixture
diff --git a/src/tests/maasapiserver/v3/services/test_resource_pools.py b/src/tests/maasapiserver/v3/services/test_resource_pools.py
index b3012a3..d3c5cbb 100644
--- a/src/tests/maasapiserver/v3/services/test_resource_pools.py
+++ b/src/tests/maasapiserver/v3/services/test_resource_pools.py
@@ -1,4 +1,5 @@
 from datetime import datetime
+from typing import Any
 from unittest.mock import AsyncMock, Mock
 
 import pytest
@@ -23,6 +24,24 @@ class TestResourcePoolsService:
     async def test_create(
         self, db_connection: AsyncConnection, fixture: Fixture
     ) -> None:
+        class ResourcePoolMatcher(ResourcePool):
+            def __init__(self, **kwargs: Any) -> None:
+                kwargs["created"] = datetime.now()
+                kwargs["updated"] = datetime.now()
+                super().__init__(**kwargs)
+
+            def __eq__(self, other: Any) -> bool:
+                if not isinstance(other, ResourcePool):
+                    return False
+                return (
+                    self.id == other.id
+                    and self.name == other.name
+                    and self.description == other.description
+                    and self.created >= other.created
+                    and self.updated  # The matcher is created after the actual resource
+                    >= other.updated
+                )
+
         now = datetime.utcnow()
         resource_pool = ResourcePool(
             id=1,
@@ -35,6 +54,7 @@ class TestResourcePoolsService:
         resource_pool_repository_mock.create = AsyncMock(
             return_value=resource_pool
         )
+        resource_pool_repository_mock.get_next_id = AsyncMock(return_value=1)
         resource_pools_service = ResourcePoolsService(
             connection=db_connection,
             resource_pools_repository=resource_pool_repository_mock,
@@ -43,7 +63,13 @@ class TestResourcePoolsService:
             name=resource_pool.name, description=resource_pool.description
         )
         created_resource_pool = await resource_pools_service.create(request)
-        resource_pool_repository_mock.create.assert_called_once_with(request)
+        resource_pool_repository_mock.create.assert_called_once_with(
+            ResourcePoolMatcher(
+                id=1,
+                name=resource_pool.name,
+                description=resource_pool.description,
+            )
+        )
         assert created_resource_pool is not None
 
     async def test_list(