diff --git a/docs/api/schema-specs.md b/docs/api/schema-specs.md index 16610500..da7e809d 100644 --- a/docs/api/schema-specs.md +++ b/docs/api/schema-specs.md @@ -311,7 +311,7 @@ fields: |-------|----------|-------------| | `name` | yes | Field identifier (snake_case) | | `type` | yes | Data type (see Field Types) | -| `required` | no | Whether mandatory (default: false) | +| `required` | no | Whether a **valid** entity must carry it (default: false). Reported by validation, not enforced when the entity is built — see [Required fields](#required-fields) | | `description` | no | Human-readable description | | `ontology_term` | no | Semantic ontology reference (e.g., `MIAPPE:DM-1`) | | `ontologies` | no | List of OLS IDs to search for `ontology_term` type fields | @@ -681,6 +681,27 @@ fields: Field constraints are enforced by Pydantic when creating model instances. Invalid data raises a validation error immediately. +`required` is the exception: it is not enforced at creation. See below. + +### Required fields + +`required: true` says what a **valid** entity must carry. It does not stop an entity being created or saved without it — validation reports the gap instead. + +```python +client.create_entity("Investigation", {"title": "Work in progress"}) # no unique_id: fine +client.validate() # reports: unique_id — Field 'unique_id' is required +``` + +The reason is that metadata is gathered a piece at a time. Refusing the whole entity because one field is not known yet would throw away the fields that are known, which is the opposite of useful when an agent or a person is working through a source document. + +What this does **not** relax: + +- A **wrong** value is still refused at creation. `unique_id: "not a valid id!"` breaks the field's `pattern` and raises, exactly as before. Only absence is tolerated. +- The generated JSON Schema still lists the profile's required fields, so a consumer reading the schema can enforce whatever it likes. +- Any surface that saves an entity reports which required fields were missing when it saved. + +A parent–child edge comes from the tree, so a child placed under its parent is linked whether or not it also carries a reference back. Marking that reference `required` is a profile decision, reported like any other required field. + ### Validation Rules (Engine Layer) Use for **cross-field** or **cross-entity** validation: diff --git a/docs/specification/system-specification.md b/docs/specification/system-specification.md index d376902d..78f7b3ab 100644 --- a/docs/specification/system-specification.md +++ b/docs/specification/system-specification.md @@ -38,8 +38,8 @@ reference. This document specifies the *behavior* built on top of it. A profile defines a directed hierarchy of entity types rooted at one `root_entity`. At runtime metaseed generates one Pydantic model per entity type -from the profile, so field types, requiredness, and constraints are enforced by -model construction. +from the profile, so field types and constraints are enforced by model +construction. Requiredness is not: see [Required fields](#required-fields). A dataset is a tree of entities: @@ -72,12 +72,14 @@ produce the same model surface (field names, types, requiredness). Validation has two layers, and both run before a dataset is considered valid. -1. **Field constraints (model layer).** Type, requiredness, and per-field - constraints (`pattern`, `min_length`/`max_length`, `minimum`/`maximum`, +1. **Field constraints (model layer).** Type and per-field constraints + (`pattern`, `min_length`/`max_length`, `minimum`/`maximum`, `min_items`/`max_items`, `enum`) are enforced when an entity model is constructed. A validator MUST check every constraint it advertises: list cardinality and zero-valued length bounds are enforced, not silently skipped (see [ADR 002](../architecture/decisions/002-edge-case-behavior.md)). + Requiredness is deliberately excluded here and reported by the engine layer + instead; see [Required fields](#required-fields). 2. **Validation rules (engine layer).** Cross-field and cross-entity rules (uniqueness, coordinate pairs, conditional requirements, reference integrity) run over the assembled dataset. @@ -99,6 +101,34 @@ The distinction between the two layers, and when to use a field constraint versu a rule, is detailed in [Specification Language › Validation](../api/schema-specs.md#validation-field-constraints-vs-rules). +### Required fields + +`required: true` states what a **valid** entity must carry. It does not gate +construction: an entity with a required field missing is built and stored, and +validation reports the gap. + +This is deliberate. Metadata is assembled a piece at a time, often by an agent +reading a source document, and refusing the whole entity because one field is +not known yet would discard the fields that are. The purpose of `required` is to +guide an incomplete record towards a correct one, not to prevent the incomplete +record existing. + +Three consequences follow, and an implementation MUST honour all three: + +- A **missing** value is allowed; a **wrong** value is not. A field that breaks + its `pattern`, bounds, or type is still rejected at construction. +- The published JSON Schema MUST continue to declare the profile's required + fields, so a consumer can decide for itself what to enforce. The generated + model no longer expresses requiredness, so this list comes from the profile + via `spec_required_fields()`. +- Any surface that saves an entity MUST report the missing required fields it + saved with. Nothing raises, so the guidance has to be looked for rather than + caught. + +A parent–child edge is carried by the tree, not by the child's reference field. +Whether a child must also name its parent is the profile's decision, expressed +as `required` on that reference and reported by validation like any other. + ## Serialization A dataset serializes to a hierarchical (tree) structure suitable for JSON, and to diff --git a/src/metaseed/agent/mcp/tools/profiles.py b/src/metaseed/agent/mcp/tools/profiles.py index 4041e34c..726b5538 100644 --- a/src/metaseed/agent/mcp/tools/profiles.py +++ b/src/metaseed/agent/mcp/tools/profiles.py @@ -443,10 +443,12 @@ def get_entity_fields(entity_type: str, profile: str, version: str) -> str: @mcp.tool() def get_required_fields(entity_type: str, profile: str, version: str) -> str: - """Get only the required field names for an entity type. + """Get the field names a profile marks required for an entity type. - Returns a simple list of field names that are mandatory when - creating an entity. Use this for quick validation checks. + Required drives validation reporting, not creation: an entity saves + with any of these missing, and validate_dataset then reports each gap. + Record what the source actually states and leave the rest empty rather + than inventing a value to satisfy this list. Args: entity_type: Entity type name (e.g., "Investigation", "Study"). diff --git a/src/metaseed/forms/__init__.py b/src/metaseed/forms/__init__.py index 0fff6a1c..7b80ba86 100644 --- a/src/metaseed/forms/__init__.py +++ b/src/metaseed/forms/__init__.py @@ -242,3 +242,38 @@ def format_validation_errors(e: ValidationError) -> str: friendly_messages.append(f"{field}: {msg}") return "; ".join(friendly_messages) + + +def missing_required_fields(instance: Any, values: dict[str, Any]) -> list[str]: + """Return the profile-required fields this entity has not filled in yet. + + Required no longer stops an entity being built, so a missing value raises + nothing to report. The guidance still has to reach the user, and this is + where it comes from. + + Args: + instance: The created entity instance. + values: The submitted values. + + Returns: + The names of required fields with no value, in the profile's order. + """ + required = getattr(type(instance), "spec_required_fields", lambda: ())() + return [ + name + for name in required + if values.get(name) in (None, "", [], {}) + and getattr(instance, name, None) in (None, "", [], {}) + ] + + +def format_missing_required(names: list[str]) -> str: + """Format missing required field names the way validation errors read. + + Args: + names: Field names with no value. + + Returns: + A semicolon-separated string, empty when nothing is missing. + """ + return "; ".join(f"{name}: This field is required" for name in names) diff --git a/src/metaseed/models/factory.py b/src/metaseed/models/factory.py index 680de570..725b0b26 100644 --- a/src/metaseed/models/factory.py +++ b/src/metaseed/models/factory.py @@ -206,9 +206,12 @@ def _coerce_string_to_entity( for field_name in simple_primary_fields: if field_name in model_fields: - # Check if this is the only required field (simple entity) + # Ask the spec: the model no longer marks anything Pydantic-required, + # so is_required() would report none and coerce far more than intended. required_fields = [ - name for name, info in model_fields.items() if info.is_required() + name + for name in getattr(model_class, "__spec_required__", ()) + if name in model_fields ] if len(required_fields) <= 1: try: @@ -235,6 +238,51 @@ class EntityBaseModel(BaseModel): extra="forbid", ) + @classmethod + def spec_required_fields(cls) -> tuple[str, ...]: + """Return the field names this entity's profile marks required. + + A required field is not enforced when the entity is built -- metadata + arrives a piece at a time, and refusing the whole entity would discard + the values that are known. ``RequiredFieldsRule`` reports the missing + ones at validation time, and this is where that list comes from. + + Returns: + The required field names, empty when the profile declares none. + """ + return getattr(cls, "__spec_required__", ()) + + @classmethod + def model_json_schema(cls, *args: Any, **kwargs: Any) -> dict[str, Any]: + """Return the JSON Schema, still declaring the profile's required fields. + + Construction does not enforce required, so Pydantic would describe every + field as nullable with no required list -- a description that contradicts + the profile. Consumers decide for themselves what to do about a required + field; they cannot do that if we stop telling them which ones there are. + """ + schema: dict[str, Any] = super().model_json_schema(*args, **kwargs) + properties = schema.get("properties", {}) + required = [name for name in cls.spec_required_fields() if name in properties] + if not required: + return schema + + for name in required: + prop = properties[name] + # Undo the "or null" the permissive field carries, so the type reads + # as the profile declares it rather than as the storage allows. + branches = [b for b in prop.get("anyOf", []) if b.get("type") != "null"] + if len(branches) == 1: + title = prop.get("title") + properties[name] = { + **branches[0], + **({"title": title} if title else {}), + } + properties[name].pop("default", None) + + schema["required"] = required + return schema + @model_validator(mode="before") @classmethod def _convert_nested_entities(cls, data: Any) -> Any: @@ -398,8 +446,6 @@ def _create_field_definition(field: FieldSpec) -> tuple[Any, Any]: # The element is a dynamically built Literal special form, which # cannot be expressed as a static type subscript. python_type = list[_build_enum_type(enum_values)] # type: ignore[misc] - if field.required: - return (python_type, Field(**constraints)) constraints["default_factory"] = list return (python_type, Field(**constraints)) @@ -410,18 +456,12 @@ def _create_field_definition(field: FieldSpec) -> tuple[Any, Any]: annotated_type = ( Annotated[python_type, Field(**constraints)] if constraints else python_type ) - if field.required: - return (annotated_type, ...) return (annotated_type | None, None) annotated_type = ( Annotated[python_type, Field(**constraints)] if constraints else python_type ) - if field.required: - if constraints: - return (annotated_type, ...) - return (python_type, ...) return (annotated_type | None, None) @@ -461,6 +501,13 @@ def create_model_from_spec(spec: EntitySpec) -> type: ) model.__entity_fields__ = entity_fields # type: ignore[attr-defined] + # The spec's required list, kept because the model no longer expresses it: + # required drives validation reporting, not construction. Anything that + # needs to know which fields a profile calls required must read this rather + # than ask Pydantic, which now answers "none". + model.__spec_required__ = tuple( # type: ignore[attr-defined] + field.name for field in spec.fields if field.required + ) register_model(spec.name, model) diff --git a/src/metaseed/specs/schema.py b/src/metaseed/specs/schema.py index 48f92961..82a2af66 100644 --- a/src/metaseed/specs/schema.py +++ b/src/metaseed/specs/schema.py @@ -128,7 +128,12 @@ class FieldSpec(BaseModel): name: Field identifier (snake_case). codename: BrAPI-compatible camelCase identifier (optional). type: Data type of the field. - required: Whether the field is mandatory. + required: Whether a valid entity must carry this field. It is not + enforced when the entity is built: metadata arrives a piece at a + time, and refusing the whole entity would discard the values that + are known. ``RequiredFieldsRule`` reports the missing ones at + validation time, and the published JSON Schema still declares them + so a consumer can decide for itself what to enforce. description: Human-readable description. ontology_term: Reference to ontology term (e.g., MIAPPE:DM-1). ontologies: List of OLS IDs to search for ontology_term type fields. @@ -249,7 +254,10 @@ def _check_single_markers(self: Self) -> Self: return self def get_required_fields(self: Self) -> list[FieldSpec]: - """Return list of required fields. + """Return the fields a valid entity must carry. + + These are reported by validation when absent rather than enforced when + the entity is built; see ``FieldSpec.required``. Returns: List of FieldSpec objects where required is True. diff --git a/src/metaseed/ui/helpers/__init__.py b/src/metaseed/ui/helpers/__init__.py index 8306920d..6dc9b6ff 100644 --- a/src/metaseed/ui/helpers/__init__.py +++ b/src/metaseed/ui/helpers/__init__.py @@ -22,9 +22,11 @@ collect_form_values, field_errors_from_validation, filter_fields, + format_missing_required, format_validation_errors, get_field_data, is_nested_field, + missing_required_fields, ) # Re-export from entity_helpers @@ -75,6 +77,7 @@ "extract_nested_items", "field_errors_from_validation", "filter_fields", + "format_missing_required", "format_table_rows", "format_validation_errors", "get_field_data", @@ -87,6 +90,7 @@ "get_table_columns", "infer_entity_type_from_field", "is_nested_field", + "missing_required_fields", "process_reference_linked_children", "rebuild_nested_items_with_failures", "to_dict", diff --git a/src/metaseed/ui/routes/crud.py b/src/metaseed/ui/routes/crud.py index f90fd39d..b817d744 100644 --- a/src/metaseed/ui/routes/crud.py +++ b/src/metaseed/ui/routes/crud.py @@ -18,7 +18,9 @@ collect_form_values, extract_nested_items, field_errors_from_validation, + format_missing_required, format_validation_errors, + missing_required_fields, process_reference_linked_children, rebuild_nested_items_with_failures, ) @@ -84,8 +86,13 @@ async def create_entity(request: Request) -> HTMLResponse: # required children afterwards. try: instance = helper.create(**values) - warning = None - field_errors: dict[str, str] = {} + # Building no longer fails on a missing required field, so the + # guidance has to be looked for rather than caught. + missing = missing_required_fields(instance, values) + warning = format_missing_required(missing) or None + field_errors: dict[str, str] = dict.fromkeys( + missing, "This field is required" + ) except ValidationError as e: instance = helper.create(skip_validation=True, **values) warning = format_validation_errors(e) @@ -160,8 +167,13 @@ async def update_entity(request: Request, node_id: str) -> HTMLResponse: # a non-blocking warning alongside any child reference-linking failures. try: instance = helper.create(**values) - parent_warning = None - field_errors: dict[str, str] = {} + # As on create: a missing required field no longer raises, so the + # warning has to be looked for rather than caught. + missing = missing_required_fields(instance, values) + parent_warning = format_missing_required(missing) or None + field_errors: dict[str, str] = dict.fromkeys( + missing, "This field is required" + ) except ValidationError as e: instance = helper.create(skip_validation=True, **values) parent_warning = format_validation_errors(e) diff --git a/src/metaseed/validators/rules.py b/src/metaseed/validators/rules.py index bc99d690..ac5ae6b3 100644 --- a/src/metaseed/validators/rules.py +++ b/src/metaseed/validators/rules.py @@ -156,7 +156,11 @@ def _parse_date( class RequiredFieldsRule(ValidationRule): - """Validates that required fields are present and non-empty. + """Reports required fields that are absent or empty. + + Building an entity does not enforce requiredness, so this rule is the only + thing that tells anyone a required value is missing. Removing it would not + loosen validation; it would make the gap invisible. Attributes: fields: List of required field names. diff --git a/tests/test_api/test_client.py b/tests/test_api/test_client.py index 8f6ac52d..7f2d81fa 100644 --- a/tests/test_api/test_client.py +++ b/tests/test_api/test_client.py @@ -180,25 +180,35 @@ def test_update_entity_not_found_raises(self, client: MetaseedClient) -> None: def test_create_entity_invalid_data_raises_api_validation_error( self, client: MetaseedClient ) -> None: - """Missing required field raises the public api ValidationError.""" + """A wrong value is refused; a missing one is not. + + Required says what a valid entity needs, so an entity still being filled + in saves and validation reports the gap. A value that breaks the + profile's rules is a different matter and is still rejected here. + """ with pytest.raises(ValidationError) as exc_info: - client.create_entity("Investigation", {"title": "no unique_id"}) + client.create_entity( + "Investigation", {"unique_id": "not a valid id!", "title": "T"} + ) error = exc_info.value assert isinstance(error, MetaseedError) assert any(detail["field"] == "unique_id" for detail in error.errors) + # Missing, as opposed to wrong, is allowed through. + client.create_entity("Investigation", {"title": "no unique_id"}) + def test_update_entity_invalid_data_raises_api_validation_error( self, client: MetaseedClient ) -> None: - """Updating with invalid data raises the public api ValidationError.""" + """Updating to a wrong value raises; clearing a required one does not.""" entity = client.create_entity( "Investigation", {"unique_id": "INV-001", "title": "Original"}, ) with pytest.raises(ValidationError) as exc_info: - client.update_entity(entity.id, {"title": "missing unique_id"}) + client.update_entity(entity.id, {"unique_id": "not a valid id!"}) assert any(detail["field"] == "unique_id" for detail in exc_info.value.errors) @@ -864,20 +874,21 @@ def test_create_entity_skip_validation(self, client: MetaseedClient) -> None: assert entity.data.get("title") == "Work in progress" assert entity.id is not None - def test_create_entity_without_skip_validation_raises( + def test_create_entity_without_skip_validation_allows_a_missing_field( self, client: MetaseedClient ) -> None: - """Create entity without skip_validation raises on missing required field. + """A missing required field no longer needs skip_validation. - The public boundary translates pydantic errors into the documented - ``metaseed.api.errors.ValidationError`` rather than leaking the - internal pydantic exception. + skip_validation remains the way to store a value that breaks the + profile's rules; absence alone is an ordinary in-progress state. """ - with pytest.raises(ValidationError): - client.create_entity( - "Investigation", - {"title": "Incomplete"}, # missing unique_id - ) + entity = client.create_entity( + "Investigation", + {"title": "Incomplete"}, # missing unique_id + ) + + assert entity.data.get("title") == "Incomplete" + assert entity.data.get("unique_id") is None def test_update_entity_skip_validation(self, client: MetaseedClient) -> None: """Update entity with skip_validation bypasses validation.""" diff --git a/tests/test_models/test_factory.py b/tests/test_models/test_factory.py index d1e62bc0..1c29e74f 100644 --- a/tests/test_models/test_factory.py +++ b/tests/test_models/test_factory.py @@ -35,8 +35,14 @@ def test_create_simple_model(self) -> None: instance = Model(name="test") assert instance.name == "test" - def test_required_fields_enforced(self) -> None: - """Required fields must be provided.""" + def test_required_fields_do_not_block_construction(self) -> None: + """A required field records what validation should report, not a gate. + + Metadata is gathered incrementally: refusing to build an entity because + one field is not known yet loses the fields that *are* known. Missing + required values are reported by ``RequiredFieldsRule`` at validation + time instead. + """ spec = EntitySpec( name="WithRequired", version="1.0", @@ -53,9 +59,9 @@ def test_required_fields_enforced(self) -> None: Model = create_model_from_spec(spec) - with pytest.raises(ValidationError) as exc_info: - Model() - assert "required_field" in str(exc_info.value) + instance = Model() + assert instance.required_field is None + assert "required_field" in Model.spec_required_fields() def test_optional_fields_default_none(self) -> None: """Optional fields default to None.""" @@ -189,8 +195,8 @@ def test_list_max_items_enforced(self) -> None: with pytest.raises(ValidationError): Model(items=["a", "b", "c"]) - def test_required_list_field_enforced(self) -> None: - """A required list field rejects a missing value.""" + def test_required_list_field_defaults_to_empty(self) -> None: + """A required list is still a list when nothing has been recorded yet.""" spec = EntitySpec( name="WithRequiredList", version="1.0", @@ -209,8 +215,8 @@ def test_required_list_field_enforced(self) -> None: Model = create_model_from_spec(spec) assert Model(items=["a"]).items == ["a"] - with pytest.raises(ValidationError): - Model() + assert Model().items == [] + assert "items" in Model.spec_required_fields() def test_optional_list_field_defaults_to_empty(self) -> None: """An optional list field still defaults to an empty list when omitted.""" diff --git a/tests/test_spec_language.py b/tests/test_spec_language.py index 5c5b8225..13bae150 100644 --- a/tests/test_spec_language.py +++ b/tests/test_spec_language.py @@ -1440,8 +1440,12 @@ def test_optional_field_none(self) -> None: instance = Model(value=None) assert instance.value is None - def test_required_field_cannot_be_none(self) -> None: - """Required field cannot be None.""" + def test_a_required_field_may_be_unset_while_work_is_in_progress(self) -> None: + """Required states what a valid entity needs, not what building one needs. + + Metadata arrives a piece at a time; the missing value is reported by + validation rather than refused here. + """ spec = EntitySpec( name="Test", version="1.0", @@ -1453,8 +1457,8 @@ def test_required_field_cannot_be_none(self) -> None: ], ) Model = create_model_from_spec(spec) - with pytest.raises(ValidationError): - Model(value=None) + assert Model(value=None).value is None + assert "value" in Model.spec_required_fields() def test_extra_fields_rejected(self) -> None: """Extra fields not in spec are rejected.""" diff --git a/tests/test_ui/test_reference_linked_children.py b/tests/test_ui/test_reference_linked_children.py index 5ad6a905..84578af8 100644 --- a/tests/test_ui/test_reference_linked_children.py +++ b/tests/test_ui/test_reference_linked_children.py @@ -777,16 +777,29 @@ def test_child_under_parent_gets_reference_filled(self) -> None: ) assert "investigation_id: INV-1" in yaml.safe_dump(client.serialize()) - def test_no_parent_still_requires_the_reference(self) -> None: + def test_a_study_without_a_parent_is_saved_and_reported(self) -> None: + """Placing a child under its parent is what links them, not the back-reference. + + A study created on its own has nowhere to point yet. Whether it must + carry a reference back to its investigation is the profile's decision, + reported by validation, rather than something creation refuses. + """ from metaseed import MetaseedClient - from metaseed.api.errors import ValidationError + from metaseed.validators.api import validate_entity client = MetaseedClient("miappe", "1.2") - try: - client.create_entity("Study", {"unique_id": "STU-1", "title": "S"}) - raise AssertionError("expected ValidationError without a parent") - except ValidationError: - pass + study = client.create_entity("Study", {"unique_id": "STU-1", "title": "S"}) + + assert study.data.get("unique_id") == "STU-1" + assert any( + issue.field == "investigation_id" + for issue in validate_entity( + {"unique_id": "STU-1", "title": "S"}, + "Study", + profile="miappe", + version="1.2", + ) + ) def test_caller_supplied_reference_is_not_overridden(self) -> None: import yaml