feat(api): Refactor campaign and node routes

Author: tcjennings

src/lsst/cmservice/routers/v2/campaigns.py

@@ -13,10 +13,9 @@
 from sqlmodel import col, select
 from sqlmodel.ext.asyncio.session import AsyncSession
 
-from ...common.enums import ManifestKind
 from ...common.logging import LOGGER
-from ...db.campaigns_v2 import Campaign, CampaignModel, CampaignUpdate, Edge, Node
-from ...db.manifests_v2 import ManifestWrapper
+from ...db.campaigns_v2 import Campaign, CampaignUpdate, Edge, Node
+from ...db.manifests_v2 import CampaignManifest
 from ...db.session import db_session_dependency
 
 # TODO should probably bind a logger to the fastapi app or something
@@ -40,7 +39,7 @@ async def read_campaign_collection(
     session: Annotated[AsyncSession, Depends(db_session_dependency)],
     limit: Annotated[int, Query(le=100)] = 10,
     offset: Annotated[int, Query()] = 0,
-) -> Sequence[CampaignModel]:
+) -> Sequence[Campaign]:
     """..."""
     try:
         campaigns = await session.exec(select(Campaign).offset(offset).limit(limit))
@@ -300,32 +299,23 @@ async def create_campaign_resource(
     request: Request,
     response: Response,
     session: Annotated[AsyncSession, Depends(db_session_dependency)],
-    manifest: ManifestWrapper,
+    manifest: CampaignManifest,
 ) -> Campaign:
-    # Validate the input by checking the "kind" of manifest is a campaign
-    if manifest.kind is not ManifestKind.campaign:
-        raise HTTPException(
-            status_code=422, detail="Campaigns may only be created from a 'campaign' manifest"
-        )
-    # and that the manifest includes any required fields, though this could
-    # just as well be a try/except ValueError around `_.model_validate()`
-    elif (campaign_name := manifest.metadata_.pop("name", None)) is None:
-        raise HTTPException(status_code=400, detail="Campaigns must have a name set in '.metadata.name'")
-
     # Create a campaign spec from the manifest, delegating the creation of new
     # dynamic fields to the model validation method, -OR- create new dynamic
     # fields here.
     campaign = Campaign.model_validate(
         dict(
-            name=campaign_name,
-            metadata_=manifest.metadata_,
+            name=manifest.metadata_.name,
+            metadata_=manifest.metadata_.model_dump(),
             # owner = ...  # TODO Get username from gafaelfawr # noqa: ERA001
         )
     )
 
     # A new campaign comes with a START and END node
     start_node = Node.model_validate(dict(name="START", namespace=campaign.id))
     end_node = Node.model_validate(dict(name="END", namespace=campaign.id))
+
     # Put the campaign in the database
     session.add(campaign)
     session.add(start_node)

src/lsst/cmservice/routers/v2/manifests.py

@@ -12,11 +12,10 @@
 from sqlmodel import col, select
 from sqlmodel.ext.asyncio.session import AsyncSession
 
-from ...common.enums import ManifestKind
 from ...common.jsonpatch import JSONPatch, JSONPatchError, apply_json_patch
 from ...common.logging import LOGGER
 from ...db.campaigns_v2 import Campaign, Manifest, _default_campaign_namespace
-from ...db.manifests_v2 import ManifestWrapper
+from ...db.manifests_v2 import ManifestModel
 from ...db.session import db_session_dependency
 
 # TODO should probably bind a logger to the fastapi app or something
@@ -103,39 +102,23 @@ async def read_single_resource(
     summary="Add a manifest resource",
     status_code=204,
 )
