diff --git a/docs/Explanations/manifests.md b/docs/Explanations/manifests.md index 95b8b46..1cdc381 100644 --- a/docs/Explanations/manifests.md +++ b/docs/Explanations/manifests.md @@ -25,6 +25,7 @@ Validation at load time: - `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 may declare the schema it was written against with a root `manifest_schema = `. It is optional, and a manifest that declares one is held to it: only the schema the installed fwl-io implements is accepted. A higher number means the reader is too old, so the error says to upgrade. A lower one means the manifest was written for a schema that stopped loading when the number rose, so the error names both numbers and points here. Accepting only the implemented number is what sharpens the rest: an unknown field in a manifest that declares its schema is reported as a misspelling alone, with no second reading to weigh. The value must be a whole number of at least 1; `true` is rejected rather than read as 1. A table *named* `manifest_schema` is an ordinary directory level, as with `subdir`, and the key written inside a table is reported as misplaced rather than misspelt. - 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). @@ -37,6 +38,8 @@ Validation at load time: 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. +A manifest that declares `manifest_schema` is checked against it directly, and only the implemented number is accepted. Incrementing the schema is therefore a breaking change for any manifest that declares the old one, which is the intent: the number rises precisely when manifests written for the previous one stop loading, so they should fail at the increment with a message naming both numbers rather than part way through a load with a message about some individual field. A manifest that declares nothing is read on a best-effort basis, as before. + ## 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/How-to/add_dataset.md b/docs/How-to/add_dataset.md index cc3af9a..3e5c29c 100644 --- a/docs/How-to/add_dataset.md +++ b/docs/How-to/add_dataset.md @@ -20,12 +20,16 @@ Choose the manifest: Add a table for the dataset: ```toml +manifest_schema = 1 + [interior.eos.wolf_bower_2018] name = "Wolf & Bower (2018) MgSiO3 equation of state" zenodo = "10.5281/zenodo.1234567" required_by = ["aragog", "zalmoxis", "spider"] ``` +The root `manifest_schema` names the schema the file is written against. It is optional and worth declaring: it lets fwl-io tell a manifest written for a different schema from a misspelt field, so a load failure names the one that applies instead of offering both. Declaring it also means the manifest has to be updated when the schema number rises, which is the point, since that is when manifests written for the previous number stop loading. The current schema is in the [schema versions](../Explanations/manifests.md#schema-versions) table. + The dotted key is the location below `FWL_DATA`, so this dataset lands in `interior/eos/wolf_bower_2018/r`, the version directory named for its Zenodo record. Choose the key to follow the [target layout](../Explanations/manifests.md#the-fwl_data-layout), using only letters, digits, `_` and `-` per segment, each starting with a letter, digit or `_`. `required_by` lists the models whose `fwl-io fetch ` should include this dataset. If the deposit is a single archive that consumers expect unpacked, add `extract = "tar"` or `extract = "zip"`; the archive is downloaded, checksum-verified, and unpacked into the dataset directory. See [Archive datasets](../Explanations/manifests.md#archive-datasets). diff --git a/src/fwl_io/data/shared_manifest.toml b/src/fwl_io/data/shared_manifest.toml index 821ad41..8b0852d 100644 --- a/src/fwl_io/data/shared_manifest.toml +++ b/src/fwl_io/data/shared_manifest.toml @@ -1,6 +1,8 @@ # Datasets shared by several models of the PROTEUS ecosystem. # -# One TOML table per dataset: +# One TOML table per dataset, below an optional schema declaration: +# +# manifest_schema = 1 # # [group.dataset_key] # name = "Human-readable dataset name" @@ -23,3 +25,5 @@ # No dataset is shared across several models yet, so this manifest declares # none. The Baraffe stellar tracks ship with the MORS package, which owns their # manifest and registry. + +manifest_schema = 1 diff --git a/src/fwl_io/manifest.py b/src/fwl_io/manifest.py index 0c45dba..89bf5f2 100644 --- a/src/fwl_io/manifest.py +++ b/src/fwl_io/manifest.py @@ -8,6 +8,8 @@ Manifest schema, one table per dataset, identified by its ``zenodo`` key:: + manifest_schema = 1 # optional, see below + [interior.eos.wolf_bower_2018] name = "Wolf & Bower (2018) MgSiO3 equation of state" zenodo = "10.5281/zenodo.1234567" # version DOI, never a concept DOI @@ -15,6 +17,14 @@ required_by = ["aragog", "zalmoxis", "spider"] extract = "tar" # optional: unpack a single-archive deposit +The optional root ``manifest_schema`` names the schema the file was written +against. A manifest that declares one is held to it: only the schema the +installed fwl-io implements is accepted, a higher number meaning the reader is +too old and a lower one meaning the manifest was written for a schema that +stopped loading when the number rose. That is what sharpens the diagnosis +elsewhere, since an unknown field in such a manifest can only be a +misspelling. + The dotted table key is the dataset location below the data root: the table above resolves into ``interior/eos/wolf_bower_2018``. Key segments are restricted to letters, digits, ``_`` and ``-``, each starting with a letter, @@ -79,6 +89,12 @@ # checkout's recorded version does not. _MANIFEST_SCHEMA = 1 +# The root key by which a manifest states the schema it was written against. +# Declaring it is optional and turns an ambiguous diagnosis into a definite +# one: a manifest that names a schema this code does not implement is from the +# future and says so, rather than being reported as a possible typo. +_SCHEMA_KEY = 'manifest_schema' + class ManifestSchemaError(ValueError): """A manifest and the installed fwl-io disagree about the manifest schema. @@ -101,18 +117,86 @@ def _reading_version() -> str: return f'manifest schema {_MANIFEST_SCHEMA} (distribution {distribution})' -def _unknown_field_error(what: str) -> ManifestSchemaError: +def _unknown_field_error(what: str, declared_schema: int | None = None) -> 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. + Without a declared schema 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. A manifest that declares one has ruled the + second reading out by the time this is reached, since the reader admits + only the schema this code implements, so the message names the typo alone. + The equality is restated here rather than assumed, so that relaxing the + reader later cannot silently sharpen the message for a schema it should + not apply to. """ + if declared_schema == _MANIFEST_SCHEMA: + return ManifestSchemaError( + f'{what}. The manifest declares {_SCHEMA_KEY} {declared_schema}, the schema ' + f'this fwl-io implements, so the field is misspelt rather than newer than ' + f'this code.' + ) 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 _read_declared_schema(tree: dict) -> int | None: + """Return the schema a manifest declares at its root, if it declares one. + + Only the schema this code implements is admitted. A higher number means the + reader is too old. A lower one means the manifest is written for a schema + that, by the rule the number follows, stopped loading when it was + incremented; refusing it fails at the increment rather than part way + through a load. Either way the message names both numbers. Raises too when + the value is not a schema number at all. + """ + if _SCHEMA_KEY not in tree: + return None + declared = tree[_SCHEMA_KEY] + # A table of this name is a directory level like any other, the same rule + # `subdir` follows: the reserved name applies to the scalar, not to a + # dataset an author happens to have called this. Leave it to the walk. + if isinstance(declared, dict) or ( + isinstance(declared, list) and any(isinstance(item, dict) for item in declared) + ): + return None + # bool is an int subclass, and `manifest_schema = true` is a mistake worth + # naming rather than reading as schema 1. + if isinstance(declared, bool) or not isinstance(declared, int) or declared < 1: + raise ManifestSchemaError( + f'the manifest root declares {_SCHEMA_KEY} {declared!r}; it must be a whole ' + f'number of at least 1, naming the manifest schema the file was written ' + f'against.' + ) + if declared > _MANIFEST_SCHEMA: + raise ManifestSchemaError( + f'the manifest declares {_SCHEMA_KEY} {declared}, but this fwl-io reads ' + f'{_reading_version()}: upgrade fwl-io to read this manifest.' + ) + if declared < _MANIFEST_SCHEMA: + raise ManifestSchemaError( + f'the manifest declares {_SCHEMA_KEY} {declared}, but this fwl-io reads ' + f'manifest schema {_MANIFEST_SCHEMA}: the schema number rises when a ' + f'manifest written for the previous one stops loading, so update the ' + f'manifest against the schema versions table in the manifests ' + f'documentation.' + ) + return declared + + +def _misplaced_schema_error(what: str) -> ManifestSchemaError: + """Build the error for the reserved schema key written below the root. + + The name is spelt correctly and sits at the wrong level, so reporting it as + an unknown field would give the one reading that is certainly wrong. + """ + return ManifestSchemaError( + f'{what}. {_SCHEMA_KEY!r} is a manifest-root key naming the schema the file was ' + f'written against: move the line above the first table.' + ) + + def _misplaced_field_error(what: str) -> ManifestSchemaError: """Build the error for a known field declared outside a dataset table.""" return ManifestSchemaError( @@ -195,7 +279,9 @@ def _reject_declared_subdir(where: str, table: dict, derived: str | None = None) ) -def _walk_tables(tree: dict, prefix: str = '') -> list[tuple[str, dict]]: +def _walk_tables( + tree: dict, prefix: str = '', declared_schema: int | None = None +) -> list[tuple[str, dict]]: """Return (dotted-key, table) pairs for the dataset tables of a manifest. A table is a dataset when it carries the ``zenodo`` key. Grouping tables @@ -217,12 +303,18 @@ def _walk_tables(tree: dict, prefix: str = '') -> list[tuple[str, dict]]: ) raise _misplaced_field_error(where) if prefix: + if name == _SCHEMA_KEY: + raise _misplaced_schema_error( + f'grouping table {prefix.rstrip(".")!r} declares {name!r}' + ) raise _unknown_field_error( f'grouping table {prefix.rstrip(".")!r} declares {name!r}, and a ' - f'grouping level takes no fields' + f'grouping level takes no fields', + declared_schema, ) # A root scalar that names no dataset field is a manifest's own - # setting. + # setting. The schema declaration is one of those, already read + # and validated before the walk began. continue dotted = f'{prefix}{name}' _validate_key_segment(name, prefix.rstrip('.')) @@ -237,15 +329,18 @@ def _walk_tables(tree: dict, prefix: str = '') -> list[tuple[str, dict]]: if has_subtables: raise ValueError(f'dataset {dotted!r}: dataset tables must not contain sub-tables') unknown = sorted(set(value) - _DATASET_FIELDS) + if _SCHEMA_KEY in unknown: + raise _misplaced_schema_error(f'dataset {dotted!r} declares {_SCHEMA_KEY!r}') 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))})' + f'(known fields: {", ".join(sorted(_DATASET_FIELDS))})', + declared_schema, ) leaves.append((dotted, value)) elif has_subtables: - leaves.extend(_walk_tables(value, prefix=f'{dotted}.')) + leaves.extend(_walk_tables(value, prefix=f'{dotted}.', declared_schema=declared_schema)) else: raise ValueError( f'table {dotted!r} has no "zenodo" key, so it is neither a dataset nor a ' @@ -261,10 +356,13 @@ def load_manifest(path: str | Path) -> list[Dataset]: with path.open('rb') as fh: tree = tomllib.load(fh) + # Read the declaration first: a manifest above this code's schema cannot be + # judged by this code's rules, so it must be refused before they are applied. + declared_schema = _read_declared_schema(tree) _reject_declared_subdir('the manifest root', tree) datasets: list[Dataset] = [] folded: dict[str, str] = {} - for key, table in _walk_tables(tree): + for key, table in _walk_tables(tree, declared_schema=declared_schema): clash = folded.setdefault(key.lower(), key) if clash != key: raise ValueError( diff --git a/tests/test_manifest.py b/tests/test_manifest.py index 824a638..7cf6ddd 100644 --- a/tests/test_manifest.py +++ b/tests/test_manifest.py @@ -6,6 +6,7 @@ import pooch import pytest +from fwl_io import manifest from fwl_io.fetch import create_fetcher from fwl_io.manifest import ( Dataset, @@ -783,3 +784,114 @@ def test_dataset_field_at_the_manifest_root_is_rejected(tmp_path): # 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' + + +DEMO = '[demo]\nzenodo = "10.5281/zenodo.1"\n' + + +def test_a_manifest_from_a_newer_schema_says_so(tmp_path): + """A schema above the implemented one can only mean the reader is too old.""" + with pytest.raises(ManifestSchemaError, match='upgrade fwl-io') as excinfo: + load_manifest(_write(tmp_path, f'manifest_schema = 99\n{DEMO}')) + # The spelling reading is dropped: nothing about this file is misspelt. + assert 'spelling' not in str(excinfo.value) + # The same file at the implemented schema loads, so the refusal is the + # number's doing and not the declaration's presence. + assert load_manifest(_write(tmp_path, f'manifest_schema = 1\n{DEMO}'))[0].key == 'demo' + + +def test_declaring_the_implemented_schema_makes_an_unknown_field_a_typo(tmp_path): + """Declaring the schema this code implements rules out the newer-field reading.""" + body = '[demo]\nzenodo = "10.5281/zenodo.1"\nrequired_bye = ["proteus"]\n' + with pytest.raises(ManifestSchemaError, match='misspelt') as declared: + load_manifest(_write(tmp_path, f'manifest_schema = 1\n{body}')) + assert 'upgrade fwl-io' not in str(declared.value) + # Without the declaration the same file still offers both readings, so the + # sharper message is attributable to the declaration. + with pytest.raises(ManifestSchemaError, match='upgrade fwl-io'): + load_manifest(_write(tmp_path, body)) + + +def test_the_sharper_message_reaches_nested_and_grouping_tables(tmp_path): + """The declaration sharpens every unknown-field message, at any depth.""" + nested = 'manifest_schema = 1\n[grp.demo]\nzenodo = "10.5281/zenodo.1"\nrequired_bye = ["p"]\n' + with pytest.raises(ManifestSchemaError, match='misspelt') as deep: + load_manifest(_write(tmp_path, nested)) + assert 'upgrade fwl-io' not in str(deep.value), ( + 'a dataset below a grouping level lost the declaration on the way down' + ) + + grouping = ( + 'manifest_schema = 1\n[grp]\nnickname = "x"\n[grp.demo]\nzenodo = "10.5281/zenodo.1"\n' + ) + with pytest.raises(ManifestSchemaError, match='misspelt') as level: + load_manifest(_write(tmp_path, grouping)) + assert 'upgrade fwl-io' not in str(level.value), 'a grouping level lost the declaration' + + +@pytest.mark.parametrize( + 'value', + ['0', '-1', '"1"', '1.5', 'true'], + ids=['zero', 'negative', 'text', 'fractional', 'boolean'], +) +def test_the_schema_declaration_must_be_a_schema_number(tmp_path, value): + """Anything that is not a positive whole number is a mistake, including true.""" + with pytest.raises(ManifestSchemaError, match='whole number') as excinfo: + load_manifest(_write(tmp_path, f'manifest_schema = {value}\n{DEMO}')) + assert 'manifest_schema' in str(excinfo.value) + + +def test_a_table_named_for_the_schema_key_is_an_ordinary_directory(tmp_path): + """The reserved name applies to the scalar, as with subdir, not to a table.""" + dataset = '[manifest_schema]\nzenodo = "10.5281/zenodo.1"\n' + assert load_manifest(_write(tmp_path, dataset))[0].key == 'manifest_schema' + # Also as a grouping level, where the walk has to recurse past it. + nested = '[manifest_schema.demo]\nzenodo = "10.5281/zenodo.1"\n' + assert load_manifest(_write(tmp_path, nested))[0].key == 'manifest_schema.demo' + + +def test_the_schema_key_below_the_root_is_misplaced_rather_than_misspelt(tmp_path): + """The reserved key inside a table is at the wrong level, not spelt wrong.""" + in_dataset = f'{DEMO}manifest_schema = 1\n' + with pytest.raises(ManifestSchemaError, match='move the line above the first table'): + load_manifest(_write(tmp_path, in_dataset)) + + in_grouping = ( + 'manifest_schema = 1\n[grp]\nmanifest_schema = 1\n[grp.demo]\nzenodo = "10.5281/zenodo.1"\n' + ) + with pytest.raises(ManifestSchemaError, match='move the line above the first table') as grp: + load_manifest(_write(tmp_path, in_grouping)) + assert 'misspelt' not in str(grp.value) + + +def test_a_future_manifest_is_refused_before_this_codes_own_rules(tmp_path): + """A manifest above this schema cannot be judged by this schema's rules.""" + future = f'manifest_schema = 99\nsubdir = "x"\n{DEMO}' + with pytest.raises(ManifestSchemaError, match='upgrade fwl-io') as excinfo: + load_manifest(_write(tmp_path, future)) + # The subdir rule is one of this code's rules, so it must not be the thing + # reported: schema 99 may well have reinstated the field. + assert 'subdir' not in str(excinfo.value) + + +def test_other_root_scalars_stay_a_manifests_own_settings(tmp_path): + """Reserving one root key leaves the root open to a manifest's own settings.""" + setting = f'schema_version = 1\nowner = "proteus"\n{DEMO}' + assert [ds.key for ds in load_manifest(_write(tmp_path, setting))] == ['demo'] + + +def test_older_schema_refusal_names_both_numbers(tmp_path, monkeypatch): + """The refusal names the manifest's schema and the reader's, and where to look.""" + monkeypatch.setattr(manifest, '_MANIFEST_SCHEMA', 3) + with pytest.raises(ManifestSchemaError) as excinfo: + load_manifest(_write(tmp_path, f'manifest_schema = 1\n{DEMO}')) + message = str(excinfo.value) + assert 'manifest_schema 1' in message, 'the refusal must name the schema the manifest declares' + assert 'manifest schema 3' in message, 'the refusal must name the schema the reader implements' + assert 'schema versions table' in message, 'the refusal must say where to look' + # It is not the upgrade case: the reader is newer, not older. + assert 'upgrade fwl-io' not in message + + # Discrimination: at the reader's own schema the same file loads, so the + # refusal is the number's doing. + assert load_manifest(_write(tmp_path, f'manifest_schema = 3\n{DEMO}'))[0].key == 'demo'