diff --git a/docs/Explanations/manifests.md b/docs/Explanations/manifests.md index 0427858..95b8b46 100644 --- a/docs/Explanations/manifests.md +++ b/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. diff --git a/docs/Reference/api/index.md b/docs/Reference/api/index.md index 4c69b15..5d63eec 100644 --- a/docs/Reference/api/index.md +++ b/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 diff --git a/src/fwl_io/__init__.py b/src/fwl_io/__init__.py index f455c6f..aa46276 100644 --- a/src/fwl_io/__init__.py +++ b/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__', diff --git a/src/fwl_io/manifest.py b/src/fwl_io/manifest.py index e15039e..0c45dba 100644 --- a/src/fwl_io/manifest.py +++ b/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}.')) diff --git a/tests/test_manifest.py b/tests/test_manifest.py index 50bc943..824a638 100644 --- a/tests/test_manifest.py +++ b/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 ` 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'