-async def create_one_or_more_resources(
+async def create_one_or_more_manifests(
     request: Request,
     response: Response,
     session: Annotated[AsyncSession, Depends(db_session_dependency)],
-    manifests: ManifestWrapper | list[ManifestWrapper],
+    manifests: ManifestModel | list[ManifestModel],
 ) -> None:
-    # TODO should support query parameters that scope the namespace, such that
-    #      response headers from a campaign-create operation can immediately
-    #      follow a link to node-create for that campaign.
-
     # We could be given a single manifest or a list of them. In the singleton
     # case, wrap it in a list so we can treat everything equally
     if not isinstance(manifests, list):
         manifests = [manifests]
 
     for manifest in manifests:
-        # Validate the input by checking the "kind" of manifest.
-        # The difference between a "manifest" and a "node" is iffy, but all we
-        # want to assert here is that nodes, campaigns, and edges don't go in
-        # the manifest table
-        if manifest.kind in [ManifestKind.campaign, ManifestKind.node, ManifestKind.edge]:
-            raise HTTPException(status_code=422, detail=f"Manifests may not be a {manifest.kind.name} kind.")
-
-        # and that the manifest includes any required fields, though this could
-        # just as well be a try/except ValueError around `_.model_validate()`
-        elif (_name := manifest.metadata_.pop("name", None)) is None:
-            raise HTTPException(status_code=400, detail="Manifests must have a name set in '.metadata.name'")
-
-        # TODO match node with jsonschema and validate
+        _name = manifest.metadata_.name
 
         # A manifest must exist in the namespace of an existing campaign
         # or the default namespace
-        _namespace: str | None = manifest.metadata_.pop("namespace", None)
+        _namespace: str | None = manifest.metadata_.namespace
         if _namespace is None:
             _namespace_uuid = _default_campaign_namespace
         else:
@@ -153,9 +136,8 @@ async def create_one_or_more_resources(
                     raise HTTPException(status_code=422, detail="Requested namespace does not exist.")
                 _namespace_uuid = _campaign_id
 
-        # A node must be a new version if name+namespace already exists
-        # - check db for node as name+namespace, get version and increment
-        _version = int(manifest.metadata_.pop("version", 0))
+        # A manifest must be a new version if name+namespace already exists
+        # check db for manifest as name+namespace, get version and increment
 
         s = (
             select(Manifest)
@@ -164,18 +146,18 @@ async def create_one_or_more_resources(
             .order_by(col(Manifest.version).desc())
             .limit(1)
         )
-        _previous = (await session.exec(s)).one_or_none()
 
-        _version = _previous.version if _previous else _version
+        _previous = (await session.exec(s)).one_or_none()
+        _version = _previous.version if _previous else manifest.metadata_.version
         _version += 1
         _manifest = Manifest(
             id=uuid5(_namespace_uuid, f"{_name}.{_version}"),
             name=_name,
             namespace=_namespace_uuid,
             kind=manifest.kind,
             version=_version,
-            metadata_=manifest.metadata_,
-            spec=manifest.spec,
+            metadata_=manifest.metadata_.model_dump(),
+            spec=manifest.spec.model_dump(),
         )
 
         # Put the node in the database

tests/v2/conftest.py

@@ -4,12 +4,13 @@
 import os
 from collections.abc import AsyncGenerator, Generator
 from typing import TYPE_CHECKING
-from uuid import uuid4
+from uuid import NAMESPACE_DNS, uuid4
 
 import pytest
 import pytest_asyncio
 from fastapi.testclient import TestClient
 from httpx import ASGITransport, AsyncClient
+from sqlalchemy import insert
 from sqlalchemy.pool import NullPool
 from sqlalchemy.schema import CreateSchema, DropSchema
 from testcontainers.postgres import PostgresContainer
@@ -86,6 +87,14 @@ async def testdb(rawdb: DatabaseSessionDependency) -> AsyncGenerator[DatabaseSes
         await aconn.run_sync(metadata.drop_all)
         await aconn.execute(CreateSchema(config.db.table_schema, if_not_exists=True))
         await aconn.run_sync(metadata.create_all)
+        await aconn.execute(
+            insert(metadata.tables[f"{metadata.schema}.campaigns_v2"]).values(
+                id="dda54a0c-6878-5c95-ac4f-007f6808049e",
+                namespace=str(NAMESPACE_DNS),
+                name="DEFAULT",
+                owner="root",
+            )
+        )
         await aconn.commit()
     yield rawdb
     async with rawdb.engine.begin() as aconn:

tests/v2/test_campaign_routes.py

@@ -3,19 +3,20 @@
 from uuid import NAMESPACE_DNS, uuid4, uuid5
 
 import pytest
-from httpx import AsyncClient, HTTPStatusError
+from httpx import AsyncClient
 
 pytestmark = pytest.mark.asyncio(loop_scope="module")
 """All tests in this module will run in the same event loop."""
 
 
-async def test_async_list_campaigns(aclient: AsyncClient) -> None:
+async def test_list_campaigns(aclient: AsyncClient) -> None:
+    """Tests listing the set of all campaigns."""
+    # initially, only the default campaign should be available.
     x = await aclient.get("/cm-service/v2/campaigns")
-    assert len(x.json()) == 0
-    assert True
+    assert len(x.json()) == 1
 
 
-async def test_async_list_campaign(aclient: AsyncClient) -> None:
+async def test_list_campaign(aclient: AsyncClient) -> None:
     """Tests lookup of a single campaign by name and by ID"""
     campaign_name = uuid4().hex[-8:]
 
@@ -47,9 +48,11 @@ async def test_async_list_campaign(aclient: AsyncClient) -> None:
     assert x.is_success
 
 
-async def test_async_create_campaign(aclient: AsyncClient) -> None:
+async def test_negative_campaign(aclient: AsyncClient) -> None:
+    """Tests campaign api negative results."""
     # Test failure to create campaign with invalid manifest (wrong kind)
     campaign_name = uuid4().hex[-8:]
+
     x = await aclient.post(
         "/cm-service/v2/campaigns",
         json={
@@ -61,11 +64,7 @@ async def test_async_create_campaign(aclient: AsyncClient) -> None:
             "spec": {},
         },
     )
-    with pytest.raises(HTTPStatusError):
-        assert x.status_code == 422
-        x.raise_for_status()
-
-    del x
+    assert x.is_client_error
 
     # Test failure to create campaign with incomplete manifest (missing name)
     x = await aclient.post(
@@ -77,11 +76,11 @@ async def test_async_create_campaign(aclient: AsyncClient) -> None:
             "spec": {},
         },
     )
-    with pytest.raises(HTTPStatusError):
-        assert x.status_code == 400
-        x.raise_for_status()
+    assert x.is_client_error
 
-    del x
+
+async def test_create_campaign(aclient: AsyncClient) -> None:
+    campaign_name = uuid4().hex[-8:]
 
     # Test successful campaign creation
     x = await aclient.post(
@@ -93,7 +92,6 @@ async def test_async_create_campaign(aclient: AsyncClient) -> None:
             "spec": {},
         },
     )
-
     assert x.is_success
 
     campaign = x.json()
@@ -124,7 +122,7 @@ async def test_async_create_campaign(aclient: AsyncClient) -> None:
     assert len(edges) == 0
 
 
-async def test_async_patch_campaign(aclient: AsyncClient) -> None:
+async def test_patch_campaign(aclient: AsyncClient) -> None:
     # Create a new campaign with spec data
     campaign_name = uuid4().hex[-8:]
     x = await aclient.post(

tests/v2/test_manifest_routes.py

@@ -9,14 +9,14 @@
 """All tests in this module will run in the same event loop."""
 
 
-async def test_async_list_manifests(aclient: AsyncClient) -> None:
+async def test_list_manifests(aclient: AsyncClient) -> None:
     x = await aclient.get("/cm-service/v2/manifests")
     assert x.is_success
     assert len(x.json()) == 0
     assert True
 
 
-async def test_async_load_manifests(aclient: AsyncClient) -> None:
+async def test_load_manifests(aclient: AsyncClient) -> None:
     campaign_name = uuid4().hex[-8:]
 
     # Try to create a campaign manifest
@@ -82,6 +82,7 @@ async def test_async_load_manifests(aclient: AsyncClient) -> None:
         json={
             "kind": "campaign",
             "metadata": {"name": campaign_name},
+            "spec": {},
         },
     )
     assert x.is_success
@@ -128,9 +129,10 @@ async def test_async_load_manifests(aclient: AsyncClient) -> None:
     assert x.is_success
     manifests = x.json()
     assert len(manifests) == 3
+    assert manifests[-1]["spec"]["one"] == 1
 
 
-async def test_async_patch_manifest(aclient: AsyncClient) -> None:
+async def test_patch_manifest(aclient: AsyncClient) -> None:
     """Tests partial update of manifests and single resource retrieval."""
     campaign_name = uuid4().hex[-8:]
     manifest_name = uuid4().hex[-8:]