feat(api): Refactor validating manifests

feat(api): Refactor campaign and node routes

Author: tcjennings

src/lsst/cmservice/db/manifests_v2.py

@@ -1,30 +1,101 @@
-"""Module for models representing generic CM Service manifests."""
+"""Module for models representing generic CM Service manifests.
 
-from pydantic import AliasChoices
-from sqlmodel import Field, SQLModel
+These manifests are used in APIs, especially when creating resources. They do
+not necessarily represent the object's database or ORM model.
+"""
 
-from ..common.enums import ManifestKind
+from typing import Self
+
+from pydantic import AliasChoices, BaseModel, ConfigDict, Field, ValidationInfo, model_validator
+
+from ..common.enums import DEFAULT_NAMESPACE, ManifestKind
 from ..common.types import KindField
 
 
-class ManifestWrapper(SQLModel):
-    """A model for an object's Manifest wrapper, used by APIs where the `spec`
-    should be the kind's table model, more or less.
+class Manifest[MetadataT, SpecT](BaseModel):
+    """A parameterized model for an object's Manifest, used by APIs where the
+    `spec` should be the kind's table model, more or less.
     """
 
     apiversion: str = Field(default="io.lsst.cmservice/v1")
     kind: KindField = Field(default=ManifestKind.other)
-    metadata_: dict = Field(
-        default_factory=dict,
-        schema_extra={
-            "validation_alias": AliasChoices("metadata", "metadata_"),
-            "serialization_alias": "metadata",
-        },
+    metadata_: MetadataT = Field(
+        validation_alias=AliasChoices("metadata", "metadata_"),
+        serialization_alias="metadata",
     )
-    spec: dict = Field(
-        default_factory=dict,
-        schema_extra={
-            "validation_alias": AliasChoices("spec", "configuration", "data"),
-            "serialization_alias": "spec",
-        },
+    spec: SpecT = Field(
+        validation_alias=AliasChoices("spec", "configuration", "data"),
+        serialization_alias="spec",
     )
+
+
+class ManifestMetadata(BaseModel):
+    """Generic metadata model for Manifests.
+
+    Conventionally denormalized fields are excluded from the model_dump when
+    serialized for ORM use.
+    """
+
+    name: str
+    namespace: str
+
+
+class ManifestSpec(BaseModel):
+    """Generic spec model for Manifests.
+
+    Notes
+    -----
+    Any spec body is allowed via config, but any fields that aren't first-class
+    fields won't be subject to validation or available as model attributes
+    except in the ``__pydantic_extra__`` dictionary. The full spec will be
+    expressed via ``model_dump()``.
+    """
+
+    model_config = ConfigDict(extra="allow")
+
+
+class VersionedMetadata(ManifestMetadata):
+    """Metadata model for versioned Manifests."""
+
+    version: int = 0
+
+
+class ManifestModelMetadata(VersionedMetadata):
+    """Manifest model for general Manifests. These manifests are versioned but
+    a namespace is optional.
+    """
+
+    namespace: str = Field(default=str(DEFAULT_NAMESPACE))
+
+
+class ManifestModel(Manifest[ManifestModelMetadata, ManifestSpec]):
+    """Manifest model for generic Manifest handling."""
+
+    @model_validator(mode="after")
+    def custom_model_validator(self, info: ValidationInfo) -> Self:
+        """Validate an Campaign Manifest after a model has been created."""
+        if self.kind in [ManifestKind.campaign, ManifestKind.node, ManifestKind.edge]:
+            raise ValueError(f"Manifests may not be a {self.kind.name} kind.")
+
+        return self
+
+
+class CampaignMetadata(BaseModel):
+    """Metadata model for a Campaign Manifest.
+
+    Campaign metadata does not require a namespace field.
+    """
+
+    name: str
+
+
+class CampaignManifest(Manifest[CampaignMetadata, ManifestSpec]):
+    """validating model for campaigns"""
+
+    @model_validator(mode="after")
+    def custom_model_validator(self, info: ValidationInfo) -> Self:
+        """Validate an Campaign Manifest after a model has been created."""
+        if self.kind is not ManifestKind.campaign:
+            raise ValueError("Campaigns may only be created from a <campaign> manifest")
+
+        return self

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
@@ -214,7 +196,7 @@ async def update_manifest_resource(
     - This API always targets the latest version of a manifest when applying
       a patch. This requires and maintains a "linear" sequence of versions;
       it is not permissible to "patch" a previous version and create a "tree"-
-      like history of manifests. For exmaple, every manifest may be diffed
+      like history of manifests. For example, every manifest may be diffed
       against any previous version without having to consider branches.
     """
     use_rfc6902 = False

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(