From 403b86940c5d7b5d2e61ff29f5f16e44baf67688 Mon Sep 17 00:00:00 2001 From: Fabio Kayser Date: Wed, 28 Oct 2026 22:17:06 +0100 Subject: [PATCH 1/9] fix: separate Unity Catalog metadata and physical source locations --- docs/backend-contract.md | 5 +++ docs/connectors.md | 7 +++ src/datam8/plugins/builtins/databricks_uc.py | 21 +++++++++ tests/test_015_source_mappings.py | 45 ++++++++++++++++++++ tests/test_021_unity_catalog.py | 18 ++++++++ tests/test_022_builtin_integration.py | 26 ++++++++++- 6 files changed, 121 insertions(+), 1 deletion(-) diff --git a/docs/backend-contract.md b/docs/backend-contract.md index 4ebfb24..20c3ce0 100644 --- a/docs/backend-contract.md +++ b/docs/backend-contract.md @@ -163,6 +163,11 @@ editor. Runtime configuration changes and process failure may require a restart. types, business names and the technical object name as a suggested entity name. The selected target locator still determines the imported entity name. Plugins inheriting `get_sources() -> None` keep their existing import path. +- Unity Catalog also supplies `get_sources()`: physical `sourceLocation` uses quoted + SQL identifiers (for example, `` `main`.`sales`.`Customers` ``), while + `metadataLocation` retains the selected connector handle. Import descriptions, + CLI/API imports and refresh use these definitions; `sourceOverride` remains a + compatibility hint for older clients. - Source metadata retains its existing schema: `dataType` is the source type. ODCS uses `physicalType`, or the declared `logicalType` when no physical type exists. Business names enrich existing model fields; no metadata hook overrides target types. diff --git a/docs/connectors.md b/docs/connectors.md index 3d280de..f712a4e 100644 --- a/docs/connectors.md +++ b/docs/connectors.md @@ -137,6 +137,9 @@ row order rather than sorting it. `SourceObject` supplies `name`, `type`, optional `schema`, `description` and `properties`. Its existing `sourceOverride` is exposed in metadata responses for compatible clients; it is not a replacement for `get_sources()` in backend imports. +New plugins should use `get_sources()` for physical locations and mappings rather +than introduce another `sourceOverride` path. No `sourceOverride` is needed to +separate metadata and data read locations. Use the dataframe spelling `numericScale`. The generated `SourceField` and metadata API currently use the legacy spelling `numbericScale`; `TableMetadata` @@ -191,6 +194,7 @@ def get_sources(self, source_location): return [ { "sourceLocation": "raw.customers", + "metadataLocation": "contracts/customers#Customers", "sourceName": "customer_id", "targetName": "id", "sourceAlias": "crm", @@ -211,6 +215,9 @@ dictionary, including both `type` and `nullable`; use the physical source type. `metadataLocation` is the handle to describe on subsequent refreshes; `sourceLocation` is the physical read location. A per-row override can point to another metadata object, and an explicit null opts out of a separate handle. +Omitting `metadataLocation` retains the selected import handle automatically, even +when it differs from the physical `sourceLocation`. `SourceObject.name` remains +the suggested entity name and does not determine either location. | Result | Meaning | | --- | --- | diff --git a/src/datam8/plugins/builtins/databricks_uc.py b/src/datam8/plugins/builtins/databricks_uc.py index 7b4e400..cb89e28 100644 --- a/src/datam8/plugins/builtins/databricks_uc.py +++ b/src/datam8/plugins/builtins/databricks_uc.py @@ -647,6 +647,27 @@ def get_table_metadata(self, source_location: str, /) -> TableMetadata: ), ) + def get_sources(self, source_location: str, /) -> list[dict[str, Any]]: + """Keep the metadata handle separate from the quoted SQL read location.""" + physical_location = _quoted_name(self._location(source_location, table=True)) + metadata = self.get_table_metadata(source_location) + return [ + { + "sourceLocation": physical_location, + "metadataLocation": source_location, + "sourceName": field.name, + "targetName": field.name, + "sourceDataType": { + "type": field.dataType, + "nullable": field.isNullable, + "charLen": field.maxLength, + "precision": field.numericPrecision, + "scale": field.numbericScale, + }, + } + for field in metadata.iter_source_fields() + ] + def get_source_capabilities(self) -> dict[str, bool]: capabilities = super().get_source_capabilities() capabilities["previewData"] = bool(_warehouse_id(self.extended_properties)) diff --git a/tests/test_015_source_mappings.py b/tests/test_015_source_mappings.py index 7aa7308..402105c 100644 --- a/tests/test_015_source_mappings.py +++ b/tests/test_015_source_mappings.py @@ -278,6 +278,51 @@ def test_read_from_data_source_uses_plugin_sources_and_import_location(monkeypat assert entity.sources[1].mapping[0].targetName == "firstName" +@pytest.mark.parametrize("mode", ["complete", "sources-only"]) +def test_plugin_sets_independent_metadata_location_without_source_override(monkeypatch, mode): + calls = [] + + class SeparateLocationsPlugin(SourcePluginStub): + def get_sources(self, location): + calls.append(location) + return [ + { + "sourceLocation": "raw.customers", + "metadataLocation": "contracts/customers#Customers", + "sourceName": name, + "targetName": name, + } + for name in ("id", "firstName") + ] + + def get_table_metadata(self, location): + metadata = super().get_table_metadata(location) + assert metadata.source_object.sourceOverride is None + return metadata + + plugin = SeparateLocationsPlugin() + monkeypatch.setattr(factory, "get_plugin_for_data_source", lambda *_args, **_kwargs: plugin) + model, locator, _ = _source_model() + model.attributeTypes = AttributeTypesStub() + entity = source.read_from_data_source("test", "browser-selection", model=model) + # Exercise serialization as well as repeated refresh from the persisted metadata handle. + model.modelEntities[locator].entity = m.ModelEntity.model_validate_json( + entity.model_dump_json() + ) + + for _ in range(2): + refreshed, diff = source.compare_entity_with_source(locator, model=model, mode=mode) + assert not diff + assert refreshed.entity.sources[0].sourceLocation == "raw.customers" + assert refreshed.entity.sources[0].metadataLocation == "contracts/customers#Customers" + model.modelEntities[locator].entity = refreshed.entity + assert calls == [ + "browser-selection", + "contracts/customers#Customers", + "contracts/customers#Customers", + ] + + def test_import_description_exposes_plugin_entity(monkeypatch) -> None: model = configure_mappings(SimpleNamespace(attributeTypes=AttributeTypesStub())) monkeypatch.setattr(factory, "get_model", lambda: model) diff --git a/tests/test_021_unity_catalog.py b/tests/test_021_unity_catalog.py index 2189883..8e2f354 100644 --- a/tests/test_021_unity_catalog.py +++ b/tests/test_021_unity_catalog.py @@ -91,6 +91,24 @@ def test_locations_round_trip(location, make_plugin): assert list(plugin.get_table_metadata(location).iter_source_fields())[0].name == "id" +@pytest.mark.parametrize( + "location", + ["main.sales.Customers", "[main.sales].[Customers]", "`main`.`sales`.`Customers`"], +) +def test_source_definitions_separate_metadata_handle_and_sql_location(location, make_plugin): + definitions = make_plugin().get_sources(location) + + assert {row["metadataLocation"] for row in definitions} == {location} + assert {row["sourceLocation"] for row in definitions} == {"`main`.`sales`.`Customers`"} + assert [(row["sourceName"], row["targetName"]) for row in definitions] == [ + (name, name) for name in ("id", "name", "amount", "active", "details") + ] + assert definitions[1]["sourceDataType"]["charLen"] == 80 + assert definitions[2]["sourceDataType"]["precision"] == 10 + assert definitions[2]["sourceDataType"]["scale"] == 0 + assert definitions[4]["sourceDataType"]["type"] == "struct" + + @pytest.mark.parametrize( "location", [ diff --git a/tests/test_022_builtin_integration.py b/tests/test_022_builtin_integration.py index 95959da..bd87a47 100644 --- a/tests/test_022_builtin_integration.py +++ b/tests/test_022_builtin_integration.py @@ -223,6 +223,15 @@ def test_uc_builtin_api_import_save_reload_refresh( factory._model = factory.create_model() with TestClient(create_app()) as client: assert client.get("/sources/UC/test").status_code == 200 + description = client.get( + "/sources/UC/locations/import-description", + params={"source_location": "[main.sales].[Customers]"}, + ) + assert description.status_code == 200, description.text + described = description.json()["entity"] + assert described["name"] == "Customers" + assert described["sources"][0]["sourceLocation"] == "`main`.`sales`.`Customers`" + assert described["sources"][0]["metadataLocation"] == "[main.sales].[Customers]" response = client.put( "/sources/UC/import", json={ @@ -232,6 +241,8 @@ def test_uc_builtin_api_import_save_reload_refresh( ) assert response.status_code == 200, response.text saved = json.loads((tmp_path / "Model/Stage/Customers.json").read_text()) + assert saved["sources"][0]["sourceLocation"] == "`main`.`sales`.`Customers`" + assert saved["sources"][0]["metadataLocation"] == "[main.sales].[Customers]" assert [a["dataType"]["type"] for a in saved["attributes"]] == [ "long", "string", @@ -242,6 +253,16 @@ def test_uc_builtin_api_import_save_reload_refresh( assert saved["attributes"][2]["dataType"]["scale"] == 0 assert saved["sources"][0]["mapping"][4]["sourceDataType"]["type"] == "struct" assert client.post("/model/reload", params={"force": True}).status_code == 200 + for refresh_mode in ("complete", "sources-only"): + unchanged = client.get( + "/sources/compare", + params={"locator": "modelEntities/Stage/Customers", "mode": refresh_mode}, + ) + assert unchanged.status_code == 200, unchanged.text + assert not unchanged.json()["has_changes"] + refreshed_source = unchanged.json()["wrapper"]["entity"]["sources"][0] + assert refreshed_source["sourceLocation"] == "`main`.`sales`.`Customers`" + assert refreshed_source["metadataLocation"] == "[main.sales].[Customers]" assert ( client.get( "/sources/UC/locations/preview", @@ -287,7 +308,10 @@ def test_cli_navigation_and_import(cls, tmp_path, builtin_plugin, make_plugin): ): result = runner.invoke(app, [*args, "-s", str(path)]) assert result.exit_code == 0, f"{result.output}\n{result.exception}" - assert (tmp_path / "Model/Stage/Imported.json").exists() + saved = json.loads((tmp_path / "Model/Stage/Imported.json").read_text()) + if cls is DatabricksUnityCatalog: + assert saved["sources"][0]["sourceLocation"] == "`main`.`sales`.`Customers`" + assert saved["sources"][0]["metadataLocation"] == location def test_optional_metadata_preserves_existing_plugin_imports( From e2764dff66183811824ebc6b178c2621783f8c55 Mon Sep 17 00:00:00 2001 From: Fabio Kayser Date: Wed, 28 Oct 2026 22:20:45 +0100 Subject: [PATCH 2/9] fix: reuse Unity Catalog source location for metadata --- docs/backend-contract.md | 10 +++++----- src/datam8/plugins/builtins/databricks_uc.py | 4 ++-- tests/test_021_unity_catalog.py | 4 ++-- tests/test_022_builtin_integration.py | 8 ++++---- 4 files changed, 13 insertions(+), 13 deletions(-) diff --git a/docs/backend-contract.md b/docs/backend-contract.md index 20c3ce0..e44b7e4 100644 --- a/docs/backend-contract.md +++ b/docs/backend-contract.md @@ -163,11 +163,11 @@ editor. Runtime configuration changes and process failure may require a restart. types, business names and the technical object name as a suggested entity name. The selected target locator still determines the imported entity name. Plugins inheriting `get_sources() -> None` keep their existing import path. -- Unity Catalog also supplies `get_sources()`: physical `sourceLocation` uses quoted - SQL identifiers (for example, `` `main`.`sales`.`Customers` ``), while - `metadataLocation` retains the selected connector handle. Import descriptions, - CLI/API imports and refresh use these definitions; `sourceOverride` remains a - compatibility hint for older clients. +- Unity Catalog also supplies `get_sources()`: `sourceLocation` uses quoted SQL + identifiers (for example, `` `main`.`sales`.`Customers` ``) for both data reads + and metadata lookup. Its definitions set `metadataLocation` to null, so imports + omit the redundant field. CLI/API imports and refresh use these definitions; + `sourceOverride` remains a compatibility hint for older clients. - Source metadata retains its existing schema: `dataType` is the source type. ODCS uses `physicalType`, or the declared `logicalType` when no physical type exists. Business names enrich existing model fields; no metadata hook overrides target types. diff --git a/src/datam8/plugins/builtins/databricks_uc.py b/src/datam8/plugins/builtins/databricks_uc.py index cb89e28..8feacec 100644 --- a/src/datam8/plugins/builtins/databricks_uc.py +++ b/src/datam8/plugins/builtins/databricks_uc.py @@ -648,13 +648,13 @@ def get_table_metadata(self, source_location: str, /) -> TableMetadata: ) def get_sources(self, source_location: str, /) -> list[dict[str, Any]]: - """Keep the metadata handle separate from the quoted SQL read location.""" + """Use the quoted table location for both metadata and data reads.""" physical_location = _quoted_name(self._location(source_location, table=True)) metadata = self.get_table_metadata(source_location) return [ { "sourceLocation": physical_location, - "metadataLocation": source_location, + "metadataLocation": None, "sourceName": field.name, "targetName": field.name, "sourceDataType": { diff --git a/tests/test_021_unity_catalog.py b/tests/test_021_unity_catalog.py index 8e2f354..a831c56 100644 --- a/tests/test_021_unity_catalog.py +++ b/tests/test_021_unity_catalog.py @@ -95,10 +95,10 @@ def test_locations_round_trip(location, make_plugin): "location", ["main.sales.Customers", "[main.sales].[Customers]", "`main`.`sales`.`Customers`"], ) -def test_source_definitions_separate_metadata_handle_and_sql_location(location, make_plugin): +def test_source_definitions_use_one_location_for_metadata_and_data(location, make_plugin): definitions = make_plugin().get_sources(location) - assert {row["metadataLocation"] for row in definitions} == {location} + assert {row["metadataLocation"] for row in definitions} == {None} assert {row["sourceLocation"] for row in definitions} == {"`main`.`sales`.`Customers`"} assert [(row["sourceName"], row["targetName"]) for row in definitions] == [ (name, name) for name in ("id", "name", "amount", "active", "details") diff --git a/tests/test_022_builtin_integration.py b/tests/test_022_builtin_integration.py index bd87a47..468f54f 100644 --- a/tests/test_022_builtin_integration.py +++ b/tests/test_022_builtin_integration.py @@ -231,7 +231,7 @@ def test_uc_builtin_api_import_save_reload_refresh( described = description.json()["entity"] assert described["name"] == "Customers" assert described["sources"][0]["sourceLocation"] == "`main`.`sales`.`Customers`" - assert described["sources"][0]["metadataLocation"] == "[main.sales].[Customers]" + assert described["sources"][0]["metadataLocation"] is None response = client.put( "/sources/UC/import", json={ @@ -242,7 +242,7 @@ def test_uc_builtin_api_import_save_reload_refresh( assert response.status_code == 200, response.text saved = json.loads((tmp_path / "Model/Stage/Customers.json").read_text()) assert saved["sources"][0]["sourceLocation"] == "`main`.`sales`.`Customers`" - assert saved["sources"][0]["metadataLocation"] == "[main.sales].[Customers]" + assert "metadataLocation" not in saved["sources"][0] assert [a["dataType"]["type"] for a in saved["attributes"]] == [ "long", "string", @@ -262,7 +262,7 @@ def test_uc_builtin_api_import_save_reload_refresh( assert not unchanged.json()["has_changes"] refreshed_source = unchanged.json()["wrapper"]["entity"]["sources"][0] assert refreshed_source["sourceLocation"] == "`main`.`sales`.`Customers`" - assert refreshed_source["metadataLocation"] == "[main.sales].[Customers]" + assert refreshed_source["metadataLocation"] is None assert ( client.get( "/sources/UC/locations/preview", @@ -311,7 +311,7 @@ def test_cli_navigation_and_import(cls, tmp_path, builtin_plugin, make_plugin): saved = json.loads((tmp_path / "Model/Stage/Imported.json").read_text()) if cls is DatabricksUnityCatalog: assert saved["sources"][0]["sourceLocation"] == "`main`.`sales`.`Customers`" - assert saved["sources"][0]["metadataLocation"] == location + assert "metadataLocation" not in saved["sources"][0] def test_optional_metadata_preserves_existing_plugin_imports( From ccb225aa815a41ed93efd1df01e4e63a95ff3161 Mon Sep 17 00:00:00 2001 From: Fabio Kayser Date: Thu, 29 Oct 2026 08:02:36 +0100 Subject: [PATCH 3/9] fix: refresh entity descriptions from source metadata --- docs/backend-contract.md | 5 ++ src/datam8/source.py | 7 +++ tests/test_015_source_mappings.py | 83 +++++++++++++++++++++++++++ tests/test_022_builtin_integration.py | 13 +++++ 4 files changed, 108 insertions(+) diff --git a/docs/backend-contract.md b/docs/backend-contract.md index e44b7e4..b2235cf 100644 --- a/docs/backend-contract.md +++ b/docs/backend-contract.md @@ -210,6 +210,11 @@ editor. Runtime configuration changes and process failure may require a restart. before the model is changed. Source-only refresh does not read attribute metadata. - Returning `None` keeps the default one-source, one-column mapping derived from `get_table_metadata()`. +- Complete refresh adopts the entity description from source metadata. Identical + descriptions from multiple sources are accepted; missing descriptions are ignored + when another source supplies one. Conflicting descriptions are rejected. If all + external sources have no description, the existing description is cleared. + Entities without external sources and source-only refresh retain their description. - Complete refresh updates source-owned attribute metadata (data type, description, business-key flag and supplied properties), while preserving local expressions, display names, history, units, refactor names and deletion markers. Existing diff --git a/src/datam8/source.py b/src/datam8/source.py index 7315856..80c0d9e 100644 --- a/src/datam8/source.py +++ b/src/datam8/source.py @@ -229,6 +229,7 @@ def compare_entity_with_source( refreshed_by_index: dict[int, list[m.ExternalModelSource]] = {} described_attributes: dict[str, at.Attribute] = {} described_properties: dict[str, p.PropertyReference] = {} + described_descriptions: set[str] = set() for existing, location, index in groups: if mode == "sources-only": refreshed = read_external_sources( @@ -249,6 +250,8 @@ def compare_entity_with_source( refreshed = [ item for item in described.sources if isinstance(item, m.ExternalModelSource) ] + if described.description is not None: + described_descriptions.add(described.description) for attr in described.attributes: previous = described_attributes.get(attr.name) comparable = {"ordinalNumber", "dateAdded", "dateModified"} @@ -295,6 +298,10 @@ def compare_entity_with_source( threshold_to_diff_deeper=0, ) + if len(described_descriptions) > 1: + raise utils.create_error(ValueError("Conflicting entity descriptions")) + if groups: + wrapper.entity.description = next(iter(described_descriptions), None) if described_properties: entity_properties = {prop.property: prop for prop in wrapper.entity.properties or []} entity_properties.update(described_properties) diff --git a/tests/test_015_source_mappings.py b/tests/test_015_source_mappings.py index 402105c..e0d9232 100644 --- a/tests/test_015_source_mappings.py +++ b/tests/test_015_source_mappings.py @@ -415,6 +415,7 @@ def _refreshed_sources() -> list[m.ExternalModelSource]: def test_source_only_refresh_preserves_entity_attributes_and_properties(monkeypatch) -> None: model, locator, original = _source_model() + model.modelEntities[locator].entity.description = "Local description" monkeypatch.setattr( source, "read_external_sources", lambda *_args, **_kwargs: _refreshed_sources() ) @@ -427,6 +428,41 @@ def test_source_only_refresh_preserves_entity_attributes_and_properties(monkeypa attribute.name for attribute in original.attributes ] assert wrapper.entity.properties[0].value == "old" + assert wrapper.entity.description == "Local description" + + +@pytest.mark.parametrize("description", ["Updated description", "", None]) +def test_complete_refresh_updates_entity_description_without_mutating_model( + monkeypatch, description +) -> None: + model, locator, original = _source_model() + original.description = "Old description" + model.modelEntities[locator].entity = original + refreshed = original.model_copy(deep=True) + refreshed.description = description + monkeypatch.setattr(source, "read_from_data_source", lambda *_args, **_kwargs: refreshed) + + wrapper, diff = source.compare_entity_with_source(locator, model=model) + + assert wrapper.entity.description == description + assert wrapper._changed and diff + assert model.modelEntities[locator].entity.description == "Old description" + model.modelEntities[locator].entity = wrapper.entity + repeated, repeated_diff = source.compare_entity_with_source(locator, model=model) + assert repeated.entity.description == description + assert not repeated._changed and not repeated_diff + + +def test_refresh_without_external_sources_preserves_entity_description() -> None: + model, locator, original = _source_model() + original.description = "Local description" + original.sources = [] + model.modelEntities[locator].entity = original + + wrapper, diff = source.compare_entity_with_source(locator, model=model) + + assert wrapper.entity.description == "Local description" + assert not diff def test_complete_refresh_updates_entity_properties_and_sources(monkeypatch) -> None: @@ -688,6 +724,53 @@ def test_complete_refresh_merges_multiple_contracts(monkeypatch) -> None: } +@pytest.mark.parametrize( + "descriptions,expected", + [ + (("Shared", "Shared"), "Shared"), + ((None, "Shared"), "Shared"), + (("Shared", None), "Shared"), + ((None, None), None), + ], +) +def test_complete_refresh_combines_entity_descriptions(monkeypatch, descriptions, expected): + model, locator = _multi_contract_model() + model.modelEntities[locator].entity.description = "Old description" + plugin = MultiContractPlugin() + get_metadata = plugin.get_table_metadata + + def metadata_with_description(location): + metadata = get_metadata(location) + metadata.source_object.description = descriptions[location == "contract_b"] + return metadata + + monkeypatch.setattr(plugin, "get_table_metadata", metadata_with_description) + monkeypatch.setattr(factory, "get_plugin_for_data_source", lambda *_args, **_kwargs: plugin) + + wrapper, _diff = source.compare_entity_with_source(locator, model=model) + + assert wrapper.entity.description == expected + + +def test_complete_refresh_rejects_conflicting_entity_descriptions(monkeypatch) -> None: + model, locator = _multi_contract_model() + original = model.modelEntities[locator].entity.model_copy(deep=True) + plugin = MultiContractPlugin() + get_metadata = plugin.get_table_metadata + + def metadata_with_description(location): + metadata = get_metadata(location) + metadata.source_object.description = location + return metadata + + monkeypatch.setattr(plugin, "get_table_metadata", metadata_with_description) + monkeypatch.setattr(factory, "get_plugin_for_data_source", lambda *_args, **_kwargs: plugin) + + with pytest.raises(ValueError, match="Conflicting entity descriptions"): + source.compare_entity_with_source(locator, model=model) + assert model.modelEntities[locator].entity == original + + def test_complete_refresh_rejects_conflicting_entity_properties(monkeypatch) -> None: model, locator = _multi_contract_model() plugin = MultiContractPlugin(conflicting=True) diff --git a/tests/test_022_builtin_integration.py b/tests/test_022_builtin_integration.py index 468f54f..bac6860 100644 --- a/tests/test_022_builtin_integration.py +++ b/tests/test_022_builtin_integration.py @@ -121,11 +121,13 @@ def test_odcs_api_import_save_reload_and_refresh(tmp_path, builtin_environment): data["schema"][0]["properties"].append( {"name": "added", "businessName": "Neu", "logicalType": "string"} ) + data["schema"][0]["description"] = "Updated customer master" write_contract(tmp_path / "contract.yaml", data) refreshed = client.get( "/sources/compare", params={"locator": "modelEntities/Stage/Customers"} ) assert refreshed.status_code == 200, refreshed.text + assert refreshed.json()["wrapper"]["entity"]["description"] == "Updated customer master" assert refreshed.json()["wrapper"]["entity"]["attributes"][-1]["displayName"] == "Neu" sources_config["dataSources"][0]["dataTypeMapping"][0]["targetType"] = "int" sources_file.write_text(json.dumps(sources_config), encoding="utf-8") @@ -142,6 +144,7 @@ def test_odcs_api_import_save_reload_and_refresh(tmp_path, builtin_environment): params={"locator": "modelEntities/Stage/Customers", "mode": "sources-only"}, ) assert sources_only.status_code == 200, sources_only.text + assert sources_only.json()["wrapper"]["entity"]["description"] == "Customer master" assert ( sources_only.json()["wrapper"]["entity"]["attributes"][0]["dataType"]["type"] == "long" ) @@ -159,6 +162,16 @@ def test_odcs_api_import_save_reload_and_refresh(tmp_path, builtin_environment): "/sources/compare", params={"locator": "modelEntities/Stage/Customers"} ) assert unchanged.status_code == 200 and not unchanged.json()["has_changes"] + saved = json.loads((tmp_path / "Model/Stage/Customers.json").read_text()) + assert saved["description"] == "Updated customer master" + del data["schema"][0]["description"] + write_contract(tmp_path / "contract.yaml", data) + cleared = client.get( + "/sources/compare", params={"locator": "modelEntities/Stage/Customers"} + ) + assert cleared.status_code == 200, cleared.text + assert cleared.json()["has_changes"] + assert cleared.json()["wrapper"]["entity"]["description"] is None def test_odcs_directory_api_navigation_and_legacy_csv_description(tmp_path, builtin_environment): From 763eaeb870b46ba7e465f2f7abc63e0b13d817b2 Mon Sep 17 00:00:00 2001 From: Fabio Kayser Date: Thu, 29 Oct 2026 10:04:55 +0100 Subject: [PATCH 4/9] chore: prepare 2.1.1 patch release --- docs/index.md | 2 ++ docs/releases/2.1.1.md | 33 +++++++++++++++++++++++++++++++++ pyproject.toml | 2 +- uv.lock | 2 +- 4 files changed, 37 insertions(+), 2 deletions(-) create mode 100644 docs/releases/2.1.1.md diff --git a/docs/index.md b/docs/index.md index 4f2923c..3ebfcac 100644 --- a/docs/index.md +++ b/docs/index.md @@ -1,5 +1,7 @@ # Backend Documentation +- [CLI 2.1.1 release notes](releases/2.1.1.md) +- [CLI 2.1.0 release notes](releases/2.1.0.md) - [Generator 2.0.1 release notes](releases/2.0.1.md) - [CLI usage and output checks](cli.md) - [Canonical HTTP contract](backend-contract.md) diff --git a/docs/releases/2.1.1.md b/docs/releases/2.1.1.md new file mode 100644 index 0000000..9474a1f --- /dev/null +++ b/docs/releases/2.1.1.md @@ -0,0 +1,33 @@ +# DataM8 CLI 2.1.1 + +This patch release fixes Unity Catalog imports and entity description refresh. + +## Source import and refresh + +- The builtin Unity Catalog connector supplies complete backend import descriptions + through `get_sources()`. Imports use the quoted SQL table location for both data + reads and metadata lookup, without persisting a redundant `metadataLocation`. +- Complete refresh updates the entity description, including clearing a removed + description. Sources-only refresh and entities without external sources retain + their existing description. Multiple sources may supply the same description; + conflicting descriptions are rejected instead of selecting one arbitrarily. + +## Other changes since 2.1.0 + +- Include the dependency and schema security corrections already merged into main, + and avoid logging secret references and registry contents. +- Clarify installation from prebuilt release packages. + +## Compatibility and release + +The solution schema version remains **2.0.0**. Existing plugin hooks remain valid; +the optional `metadataLocation` field and `get_sources()` contract are unchanged. +Older clients may continue to consume `sourceOverride` compatibility hints. +The frontend needs the updated backend package or submodule revision for these fixes. + +After merging the release PR into main, push the protected tag `v2.1.1` at the merged +commit. The release workflow verifies the version, runs CI and publishes the tested +wheel and source archive to GitHub Releases. It does not publish to PyPI. + +See the [backend contract](../backend-contract.md), +[connector contract](../connectors.md) and [release process](../ci.md). diff --git a/pyproject.toml b/pyproject.toml index 50515bd..3936454 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -1,6 +1,6 @@ [project] name = "datam8" -version = "2.1.0" +version = "2.1.1" description = "ORAYLIS DataM8 CLI" license = "GPL-3.0" requires-python = ">=3.12" diff --git a/uv.lock b/uv.lock index 4580c25..3c13fe1 100644 --- a/uv.lock +++ b/uv.lock @@ -553,7 +553,7 @@ wheels = [ [[package]] name = "datam8" -version = "2.1.0" +version = "2.1.1" source = { editable = "." } dependencies = [ { name = "deepdiff" }, From 97ba862c1b66c2ffba536f3915b8d53bc4e1c74e Mon Sep 17 00:00:00 2001 From: Fabio Kayser Date: Thu, 29 Oct 2026 10:26:53 +0100 Subject: [PATCH 5/9] refactor: limit release to entity description refresh fix --- docs/backend-contract.md | 5 --- docs/connectors.md | 7 --- docs/releases/2.1.1.md | 17 +++----- src/datam8/plugins/builtins/databricks_uc.py | 21 --------- tests/test_015_source_mappings.py | 45 -------------------- tests/test_021_unity_catalog.py | 18 -------- tests/test_022_builtin_integration.py | 26 +---------- 7 files changed, 7 insertions(+), 132 deletions(-) diff --git a/docs/backend-contract.md b/docs/backend-contract.md index b2235cf..384463b 100644 --- a/docs/backend-contract.md +++ b/docs/backend-contract.md @@ -163,11 +163,6 @@ editor. Runtime configuration changes and process failure may require a restart. types, business names and the technical object name as a suggested entity name. The selected target locator still determines the imported entity name. Plugins inheriting `get_sources() -> None` keep their existing import path. -- Unity Catalog also supplies `get_sources()`: `sourceLocation` uses quoted SQL - identifiers (for example, `` `main`.`sales`.`Customers` ``) for both data reads - and metadata lookup. Its definitions set `metadataLocation` to null, so imports - omit the redundant field. CLI/API imports and refresh use these definitions; - `sourceOverride` remains a compatibility hint for older clients. - Source metadata retains its existing schema: `dataType` is the source type. ODCS uses `physicalType`, or the declared `logicalType` when no physical type exists. Business names enrich existing model fields; no metadata hook overrides target types. diff --git a/docs/connectors.md b/docs/connectors.md index f712a4e..3d280de 100644 --- a/docs/connectors.md +++ b/docs/connectors.md @@ -137,9 +137,6 @@ row order rather than sorting it. `SourceObject` supplies `name`, `type`, optional `schema`, `description` and `properties`. Its existing `sourceOverride` is exposed in metadata responses for compatible clients; it is not a replacement for `get_sources()` in backend imports. -New plugins should use `get_sources()` for physical locations and mappings rather -than introduce another `sourceOverride` path. No `sourceOverride` is needed to -separate metadata and data read locations. Use the dataframe spelling `numericScale`. The generated `SourceField` and metadata API currently use the legacy spelling `numbericScale`; `TableMetadata` @@ -194,7 +191,6 @@ def get_sources(self, source_location): return [ { "sourceLocation": "raw.customers", - "metadataLocation": "contracts/customers#Customers", "sourceName": "customer_id", "targetName": "id", "sourceAlias": "crm", @@ -215,9 +211,6 @@ dictionary, including both `type` and `nullable`; use the physical source type. `metadataLocation` is the handle to describe on subsequent refreshes; `sourceLocation` is the physical read location. A per-row override can point to another metadata object, and an explicit null opts out of a separate handle. -Omitting `metadataLocation` retains the selected import handle automatically, even -when it differs from the physical `sourceLocation`. `SourceObject.name` remains -the suggested entity name and does not determine either location. | Result | Meaning | | --- | --- | diff --git a/docs/releases/2.1.1.md b/docs/releases/2.1.1.md index 9474a1f..9c6b7b7 100644 --- a/docs/releases/2.1.1.md +++ b/docs/releases/2.1.1.md @@ -1,12 +1,9 @@ # DataM8 CLI 2.1.1 -This patch release fixes Unity Catalog imports and entity description refresh. +This patch release fixes entity description refresh. -## Source import and refresh +## Entity description refresh -- The builtin Unity Catalog connector supplies complete backend import descriptions - through `get_sources()`. Imports use the quoted SQL table location for both data - reads and metadata lookup, without persisting a redundant `metadataLocation`. - Complete refresh updates the entity description, including clearing a removed description. Sources-only refresh and entities without external sources retain their existing description. Multiple sources may supply the same description; @@ -20,14 +17,12 @@ This patch release fixes Unity Catalog imports and entity description refresh. ## Compatibility and release -The solution schema version remains **2.0.0**. Existing plugin hooks remain valid; -the optional `metadataLocation` field and `get_sources()` contract are unchanged. -Older clients may continue to consume `sourceOverride` compatibility hints. -The frontend needs the updated backend package or submodule revision for these fixes. +The solution schema version remains **2.0.0**. Existing plugin hooks and source +location handling are unchanged. Use the updated backend package or submodule +revision to receive the description-refresh fix. After merging the release PR into main, push the protected tag `v2.1.1` at the merged commit. The release workflow verifies the version, runs CI and publishes the tested wheel and source archive to GitHub Releases. It does not publish to PyPI. -See the [backend contract](../backend-contract.md), -[connector contract](../connectors.md) and [release process](../ci.md). +See the [backend contract](../backend-contract.md) and [release process](../ci.md). diff --git a/src/datam8/plugins/builtins/databricks_uc.py b/src/datam8/plugins/builtins/databricks_uc.py index 8feacec..7b4e400 100644 --- a/src/datam8/plugins/builtins/databricks_uc.py +++ b/src/datam8/plugins/builtins/databricks_uc.py @@ -647,27 +647,6 @@ def get_table_metadata(self, source_location: str, /) -> TableMetadata: ), ) - def get_sources(self, source_location: str, /) -> list[dict[str, Any]]: - """Use the quoted table location for both metadata and data reads.""" - physical_location = _quoted_name(self._location(source_location, table=True)) - metadata = self.get_table_metadata(source_location) - return [ - { - "sourceLocation": physical_location, - "metadataLocation": None, - "sourceName": field.name, - "targetName": field.name, - "sourceDataType": { - "type": field.dataType, - "nullable": field.isNullable, - "charLen": field.maxLength, - "precision": field.numericPrecision, - "scale": field.numbericScale, - }, - } - for field in metadata.iter_source_fields() - ] - def get_source_capabilities(self) -> dict[str, bool]: capabilities = super().get_source_capabilities() capabilities["previewData"] = bool(_warehouse_id(self.extended_properties)) diff --git a/tests/test_015_source_mappings.py b/tests/test_015_source_mappings.py index e0d9232..439e0ca 100644 --- a/tests/test_015_source_mappings.py +++ b/tests/test_015_source_mappings.py @@ -278,51 +278,6 @@ def test_read_from_data_source_uses_plugin_sources_and_import_location(monkeypat assert entity.sources[1].mapping[0].targetName == "firstName" -@pytest.mark.parametrize("mode", ["complete", "sources-only"]) -def test_plugin_sets_independent_metadata_location_without_source_override(monkeypatch, mode): - calls = [] - - class SeparateLocationsPlugin(SourcePluginStub): - def get_sources(self, location): - calls.append(location) - return [ - { - "sourceLocation": "raw.customers", - "metadataLocation": "contracts/customers#Customers", - "sourceName": name, - "targetName": name, - } - for name in ("id", "firstName") - ] - - def get_table_metadata(self, location): - metadata = super().get_table_metadata(location) - assert metadata.source_object.sourceOverride is None - return metadata - - plugin = SeparateLocationsPlugin() - monkeypatch.setattr(factory, "get_plugin_for_data_source", lambda *_args, **_kwargs: plugin) - model, locator, _ = _source_model() - model.attributeTypes = AttributeTypesStub() - entity = source.read_from_data_source("test", "browser-selection", model=model) - # Exercise serialization as well as repeated refresh from the persisted metadata handle. - model.modelEntities[locator].entity = m.ModelEntity.model_validate_json( - entity.model_dump_json() - ) - - for _ in range(2): - refreshed, diff = source.compare_entity_with_source(locator, model=model, mode=mode) - assert not diff - assert refreshed.entity.sources[0].sourceLocation == "raw.customers" - assert refreshed.entity.sources[0].metadataLocation == "contracts/customers#Customers" - model.modelEntities[locator].entity = refreshed.entity - assert calls == [ - "browser-selection", - "contracts/customers#Customers", - "contracts/customers#Customers", - ] - - def test_import_description_exposes_plugin_entity(monkeypatch) -> None: model = configure_mappings(SimpleNamespace(attributeTypes=AttributeTypesStub())) monkeypatch.setattr(factory, "get_model", lambda: model) diff --git a/tests/test_021_unity_catalog.py b/tests/test_021_unity_catalog.py index a831c56..2189883 100644 --- a/tests/test_021_unity_catalog.py +++ b/tests/test_021_unity_catalog.py @@ -91,24 +91,6 @@ def test_locations_round_trip(location, make_plugin): assert list(plugin.get_table_metadata(location).iter_source_fields())[0].name == "id" -@pytest.mark.parametrize( - "location", - ["main.sales.Customers", "[main.sales].[Customers]", "`main`.`sales`.`Customers`"], -) -def test_source_definitions_use_one_location_for_metadata_and_data(location, make_plugin): - definitions = make_plugin().get_sources(location) - - assert {row["metadataLocation"] for row in definitions} == {None} - assert {row["sourceLocation"] for row in definitions} == {"`main`.`sales`.`Customers`"} - assert [(row["sourceName"], row["targetName"]) for row in definitions] == [ - (name, name) for name in ("id", "name", "amount", "active", "details") - ] - assert definitions[1]["sourceDataType"]["charLen"] == 80 - assert definitions[2]["sourceDataType"]["precision"] == 10 - assert definitions[2]["sourceDataType"]["scale"] == 0 - assert definitions[4]["sourceDataType"]["type"] == "struct" - - @pytest.mark.parametrize( "location", [ diff --git a/tests/test_022_builtin_integration.py b/tests/test_022_builtin_integration.py index bac6860..3e56247 100644 --- a/tests/test_022_builtin_integration.py +++ b/tests/test_022_builtin_integration.py @@ -236,15 +236,6 @@ def test_uc_builtin_api_import_save_reload_refresh( factory._model = factory.create_model() with TestClient(create_app()) as client: assert client.get("/sources/UC/test").status_code == 200 - description = client.get( - "/sources/UC/locations/import-description", - params={"source_location": "[main.sales].[Customers]"}, - ) - assert description.status_code == 200, description.text - described = description.json()["entity"] - assert described["name"] == "Customers" - assert described["sources"][0]["sourceLocation"] == "`main`.`sales`.`Customers`" - assert described["sources"][0]["metadataLocation"] is None response = client.put( "/sources/UC/import", json={ @@ -254,8 +245,6 @@ def test_uc_builtin_api_import_save_reload_refresh( ) assert response.status_code == 200, response.text saved = json.loads((tmp_path / "Model/Stage/Customers.json").read_text()) - assert saved["sources"][0]["sourceLocation"] == "`main`.`sales`.`Customers`" - assert "metadataLocation" not in saved["sources"][0] assert [a["dataType"]["type"] for a in saved["attributes"]] == [ "long", "string", @@ -266,16 +255,6 @@ def test_uc_builtin_api_import_save_reload_refresh( assert saved["attributes"][2]["dataType"]["scale"] == 0 assert saved["sources"][0]["mapping"][4]["sourceDataType"]["type"] == "struct" assert client.post("/model/reload", params={"force": True}).status_code == 200 - for refresh_mode in ("complete", "sources-only"): - unchanged = client.get( - "/sources/compare", - params={"locator": "modelEntities/Stage/Customers", "mode": refresh_mode}, - ) - assert unchanged.status_code == 200, unchanged.text - assert not unchanged.json()["has_changes"] - refreshed_source = unchanged.json()["wrapper"]["entity"]["sources"][0] - assert refreshed_source["sourceLocation"] == "`main`.`sales`.`Customers`" - assert refreshed_source["metadataLocation"] is None assert ( client.get( "/sources/UC/locations/preview", @@ -321,10 +300,7 @@ def test_cli_navigation_and_import(cls, tmp_path, builtin_plugin, make_plugin): ): result = runner.invoke(app, [*args, "-s", str(path)]) assert result.exit_code == 0, f"{result.output}\n{result.exception}" - saved = json.loads((tmp_path / "Model/Stage/Imported.json").read_text()) - if cls is DatabricksUnityCatalog: - assert saved["sources"][0]["sourceLocation"] == "`main`.`sales`.`Customers`" - assert "metadataLocation" not in saved["sources"][0] + assert (tmp_path / "Model/Stage/Imported.json").exists() def test_optional_metadata_preserves_existing_plugin_imports( From 0c8e74aec89472eece75883c4d3f3754f3f24a54 Mon Sep 17 00:00:00 2001 From: Fabio Kayser Date: Thu, 5 Nov 2026 22:44:42 +0100 Subject: [PATCH 6/9] feat: assign random safe integer IDs to model entities --- .../workflows/reusable-lint-build-test.yml | 3 + docs/backend-contract.md | 14 ++ src/datam8/model/entity_wrapper.py | 8 + src/datam8/model/ids.py | 16 ++ src/datam8/model/model.py | 40 +++- tests/test_016_entity_tree_operations.py | 5 +- tests/test_025_model_entity_ids.py | 199 ++++++++++++++++++ 7 files changed, 274 insertions(+), 11 deletions(-) create mode 100644 src/datam8/model/ids.py create mode 100644 tests/test_025_model_entity_ids.py diff --git a/.github/workflows/reusable-lint-build-test.yml b/.github/workflows/reusable-lint-build-test.yml index 81853a2..a69ee97 100644 --- a/.github/workflows/reusable-lint-build-test.yml +++ b/.github/workflows/reusable-lint-build-test.yml @@ -126,6 +126,9 @@ jobs: FORCE_COLOR: 0 TERM: dumb + - name: Validate solution IDs and model + run: uv run --frozen datam8 validate --solution datam8-sample-solution/ORAYLISDatabricksSample.dm8s --json + - name: Upload Test Artifacts if: always() uses: actions/upload-artifact@cf430e030ddbb5b0abf93d22962f4752f3646cd9 # v7 diff --git a/docs/backend-contract.md b/docs/backend-contract.md index 63ccac5..dcf6af4 100644 --- a/docs/backend-contract.md +++ b/docs/backend-contract.md @@ -91,6 +91,20 @@ editor. Runtime configuration changes and process failure may require a restart. ## Entity and function invariants +- Model entity IDs are assigned by the backend on create, clone and source import, + uniformly in `1..9007199254740991`, independently of the solution's highest ID. + Create ignores a supplied ID; the returned wrapper's `entity.id` is authoritative. + Updates may include the unchanged ID but cannot change it. Rename/move preserve it. + Loading (including lazy load/reload) and saving reject duplicate IDs among live + model entities and IDs outside that range. Duplicate errors identify the ID and + affected locators. Existing valid IDs are retained. Solution repositories must + run `datam8 validate --solution --json` as a required merge check + on their merged solution state; independent branches cannot reserve each other's IDs. +- Wizard imports create without saving, collect the assigned IDs, resolve internal + relationships, then patch and save. On failure, consumers delete only entities + successfully created by that attempt and save those deletions by locator. An + unsaved deletion is discarded without creating a file. This uses the existing + persistence model; saves across multiple files are not a filesystem transaction. - `PUT /entities/{locator}` accepts an optional `sourceFile` query parameter for Attribute Types, Data Types, Data Sources, Data Source Types, Data Products, Zones, Properties and Property Values. It selects the existing collection JSON diff --git a/src/datam8/model/entity_wrapper.py b/src/datam8/model/entity_wrapper.py index 6007a67..d40e27d 100644 --- a/src/datam8/model/entity_wrapper.py +++ b/src/datam8/model/entity_wrapper.py @@ -37,6 +37,7 @@ from datam8_model import property as p from datam8_model import zone as z +from .ids import validate_model_entity_id from .locator import Locator, LocatorOrString, _ensure_locator logger = logging.getLogger(__name__) @@ -458,6 +459,13 @@ def has_property(self, property_name: str, /) -> bool: return False def update(self, **kwargs: Any) -> None: + if isinstance(self.entity, m.ModelEntity) and "id" in kwargs: + validate_model_entity_id(kwargs["id"], locator=str(self.locator)) + if kwargs["id"] != self.entity.id: + raise errors.Datam8ValidationError( + message=f"Model entity ID cannot be changed at {self.locator}", + details={"id": self.entity.id, "requestedId": kwargs["id"]}, + ) new_entity = self.entity.model_copy(update=kwargs, deep=True) try: diff --git a/src/datam8/model/ids.py b/src/datam8/model/ids.py new file mode 100644 index 0000000..e6d9eb8 --- /dev/null +++ b/src/datam8/model/ids.py @@ -0,0 +1,16 @@ +"""Model entity IDs shared by loading, creation and updates.""" + +from datam8.errors import Datam8ValidationError + +MAX_MODEL_ENTITY_ID = (1 << 53) - 1 + + +def validate_model_entity_id(value: object, *, locator: str) -> None: + if type(value) is not int or not 1 <= value <= MAX_MODEL_ENTITY_ID: + raise Datam8ValidationError( + message=( + f"Invalid model entity ID {value!r} at {locator}; " + f"expected an integer in 1..{MAX_MODEL_ENTITY_ID}" + ), + details={"id": value, "locator": locator}, + ) diff --git a/src/datam8/model/model.py b/src/datam8/model/model.py index dae45e8..72e2873 100644 --- a/src/datam8/model/model.py +++ b/src/datam8/model/model.py @@ -18,6 +18,7 @@ from __future__ import annotations +import secrets from collections.abc import Iterator, Sequence from pathlib import Path from threading import Lock @@ -39,6 +40,7 @@ from . import entity_wrapper as ew from .entity_wrapper import EntityRepository, EntityWrapper, EntityWrapperVariant +from .ids import MAX_MODEL_ENTITY_ID, validate_model_entity_id from .locator import ROOT_LOCATOR, Locator, LocatorOrString, _ensure_locator logger = logging.getLogger(__name__) @@ -146,21 +148,37 @@ def __init__(self, solution: s.Solution, /, **kwargs: EntityRepository[b.BaseEnt self._model_files: dict[Path, EntityFileRef] = {} """Internal dictionary to allow easy mapping of files and their entities""" - if len(self.modelEntities) == 0: - self.__next_model_id = 1 - else: - self.__next_model_id = ( - max(wrapper.entity.id for wrapper in self.modelEntities.values()) + 1 - ) + self.validate_model_entity_ids() + self._used_model_ids = {wrapper.entity.id for wrapper in self.modelEntities.values()} def __getitem__(self, type_: str) -> EntityRepository[b.BaseEntityType]: return getattr(self, type_) def get_next_model_id(self) -> int: with self.lock: - next = self.__next_model_id - self.__next_model_id += 1 - return next + while True: + entity_id = secrets.randbelow(MAX_MODEL_ENTITY_ID) + 1 + if entity_id not in self._used_model_ids: + self._used_model_ids.add(entity_id) + return entity_id + + def validate_model_entity_ids(self) -> None: + locators_by_id: dict[int, list[str]] = {} + for wrapper in self.modelEntities.values(): + if wrapper.is_deleted: + continue + entity_id = wrapper.entity.id + locator = str(wrapper.locator) + validate_model_entity_id(entity_id, locator=locator) + locators_by_id.setdefault(entity_id, []).append(locator) + + duplicates = {key: value for key, value in locators_by_id.items() if len(value) > 1} + if duplicates: + raise errors.Datam8ValidationError( + message="Duplicate model entity IDs: " + + "; ".join(f"{key}: {', '.join(value)}" for key, value in duplicates.items()), + details={"duplicates": duplicates}, + ) def init_file_references(self) -> None: self._model_files = {} @@ -897,6 +915,7 @@ def save(self, locator: str | None = None, /) -> None: """ Saves the current state of the model or a specific entity to the corresponding json file. """ + self.validate_model_entity_ids() changed_wrappers = [ w for w in self.get_entities(locator or ROOT_LOCATOR) @@ -977,6 +996,9 @@ def save(self, locator: str | None = None, /) -> None: for wrapper in file_to_wrappers[_file]["deleted"]: del self[wrapper.locator.entityType][wrapper.locator] + file_ref = self._model_files[_file] + if wrapper.locator in file_ref.locators: + file_ref.locators.remove(wrapper.locator) # remove file refs if there are no more locators associated with them self.cleanup_entity_file_references() diff --git a/tests/test_016_entity_tree_operations.py b/tests/test_016_entity_tree_operations.py index 277f12e..649a101 100644 --- a/tests/test_016_entity_tree_operations.py +++ b/tests/test_016_entity_tree_operations.py @@ -57,7 +57,7 @@ def test_move_folder_rebases_complete_subtree_and_metadata(model: Model) -> None def test_add_model_entity_ignores_client_id_and_uses_dedicated_file(model: Model) -> None: existing = next(iter(model.modelEntities.values())) - existing_max = max(wrapper.entity.id for wrapper in model.modelEntities.values()) + existing_ids = {wrapper.entity.id for wrapper in model.modelEntities.values()} payload = existing.entity.model_copy().model_dump(mode="json") payload["id"] = 1 payload["name"] = "ClientProvidedName" @@ -68,7 +68,8 @@ def test_add_model_entity_ignores_client_id_and_uses_dedicated_file(model: Model payload, ) - assert created.entity.id == existing_max + 1 + assert 1 <= created.entity.id <= (1 << 53) - 1 + assert created.entity.id not in existing_ids assert created.entity.name == "CreatedEntity" assert created.source_file == ( model.get_base_path_for_entity_type(EntityType.MODEL_ENTITIES) diff --git a/tests/test_025_model_entity_ids.py b/tests/test_025_model_entity_ids.py new file mode 100644 index 0000000..6fcec04 --- /dev/null +++ b/tests/test_025_model_entity_ids.py @@ -0,0 +1,199 @@ +"""ID allocation, validation and persistence using disposable solutions.""" + +import asyncio +import json +from concurrent.futures import ThreadPoolExecutor + +import pytest +from fastapi.testclient import TestClient +from typer.testing import CliRunner + +from datam8 import config, factory, solution +from datam8.api.app import create_app +from datam8.app import app +from datam8.errors import Datam8ValidationError +from datam8.model import Locator +from datam8.model.ids import MAX_MODEL_ENTITY_ID + + +def entity_data(name="Existing", entity_id=7): + return { + "id": entity_id, + "name": name, + "attributes": [ + { + "name": "Id", + "ordinalNumber": 1, + "attributeType": "Regular", + "dataType": {"type": "int", "nullable": False}, + "dateAdded": "2026-01-01T00:00:00Z", + } + ], + "sources": [], + "relationships": [], + "transformations": [], + } + + +@pytest.fixture +def id_workspace(tmp_path, monkeypatch): + path = tmp_path / "IDs.dm8s" + solution.init_solution(path) + (tmp_path / "base/Zones.json").write_text( + json.dumps( + {"type": "zones", "zones": [{"name": "Raw", "targetName": "raw", "displayName": "Raw"}]} + ), + encoding="utf-8", + ) + folder = tmp_path / "model/Raw" + folder.mkdir(parents=True) + (folder / "Existing.json").write_text(json.dumps(entity_data()), encoding="utf-8") + monkeypatch.setattr(config, "solution_path", path, raising=False) + monkeypatch.setattr(config, "solution_folder_path", tmp_path, raising=False) + monkeypatch.setattr(config, "lazy", True) + monkeypatch.setattr(config, "mode", config.RunMode.TEST) + monkeypatch.setattr(factory, "_model", None) + return path + + +def test_allocation_retries_loaded_and_reserved_ids(id_workspace, monkeypatch): + model = factory.get_model() + draws = iter([6, MAX_MODEL_ENTITY_ID - 1, MAX_MODEL_ENTITY_ID - 1, 0]) + monkeypatch.setattr("datam8.model.model.secrets.randbelow", lambda _: next(draws)) + assert model.get_next_model_id() == MAX_MODEL_ENTITY_ID + assert model.get_next_model_id() == 1 + + +def test_parallel_allocation_reserves_unique_safe_ids(id_workspace): + model = factory.get_model() + with ThreadPoolExecutor(max_workers=8) as executor: + ids = list(executor.map(lambda _: model.get_next_model_id(), range(500))) + assert len(set(ids)) == 500 + assert 7 not in ids + assert all(type(value) is int and 1 <= value <= MAX_MODEL_ENTITY_ID for value in ids) + + +def test_create_clone_update_move_and_reload_keep_actual_ids(id_workspace, monkeypatch): + factory.get_model() + draws = iter([MAX_MODEL_ENTITY_ID - 1, 0]) + monkeypatch.setattr("datam8.model.model.secrets.randbelow", lambda _: next(draws)) + with TestClient(create_app()) as client: + body = entity_data("New") + body.pop("id") + created = client.put("/entities/modelEntities/Raw/New", json=body) + assert created.status_code == 200, created.text + assert created.json()["item"]["entity"]["id"] == MAX_MODEL_ENTITY_ID + cloned = client.put( + "/entities/clone", + json={"locator": "modelEntities/Raw/New", "newLocator": "modelEntities/Raw/Copy"}, + ) + assert cloned.status_code == 200, cloned.text + assert cloned.json()["items"][0]["entity"]["id"] == 1 + updated = client.patch( + "/entities/modelEntities/Raw/New", + json={"id": MAX_MODEL_ENTITY_ID, "description": "Changed"}, + ) + assert updated.status_code == 200, updated.text + rejected = client.patch("/entities/modelEntities/Raw/New", json={"id": 7}) + assert rejected.status_code == 400 + assert rejected.json()["code"] == "validation_error" + moved = client.post( + "/entities/move-single", + json={"from": "modelEntities/Raw/New", "to": "modelEntities/Raw/Renamed"}, + ) + assert moved.status_code == 200, moved.text + assert moved.json()["item"]["entity"]["id"] == MAX_MODEL_ENTITY_ID + assert client.post("/model/save").status_code == 204 + assert client.post("/model/reload").status_code == 200 + model = factory.get_model() + assert model.modelEntities.get("Raw/Renamed").entity.id == MAX_MODEL_ENTITY_ID + assert model.modelEntities.get("Raw/Copy").entity.id == 1 + saved = json.loads((id_workspace.parent / "model/Raw/Renamed.json").read_text()) + assert saved["id"] == MAX_MODEL_ENTITY_ID + + +@pytest.mark.parametrize("entity_id", [0, -1, MAX_MODEL_ENTITY_ID + 1]) +def test_cli_validation_rejects_invalid_ids(id_workspace, entity_id): + file = id_workspace.parent / "model/Raw/Invalid.json" + file.write_text(json.dumps(entity_data("Invalid", entity_id)), encoding="utf-8") + result = CliRunner().invoke(app, ["validate", "-s", str(id_workspace), "--json"]) + assert result.exit_code == 2, result.output + assert json.loads(result.stdout)["error"]["code"] == "validation_error" + + +def test_duplicate_ids_fail_loading_and_cli_with_locators(id_workspace): + file = id_workspace.parent / "model/Raw/Duplicate.json" + file.write_text(json.dumps(entity_data("Duplicate")), encoding="utf-8") + with pytest.raises(Datam8ValidationError, match="Duplicate model entity IDs") as caught: + asyncio.run(factory.load_model(id_workspace)) + assert set(caught.value.details["duplicates"][7]) == { + "modelEntities/Raw/Existing", + "modelEntities/Raw/Duplicate", + } + result = CliRunner().invoke(app, ["validate", "-s", str(id_workspace), "--json"]) + assert result.exit_code == 2, result.output + message = json.loads(result.stdout)["error"]["message"] + assert "7" in message and "Existing" in message and "Duplicate" in message + + +def test_valid_mixed_ids_pass_cli_validation(id_workspace): + file = id_workspace.parent / "model/Raw/New.json" + file.write_text(json.dumps(entity_data("New", MAX_MODEL_ENTITY_ID)), encoding="utf-8") + result = CliRunner().invoke(app, ["validate", "-s", str(id_workspace), "--json"]) + assert result.exit_code == 0, result.output + assert json.loads(result.stdout)["item"]["valid"] + + +def test_source_import_uses_backend_id(tmp_path, builtin_environment, monkeypatch): + from builtin_plugins.solution import create_solution + from test_019_odcs import write_contract + from test_019_odcs_cases import contract + + from datam8.plugins.builtins.odcs import OdcsFile + + write_contract(tmp_path / "contract.json", contract()) + path = create_solution(tmp_path, [(OdcsFile, "Contracts", {"path": "contract.json"})]) + config.set_solution(path) + factory.get_model() + monkeypatch.setattr("datam8.model.model.secrets.randbelow", lambda _: MAX_MODEL_ENTITY_ID - 1) + with TestClient(create_app()) as client: + response = client.put( + "/sources/Contracts/import", + json={ + "locator": "modelEntities/Stage/Imported", + "sourceLocation": "contract.json#customers", + }, + ) + assert response.status_code == 200, response.text + model = asyncio.run(factory.load_model(path)) + assert model.modelEntities.get("Stage/Imported").entity.id == MAX_MODEL_ENTITY_ID + + +def test_save_rejects_duplicate_before_writing(id_workspace): + model = factory.get_model() + new = model.add_entity("modelEntities/Raw/New", entity_data()) + new.entity.id = 7 + with pytest.raises(Datam8ValidationError, match="Duplicate"): + model.save() + assert not new.source_file.exists() + + +@pytest.mark.parametrize("persist_first", [False, True]) +def test_rollback_deletes_only_created_entity(id_workspace, persist_first): + model = factory.get_model() + new = model.add_entity("modelEntities/Raw/New", entity_data()) + original = model.modelEntities.get("Raw/Existing") + original.update(description="Unsaved draft") + if persist_first: + model.save("modelEntities/Raw/New") + model.delete_entity(Locator.from_path("modelEntities/Raw/New")) + model.save("modelEntities/Raw/New") + assert not new.source_file.exists() + assert new.locator not in model.modelEntities + assert new.source_file not in model._model_files + assert original.has_changed + assert "description" not in json.loads(original.source_file.read_text()) + model.save() + reloaded = asyncio.run(factory.load_model(id_workspace)) + assert len(reloaded.modelEntities) == 1 + assert reloaded.modelEntities.get("Raw/Existing").entity.description == "Unsaved draft" From 8118f0fd4d06cf8875bf7417a74c2ebb0323cce7 Mon Sep 17 00:00:00 2001 From: Fabio Kayser Date: Fri, 6 Nov 2026 08:57:32 +0100 Subject: [PATCH 7/9] fix: create model entity batches within one backend request --- docs/backend-contract.md | 17 ++-- src/datam8/api/routes/entities.py | 19 +++++ src/datam8/model/model.py | 81 +++++++++++++++--- tests/test_013_utf8_persistence.py | 2 + tests/test_025_model_entity_ids.py | 133 ++++++++++++++++++----------- 5 files changed, 187 insertions(+), 65 deletions(-) diff --git a/docs/backend-contract.md b/docs/backend-contract.md index dcf6af4..daefc00 100644 --- a/docs/backend-contract.md +++ b/docs/backend-contract.md @@ -43,7 +43,7 @@ The current `create_app().openapi()` exposes these domains (no `/api` prefix): - `GET /health` (204), `GET /version` (`schemaVersion`, `appVersion`), `GET /config`. - `GET /solution`, `GET /solution/full`. -- `GET /entities`, `GET|PUT|PATCH|DELETE /entities/{locator}`, `PUT /entities/clone`, +- `GET /entities`, `GET|PUT|PATCH|DELETE /entities/{locator}`, `PUT /entities/clone`, `PUT /entities/batch`, `POST /entities/rename`, `POST /entities/move`, `POST /entities/move-single`. - `POST /model/save`, `POST /model/reload`, `GET /model/unsaved`. - `POST /model/generate`: optional body `{ "target": "...", "cleanOutput": true, @@ -100,11 +100,16 @@ editor. Runtime configuration changes and process failure may require a restart. affected locators. Existing valid IDs are retained. Solution repositories must run `datam8 validate --solution --json` as a required merge check on their merged solution state; independent branches cannot reserve each other's IDs. -- Wizard imports create without saving, collect the assigned IDs, resolve internal - relationships, then patch and save. On failure, consumers delete only entities - successfully created by that attempt and save those deletions by locator. An - unsaved deletion is discarded without creating a file. This uses the existing - persistence model; saves across multiple files are not a filesystem transaction. +- `PUT /entities/batch` creates and saves model entities in one request: + `{ "items": [{ "locator": "modelEntities/Raw/Customer", "content": { ... } }] }`. + Internal `sourceLocation` and `targetLocation` may contain model entity locators, + including forward/self references within the batch. The backend assigns IDs, + resolves these references and validates all items before saving only this batch. + Existing locators/files are rejected. The response's `items` contains the created + wrappers in request order. Failure removes this attempt's entities and new JSON + files; existing files, function sources and unrelated drafts are retained. A lost + success response leaves a complete saved batch; reload before retrying. This is + not a transaction against process crashes or persistent filesystem failures. - `PUT /entities/{locator}` accepts an optional `sourceFile` query parameter for Attribute Types, Data Types, Data Sources, Data Source Types, Data Products, Zones, Properties and Property Values. It selects the existing collection JSON diff --git a/src/datam8/api/routes/entities.py b/src/datam8/api/routes/entities.py index 5fd6eea..b135dfe 100644 --- a/src/datam8/api/routes/entities.py +++ b/src/datam8/api/routes/entities.py @@ -72,6 +72,25 @@ async def clone_entity( return MultiItemResponse.from_list([entity]) +class BatchEntityBody(BaseModel): + locator: str + content: dict[str, Any] + + +class BatchCreateBody(BaseModel): + items: Annotated[list[BatchEntityBody], Field(min_length=1)] + + +@entities_router.put("/batch") +async def create_model_entities( + body: BatchCreateBody, +) -> MultiItemResponse[model.EntityWrapper[b.BaseEntityType]]: + created = factory.get_model().add_model_entities( + [(item.locator, item.content) for item in body.items] + ) + return MultiItemResponse.from_list(created) + + @entities_router.put("/{locator:path}") async def create_entity( body: dict[str, Any], diff --git a/src/datam8/model/model.py b/src/datam8/model/model.py index 72e2873..e5f86bb 100644 --- a/src/datam8/model/model.py +++ b/src/datam8/model/model.py @@ -20,6 +20,7 @@ import secrets from collections.abc import Iterator, Sequence +from copy import deepcopy from pathlib import Path from threading import Lock from typing import Annotated, Any, overload @@ -509,6 +510,68 @@ def add_entity[T: b.BaseEntityType]( return new_wrapper + def add_model_entities( + self, items: Sequence[tuple[str, dict[str, Any]]] + ) -> list[EntityWrapper[b.BaseEntityType]]: + """Create a complete batch, retaining existing files and unrelated drafts on failure.""" + created: list[EntityWrapper[b.BaseEntityType]] = [] + paths: set[Path] = set() + root = self.get_base_path_for_entity_type(b.EntityType.MODEL_ENTITIES).resolve() + try: + for locator, content in items: + loc = _ensure_locator(locator) + path = (root / Path(*loc.folders) / f"{loc.entityName}.json").resolve() + if ( + loc.entityType != b.EntityType.MODEL_ENTITIES.value + or loc.entityName is None + or not path.is_relative_to(root) + or loc in self.modelEntities + or path in paths + or path.exists() + ): + raise errors.Datam8ValidationError( + message=f"Invalid or existing batch target: {loc}" + ) + created.append( + self.add_entity(loc, {**content, "sources": [], "relationships": []}) + ) + paths.add(path) + + references = { + str(w.locator): w.entity.id for w in self.modelEntities.values() if not w.is_deleted + } + live_ids = set(references.values()) + for wrapper, (_, content) in zip(created, items, strict=True): + updates = {} + for field, location in ( + ("sources", "sourceLocation"), + ("relationships", "targetLocation"), + ): + rows = deepcopy(content.get(field, [])) + for row in rows if isinstance(rows, list) else []: + if isinstance(row, dict) and not row.get("dataSource"): + value = row.get(location) + if isinstance(value, str): + value = references.get(value.lstrip("/")) + if type(value) is not int or value not in live_ids: + raise errors.Datam8ValidationError( + message=f"Unknown model reference: {row.get(location)}" + ) + row[location] = value + updates[field] = rows + wrapper.entity = m.ModelEntity.from_dict({**wrapper.entity.model_dump(), **updates}) + self.save(locators=[str(wrapper.locator) for wrapper in created]) + except Exception as exc: + for wrapper in created: + self.modelEntities.remove(wrapper.locator) + self._model_files.pop(wrapper.source_file, None) + for path in paths: + path.unlink(missing_ok=True) + if isinstance(exc, ValidationError): + raise errors.Datam8ValidationError(message=str(exc)) from exc + raise + return created + def _existing_base_target(self, _type: b.EntityType, source_file: str | Path) -> Path: """Validate explicit collection placement before mutating model state.""" if _type in (b.EntityType.MODEL_ENTITIES, b.EntityType.FOLDERS, b.EntityType.DATA_MODULES): @@ -911,17 +974,18 @@ def get_child_locators(self, search_locator: str | Locator, /) -> list[Locator]: return found_locators - def save(self, locator: str | None = None, /) -> None: + def save(self, locator: str | None = None, /, *, locators: Sequence[str] | None = None) -> None: """ Saves the current state of the model or a specific entity to the corresponding json file. """ self.validate_model_entity_ids() - changed_wrappers = [ - w - for w in self.get_entities(locator or ROOT_LOCATOR) - if w.has_changed and not w.is_deleted - ] - deleted_wrappers = [w for w in self.get_entities(locator or ROOT_LOCATOR) if w.is_deleted] + selected = ( + self.get_entities(locator or ROOT_LOCATOR) + if locators is None + else [self.get_entity_by_locator(loc) for loc in locators] + ) + changed_wrappers = [w for w in selected if w.has_changed and not w.is_deleted] + deleted_wrappers = [w for w in selected if w.is_deleted] no_of_changes = len(changed_wrappers) no_of_deletions = len(deleted_wrappers) @@ -996,9 +1060,6 @@ def save(self, locator: str | None = None, /) -> None: for wrapper in file_to_wrappers[_file]["deleted"]: del self[wrapper.locator.entityType][wrapper.locator] - file_ref = self._model_files[_file] - if wrapper.locator in file_ref.locators: - file_ref.locators.remove(wrapper.locator) # remove file refs if there are no more locators associated with them self.cleanup_entity_file_references() diff --git a/tests/test_013_utf8_persistence.py b/tests/test_013_utf8_persistence.py index c445073..e78c0bb 100644 --- a/tests/test_013_utf8_persistence.py +++ b/tests/test_013_utf8_persistence.py @@ -98,6 +98,8 @@ def test_import_refresh_reload_and_output_preserve_utf8(tmp_path, builtin_enviro assert response.status_code == 200, response.text response = client.post("/model/save") assert response.status_code in (200, 204), response.text + saved_id = json.loads(entity_path.read_text(encoding="utf-8"))["id"] + assert 1 <= saved_id <= (1 << 53) - 1 assert ( json.loads(entity_path.read_bytes().decode("utf-8"))["attributes"][0]["description"] == DESCRIPTION diff --git a/tests/test_025_model_entity_ids.py b/tests/test_025_model_entity_ids.py index 6fcec04..47eef87 100644 --- a/tests/test_025_model_entity_ids.py +++ b/tests/test_025_model_entity_ids.py @@ -12,8 +12,8 @@ from datam8.api.app import create_app from datam8.app import app from datam8.errors import Datam8ValidationError -from datam8.model import Locator from datam8.model.ids import MAX_MODEL_ENTITY_ID +from datam8.model.model import EntityFileRef def entity_data(name="Existing", entity_id=7): @@ -112,13 +112,15 @@ def test_create_clone_update_move_and_reload_keep_actual_ids(id_workspace, monke assert saved["id"] == MAX_MODEL_ENTITY_ID -@pytest.mark.parametrize("entity_id", [0, -1, MAX_MODEL_ENTITY_ID + 1]) -def test_cli_validation_rejects_invalid_ids(id_workspace, entity_id): +@pytest.mark.parametrize( + "entity_id,exit_code", [(0, 2), (-1, 2), (MAX_MODEL_ENTITY_ID + 1, 2), (MAX_MODEL_ENTITY_ID, 0)] +) +def test_cli_validation_checks_id_range(id_workspace, entity_id, exit_code): file = id_workspace.parent / "model/Raw/Invalid.json" file.write_text(json.dumps(entity_data("Invalid", entity_id)), encoding="utf-8") result = CliRunner().invoke(app, ["validate", "-s", str(id_workspace), "--json"]) - assert result.exit_code == 2, result.output - assert json.loads(result.stdout)["error"]["code"] == "validation_error" + assert result.exit_code == exit_code, result.output + assert json.loads(result.stdout)["ok"] == (exit_code == 0) def test_duplicate_ids_fail_loading_and_cli_with_locators(id_workspace): @@ -136,39 +138,6 @@ def test_duplicate_ids_fail_loading_and_cli_with_locators(id_workspace): assert "7" in message and "Existing" in message and "Duplicate" in message -def test_valid_mixed_ids_pass_cli_validation(id_workspace): - file = id_workspace.parent / "model/Raw/New.json" - file.write_text(json.dumps(entity_data("New", MAX_MODEL_ENTITY_ID)), encoding="utf-8") - result = CliRunner().invoke(app, ["validate", "-s", str(id_workspace), "--json"]) - assert result.exit_code == 0, result.output - assert json.loads(result.stdout)["item"]["valid"] - - -def test_source_import_uses_backend_id(tmp_path, builtin_environment, monkeypatch): - from builtin_plugins.solution import create_solution - from test_019_odcs import write_contract - from test_019_odcs_cases import contract - - from datam8.plugins.builtins.odcs import OdcsFile - - write_contract(tmp_path / "contract.json", contract()) - path = create_solution(tmp_path, [(OdcsFile, "Contracts", {"path": "contract.json"})]) - config.set_solution(path) - factory.get_model() - monkeypatch.setattr("datam8.model.model.secrets.randbelow", lambda _: MAX_MODEL_ENTITY_ID - 1) - with TestClient(create_app()) as client: - response = client.put( - "/sources/Contracts/import", - json={ - "locator": "modelEntities/Stage/Imported", - "sourceLocation": "contract.json#customers", - }, - ) - assert response.status_code == 200, response.text - model = asyncio.run(factory.load_model(path)) - assert model.modelEntities.get("Stage/Imported").entity.id == MAX_MODEL_ENTITY_ID - - def test_save_rejects_duplicate_before_writing(id_workspace): model = factory.get_model() new = model.add_entity("modelEntities/Raw/New", entity_data()) @@ -178,22 +147,88 @@ def test_save_rejects_duplicate_before_writing(id_workspace): assert not new.source_file.exists() -@pytest.mark.parametrize("persist_first", [False, True]) -def test_rollback_deletes_only_created_entity(id_workspace, persist_first): +@pytest.mark.parametrize( + "failure", [None, "validation", "write", "existing", "reference", "target"] +) +def test_batch_preserves_files_drafts_and_resolves_forward_references( + id_workspace, monkeypatch, failure +): model = factory.get_model() - new = model.add_entity("modelEntities/Raw/New", entity_data()) original = model.modelEntities.get("Raw/Existing") original.update(description="Unsaved draft") - if persist_first: - model.save("modelEntities/Raw/New") - model.delete_entity(Locator.from_path("modelEntities/Raw/New")) - model.save("modelEntities/Raw/New") - assert not new.source_file.exists() - assert new.locator not in model.modelEntities - assert new.source_file not in model._model_files + function = original.source_file.parent / "keep.py" + function.write_text("# Existing user file\n", encoding="utf-8") + first, second = entity_data("First"), entity_data("Second") + first["sources"] = [{"sourceLocation": "modelEntities/Raw/Second"}] + first["transformations"] = [ + {"stepNo": 1, "name": "Function", "kind": "function", "function": {"source": "keep.py"}} + ] + second["relationships"] = [ + { + "targetLocation": "modelEntities/Raw/First", + "attributes": [{"sourceName": "Id", "targetName": "Id"}], + } + ] + if failure == "validation": + second["attributes"] = [] + if failure == "reference": + first["sources"][0]["sourceLocation"] = "modelEntities/Raw/Missing" + if failure == "write": + create = EntityFileRef.create + + def fail_second(file_ref, *, wrappers): + if file_ref.file_path.stem == "Second": + assert (file_ref.file_path.parent / "First.json").exists() + raise OSError("Simulated write failure") + create(file_ref, wrappers=wrappers) + + monkeypatch.setattr(EntityFileRef, "create", fail_second) + with TestClient(create_app(), raise_server_exceptions=False) as client: + response = client.put( + "/entities/batch", + json={ + "items": [ + {"locator": "modelEntities/Raw/First", "content": first}, + { + "locator": { + "existing": "modelEntities/Raw/Existing", + "target": "modelEntities/Raw/", + }.get(failure, "modelEntities/Raw/Second"), + "content": second, + }, + ] + }, + ) + if failure: + assert response.status_code == (500 if failure == "write" else 400), response.text + expected = { + "validation": "attributes", + "write": "Simulated write failure", + "existing": "batch target", + "reference": "Unknown model reference", + "target": "batch target", + } + assert expected[failure] in response.text + assert len(model.modelEntities) == 1 + assert not (original.source_file.parent / "First.json").exists() + assert not (original.source_file.parent / "Second.json").exists() + assert all(path.stem not in ("First", "Second") for path in model._model_files) + else: + assert response.status_code == 200, response.text + saved = [ + json.loads((original.source_file.parent / f"{name}.json").read_text()) + for name in ("First", "Second") + ] + assert all( + item["id"] == row["entity"]["id"] + for item, row in zip(saved, response.json()["items"], strict=True) + ) + assert saved[0]["sources"][0]["sourceLocation"] == saved[1]["id"] + assert saved[1]["relationships"][0]["targetLocation"] == saved[0]["id"] + assert function.read_text(encoding="utf-8") == "# Existing user file\n" assert original.has_changed assert "description" not in json.loads(original.source_file.read_text()) model.save() reloaded = asyncio.run(factory.load_model(id_workspace)) - assert len(reloaded.modelEntities) == 1 + assert len(reloaded.modelEntities) == (1 if failure else 3) assert reloaded.modelEntities.get("Raw/Existing").entity.description == "Unsaved draft" From 3e79ebf90f501eddad7f95797dd36101155a7d81 Mon Sep 17 00:00:00 2001 From: Fabio Kayser Date: Fri, 6 Nov 2026 10:40:33 +0100 Subject: [PATCH 8/9] Simplify entity creation and centralize folder ID allocation --- docs/backend-contract.md | 30 ++-- src/datam8/api/routes/entities.py | 19 --- src/datam8/factory.py | 11 +- src/datam8/migration_v1.py | 17 +- src/datam8/model/entity_wrapper.py | 8 +- src/datam8/model/ids.py | 16 -- src/datam8/model/model.py | 152 ++++++------------ src/datam8/operations.py | 3 +- src/datam8/parser_v1.py | 6 +- src/datam8/utils/ids.py | 26 ++++ tests/test_025_model_entity_ids.py | 240 +++++++++++++++++------------ 11 files changed, 252 insertions(+), 276 deletions(-) delete mode 100644 src/datam8/model/ids.py create mode 100644 src/datam8/utils/ids.py diff --git a/docs/backend-contract.md b/docs/backend-contract.md index daefc00..213ad1e 100644 --- a/docs/backend-contract.md +++ b/docs/backend-contract.md @@ -43,7 +43,7 @@ The current `create_app().openapi()` exposes these domains (no `/api` prefix): - `GET /health` (204), `GET /version` (`schemaVersion`, `appVersion`), `GET /config`. - `GET /solution`, `GET /solution/full`. -- `GET /entities`, `GET|PUT|PATCH|DELETE /entities/{locator}`, `PUT /entities/clone`, `PUT /entities/batch`, +- `GET /entities`, `GET|PUT|PATCH|DELETE /entities/{locator}`, `PUT /entities/clone`, `POST /entities/rename`, `POST /entities/move`, `POST /entities/move-single`. - `POST /model/save`, `POST /model/reload`, `GET /model/unsaved`. - `POST /model/generate`: optional body `{ "target": "...", "cleanOutput": true, @@ -91,25 +91,19 @@ editor. Runtime configuration changes and process failure may require a restart. ## Entity and function invariants -- Model entity IDs are assigned by the backend on create, clone and source import, - uniformly in `1..9007199254740991`, independently of the solution's highest ID. +- Model and folder entity IDs are assigned centrally by the backend on create, + clone, source import and migration, uniformly in `1..9007199254740991`, + independently of the solution's highest ID. Implicit folders use the same + allocator. New IDs avoid IDs already used by either entity type. Create ignores a supplied ID; the returned wrapper's `entity.id` is authoritative. Updates may include the unchanged ID but cannot change it. Rename/move preserve it. - Loading (including lazy load/reload) and saving reject duplicate IDs among live - model entities and IDs outside that range. Duplicate errors identify the ID and - affected locators. Existing valid IDs are retained. Solution repositories must - run `datam8 validate --solution --json` as a required merge check - on their merged solution state; independent branches cannot reserve each other's IDs. -- `PUT /entities/batch` creates and saves model entities in one request: - `{ "items": [{ "locator": "modelEntities/Raw/Customer", "content": { ... } }] }`. - Internal `sourceLocation` and `targetLocation` may contain model entity locators, - including forward/self references within the batch. The backend assigns IDs, - resolves these references and validates all items before saving only this batch. - Existing locators/files are rejected. The response's `items` contains the created - wrappers in request order. Failure removes this attempt's entities and new JSON - files; existing files, function sources and unrelated drafts are retained. A lost - success response leaves a complete saved batch; reload before retrying. This is - not a transaction against process crashes or persistent filesystem failures. + Loading (including lazy load/reload) and saving reject duplicate IDs within each + entity type and IDs outside that range. Duplicate errors identify the ID and + affected locators. Existing valid IDs are retained, including historical overlap + between model and folder IDs. Solution repositories must run + `datam8 validate --solution --json` as a required merge check on + their merged solution state; independent branches cannot reserve each other's IDs. + Multiple creates use the regular entity endpoint followed by `/model/save`. - `PUT /entities/{locator}` accepts an optional `sourceFile` query parameter for Attribute Types, Data Types, Data Sources, Data Source Types, Data Products, Zones, Properties and Property Values. It selects the existing collection JSON diff --git a/src/datam8/api/routes/entities.py b/src/datam8/api/routes/entities.py index b135dfe..5fd6eea 100644 --- a/src/datam8/api/routes/entities.py +++ b/src/datam8/api/routes/entities.py @@ -72,25 +72,6 @@ async def clone_entity( return MultiItemResponse.from_list([entity]) -class BatchEntityBody(BaseModel): - locator: str - content: dict[str, Any] - - -class BatchCreateBody(BaseModel): - items: Annotated[list[BatchEntityBody], Field(min_length=1)] - - -@entities_router.put("/batch") -async def create_model_entities( - body: BatchCreateBody, -) -> MultiItemResponse[model.EntityWrapper[b.BaseEntityType]]: - created = factory.get_model().add_model_entities( - [(item.locator, item.content) for item in body.items] - ) - return MultiItemResponse.from_list(created) - - @entities_router.put("/{locator:path}") async def create_entity( body: dict[str, Any], diff --git a/src/datam8/factory.py b/src/datam8/factory.py index 74b0a9f..fe556cc 100644 --- a/src/datam8/factory.py +++ b/src/datam8/factory.py @@ -221,7 +221,7 @@ def create_undefined_folders(_model: model.Model, /): Goes through all model entities and adds folder wrappers for every parent locator of an entity that does not exist yet. - Folder IDs are simply sequential increased during runtime. + Folder IDs use the model's central ID allocator. Parameters ---------- @@ -230,15 +230,12 @@ def create_undefined_folders(_model: model.Model, /): """ logger.debug("Create undefined folders") - existing_folder_ids = [w.entity.id for w in _model.folders.values()] - next_id = (max(existing_folder_ids) if len(existing_folder_ids) != 0 else 0) + 1 - undefined_folders: model.EntityDict[f.Folder] = {} for wrapper in _model.get_entity_iterator(): loc = wrapper.locator for ploc in loc.parents: - if ploc in _model.folders or ploc.entityName is None: + if ploc in _model.folders or ploc in undefined_folders or ploc.entityName is None: continue source_file_path = pathlib.Path( @@ -254,13 +251,11 @@ def create_undefined_folders(_model: model.Model, /): source_file=source_file_path.absolute(), locator=ploc, entity=f.Folder( - id=next_id, + id=_model.get_next_entity_id(), name=ploc.entityName, ), ) - next_id += 1 - for loc in undefined_folders: if loc not in _model.folders: _model.folders[loc] = undefined_folders[loc] diff --git a/src/datam8/migration_v1.py b/src/datam8/migration_v1.py index ab62678..728bd3e 100644 --- a/src/datam8/migration_v1.py +++ b/src/datam8/migration_v1.py @@ -7,6 +7,7 @@ from typing import Any, overload from datam8 import parser_v1, utils +from datam8.utils.ids import generate_entity_id from datam8_model import attribute as a from datam8_model import base as b from datam8_model import data_product as dp @@ -780,14 +781,14 @@ def create_new_properties(tags: Tags, output_path: pathlib.Path) -> None: with open(output_path / "property_values.json", "w", encoding="utf-8") as file: file.write(new_property_values.model_dump_json(**MODEL_DUMP_OPTIONS)) - @staticmethod def create_properties_for_folders( + self, zones: Sequence[z.Zone], data_products: Sequence[dp.DataProduct], output_path: pathlib.Path, ) -> None: """Should only be called AFTER other models have been migrated. No new folders will be created.""" - next_id = 1 + used_ids = {ref.id for ref in self.model_file_references.values()} for zone in zones: zone_path = output_path / zone.name @@ -801,9 +802,14 @@ def create_properties_for_folders( product_folder = b.Folders( type="folders", - folders=[f.Folder(id=next_id, name=product.name, dataProduct=product.name)], + folders=[ + f.Folder( + id=generate_entity_id(used_ids), + name=product.name, + dataProduct=product.name, + ) + ], ) - next_id += 1 with open(product_path / ".properties.json", "w", encoding="utf-8") as _file: _file.write(product_folder.model_dump_json(**MODEL_DUMP_OPTIONS)) @@ -817,14 +823,13 @@ def create_properties_for_folders( type="folders", folders=[ f.Folder( - id=next_id, + id=generate_entity_id(used_ids), name=module.name, dataProduct=product.name, dataModule=module.name, ) ], ) - next_id += 1 with open(module_path / ".properties.json", "w", encoding="utf-8") as _file: _file.write(module_folder.model_dump_json(**MODEL_DUMP_OPTIONS)) diff --git a/src/datam8/model/entity_wrapper.py b/src/datam8/model/entity_wrapper.py index d40e27d..96e894a 100644 --- a/src/datam8/model/entity_wrapper.py +++ b/src/datam8/model/entity_wrapper.py @@ -27,6 +27,7 @@ from pydantic import BaseModel, ConfigDict, ValidationError from datam8 import errors, logging, utils +from datam8.utils.ids import validate_entity_id from datam8_model import attribute as a from datam8_model import base as b from datam8_model import data_product as dp @@ -37,7 +38,6 @@ from datam8_model import property as p from datam8_model import zone as z -from .ids import validate_model_entity_id from .locator import Locator, LocatorOrString, _ensure_locator logger = logging.getLogger(__name__) @@ -459,11 +459,11 @@ def has_property(self, property_name: str, /) -> bool: return False def update(self, **kwargs: Any) -> None: - if isinstance(self.entity, m.ModelEntity) and "id" in kwargs: - validate_model_entity_id(kwargs["id"], locator=str(self.locator)) + if isinstance(self.entity, (m.ModelEntity, f.Folder)) and "id" in kwargs: + validate_entity_id(kwargs["id"], locator=str(self.locator)) if kwargs["id"] != self.entity.id: raise errors.Datam8ValidationError( - message=f"Model entity ID cannot be changed at {self.locator}", + message=f"Entity ID cannot be changed at {self.locator}", details={"id": self.entity.id, "requestedId": kwargs["id"]}, ) new_entity = self.entity.model_copy(update=kwargs, deep=True) diff --git a/src/datam8/model/ids.py b/src/datam8/model/ids.py deleted file mode 100644 index e6d9eb8..0000000 --- a/src/datam8/model/ids.py +++ /dev/null @@ -1,16 +0,0 @@ -"""Model entity IDs shared by loading, creation and updates.""" - -from datam8.errors import Datam8ValidationError - -MAX_MODEL_ENTITY_ID = (1 << 53) - 1 - - -def validate_model_entity_id(value: object, *, locator: str) -> None: - if type(value) is not int or not 1 <= value <= MAX_MODEL_ENTITY_ID: - raise Datam8ValidationError( - message=( - f"Invalid model entity ID {value!r} at {locator}; " - f"expected an integer in 1..{MAX_MODEL_ENTITY_ID}" - ), - details={"id": value, "locator": locator}, - ) diff --git a/src/datam8/model/model.py b/src/datam8/model/model.py index e5f86bb..3c2ba67 100644 --- a/src/datam8/model/model.py +++ b/src/datam8/model/model.py @@ -18,9 +18,7 @@ from __future__ import annotations -import secrets from collections.abc import Iterator, Sequence -from copy import deepcopy from pathlib import Path from threading import Lock from typing import Annotated, Any, overload @@ -28,6 +26,7 @@ from pydantic import ConfigDict, Field, ValidationError from datam8 import config, errors, functions, logging, opts, utils +from datam8.utils.ids import generate_entity_id, validate_entity_id from datam8_model import attribute as a from datam8_model import base as b from datam8_model import data_product as dp @@ -41,7 +40,6 @@ from . import entity_wrapper as ew from .entity_wrapper import EntityRepository, EntityWrapper, EntityWrapperVariant -from .ids import MAX_MODEL_ENTITY_ID, validate_model_entity_id from .locator import ROOT_LOCATOR, Locator, LocatorOrString, _ensure_locator logger = logging.getLogger(__name__) @@ -149,37 +147,42 @@ def __init__(self, solution: s.Solution, /, **kwargs: EntityRepository[b.BaseEnt self._model_files: dict[Path, EntityFileRef] = {} """Internal dictionary to allow easy mapping of files and their entities""" - self.validate_model_entity_ids() - self._used_model_ids = {wrapper.entity.id for wrapper in self.modelEntities.values()} + self.validate_entity_ids() + self._used_entity_ids = { + wrapper.entity.id + for repo in (self.modelEntities, self.folders) + for wrapper in repo.values() + } def __getitem__(self, type_: str) -> EntityRepository[b.BaseEntityType]: return getattr(self, type_) - def get_next_model_id(self) -> int: - with self.lock: - while True: - entity_id = secrets.randbelow(MAX_MODEL_ENTITY_ID) + 1 - if entity_id not in self._used_model_ids: - self._used_model_ids.add(entity_id) - return entity_id - - def validate_model_entity_ids(self) -> None: - locators_by_id: dict[int, list[str]] = {} - for wrapper in self.modelEntities.values(): - if wrapper.is_deleted: - continue - entity_id = wrapper.entity.id - locator = str(wrapper.locator) - validate_model_entity_id(entity_id, locator=locator) - locators_by_id.setdefault(entity_id, []).append(locator) + def get_next_entity_id(self) -> int: + return generate_entity_id(self._used_entity_ids) - duplicates = {key: value for key, value in locators_by_id.items() if len(value) > 1} - if duplicates: - raise errors.Datam8ValidationError( - message="Duplicate model entity IDs: " - + "; ".join(f"{key}: {', '.join(value)}" for key, value in duplicates.items()), - details={"duplicates": duplicates}, - ) + get_next_model_id = get_next_entity_id + + def validate_entity_ids(self) -> None: + for entity_type, repo in ( + (b.EntityType.MODEL_ENTITIES, self.modelEntities), + (b.EntityType.FOLDERS, self.folders), + ): + locators_by_id: dict[int, list[str]] = {} + for wrapper in repo.values(): + if wrapper.is_deleted: + continue + entity_id = wrapper.entity.id + locator = str(wrapper.locator) + validate_entity_id(entity_id, locator=locator) + locators_by_id.setdefault(entity_id, []).append(locator) + + duplicates = {key: value for key, value in locators_by_id.items() if len(value) > 1} + if duplicates: + raise errors.Datam8ValidationError( + message=f"Duplicate IDs in {entity_type.value}: " + + "; ".join(f"{key}: {', '.join(value)}" for key, value in duplicates.items()), + details={"entityType": entity_type.value, "duplicates": duplicates}, + ) def init_file_references(self) -> None: self._model_files = {} @@ -480,15 +483,17 @@ def add_entity[T: b.BaseEntityType]( match content: case dict() as c: c["name"] = _locator.entityName - if _type == b.EntityType.MODEL_ENTITIES: - c["id"] = self.get_next_model_id() + if _type in (b.EntityType.MODEL_ENTITIES, b.EntityType.FOLDERS): + c["id"] = self.get_next_entity_id() if _type == b.EntityType.FOLDERS: c["path"] = "/".join([*_locator.folders, _locator.entityName]) new_entity = class_from_type(_type).from_dict(content) - case m.ModelEntity() as me: - new_entity = me - new_entity.id = self.get_next_model_id() + case (m.ModelEntity() | f.Folder()) as identified: + new_entity = identified.model_copy(deep=True) + new_entity.id = self.get_next_entity_id() new_entity.name = _locator.entityName + if isinstance(new_entity, f.Folder): + new_entity.path = "/".join([*_locator.folders, _locator.entityName]) case _ as rest: new_entity = rest @@ -510,68 +515,6 @@ def add_entity[T: b.BaseEntityType]( return new_wrapper - def add_model_entities( - self, items: Sequence[tuple[str, dict[str, Any]]] - ) -> list[EntityWrapper[b.BaseEntityType]]: - """Create a complete batch, retaining existing files and unrelated drafts on failure.""" - created: list[EntityWrapper[b.BaseEntityType]] = [] - paths: set[Path] = set() - root = self.get_base_path_for_entity_type(b.EntityType.MODEL_ENTITIES).resolve() - try: - for locator, content in items: - loc = _ensure_locator(locator) - path = (root / Path(*loc.folders) / f"{loc.entityName}.json").resolve() - if ( - loc.entityType != b.EntityType.MODEL_ENTITIES.value - or loc.entityName is None - or not path.is_relative_to(root) - or loc in self.modelEntities - or path in paths - or path.exists() - ): - raise errors.Datam8ValidationError( - message=f"Invalid or existing batch target: {loc}" - ) - created.append( - self.add_entity(loc, {**content, "sources": [], "relationships": []}) - ) - paths.add(path) - - references = { - str(w.locator): w.entity.id for w in self.modelEntities.values() if not w.is_deleted - } - live_ids = set(references.values()) - for wrapper, (_, content) in zip(created, items, strict=True): - updates = {} - for field, location in ( - ("sources", "sourceLocation"), - ("relationships", "targetLocation"), - ): - rows = deepcopy(content.get(field, [])) - for row in rows if isinstance(rows, list) else []: - if isinstance(row, dict) and not row.get("dataSource"): - value = row.get(location) - if isinstance(value, str): - value = references.get(value.lstrip("/")) - if type(value) is not int or value not in live_ids: - raise errors.Datam8ValidationError( - message=f"Unknown model reference: {row.get(location)}" - ) - row[location] = value - updates[field] = rows - wrapper.entity = m.ModelEntity.from_dict({**wrapper.entity.model_dump(), **updates}) - self.save(locators=[str(wrapper.locator) for wrapper in created]) - except Exception as exc: - for wrapper in created: - self.modelEntities.remove(wrapper.locator) - self._model_files.pop(wrapper.source_file, None) - for path in paths: - path.unlink(missing_ok=True) - if isinstance(exc, ValidationError): - raise errors.Datam8ValidationError(message=str(exc)) from exc - raise - return created - def _existing_base_target(self, _type: b.EntityType, source_file: str | Path) -> Path: """Validate explicit collection placement before mutating model state.""" if _type in (b.EntityType.MODEL_ENTITIES, b.EntityType.FOLDERS, b.EntityType.DATA_MODULES): @@ -974,18 +917,17 @@ def get_child_locators(self, search_locator: str | Locator, /) -> list[Locator]: return found_locators - def save(self, locator: str | None = None, /, *, locators: Sequence[str] | None = None) -> None: + def save(self, locator: str | None = None, /) -> None: """ Saves the current state of the model or a specific entity to the corresponding json file. """ - self.validate_model_entity_ids() - selected = ( - self.get_entities(locator or ROOT_LOCATOR) - if locators is None - else [self.get_entity_by_locator(loc) for loc in locators] - ) - changed_wrappers = [w for w in selected if w.has_changed and not w.is_deleted] - deleted_wrappers = [w for w in selected if w.is_deleted] + self.validate_entity_ids() + changed_wrappers = [ + w + for w in self.get_entities(locator or ROOT_LOCATOR) + if w.has_changed and not w.is_deleted + ] + deleted_wrappers = [w for w in self.get_entities(locator or ROOT_LOCATOR) if w.is_deleted] no_of_changes = len(changed_wrappers) no_of_deletions = len(deleted_wrappers) diff --git a/src/datam8/operations.py b/src/datam8/operations.py index acb51e3..2c2cd71 100644 --- a/src/datam8/operations.py +++ b/src/datam8/operations.py @@ -32,6 +32,7 @@ from datam8.model.model import class_from_type from datam8.plugins.navigation import normalize_source_item from datam8_model import base as b +from datam8_model import folder as f from datam8_model import model as m @@ -380,7 +381,7 @@ def execute(self, operation: Operation) -> dict: assert op.new_locator is not None self.model.move_entities(locator, op.new_locator, force=op.force) result = self.persist() - if created is not None and isinstance(created.entity, m.ModelEntity): + if created is not None and isinstance(created.entity, (m.ModelEntity, f.Folder)): result["item"] = {"locator": str(created.locator), "id": created.entity.id} return result diff --git a/src/datam8/parser_v1.py b/src/datam8/parser_v1.py index 82af96d..2a645d8 100644 --- a/src/datam8/parser_v1.py +++ b/src/datam8/parser_v1.py @@ -2,6 +2,7 @@ import json import pathlib +from datam8.utils.ids import generate_entity_id from datam8_model.v1 import ( CoreModelEntry, CuratedModelEntry, @@ -24,7 +25,7 @@ def parse_solution_file( index = Index.Model.from_json_file(solution_file_path.parent / "index.json") indeces: list[IndexEntry] = [] mapping_file_id: dict[str, ModelFileReference] = {} - next_id = 1 + used_ids: set[int] = set() if index.rawIndex is not None and index.rawIndex.entry is not None: indeces.extend(index.rawIndex.entry) @@ -40,9 +41,8 @@ def parse_solution_file( for idx in indeces: mapping_file_id[idx.locator.lower()] = ModelFileReference( - id=next_id, path=pathlib.Path(idx.absPath) + id=generate_entity_id(used_ids), path=pathlib.Path(idx.absPath) ) - next_id += 1 return solution, mapping_file_id diff --git a/src/datam8/utils/ids.py b/src/datam8/utils/ids.py new file mode 100644 index 0000000..2360c4a --- /dev/null +++ b/src/datam8/utils/ids.py @@ -0,0 +1,26 @@ +"""Central allocation and validation of model and folder entity IDs.""" + +import secrets +from threading import Lock + +from datam8.errors import Datam8ValidationError + +MAX_ENTITY_ID = (1 << 53) - 1 +_lock = Lock() + + +def generate_entity_id(used_ids: set[int]) -> int: + with _lock: + while True: + entity_id = secrets.randbelow(MAX_ENTITY_ID) + 1 + if entity_id not in used_ids: + used_ids.add(entity_id) + return entity_id + + +def validate_entity_id(value: object, *, locator: str) -> None: + if type(value) is not int or not 1 <= value <= MAX_ENTITY_ID: + raise Datam8ValidationError( + message=f"Invalid entity ID {value!r} at {locator}; expected an integer in 1..{MAX_ENTITY_ID}", + details={"id": repr(value), "locator": locator}, + ) diff --git a/tests/test_025_model_entity_ids.py b/tests/test_025_model_entity_ids.py index 47eef87..9df5dd6 100644 --- a/tests/test_025_model_entity_ids.py +++ b/tests/test_025_model_entity_ids.py @@ -2,6 +2,7 @@ import asyncio import json +import secrets from concurrent.futures import ThreadPoolExecutor import pytest @@ -12,8 +13,7 @@ from datam8.api.app import create_app from datam8.app import app from datam8.errors import Datam8ValidationError -from datam8.model.ids import MAX_MODEL_ENTITY_ID -from datam8.model.model import EntityFileRef +from datam8.utils.ids import MAX_ENTITY_ID def entity_data(name="Existing", entity_id=7): @@ -47,6 +47,10 @@ def id_workspace(tmp_path, monkeypatch): ) folder = tmp_path / "model/Raw" folder.mkdir(parents=True) + (folder / ".properties.json").write_text( + json.dumps({"type": "folders", "folders": [{"id": 11, "name": "Raw", "path": "Raw"}]}), + encoding="utf-8", + ) (folder / "Existing.json").write_text(json.dumps(entity_data()), encoding="utf-8") monkeypatch.setattr(config, "solution_path", path, raising=False) monkeypatch.setattr(config, "solution_folder_path", tmp_path, raising=False) @@ -58,9 +62,9 @@ def id_workspace(tmp_path, monkeypatch): def test_allocation_retries_loaded_and_reserved_ids(id_workspace, monkeypatch): model = factory.get_model() - draws = iter([6, MAX_MODEL_ENTITY_ID - 1, MAX_MODEL_ENTITY_ID - 1, 0]) - monkeypatch.setattr("datam8.model.model.secrets.randbelow", lambda _: next(draws)) - assert model.get_next_model_id() == MAX_MODEL_ENTITY_ID + draws = iter([6, 10, MAX_ENTITY_ID - 1, MAX_ENTITY_ID - 1, 0]) + monkeypatch.setattr("datam8.utils.ids.secrets.randbelow", lambda _: next(draws)) + assert model.get_next_model_id() == MAX_ENTITY_ID assert model.get_next_model_id() == 1 @@ -70,19 +74,20 @@ def test_parallel_allocation_reserves_unique_safe_ids(id_workspace): ids = list(executor.map(lambda _: model.get_next_model_id(), range(500))) assert len(set(ids)) == 500 assert 7 not in ids - assert all(type(value) is int and 1 <= value <= MAX_MODEL_ENTITY_ID for value in ids) + assert all(type(value) is int and 1 <= value <= MAX_ENTITY_ID for value in ids) def test_create_clone_update_move_and_reload_keep_actual_ids(id_workspace, monkeypatch): factory.get_model() - draws = iter([MAX_MODEL_ENTITY_ID - 1, 0]) - monkeypatch.setattr("datam8.model.model.secrets.randbelow", lambda _: next(draws)) + original_randbelow = secrets.randbelow + draws = iter([MAX_ENTITY_ID - 1, 0]) + monkeypatch.setattr("datam8.utils.ids.secrets.randbelow", lambda _: next(draws)) with TestClient(create_app()) as client: body = entity_data("New") body.pop("id") created = client.put("/entities/modelEntities/Raw/New", json=body) assert created.status_code == 200, created.text - assert created.json()["item"]["entity"]["id"] == MAX_MODEL_ENTITY_ID + assert created.json()["item"]["entity"]["id"] == MAX_ENTITY_ID cloned = client.put( "/entities/clone", json={"locator": "modelEntities/Raw/New", "newLocator": "modelEntities/Raw/Copy"}, @@ -91,7 +96,7 @@ def test_create_clone_update_move_and_reload_keep_actual_ids(id_workspace, monke assert cloned.json()["items"][0]["entity"]["id"] == 1 updated = client.patch( "/entities/modelEntities/Raw/New", - json={"id": MAX_MODEL_ENTITY_ID, "description": "Changed"}, + json={"id": MAX_ENTITY_ID, "description": "Changed"}, ) assert updated.status_code == 200, updated.text rejected = client.patch("/entities/modelEntities/Raw/New", json={"id": 7}) @@ -102,18 +107,19 @@ def test_create_clone_update_move_and_reload_keep_actual_ids(id_workspace, monke json={"from": "modelEntities/Raw/New", "to": "modelEntities/Raw/Renamed"}, ) assert moved.status_code == 200, moved.text - assert moved.json()["item"]["entity"]["id"] == MAX_MODEL_ENTITY_ID + assert moved.json()["item"]["entity"]["id"] == MAX_ENTITY_ID assert client.post("/model/save").status_code == 204 + monkeypatch.setattr("datam8.utils.ids.secrets.randbelow", original_randbelow) assert client.post("/model/reload").status_code == 200 model = factory.get_model() - assert model.modelEntities.get("Raw/Renamed").entity.id == MAX_MODEL_ENTITY_ID + assert model.modelEntities.get("Raw/Renamed").entity.id == MAX_ENTITY_ID assert model.modelEntities.get("Raw/Copy").entity.id == 1 saved = json.loads((id_workspace.parent / "model/Raw/Renamed.json").read_text()) - assert saved["id"] == MAX_MODEL_ENTITY_ID + assert saved["id"] == MAX_ENTITY_ID @pytest.mark.parametrize( - "entity_id,exit_code", [(0, 2), (-1, 2), (MAX_MODEL_ENTITY_ID + 1, 2), (MAX_MODEL_ENTITY_ID, 0)] + "entity_id,exit_code", [(0, 2), (-1, 2), (MAX_ENTITY_ID + 1, 2), (MAX_ENTITY_ID, 0)] ) def test_cli_validation_checks_id_range(id_workspace, entity_id, exit_code): file = id_workspace.parent / "model/Raw/Invalid.json" @@ -126,7 +132,7 @@ def test_cli_validation_checks_id_range(id_workspace, entity_id, exit_code): def test_duplicate_ids_fail_loading_and_cli_with_locators(id_workspace): file = id_workspace.parent / "model/Raw/Duplicate.json" file.write_text(json.dumps(entity_data("Duplicate")), encoding="utf-8") - with pytest.raises(Datam8ValidationError, match="Duplicate model entity IDs") as caught: + with pytest.raises(Datam8ValidationError, match="Duplicate IDs in modelEntities") as caught: asyncio.run(factory.load_model(id_workspace)) assert set(caught.value.details["duplicates"][7]) == { "modelEntities/Raw/Existing", @@ -147,88 +153,130 @@ def test_save_rejects_duplicate_before_writing(id_workspace): assert not new.source_file.exists() -@pytest.mark.parametrize( - "failure", [None, "validation", "write", "existing", "reference", "target"] -) -def test_batch_preserves_files_drafts_and_resolves_forward_references( - id_workspace, monkeypatch, failure -): +@pytest.mark.parametrize("supplied_id", [None, 7]) +def test_folder_create_clone_update_move_and_reload(id_workspace, supplied_id): + factory.get_model() + with TestClient(create_app()) as client: + body = {"name": "Ignored"} + if supplied_id is not None: + body["id"] = supplied_id + created = client.put("/entities/folders/Raw/New", json=body) + assert created.status_code == 200, created.text + saved = created.json()["item"]["entity"] + folder_id = saved["id"] + assert type(folder_id) is int and 1 <= folder_id <= MAX_ENTITY_ID + assert folder_id not in (7, 11) + assert saved["name"] == "New" and saved["path"] == "Raw/New" + cloned = client.put( + "/entities/clone", + json={"locator": "folders/Raw/New", "newLocator": "folders/Raw/Copy"}, + ) + assert cloned.status_code == 200, cloned.text + copy = cloned.json()["items"][0]["entity"] + assert copy["id"] not in (7, 11, folder_id) + assert copy["path"] == "Raw/Copy" + updated = client.patch( + "/entities/folders/Raw/New", json={"id": folder_id, "description": "Saved"} + ) + assert updated.status_code == 200, updated.text + assert client.patch("/entities/folders/Raw/New", json={"id": 7}).status_code == 400 + moved = client.post( + "/entities/move-single", + json={"from": "folders/Raw/New", "to": "folders/Raw/Renamed"}, + ) + assert moved.status_code == 200, moved.text + assert moved.json()["item"]["entity"]["id"] == folder_id + assert client.post("/model/save").status_code == 204 + assert client.post("/model/reload").status_code == 200 model = factory.get_model() - original = model.modelEntities.get("Raw/Existing") - original.update(description="Unsaved draft") - function = original.source_file.parent / "keep.py" - function.write_text("# Existing user file\n", encoding="utf-8") - first, second = entity_data("First"), entity_data("Second") - first["sources"] = [{"sourceLocation": "modelEntities/Raw/Second"}] - first["transformations"] = [ - {"stepNo": 1, "name": "Function", "kind": "function", "function": {"source": "keep.py"}} - ] - second["relationships"] = [ - { - "targetLocation": "modelEntities/Raw/First", - "attributes": [{"sourceName": "Id", "targetName": "Id"}], - } - ] - if failure == "validation": - second["attributes"] = [] - if failure == "reference": - first["sources"][0]["sourceLocation"] = "modelEntities/Raw/Missing" - if failure == "write": - create = EntityFileRef.create - - def fail_second(file_ref, *, wrappers): - if file_ref.file_path.stem == "Second": - assert (file_ref.file_path.parent / "First.json").exists() - raise OSError("Simulated write failure") - create(file_ref, wrappers=wrappers) - - monkeypatch.setattr(EntityFileRef, "create", fail_second) - with TestClient(create_app(), raise_server_exceptions=False) as client: - response = client.put( - "/entities/batch", - json={ - "items": [ - {"locator": "modelEntities/Raw/First", "content": first}, - { - "locator": { - "existing": "modelEntities/Raw/Existing", - "target": "modelEntities/Raw/", - }.get(failure, "modelEntities/Raw/Second"), - "content": second, - }, - ] - }, + assert model.folders.get("Raw/Renamed").entity.id == folder_id + assert model.folders.get("Raw/Copy").entity.id == copy["id"] + persisted = json.loads((id_workspace.parent / "model/Raw/Renamed/.properties.json").read_text()) + assert persisted["folders"][0]["id"] == folder_id + assert persisted["folders"][0]["description"] == "Saved" + + +def test_implicit_folders_use_allocator_once_per_path(id_workspace, monkeypatch): + model = factory.get_model() + model.add_entity("modelEntities/Raw/Second", entity_data("Second")) + model.folders.remove("Raw") + existing_count = len(model.folders) + # All other implicit folders have already been created; only Raw is missing. + draws = iter([6, MAX_ENTITY_ID - 1]) + monkeypatch.setattr("datam8.utils.ids.secrets.randbelow", lambda _: next(draws)) + factory.create_undefined_folders(model) + assert model.folders.get("Raw").entity.id == MAX_ENTITY_ID + assert len(model.folders) == existing_count + 1 + + +@pytest.mark.parametrize("folder_id", [7, 11, MAX_ENTITY_ID + 1]) +def test_folder_load_preserves_overlap_and_rejects_invalid_ids(id_workspace, folder_id): + folder = id_workspace.parent / "model/Raw/Nested" + folder.mkdir() + (folder / ".properties.json").write_text( + json.dumps( + { + "type": "folders", + "folders": [{"id": folder_id, "name": "Nested", "path": "Raw/Nested"}], + } + ), + encoding="utf-8", + ) + if folder_id == 7: + model = factory.get_model() + assert ( + model.folders.get("Raw/Nested").entity.id + == model.modelEntities.get("Raw/Existing").entity.id ) - if failure: - assert response.status_code == (500 if failure == "write" else 400), response.text - expected = { - "validation": "attributes", - "write": "Simulated write failure", - "existing": "batch target", - "reference": "Unknown model reference", - "target": "batch target", - } - assert expected[failure] in response.text - assert len(model.modelEntities) == 1 - assert not (original.source_file.parent / "First.json").exists() - assert not (original.source_file.parent / "Second.json").exists() - assert all(path.stem not in ("First", "Second") for path in model._model_files) else: - assert response.status_code == 200, response.text - saved = [ - json.loads((original.source_file.parent / f"{name}.json").read_text()) - for name in ("First", "Second") - ] - assert all( - item["id"] == row["entity"]["id"] - for item, row in zip(saved, response.json()["items"], strict=True) + with pytest.raises(Datam8ValidationError): + factory.get_model() + + +def test_typed_creation_does_not_change_existing_identity(id_workspace): + model = factory.get_model() + for locator, target in ( + ("modelEntities/Raw/Existing", "modelEntities/Raw/Copy"), + ("folders/Raw", "folders/Copy"), + ): + original = model.get_entity_by_locator(locator).entity + old_id, old_name = original.id, original.name + new = model.add_entity(target, original).entity + assert new.id != old_id + assert original.id == old_id and original.name == old_name + + +@pytest.mark.parametrize("locator", ["modelEntities/Raw/Existing", "folders/Raw"]) +def test_nonfinite_patch_id_returns_validation_error(id_workspace, locator): + factory.get_model() + with TestClient(create_app(), raise_server_exceptions=False) as client: + response = client.patch( + f"/entities/{locator}", + content='{"id":1e1000}', + headers={"Content-Type": "application/json"}, ) - assert saved[0]["sources"][0]["sourceLocation"] == saved[1]["id"] - assert saved[1]["relationships"][0]["targetLocation"] == saved[0]["id"] - assert function.read_text(encoding="utf-8") == "# Existing user file\n" - assert original.has_changed - assert "description" not in json.loads(original.source_file.read_text()) - model.save() - reloaded = asyncio.run(factory.load_model(id_workspace)) - assert len(reloaded.modelEntities) == (1 if failure else 3) - assert reloaded.modelEntities.get("Raw/Existing").entity.description == "Unsaved draft" + assert response.status_code == 400, response.text + assert response.json()["code"] == "validation_error" + + +def test_migration_folders_avoid_model_and_folder_ids(tmp_path, monkeypatch): + from datam8 import migration_v1, parser_v1 + from datam8_model import data_product as dp + from datam8_model import zone as z + + module_path = tmp_path / "Raw/Product/Module" + module_path.mkdir(parents=True) + migration = migration_v1.MigrationV1( + {"existing": parser_v1.ModelFileReference(id=7, path=tmp_path)} + ) + draws = iter([6, MAX_ENTITY_ID - 1, MAX_ENTITY_ID - 1, 0]) + monkeypatch.setattr("datam8.utils.ids.secrets.randbelow", lambda _: next(draws)) + migration.create_properties_for_folders( + zones=[z.Zone(name="Raw", targetName="raw", displayName="Raw")], + data_products=[dp.DataProduct(name="Product", dataModules=[dp.DataModule(name="Module")])], + output_path=tmp_path, + ) + product = json.loads((module_path.parent / ".properties.json").read_text()) + module = json.loads((module_path / ".properties.json").read_text()) + assert product["folders"][0]["id"] == MAX_ENTITY_ID + assert module["folders"][0]["id"] == 1 From c9a9b25dac1e0b01530b6a1239513109821dedef Mon Sep 17 00:00:00 2001 From: Fabio Kayser Date: Fri, 6 Nov 2026 12:52:01 +0100 Subject: [PATCH 9/9] docs: include backend entity IDs in 2.1.1 release notes --- docs/releases/2.1.1.md | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/docs/releases/2.1.1.md b/docs/releases/2.1.1.md index 9c6b7b7..e54406b 100644 --- a/docs/releases/2.1.1.md +++ b/docs/releases/2.1.1.md @@ -1,6 +1,7 @@ # DataM8 CLI 2.1.1 -This patch release fixes entity description refresh. +This patch release fixes entity description refresh and centralizes model and folder +entity ID allocation in the backend. ## Entity description refresh @@ -9,6 +10,18 @@ This patch release fixes entity description refresh. their existing description. Multiple sources may supply the same description; conflicting descriptions are rejected instead of selecting one arbitrarily. +## Backend-assigned entity IDs + +- New model and folder entities receive random numeric IDs in the JavaScript-safe + range from one backend allocator. Create, clone, source import, migration and + implicit folders avoid IDs already used by either entity type. +- Existing valid IDs remain unchanged on update, rename and move. Loading and + saving reject IDs outside the supported range and duplicates within each entity + type, with the affected locators in the error. Historical overlap between model + and folder IDs remains supported. +- Create ignores supplied IDs; clients must use the returned entity ID. Multiple + creates continue to use the existing entity endpoint followed by one model save. + ## Other changes since 2.1.0 - Include the dependency and schema security corrections already merged into main, @@ -19,7 +32,10 @@ This patch release fixes entity description refresh. The solution schema version remains **2.0.0**. Existing plugin hooks and source location handling are unchanged. Use the updated backend package or submodule -revision to receive the description-refresh fix. +revision to receive these changes. Clients creating model or folder entities must +retain the IDs returned by the backend. Existing solutions with duplicate or +out-of-range IDs must be corrected before they can be loaded or saved; valid +existing IDs are not renumbered. After merging the release PR into main, push the protected tag `v2.1.1` at the merged commit. The release workflow verifies the version, runs CI and publishes the tested