Name a manifest that disagrees with the installed fwl-io

docs/Explanations/manifests.md

@@ -24,9 +24,19 @@ Validation at load time:
 - `required_by`, when present, must be a list of model names.
 - `extract`, when present, must be `"tar"` or `"zip"`.
 - A dataset table must not contain sub-tables, and arrays of tables are rejected; ambiguous structures fail loudly instead of being silently dropped.
+- A dataset table declares only the fields above, and a grouping level declares none: anything else raises `ManifestSchemaError`, naming the field and the manifest schema this fwl-io implements. A model ships its manifest with its own code, so a manifest can be newer than the installed fwl-io; ignoring an unknown field silently would leave the manifest asking for something it never gets, and a `required_by` written one level above its dataset would leave the dataset claiming no model needs it. The message names the action that fits the case: move a dataset field that sits too high, delete a field this fwl-io no longer takes, and for a name it does not know at all, check the spelling or upgrade. Two things are outside the check. A scalar at the manifest root is reserved for a manifest's own settings and is ignored, unless it names a dataset field or `subdir`. And a table is recognised as a dataset by its `zenodo` key, so a misspelt `zenodo` is reported as a table with no pin rather than as an unknown field.
+- A manifest that fails to load takes its whole provider with it: `discover_manifests` skips that package and logs a warning, so its other datasets disappear from the result too. Use `fwl-io list` to see the error.
 - A `subdir` field is rejected on any table, dataset or grouping level, and at the manifest root: the location comes from the key. A table *named* `subdir` is an ordinary directory level.
 - Within one manifest, two keys that differ only in case are rejected: they would share one directory and one registry file on a case-insensitive filesystem. Two installed packages declaring keys that collide is a separate check, tracked in [#18](https://github.com/FormingWorlds/fwl-io/issues/18).
 
+### Schema versions
+
+| Schema | From | What it means |
+|---|---|---|
+| 1 | 26.7.22 | A dataset's location is derived from its dotted table key; `subdir` is not a field. |
+
+An error raised while reading a manifest names the schema the running code implements, so a mismatch between a manifest and an installed fwl-io can be placed against this table.
+
 ## Archive datasets
 
 A deposit packaged as a single archive sets `extract = "tar"` or `"zip"`. Its registry lists the one archive file and its checksum; the fetcher downloads and verifies the archive, then extracts the members into the dataset directory and discards the archive, so consumers see the extracted tree rather than a tarball. Extraction is staged and the tree is moved into place atomically, so an interrupted fetch never leaves a half-populated dataset, and any member that escapes the directory (an absolute path or a `..` component) or is not a plain file or directory (a symlink, hardlink, or device node) is rejected before anything is written.

docs/Reference/api/index.md

@@ -9,13 +9,14 @@ from fwl_io import (
     fetch_for, Dataset,
     resolve_data_root, resolve_cache_root,
     DownloadError, OfflineDataError, MissingDataRootError,
+    ManifestSchemaError,
 )
 ```
 
 Per-module reference pages:
 
 - [Fetching](fetch.md): `Fetcher`, `create_fetcher`, error types
-- [Manifests](manifest.md): `Dataset`, `load_manifest`, `discover_manifests`, `fetch_for`
+- [Manifests](manifest.md): `Dataset`, `load_manifest`, `discover_manifests`, `fetch_for`, `ManifestSchemaError`
 - [Registries](registry.md): registry file reading and writing
 - [Sync](sync.md): registry generation from the Zenodo API
 - [Paths](paths.md): data root, shared cache, offline mode

src/fwl_io/__init__.py

@@ -10,7 +10,13 @@
 from importlib.metadata import PackageNotFoundError, version
 
 from fwl_io.fetch import DownloadError, Fetcher, OfflineDataError, create_fetcher
-from fwl_io.manifest import Dataset, discover_manifests, fetch_for, load_manifest
+from fwl_io.manifest import (
+    Dataset,
+    ManifestSchemaError,
+    discover_manifests,
+    fetch_for,
+    load_manifest,
+)
 from fwl_io.paths import MissingDataRootError, resolve_cache_root, resolve_data_root
 
 try:
@@ -22,6 +28,7 @@
     'Dataset',
     'DownloadError',
     'Fetcher',
+    'ManifestSchemaError',
     'MissingDataRootError',
     'OfflineDataError',
     '__version__',

src/fwl_io/manifest.py

@@ -56,6 +56,7 @@
 # Re-exported so existing importers keep working; the parser lives in doi.
 __all__ = [
     'Dataset',
+    'ManifestSchemaError',
     'ZENODO_DOI_PATTERN',
     'discover_manifests',
     'fetch_for',
@@ -67,6 +68,67 @@
 _GENERIC_DOI_PATTERN = re.compile(r'(doi:)?10\.\S+')
 _KEY_SEGMENT_PATTERN = re.compile(r'[A-Za-z0-9_][A-Za-z0-9_-]*')
 
+# Every field a dataset table may declare. A manifest naming anything else is
+# either a typo or written against a schema this fwl-io does not know.
+_DATASET_FIELDS = frozenset({'name', 'zenodo', 'dataverse', 'required_by', 'extract'})
+
+# The manifest schema this code implements, listed in the manifests
+# documentation. Incremented whenever a manifest written for the previous
+# number stops loading. It lives in the source rather than in packaging
+# metadata, so it describes the code actually running, which an editable
+# checkout's recorded version does not.
+_MANIFEST_SCHEMA = 1
+
+
+class ManifestSchemaError(ValueError):
+    """A manifest and the installed fwl-io disagree about the manifest schema.
+
+    Raised when a manifest declares something this fwl-io does not understand,
+    something it no longer understands, or a field it reads only inside a
+    dataset table. Subclasses ValueError, so callers that already handle a
+    malformed manifest keep working.
+    """
+
+
+def _reading_version() -> str:
+    """Describe the code reading the manifest, for use in an error message."""
+    try:
+        from fwl_io import __version__
+
+        distribution = __version__
+    except Exception:  # noqa: BLE001 -- a diagnostic must not raise
+        distribution = 'unknown'
+    return f'manifest schema {_MANIFEST_SCHEMA} (distribution {distribution})'
+
+
+def _unknown_field_error(what: str) -> ManifestSchemaError:
+    """Build the error for a field this fwl-io does not know.
+
+    Both readings are offered because both are common: a misspelt field, and a
+    manifest written against a schema newer than the fwl-io reading it.
+    """
+    return ManifestSchemaError(
+        f'{what}. This fwl-io reads {_reading_version()}: check the spelling, or '
+        f'upgrade fwl-io if the manifest was written against a newer schema.'
+    )
+
+
+def _misplaced_field_error(what: str) -> ManifestSchemaError:
+    """Build the error for a known field declared outside a dataset table."""
+    return ManifestSchemaError(
+        f'{what}. This fwl-io reads {_reading_version()}, in which it is a dataset '
+        f'field: move the line into the dataset table, the one carrying the '
+        f'"zenodo" pin.'
+    )
+
+
+def _dropped_field_error(what: str) -> ManifestSchemaError:
+    """Build the error for a field this fwl-io no longer reads."""
+    return ManifestSchemaError(
+        f'{what}. This fwl-io reads {_reading_version()}, which no longer takes '
+        f'that field: remove the line.'
+    )
+
 
 @dataclass(frozen=True, kw_only=True)
 class Dataset:
@@ -127,7 +189,7 @@ def _reject_declared_subdir(where: str, table: dict, derived: str | None = None)
         if derived
         else 'a dataset location is derived from its own table key'
     )
-    raise ValueError(
+    raise _dropped_field_error(
         f'{where}: "subdir" is not a manifest field; {location} '
         f'(create_fetcher() still takes a subdir argument, manifests do not)'
     )
@@ -147,6 +209,20 @@ def _walk_tables(tree: dict, prefix: str = '') -> list[tuple[str, dict]]:
                 f'{prefix}{name}: arrays of tables ([[...]]) are not supported in manifests'
             )
         if not isinstance(value, dict):
+            if name in _DATASET_FIELDS:
+                where = (
+                    f'grouping table {prefix.rstrip(".")!r} declares {name!r}'
+                    if prefix
+                    else f'the manifest root declares {name!r}'
+                )
+                raise _misplaced_field_error(where)
+            if prefix:
+                raise _unknown_field_error(
+                    f'grouping table {prefix.rstrip(".")!r} declares {name!r}, and a '
+                    f'grouping level takes no fields'
+                )
+            # A root scalar that names no dataset field is a manifest's own
+            # setting.
             continue
         dotted = f'{prefix}{name}'
         _validate_key_segment(name, prefix.rstrip('.'))
@@ -160,6 +236,13 @@ def _walk_tables(tree: dict, prefix: str = '') -> list[tuple[str, dict]]:
         if 'zenodo' in value:
             if has_subtables:
                 raise ValueError(f'dataset {dotted!r}: dataset tables must not contain sub-tables')
+            unknown = sorted(set(value) - _DATASET_FIELDS)
+            if unknown:
+                raise _unknown_field_error(
+                    f'dataset {dotted!r} declares {", ".join(repr(f) for f in unknown)}, '
+                    f'which is not a dataset field '
+                    f'(known fields: {", ".join(sorted(_DATASET_FIELDS))})'
+                )
             leaves.append((dotted, value))
         elif has_subtables:
             leaves.extend(_walk_tables(value, prefix=f'{dotted}.'))

tests/test_manifest.py

@@ -9,6 +9,7 @@
 from fwl_io.fetch import create_fetcher
 from fwl_io.manifest import (
     Dataset,
+    ManifestSchemaError,
     discover_manifests,
     fetch_for,
     load_manifest,
@@ -586,3 +587,199 @@ def test_fetch_for_stamps_each_required_dataset_and_skips_others(tmp_path, monke
     assert stamp.is_file()
     assert json.loads(stamp.read_text())['record_id'] == '111'
     assert not (data_root / 'interior/eos/demo/r222/.fwl-io.json').exists()
+
+
+def test_unknown_dataset_field_reports_both_readings(tmp_path):
+    """A field this fwl-io does not know is a schema disagreement, not a
+    malformed file, and the error offers both readings of it.
+
+    A model ships its manifest with its own code, so the manifest can be newer
+    than the installed fwl-io. Ignoring the field silently would leave the
+    manifest asking for something it never gets. The two causes, a misspelt
+    field and a newer schema, need different actions, so the message names
+    both rather than asserting one.
+    """
+    bad = '[g.d]\nzenodo = "10.5281/zenodo.1"\nchecksum_algorithm = "sha256"\nmirror_priority = 2\n'
+
+    with pytest.raises(ManifestSchemaError) as excinfo:
+        load_manifest(_write(tmp_path, bad))
+
+    message = str(excinfo.value)
+    # Every unknown field is named, not just the first one found.
+    assert "'checksum_algorithm'" in message
+    assert "'mirror_priority'" in message
+    # The accepted set is spelled out in full, so the reader can see what was
+    # expected rather than a sample of it.
+    known = message.split('known fields:')[1]
+    for accepted in ('dataverse', 'extract', 'name', 'required_by', 'zenodo'):
+        assert accepted in known
+    # Both actions are offered, because either cause is plausible.
+    assert 'check the spelling' in message
+    assert 'upgrade fwl-io' in message
+    # Discrimination: the same table without those fields loads, so the error
+    # is the fields and not the table.
+    good = '[g.d]\nzenodo = "10.5281/zenodo.1"\n'
+    assert load_manifest(_write(tmp_path, good))[0].key == 'g.d'
+
+
+def test_error_reports_the_schema_the_running_code_implements(tmp_path):
+    """The message identifies the code doing the reading by its schema number.
+
+    An editable checkout keeps the version recorded at install time, so the
+    distribution version can name a release that contains none of the code
+    actually running. The schema number lives in the source and travels with
+    it, so it cannot go stale that way.
+    """
+    import fwl_io
+    from fwl_io.manifest import _MANIFEST_SCHEMA
+
+    bad = '[g.d]\nzenodo = "10.5281/zenodo.1"\nunknown_field = 1\n'
+
+    with pytest.raises(ManifestSchemaError) as excinfo:
+        load_manifest(_write(tmp_path, bad))
+
+    message = str(excinfo.value)
+    # The schema number, pinned to a literal so a silent renumber is caught.
+    assert _MANIFEST_SCHEMA == 1
+    assert 'manifest schema 1' in message
+    # The packaging version is reported alongside it and labelled as what it
+    # is, so the two are not confused for each other.
+    assert f'distribution {fwl_io.__version__}' in message
+
+
+def test_schema_error_reaches_a_caller_catching_value_error(tmp_path):
+    """A caller that already handles a malformed manifest receives the typed
+    error unchanged, so a consumer needs no new except clause to keep working.
+    """
+    bad = '[g.d]\nzenodo = "10.5281/zenodo.1"\nunknown_field = 1\n'
+
+    with pytest.raises(ValueError) as excinfo:
+        load_manifest(_write(tmp_path, bad))
+
+    # Caught as ValueError, delivered as the specific type.
+    assert isinstance(excinfo.value, ManifestSchemaError)
+    assert type(excinfo.value) is not ValueError
+
+
+def test_every_declared_dataset_field_is_accepted(tmp_path):
+    """The accepted set is exactly the model the loader fills in.
+
+    Discrimination: a field dropped from the set makes this manifest fail, and
+    a field added to the set without a home on the dataset is caught by the
+    comparison against the model rather than by loading.
+    """
+    import dataclasses
+
+    from fwl_io.manifest import _DATASET_FIELDS
+
+    manifest = (
+        '[g.d]\n'
+        'name = "Demo"\n'
+        'zenodo = "10.5281/zenodo.1234567"\n'
+        'dataverse = "10.34894/ABCDEF"\n'
+        'required_by = ["mors"]\n'
+        'extract = "tar"\n'
+    )
+
+    dataset = load_manifest(_write(tmp_path, manifest))[0]
+
+    assert dataset.name == 'Demo'
+    assert dataset.zenodo == '10.5281/zenodo.1234567'
+    assert dataset.dataverse == '10.34894/ABCDEF'
+    assert dataset.required_by == ('mors',)
+    assert dataset.extract == 'tar'
+    # The set cannot drift open: it is the dataset model minus the two fields
+    # the loader derives rather than reads.
+    derived = {'key', 'registry_path'}
+    assert _DATASET_FIELDS == {f.name for f in dataclasses.fields(Dataset)} - derived
+    # The location is derived, so re-admitting it as a field would undo that.
+    assert 'subdir' not in _DATASET_FIELDS
+
+
+def test_declared_subdir_is_reported_as_a_dropped_field(tmp_path):
+    """A manifest still declaring `subdir` is the other direction of the same
+    disagreement: the manifest is older than the fwl-io reading it, so the
+    action is to delete the line, not to upgrade.
+    """
+    bad = '[g.d]\nzenodo = "10.5281/zenodo.1"\nsubdir = "somewhere/else"\n'
+
+    with pytest.raises(ManifestSchemaError) as excinfo:
+        load_manifest(_write(tmp_path, bad))
+
+    message = str(excinfo.value)
+    assert 'subdir' in message
+    # The location is derived, and the message says where to.
+    assert 'g/d' in message
+    assert 'remove the line' in message
+    # The reading code is named here too, so every branch can be placed
+    # against the schema table.
+    assert 'manifest schema 1' in message
+    # Discrimination: upgrading fwl-io is the wrong action here, and offering
+    # it would send the reader in the opposite direction.
+    assert 'upgrade fwl-io' not in message
+
+
+def test_dataset_field_written_one_level_too_high_is_rejected(tmp_path):
+    """A dataset field on a grouping table is refused rather than dropped.
+
+    A `required_by` on the grouping table would leave the dataset claiming no
+    model needs it, so `fwl-io fetch <model>` would fetch nothing while the
+    manifest said otherwise. It is refused instead.
+    """
+    misplaced = '[star]\nrequired_by = ["mors"]\n[star.tracks]\nzenodo = "10.5281/zenodo.1"\n'
+
+    with pytest.raises(ManifestSchemaError) as excinfo:
+        load_manifest(_write(tmp_path, misplaced))
+
+    message = str(excinfo.value)
+    assert "'required_by'" in message
+    assert "'star'" in message
+    # The action is to move the line, since the field is spelled correctly and
+    # no fwl-io reads it where it sits.
+    assert 'move the line into the dataset table' in message
+    assert 'manifest schema 1' in message
+    assert 'check the spelling' not in message
+    assert 'upgrade fwl-io' not in message
+    # Discrimination: the same field inside the dataset table is read, so the
+    # rejection is about where it sits, not about the field itself.
+    correct = '[star.tracks]\nzenodo = "10.5281/zenodo.1"\nrequired_by = ["mors"]\n'
+    assert load_manifest(_write(tmp_path, correct))[0].required_by == ('mors',)
+
+
+def test_unknown_name_on_a_grouping_level_keeps_the_two_readings(tmp_path):
+    """A name that is not a dataset field at all gets the spelling-or-upgrade
+    advice wherever it appears, because either cause remains possible.
+
+    Discrimination against the misplaced-field case: that one names an action
+    that only applies to a field this fwl-io does read.
+    """
+    bad = '[star]\nchecksum_algorithm = "sha256"\n[star.tracks]\nzenodo = "10.5281/zenodo.1"\n'
+
+    with pytest.raises(ManifestSchemaError) as excinfo:
+        load_manifest(_write(tmp_path, bad))
+
+    message = str(excinfo.value)
+    assert "'checksum_algorithm'" in message
+    assert 'check the spelling' in message
+    assert 'move the line' not in message
+
+
+def test_dataset_field_at_the_manifest_root_is_rejected(tmp_path):
+    """A dataset field at the root is the same misplacement as one on a
+    grouping level: for a top-level dataset table, the root is the level above
+    it, so a `required_by` there would leave the dataset claiming no model
+    needs it.
+    """
+    misplaced = 'required_by = ["mors"]\n[demo]\nzenodo = "10.5281/zenodo.1"\n'
+
+    with pytest.raises(ManifestSchemaError) as excinfo:
+        load_manifest(_write(tmp_path, misplaced))
+
+    message = str(excinfo.value)
+    assert "'required_by'" in message
+    assert 'move the line into the dataset table' in message
+    # Discrimination: a root scalar that names no dataset field is a
+    # manifest's own setting and still loads, so the rejection is about the
+    # name and not about scalars at the root.
+    setting = 'schema_version = 1\n[demo]\nzenodo = "10.5281/zenodo.1"\n'
+    assert load_manifest(_write(tmp_path, setting))[0].key == 'demo'