diff --git a/mpcontribs-api/src/mpcontribs_api/_openapi.py b/mpcontribs-api/src/mpcontribs_api/_openapi.py index 95b6b3bfa..ef08d46ea 100644 --- a/mpcontribs-api/src/mpcontribs_api/_openapi.py +++ b/mpcontribs-api/src/mpcontribs_api/_openapi.py @@ -7,11 +7,11 @@ Hierarchical, JSON-object data for the contribution. Nesting deeper than 7 levels is rejected. Lists are allowed; any dictionaries inside them have their keys coerced and validated like every other key. -**Key coercion (important):** every dictionary key is coerced to `snake_case` on write. Casing is -lowercased, `camelCase`/`PascalCase` boundaries are split, and any run of spaces/hyphens/punctuation -collapses to a single underscore. So `"bandGap"`, `"Band Gap"`, and `"band-gap"` are all stored as -`"band_gap"`, and the **keys you read back may differ from the keys you submitted**. Keys must be -ASCII and must not reduce to an empty string (e.g. `"***"` is rejected). +**Key coercion (important):** every dictionary key is coerced to `camelCase` on write. Word +boundaries are detected from casing, acronyms, and any run of spaces/hyphens/punctuation, then joined +so the first word is lowercase and each subsequent word is capitalized. So `"band_gap"`, `"Band Gap"`, +and `"band-gap"` are all stored as `"bandGap"`, and the **keys you read back may differ from the keys +you submitted**. Keys must be ASCII and must not reduce to an empty string (e.g. `"***"` is rejected). **Reserved keys:** `si_value`, `si_unit`, `value`, `unit`, `si_error`, `error`, `precision`, and `display` are reserved for the stored value-leaf shape and may **not** be used as @@ -31,7 +31,7 @@ Annotation rules: the single token without an `=` is the **unit** (e.g. `eV`, `S/cm`, `K`), left verbatim so it round-trips through Pint; each `k=v` token is a **condition** (names coerced to -`snake_case`); `name` may be a dotted path (`"transport.conductivity (S/cm)"`) to nest the value; a +`camelCase`); `name` may be a dotted path (`"transport.conductivity (S/cm)"`) to nest the value; a key with no parentheses is a plain key. If a unit is given in **both** the key and the value and they differ, the **key's unit wins** — the value is converted into it (a dimensional mismatch is rejected). @@ -58,7 +58,7 @@ """ CONTRIBUTION_DATA_OUTPUT_DESCRIPTION = """\ -Hierarchical contribution data. Keys are stored in `snake_case` (see the write schema for the +Hierarchical contribution data. Keys are stored in `camelCase` (see the write schema for the coercion rules), so they may differ from the keys originally submitted. Any value that reads as a number is stored as a **quantity leaf** object: @@ -76,7 +76,7 @@ `precision` reproduce the submitted form exactly, so a client can format the value however it likes. Values that are not numeric (words, booleans, lists) keep whatever JSON shape they were submitted -with (after `snake_case` key coercion). +with (after `camelCase` key coercion). """ openapi_tags = [ @@ -96,7 +96,7 @@ "Each contribution uses `mp-id` or composition as identifier to associate its data with the according entries " "on MP. Only admins or users on the project can create, update or delete contributions, and while unpublished, " "retrieve its data or view it on the Portal. Contribution components (tables, structures, and attachments) are " - "deleted along with a contribution. **Note:** `data` keys are coerced to `snake_case` on write and may carry " + "deleted along with a contribution. **Note:** `data` keys are coerced to `camelCase` on write and may carry " "unit/condition annotations of the form `name (unit, cond=value, ...)`; keys with conditions cause the " "submission to pivot into one contribution per condition signature. See the `data` field on the request and " "response schemas for the full grammar and the annotated-value shape.", diff --git a/mpcontribs-api/src/mpcontribs_api/domains/_shared/types.py b/mpcontribs-api/src/mpcontribs_api/domains/_shared/types.py index 6cfd3d4f3..ca5d8febc 100644 --- a/mpcontribs-api/src/mpcontribs_api/domains/_shared/types.py +++ b/mpcontribs-api/src/mpcontribs_api/domains/_shared/types.py @@ -1,8 +1,9 @@ import re import unicodedata from collections.abc import Callable, Mapping +from dataclasses import dataclass from enum import StrEnum -from typing import Annotated, Any, Self +from typing import Annotated, Any, Literal, Self import polars as pl from fastapi import Query @@ -339,9 +340,30 @@ def to_snake_case(name: str) -> str: return s.strip("_").lower() +def to_camel_case(name: str) -> str: + """Coerce a single key token to canonical ``camelCase``. + + Uses ``to_snake_case`` as an easy way to coerce. + Examples: ``"band_gap"``/``"Band Gap"`` -> ``"bandGap"``, ``"pH-Value"`` -> ``"phValue"``, + ``"bandGap"`` -> ``"bandGap"``. + """ + snake = to_snake_case(name) + if not snake: + return "" + # to_snake_case never emits empty or doubled-underscore segments, so every tail word is a + # non-empty, already-lowercase token. + head, *tail = snake.split("_") + return head + "".join(word[:1].upper() + word[1:] for word in tail) + + +CANONICAL_KEY_COERCION: Callable[[str], str] = to_camel_case + # Converts strs to snake case SnakeCaseStr = Annotated[str, BeforeValidator(func=to_snake_case)] +# Converts strs to camel case +CamelCaseStr = Annotated[str, BeforeValidator(func=to_camel_case)] + # Converts strs to searchable form (NFKC compatibility fold + casefold) SearchStr = Annotated[str, BeforeValidator(func=_nfkc_casefold)] @@ -369,8 +391,14 @@ def _validate_slug(v: str) -> str: Slug = Annotated[str, Field(min_length=3, max_length=50), BeforeValidator(_validate_slug)] -def coerce_key(key: Any, *, require_ascii: bool = False, reserved: frozenset[str] | None = None) -> str: - """Coerce one dict key to canonical ``snake_case``, enforcing the shared write-path key guards. +def coerce_key( + key: Any, + *, + require_ascii: bool = True, + reserved: frozenset[str] | None = None, + coercion_method: Callable[[str], str] = CANONICAL_KEY_COERCION, +) -> str: + """Coerce one dict key to canonical case using ``coercion_method``, enforcing the shared write-path key guards. Always rejects a key that reduces to an empty string after coercion. The extra guards are opt-in per call site, since not every caller wants them (e.g. the post-validation write path skips the @@ -378,16 +406,19 @@ def coerce_key(key: Any, *, require_ascii: bool = False, reserved: frozenset[str - ``require_ascii``: reject a non-``str`` or non-ASCII key before coercion. - ``reserved``: reject a coerced key that lands in the reserved-leaf-key set. + - ``coercion_method``: the method to use to coerce a key Raises: DataKeyError: on a non-ASCII (when required), empty-after-coercion, or reserved key. It is a :class:`ValidationError` subclass, so callers catching either still see it. """ + if not key: + raise DataKeyError(message="Key must be truthy", key=key) if require_ascii and (not isinstance(key, str) or not key.isascii()): - raise DataKeyError("Non-ASCII key found in Contribution.data. All dict keys must be only ASCII") - coerced = to_snake_case(key) + raise DataKeyError("Non-ASCII key found. All dict keys must be only ASCII", key=key) + coerced = coercion_method(key) if not coerced: - raise DataKeyError(f"data key '{key}' reduces to an empty string after snake_case coercion") + raise DataKeyError(f"data key '{key}' reduces to an empty string after key coercion", key=key) if reserved is not None and coerced in reserved: raise DataKeyError( f"data key '{key}' is reserved for annotated-value leaves and may not be used", @@ -397,6 +428,47 @@ def coerce_key(key: Any, *, require_ascii: bool = False, reserved: frozenset[str return coerced +@dataclass(frozen=True, slots=True) +class KeyOffense: + """One ``data`` key that is not already in the expected canonical form. + + ``suggestion`` is the canonical spelling the caller should use, or ``None`` when there is no + clean suggestion (a non-ASCII key, a key that reduces to an empty string, or a reserved leaf + name that is already canonical). ``reason`` is a stable, machine-readable tag. + """ + + key: Any + suggestion: str | None + reason: Literal["not_camel_case", "non_ascii", "empty_after_coercion", "reserved"] + + @classmethod + def from_key( + cls, + key: Any, + *, + reserved: frozenset[str] | None = None, + coercion_method: Callable[[str], str] = CANONICAL_KEY_COERCION, + ) -> KeyOffense | None: + """Return a :class:`KeyOffense` when ``key`` is not already an acceptable canonical data key, else ``None``. + + A key is acceptable iff it is a non-empty ASCII string that equals its own canonical form + (``coercion_method(key) == key``) and is not a reserved leaf name. + """ + if not isinstance(key, str) or not key.isascii(): + return cls(key=key, suggestion=None, reason="non_ascii") + canonical = coercion_method(key) + if not canonical: + return cls(key=key, suggestion=None, reason="empty_after_coercion") + # Check reserved against the canonical form (not the raw key), so a non-canonical key whose + # canonical spelling is reserved (e.g. "Value" -> "value") is reported as reserved rather than + # suggesting a reserved name the caller could never use. + if reserved is not None and canonical in reserved: + return cls(key=key, suggestion=None, reason="reserved") + if canonical != key: + return cls(key=key, suggestion=canonical, reason="not_camel_case") + return None + + def map_keys(value: Any, *, coerce: Callable[[Any], str], on_scalar: Callable[[Any], Any] = lambda x: x) -> Any: """Recursively rebuild ``value`` with every dict key coerced via ``coerce``. diff --git a/mpcontribs-api/src/mpcontribs_api/domains/contributions/data.py b/mpcontribs-api/src/mpcontribs_api/domains/contributions/data.py index 659601662..b2b008238 100644 --- a/mpcontribs-api/src/mpcontribs_api/domains/contributions/data.py +++ b/mpcontribs-api/src/mpcontribs_api/domains/contributions/data.py @@ -5,7 +5,7 @@ from pydantic import BeforeValidator from mpcontribs_api.config import get_settings -from mpcontribs_api.domains._shared.types import coerce_key +from mpcontribs_api.domains._shared.types import KeyOffense from mpcontribs_api.domains._shared.units import QuantityLeaf from mpcontribs_api.exceptions import DataKeyError, ValidationError @@ -99,67 +99,58 @@ def _validate_data_depth(data: dict[str, Any] | None) -> dict[str, Any] | None: return data -def _validate_plain_key(key: Any) -> None: - """Validate a single plain key token (a path segment or a condition name). - - Punctuation, spaces, and casing are no longer rejected: keys are coerced to ``snake_case`` on the - write path (see :func:`to_snake_case`), which folds ``*``/``/``/``|`` and any other non-alphanumeric - run to ``_``. This only rejects keys that cannot be coerced into a usable token: non-ASCII, empty, - or ones that reduce to an empty string after coercion (e.g. ``"***"``). - """ - if key == "": - raise ValidationError("Empty key found in Contribution.data. Keys must be non-empty.") - coerce_key(key, require_ascii=True, reserved=QuantityLeaf.reserved_keys()) +def _check_key(raw_key: Any, offenses: list[KeyOffense]) -> None: + """Append a :class:`KeyOffense` for ``raw_key`` unless it is already an acceptable canonical key.""" + offense = KeyOffense.from_key(raw_key, reserved=QuantityLeaf.reserved_keys()) + if offense is not None: + offenses.append(offense) -def _validate_nested_keys(value: Any, *, allow_leaf_fragments: bool = False) -> None: +def _collect_nested_offenses(value: Any, offenses: list[KeyOffense], *, allow_leaf_fragments: bool) -> None: if isinstance(value, dict): - _validate_keys(value, allow_leaf_fragments=allow_leaf_fragments) + _collect_plain_offenses(value, offenses, allow_leaf_fragments=allow_leaf_fragments) elif isinstance(value, list): for item in value: - _validate_nested_keys(item, allow_leaf_fragments=allow_leaf_fragments) + _collect_nested_offenses(item, offenses, allow_leaf_fragments=allow_leaf_fragments) -def _validate_keys(data: dict[str, Any] | None, *, allow_leaf_fragments: bool = False) -> dict[str, Any] | None: - """Strict plain-key validation for a single dict level (used for nested levels). +def _collect_plain_offenses( + data: dict[str, Any] | None, offenses: list[KeyOffense], *, allow_leaf_fragments: bool +) -> None: + """Collect non-canonical keys for a single dict level (strict plain keys, used for nested levels). With ``allow_leaf_fragments`` (the patch/merge path), a dict whose keys are all reserved leaf keys is accepted as a terminal fragment addressing fields *inside* a stored quantity leaf (e.g. ``{'unit': 'kg'}``); the strict insert path leaves this off and rejects reserved keys as plain keys. """ - if data is None: - return None - # A server-built quantity leaf legitimately uses the reserved key names; do not descend into it - # (re-validation after normalization/expansion would otherwise reject its own keys). - if QuantityLeaf.is_leaf(data): - return data - if allow_leaf_fragments and QuantityLeaf.is_fragment(data): - return data + if data is None or QuantityLeaf.is_leaf(data) or allow_leaf_fragments and QuantityLeaf.is_fragment(data): + return for key in data: - _validate_plain_key(key) + _check_key(key, offenses) # Recurse into nested dicts, including dicts nested inside lists. for v in data.values(): - _validate_nested_keys(v, allow_leaf_fragments=allow_leaf_fragments) - return data + _collect_nested_offenses(v, offenses, allow_leaf_fragments=allow_leaf_fragments) -def _validate_data_keys(data: dict[str, Any] | None, *, allow_leaf_fragments: bool = False) -> dict[str, Any] | None: - """Top-level ``data`` key validation, allowing the annotated pattern. +def _collect_data_offenses( + data: dict[str, Any] | None, offenses: list[KeyOffense], *, allow_leaf_fragments: bool +) -> None: + """Collect non-canonical keys for the top ``data`` level, allowing the annotated pattern. - Each top-level key may be either a plain key or the annotated form - ``name (unit, cond1=..., cond2=...)``. The name's dotted segments and every condition name are - held to the same plain-key rules (units are unconstrained); nested levels stay strictly plain. - Expansion (see :mod:`mpcontribs_api.domains.contributions.pivot`) later rewrites annotated keys - into plain ones, so stored keys always satisfy :func:`_validate_keys`. + Each top-level key may be either a plain key or the annotated form ``name (unit, cond1=..., cond2=...)``. + The name's dotted segments and every condition name are held to the same canonical-key rules (units + are unconstrained); nested levels stay strictly plain. A malformed annotation is a syntax error and is + raised eagerly (it is not a format-mismatch we can suggest a spelling for). ``allow_leaf_fragments`` is threaded to nested levels only (the patch/merge path); top-level keys stay strict, since a reserved key at the root addresses no leaf. """ if data is None: - return None + return for raw_key in data: if not isinstance(raw_key, str): - raise ValidationError("Non-ASCII key found in Contribution.data. All dict keys must be only ASCII") + _check_key(raw_key, offenses) + continue try: parsed = parse_annotated_key(raw_key) except ValidationError as err: @@ -167,15 +158,22 @@ def _validate_data_keys(data: dict[str, Any] | None, *, allow_leaf_fragments: bo if not parsed.is_annotated: # A plain key keeps the original strict rule (no '.' nesting); only annotated keys may # use dotted paths, whose segments are validated individually below. - _validate_plain_key(raw_key) + _check_key(raw_key, offenses) continue for segment in parsed.segments: - _validate_plain_key(segment) + _check_key(segment, offenses) for condition_name in parsed.conditions: - _validate_plain_key(condition_name) + _check_key(condition_name, offenses) for v in data.values(): - _validate_nested_keys(v, allow_leaf_fragments=allow_leaf_fragments) - return data + _collect_nested_offenses(v, offenses, allow_leaf_fragments=allow_leaf_fragments) + + +def _raise_on_offenses(offenses: list[KeyOffense]) -> None: + if offenses: + raise DataKeyError( + "Contribution.data contains keys not in the expected canonical (camelCase) format", + offending_keys=[{"key": o.key, "suggestion": o.suggestion, "reason": o.reason} for o in offenses], + ) def validate_contribution_data( @@ -183,13 +181,19 @@ def validate_contribution_data( ) -> dict[str, Any] | None: """Run the write-path ``data`` validation (depth + annotated/plain keys). + Keys are validated but never rewritten: a key not already in the expected canonical form is rejected, and + every offending key is collected so the error lists them all at once (with a suggested spelling) rather than + failing on the first. + ``allow_leaf_fragments`` (the merge-patch path) additionally accepts a nested dict of only reserved - leaf keys as a terminal fragment addressing a field inside a stored quantity leaf (e.g. ``{'bandgap': + leaf keys as a terminal fragment addressing a field inside a stored quantity leaf (e.g. ``{'bandGap': {'unit': 'kg'}}``). The strict insert path rejects reserved keys as plain keys; a whole-dict overwrite (``replace_data=True``) re-runs this strictly, since the payload becomes a full document. """ _validate_data_depth(data) - _validate_data_keys(data, allow_leaf_fragments=allow_leaf_fragments) + offenses: list[KeyOffense] = [] + _collect_data_offenses(data, offenses, allow_leaf_fragments=allow_leaf_fragments) + _raise_on_offenses(offenses) return data @@ -198,20 +202,22 @@ def validate_stored_contribution_data(data: dict[str, Any] | None) -> dict[str, This is the stored-document counterpart to :func:`validate_contribution_data`. By the time a :class:`~mpcontribs_api.domains.contributions.models.Contribution` is built, pivot/expansion has - already coerced every key to canonical snake_case, so the stored payload must satisfy the plain-key - rules at *every* level — the annotated-key grammar (``name (unit, cond=...)``) is no longer allowed - even at the top level. The depth bound is the same settings-driven limit as the input path, so the - two cannot drift. + unwrapped any annotated keys, so the stored payload must satisfy the canonical plain-key rules at + *every* level — the annotated-key grammar (``name (unit, cond=...)``) is no longer allowed even at + the top level. The depth bound is the same settings-driven limit as the input path, so the two + cannot drift. """ _validate_data_depth(data) - _validate_keys(data) + offenses: list[KeyOffense] = [] + _collect_plain_offenses(data, offenses, allow_leaf_fragments=False) + _raise_on_offenses(offenses) return data # Three field types over one shared validation core, so the input, patch, and stored paths cannot # drift on depth or key rules: inserts/whole-document writes are strict; a merge patch additionally -# permits leaf fragments (see ``allow_leaf_fragments`` above); the stored document requires fully -# coerced plain keys (no annotated-key grammar). +# permits leaf fragments (see ``allow_leaf_fragments`` above); the stored document requires canonical +# plain keys at every level (no annotated-key grammar). ContributionData = Annotated[dict[str, Any] | None, BeforeValidator(validate_contribution_data)] ContributionPatchData = Annotated[ dict[str, Any] | None, diff --git a/mpcontribs-api/src/mpcontribs_api/domains/contributions/pivot.py b/mpcontribs-api/src/mpcontribs_api/domains/contributions/pivot.py index 929d7345e..2cc651f09 100644 --- a/mpcontribs-api/src/mpcontribs_api/domains/contributions/pivot.py +++ b/mpcontribs-api/src/mpcontribs_api/domains/contributions/pivot.py @@ -59,11 +59,11 @@ def _try_coerce_leaf(value: Any, key_unit: str | None = None) -> Any: def normalize_node(value: Any, key_unit: str | None = None) -> Any: - """Recursively coerce dict keys to ``snake_case`` and turn scalar measurements into quantity leaves. + """Recursively coerce dict keys and turn scalar measurements into quantity leaves. This is the write-path core of "every numeric becomes a leaf": - - **dict**: each key is coerced to ``snake_case`` (sibling collisions rejected); values recurse + - **dict**: each key is coerced (sibling collisions rejected); values recurse with no inherited unit — only a top-level annotated key carries a unit (``key_unit``). - **list**: elements recurse, but scalar elements are left verbatim (a list is array data, not a column of measurements — mirrors :func:`mpcontribs_api.domains.contributions.stats.iter_leaves`, @@ -95,7 +95,7 @@ def expand_data(data: dict[str, Any]) -> list[ExpandedData]: Returns: - a single element with ``condition_key == ""`` when nothing pivots (no annotations at all, or annotations but no conditions). When no annotation is present the returned ``data`` is - the same object as the input if snake_case coercion is a no-op, else the coerced copy. + the same object as the input if coercion is a no-op, else the coerced copy. - one element per distinct condition signature otherwise. Raises: @@ -117,13 +117,13 @@ def expand_data(data: dict[str, Any]) -> list[ExpandedData]: if not pk.conditions: broadcast.append((raw_key, pk)) continue - # Condition names become data columns after pivoting, so they are coerced to snake_case like + # Condition names become data columns after pivoting, so they are coerced # any other key (values and the unit are left verbatim). parsed_conditions: dict[str, Any] = {} for name, val in pk.conditions.items(): cname = coerce_key(name) if cname in parsed_conditions: - raise ValidationError(f"condition names collide after snake_case coercion: '{cname}'") + raise ValidationError(f"condition names collide after coercion: '{cname}'") parsed_conditions[cname] = parse_condition_value(val) ckey = QuantityLeaf.condition_key(parsed_conditions) groups.setdefault(ckey, []).append((raw_key, pk)) diff --git a/mpcontribs-api/src/mpcontribs_api/domains/projects/models.py b/mpcontribs-api/src/mpcontribs_api/domains/projects/models.py index 99a9e63f2..ce8b22f5f 100644 --- a/mpcontribs-api/src/mpcontribs_api/domains/projects/models.py +++ b/mpcontribs-api/src/mpcontribs_api/domains/projects/models.py @@ -6,8 +6,8 @@ from mpcontribs_api import pagination from mpcontribs_api.domains._shared.filters import BaseFilter -from mpcontribs_api.domains._shared.models import BaseDocumentWithInput, DocumentOut -from mpcontribs_api.domains._shared.types import Identity, PrefixedEmail, SearchStr, ShortStr +from mpcontribs_api.domains._shared.models import BaseDocumentWithInput, DocumentOut, Identity +from mpcontribs_api.domains._shared.types import CANONICAL_KEY_COERCION, PrefixedEmail, SearchStr, ShortStr from mpcontribs_api.domains.initiatives.models import Initiative from mpcontribs_api.exceptions import ValidationError @@ -24,12 +24,22 @@ class ProjectIdentity(Identity): def _validate_unique_column(value: str | None) -> str | None: - """Shape-only check for ``unique_column``: a non-empty, non-blank dotted-path string or None.""" + """Validate and canonicalize ``unique_column``: a non-empty, non-blank dotted path. + + ``unique_column`` is a path *into* ``Contribution.data``, so each segment is coerced through the + same :data:`CANONICAL_KEY_COERCION` symbol as data keys. + """ if value is None: return None - if not value.strip() or any(not segment for segment in value.split(".")): + segments = value.split(".") + if not value.strip() or any(not segment for segment in segments): raise ValidationError("unique_column must be a non-empty dotted path (no blank segments).", value=value) - return value + coerced = [CANONICAL_KEY_COERCION(segment) for segment in segments] + if any(not segment for segment in coerced): + raise ValidationError( + "unique_column segments must not reduce to an empty string after canonical key coercion.", value=value + ) + return ".".join(coerced) def validate_column_limit(columns: Any, max_columns: int) -> None: diff --git a/mpcontribs-api/tests/integration/db/test_stats_recompute.py b/mpcontribs-api/tests/integration/db/test_stats_recompute.py index 2b620523c..236f2c3f5 100644 --- a/mpcontribs-api/tests/integration/db/test_stats_recompute.py +++ b/mpcontribs-api/tests/integration/db/test_stats_recompute.py @@ -171,9 +171,10 @@ async def test_full_lifecycle(self, db, mongo_client): assert project.stats.attachments == 0 assert project.stats.size > 0 cols = _columns_by_path(project) - assert set(cols) == {"band_gap", "energy"} + # Data keys are coerced to camelCase on write, so derived columns use the camelCase form. + assert set(cols) == {"bandGap", "energy"} assert project.stats.columns == 2 - assert (cols["band_gap"].min, cols["band_gap"].max) == (2.1, 2.1) + assert (cols["bandGap"].min, cols["bandGap"].max) == (2.1, 2.1) assert (cols["energy"].min, cols["energy"].max) == (-5.0, -5.0) # --- remove it -> back to empty --- @@ -200,8 +201,8 @@ async def test_full_lifecycle(self, db, mongo_client): assert project.stats.tables == 0 assert project.stats.size > 0 cols = _columns_by_path(project) - assert set(cols) == {"band_gap"} - assert (cols["band_gap"].min, cols["band_gap"].max) == (3.0, 3.0) + assert set(cols) == {"bandGap"} + assert (cols["bandGap"].min, cols["bandGap"].max) == (3.0, 3.0) # --- remove it -> empty again --- deleted = await svc.delete_many(ContributionFilter(id=summary.succeeded[0].id)) diff --git a/mpcontribs-api/tests/integration/test_bulk_limits.py b/mpcontribs-api/tests/integration/test_bulk_limits.py index 4cb877734..3cef3a73c 100644 --- a/mpcontribs-api/tests/integration/test_bulk_limits.py +++ b/mpcontribs-api/tests/integration/test_bulk_limits.py @@ -16,7 +16,7 @@ def _valid_contribution_body(**overrides) -> dict: "material_id": "mp-1234", "chemical_system_id": "Fe-O", "formula": "Fe2O3", - "data": {"band_gap": 2.1}, + "data": {"bandGap": 2.1}, } body.update(overrides) return body diff --git a/mpcontribs-api/tests/integration/test_contributions_routes.py b/mpcontribs-api/tests/integration/test_contributions_routes.py index 6f9ee71e2..dc7c8b41c 100644 --- a/mpcontribs-api/tests/integration/test_contributions_routes.py +++ b/mpcontribs-api/tests/integration/test_contributions_routes.py @@ -40,7 +40,7 @@ def _valid_contribution_body(**overrides) -> dict: "material_id": "mp-1234", "chemical_system_id": "Fe-O", "formula": "Fe2O3", - "data": {"band_gap": 2.1}, + "data": {"bandGap": 2.1}, } body.update(overrides) return body diff --git a/mpcontribs-api/tests/unit/domains/test_contribution_service.py b/mpcontribs-api/tests/unit/domains/test_contribution_service.py index 9137e20b8..e34175a1f 100644 --- a/mpcontribs-api/tests/unit/domains/test_contribution_service.py +++ b/mpcontribs-api/tests/unit/domains/test_contribution_service.py @@ -785,7 +785,7 @@ async def _insert(doc, session=None): # One submission that pivots into two rows (T=300K / T=400K) and carries a structure. contrib = _contrib_in( identifier="mp-1", - data={"x (eV, T=300K)": 1, "x (eV, T=400K)": 2}, + data={"x (eV, t=300K)": 1, "x (eV, t=400K)": 2}, structures=[_structure_in()], ) summary = await svc.insert_many([contrib]) @@ -884,20 +884,20 @@ async def test_insert_no_existing_identity_succeeds(self): assert docs[0].unique_value is None async def test_insert_unique_column_promotes_value_to_unique_value(self): - svc, contrib_repo, *_ = _make_service(unique_column="sample_id") + svc, contrib_repo, *_ = _make_service(unique_column="sampleId") contrib_repo.insert_many.return_value = None - summary = await svc.insert_many([_contrib_in(data={"sample_id": "A"})]) + summary = await svc.insert_many([_contrib_in(data={"sampleId": "A"})]) assert len(summary.succeeded) == 1 docs = contrib_repo.insert_many.call_args[0][0] assert docs[0].unique_value == "A" async def test_insert_same_triple_distinct_unique_value_both_succeed(self): - svc, contrib_repo, *_ = _make_service(unique_column="sample_id") + svc, contrib_repo, *_ = _make_service(unique_column="sampleId") contrib_repo.insert_many.return_value = None - contribs = [_contrib_in(data={"sample_id": "A"}), _contrib_in(data={"sample_id": "B"})] + contribs = [_contrib_in(data={"sampleId": "A"}), _contrib_in(data={"sampleId": "B"})] summary = await svc.insert_many(contribs) assert len(summary.succeeded) == 2 @@ -905,7 +905,7 @@ async def test_insert_same_triple_distinct_unique_value_both_succeed(self): assert sorted(d.unique_value for d in docs) == ["A", "B"] async def test_insert_missing_unique_column_value_is_validation_failure(self): - svc, contrib_repo, *_ = _make_service(unique_column="sample_id") + svc, contrib_repo, *_ = _make_service(unique_column="sampleId") contrib_repo.insert_many.return_value = None summary = await svc.insert_many([_contrib_in(data={"other": 1})]) @@ -915,10 +915,10 @@ async def test_insert_missing_unique_column_value_is_validation_failure(self): contrib_repo.insert_many.assert_not_called() async def test_insert_non_scalar_unique_column_value_is_validation_failure(self): - svc, contrib_repo, *_ = _make_service(unique_column="sample_id") + svc, contrib_repo, *_ = _make_service(unique_column="sampleId") contrib_repo.insert_many.return_value = None - summary = await svc.insert_many([_contrib_in(data={"sample_id": {"nested": 1}})]) + summary = await svc.insert_many([_contrib_in(data={"sampleId": {"nested": 1}})]) assert summary.succeeded == [] assert [f.error_code for f in summary.failed] == ["validation_error"] @@ -955,17 +955,17 @@ async def test_upsert_does_not_conflict_on_existing_identity(self): contrib_repo.upsert_one.assert_called_once() async def test_upsert_passes_resolved_unique_value_in_identifiers(self): - svc, contrib_repo, *_ = _make_service(unique_column="sample_id") + svc, contrib_repo, *_ = _make_service(unique_column="sampleId") contrib_repo.upsert_one.return_value = MagicMock(spec=Contribution, project="proj") - await svc.upsert_many([_contrib_in(data={"sample_id": "A"})]) + await svc.upsert_many([_contrib_in(data={"sampleId": "A"})]) # The batch path builds the document and stamps the server-resolved unique_value onto it. doc = contrib_repo.upsert_one.call_args.args[0] assert doc.unique_value == "A" async def test_upsert_missing_unique_column_value_is_validation_failure(self): - svc, contrib_repo, *_ = _make_service(unique_column="sample_id") + svc, contrib_repo, *_ = _make_service(unique_column="sampleId") summary = await svc.upsert_many([_contrib_in(data={"other": 1})]) @@ -1582,23 +1582,23 @@ async def test_replace_data_flag_forwarded_to_repo(self): async def test_merge_resolves_unique_value_from_merged_state(self): # The unique_column value lives in the stored data and is NOT in the patch. A merge preserves # it, so unique_value must resolve against the merged view rather than raising "missing". - svc, contrib_repo, *_ = _make_service(unique_column="sample_id") + svc, contrib_repo, *_ = _make_service(unique_column="sampleId") existing = _existing_doc(material_id=None, chemical_system_id="Fe-O", formula="Fe2O3") - existing.data = {"sample_id": 42, "x": 1.0} + existing.data = {"sampleId": 42, "x": 1.0} contrib_repo.read_one.return_value = existing contrib_repo.update_one.return_value = MagicMock(spec=Contribution) await svc.update_one({"id": str(existing.id)}, ContributionPatch(data={"y": 9.0})) - # Resolved from {sample_id:42, x:1, y:9}, so the untouched unique_value survives the merge. + # Resolved from {sampleId:42, x:1, y:9}, so the untouched unique_value survives the merge. assert contrib_repo.update_one.call_args.kwargs["unique_value"] == 42 async def test_replace_resolves_unique_value_from_patch_data_only(self): # On replace the stored data is discarded, so a unique_column absent from the patch is a # genuine validation failure (the resulting document would lack it). - svc, contrib_repo, *_ = _make_service(unique_column="sample_id") + svc, contrib_repo, *_ = _make_service(unique_column="sampleId") existing = _existing_doc(material_id=None, chemical_system_id="Fe-O", formula="Fe2O3") - existing.data = {"sample_id": 42} + existing.data = {"sampleId": 42} contrib_repo.read_one.return_value = existing with pytest.raises(ValidationError, match="unique_column"): diff --git a/mpcontribs-api/tests/unit/domains/test_contribution_stats.py b/mpcontribs-api/tests/unit/domains/test_contribution_stats.py index a277d429c..7ea9c6b68 100644 --- a/mpcontribs-api/tests/unit/domains/test_contribution_stats.py +++ b/mpcontribs-api/tests/unit/domains/test_contribution_stats.py @@ -41,6 +41,14 @@ def test_nested_plain_object_is_recursed_with_dotted_path(self): def test_bare_number_is_numeric_without_unit(self): assert list(iter_leaves({"n": 5})) == [("n", 5.0, None)] + def test_paths_are_verbatim_and_never_re_coerced(self): + # Invariant: column paths come straight from the stored keys; this walk applies NO coercion of + # its own. A non-canonical key (snake_case, when the canonical data-key form is camelCase) must + # pass through verbatim rather than being re-coerced to "bandGap" — that verbatim coupling is + # what keeps Project.columns aligned with the data-key coercion instead of drifting from it. + paths = [p for p, _, _ in iter_leaves({"band_gap": _annotated(1.0), "nested": {"sub_key": _annotated(2.0)}})] + assert paths == ["band_gap", "nested.sub_key"] + def test_string_and_bool_are_non_numeric(self): leaves = dict((p, (v, u)) for p, v, u in iter_leaves({"s": "cubic", "flag": True})) assert leaves["s"] == (None, NON_NUMERIC_UNIT) diff --git a/mpcontribs-api/tests/unit/domains/test_contributions_models.py b/mpcontribs-api/tests/unit/domains/test_contributions_models.py index 028635bea..cb66c58b8 100644 --- a/mpcontribs-api/tests/unit/domains/test_contributions_models.py +++ b/mpcontribs-api/tests/unit/domains/test_contributions_models.py @@ -18,7 +18,13 @@ ContributionPatch, extract_unique_value, ) -from mpcontribs_api.exceptions import ValidationError +from mpcontribs_api.domains.contributions.pivot import expand_contribution +from mpcontribs_api.exceptions import DataKeyError, ValidationError + + +def _offenders(exc: DataKeyError) -> dict[str, str | None]: + """Map each offending key from a DataKeyError to its suggested canonical spelling (or None).""" + return {entry["key"]: entry["suggestion"] for entry in exc.context["offending_keys"]} # The identity/index column order, declared once here so the tests fail loudly if the field order # in ContributionIdentity ever drifts (which silently forces a Mongo index migration). @@ -44,7 +50,7 @@ def _make_contribution_in(**overrides) -> ContributionIn: "material_id": "mp-1234", "chemical_system_id": "Fe-O", "formula": "Fe2O3", - "data": {"band_gap": QuantityLeaf.from_submission(2.1, "eV").as_dict()}, + "data": {"bandGap": QuantityLeaf.from_submission(2.1, "eV").as_dict()}, } defaults.update(overrides) return ContributionIn(**defaults) @@ -117,49 +123,76 @@ def test_data_can_be_empty_dict(self): assert contrib.data == {} def test_data_accepts_nested_structure(self): - nested = {"band_gap": QuantityLeaf.from_submission(1.5, "eV").as_dict(), "volume": 42.3} + nested = {"bandGap": QuantityLeaf.from_submission(1.5, "eV").as_dict(), "volume": 42.3} contrib = _make_contribution_in(data=nested) - assert contrib.data["band_gap"]["value"] == 1.5 + assert contrib.data["bandGap"]["value"] == 1.5 def test_data_depth_validation(self): - max_nesting = {"lvl_1": {"lvl_2": {"lvl_3": {"lvl_4": {"lvl_5": {"lvl_6": {"lvl_7": "pass"}}}}}}} - invalid_nesting = {"lvl_1": {"lvl_2": {"lvl_3": {"lvl_4": {"lvl_5": {"lvl_6": {"lvl_7": {"lvl_8": "fail"}}}}}}}} + max_nesting = {"lvl1": {"lvl2": {"lvl3": {"lvl4": {"lvl5": {"lvl6": {"lvl7": "pass"}}}}}}} + invalid_nesting = {"lvl1": {"lvl2": {"lvl3": {"lvl4": {"lvl5": {"lvl6": {"lvl7": {"lvl8": "fail"}}}}}}}} _make_contribution_in(data=max_nesting) assert True with pytest.raises(ValidationError, match="Depth of Contribution.data"): _make_contribution_in(data=invalid_nesting) def test_data_key_validation(self): - # Input keys are coerced to snake_case on write, so punctuation is folded (not rejected) and - # the annotation grammar is allowed. Only keys that can't be coerced are rejected on input. - _make_contribution_in(data={"test*/|": "pass"}) # folds to "test", accepted - _make_contribution_in(data={"a.b (eV, T=300K)": 1}) # annotated key + dotted path, accepted - with pytest.raises(ValidationError, match="Non-ASCII key found in Contribution.data"): + # Keys are validated, never rewritten: a key not already in canonical camelCase is rejected and + # every offender is reported at once with its suggested spelling. An already-canonical key and + # the annotation grammar (with canonical name/condition parts) are accepted. + _make_contribution_in(data={"bandGap": "pass"}) # already canonical, accepted + _make_contribution_in(data={"a.b (eV, temperature=300K)": 1}) # canonical segments + condition + # A non-canonical plain key is rejected with a camelCase suggestion; the offending raw key is + # never silently changed. + with pytest.raises(DataKeyError) as exc: + _make_contribution_in(data={"Band Gap": 1, "band_gap": 2}) + assert _offenders(exc.value) == {"Band Gap": "bandGap", "band_gap": "bandGap"} + # Non-ASCII and empty-after-coercion keys have no clean suggestion. + with pytest.raises(DataKeyError) as exc: _make_contribution_in(data={"ΔE": "fail"}) - with pytest.raises(ValidationError, match="reduces to an empty string"): + assert _offenders(exc.value) == {"ΔE": None} + with pytest.raises(DataKeyError) as exc: _make_contribution_in(data={"***": "fail"}) - - # The stored document shares the input path's key rules (ContributionStoredData draws on the - # same data.py core), so a coercible-punctuation key is accepted here just as on input, while - # reserved and non-ASCII keys are still rejected. The BeforeValidator runs before Beanie's - # collection lookup, so any ValidationError surfaces first. - Contribution(_id=PydanticObjectId(), project="p", chemical_system_id="Fe-O", data={"bad.key": 1}) - with pytest.raises(ValidationError, match="reserved"): + assert _offenders(exc.value) == {"***": None} + + # The stored document shares the input path's canonical-key rules (ContributionStoredData draws + # on the same data.py core), so an already-canonical key is accepted while non-canonical, + # reserved, and non-ASCII keys are rejected. The BeforeValidator runs before Beanie's collection + # lookup, so the DataKeyError surfaces first. + Contribution(_id=PydanticObjectId(), project="p", chemical_system_id="Fe-O", data={"goodKey": 1}) + with pytest.raises(DataKeyError) as exc: + Contribution(_id=PydanticObjectId(), project="p", chemical_system_id="Fe-O", data={"bad.key": 1}) + assert _offenders(exc.value) == {"bad.key": "badKey"} + with pytest.raises(DataKeyError) as exc: Contribution(_id=PydanticObjectId(), project="p", chemical_system_id="Fe-O", data={"group": {"unit": "x"}}) - with pytest.raises(ValidationError, match="Non-ASCII key found in Contribution.data"): - Contribution(_id=PydanticObjectId(), project="p", chemical_system_id="Fe-O", data={"ΔE": 1}) + assert exc.value.context["offending_keys"] == [{"key": "unit", "suggestion": None, "reason": "reserved"}] def test_reserved_leaf_keys_rejected(self): - # A data key that coerces to a reserved value-leaf name is rejected on write. - for bad in ("value", "unit", "error", "precision", "si_unit", "display"): - with pytest.raises(ValidationError, match="reserved"): + # A data key that IS a reserved value-leaf name is rejected on write (reason "reserved", no + # suggestion — it is already canonical, just disallowed). Only the single-word leaf keys are + # reserved; the SI spellings (siValue, siUnit, siError) are ordinary columns — see + # ``test_si_column_keys_are_ordinary``. + for bad in ("value", "unit", "error", "precision", "display"): + with pytest.raises(DataKeyError) as exc: _make_contribution_in(data={bad: 1}) + assert exc.value.context["offending_keys"] == [{"key": bad, "suggestion": None, "reason": "reserved"}] # rejected when nested, too - with pytest.raises(ValidationError, match="reserved"): + with pytest.raises(DataKeyError) as exc: _make_contribution_in(data={"group": {"unit": "x"}}) + assert _offenders(exc.value) == {"unit": None} # and when used as a condition name in an annotated key - with pytest.raises(ValidationError, match="reserved"): + with pytest.raises(DataKeyError) as exc: _make_contribution_in(data={"x (eV, value=3)": 1}) + assert _offenders(exc.value) == {"value": None} + + def test_si_column_keys_are_ordinary(self): + # The SI leaf field names hold underscores, so their camelCase spellings (siValue, siUnit, + # siError) are ordinary column names, distinct from the reserved single-word leaf keys and + # accepted as-is. The snake_case spellings are rejected with the camelCase suggestion. + contrib = _make_contribution_in(data={"siValue": 1, "siUnit": "x", "siError": 2}) + assert set(contrib.data) == {"siValue", "siUnit", "siError"} + with pytest.raises(DataKeyError) as exc: + _make_contribution_in(data={"si_value": 1}) + assert _offenders(exc.value) == {"si_value": "siValue"} # There isn't currently value validation. This is to check that that is true def test_data_value_validation(self): @@ -428,24 +461,33 @@ def test_is_public_settable(self): assert ContributionPatch(is_public=False).is_public is False def test_data_can_be_set(self): - patch = ContributionPatch(data={"new_key": 42}) - assert patch.data == {"new_key": 42} + patch = ContributionPatch(data={"newKey": 42}) + assert patch.data == {"newKey": 42} def test_data_keys_validated_not_coerced(self): - # ContributionPatch only *validates* keys; it no longer rewrites them. snake_case coercion - # and unit annotation happen in the service (expand_data), not at the model layer. - patch = ContributionPatch(data={"Band Gap": 1, "nested": {"pH-Value": 7}}) - assert patch.data == {"Band Gap": 1, "nested": {"pH-Value": 7}} + # ContributionPatch validates keys and never rewrites them: a non-canonical key is rejected + # (at every level) rather than silently folded to camelCase. All offenders are reported at once. + with pytest.raises(DataKeyError) as exc: + ContributionPatch(data={"Band Gap": 1, "nested": {"pH-Value": 7}}) + assert _offenders(exc.value) == {"Band Gap": "bandGap", "pH-Value": "phValue"} def test_data_annotated_key_accepted(self): - # Annotated keys (unit + conditions) pass model validation unchanged; the service expands them. - patch = ContributionPatch(data={"conductivity (S/cm, T=300K)": 1.2}) - assert patch.data == {"conductivity (S/cm, T=300K)": 1.2} + # Annotated keys (unit + conditions) pass model validation unchanged when the name and every + # condition name are already canonical; the unit is exempt. The service expands them later. + patch = ContributionPatch(data={"conductivity (S/cm, temperature=300K)": 1.2}) + assert patch.data == {"conductivity (S/cm, temperature=300K)": 1.2} + + def test_data_annotated_key_non_canonical_parts_rejected(self): + # A non-canonical condition name (or path segment) is rejected even though the unit is exempt. + with pytest.raises(DataKeyError) as exc: + ContributionPatch(data={"conductivity (S/cm, T=300K)": 1.2}) + assert _offenders(exc.value) == {"T": "t"} def test_data_uncoercible_key_rejected(self): - # A key that reduces to an empty string after coercion is still rejected at validation time. - with pytest.raises(ValidationError, match="empty string after snake_case coercion"): + # A key that reduces to an empty string after coercion is rejected with no suggestion. + with pytest.raises(DataKeyError) as exc: ContributionPatch(data={"***": 1}) + assert _offenders(exc.value) == {"***": None} # --------------------------------------------------------------------------- diff --git a/mpcontribs-api/tests/unit/domains/test_contributions_pivot.py b/mpcontribs-api/tests/unit/domains/test_contributions_pivot.py index e934c8936..3c8556e26 100644 --- a/mpcontribs-api/tests/unit/domains/test_contributions_pivot.py +++ b/mpcontribs-api/tests/unit/domains/test_contributions_pivot.py @@ -3,12 +3,12 @@ import pytest -from mpcontribs_api.domains._shared.types import coerce_key, map_keys, to_snake_case +from mpcontribs_api.domains._shared.types import coerce_key, map_keys, to_camel_case, to_snake_case from mpcontribs_api.domains.contributions.data import ( parse_annotated_key, ) -# The recursive snake_case key walk formerly exposed as ``data.coerce_keys`` — now the shared +# The recursive camelCase key walk formerly exposed as ``data.coerce_keys`` — now the shared # ``map_keys`` driven by ``coerce_key`` with the strict (ASCII-checking) key guard. _coerce_keys = partial(map_keys, coerce=partial(coerce_key, require_ascii=True)) from mpcontribs_api.domains.contributions.models import ContributionIn @@ -28,7 +28,7 @@ def _contrib_in(data, **overrides) -> ContributionIn: # --------------------------------------------------------------------------- -# to_snake_case / map_keys + coerce_key +# to_snake_case / to_camel_case / map_keys + coerce_key # --------------------------------------------------------------------------- @@ -55,10 +55,35 @@ def test_only_separators_reduces_to_empty(self): assert to_snake_case("***") == "" +class TestToCamelCase: + @pytest.mark.parametrize( + ("raw", "expected"), + [ + ("band_gap", "bandGap"), + ("Band Gap", "bandGap"), + ("BandGap", "bandGap"), + ("pH-Value", "phValue"), + ("Seebeck coef", "seebeckCoef"), + ("T", "t"), + ("carrier_transport", "carrierTransport"), + ("bandGap", "bandGap"), # already camelCase -> no-op + (" spaced key ", "spacedKey"), + ("multiple___underscores", "multipleUnderscores"), + ("2theta", "2theta"), + ("HTTPServer", "httpServer"), + ], + ) + def test_coercion(self, raw, expected): + assert to_camel_case(raw) == expected + + def test_only_separators_reduces_to_empty(self): + assert to_camel_case("***") == "" + + class TestMapKeys: def test_recurses_dicts_and_lists(self): out = _coerce_keys({"Band Gap": {"pH-Value": 1}, "List": [{"Inner Key": 2}]}) - assert out == {"band_gap": {"ph_value": 1}, "list": [{"inner_key": 2}]} + assert out == {"bandGap": {"phValue": 1}, "list": [{"innerKey": 2}]} def test_leaves_values_untouched(self): # only keys are coerced; string/number values pass through verbatim @@ -155,8 +180,8 @@ def test_pivots_into_one_row_per_signature(self): rows = expand_contribution( _contrib_in( { - "conductivity (S/cm, T=300K, P=1atm)": 4.2, - "conductivity (S/cm, T=400K, P=1atm)": 5.1, + "conductivity (S/cm, t=300K, p=1atm)": 4.2, + "conductivity (S/cm, t=400K, p=1atm)": 5.1, "bandgap (eV)": 1.1, } ) @@ -164,38 +189,38 @@ def test_pivots_into_one_row_per_signature(self): assert len(rows) == 2 assert len({r.condition_key for r in rows}) == 2 for r in rows: - # conditions + measurement + broadcast column all present; condition names are - # snake_case-coerced (T -> t, P -> p) like every other data key. + # conditions + measurement + broadcast column all present. Condition names must already be + # canonical (the model rejects non-canonical keys), so they become columns verbatim. assert set(r.contribution.data) == {"t", "p", "conductivity", "bandgap"} def test_condition_less_column_broadcasts(self): rows = expand_contribution( - _contrib_in({"x (eV, T=300K)": 1.0, "x (eV, T=400K)": 2.0, "shared (eV)": 9.0}) + _contrib_in({"x (eV, t=300K)": 1.0, "x (eV, t=400K)": 2.0, "shared (eV)": 9.0}) ) assert len(rows) == 2 for r in rows: assert math.isclose(r.contribution.data["shared"]["value"], 9.0) def test_conditions_stored_as_columns(self): - rows = expand_contribution(_contrib_in({"conductivity (S/cm, T=300K)": 4.2})) + rows = expand_contribution(_contrib_in({"conductivity (S/cm, t=300K)": 4.2})) assert len(rows) == 1 data = rows[0].contribution.data - # condition name coerced to snake_case (T -> t) + # the (already-canonical) condition name becomes a column assert "t" in data assert math.isclose(data["t"]["si_value"], 300.0) def test_dotted_path_nests(self): - rows = expand_contribution(_contrib_in({"a.b.c (eV, T=300K)": 2.0})) + rows = expand_contribution(_contrib_in({"a.b.c (eV, t=300K)": 2.0})) data = rows[0].contribution.data assert "si_value" in data["a"]["b"]["c"] def test_same_name_signature_different_unit_collision(self): with pytest.raises(ValidationError, match="same path"): - expand_contribution(_contrib_in({"x (S/cm, T=300K)": 1, "x (mS/cm, T=300K)": 2})) + expand_contribution(_contrib_in({"x (S/cm, t=300K)": 1, "x (mS/cm, t=300K)": 2})) def test_condition_name_collides_with_measurement(self): with pytest.raises(ValidationError, match="same path"): - expand_contribution(_contrib_in({"T (K, T=300K)": 1})) + expand_contribution(_contrib_in({"t (K, t=300K)": 1})) def test_malformed_annotation_raises_validation_error(self): # ContributionIn validation already rejects most malformed keys; expand raises on any that @@ -210,7 +235,7 @@ def test_components_ride_along_on_every_pivoted_row(self): # same components, so every pivoted row keeps the full component set (the insert path stores # them once, deduplicated by hash, and links every row to the shared ids). struct = _structure_in() - c = _contrib_in({"x (eV, T=300K)": 1, "x (eV, T=400K)": 2}, structures=[struct]) + c = _contrib_in({"x (eV, t=300K)": 1, "x (eV, t=400K)": 2}, structures=[struct]) rows = expand_contribution(c) assert len(rows) == 2 assert len({r.condition_key for r in rows}) == 2 @@ -225,45 +250,31 @@ def test_components_allowed_when_not_pivoting(self): rows = expand_contribution(c) assert len(rows) == 1 - def test_plain_keys_coerced_when_no_annotations(self): - rows = expand_contribution(_contrib_in({"Band Gap": 1.5, "nested": {"Sub Key": 2}})) - assert len(rows) == 1 - # keys coerced to snake_case, numeric values promoted to leaves - assert rows[0].contribution.data == {"band_gap": {"si_value": 1.5}, "nested": {"sub_key": {"si_value": 2.0}}} - - def test_already_snake_case_keys_still_leafify_numbers(self): + def test_already_camel_case_keys_still_leafify_numbers(self): # Even with nothing to coerce, bare numbers are normalized to quantity leaves. - c = _contrib_in({"band_gap": 1.5, "nested": {"sub_key": 2}}) + c = _contrib_in({"bandGap": 1.5, "nested": {"subKey": 2}}) rows = expand_contribution(c) - assert rows[0].contribution.data == {"band_gap": {"si_value": 1.5}, "nested": {"sub_key": {"si_value": 2.0}}} + assert rows[0].contribution.data == {"bandGap": {"si_value": 1.5}, "nested": {"subKey": {"si_value": 2.0}}} - def test_annotated_path_segments_coerced(self): - rows = expand_contribution(_contrib_in({"Band Gap (eV, T=300K)": 1.1})) + def test_annotated_key_produces_name_and_condition_columns(self): + # An annotated key with an already-canonical name and condition pivots into a measurement + # column plus a condition column; the unit annotates the measurement leaf. + rows = expand_contribution(_contrib_in({"bandGap (eV, t=300K)": 1.1})) data = rows[0].contribution.data - assert set(data) == {"band_gap", "t"} - assert data["band_gap"]["unit"] == "eV" + assert set(data) == {"bandGap", "t"} + assert data["bandGap"]["unit"] == "eV" - def test_forbidden_name_chars_folded_to_underscore(self): - # '*', '/', and '|' are allowed in the name portion but folded to '_' (not rejected). The - # same characters stay verbatim inside a unit (S/cm), which is never snake_cased. - rows = expand_contribution(_contrib_in({"a/b*c|d (S/cm)": 5})) + def test_dotted_path_nests_unit_only(self): + # A canonical dotted path nests, and its scalar is promoted to a leaf. + rows = expand_contribution(_contrib_in({"outer.innerKey (eV)": 1.1})) data = rows[0].contribution.data - assert set(data) == {"a_b_c_d"} - assert data["a_b_c_d"]["unit"] == "S/cm" - - def test_dotted_path_segments_coerced(self): - rows = expand_contribution(_contrib_in({"Outer.Inner Key (eV)": 1.1})) - data = rows[0].contribution.data - assert "si_value" in data["outer"]["inner_key"] + assert "si_value" in data["outer"]["innerKey"] def test_unit_and_condition_value_preserved_verbatim(self): - # unit (eV) and condition value (300K -> canonical) are never snake_cased; only names are - rows = expand_contribution(_contrib_in({"Band Gap (eV, Temp=300K)": 1.1})) + # unit (eV) and condition value (300K -> canonical magnitude) are never coerced; names are + # required to be canonical already, so they pass through as the stored columns. + rows = expand_contribution(_contrib_in({"bandGap (eV, temp=300K)": 1.1})) data = rows[0].contribution.data - assert data["band_gap"]["unit"] == "eV" - assert "temp" in data # condition name coerced + assert data["bandGap"]["unit"] == "eV" + assert "temp" in data assert math.isclose(data["temp"]["si_value"], 300.0) - - def test_coercion_collision_across_columns_rejected(self): - with pytest.raises(ValidationError, match="same path"): - expand_contribution(_contrib_in({"Band Gap (eV)": 1, "band_gap (eV)": 2})) diff --git a/mpcontribs-api/tests/unit/domains/test_projects_models.py b/mpcontribs-api/tests/unit/domains/test_projects_models.py index 5a35cbdac..2c4cf8f54 100644 --- a/mpcontribs-api/tests/unit/domains/test_projects_models.py +++ b/mpcontribs-api/tests/unit/domains/test_projects_models.py @@ -35,6 +35,12 @@ def test_dotted_path_accepted_even_if_absent_from_columns(self): # No subset-of-columns check: columns is derived/eventually-consistent. assert self._make_input(unique_column="conditions.temp").unique_column == "conditions.temp" + def test_segments_coerced_to_camel_case(self): + # unique_column is a path into Contribution.data, so each segment is coerced to the same + # canonical camelCase form as data keys, keeping the path aligned with stored data. + assert self._make_input(unique_column="sample_id").unique_column == "sampleId" + assert self._make_input(unique_column="nested.Sample Id").unique_column == "nested.sampleId" + def test_empty_string_rejected(self): with pytest.raises(ValidationError): self._make_input(unique_column="") @@ -43,10 +49,17 @@ def test_blank_segment_rejected(self): with pytest.raises(ValidationError): self._make_input(unique_column="a..b") + def test_segment_reducing_to_empty_rejected(self): + with pytest.raises(ValidationError): + self._make_input(unique_column="a.***") + def test_patch_validates_unique_column(self): with pytest.raises(ValidationError): ProjectPatch(unique_column="a..b") + def test_patch_coerces_unique_column(self): + assert ProjectPatch(unique_column="sample_id").unique_column == "sampleId" + # --------------------------------------------------------------------------- # Column # --------------------------------------------------------------------------- diff --git a/mpcontribs-api/tests/unit/test_types.py b/mpcontribs-api/tests/unit/test_types.py index a7ec39978..9ccc67c7d 100644 --- a/mpcontribs-api/tests/unit/test_types.py +++ b/mpcontribs-api/tests/unit/test_types.py @@ -4,11 +4,16 @@ from mpcontribs_api.exceptions import ValidationError as AppValidationError from mpcontribs_api.domains._shared.types import ( + CANONICAL_KEY_COERCION, DisplayStr, + KeyOffense, NFKCStr, PrefixedEmail, - SearchStr, ShortStr, + SearchStr, + coerce_key, + to_camel_case, + to_snake_case, _validate_prefixed_email, nfc_normalize, nfkc_normalize, @@ -184,3 +189,61 @@ def test_invalid_email_raises_app_validation_error(self): def test_whitespace_stripped(self): m = PrefixedEmailModel(email=" orcid:12345@orcid.org ") assert m.email == "orcid:12345@orcid.org" + + +class TestCanonicalKeyCoercion: + """The single source of truth every data-key call site shares, so they can't drift.""" + + def test_points_at_the_current_choice(self): + # Documents that camelCase is today's canonical form. Swapping this one symbol re-points every + # site (data keys via coerce_key, Project.unique_column) at once. + assert CANONICAL_KEY_COERCION is to_camel_case + + def test_coerce_key_defaults_to_the_canonical_symbol(self): + assert coerce_key("band_gap") == CANONICAL_KEY_COERCION("band_gap") == "bandGap" + + def test_coercion_method_override_is_honored(self): + # The parameterization still works: an explicit method overrides the canonical default. + assert coerce_key("bandGap", coercion_method=to_snake_case) == "band_gap" + + +class TestCanonicalKeyOffense: + """The non-raising, non-rewriting predicate behind rejecting non-canonical data keys.""" + + def test_already_canonical_key_is_no_offense(self): + assert KeyOffense.from_key("bandGap") is None + assert KeyOffense.from_key("volume") is None + assert KeyOffense.from_key("2theta") is None + + @pytest.mark.parametrize( + ("key", "suggestion"), + [ + ("band_gap", "bandGap"), + ("Band Gap", "bandGap"), + ("BandGap", "bandGap"), + ("pH-Value", "phValue"), + ("si_value", "siValue"), + ], + ) + def test_non_canonical_key_suggests_its_camel_case_form(self, key, suggestion): + # The suggestion is exactly what the canonical coercion would produce, so it never drifts from + # the accept/reject rule. + assert KeyOffense.from_key(key) == KeyOffense(key=key, suggestion=suggestion, reason="not_camel_case") + assert suggestion == CANONICAL_KEY_COERCION(key) + + def test_non_ascii_key_has_no_suggestion(self): + assert KeyOffense.from_key("ΔE") == KeyOffense(key="ΔE", suggestion=None, reason="non_ascii") + + def test_non_string_key_is_non_ascii_offense(self): + assert KeyOffense.from_key(3) == KeyOffense(key=3, suggestion=None, reason="non_ascii") + + def test_empties_out_key_has_no_suggestion(self): + assert KeyOffense.from_key("***") == KeyOffense(key="***", suggestion=None, reason="empty_after_coercion") + + def test_reserved_key_is_flagged_only_when_reserved_supplied(self): + # A key that is already canonical but names a reserved leaf field is an offense only when the + # caller passes the reserved set; without it the key is accepted. + assert KeyOffense.from_key("unit") is None + assert KeyOffense.from_key("unit", reserved=frozenset({"unit"})) == KeyOffense( + key="unit", suggestion=None, reason="reserved" + )