feat(grz-db,grz-common): clean up populate API surface

Four related changes to the populate-side public surface:

- Split 'force' from 'on_missing' on SubmissionDb.populate. 'force'
  previously double-duty'd as 'allow destructive overwrites' AND
  'silently treat a missing submission as fresh-create'. Add explicit
  'on_missing: Literal["create", "error"]' defaulting to 'error'
  (raises SubmissionNotFoundError, matching grzctl 'db submission
  populate'). The download path opts into 'create' explicitly.

- Drop the now-redundant implicit 'force=True' after auto-create.
  Verified: 'Diff.classify' produces only NEW states when old_value is
  None, so 'has_pending_destructive' is always False for a fresh row.

- Drop the 'log: logging.Logger | None' parameter. Use the module
  logger throughout. Idiomatic Python; callers configure via
  'logging.getLogger("grz_db.models.submission")'.

- Drop Worker.load_metadata_with_submission_date. It bundled an S3
  head_object and a local file read into one tuple-returning method
  to keep the download.py call site short, but those are two distinct
  concerns. download.py now calls 'get_metadata_upload_timestamp' and
  'worker_inst.parse_submission().metadata.content' separately.

Author: Hoeze

packages/grz-common/src/grz_common/workers/worker.py

@@ -2,7 +2,6 @@
 
 from __future__ import annotations
 
-import datetime
 import logging
 from os import PathLike
 from pathlib import Path
@@ -12,12 +11,10 @@
     EncryptionError,
     IncompleteSubmissionError,
 )
-from grz_pydantic_models.submission.metadata import GrzSubmissionMetadata
 
 from ..models.identifiers import IdentifiersModel
 from ..models.s3 import S3Options
 from ..progress import EncryptionState, FileProgressLogger, ValidationState
-from ..transfer import get_metadata_upload_timestamp, init_s3_client
 from .download import S3BotoDownloadWorker
 from .submission import EncryptedSubmission, Submission, SubmissionValidationError
 from .upload import S3BotoUploadWorker, UploadError
