diff --git a/PYEGERIA_ISSUES.md b/PYEGERIA_ISSUES.md index 395a45cf..1ef10f1a 100644 --- a/PYEGERIA_ISSUES.md +++ b/PYEGERIA_ISSUES.md @@ -1121,8 +1121,14 @@ into the entry now, so it isn't rediscovered from scratch later. ### ISSUE-126: `ProductManager` and `DigitalBusiness` never set `collection_command_root`, so every inherited `CollectionManager` method that uses it raises `AttributeError` -**Layer:** pyegeria · **Status:** open, logged only, not fixed (awaiting approval) · **Found:** 2026-10-05, -live cleanup of a throwaway data contract (PR #427's live check). +**Layer:** pyegeria · **Status:** fixed on branch `fix/issue-126-127-collection-manager-subclasses` +(2026-10-05), pending PR/merge · **Found:** 2026-10-05, live cleanup of a throwaway data contract (PR #427's +live check). + +**Fix + scope (2026-10-05):** both constructors now call `CollectionManager.__init__`, as `GlossaryManager` +does; that also restores `metadata_expert_command_root`, which the SmartQuery relationship methods use. An AST scan of all 91 classes in +`pyegeria/omvs/` found no other subclass whose `__init__` skips its base's, and +`test_every_omvs_subclass_runs_its_base_constructor` now fails if one ever does. **What:** `ProductManager.__init__` and `DigitalBusiness.__init__` call `ServerClient.__init__` directly instead of `CollectionManager.__init__` (`GlossaryManager` does it correctly). `CollectionManager.__init__` is what sets @@ -1142,8 +1148,17 @@ constructs each `CollectionManager` subclass and asserts the attribute exists wo ### ISSUE-127: `CollectionManager.delete_collection(cascade=True)` silently never sends `cascadeDelete` -- same shape as ISSUE-62 -**Layer:** pyegeria · **Status:** open, logged only, not fixed (awaiting approval) · **Found:** 2026-10-05, found -by capturing the request body while diagnosing a failed cleanup. +**Layer:** pyegeria · **Status:** fixed on branch `fix/issue-126-127-collection-manager-subclasses` +(2026-10-05), pending PR/merge · **Found:** 2026-10-05, found by capturing the request body while diagnosing a +failed cleanup. + +**Fix + scope (2026-10-05):** a scan of all 42 callers of `_async_delete_element_request` found exactly two +that pre-fill a default body *and* pass a cascade flag: `CollectionManager._async_delete_collection` and +`ProductManager._async_delete_digital_product` (so `delete_digital_product(cascade=True)` was never cascading +either -- the live check's own product cleanup included). Both now pass `None` so the helper applies the flag. +`delete_digital_product`'s docstring sample also used the wrong key (`cascadedDelete`); corrected. The helper +itself is unchanged: a caller that passes an explicit body dict *and* `cascade=True` still gets the body's +value, which is documented behaviour ("body supersedes"). **What:** `_async_delete_collection` always pre-fills `body = {"class": "DeleteElementRequestBody"}` when none is given and hands that dict to `_async_delete_element_request(url, body, cascade)`. diff --git a/pyegeria/omvs/collection_manager.py b/pyegeria/omvs/collection_manager.py index b92401fa..420338b5 100644 --- a/pyegeria/omvs/collection_manager.py +++ b/pyegeria/omvs/collection_manager.py @@ -6567,8 +6567,8 @@ async def _async_delete_collection(self, collection_guid: str, body: dict | Dele """ - if body is None: - body = {"class": "DeleteElementRequestBody"} + # No default body here: _async_delete_element_request applies `cascade` only when it builds the body + # itself, so a pre-filled dict made `cascade=True` a silent no-op (ISSUE-127). url = f"{self.collection_command_root}/{collection_guid}/delete" await self._async_delete_element_request(url, body, cascade) logger.info(f"Deleted collection {collection_guid} with cascade {cascade}") diff --git a/pyegeria/omvs/digital_business.py b/pyegeria/omvs/digital_business.py index e9152f1b..85c91b70 100644 --- a/pyegeria/omvs/digital_business.py +++ b/pyegeria/omvs/digital_business.py @@ -85,7 +85,7 @@ def __init__( user_pwd: Optional[str] = None, token: Optional[str] = None, timeout: int = None): - ServerClient.__init__(self, view_server, platform_url, user_id, user_pwd, token, timeout=timeout) + CollectionManager.__init__(self, view_server, platform_url, user_id, user_pwd, token, timeout=timeout) self.view_server = self.server_name self.platform_url = self.platform_url self.user_id = self.user_id diff --git a/pyegeria/omvs/product_manager.py b/pyegeria/omvs/product_manager.py index 7f38fefd..41b52235 100644 --- a/pyegeria/omvs/product_manager.py +++ b/pyegeria/omvs/product_manager.py @@ -92,7 +92,7 @@ def __init__( user_pwd: Optional[str] = None, token: Optional[str] = None, timeout: int = None): - ServerClient.__init__(self, view_server, platform_url, user_id, user_pwd, token, timeout=timeout) + CollectionManager.__init__(self, view_server, platform_url, user_id, user_pwd, token, timeout=timeout) self.view_server = self.server_name self.platform_url = self.platform_url self.user_id = self.user_id @@ -322,12 +322,12 @@ async def _async_delete_digital_product( ```json { "class": "DeleteElementRequestBody", - "cascadedDelete": false + "cascadeDelete": false } ``` """ - if body is None: - body = {"class": "DeleteElementRequestBody"} + # No default body here: _async_delete_element_request applies `cascade` only when it builds the body + # itself, so a pre-filled dict made `cascade=True` a silent no-op (ISSUE-127). url = f"{self.product_manager_command_root}/collections/{digital_product_guid}/delete" await self._async_delete_element_request(url, body, cascade) logger.info(f"Deleted digital product {digital_product_guid} with cascade {cascade}") @@ -364,7 +364,7 @@ def delete_digital_product( ```json { "class": "DeleteElementRequestBody", - "cascadedDelete": false + "cascadeDelete": false } ``` """ diff --git a/tests/micro-tests/test_collection_manager_subclasses.py b/tests/micro-tests/test_collection_manager_subclasses.py new file mode 100644 index 00000000..c037da0e --- /dev/null +++ b/tests/micro-tests/test_collection_manager_subclasses.py @@ -0,0 +1,118 @@ +""" +ISSUE-126: ProductManager and DigitalBusiness called ServerClient.__init__ instead of +CollectionManager.__init__, so collection_command_root (used by 27 inherited methods) was never set. +ISSUE-127: delete_collection / delete_digital_product pre-filled a body dict, so the shared helper +silently ignored cascade=True. +""" +import ast +import glob +import json +import os + +import pytest + +from pyegeria.omvs.collection_manager import CollectionManager +from pyegeria.omvs.digital_business import DigitalBusiness +from pyegeria.omvs.glossary_manager import GlossaryManager +from pyegeria.omvs.product_manager import ProductManager + +OMVS_DIR = os.path.join(os.path.dirname(__file__), "..", "..", "pyegeria", "omvs") + + +def test_every_omvs_subclass_runs_its_base_constructor(): + """Structural guard for the whole bug class: a client that derives from another OMVS client but whose + __init__ never calls that client's __init__ silently misses the attributes it sets.""" + classes = {} + for path in sorted(glob.glob(os.path.join(OMVS_DIR, "*.py"))): + for node in ast.parse(open(path).read()).body: + if not isinstance(node, ast.ClassDef): + continue + bases = [b.id for b in node.bases if isinstance(b, ast.Name)] + init = next((m for m in node.body if isinstance(m, ast.FunctionDef) and m.name == "__init__"), None) + called = set() + if init: + for call in ast.walk(init): + if isinstance(call, ast.Call) and isinstance(call.func, ast.Attribute) and call.func.attr == "__init__": + owner = call.func.value + called.add(owner.id if isinstance(owner, ast.Name) else "super()") + classes[node.name] = (bases, init is not None, called) + + offenders = [f"{name} (skips {base}.__init__)" + for name, (bases, has_init, called) in classes.items() if has_init + for base in bases + if base in classes and base != "ServerClient" and base not in called and "super()" not in called] + assert offenders == [] + + +@pytest.mark.parametrize("cls", [CollectionManager, GlossaryManager, ProductManager, DigitalBusiness]) +def test_collection_manager_subclasses_have_the_collection_roots(cls, monkeypatch): + monkeypatch.setattr(cls, "check_connection", lambda self: "test") + client = cls("view-server", "https://localhost:1", "user", "pwd") + + assert client.collection_command_root.endswith("/api/open-metadata/collection-manager/collections") + assert client.metadata_expert_command_root.endswith("/api/open-metadata/metadata-expert") + + +def test_each_subclass_keeps_its_own_root(monkeypatch): + monkeypatch.setattr(ProductManager, "check_connection", lambda self: "test") + monkeypatch.setattr(DigitalBusiness, "check_connection", lambda self: "test") + + assert ProductManager("v", "https://localhost:1", "u", "p").product_manager_command_root.endswith("/product-manager") + assert DigitalBusiness("v", "https://localhost:1", "u", "p").digital_business_command_root.endswith("/digital-business") + + +# --- ISSUE-127 ------------------------------------------------------------------------------ + +class _Response: + def json(self): + return {"relatedHTTPCode": 200} + + +def _capture(cls, monkeypatch): + monkeypatch.setattr(cls, "check_connection", lambda self: "test") + client = cls("view-server", "https://localhost:1", "user", "pwd") + calls: list = [] + + async def fake_make_request(method, url, payload=None, *args, **kwargs): + calls.append({"url": url, "body": json.loads(payload) if isinstance(payload, str) else payload}) + return _Response() + + client._async_make_request = fake_make_request + return client, calls + + +@pytest.mark.asyncio +@pytest.mark.parametrize("cls, method, tail", [ + (CollectionManager, "_async_delete_collection", "/collections/g1/delete"), + (ProductManager, "_async_delete_collection", "/collections/g1/delete"), # inherited; was broken by ISSUE-126 + (DigitalBusiness, "_async_delete_collection", "/collections/g1/delete"), # likewise + (ProductManager, "_async_delete_digital_product", "/collections/g1/delete"), +]) +async def test_cascade_true_reaches_the_server(cls, method, tail, monkeypatch): + client, calls = _capture(cls, monkeypatch) + + await getattr(client, method)("g1", cascade=True) + + assert calls[0]["url"].endswith(tail) + assert calls[0]["body"]["cascadeDelete"] is True + assert calls[0]["body"]["class"] == "DeleteElementRequestBody" + + +@pytest.mark.asyncio +@pytest.mark.parametrize("method", ["_async_delete_collection", "_async_delete_digital_product"]) +async def test_cascade_default_does_not_cascade(method, monkeypatch): + client, calls = _capture(ProductManager, monkeypatch) + + await getattr(client, method)("g1") + + assert calls[0]["body"].get("cascadeDelete") in (None, False) + + +@pytest.mark.asyncio +async def test_an_explicit_body_is_still_honoured(monkeypatch): + client, calls = _capture(CollectionManager, monkeypatch) + body = {"class": "DeleteElementRequestBody", "cascadeDelete": True, "deleteMethod": "PURGE"} + + await client._async_delete_collection("g1", body=body) + + assert calls[0]["body"]["cascadeDelete"] is True and calls[0]["body"]["deleteMethod"] == "PURGE"