@@ -312,25 +309,3 @@ def download(self, s3_options: S3Options, submission_id: str, force: bool = Fals
 
         self.__log.info("Downloading encrypted files...")
         download_worker.download(submission_id, EncryptedSubmission(self.metadata_dir, self.encrypted_files_dir))
-
-    def load_metadata_with_submission_date(
-        self, s3_options: S3Options, submission_id: str
-    ) -> tuple[GrzSubmissionMetadata, datetime.date]:
-        """Load the downloaded metadata.json and fetch its S3 last-modified date.
-
-        :param s3_options: S3 connection options for the inbox bucket.
-        :param submission_id: Submission identifier (the top-level S3 prefix).
-        :returns: The parsed :class:`GrzSubmissionMetadata` plus the inbox upload
-            date derived from
-            :func:`grz_common.transfer.get_metadata_upload_timestamp`, suitable
-            for feeding into
-            :meth:`grz_db.models.submission.SubmissionDb.populate`.
-        """
-        s3_client = init_s3_client(s3_options)
-        submission_date = get_metadata_upload_timestamp(s3_client, s3_options.bucket, submission_id).date()
-
-        metadata_file_path = self.metadata_dir / "metadata.json"
-        with open(metadata_file_path) as fd:
-            metadata = GrzSubmissionMetadata.model_validate_json(fd.read())
-
-        return metadata, submission_date

packages/grz-db/src/grz_db/models/submission/__init__.py

@@ -7,7 +7,7 @@
 from collections.abc import Generator, Sequence
 from contextlib import contextmanager
 from operator import attrgetter
-from typing import Any, ClassVar, Optional, Self
+from typing import Any, ClassVar, Literal, Optional, Self
 
 import sqlalchemy as sa
 import sqlalchemy.dialects.postgresql as sa_psql
@@ -1251,36 +1251,35 @@ def _log_pending_changes(
         submission_id: str,
         submission_diff: SubmissionDiffCollection,
         donors_diff: DonorsDiffCollection,
-        log: logging.Logger,
     ) -> None:
         """Emit info-level log lines summarising what is about to be committed."""
         sid = f"Submission: {submission_id}"
 
         pending_keys = [d.key for d in submission_diff.pending]
         unchanged_keys = [d.key for d in submission_diff.unchanged]
         if pending_keys:
-            log.info("%s - Updating fields: %s in database", sid, ", ".join(f'"{k}"' for k in pending_keys))
+            logger.info("%s - Updating fields: %s in database", sid, ", ".join(f'"{k}"' for k in pending_keys))
         if unchanged_keys:
-            log.info("%s - Not updating fields: %s in database", sid, ", ".join(f'"{k}"' for k in unchanged_keys))
+            logger.info("%s - Not updating fields: %s in database", sid, ", ".join(f'"{k}"' for k in unchanged_keys))
 
         if donors_diff.unchanged:
-            log.info(
+            logger.info(
                 "%s - Keep existing donor(s): %s", sid, ", ".join(f'"{d.pseudonym}"' for d in donors_diff.unchanged)
             )
         if donors_diff.added:
-            log.info(
+            logger.info(
                 "%s - Adding new donor(s): %s",
                 sid,
                 ", ".join(f'"{d.pseudonym}"' for d in donors_diff.added),
             )
         if donors_diff.updated:
-            log.info(
+            logger.info(
                 "%s - Modifying existing donor(s): %s",
                 sid,
                 ", ".join(f'"{d.pseudonym}"' for d in donors_diff.updated),
             )
         if donors_diff.deleted:
-            log.info("%s - Dropping donor(s): %s", sid, ", ".join(f'"{d.pseudonym}"' for d in donors_diff.deleted))
+            logger.info("%s - Dropping donor(s): %s", sid, ", ".join(f'"{d.pseudonym}"' for d in donors_diff.deleted))
 
     @staticmethod
     def assert_metadata_not_redacted(
@@ -1317,38 +1316,44 @@ def populate(  # noqa: PLR0913
         submission_date: datetime.date | None,
         *,
         force: bool = False,
+        on_missing: Literal["create", "error"] = "error",
         ignore_fields: set[str] | None = None,
-        log: logging.Logger | None = None,
     ) -> None:
         """Reconcile DB state for ``submission_id`` with ``metadata``.
 
-        Creates the submission row if absent (then forces overwrite). Rejects
-        redacted ``tan_g`` or missing/redacted ``local_case_id`` via
+        Rejects redacted ``tan_g`` or missing/redacted ``local_case_id`` via
         :meth:`assert_metadata_not_redacted` unless the corresponding key
         (``"tan_g"`` or ``"pseudonym"``) is in ``ignore_fields``. Computes diffs
         via :meth:`diff`, rejects destructive changes unless ``force``, and
-        commits via :meth:`commit_changes`.
+        commits via :meth:`commit_changes`. Operational progress is logged via
+        the module-level logger; callers configure verbosity through
+        ``logging.getLogger("grz_db.models.submission")``.
 
         :param submission_id: ID of the submission being populated.
         :param metadata: Parsed submission metadata.
         :param submission_date: S3 last-modified date of the metadata file, if known.
-        :param force: If ``True``, allow destructive updates/deletes.
+        :param force: If ``True``, allow destructive updates/deletes of existing fields.
+        :param on_missing: What to do when ``submission_id`` is not yet in the
+            database. ``"error"`` (default) raises :class:`SubmissionNotFoundError`.
+            ``"create"`` calls :meth:`add_submission` first and then proceeds; the
+            resulting diff against the fresh row contains only additive changes,
+            so ``force`` does not need to be set.
         :param ignore_fields: Field keys to skip during diff and redaction
             validation. See :meth:`assert_metadata_not_redacted`.
-        :param log: Logger for info/warning summaries; defaults to module logger.
+        :raises SubmissionNotFoundError: if the submission row is absent and
+            ``on_missing`` is ``"error"``.
         :raises ValueError: if ``tan_g`` or ``local_case_id`` is redacted/missing
             and the corresponding key is not in ``ignore_fields``.
         :raises RuntimeError: if pending changes are destructive and ``force`` is False.
         """
-        if log is None:
-            log = logger
         ignore_fields = ignore_fields or set()
 
         if not self.get_submission(submission_id):
-            log.warning("Submission %s does not exist. Creating ...", submission_id)
+            if on_missing == "error":
+                raise SubmissionNotFoundError(submission_id)
+            logger.warning("Submission %s does not exist. Creating ...", submission_id)
             self.add_submission(submission_id)
-            log.debug("Submission %s added to database. Force populate", submission_id)
-            force = True
+            logger.debug("Submission %s added to database.", submission_id)
 
         self.assert_metadata_not_redacted(metadata, submission_id, ignore_fields)
 
@@ -1367,8 +1372,8 @@ def populate(  # noqa: PLR0913
             )
 
         if submission_diff.has_pending or donors_diff.has_pending:
-            log.info("Submission: %s - Updating...", submission_id)
-            self._log_pending_changes(submission_id, submission_diff, donors_diff, log)
+            logger.info("Submission: %s - Updating...", submission_id)
+            self._log_pending_changes(submission_id, submission_diff, donors_diff)
             self.commit_changes(submission_id, submission_diff, donors_diff)
         else:
-            log.info("Submission: %s - No updates necessary.", submission_id)
+            logger.info("Submission: %s - No updates necessary.", submission_id)

packages/grzctl/src/grzctl/commands/download.py

@@ -6,6 +6,7 @@
 
 import click
 import grz_common.cli as grzcli
+from grz_common.transfer import get_metadata_upload_timestamp, init_s3_client
 from grz_common.workers.worker import Worker
 from grz_db.models.submission import SubmissionStateEnum
 
@@ -72,7 +73,15 @@ def download(  # noqa: PLR0913
             if not db_context.db:
                 log.warning("Database context is not available, skipping population of submission metadata in DB.")
             else:
-                metadata, submission_date = worker_inst.load_metadata_with_submission_date(config.s3, submission_id)
-                db_context.db.populate(submission_id, metadata, submission_date, force=force, log=log)
+                s3_client = init_s3_client(config.s3)
+                submission_date = get_metadata_upload_timestamp(s3_client, config.s3.bucket, submission_id).date()
+                metadata = worker_inst.parse_submission().metadata.content
+                db_context.db.populate(
+                    submission_id,
+                    metadata,
+                    submission_date,
+                    force=force,
+                    on_missing="create",
+                )
 
     log.info("Download finished!")

tests/cli/test_db_context.py

@@ -270,15 +270,19 @@ def test_db_wrappers(
     )
 
     with mock_command(command_spec, submission_id) as (mock_worker, mock_extra):
+        # download.py's --populate path calls `worker.parse_submission().metadata.content`
+        # plus `get_metadata_upload_timestamp(...)`. Configure the mocked Worker and patch
+        # the S3 helpers so the populate runs end-to-end against the real db. Harmless for
+        # commands that never call these.
         if mock_worker is not None:
-            # download.py's --populate path calls Worker.load_metadata_with_submission_date
-            # and unpacks (metadata, submission_date). Provide a usable return value so the
-            # mocked Worker plays along; harmless for commands that never call the method.
-            mock_worker.load_metadata_with_submission_date.return_value = (
-                parsed_metadata,
-                datetime.date.today(),
-            )
-        result = runner.invoke(cli, args)
+            mock_worker.parse_submission.return_value.metadata.content = parsed_metadata
+        with (
+            patch("grzctl.commands.download.init_s3_client") as mock_init_s3,
+            patch("grzctl.commands.download.get_metadata_upload_timestamp") as mock_get_ts,
+        ):
+            mock_init_s3.return_value = MagicMock()
+            mock_get_ts.return_value.date.return_value = datetime.date.today()
+            result = runner.invoke(cli, args)
 
         assert result.exit_code == 0, f"Command failed: {result.output}"