From 3ef8a2dc0d2662f433f6f4deb42276418c3f5c75 Mon Sep 17 00:00:00 2001 From: Sivakami Sivaraman Date: Wed, 27 May 2026 18:36:11 +0530 Subject: [PATCH 01/22] Add shared module utility support --- plugins/module_utils/common/data.py | 76 ++++++++++++++++++++++++ plugins/module_utils/models/base.py | 7 ++- plugins/module_utils/nd_state_machine.py | 35 +++++++---- plugins/module_utils/utils.py | 22 +++++-- 4 files changed, 124 insertions(+), 16 deletions(-) create mode 100644 plugins/module_utils/common/data.py diff --git a/plugins/module_utils/common/data.py b/plugins/module_utils/common/data.py new file mode 100644 index 000000000..5818bac09 --- /dev/null +++ b/plugins/module_utils/common/data.py @@ -0,0 +1,76 @@ +# -*- coding: utf-8 -*- + +# Copyright: (c) 2026, Sivakami Sivaraman + +# GNU General Public License v3.0+ (see LICENSE or https://www.gnu.org/licenses/gpl-3.0.txt) + +from __future__ import absolute_import, annotations, division, print_function + +import ast +import json +from typing import Any, Optional + + +def get_params(source: Any) -> dict[str, Any]: + """Return a mutable params mapping from either module.params or a raw params dict.""" + if isinstance(source, dict): + return source + + params = getattr(source, "params", None) + if isinstance(params, dict): + return params + + return {} + + +def loads_maybe_json(value: Any) -> Any: + """Parse JSON or Python-literal strings while leaving parsed values unchanged.""" + if isinstance(value, (dict, list)): + return value + if value is None: + return None + + text = str(value).strip() + if not text: + return None + + try: + return json.loads(text) + except Exception: + try: + return ast.literal_eval(text) + except Exception: + return None + + +def coerce_dict_list(data: Any, list_keys: tuple[str, ...] = ("DATA", "data", "items")) -> list[dict[str, Any]]: + """Return a list containing only dict items from common controller response shapes.""" + if isinstance(data, list): + return [item for item in data if isinstance(item, dict)] + + if isinstance(data, dict): + for key in list_keys: + value = data.get(key) + if isinstance(value, list): + return [item for item in value if isinstance(item, dict)] + + return [] + + +def copy_dict_items(items: Any) -> list[dict[str, Any]]: + """Copy a list of dict-like or pydantic-like objects into plain dicts.""" + copied = [] + for item in items or []: + if isinstance(item, dict): + copied.append(dict(item)) + elif hasattr(item, "model_dump"): + copied.append(item.model_dump(by_alias=False, exclude_none=True)) + return copied + + +def try_int(value: Any) -> Optional[int]: + """Best-effort integer conversion.""" + try: + return int(value) + except (TypeError, ValueError): + return None diff --git a/plugins/module_utils/models/base.py b/plugins/module_utils/models/base.py index 57689c3a6..314b787df 100644 --- a/plugins/module_utils/models/base.py +++ b/plugins/module_utils/models/base.py @@ -204,11 +204,14 @@ def get_diff(self, other: "NDBaseModel", exclude_unset: bool = False) -> bool: exclude_unset: When True, only compare fields explicitly set in ``other`` (via Pydantic's ``exclude_unset``). This prevents default values from triggering false diffs during merge - operations. + operations. List elements are matched one-directionally so + that an existing item with extra fields (e.g. ``deploy``) does + not trigger a spurious diff when the proposed item omits those + fields. """ self_data = self.to_diff_dict() other_data = other.to_diff_dict(exclude_unset=exclude_unset) - return issubset(other_data, self_data) + return issubset(other_data, self_data, allow_superset=exclude_unset) def merge(self, other: "NDBaseModel") -> "NDBaseModel": """ diff --git a/plugins/module_utils/nd_state_machine.py b/plugins/module_utils/nd_state_machine.py index ae010ead4..a1e938dac 100644 --- a/plugins/module_utils/nd_state_machine.py +++ b/plugins/module_utils/nd_state_machine.py @@ -15,7 +15,6 @@ from ansible_collections.cisco.nd.plugins.module_utils.orchestrators.types import ResponseType from ansible_collections.cisco.nd.plugins.module_utils.rest.response_handler_nd import ResponseHandler from ansible_collections.cisco.nd.plugins.module_utils.rest.rest_send import RestSend -from ansible_collections.cisco.nd.plugins.module_utils.rest.results import Results from ansible_collections.cisco.nd.plugins.module_utils.rest.sender_nd import Sender @@ -34,25 +33,24 @@ def __init__(self, module: AnsibleModule, model_orchestrator: type[NDBaseOrchest sender = Sender() sender.ansible_module = self.module - rest_send_params = dict(self.module.params) - rest_send_params["check_mode"] = self.module.check_mode - self.rest_send = RestSend(rest_send_params) + self.rest_send = RestSend( + { + "check_mode": self.module.check_mode, + "state": self.module.params.get("state"), + } + ) self.rest_send.sender = sender self.rest_send.response_handler = ResponseHandler() # Operation tracking self.output = NDOutput(output_level=module.params.get("output_level", "normal")) - self.results = Results() - self.results.state = self.module.params.get("state", "") - self.results.check_mode = self.module.check_mode # Configuration # Accept either an orchestrator instance or a class. if isinstance(model_orchestrator, type) and issubclass(model_orchestrator, NDBaseOrchestrator): - self.model_orchestrator = model_orchestrator(rest_send=self.rest_send, results=self.results) + self.model_orchestrator = model_orchestrator(rest_send=self.rest_send) elif isinstance(model_orchestrator, NDBaseOrchestrator): self.model_orchestrator = model_orchestrator - self.model_orchestrator.results = self.results else: raise NDStateMachineError(f"model_orchestrator must be an NDBaseOrchestrator class or instance. Got: {type(model_orchestrator)}") @@ -68,6 +66,10 @@ def __init__(self, module: AnsibleModule, model_orchestrator: type[NDBaseOrchest # Initialize collections try: response_data = self.model_orchestrator.query_all() + config_data = self.module.params.get("config", []) + normalize_config = getattr(self.model_orchestrator, "normalize_proposed_config", None) + if callable(normalize_config): + config_data = normalize_config(config=config_data, current=response_data, state=self.state) # State of configuration objects in ND before change execution self.before = NDConfigCollection.from_api_response(response_data=response_data, model_class=self.model_class) # State of current configuration objects in ND during change execution @@ -75,7 +77,7 @@ def __init__(self, module: AnsibleModule, model_orchestrator: type[NDBaseOrchest # Ongoing collection of configuration objects that were changed self.sent = NDConfigCollection(model_class=self.model_class) # Collection of configuration objects given by user - self.proposed = NDConfigCollection.from_ansible_config(data=self.module.params.get("config", []), model_class=self.model_class) + self.proposed = NDConfigCollection.from_ansible_config(data=config_data, model_class=self.model_class) self.output.assign(after=self.existing, before=self.before, proposed=self.proposed) @@ -90,6 +92,9 @@ def manage_state(self) -> None: if self.state in ["merged", "replaced", "overridden"]: self._manage_create_update_state() + if self.state == "replaced": + self._manage_replace_deletions() + if self.state == "overridden": self._manage_override_deletions() @@ -194,6 +199,15 @@ def _manage_override_deletions(self) -> None: items_to_delete = [existing_item for identifier in diff_identifiers if (existing_item := self.existing.get(identifier)) is not None] self._delete_items(items_to_delete) + def _manage_replace_deletions(self) -> None: + """Allow orchestrators to scope replaced-state child deletions.""" + get_replaced_deletion_items = getattr(self.model_orchestrator, "get_replaced_deletion_items", None) + if not callable(get_replaced_deletion_items): + return + + items_to_delete = get_replaced_deletion_items(before=self.before, proposed=self.proposed, existing=self.existing) + self._delete_items(items_to_delete or []) + def _manage_delete_state(self) -> None: """Handle deleted state.""" items_to_delete = [ @@ -216,6 +230,7 @@ def _delete_items(self, items: list[NDBaseModel]) -> None: # Batch remove from collection (single index rebuild) keys_to_delete = [item.get_identifier_value() for item in items] self.existing.delete_many(keys_to_delete) + self.sent.add_many(items) # Log deletion self.output.assign(after=self.existing) diff --git a/plugins/module_utils/utils.py b/plugins/module_utils/utils.py index b6757ccdc..46b2f44fd 100644 --- a/plugins/module_utils/utils.py +++ b/plugins/module_utils/utils.py @@ -37,8 +37,18 @@ def sanitize_dict(dict_to_sanitize, keys=None, values=None, recursive=True, remo return result -def issubset(subset: Any, superset: Any) -> bool: - """Check if subset is contained in superset.""" +def issubset(subset: Any, superset: Any, allow_superset: bool = False) -> bool: + """Check if subset is contained in superset. + + Args: + subset: The value to check. + superset: The value to check against. + allow_superset: When True, list element matching is one-directional: + an element in ``subset`` is considered matched when it is a subset + of a candidate in ``superset``, even if the candidate has + additional keys. When False (default) both directions are + required, which is equivalent to equality for lists of dicts. + """ if type(subset) is not type(superset): return False @@ -50,7 +60,11 @@ def issubset(subset: Any, superset: Any) -> bool: remaining = list(superset) for item in subset: for index, candidate in enumerate(remaining): - if issubset(item, candidate) and issubset(candidate, item): + if allow_superset: + match = issubset(item, candidate, allow_superset=True) + else: + match = issubset(item, candidate) and issubset(candidate, item) + if match: del remaining[index] break else: @@ -65,7 +79,7 @@ def issubset(subset: Any, superset: Any) -> bool: if key not in superset: return False - if not issubset(value, superset[key]): + if not issubset(value, superset[key], allow_superset=allow_superset): return False return True From 7416d663b1fe10a4113672d21f3fd440aff78043 Mon Sep 17 00:00:00 2001 From: Sivakami Sivaraman Date: Mon, 1 Jun 2026 15:48:10 +0530 Subject: [PATCH 02/22] Addressing review comments and adding UT --- plugins/module_utils/common/data.py | 47 +++++++++---- plugins/module_utils/nd_state_machine.py | 2 +- tests/unit/module_utils/test_common_data.py | 77 +++++++++++++++++++++ 3 files changed, 110 insertions(+), 16 deletions(-) create mode 100644 tests/unit/module_utils/test_common_data.py diff --git a/plugins/module_utils/common/data.py b/plugins/module_utils/common/data.py index 5818bac09..ce4b61cb2 100644 --- a/plugins/module_utils/common/data.py +++ b/plugins/module_utils/common/data.py @@ -8,7 +8,10 @@ import ast import json -from typing import Any, Optional +from typing import Any, Callable, Iterable, Mapping, Optional, Sequence + +Parser = Callable[[str], Any] +DEFAULT_VALUE_PARSERS: tuple[Parser, ...] = (json.loads, ast.literal_eval) def get_params(source: Any) -> dict[str, Any]: @@ -23,28 +26,42 @@ def get_params(source: Any) -> dict[str, Any]: return {} -def loads_maybe_json(value: Any) -> Any: - """Parse JSON or Python-literal strings while leaving parsed values unchanged.""" +def parse_value(value: Any, parsers: Sequence[Parser] = DEFAULT_VALUE_PARSERS, default: Any = None) -> Any: + """Parse a serialized value with the first parser that accepts it. + + Dicts and lists are returned unchanged because callers often pass values + that were already decoded by the controller client. Empty, invalid, or + unprintable values return ``default``. + """ if isinstance(value, (dict, list)): return value if value is None: - return None - - text = str(value).strip() - if not text: - return None + return default try: - return json.loads(text) + text = str(value).strip() except Exception: + return default + + if not text: + return default + + for parser in parsers: try: - return ast.literal_eval(text) + return parser(text) except Exception: - return None + continue + + return default + +def coerce_dict_list(data: Any, list_keys: Sequence[str] = ("DATA", "data", "items")) -> list[dict[str, Any]]: + """Return dict items from common shallow controller response shapes. -def coerce_dict_list(data: Any, list_keys: tuple[str, ...] = ("DATA", "data", "items")) -> list[dict[str, Any]]: - """Return a list containing only dict items from common controller response shapes.""" + This intentionally checks only the top-level value or one configured + top-level wrapper key. Deeper response shapes should be handled by the + caller because those paths are resource-specific. + """ if isinstance(data, list): return [item for item in data if isinstance(item, dict)] @@ -57,11 +74,11 @@ def coerce_dict_list(data: Any, list_keys: tuple[str, ...] = ("DATA", "data", "i return [] -def copy_dict_items(items: Any) -> list[dict[str, Any]]: +def copy_dict_items(items: Optional[Iterable[Any]]) -> list[dict[str, Any]]: """Copy a list of dict-like or pydantic-like objects into plain dicts.""" copied = [] for item in items or []: - if isinstance(item, dict): + if isinstance(item, Mapping): copied.append(dict(item)) elif hasattr(item, "model_dump"): copied.append(item.model_dump(by_alias=False, exclude_none=True)) diff --git a/plugins/module_utils/nd_state_machine.py b/plugins/module_utils/nd_state_machine.py index a1e938dac..a7be0d101 100644 --- a/plugins/module_utils/nd_state_machine.py +++ b/plugins/module_utils/nd_state_machine.py @@ -66,7 +66,7 @@ def __init__(self, module: AnsibleModule, model_orchestrator: type[NDBaseOrchest # Initialize collections try: response_data = self.model_orchestrator.query_all() - config_data = self.module.params.get("config", []) + config_data = self.module.params.get("config") or [] normalize_config = getattr(self.model_orchestrator, "normalize_proposed_config", None) if callable(normalize_config): config_data = normalize_config(config=config_data, current=response_data, state=self.state) diff --git a/tests/unit/module_utils/test_common_data.py b/tests/unit/module_utils/test_common_data.py new file mode 100644 index 000000000..7e6b9ce32 --- /dev/null +++ b/tests/unit/module_utils/test_common_data.py @@ -0,0 +1,77 @@ +# -*- coding: utf-8 -*- + +# Copyright: (c) 2026, Sivakami Sivaraman + +# GNU General Public License v3.0+ (see LICENSE or https://www.gnu.org/licenses/gpl-3.0.txt) + +from __future__ import absolute_import, annotations, division, print_function + +from ansible_collections.cisco.nd.plugins.module_utils.common.data import ( + coerce_dict_list, + copy_dict_items, + get_params, + parse_value, + try_int, +) + + +class _ModuleLike: + def __init__(self, params: dict[str, object]) -> None: + self.params = params + + +class _ModelLike: + def model_dump(self, by_alias: bool = False, exclude_none: bool = False) -> dict[str, bool]: + return { + "by_alias": by_alias, + "exclude_none": exclude_none, + } + + +def test_common_data_get_params_accepts_dict_and_module_like_object(): + params = {"state": "merged"} + + assert get_params(params) is params + assert get_params(_ModuleLike(params)) is params + assert get_params(object()) == {} + + +class _BrokenStr: + def __str__(self) -> str: + raise RuntimeError("cannot stringify") + + +def test_common_data_parse_value_supports_json_and_python_literal_strings(): + assert parse_value('{"DATA": [{"name": "BLUE"}]}') == {"DATA": [{"name": "BLUE"}]} + assert parse_value("{'DATA': [{'name': 'BLUE'}]}") == {"DATA": [{"name": "BLUE"}]} + assert parse_value({"already": "parsed"}) == {"already": "parsed"} + assert parse_value("") is None + assert parse_value("", default="") == "" + assert parse_value("not-json") is None + assert parse_value(_BrokenStr()) is None + + +def test_common_data_coerce_dict_list_handles_controller_response_shapes(): + assert coerce_dict_list([{"a": 1}, "skip", {"b": 2}]) == [{"a": 1}, {"b": 2}] + assert coerce_dict_list({"DATA": [{"a": 1}, 2]}) == [{"a": 1}] + assert coerce_dict_list({"vrfs": [{"name": "BLUE"}]}, list_keys=("vrfs", "DATA")) == [{"name": "BLUE"}] + assert coerce_dict_list({"DATA": {"not": "a list"}}) == [] + + +def test_common_data_copy_dict_items_copies_dicts_and_model_dump_items(): + source = [{"interface": "Ethernet1/1"}, _ModelLike(), object()] + + copied = copy_dict_items(source) + + assert copied == [ + {"interface": "Ethernet1/1"}, + {"by_alias": False, "exclude_none": True}, + ] + assert copied[0] is not source[0] + + +def test_common_data_try_int_returns_none_for_invalid_values(): + assert try_int("42") == 42 + assert try_int(7) == 7 + assert try_int(None) is None + assert try_int("not-int") is None From 76042ba4dbd12d1100a9271f1a715463f3a9086c Mon Sep 17 00:00:00 2001 From: Sivakami Sivaraman Date: Thu, 4 Jun 2026 11:09:11 +0530 Subject: [PATCH 03/22] Restored the rest_send params --- plugins/module_utils/nd_state_machine.py | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/plugins/module_utils/nd_state_machine.py b/plugins/module_utils/nd_state_machine.py index a7be0d101..ee7fec17a 100644 --- a/plugins/module_utils/nd_state_machine.py +++ b/plugins/module_utils/nd_state_machine.py @@ -33,12 +33,9 @@ def __init__(self, module: AnsibleModule, model_orchestrator: type[NDBaseOrchest sender = Sender() sender.ansible_module = self.module - self.rest_send = RestSend( - { - "check_mode": self.module.check_mode, - "state": self.module.params.get("state"), - } - ) + rest_send_params = dict(self.module.params) + rest_send_params["check_mode"] = self.module.check_mode + self.rest_send = RestSend(rest_send_params) self.rest_send.sender = sender self.rest_send.response_handler = ResponseHandler() From 24ba1f42c7e5ef6ea1646f6a8ba8c105f1d043d4 Mon Sep 17 00:00:00 2001 From: Sivakami Sivaraman Date: Mon, 8 Jun 2026 13:35:07 +0530 Subject: [PATCH 04/22] Align common files with vrf lite branch --- plugins/module_utils/common/data.py | 93 ------------------------ plugins/module_utils/nd_state_machine.py | 25 ++----- 2 files changed, 7 insertions(+), 111 deletions(-) delete mode 100644 plugins/module_utils/common/data.py diff --git a/plugins/module_utils/common/data.py b/plugins/module_utils/common/data.py deleted file mode 100644 index ce4b61cb2..000000000 --- a/plugins/module_utils/common/data.py +++ /dev/null @@ -1,93 +0,0 @@ -# -*- coding: utf-8 -*- - -# Copyright: (c) 2026, Sivakami Sivaraman - -# GNU General Public License v3.0+ (see LICENSE or https://www.gnu.org/licenses/gpl-3.0.txt) - -from __future__ import absolute_import, annotations, division, print_function - -import ast -import json -from typing import Any, Callable, Iterable, Mapping, Optional, Sequence - -Parser = Callable[[str], Any] -DEFAULT_VALUE_PARSERS: tuple[Parser, ...] = (json.loads, ast.literal_eval) - - -def get_params(source: Any) -> dict[str, Any]: - """Return a mutable params mapping from either module.params or a raw params dict.""" - if isinstance(source, dict): - return source - - params = getattr(source, "params", None) - if isinstance(params, dict): - return params - - return {} - - -def parse_value(value: Any, parsers: Sequence[Parser] = DEFAULT_VALUE_PARSERS, default: Any = None) -> Any: - """Parse a serialized value with the first parser that accepts it. - - Dicts and lists are returned unchanged because callers often pass values - that were already decoded by the controller client. Empty, invalid, or - unprintable values return ``default``. - """ - if isinstance(value, (dict, list)): - return value - if value is None: - return default - - try: - text = str(value).strip() - except Exception: - return default - - if not text: - return default - - for parser in parsers: - try: - return parser(text) - except Exception: - continue - - return default - - -def coerce_dict_list(data: Any, list_keys: Sequence[str] = ("DATA", "data", "items")) -> list[dict[str, Any]]: - """Return dict items from common shallow controller response shapes. - - This intentionally checks only the top-level value or one configured - top-level wrapper key. Deeper response shapes should be handled by the - caller because those paths are resource-specific. - """ - if isinstance(data, list): - return [item for item in data if isinstance(item, dict)] - - if isinstance(data, dict): - for key in list_keys: - value = data.get(key) - if isinstance(value, list): - return [item for item in value if isinstance(item, dict)] - - return [] - - -def copy_dict_items(items: Optional[Iterable[Any]]) -> list[dict[str, Any]]: - """Copy a list of dict-like or pydantic-like objects into plain dicts.""" - copied = [] - for item in items or []: - if isinstance(item, Mapping): - copied.append(dict(item)) - elif hasattr(item, "model_dump"): - copied.append(item.model_dump(by_alias=False, exclude_none=True)) - return copied - - -def try_int(value: Any) -> Optional[int]: - """Best-effort integer conversion.""" - try: - return int(value) - except (TypeError, ValueError): - return None diff --git a/plugins/module_utils/nd_state_machine.py b/plugins/module_utils/nd_state_machine.py index ee7fec17a..6289258a3 100644 --- a/plugins/module_utils/nd_state_machine.py +++ b/plugins/module_utils/nd_state_machine.py @@ -15,6 +15,7 @@ from ansible_collections.cisco.nd.plugins.module_utils.orchestrators.types import ResponseType from ansible_collections.cisco.nd.plugins.module_utils.rest.response_handler_nd import ResponseHandler from ansible_collections.cisco.nd.plugins.module_utils.rest.rest_send import RestSend +from ansible_collections.cisco.nd.plugins.module_utils.rest.results import Results from ansible_collections.cisco.nd.plugins.module_utils.rest.sender_nd import Sender @@ -41,13 +42,17 @@ def __init__(self, module: AnsibleModule, model_orchestrator: type[NDBaseOrchest # Operation tracking self.output = NDOutput(output_level=module.params.get("output_level", "normal")) + self.results = Results() + self.results.state = self.module.params.get("state", "") + self.results.check_mode = self.module.check_mode # Configuration # Accept either an orchestrator instance or a class. if isinstance(model_orchestrator, type) and issubclass(model_orchestrator, NDBaseOrchestrator): - self.model_orchestrator = model_orchestrator(rest_send=self.rest_send) + self.model_orchestrator = model_orchestrator(rest_send=self.rest_send, results=self.results) elif isinstance(model_orchestrator, NDBaseOrchestrator): self.model_orchestrator = model_orchestrator + self.model_orchestrator.results = self.results else: raise NDStateMachineError(f"model_orchestrator must be an NDBaseOrchestrator class or instance. Got: {type(model_orchestrator)}") @@ -63,10 +68,6 @@ def __init__(self, module: AnsibleModule, model_orchestrator: type[NDBaseOrchest # Initialize collections try: response_data = self.model_orchestrator.query_all() - config_data = self.module.params.get("config") or [] - normalize_config = getattr(self.model_orchestrator, "normalize_proposed_config", None) - if callable(normalize_config): - config_data = normalize_config(config=config_data, current=response_data, state=self.state) # State of configuration objects in ND before change execution self.before = NDConfigCollection.from_api_response(response_data=response_data, model_class=self.model_class) # State of current configuration objects in ND during change execution @@ -74,7 +75,7 @@ def __init__(self, module: AnsibleModule, model_orchestrator: type[NDBaseOrchest # Ongoing collection of configuration objects that were changed self.sent = NDConfigCollection(model_class=self.model_class) # Collection of configuration objects given by user - self.proposed = NDConfigCollection.from_ansible_config(data=config_data, model_class=self.model_class) + self.proposed = NDConfigCollection.from_ansible_config(data=self.module.params.get("config", []), model_class=self.model_class) self.output.assign(after=self.existing, before=self.before, proposed=self.proposed) @@ -89,9 +90,6 @@ def manage_state(self) -> None: if self.state in ["merged", "replaced", "overridden"]: self._manage_create_update_state() - if self.state == "replaced": - self._manage_replace_deletions() - if self.state == "overridden": self._manage_override_deletions() @@ -196,15 +194,6 @@ def _manage_override_deletions(self) -> None: items_to_delete = [existing_item for identifier in diff_identifiers if (existing_item := self.existing.get(identifier)) is not None] self._delete_items(items_to_delete) - def _manage_replace_deletions(self) -> None: - """Allow orchestrators to scope replaced-state child deletions.""" - get_replaced_deletion_items = getattr(self.model_orchestrator, "get_replaced_deletion_items", None) - if not callable(get_replaced_deletion_items): - return - - items_to_delete = get_replaced_deletion_items(before=self.before, proposed=self.proposed, existing=self.existing) - self._delete_items(items_to_delete or []) - def _manage_delete_state(self) -> None: """Handle deleted state.""" items_to_delete = [ From 1fc43298b558139b082d6b7d2a7e9a27fca16ef8 Mon Sep 17 00:00:00 2001 From: Sivakami Sivaraman Date: Wed, 10 Jun 2026 19:08:56 +0530 Subject: [PATCH 05/22] Removing UT as the source file is removed. --- tests/unit/module_utils/test_common_data.py | 77 --------------------- 1 file changed, 77 deletions(-) delete mode 100644 tests/unit/module_utils/test_common_data.py diff --git a/tests/unit/module_utils/test_common_data.py b/tests/unit/module_utils/test_common_data.py deleted file mode 100644 index 7e6b9ce32..000000000 --- a/tests/unit/module_utils/test_common_data.py +++ /dev/null @@ -1,77 +0,0 @@ -# -*- coding: utf-8 -*- - -# Copyright: (c) 2026, Sivakami Sivaraman - -# GNU General Public License v3.0+ (see LICENSE or https://www.gnu.org/licenses/gpl-3.0.txt) - -from __future__ import absolute_import, annotations, division, print_function - -from ansible_collections.cisco.nd.plugins.module_utils.common.data import ( - coerce_dict_list, - copy_dict_items, - get_params, - parse_value, - try_int, -) - - -class _ModuleLike: - def __init__(self, params: dict[str, object]) -> None: - self.params = params - - -class _ModelLike: - def model_dump(self, by_alias: bool = False, exclude_none: bool = False) -> dict[str, bool]: - return { - "by_alias": by_alias, - "exclude_none": exclude_none, - } - - -def test_common_data_get_params_accepts_dict_and_module_like_object(): - params = {"state": "merged"} - - assert get_params(params) is params - assert get_params(_ModuleLike(params)) is params - assert get_params(object()) == {} - - -class _BrokenStr: - def __str__(self) -> str: - raise RuntimeError("cannot stringify") - - -def test_common_data_parse_value_supports_json_and_python_literal_strings(): - assert parse_value('{"DATA": [{"name": "BLUE"}]}') == {"DATA": [{"name": "BLUE"}]} - assert parse_value("{'DATA': [{'name': 'BLUE'}]}") == {"DATA": [{"name": "BLUE"}]} - assert parse_value({"already": "parsed"}) == {"already": "parsed"} - assert parse_value("") is None - assert parse_value("", default="") == "" - assert parse_value("not-json") is None - assert parse_value(_BrokenStr()) is None - - -def test_common_data_coerce_dict_list_handles_controller_response_shapes(): - assert coerce_dict_list([{"a": 1}, "skip", {"b": 2}]) == [{"a": 1}, {"b": 2}] - assert coerce_dict_list({"DATA": [{"a": 1}, 2]}) == [{"a": 1}] - assert coerce_dict_list({"vrfs": [{"name": "BLUE"}]}, list_keys=("vrfs", "DATA")) == [{"name": "BLUE"}] - assert coerce_dict_list({"DATA": {"not": "a list"}}) == [] - - -def test_common_data_copy_dict_items_copies_dicts_and_model_dump_items(): - source = [{"interface": "Ethernet1/1"}, _ModelLike(), object()] - - copied = copy_dict_items(source) - - assert copied == [ - {"interface": "Ethernet1/1"}, - {"by_alias": False, "exclude_none": True}, - ] - assert copied[0] is not source[0] - - -def test_common_data_try_int_returns_none_for_invalid_values(): - assert try_int("42") == 42 - assert try_int(7) == 7 - assert try_int(None) is None - assert try_int("not-int") is None From d5792334bcaa3dbf367c28bca20b4cde82393194 Mon Sep 17 00:00:00 2001 From: Sivakami Sivaraman Date: Mon, 15 Jun 2026 20:16:54 +0530 Subject: [PATCH 06/22] Addressing review comments --- plugins/module_utils/models/base.py | 2 +- plugins/module_utils/nd_state_machine.py | 30 ++- plugins/module_utils/utils.py | 60 ++++- tests/unit/module_utils/test_utils.py | 280 +++++++++++++++++++++++ 4 files changed, 353 insertions(+), 19 deletions(-) create mode 100644 tests/unit/module_utils/test_utils.py diff --git a/plugins/module_utils/models/base.py b/plugins/module_utils/models/base.py index 314b787df..4fb6f397e 100644 --- a/plugins/module_utils/models/base.py +++ b/plugins/module_utils/models/base.py @@ -204,7 +204,7 @@ def get_diff(self, other: "NDBaseModel", exclude_unset: bool = False) -> bool: exclude_unset: When True, only compare fields explicitly set in ``other`` (via Pydantic's ``exclude_unset``). This prevents default values from triggering false diffs during merge - operations. List elements are matched one-directionally so + operations. List elements are matched one-directionally so that an existing item with extra fields (e.g. ``deploy``) does not trigger a spurious diff when the proposed item omits those fields. diff --git a/plugins/module_utils/nd_state_machine.py b/plugins/module_utils/nd_state_machine.py index 6289258a3..c56b87f3e 100644 --- a/plugins/module_utils/nd_state_machine.py +++ b/plugins/module_utils/nd_state_machine.py @@ -167,19 +167,25 @@ def _manage_create_update_state(self) -> None: raise NDStateMachineError(error_msg) from e # Execute updates (always individual) + successfully_sent: List[NDBaseModel] = [] for item in items_to_update: - self._execute_operation(self.model_orchestrator.update, item, error_msg_prefix=f"Failed to update {item.get_identifier_value()}") + if self._execute_operation(self.model_orchestrator.update, item, error_msg_prefix=f"Failed to update {item.get_identifier_value()}"): + successfully_sent.append(item) # Execute creates (bulk or individual) if items_to_create: if self.supports_bulk_create: - self._execute_operation(self.model_orchestrator.create_bulk, items_to_create, error_msg_prefix="Failed to create in bulk") + if self._execute_operation(self.model_orchestrator.create_bulk, items_to_create, error_msg_prefix="Failed to create in bulk"): + successfully_sent.extend(items_to_create) else: for item in items_to_create: - self._execute_operation(self.model_orchestrator.create, item, error_msg_prefix=f"Failed to create {item.get_identifier_value()}") + if self._execute_operation(self.model_orchestrator.create, item, error_msg_prefix=f"Failed to create {item.get_identifier_value()}"): + successfully_sent.append(item) - # Mark as sent only after successful API operations - successfully_sent = items_to_update + items_to_create + # Mark as sent only after successful API operations. In check mode no + # API call is made, so nothing is marked as sent (consistent with + # _delete_items); the preview 'after' state is still reflected in + # self.existing, which is what drives 'changed'. if successfully_sent: self.sent.add_many(successfully_sent) @@ -207,16 +213,22 @@ def _delete_items(self, items: list[NDBaseModel]) -> None: return # Execute deletes (bulk or individual) + successfully_deleted: List[NDBaseModel] = [] if self.supports_bulk_delete: - self._execute_operation(self.model_orchestrator.delete_bulk, items, error_msg_prefix="Failed to delete in bulk") + if self._execute_operation(self.model_orchestrator.delete_bulk, items, error_msg_prefix="Failed to delete in bulk"): + successfully_deleted.extend(items) else: for item in items: - self._execute_operation(self.model_orchestrator.delete, item, error_msg_prefix=f"Failed to delete {item.get_identifier_value()}") + if self._execute_operation(self.model_orchestrator.delete, item, error_msg_prefix=f"Failed to delete {item.get_identifier_value()}"): + successfully_deleted.append(item) # Batch remove from collection (single index rebuild) - keys_to_delete = [item.get_identifier_value() for item in items] + # In check mode, update the preview state without marking anything as sent. + items_to_remove = items if self.check_mode else successfully_deleted + keys_to_delete = [item.get_identifier_value() for item in items_to_remove] self.existing.delete_many(keys_to_delete) - self.sent.add_many(items) + if successfully_deleted: + self.sent.add_many(successfully_deleted) # Log deletion self.output.assign(after=self.existing) diff --git a/plugins/module_utils/utils.py b/plugins/module_utils/utils.py index 46b2f44fd..21797fce5 100644 --- a/plugins/module_utils/utils.py +++ b/plugins/module_utils/utils.py @@ -5,7 +5,7 @@ from __future__ import absolute_import, annotations, division, print_function from copy import deepcopy -from typing import Any +from typing import Any, Dict, List, Set from ansible_collections.cisco.nd.plugins.module_utils.endpoints.v1.manage.manage_fabrics_actions_config_save import ( EpFabricConfigSavePost, @@ -37,17 +37,53 @@ def sanitize_dict(dict_to_sanitize, keys=None, values=None, recursive=True, remo return result +def _has_perfect_matching(adjacency: List[List[int]]) -> bool: + """Return True if every subset item can be matched to a distinct candidate. + + ``adjacency[i]`` holds the indices of the candidates that subset item ``i`` + can match. This solves the maximum bipartite matching problem with Kuhn's + augmenting-path algorithm so that a less-specific item never greedily + consumes a candidate that a more-specific item needs. + """ + # candidate index -> subset item index it is currently assigned to + match_to_item: Dict[int, int] = {} + + def _try_assign(item_index: int, visited: Set[int]) -> bool: + for candidate_index in adjacency[item_index]: + if candidate_index in visited: + continue + visited.add(candidate_index) + assigned_item = match_to_item.get(candidate_index) + # Candidate is free, or its current owner can be reassigned elsewhere. + if assigned_item is None or _try_assign(assigned_item, visited): + match_to_item[candidate_index] = item_index + return True + return False + + for item_index in range(len(adjacency)): + if not _try_assign(item_index, set()): + return False + return True + + def issubset(subset: Any, superset: Any, allow_superset: bool = False) -> bool: """Check if subset is contained in superset. + For dicts, only the non-``None`` keys of ``subset`` are compared; keys whose + value is ``None`` are ignored, and keys present only in ``superset`` are + allowed. For lists, both must be the same length and a one-to-one pairing of + elements must exist (matching is order-independent). + Args: subset: The value to check. superset: The value to check against. allow_superset: When True, list element matching is one-directional: an element in ``subset`` is considered matched when it is a subset of a candidate in ``superset``, even if the candidate has - additional keys. When False (default) both directions are - required, which is equivalent to equality for lists of dicts. + additional keys. When False (default) matching is bidirectional. + For lists of dicts this is equivalent to equality *after* ``None`` + -valued keys are dropped from both sides (it is not strict ``==`` + equality, because such keys are ignored). """ if type(subset) is not type(superset): return False @@ -57,19 +93,25 @@ def issubset(subset: Any, superset: Any, allow_superset: bool = False) -> bool: if len(subset) != len(superset): return False - remaining = list(superset) + # Build the bipartite adjacency: for each subset item, which + # candidates it can match. A full matching is then required so a + # less-specific item cannot greedily consume a candidate that a + # more-specific item needs (relevant under allow_superset=True). + adjacency: List[List[int]] = [] for item in subset: - for index, candidate in enumerate(remaining): + matches = [] + for index, candidate in enumerate(superset): if allow_superset: match = issubset(item, candidate, allow_superset=True) else: match = issubset(item, candidate) and issubset(candidate, item) if match: - del remaining[index] - break - else: + matches.append(index) + if not matches: return False - return True + adjacency.append(matches) + + return _has_perfect_matching(adjacency) return subset == superset for key, value in subset.items(): diff --git a/tests/unit/module_utils/test_utils.py b/tests/unit/module_utils/test_utils.py new file mode 100644 index 000000000..d0f5e8eae --- /dev/null +++ b/tests/unit/module_utils/test_utils.py @@ -0,0 +1,280 @@ +# -*- coding: utf-8 -*- + +# Copyright: (c) 2026, Cisco Systems, Inc. + +# GNU General Public License v3.0+ (see LICENSE or https://www.gnu.org/licenses/gpl-3.0.txt) + +""" +Unit tests for utils.py + +Tests the ``issubset`` helper (both the default bidirectional list matching and +the one-directional ``allow_superset=True`` matching) and ``NDBaseModel.get_diff`` +with list-valued fields under ``exclude_unset=True``. +""" + +# pylint: disable=protected-access + +from __future__ import absolute_import, annotations, division, print_function + +# pylint: disable=invalid-name +__metaclass__ = type +# pylint: enable=invalid-name + +from typing import Any, ClassVar, Dict, List, Literal, Optional + +import pytest +from ansible_collections.cisco.nd.plugins.module_utils.common.pydantic_compat import Field +from ansible_collections.cisco.nd.plugins.module_utils.models.base import NDBaseModel +from ansible_collections.cisco.nd.plugins.module_utils.utils import issubset + + +# ============================================================================= +# issubset - scalars +# ============================================================================= + + +@pytest.mark.parametrize( + "subset, superset, expected", + [ + (1, 1, True), + (1, 2, False), + ("a", "a", True), + ("a", "b", False), + (True, True, True), + (1, "1", False), # different types + (1.0, 1, False), # different types (float vs int) + ], +) +def test_issubset_scalars(subset, superset, expected): + """Scalar comparison is strict equality and type-sensitive.""" + assert issubset(subset, superset) is expected + + +# ============================================================================= +# issubset - dicts +# ============================================================================= + + +@pytest.mark.parametrize( + "subset, superset, expected", + [ + # Subset of keys present with matching values -> True + ({"a": 1}, {"a": 1, "b": 2}, True), + # Exact match -> True + ({"a": 1, "b": 2}, {"a": 1, "b": 2}, True), + # Missing key in superset -> False + ({"a": 1, "c": 3}, {"a": 1, "b": 2}, False), + # Value mismatch -> False + ({"a": 1}, {"a": 2}, False), + # None values in subset are ignored + ({"a": 1, "b": None}, {"a": 1}, True), + # Nested dict subset + ({"a": {"x": 1}}, {"a": {"x": 1, "y": 2}}, True), + # Nested dict mismatch + ({"a": {"x": 1}}, {"a": {"x": 2}}, False), + ], +) +def test_issubset_dicts(subset, superset, expected): + """Dict comparison checks each non-None key/value of subset.""" + assert issubset(subset, superset) is expected + + +def test_issubset_dicts_none_keys_not_strict_equality(): + """ + Bidirectional dict matching is NOT strict ``==`` equality: ``None``-valued + keys are skipped, so unequal dicts can still match in both directions. + This documents the behavior described in the ``issubset`` docstring. + """ + a = {"a": 1, "b": None} + b = {"a": 1} + + # Not equal as plain dicts ... + assert a != b + # ... yet issubset matches in both directions (b: None is ignored). + assert issubset(a, b) is True + assert issubset(b, a) is True + + +# ============================================================================= +# issubset - lists with allow_superset=False (default, bidirectional) +# +# This is the behavior deliberately added in PR #209: for lists of dicts the +# match is bidirectional, which is equivalent to equality. An element whose +# candidate carries extra keys does NOT match. +# ============================================================================= + + +@pytest.mark.parametrize( + "subset, superset, expected", + [ + # Equal lists -> True + ([1, 2, 3], [1, 2, 3], True), + # Order-independent matching -> True + ([3, 1, 2], [1, 2, 3], True), + # Different length -> False + ([1, 2], [1, 2, 3], False), + # Element missing -> False + ([1, 2, 4], [1, 2, 3], False), + # Lists of dicts, exact element match -> True + ([{"a": 1}], [{"a": 1}], True), + # Lists of dicts, candidate has EXTRA key -> False (bidirectional) + ([{"a": 1}], [{"a": 1, "b": 2}], False), + # Lists of dicts, subset element has extra key -> False + ([{"a": 1, "b": 2}], [{"a": 1}], False), + ], +) +def test_issubset_lists_bidirectional(subset, superset, expected): + """Default list matching is bidirectional (equivalent to equality).""" + assert issubset(subset, superset) is expected + + +# ============================================================================= +# issubset - lists with allow_superset=True (one-directional) +# +# New behavior introduced in this PR: an element in ``subset`` matches a +# candidate in ``superset`` when it is a subset of that candidate, even if the +# candidate has additional keys. +# ============================================================================= + + +@pytest.mark.parametrize( + "subset, superset, expected", + [ + # Candidate has EXTRA key -> now matches (one-directional) + ([{"a": 1}], [{"a": 1, "b": 2}], True), + # Multiple elements, each a subset of a distinct candidate -> True + ( + [{"a": 1}, {"c": 3}], + [{"a": 1, "b": 2}, {"c": 3, "d": 4}], + True, + ), + # Subset element with extra key not in candidate -> still False + ([{"a": 1, "z": 9}], [{"a": 1, "b": 2}], False), + # Length must still match + ([{"a": 1}], [{"a": 1, "b": 2}, {"c": 3}], False), + # Value mismatch -> False + ([{"a": 2}], [{"a": 1, "b": 2}], False), + ], +) +def test_issubset_lists_one_directional(subset, superset, expected): + """``allow_superset=True`` relaxes list matching to one-directional.""" + assert issubset(subset, superset, allow_superset=True) is expected + + +def test_issubset_one_directional_does_not_reuse_candidate(): + """Each candidate is consumed at most once during list matching.""" + # Two identical subset elements require two matching candidates. + subset = [{"a": 1}, {"a": 1}] + superset = [{"a": 1, "b": 2}, {"a": 1, "c": 3}] + assert issubset(subset, superset, allow_superset=True) is True + + # Only one candidate matches -> the second subset element fails. + subset = [{"a": 1}, {"a": 1}] + superset = [{"a": 1, "b": 2}, {"x": 9}] + assert issubset(subset, superset, allow_superset=True) is False + + +def test_issubset_type_mismatch_returns_false(): + """Mismatched top-level types short-circuit to False.""" + assert issubset({"a": 1}, [1, 2]) is False + assert issubset([1], {"a": 1}) is False + + +# ============================================================================= +# issubset - greedy-matching regression (bipartite matching) +# +# A less-specific subset item must not greedily consume a candidate that a +# more-specific item needs. These cases fail with first-match/greedy logic but +# pass with a proper maximum-matching solution. +# ============================================================================= + + +def test_issubset_one_directional_no_greedy_false_diff(): + """ + The less-specific ``{"a": 1}`` must yield the ``{"a": 1, "b": 2}`` candidate + to the more-specific ``{"a": 1, "b": 2}`` proposed item, matching instead + against ``{"a": 1, "b": 3}``. A valid pairing exists, so no diff. + """ + subset = [{"a": 1}, {"a": 1, "b": 2}] + superset = [{"a": 1, "b": 2}, {"a": 1, "b": 3}] + assert issubset(subset, superset, allow_superset=True) is True + + +def test_issubset_one_directional_no_greedy_reordered(): + """Order-independence: same data, subset elements swapped.""" + subset = [{"a": 1, "b": 2}, {"a": 1}] + superset = [{"a": 1, "b": 3}, {"a": 1, "b": 2}] + assert issubset(subset, superset, allow_superset=True) is True + + +def test_issubset_one_directional_no_valid_pairing_is_false(): + """When no perfect pairing exists, a diff is still correctly reported.""" + # Both proposed items demand b==2, but only one candidate has b==2. + subset = [{"a": 1, "b": 2}, {"a": 1, "b": 2}] + superset = [{"a": 1, "b": 2}, {"a": 1, "b": 3}] + assert issubset(subset, superset, allow_superset=True) is False + + +def test_issubset_bidirectional_no_greedy_false_diff(): + """ + Greedy matching can also misfire in the default bidirectional mode when + duplicate values are present. A perfect matching still exists here. + """ + subset = [{"a": 1}, {"a": 1}] + superset = [{"a": 1}, {"a": 1}] + assert issubset(subset, superset) is True + + +# ============================================================================= +# NDBaseModel.get_diff with list-valued fields under exclude_unset=True +# ============================================================================= + + +class _ListFieldModel(NDBaseModel): + """Minimal model with a list-valued field for diff testing.""" + + identifiers: ClassVar[Optional[List[str]]] = ["name"] + identifier_strategy: ClassVar[Literal["single", "composite", "hierarchical", "singleton"]] = "single" + + name: str = Field(alias="name") + members: Optional[List[Dict[str, Any]]] = Field(default=None, alias="members") + + +def test_get_diff_list_field_subset_with_extra_keys_no_diff(): + """ + With exclude_unset=True, an existing element carrying extra keys (e.g. a + controller-populated ``deploy`` flag) does not trigger a spurious diff when + the proposed element omits those keys. + """ + existing = _ListFieldModel(name="vrf1", members=[{"id": 1, "deploy": True}]) + proposed = _ListFieldModel(name="vrf1", members=[{"id": 1}]) + + # exclude_unset=True -> one-directional list match -> proposed is a subset. + assert existing.get_diff(proposed, exclude_unset=True) is True + + +def test_get_diff_list_field_extra_keys_triggers_diff_without_exclude_unset(): + """ + Without exclude_unset (default), list matching is bidirectional, so the + extra ``deploy`` key on the existing element produces a diff. + """ + existing = _ListFieldModel(name="vrf1", members=[{"id": 1, "deploy": True}]) + proposed = _ListFieldModel(name="vrf1", members=[{"id": 1}]) + + assert existing.get_diff(proposed) is False + + +def test_get_diff_list_field_value_change_triggers_diff(): + """A genuine value change in a list element is always a diff.""" + existing = _ListFieldModel(name="vrf1", members=[{"id": 1}]) + proposed = _ListFieldModel(name="vrf1", members=[{"id": 2}]) + + assert existing.get_diff(proposed, exclude_unset=True) is False + + +def test_get_diff_unset_list_field_ignored(): + """A list field not set on the proposed model is ignored under exclude_unset.""" + existing = _ListFieldModel(name="vrf1", members=[{"id": 1, "deploy": True}]) + proposed = _ListFieldModel(name="vrf1") # members not set + + assert existing.get_diff(proposed, exclude_unset=True) is True From 4f2ee419a1d62b13d3dbe5f80fba848bf665611b Mon Sep 17 00:00:00 2001 From: Sivakami Sivaraman Date: Mon, 15 Jun 2026 20:18:47 +0530 Subject: [PATCH 07/22] Refactored execute_operation --- plugins/module_utils/nd_state_machine.py | 64 +++++++++++++++--------- 1 file changed, 40 insertions(+), 24 deletions(-) diff --git a/plugins/module_utils/nd_state_machine.py b/plugins/module_utils/nd_state_machine.py index c56b87f3e..e4ed36dc7 100644 --- a/plugins/module_utils/nd_state_machine.py +++ b/plugins/module_utils/nd_state_machine.py @@ -105,17 +105,27 @@ def _execute_operation( *args: Any, error_msg_prefix: str = "Operation failed", **kwargs: Any, - ) -> ResponseType | None: - """Execute an API operation with standardized error handling.""" + ) -> bool: + """Execute an API operation with standardized error handling. + + Returns True if the operation completed without raising. In check mode + the API call is skipped but still reported as completed (True) so that + the previewed 'after' state can be built. Returns False only when the + operation raised and the error was suppressed via ``ignore_errors``. + + The orchestrator's return value is deliberately not used as the success + signal: some orchestrators defer the actual API call (queue-and-deploy) + and legitimately return None on success. + """ try: if not self.check_mode: - return operation(*args, **kwargs) - return None + operation(*args, **kwargs) + return True except Exception as e: error_msg = f"{error_msg_prefix}: {e}" if not self.ignore_errors: raise NDStateMachineError(error_msg) from e - return None + return False def _manage_create_update_state(self) -> None: """ @@ -166,8 +176,9 @@ def _manage_create_update_state(self) -> None: if not self.ignore_errors: raise NDStateMachineError(error_msg) from e - # Execute updates (always individual) - successfully_sent: List[NDBaseModel] = [] + # Execute updates (always individual). An operation that does not fail + # is counted as sent; check mode skips the API call but returns True. + successfully_sent: list[NDBaseModel] = [] for item in items_to_update: if self._execute_operation(self.model_orchestrator.update, item, error_msg_prefix=f"Failed to update {item.get_identifier_value()}"): successfully_sent.append(item) @@ -182,11 +193,11 @@ def _manage_create_update_state(self) -> None: if self._execute_operation(self.model_orchestrator.create, item, error_msg_prefix=f"Failed to create {item.get_identifier_value()}"): successfully_sent.append(item) - # Mark as sent only after successful API operations. In check mode no - # API call is made, so nothing is marked as sent (consistent with - # _delete_items); the preview 'after' state is still reflected in - # self.existing, which is what drives 'changed'. - if successfully_sent: + # Mark as sent only for items actually pushed to the controller. In + # check mode no API call is made, so nothing is marked as sent (avoids + # false deploy triggers); the previewed 'after' state is still reflected + # in self.existing (mutated above), which is what drives 'changed'. + if not self.check_mode and successfully_sent: self.sent.add_many(successfully_sent) # Log operation @@ -212,23 +223,28 @@ def _delete_items(self, items: list[NDBaseModel]) -> None: if not items: return - # Execute deletes (bulk or individual) - successfully_deleted: List[NDBaseModel] = [] + # Execute deletes (bulk or individual). An item is counted as deleted + # when the operation does not fail; check mode skips the API call but + # returns True so the deletion is still previewed. Items whose delete + # failed under ignore_errors are left in 'existing' so the reported + # 'after' state stays accurate. + deleted: list[NDBaseModel] = [] if self.supports_bulk_delete: if self._execute_operation(self.model_orchestrator.delete_bulk, items, error_msg_prefix="Failed to delete in bulk"): - successfully_deleted.extend(items) + deleted.extend(items) else: for item in items: if self._execute_operation(self.model_orchestrator.delete, item, error_msg_prefix=f"Failed to delete {item.get_identifier_value()}"): - successfully_deleted.append(item) - - # Batch remove from collection (single index rebuild) - # In check mode, update the preview state without marking anything as sent. - items_to_remove = items if self.check_mode else successfully_deleted - keys_to_delete = [item.get_identifier_value() for item in items_to_remove] - self.existing.delete_many(keys_to_delete) - if successfully_deleted: - self.sent.add_many(successfully_deleted) + deleted.append(item) + + # Batch remove from collection (single index rebuild). + self.existing.delete_many([item.get_identifier_value() for item in deleted]) + + # Mark as sent only for items actually pushed to the controller. In + # check mode no API call is made, so nothing is marked as sent (avoids + # false deploy triggers). + if not self.check_mode and deleted: + self.sent.add_many(deleted) # Log deletion self.output.assign(after=self.existing) From 75189dc67be7d054b9f6a07324fb5d21dd900fd0 Mon Sep 17 00:00:00 2001 From: Sivakami Sivaraman Date: Mon, 15 Jun 2026 23:30:32 +0530 Subject: [PATCH 08/22] Reusable helper API redesign --- plugins/module_utils/models/base.py | 16 ++++++---- plugins/module_utils/nd_config_collection.py | 7 +++-- plugins/module_utils/nd_state_machine.py | 6 ++-- tests/unit/module_utils/test_utils.py | 31 ++++++++++++++++++-- 4 files changed, 47 insertions(+), 13 deletions(-) diff --git a/plugins/module_utils/models/base.py b/plugins/module_utils/models/base.py index 4fb6f397e..4fe159d19 100644 --- a/plugins/module_utils/models/base.py +++ b/plugins/module_utils/models/base.py @@ -196,7 +196,7 @@ def to_diff_dict(self, **kwargs) -> Dict[str, Any]: **kwargs, ) - def get_diff(self, other: "NDBaseModel", exclude_unset: bool = False) -> bool: + def get_diff(self, other: "NDBaseModel", exclude_unset: bool = False, allow_superset: bool = False) -> bool: """Diff comparison. Args: @@ -204,14 +204,18 @@ def get_diff(self, other: "NDBaseModel", exclude_unset: bool = False) -> bool: exclude_unset: When True, only compare fields explicitly set in ``other`` (via Pydantic's ``exclude_unset``). This prevents default values from triggering false diffs during merge - operations. List elements are matched one-directionally so - that an existing item with extra fields (e.g. ``deploy``) does - not trigger a spurious diff when the proposed item omits those - fields. + operations. + allow_superset: When True, list elements are matched + one-directionally so that an existing item with extra fields + (e.g. ``deploy``) does not trigger a spurious diff when the + proposed item omits those fields. This is independent of + ``exclude_unset``: the former controls which of ``other``'s + fields are compared, while this controls how list elements are + matched. """ self_data = self.to_diff_dict() other_data = other.to_diff_dict(exclude_unset=exclude_unset) - return issubset(other_data, self_data, allow_superset=exclude_unset) + return issubset(other_data, self_data, allow_superset=allow_superset) def merge(self, other: "NDBaseModel") -> "NDBaseModel": """ diff --git a/plugins/module_utils/nd_config_collection.py b/plugins/module_utils/nd_config_collection.py index 8538bf261..767fd6eec 100644 --- a/plugins/module_utils/nd_config_collection.py +++ b/plugins/module_utils/nd_config_collection.py @@ -149,7 +149,7 @@ def delete_many(self, keys: List[IdentifierKey]) -> List[IdentifierKey]: # Diff Operations - def get_diff_config(self, new_item: NDBaseModel, exclude_unset: bool = False) -> Literal["new", "no_diff", "changed"]: + def get_diff_config(self, new_item: NDBaseModel, exclude_unset: bool = False, allow_superset: bool = False) -> Literal["new", "no_diff", "changed"]: """ Compare single item against collection. @@ -158,6 +158,9 @@ def get_diff_config(self, new_item: NDBaseModel, exclude_unset: bool = False) -> exclude_unset: When True, only compare fields explicitly set in ``new_item``. Useful for merge operations where unspecified fields should not trigger a diff. + allow_superset: When True, list elements are matched + one-directionally so an existing item carrying extra list + elements (or extra dict keys) is not flagged as changed. """ try: key = self._extract_key(new_item) @@ -169,7 +172,7 @@ def get_diff_config(self, new_item: NDBaseModel, exclude_unset: bool = False) -> if existing is None: return "new" - is_subset = existing.get_diff(new_item, exclude_unset=exclude_unset) + is_subset = existing.get_diff(new_item, exclude_unset=exclude_unset, allow_superset=allow_superset) return "no_diff" if is_subset else "changed" diff --git a/plugins/module_utils/nd_state_machine.py b/plugins/module_utils/nd_state_machine.py index e4ed36dc7..ab55e80ae 100644 --- a/plugins/module_utils/nd_state_machine.py +++ b/plugins/module_utils/nd_state_machine.py @@ -142,9 +142,11 @@ def _manage_create_update_state(self) -> None: # Determine diff status # For merged state, only compare fields explicitly provided by # the user so that Pydantic default values do not trigger false - # diffs or overwrite existing configuration. + # diffs or overwrite existing configuration. Merged also matches + # list elements one-directionally (allow_superset) so an + # existing item with extra list entries is not seen as changed. exclude_unset = self.state == "merged" - diff_status = self.existing.get_diff_config(proposed_item, exclude_unset=exclude_unset) + diff_status = self.existing.get_diff_config(proposed_item, exclude_unset=exclude_unset, allow_superset=exclude_unset) # No changes needed if diff_status == "no_diff": diff --git a/tests/unit/module_utils/test_utils.py b/tests/unit/module_utils/test_utils.py index d0f5e8eae..6646d1b71 100644 --- a/tests/unit/module_utils/test_utils.py +++ b/tests/unit/module_utils/test_utils.py @@ -242,15 +242,40 @@ class _ListFieldModel(NDBaseModel): def test_get_diff_list_field_subset_with_extra_keys_no_diff(): """ - With exclude_unset=True, an existing element carrying extra keys (e.g. a + With allow_superset=True, an existing element carrying extra keys (e.g. a controller-populated ``deploy`` flag) does not trigger a spurious diff when the proposed element omits those keys. """ existing = _ListFieldModel(name="vrf1", members=[{"id": 1, "deploy": True}]) proposed = _ListFieldModel(name="vrf1", members=[{"id": 1}]) - # exclude_unset=True -> one-directional list match -> proposed is a subset. - assert existing.get_diff(proposed, exclude_unset=True) is True + # allow_superset=True -> one-directional list match -> proposed is a subset. + assert existing.get_diff(proposed, exclude_unset=True, allow_superset=True) is True + + +def test_get_diff_exclude_unset_without_allow_superset_flags_extra_keys(): + """ + exclude_unset and allow_superset are independent: comparing only the + proposed model's set fields (exclude_unset=True) while keeping strict + bidirectional list matching (allow_superset=False) still flags an existing + element that carries extra keys. + """ + existing = _ListFieldModel(name="vrf1", members=[{"id": 1, "deploy": True}]) + proposed = _ListFieldModel(name="vrf1", members=[{"id": 1}]) + + assert existing.get_diff(proposed, exclude_unset=True, allow_superset=False) is False + + +def test_get_diff_allow_superset_without_exclude_unset(): + """ + allow_superset can be requested on its own: with exclude_unset=False the + proposed model's defaults are compared, but list elements are still matched + one-directionally so the extra ``deploy`` key is tolerated. + """ + existing = _ListFieldModel(name="vrf1", members=[{"id": 1, "deploy": True}]) + proposed = _ListFieldModel(name="vrf1", members=[{"id": 1}]) + + assert existing.get_diff(proposed, allow_superset=True) is True def test_get_diff_list_field_extra_keys_triggers_diff_without_exclude_unset(): From 6318e7c0aa05ba814b060a04d1619aeff2dc3041 Mon Sep 17 00:00:00 2001 From: Sivakami Sivaraman Date: Wed, 17 Jun 2026 19:16:42 +0530 Subject: [PATCH 09/22] pep8 compliance fix --- tests/unit/module_utils/test_utils.py | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/unit/module_utils/test_utils.py b/tests/unit/module_utils/test_utils.py index 6646d1b71..8a9166614 100644 --- a/tests/unit/module_utils/test_utils.py +++ b/tests/unit/module_utils/test_utils.py @@ -27,7 +27,6 @@ from ansible_collections.cisco.nd.plugins.module_utils.models.base import NDBaseModel from ansible_collections.cisco.nd.plugins.module_utils.utils import issubset - # ============================================================================= # issubset - scalars # ============================================================================= From fa24bb6243ceea406e83ac37a938345cc313bdd0 Mon Sep 17 00:00:00 2001 From: Sivakami Sivaraman Date: Tue, 30 Jun 2026 19:13:00 +0530 Subject: [PATCH 10/22] Addressing the review comments --- plugins/module_utils/nd_config_collection.py | 8 ++- plugins/module_utils/utils.py | 28 ++++++++--- tests/unit/module_utils/test_utils.py | 51 ++++++++++++++++++-- 3 files changed, 74 insertions(+), 13 deletions(-) diff --git a/plugins/module_utils/nd_config_collection.py b/plugins/module_utils/nd_config_collection.py index 767fd6eec..50feee312 100644 --- a/plugins/module_utils/nd_config_collection.py +++ b/plugins/module_utils/nd_config_collection.py @@ -237,11 +237,15 @@ def to_payload_list(self, **kwargs) -> List[Dict[str, Any]]: return [item.to_payload(**kwargs) for item in self._items] @staticmethod - def from_ansible_config(data: List[Dict], model_class: type[NDBaseModel], **kwargs) -> "NDConfigCollection": + def from_ansible_config(data: Optional[List[Dict]], model_class: type[NDBaseModel], **kwargs) -> "NDConfigCollection": """ Create collection from Ansible config. + + ``data`` may be ``None`` when the module's ``config`` parameter is + omitted or explicitly set to null. It is normalized to an empty + collection so callers never have to special-case the absent config. """ - items = [model_class.from_config(item_data, **kwargs) for item_data in data] + items = [model_class.from_config(item_data, **kwargs) for item_data in (data or [])] return NDConfigCollection(model_class=model_class, items=items) @staticmethod diff --git a/plugins/module_utils/utils.py b/plugins/module_utils/utils.py index 21797fce5..27cfff05d 100644 --- a/plugins/module_utils/utils.py +++ b/plugins/module_utils/utils.py @@ -71,17 +71,22 @@ def issubset(subset: Any, superset: Any, allow_superset: bool = False) -> bool: For dicts, only the non-``None`` keys of ``subset`` are compared; keys whose value is ``None`` are ignored, and keys present only in ``superset`` are - allowed. For lists, both must be the same length and a one-to-one pairing of - elements must exist (matching is order-independent). + allowed. For lists, every ``subset`` element must pair with a distinct + ``superset`` element (matching is order-independent). By default the two + lists must be the same length; when ``allow_superset`` is True the + ``subset`` list may be shorter so that extra existing elements are + tolerated (``len(subset) <= len(superset)``). Args: subset: The value to check. superset: The value to check against. - allow_superset: When True, list element matching is one-directional: - an element in ``subset`` is considered matched when it is a subset - of a candidate in ``superset``, even if the candidate has - additional keys. When False (default) matching is bidirectional. - For lists of dicts this is equivalent to equality *after* ``None`` + allow_superset: When True, list matching is one-directional: an element + in ``subset`` is considered matched when it is a subset of a + candidate in ``superset``, even if the candidate has additional + keys, and ``superset`` may contain extra elements that ``subset`` + does not (``len(subset) <= len(superset)``). When False (default) + matching is bidirectional and the lengths must be equal. For lists + of dicts the default is equivalent to equality *after* ``None`` -valued keys are dropped from both sides (it is not strict ``==`` equality, because such keys are ignored). """ @@ -90,7 +95,14 @@ def issubset(subset: Any, superset: Any, allow_superset: bool = False) -> bool: if not isinstance(subset, dict): if isinstance(subset, list): - if len(subset) != len(superset): + # Under allow_superset the proposed list only needs to map into the + # existing one, so extra existing elements are tolerated + # (len(subset) <= len(superset)). Otherwise matching is + # bidirectional and the lengths must be identical. + if allow_superset: + if len(subset) > len(superset): + return False + elif len(subset) != len(superset): return False # Build the bipartite adjacency: for each subset item, which diff --git a/tests/unit/module_utils/test_utils.py b/tests/unit/module_utils/test_utils.py index 8a9166614..8cc8104e6 100644 --- a/tests/unit/module_utils/test_utils.py +++ b/tests/unit/module_utils/test_utils.py @@ -132,7 +132,9 @@ def test_issubset_lists_bidirectional(subset, superset, expected): # # New behavior introduced in this PR: an element in ``subset`` matches a # candidate in ``superset`` when it is a subset of that candidate, even if the -# candidate has additional keys. +# candidate has additional keys. The ``subset`` list may also be shorter than +# ``superset`` (extra existing elements are tolerated) as long as every +# proposed element matches a distinct candidate. # ============================================================================= @@ -149,8 +151,11 @@ def test_issubset_lists_bidirectional(subset, superset, expected): ), # Subset element with extra key not in candidate -> still False ([{"a": 1, "z": 9}], [{"a": 1, "b": 2}], False), - # Length must still match - ([{"a": 1}], [{"a": 1, "b": 2}, {"c": 3}], False), + # Proposed list shorter than existing: the proposed item matches a + # candidate and the extra existing element is tolerated -> True + ([{"a": 1}], [{"a": 1, "b": 2}, {"c": 3}], True), + # More proposed items than candidates -> no one-to-one match -> False + ([{"a": 1}, {"c": 3}], [{"a": 1, "b": 2}], False), # Value mismatch -> False ([{"a": 2}], [{"a": 1, "b": 2}], False), ], @@ -173,6 +178,46 @@ def test_issubset_one_directional_does_not_reuse_candidate(): assert issubset(subset, superset, allow_superset=True) is False +def test_issubset_one_directional_allows_shorter_subset(): + """Under allow_superset the proposed list may be shorter than the existing. + + This is the merged-state contract: a user who names only some children must + not flag the parent as changed when the controller already holds additional + children. Every proposed child must still match a distinct existing child. + """ + # One proposed child, two existing children -> matches the first; the extra + # existing child is tolerated. + subset = [{"vrf_name": "TENANT_A"}] + superset = [ + {"vrf_name": "TENANT_A", "vlan_id": 500}, + {"vrf_name": "TENANT_B", "vlan_id": 600}, + ] + assert issubset(subset, superset, allow_superset=True) is True + + # Two proposed children, each matching a distinct existing child that + # carries extra keys, with a third existing child left untouched -> True. + subset = [{"vrf_name": "TENANT_A"}, {"vrf_name": "TENANT_B"}] + superset = [ + {"vrf_name": "TENANT_A", "vlan_id": 500}, + {"vrf_name": "TENANT_B", "vlan_id": 600}, + {"vrf_name": "TENANT_C", "vlan_id": 700}, + ] + assert issubset(subset, superset, allow_superset=True) is True + + # A proposed child that matches none of the existing children -> False. + subset = [{"vrf_name": "TENANT_Z"}] + superset = [ + {"vrf_name": "TENANT_A", "vlan_id": 500}, + {"vrf_name": "TENANT_B", "vlan_id": 600}, + ] + assert issubset(subset, superset, allow_superset=True) is False + + # More proposed children than existing -> one-to-one match impossible. + subset = [{"vrf_name": "TENANT_A"}, {"vrf_name": "TENANT_B"}] + superset = [{"vrf_name": "TENANT_A", "vlan_id": 500}] + assert issubset(subset, superset, allow_superset=True) is False + + def test_issubset_type_mismatch_returns_false(): """Mismatched top-level types short-circuit to False.""" assert issubset({"a": 1}, [1, 2]) is False From 3266c052b4f4e06cd23446d1f76269488d3afcb6 Mon Sep 17 00:00:00 2001 From: Sivakami Sivaraman Date: Tue, 30 Jun 2026 22:50:22 +0530 Subject: [PATCH 11/22] Add NDStateMachine operation unit tests --- .../test_nd_state_machine_operations.py | 459 ++++++++++++++++++ 1 file changed, 459 insertions(+) create mode 100644 tests/unit/module_utils/test_nd_state_machine_operations.py diff --git a/tests/unit/module_utils/test_nd_state_machine_operations.py b/tests/unit/module_utils/test_nd_state_machine_operations.py new file mode 100644 index 000000000..53f187908 --- /dev/null +++ b/tests/unit/module_utils/test_nd_state_machine_operations.py @@ -0,0 +1,459 @@ +# -*- coding: utf-8 -*- + +# Copyright: (c) 2026, Cisco Systems, Inc. + +# GNU General Public License v3.0+ (see LICENSE or https://www.gnu.org/licenses/gpl-3.0.txt) + +""" +Unit tests for ``NDStateMachine`` operation wiring. + +These cover the state-machine behavior that the pure ``issubset``/``get_diff`` +tests in ``test_utils.py`` cannot reach: + +- ``_execute_operation`` returns a boolean success signal and skips the API call + in check mode. +- creates/updates/deletes are added to ``sent`` only after a successful, non + check-mode operation (so check-mode runs do not produce false deploy triggers). +- under ``ignore_errors`` a failed delete leaves the item in ``existing`` and out + of ``sent``; without it the failure is raised as ``NDStateMachineError``. +- bulk-delete failure keeps every item, while bulk-delete success removes only + the targeted items. +- ``NDConfigCollection.from_ansible_config`` normalizes a ``None`` config. + +A tiny real ``NDBaseModel`` subclass drives the genuine diff/merge logic, while a +duck-typed fake orchestrator records the CRUD calls it receives and can simulate +failures. The heavy ``NDStateMachine.__init__`` (RestSend/Sender/endpoints) is +bypassed with ``object.__new__`` so the tests stay focused on the state logic. +""" + +# pylint: disable=protected-access + +from __future__ import absolute_import, annotations, division, print_function + +# pylint: disable=invalid-name +__metaclass__ = type +# pylint: enable=invalid-name + +from typing import Any, ClassVar, List, Literal, Optional + +import pytest +from ansible_collections.cisco.nd.plugins.module_utils.common.exceptions import NDStateMachineError +from ansible_collections.cisco.nd.plugins.module_utils.models.base import NDBaseModel +from ansible_collections.cisco.nd.plugins.module_utils.nd_config_collection import NDConfigCollection +from ansible_collections.cisco.nd.plugins.module_utils.nd_state_machine import NDStateMachine + + +# ============================================================================= +# Test doubles +# ============================================================================= + + +class _FakeModel(NDBaseModel): + """Minimal single-identifier model so the real diff/merge logic runs.""" + + identifiers: ClassVar[Optional[List[str]]] = ["name"] + identifier_strategy: ClassVar[Literal["single", "composite", "hierarchical", "singleton"]] = "single" + + name: str + value: Optional[str] = None + + +class _FakeOutput: + """No-op stand-in for ``NDOutput``; the state machine only calls ``assign``.""" + + def assign(self, **kwargs: Any) -> None: + return None + + +class _FakeOrchestrator: + """Duck-typed orchestrator that records CRUD calls and can simulate failures. + + ``fail_ops`` is a set of operation names ("create", "update", "delete", + "create_bulk", "delete_bulk"); a listed operation records the attempt and + then raises, mirroring an orchestrator whose API call failed. + """ + + def __init__(self, supports_bulk_create: bool = False, supports_bulk_delete: bool = False, fail_ops: Optional[set] = None) -> None: + self.model_class = _FakeModel + self.supports_bulk_create = supports_bulk_create + self.supports_bulk_delete = supports_bulk_delete + self.results = None + self.fail_ops = set(fail_ops or set()) + self.calls: dict = {"create": [], "update": [], "delete": [], "create_bulk": [], "delete_bulk": [], "preflight": []} + + def preflight(self, model_instances) -> None: + self.calls["preflight"].append(list(model_instances)) + + def query_all(self, model_instance=None, **kwargs): + return [] + + def create(self, model_instance, **kwargs): + self.calls["create"].append(model_instance) + if "create" in self.fail_ops: + raise Exception("create failed") + return {} + + def create_bulk(self, model_instances, **kwargs): + self.calls["create_bulk"].append(list(model_instances)) + if "create_bulk" in self.fail_ops: + raise Exception("create_bulk failed") + return {} + + def update(self, model_instance, **kwargs): + self.calls["update"].append(model_instance) + if "update" in self.fail_ops: + raise Exception("update failed") + return {} + + def delete(self, model_instance, **kwargs): + self.calls["delete"].append(model_instance) + if "delete" in self.fail_ops: + raise Exception("delete failed") + return None + + def delete_bulk(self, model_instances, **kwargs): + self.calls["delete_bulk"].append(list(model_instances)) + if "delete_bulk" in self.fail_ops: + raise Exception("delete_bulk failed") + return None + + +def _model(name: str, value: Optional[str] = None) -> _FakeModel: + return _FakeModel(name=name, value=value) + + +def _make_state_machine( + state: str = "merged", + check_mode: bool = False, + ignore_errors: bool = False, + orchestrator: Optional[_FakeOrchestrator] = None, + existing: Optional[List[_FakeModel]] = None, + proposed: Optional[List[_FakeModel]] = None, +) -> NDStateMachine: + """Build an ``NDStateMachine`` wired to the fakes, bypassing ``__init__``.""" + if orchestrator is None: + orchestrator = _FakeOrchestrator() + + sm = object.__new__(NDStateMachine) + sm.state = state + sm.check_mode = check_mode + sm.ignore_errors = ignore_errors + sm.model_class = _FakeModel + sm.model_orchestrator = orchestrator + sm.supports_bulk_create = orchestrator.supports_bulk_create + sm.supports_bulk_delete = orchestrator.supports_bulk_delete + sm.output = _FakeOutput() + sm.before = NDConfigCollection(model_class=_FakeModel, items=list(existing or [])) + sm.existing = sm.before.copy() + sm.proposed = NDConfigCollection(model_class=_FakeModel, items=list(proposed or [])) + sm.sent = NDConfigCollection(model_class=_FakeModel) + return sm + + +def _names(items) -> List[str]: + return sorted(item.name for item in items) + + +# ============================================================================= +# manage_state dispatch +# ============================================================================= + + +def test_manage_state_invalid_state_raises(): + """An unknown state is rejected rather than silently ignored.""" + sm = _make_state_machine(state="bogus") + with pytest.raises(NDStateMachineError, match="Invalid state"): + sm.manage_state() + + +# ============================================================================= +# check mode: API is skipped and nothing is marked sent +# ============================================================================= + + +def test_check_mode_create_skips_api_and_sent(): + """Check-mode create previews the new item but issues no API call/sent.""" + orch = _FakeOrchestrator() + sm = _make_state_machine(state="merged", check_mode=True, orchestrator=orch, proposed=[_model("a", "x")]) + + sm.manage_state() + + assert orch.calls["create"] == [] # no API call in check mode + assert len(sm.sent) == 0 # nothing marked sent -> no false deploy trigger + assert sm.existing.get("a") is not None # previewed 'after' still reflects it + + +def test_check_mode_update_skips_api_and_sent(): + """Check-mode update previews the change but issues no API call/sent.""" + orch = _FakeOrchestrator() + sm = _make_state_machine(state="replaced", check_mode=True, orchestrator=orch, existing=[_model("a", "x")], proposed=[_model("a", "y")]) + + sm.manage_state() + + assert orch.calls["update"] == [] + assert len(sm.sent) == 0 + assert sm.existing.get("a").value == "y" # previewed change + + +def test_check_mode_delete_skips_api_and_sent(): + """Check-mode delete previews the removal but issues no API call/sent.""" + orch = _FakeOrchestrator() + sm = _make_state_machine(state="deleted", check_mode=True, orchestrator=orch, existing=[_model("a", "x")], proposed=[_model("a")]) + + sm.manage_state() + + assert orch.calls["delete"] == [] + assert len(sm.sent) == 0 + assert len(sm.existing) == 0 # previewed removal + + +# ============================================================================= +# normal mode: successful operations are marked sent +# ============================================================================= + + +def test_create_individual_marks_sent(): + """A successful individual create is pushed and recorded in ``sent``.""" + orch = _FakeOrchestrator(supports_bulk_create=False) + sm = _make_state_machine(state="merged", orchestrator=orch, proposed=[_model("a", "x")]) + + sm.manage_state() + + assert _names(orch.calls["create"]) == ["a"] + assert orch.calls["create_bulk"] == [] + assert _names(sm.sent) == ["a"] + assert sm.existing.get("a") is not None + + +def test_create_bulk_marks_sent(): + """When bulk create is supported, creates go through ``create_bulk``.""" + orch = _FakeOrchestrator(supports_bulk_create=True) + sm = _make_state_machine(state="merged", orchestrator=orch, proposed=[_model("a", "x"), _model("b", "y")]) + + sm.manage_state() + + assert orch.calls["create"] == [] + assert len(orch.calls["create_bulk"]) == 1 + assert _names(orch.calls["create_bulk"][0]) == ["a", "b"] + assert _names(sm.sent) == ["a", "b"] + + +def test_update_marks_sent(): + """A successful update is pushed, recorded in ``sent``, and reflected in 'after'.""" + orch = _FakeOrchestrator() + sm = _make_state_machine(state="replaced", orchestrator=orch, existing=[_model("a", "x")], proposed=[_model("a", "y")]) + + sm.manage_state() + + assert _names(orch.calls["update"]) == ["a"] + assert _names(sm.sent) == ["a"] + assert sm.existing.get("a").value == "y" + + +def test_delete_individual_marks_sent(): + """A successful individual delete removes the item and records it in ``sent``.""" + orch = _FakeOrchestrator(supports_bulk_delete=False) + sm = _make_state_machine(state="deleted", orchestrator=orch, existing=[_model("a", "x")], proposed=[_model("a")]) + + sm.manage_state() + + assert _names(orch.calls["delete"]) == ["a"] + assert len(sm.existing) == 0 + assert _names(sm.sent) == ["a"] + + +def test_delete_bulk_marks_sent(): + """When bulk delete is supported, deletes go through ``delete_bulk``.""" + orch = _FakeOrchestrator(supports_bulk_delete=True) + sm = _make_state_machine(state="deleted", orchestrator=orch, existing=[_model("a", "x"), _model("b", "y")], proposed=[_model("a"), _model("b")]) + + sm.manage_state() + + assert orch.calls["delete"] == [] + assert len(orch.calls["delete_bulk"]) == 1 + assert _names(orch.calls["delete_bulk"][0]) == ["a", "b"] + assert _names(sm.sent) == ["a", "b"] + assert len(sm.existing) == 0 + + +# ============================================================================= +# merged diff handling +# ============================================================================= + + +def test_merged_no_diff_is_noop(): + """An item already matching its desired config triggers no operation.""" + orch = _FakeOrchestrator() + sm = _make_state_machine(state="merged", orchestrator=orch, existing=[_model("a", "x")], proposed=[_model("a", "x")]) + + sm.manage_state() + + assert orch.calls["create"] == [] + assert orch.calls["update"] == [] + assert len(sm.sent) == 0 + assert sm.existing.get("a").value == "x" + + +def test_merged_merges_changed_fields(): + """Merged state merges explicitly set fields and updates the item.""" + orch = _FakeOrchestrator() + sm = _make_state_machine(state="merged", orchestrator=orch, existing=[_model("a", "x")], proposed=[_model("a", "y")]) + + sm.manage_state() + + assert _names(orch.calls["update"]) == ["a"] + assert sm.existing.get("a").value == "y" + assert _names(sm.sent) == ["a"] + + +# ============================================================================= +# ignore_errors handling +# ============================================================================= + + +def test_delete_ignored_error_keeps_item_and_skips_sent(): + """An ignored delete failure leaves the item in 'after' and out of ``sent``.""" + orch = _FakeOrchestrator(supports_bulk_delete=False, fail_ops={"delete"}) + sm = _make_state_machine(state="deleted", ignore_errors=True, orchestrator=orch, existing=[_model("a", "x")], proposed=[_model("a")]) + + sm.manage_state() + + assert _names(orch.calls["delete"]) == ["a"] # the delete was attempted + assert sm.existing.get("a") is not None # but the item is kept + assert len(sm.sent) == 0 # and not marked sent + + +def test_delete_non_ignored_error_raises(): + """Without ignore_errors a failed delete surfaces as NDStateMachineError.""" + orch = _FakeOrchestrator(supports_bulk_delete=False, fail_ops={"delete"}) + sm = _make_state_machine(state="deleted", ignore_errors=False, orchestrator=orch, existing=[_model("a", "x")], proposed=[_model("a")]) + + with pytest.raises(NDStateMachineError, match="Failed to delete"): + sm.manage_state() + + +def test_bulk_delete_ignored_error_keeps_all_items(): + """An ignored bulk-delete failure keeps every targeted item.""" + orch = _FakeOrchestrator(supports_bulk_delete=True, fail_ops={"delete_bulk"}) + sm = _make_state_machine(state="deleted", ignore_errors=True, orchestrator=orch, existing=[_model("a", "x"), _model("b", "y")], proposed=[_model("a"), _model("b")]) + + sm.manage_state() + + assert len(orch.calls["delete_bulk"]) == 1 # bulk delete attempted once + assert _names(sm.existing) == ["a", "b"] # nothing removed + assert len(sm.sent) == 0 # nothing marked sent + + +def test_bulk_delete_non_ignored_error_raises(): + """Without ignore_errors a failed bulk delete surfaces as NDStateMachineError.""" + orch = _FakeOrchestrator(supports_bulk_delete=True, fail_ops={"delete_bulk"}) + sm = _make_state_machine(state="deleted", ignore_errors=False, orchestrator=orch, existing=[_model("a", "x")], proposed=[_model("a")]) + + with pytest.raises(NDStateMachineError, match="Failed to delete in bulk"): + sm.manage_state() + + +def test_bulk_delete_success_removes_only_targeted(): + """A successful bulk delete removes only the proposed items.""" + orch = _FakeOrchestrator(supports_bulk_delete=True) + sm = _make_state_machine( + state="deleted", + orchestrator=orch, + existing=[_model("a", "x"), _model("b", "y"), _model("c", "z")], + proposed=[_model("a"), _model("b")], + ) + + sm.manage_state() + + assert _names(sm.existing) == ["c"] # only the untargeted item remains + assert _names(sm.sent) == ["a", "b"] + + +# ============================================================================= +# overridden deletions +# ============================================================================= + + +def test_overridden_deletes_non_proposed_items(): + """Overridden removes existing items absent from the proposed config.""" + orch = _FakeOrchestrator(supports_bulk_delete=False) + sm = _make_state_machine( + state="overridden", + orchestrator=orch, + existing=[_model("a", "x"), _model("b", "y"), _model("c", "z")], + proposed=[_model("a", "x")], + ) + + sm.manage_state() + + assert _names(orch.calls["delete"]) == ["b", "c"] # only non-proposed deleted + assert _names(sm.existing) == ["a"] + assert _names(sm.sent) == ["b", "c"] + + +# ============================================================================= +# _execute_operation contract +# ============================================================================= + + +def test_execute_operation_check_mode_skips_call(): + """In check mode the API callable is not invoked but success is reported.""" + sm = _make_state_machine(check_mode=True) + calls: List[tuple] = [] + + result = sm._execute_operation(lambda *a, **k: calls.append(a), "payload") + + assert result is True + assert calls == [] # callable skipped + + +def test_execute_operation_success_returns_true(): + """Outside check mode a successful callable runs and reports success.""" + sm = _make_state_machine(check_mode=False) + calls: List[tuple] = [] + + result = sm._execute_operation(lambda *a, **k: calls.append(a), "payload") + + assert result is True + assert calls == [("payload",)] + + +def test_execute_operation_ignored_error_returns_false(): + """An ignored failure is swallowed and reported as not-sent (False).""" + sm = _make_state_machine(check_mode=False, ignore_errors=True) + + def _boom(*args, **kwargs): + raise Exception("boom") + + assert sm._execute_operation(_boom, "payload") is False + + +def test_execute_operation_non_ignored_error_raises(): + """A non-ignored failure is wrapped in NDStateMachineError.""" + sm = _make_state_machine(check_mode=False, ignore_errors=False) + + def _boom(*args, **kwargs): + raise Exception("boom") + + with pytest.raises(NDStateMachineError, match="Operation failed"): + sm._execute_operation(_boom, "payload") + + +# ============================================================================= +# from_ansible_config normalization (config may be None) +# ============================================================================= + + +@pytest.mark.parametrize( + "data, expected_len", + [ + (None, 0), # config omitted / explicitly null must not crash + ([], 0), + ([{"name": "a"}], 1), + ([{"name": "a"}, {"name": "b"}], 2), + ], +) +def test_from_ansible_config_normalizes_data(data, expected_len): + """``from_ansible_config`` tolerates None and builds a collection otherwise.""" + collection = NDConfigCollection.from_ansible_config(data=data, model_class=_FakeModel) + assert len(collection) == expected_len From 98e683f984cfa57dc60240b34ff40e5948343bc7 Mon Sep 17 00:00:00 2001 From: Sivakami Sivaraman Date: Wed, 1 Jul 2026 12:09:30 +0530 Subject: [PATCH 12/22] Fixing pep8 sanity issue --- .../unit/module_utils/test_nd_state_machine_operations.py | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/tests/unit/module_utils/test_nd_state_machine_operations.py b/tests/unit/module_utils/test_nd_state_machine_operations.py index 53f187908..8fd580a66 100644 --- a/tests/unit/module_utils/test_nd_state_machine_operations.py +++ b/tests/unit/module_utils/test_nd_state_machine_operations.py @@ -335,7 +335,13 @@ def test_delete_non_ignored_error_raises(): def test_bulk_delete_ignored_error_keeps_all_items(): """An ignored bulk-delete failure keeps every targeted item.""" orch = _FakeOrchestrator(supports_bulk_delete=True, fail_ops={"delete_bulk"}) - sm = _make_state_machine(state="deleted", ignore_errors=True, orchestrator=orch, existing=[_model("a", "x"), _model("b", "y")], proposed=[_model("a"), _model("b")]) + sm = _make_state_machine( + state="deleted", + ignore_errors=True, + orchestrator=orch, + existing=[_model("a", "x"), _model("b", "y")], + proposed=[_model("a"), _model("b")], + ) sm.manage_state() From aea71148c827dce423394d67c0c759a302f70fdd Mon Sep 17 00:00:00 2001 From: Sivakami Sivaraman Date: Wed, 1 Jul 2026 18:53:21 +0530 Subject: [PATCH 13/22] Fix black formatting in test_nd_state_machine_operations.py --- tests/unit/module_utils/test_nd_state_machine_operations.py | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/unit/module_utils/test_nd_state_machine_operations.py b/tests/unit/module_utils/test_nd_state_machine_operations.py index 8fd580a66..63904bde2 100644 --- a/tests/unit/module_utils/test_nd_state_machine_operations.py +++ b/tests/unit/module_utils/test_nd_state_machine_operations.py @@ -42,7 +42,6 @@ from ansible_collections.cisco.nd.plugins.module_utils.nd_config_collection import NDConfigCollection from ansible_collections.cisco.nd.plugins.module_utils.nd_state_machine import NDStateMachine - # ============================================================================= # Test doubles # ============================================================================= From f44ee066fb7139caa536c6720ee31280eccc9cc9 Mon Sep 17 00:00:00 2001 From: Sivakami Sivaraman Date: Mon, 13 Jul 2026 22:44:40 +0530 Subject: [PATCH 14/22] Addressing review comments --- .../maintenance_mode/maintenance_mode.py | 4 +- plugins/module_utils/nd_config_collection.py | 7 +- plugins/module_utils/nd_state_machine.py | 9 +- .../models/test_maintenance_mode_model.py | 26 +++++ .../test_nd_state_machine_operations.py | 101 +++++++++++++++++- 5 files changed, 140 insertions(+), 7 deletions(-) diff --git a/plugins/module_utils/models/maintenance_mode/maintenance_mode.py b/plugins/module_utils/models/maintenance_mode/maintenance_mode.py index 129e0b080..27bb4c68f 100644 --- a/plugins/module_utils/models/maintenance_mode/maintenance_mode.py +++ b/plugins/module_utils/models/maintenance_mode/maintenance_mode.py @@ -118,7 +118,7 @@ def _require_switches_when_mode_set(self) -> "MaintenanceModeModel": # --- Custom Diff (per-switch mode comparison) --- - def get_diff(self, other: "NDBaseModel", exclude_unset: bool = False) -> bool: + def get_diff(self, other: "NDBaseModel", exclude_unset: bool = False, allow_superset: bool = False) -> bool: """ # Summary @@ -127,6 +127,8 @@ def get_diff(self, other: "NDBaseModel", exclude_unset: bool = False) -> bool: `self` is the snapshot (existing); `other` is the proposed config. Default `NDBaseModel.get_diff` does a generic subset check that does not understand the per-switch mode comparison we need. + ``exclude_unset`` and ``allow_superset`` are accepted for NDConfigCollection compatibility; + this custom per-switch comparison intentionally ignores both. ## Raises diff --git a/plugins/module_utils/nd_config_collection.py b/plugins/module_utils/nd_config_collection.py index 50feee312..dd0c6b5ae 100644 --- a/plugins/module_utils/nd_config_collection.py +++ b/plugins/module_utils/nd_config_collection.py @@ -241,9 +241,10 @@ def from_ansible_config(data: Optional[List[Dict]], model_class: type[NDBaseMode """ Create collection from Ansible config. - ``data`` may be ``None`` when the module's ``config`` parameter is - omitted or explicitly set to null. It is normalized to an empty - collection so callers never have to special-case the absent config. + ``data`` may be ``None`` for callers that intentionally treat absent + config as an empty collection. Callers whose state semantics distinguish + omitted/null config from an explicit empty list must validate that + before calling this helper. """ items = [model_class.from_config(item_data, **kwargs) for item_data in (data or [])] return NDConfigCollection(model_class=model_class, items=items) diff --git a/plugins/module_utils/nd_state_machine.py b/plugins/module_utils/nd_state_machine.py index 15bc2169f..c8310076f 100644 --- a/plugins/module_utils/nd_state_machine.py +++ b/plugins/module_utils/nd_state_machine.py @@ -59,6 +59,13 @@ def __init__(self, module: AnsibleModule, model_orchestrator: type[NDBaseOrchest self.model_class = self.model_orchestrator.model_class self.state = self.module.params["state"] + raw_config = self.module.params.get("config") + + if self.state in ["merged", "replaced", "overridden"] and raw_config is None: + raise NDStateMachineError( + f"config must be provided and cannot be null for state '{self.state}'. " + "Use config: [] only when intentionally managing an explicit empty set." + ) # Cached flags self.check_mode = self.module.check_mode @@ -76,7 +83,7 @@ def __init__(self, module: AnsibleModule, model_orchestrator: type[NDBaseOrchest # Ongoing collection of configuration objects that were changed self.sent = NDConfigCollection(model_class=self.model_class) # Collection of configuration objects given by user - self.proposed = NDConfigCollection.from_ansible_config(data=self.module.params.get("config", []), model_class=self.model_class) + self.proposed = NDConfigCollection.from_ansible_config(data=raw_config, model_class=self.model_class) self.output.assign(after=self.existing, before=self.before, proposed=self.proposed) diff --git a/tests/unit/module_utils/models/test_maintenance_mode_model.py b/tests/unit/module_utils/models/test_maintenance_mode_model.py index 7f822f903..6f02e96d9 100644 --- a/tests/unit/module_utils/models/test_maintenance_mode_model.py +++ b/tests/unit/module_utils/models/test_maintenance_mode_model.py @@ -234,6 +234,32 @@ def test_maintenance_mode_model_00070() -> None: assert snapshot.get_diff(proposed) is True +def test_maintenance_mode_model_00075() -> None: + """ + # Summary + + Verify the custom `get_diff` accepts NDConfigCollection's shared diff + keyword arguments. + + ## Classes and Methods + + - MaintenanceModeModel.get_diff + """ + snapshot = MaintenanceModeModel.from_response( + { + "switches": [{"switch_ip": "192.168.12.131"}], + "switch_modes": {"192.168.12.131": "maintenance"}, + } + ) + proposed = MaintenanceModeModel.from_config( + { + "mode": "maintenance", + "switches": [{"switch_ip": "192.168.12.131"}], + } + ) + assert snapshot.get_diff(proposed, exclude_unset=True, allow_superset=True) is True + + def test_maintenance_mode_model_00080() -> None: """ # Summary diff --git a/tests/unit/module_utils/test_nd_state_machine_operations.py b/tests/unit/module_utils/test_nd_state_machine_operations.py index 63904bde2..e1775ec61 100644 --- a/tests/unit/module_utils/test_nd_state_machine_operations.py +++ b/tests/unit/module_utils/test_nd_state_machine_operations.py @@ -18,7 +18,8 @@ of ``sent``; without it the failure is raised as ``NDStateMachineError``. - bulk-delete failure keeps every item, while bulk-delete success removes only the targeted items. -- ``NDConfigCollection.from_ansible_config`` normalizes a ``None`` config. +- ``NDStateMachine`` rejects omitted/null config for mutating write states so + ``state: overridden`` cannot accidentally delete everything. A tiny real ``NDBaseModel`` subclass drives the genuine diff/merge logic, while a duck-typed fake orchestrator records the CRUD calls it receives and can simulate @@ -41,6 +42,7 @@ from ansible_collections.cisco.nd.plugins.module_utils.models.base import NDBaseModel from ansible_collections.cisco.nd.plugins.module_utils.nd_config_collection import NDConfigCollection from ansible_collections.cisco.nd.plugins.module_utils.nd_state_machine import NDStateMachine +from ansible_collections.cisco.nd.plugins.module_utils.orchestrators.base import NDBaseOrchestrator # ============================================================================= # Test doubles @@ -117,6 +119,46 @@ def delete_bulk(self, model_instances, **kwargs): return None +_MISSING = object() + + +class _FakeModule: + """Minimal module object for exercising ``NDStateMachine.__init__``.""" + + def __init__(self, state: str = "merged", config: Any = _MISSING, check_mode: bool = False, ignore_errors: bool = False) -> None: + self.check_mode = check_mode + self.params: dict[str, Any] = { + "state": state, + "output_level": "normal", + "ignore_errors": ignore_errors, + } + if config is not _MISSING: + self.params["config"] = config + + +class _InitFakeOrchestrator(NDBaseOrchestrator): + """Minimal real orchestrator subclass for ``NDStateMachine.__init__`` tests.""" + + model_class: ClassVar[type[NDBaseModel]] = _FakeModel + create_endpoint: ClassVar[Any] = None + update_endpoint: ClassVar[Any] = None + delete_endpoint: ClassVar[Any] = None + query_one_endpoint: ClassVar[Any] = None + query_all_endpoint: ClassVar[Any] = None + + def query_all(self, model_instance=None, **kwargs): + return [] + + def create(self, model_instance, **kwargs): + return {} + + def update(self, model_instance, **kwargs): + return {} + + def delete(self, model_instance, **kwargs): + return None + + def _model(name: str, value: Optional[str] = None) -> _FakeModel: return _FakeModel(name=name, value=value) @@ -396,6 +438,23 @@ def test_overridden_deletes_non_proposed_items(): assert _names(sm.sent) == ["b", "c"] +def test_overridden_explicit_empty_config_deletes_all_existing(): + """An explicit empty overridden config means delete every existing item.""" + orch = _FakeOrchestrator(supports_bulk_delete=False) + sm = _make_state_machine( + state="overridden", + orchestrator=orch, + existing=[_model("a", "x"), _model("b", "y")], + proposed=[], + ) + + sm.manage_state() + + assert _names(orch.calls["delete"]) == ["a", "b"] + assert len(sm.existing) == 0 + assert _names(sm.sent) == ["a", "b"] + + # ============================================================================= # _execute_operation contract # ============================================================================= @@ -445,7 +504,45 @@ def _boom(*args, **kwargs): # ============================================================================= -# from_ansible_config normalization (config may be None) +# config missing/null/empty handling +# ============================================================================= + + +@pytest.mark.parametrize("state", ["merged", "replaced", "overridden"]) +@pytest.mark.parametrize( + "config", + [ + pytest.param(_MISSING, id="missing"), + pytest.param(None, id="null"), + ], +) +def test_state_machine_rejects_missing_or_null_config_for_write_states(state, config): + """Write states require explicit config so null cannot become destructive.""" + module = _FakeModule(state=state, config=config) + + with pytest.raises(NDStateMachineError, match=r"config must be provided and cannot be null"): + NDStateMachine(module=module, model_orchestrator=_InitFakeOrchestrator) + + +@pytest.mark.parametrize("state", ["merged", "replaced", "overridden", "deleted"]) +def test_state_machine_accepts_explicit_empty_config(state): + """Explicit ``config: []`` remains distinct from omitted/null config.""" + module = _FakeModule(state=state, config=[]) + sm = NDStateMachine(module=module, model_orchestrator=_InitFakeOrchestrator) + + assert len(sm.proposed) == 0 + + +def test_deleted_state_tolerates_null_config_as_empty(): + """Null delete config is non-destructive: it targets no proposed items.""" + module = _FakeModule(state="deleted", config=None) + sm = NDStateMachine(module=module, model_orchestrator=_InitFakeOrchestrator) + + assert len(sm.proposed) == 0 + + +# ============================================================================= +# from_ansible_config normalization # ============================================================================= From b3d5a016d01925323cdaff8dac0328df39e25a99 Mon Sep 17 00:00:00 2001 From: Sivakami Sivaraman Date: Wed, 15 Jul 2026 13:48:25 +0530 Subject: [PATCH 15/22] Fix Black formatting in state machine --- plugins/module_utils/nd_state_machine.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/plugins/module_utils/nd_state_machine.py b/plugins/module_utils/nd_state_machine.py index bd129d171..035ca3862 100644 --- a/plugins/module_utils/nd_state_machine.py +++ b/plugins/module_utils/nd_state_machine.py @@ -85,9 +85,7 @@ def __init__(self, module: AnsibleModule, model_orchestrator: type[NDBaseOrchest # ``context={"state": ...}`` is threaded into pydantic validation so models can apply # state-aware validation (e.g. require certain fields for write states while accepting # identifier-only items for ``deleted``). Models that do not read the context ignore it. - self.proposed = NDConfigCollection.from_ansible_config( - data=raw_config, model_class=self.model_class, context={"state": self.state} - ) + self.proposed = NDConfigCollection.from_ansible_config(data=raw_config, model_class=self.model_class, context={"state": self.state}) self.output.assign(after=self.existing, before=self.before, proposed=self.proposed) From 472ac2ab571c22d7cf28a0be1bc08e32233c33ab Mon Sep 17 00:00:00 2001 From: Sivakami Sivaraman Date: Thu, 23 Jul 2026 12:48:13 +0530 Subject: [PATCH 16/22] Updated the documentation for get_diff --- plugins/module_utils/models/base.py | 12 ++++++++++++ plugins/module_utils/nd_config_collection.py | 9 +++++++++ 2 files changed, 21 insertions(+) diff --git a/plugins/module_utils/models/base.py b/plugins/module_utils/models/base.py index fa0b7cee3..a44e24da7 100644 --- a/plugins/module_utils/models/base.py +++ b/plugins/module_utils/models/base.py @@ -219,6 +219,18 @@ def to_diff_dict(self, **kwargs) -> Dict[str, Any]: def get_diff(self, other: "NDBaseModel", exclude_unset: bool = False, allow_superset: bool = False) -> bool: """Diff comparison. + Subclass contract: + Any subclass that overrides ``get_diff`` MUST accept both the + ``exclude_unset`` and ``allow_superset`` keyword arguments. + ``NDConfigCollection.get_diff_config`` always forwards them to the + concrete model, and Python dispatches to the subclass override + rather than to this base method. An override may ignore + ``allow_superset`` when its comparison does not need superset + semantics (see ``MaintenanceModeModel``), but it must still accept + the keyword. A non-conforming override raises ``TypeError`` at diff + time; that failure is intentional and must not be masked by catching + ``TypeError`` or inspecting the method signature at runtime. + Args: other: The model to compare against. exclude_unset: When True, only compare fields explicitly set in diff --git a/plugins/module_utils/nd_config_collection.py b/plugins/module_utils/nd_config_collection.py index dd0c6b5ae..52ff50f2d 100644 --- a/plugins/module_utils/nd_config_collection.py +++ b/plugins/module_utils/nd_config_collection.py @@ -161,6 +161,11 @@ def get_diff_config(self, new_item: NDBaseModel, exclude_unset: bool = False, al allow_superset: When True, list elements are matched one-directionally so an existing item carrying extra list elements (or extra dict keys) is not flagged as changed. + + Both ``exclude_unset`` and ``allow_superset`` are forwarded to the + concrete model's ``get_diff``. Any model that overrides ``get_diff`` + must accept these keywords; see the subclass contract on + ``NDBaseModel.get_diff``. """ try: key = self._extract_key(new_item) @@ -172,6 +177,10 @@ def get_diff_config(self, new_item: NDBaseModel, exclude_unset: bool = False, al if existing is None: return "new" + # ``get_diff`` is dispatched to the concrete model, so every override + # must accept ``exclude_unset`` and ``allow_superset`` (see the subclass + # contract on ``NDBaseModel.get_diff``). A non-conforming override raises + # TypeError here by design; do not catch it or inspect the signature. is_subset = existing.get_diff(new_item, exclude_unset=exclude_unset, allow_superset=allow_superset) return "no_diff" if is_subset else "changed" From 7a1a5512bd464aede9815ff48c5b62f474014ea0 Mon Sep 17 00:00:00 2001 From: Sivakami Sivaraman Date: Thu, 23 Jul 2026 12:50:41 +0530 Subject: [PATCH 17/22] UT for NDConfigCollection --- .../module_utils/test_nd_config_collection.py | 129 ++++++++++++++++++ 1 file changed, 129 insertions(+) create mode 100644 tests/unit/module_utils/test_nd_config_collection.py diff --git a/tests/unit/module_utils/test_nd_config_collection.py b/tests/unit/module_utils/test_nd_config_collection.py new file mode 100644 index 000000000..355e913a0 --- /dev/null +++ b/tests/unit/module_utils/test_nd_config_collection.py @@ -0,0 +1,129 @@ +# -*- coding: utf-8 -*- + +# Copyright: (c) 2026, Cisco Systems, Inc. + +# GNU General Public License v3.0+ (see LICENSE or https://www.gnu.org/licenses/gpl-3.0.txt) + +""" +Unit tests for ``NDConfigCollection.get_diff_config`` dispatch behavior. + +``get_diff_config`` forwards ``exclude_unset`` and ``allow_superset`` to the +concrete model's ``get_diff``. Because Python dispatches ``get_diff`` to the +subclass override (not to ``NDBaseModel.get_diff``), every override must accept +both keywords. These tests exercise that contract *through* ``NDConfigCollection`` +rather than by calling ``get_diff`` directly, so a subclass that predates the +shared signature is caught here: + +- a conforming override receives ``allow_superset`` unchanged, and its return + value flows back out as ``no_diff`` / ``changed``. +- ``allow_superset`` defaults to ``False`` when the caller omits it. +- a non-conforming override (old ``get_diff(self, other, exclude_unset=False)`` + signature) surfaces the incompatibility as a loud ``TypeError`` -- the failure + is intentional and is not hidden by the collection. +""" + +# pylint: disable=protected-access + +from __future__ import absolute_import, annotations, division, print_function + +# pylint: disable=invalid-name +__metaclass__ = type +# pylint: enable=invalid-name + +from typing import ClassVar, List, Literal, Optional + +import pytest +from ansible_collections.cisco.nd.plugins.module_utils.models.base import NDBaseModel +from ansible_collections.cisco.nd.plugins.module_utils.nd_config_collection import NDConfigCollection + +# ============================================================================= +# Test doubles +# ============================================================================= + + +class _ConformingModel(NDBaseModel): + """Override that conforms to the shared ``get_diff`` contract. + + It accepts ``allow_superset`` and lets the forwarded value drive the result, + so a test can prove the keyword was dispatched through ``NDConfigCollection``: + ``True`` -> "no diff" (subset), ``False`` -> "changed". + """ + + identifiers: ClassVar[Optional[List[str]]] = ["name"] + identifier_strategy: ClassVar[Literal["single", "composite", "hierarchical", "singleton"]] = "single" + + name: str + value: Optional[str] = None + + def get_diff(self, other: "NDBaseModel", exclude_unset: bool = False, allow_superset: bool = False) -> bool: + return bool(allow_superset) + + +class _NonConformingModel(NDBaseModel): + """Override predating the contract: it omits ``allow_superset`` by design. + + Mirrors the ``get_diff`` overrides on in-flight PRs #286/#312 that have not + yet adopted the shared signature. Forwarding ``allow_superset`` to it must + raise ``TypeError`` rather than be silently swallowed. + """ + + identifiers: ClassVar[Optional[List[str]]] = ["name"] + identifier_strategy: ClassVar[Literal["single", "composite", "hierarchical", "singleton"]] = "single" + + name: str + value: Optional[str] = None + + def get_diff(self, other: "NDBaseModel", exclude_unset: bool = False) -> bool: # noqa: type checked at runtime + return True + + +# ============================================================================= +# Tests +# ============================================================================= + + +def test_get_diff_config_forwards_allow_superset_to_override(): + """``allow_superset`` is dispatched to the concrete override unchanged. + + The conforming override returns the forwarded value, so ``True`` collapses to + "no_diff" and ``False`` to "changed" -- both proving the keyword reached the + subclass through ``NDConfigCollection.get_diff_config``. + """ + existing = _ConformingModel(name="a") + collection = NDConfigCollection(model_class=_ConformingModel, items=[existing]) + proposed = _ConformingModel(name="a") + + assert collection.get_diff_config(proposed, exclude_unset=True, allow_superset=True) == "no_diff" + assert collection.get_diff_config(proposed, exclude_unset=True, allow_superset=False) == "changed" + + +def test_get_diff_config_defaults_allow_superset_to_false(): + """Omitting ``allow_superset`` forwards ``False`` to the override.""" + existing = _ConformingModel(name="a") + collection = NDConfigCollection(model_class=_ConformingModel, items=[existing]) + proposed = _ConformingModel(name="a") + + assert collection.get_diff_config(proposed) == "changed" + + +def test_get_diff_config_returns_new_when_item_absent(): + """An item with no existing match short-circuits to "new" before ``get_diff``.""" + collection = NDConfigCollection(model_class=_ConformingModel, items=[_ConformingModel(name="a")]) + proposed = _ConformingModel(name="b") + + assert collection.get_diff_config(proposed, allow_superset=True) == "new" + + +def test_get_diff_config_nonconforming_override_raises_typeerror(): + """A non-conforming override surfaces the incompatibility loudly. + + ``get_diff_config`` forwards ``allow_superset``; the old-signature override + rejects it and the ``TypeError`` propagates. The collection must not hide the + mismatch by catching ``TypeError`` or inspecting the signature. + """ + existing = _NonConformingModel(name="a") + collection = NDConfigCollection(model_class=_NonConformingModel, items=[existing]) + proposed = _NonConformingModel(name="a") + + with pytest.raises(TypeError, match="allow_superset"): + collection.get_diff_config(proposed, exclude_unset=True, allow_superset=True) From 1b145478d9bf972395215521f958426b999a79eb Mon Sep 17 00:00:00 2001 From: Sivakami Sivaraman Date: Thu, 30 Jul 2026 13:50:55 +0530 Subject: [PATCH 18/22] Reject empty config so overridden can't silently delete everything - Drop default=[] on nd_manage_networks config so an omitted value stays None instead of being coerced into an empty list - Add NDStateMachine.validate_config_presence and call it on the raw config (before normalization) in the network coordinator and the vpc_pair wrapper, plus in the state machine itself - Explicit config: [] still works for intentional delete-all under overridden - Fix PrefixListModel.get_diff to accept exclude_unset/allow_superset so it matches the shared NDBaseModel signature used by NDConfigCollection - Add composed wrapper tests covering omitted/null/explicit-empty config (overridden, check mode, existing resources) and a get_diff regression --- .../manage_prefix_list/manage_prefix_list.py | 26 +- plugins/module_utils/nd_state_machine.py | 23 +- .../network_workflow_coordinator.py | 4 + plugins/modules/nd_manage_networks.py | 4 +- plugins/modules/nd_manage_vpc_pair.py | 15 +- .../models/test_manage_prefix_list.py | 99 ++++++++ .../orchestrators/test_networks.py | 48 ++++ .../test_nd_state_machine_operations.py | 25 +- tests/unit/modules/test_nd_manage_networks.py | 181 ++++++++++++++ tests/unit/modules/test_nd_manage_vpc_pair.py | 227 ++++++++++++++++++ 10 files changed, 635 insertions(+), 17 deletions(-) create mode 100644 tests/unit/modules/test_nd_manage_vpc_pair.py diff --git a/plugins/module_utils/models/manage_prefix_list/manage_prefix_list.py b/plugins/module_utils/models/manage_prefix_list/manage_prefix_list.py index 4a9658a33..7944b380d 100644 --- a/plugins/module_utils/models/manage_prefix_list/manage_prefix_list.py +++ b/plugins/module_utils/models/manage_prefix_list/manage_prefix_list.py @@ -67,6 +67,7 @@ IpVersionEnum, PrefixListActionEnum, ) +from ansible_collections.cisco.nd.plugins.module_utils.utils import issubset # Allowed characters for prefix list / tenant names (from OpenAPI pattern) _NAME_RE = re.compile(r"^[a-zA-Z0-9~_-]+$") @@ -357,7 +358,7 @@ def get_identifier_value(self) -> tuple[str, str | None, str]: """Return the tenant-aware composite identifier.""" return (str(self.ip_version), self.tenant_name, self.name) - def get_diff(self, other: NDBaseModel, exclude_unset: bool = False) -> bool: + def get_diff(self, other: NDBaseModel, exclude_unset: bool = False, allow_superset: bool = False) -> bool: """ Compare prefix-list config, treating omitted description as empty in replace-style states. @@ -366,11 +367,32 @@ def get_diff(self, other: NDBaseModel, exclude_unset: bool = False) -> bool: ``overridden`` it passes ``exclude_unset=False``; in that path an omitted description means the desired value is empty/absent, so a stale controller description must trigger an update. + + ``allow_superset`` is part of the ``NDBaseModel.get_diff`` subclass + contract and must be forwarded. Authoritative entry-list changes that + superset matching would otherwise hide are detected by + ``merge_would_change`` below. """ if not exclude_unset and isinstance(other, PrefixListModel): if other.description is None and self.description not in (None, ""): return False - return super().get_diff(other, exclude_unset=exclude_unset) + return super().get_diff(other, exclude_unset=exclude_unset, allow_superset=allow_superset) + + def merge_would_change(self, other: NDBaseModel) -> bool: + """ + Detect authoritative prefix-list entry changes during merged comparison. + + Entry models are serialized with defaults included so an omitted + ``action`` remains equivalent to its documented ``permit`` default. + List matching remains order-independent but requires the same entries + and fields in both directions. + """ + if isinstance(other, PrefixListModel) and "entries" in other.model_fields_set: + current_entries = [entry.to_diff_dict() for entry in self.entries or []] + proposed_entries = [entry.to_diff_dict() for entry in other.entries or []] + if not issubset(proposed_entries, current_entries, allow_superset=False): + return True + return super().merge_would_change(other) @classmethod def validate_config_for_state(cls, config: list[dict[str, Any]], state: str) -> None: diff --git a/plugins/module_utils/nd_state_machine.py b/plugins/module_utils/nd_state_machine.py index 035ca3862..2ffd00919 100644 --- a/plugins/module_utils/nd_state_machine.py +++ b/plugins/module_utils/nd_state_machine.py @@ -25,6 +25,22 @@ class NDStateMachine: Generic State Machine for Nexus Dashboard (Bulk Support). """ + WRITE_STATES_REQUIRING_CONFIG = frozenset({"merged", "replaced", "overridden"}) + + @classmethod + def validate_config_presence(cls, state: str, config: Any) -> None: + """ + Reject omitted or null config before production wrappers normalize it. + + An explicit empty list remains valid because it can represent an + intentional empty desired set. + """ + if state in cls.WRITE_STATES_REQUIRING_CONFIG and config is None: + raise NDStateMachineError( + f"config must be provided and cannot be null for state '{state}'. " + "Use config: [] only when intentionally managing an explicit empty set." + ) + def __init__(self, module: AnsibleModule, model_orchestrator: type[NDBaseOrchestrator] | NDBaseOrchestrator): """ Initialize the ND State Machine. @@ -59,12 +75,7 @@ def __init__(self, module: AnsibleModule, model_orchestrator: type[NDBaseOrchest self.model_class = self.model_orchestrator.model_class self.state = self.module.params["state"] raw_config = self.module.params.get("config") - - if self.state in ["merged", "replaced", "overridden"] and raw_config is None: - raise NDStateMachineError( - f"config must be provided and cannot be null for state '{self.state}'. " - "Use config: [] only when intentionally managing an explicit empty set." - ) + self.validate_config_presence(self.state, raw_config) # Cached flags self.check_mode = self.module.check_mode diff --git a/plugins/module_utils/orchestrators/network_workflow_coordinator.py b/plugins/module_utils/orchestrators/network_workflow_coordinator.py index d02fa372e..108477283 100644 --- a/plugins/module_utils/orchestrators/network_workflow_coordinator.py +++ b/plugins/module_utils/orchestrators/network_workflow_coordinator.py @@ -109,6 +109,10 @@ def run(self) -> dict[str, Any]: Returns a result dict suitable for module.exit_json(**result). """ module_args: dict = dict(self.module.params) + NDStateMachine.validate_config_presence( + module_args.get("state", "merged"), + module_args.get("config"), + ) if self.strategy is None: self.strategy = self._resolve_strategy(module_args) self._trace( diff --git a/plugins/modules/nd_manage_networks.py b/plugins/modules/nd_manage_networks.py index a8dee3231..5feadb86c 100644 --- a/plugins/modules/nd_manage_networks.py +++ b/plugins/modules/nd_manage_networks.py @@ -30,9 +30,10 @@ config: description: - List of Network definitions to manage. + - Required and must not be null for states V(merged), V(replaced), and V(overridden). + - With state V(overridden), an explicit empty list removes all managed Networks in scope. type: list elements: dict - default: [] suboptions: network_name: description: Name of the Network. @@ -586,7 +587,6 @@ def main(): type="list", elements="dict", required=False, - default=[], options=network_parent_argument_spec(), ), ) diff --git a/plugins/modules/nd_manage_vpc_pair.py b/plugins/modules/nd_manage_vpc_pair.py index a6b97b956..538ceb0f4 100644 --- a/plugins/modules/nd_manage_vpc_pair.py +++ b/plugins/modules/nd_manage_vpc_pair.py @@ -90,6 +90,8 @@ config: description: - List of vPC pair configuration dictionaries. + - Required and must not be null for states V(merged), V(replaced), and V(overridden). + - With state V(overridden), an explicit empty list removes all managed vPC pairs in scope. type: list elements: dict suboptions: @@ -353,7 +355,13 @@ from ansible_collections.cisco.nd.plugins.module_utils.common.pydantic_compat import ( ValidationError, ) +from ansible_collections.cisco.nd.plugins.module_utils.common.exceptions import ( + NDStateMachineError, +) from ansible_collections.cisco.nd.plugins.module_utils.nd import nd_argument_spec +from ansible_collections.cisco.nd.plugins.module_utils.nd_state_machine import ( + NDStateMachine, +) # Service layer imports from ansible_collections.cisco.nd.plugins.module_utils.manage_vpc_pair.resources import ( @@ -460,6 +468,11 @@ def main() -> None: # State-specific parameter validations state = module_config.state + try: + NDStateMachine.validate_config_presence(state, module_config.config) + except NDStateMachineError as e: + module.fail_json(msg=str(e)) + config_actions = get_config_actions(module) verify_settings = get_verify_settings(module) raw_module_args = _get_raw_module_args() @@ -507,7 +520,7 @@ def main() -> None: module.warn("Parameter 'force' only applies to state 'deleted'. " f"Ignoring force for state '{state}'.") # Normalize config keys for runtime/state-machine model handling. - normalized_config = [item.to_runtime_config() for item in (module_config.config or [])] + normalized_config = [] if module_config.config is None else [item.to_runtime_config() for item in module_config.config] module.params["config"] = normalized_config diff --git a/tests/unit/module_utils/models/test_manage_prefix_list.py b/tests/unit/module_utils/models/test_manage_prefix_list.py index 8fbf2b0a8..3126e4517 100644 --- a/tests/unit/module_utils/models/test_manage_prefix_list.py +++ b/tests/unit/module_utils/models/test_manage_prefix_list.py @@ -23,6 +23,7 @@ PrefixListEntryModel, PrefixListModel, ) +from ansible_collections.cisco.nd.plugins.module_utils.nd_config_collection import NDConfigCollection from ansible_collections.cisco.nd.tests.unit.module_utils.common_utils import does_not_raise SAMPLE_IPV4_CONFIG = { @@ -773,3 +774,101 @@ def test_manage_prefix_list_00210() -> None: bad_config["name"] = "P" * 107 with pytest.raises(ValidationError, match="combined tenant-qualified"): PrefixListModel.from_config(bad_config) + + +# ============================================================================= +# Test: NDConfigCollection contract (regression) +# ============================================================================= + + +def test_manage_prefix_list_00220() -> None: + """ + # Summary + + Reconcile the real ``PrefixListModel`` through ``NDConfigCollection`` to lock + in the shared ``get_diff`` signature contract. + + ``NDConfigCollection.get_diff_config`` always forwards ``allow_superset`` to + the concrete model's ``get_diff``. Before ``PrefixListModel.get_diff`` adopted + the shared signature this raised + ``TypeError: get_diff() got an unexpected keyword argument 'allow_superset'`` + whenever an existing prefix list was compared, breaking idempotency and + replace/override checks. This drives the real production model (not a fake + override) through the collection so the incompatibility cannot regress. + + ## Classes and Methods + + - PrefixListModel.get_diff + - NDConfigCollection.get_diff_config + """ + existing = PrefixListModel.from_response(copy.deepcopy(SAMPLE_IPV4_API_RESPONSE)) + collection = NDConfigCollection(model_class=PrefixListModel, items=[existing]) + + # Idempotency check on an existing prefix list must not raise and is "no_diff". + idempotent = PrefixListModel.from_config( + { + "ip_version": "ipv4", + "tenant_name": "TENANT1", + "name": "PL-IPV4-BORDERS", + "entries": copy.deepcopy(SAMPLE_IPV4_CONFIG["entries"]), + } + ) + assert collection.get_diff_config(idempotent, exclude_unset=True, allow_superset=True) == "no_diff" + + # Prefix-list entries are authoritative even for merged. The shared state + # machine requests allow_superset=True for merged resources, but this model + # must still detect an existing entry omitted from the desired list. + missing_existing_entry = PrefixListModel.from_config( + { + "ip_version": "ipv4", + "tenant_name": "TENANT1", + "name": "PL-IPV4-BORDERS", + "entries": copy.deepcopy(SAMPLE_IPV4_CONFIG["entries"][:1]), + } + ) + assert collection.get_diff_config(missing_existing_entry, exclude_unset=True, allow_superset=True) == "changed" + + # Default-expanded equality remains idempotent: omitting action means the + # documented default "permit", not a request to change an existing permit. + default_action_config = copy.deepcopy(SAMPLE_IPV4_CONFIG) + default_action_config["entries"][0].pop("action") + default_action = PrefixListModel.from_config(default_action_config) + assert collection.get_diff_config(default_action, exclude_unset=True, allow_superset=True) == "no_diff" + + # Entry order is not semantic; sequence numbers identify the entries. + reordered_config = copy.deepcopy(SAMPLE_IPV4_CONFIG) + reordered_config["entries"].reverse() + reordered = PrefixListModel.from_config(reordered_config) + assert collection.get_diff_config(reordered, exclude_unset=True, allow_superset=True) == "no_diff" + + # Conversely, omitting a controller-side length constraint from the + # authoritative desired entry requests its removal. + constrained_response = copy.deepcopy(SAMPLE_IPV4_API_RESPONSE) + constrained_response["entries"][0]["exactLength"] = 24 + constrained_existing = PrefixListModel.from_response(constrained_response) + constrained_collection = NDConfigCollection(model_class=PrefixListModel, items=[constrained_existing]) + clear_constraint = PrefixListModel.from_config(copy.deepcopy(SAMPLE_IPV4_CONFIG)) + assert constrained_collection.get_diff_config(clear_constraint, exclude_unset=True, allow_superset=True) == "changed" + + # Replace-style comparison of the same prefix list with a stale controller + # description (config omits description) reaches the override and is "changed". + stale_description = PrefixListModel.from_config( + { + "ip_version": "ipv4", + "tenant_name": "TENANT1", + "name": "PL-IPV4-BORDERS", + "entries": copy.deepcopy(SAMPLE_IPV4_CONFIG["entries"]), + } + ) + assert collection.get_diff_config(stale_description, exclude_unset=False, allow_superset=True) == "changed" + + # A different composite key short-circuits to "new" before get_diff runs. + different_name = PrefixListModel.from_config( + { + "ip_version": "ipv4", + "tenant_name": "TENANT1", + "name": "PL-IPV4-OTHER", + "entries": copy.deepcopy(SAMPLE_IPV4_CONFIG["entries"]), + } + ) + assert collection.get_diff_config(different_name, allow_superset=True) == "new" diff --git a/tests/unit/module_utils/orchestrators/test_networks.py b/tests/unit/module_utils/orchestrators/test_networks.py index 17215984b..8ac8de70a 100644 --- a/tests/unit/module_utils/orchestrators/test_networks.py +++ b/tests/unit/module_utils/orchestrators/test_networks.py @@ -8,6 +8,7 @@ from unittest.mock import patch +from ansible_collections.cisco.nd.plugins.module_utils.common.exceptions import NDStateMachineError from ansible_collections.cisco.nd.plugins.module_utils.enums import HttpVerbEnum, OperationType from ansible_collections.cisco.nd.plugins.module_utils.models.manage_networks.config_models import ( NetworkConfigModel, @@ -57,6 +58,9 @@ def fail_json(self, **kwargs): raise AssertionError(kwargs) +_MISSING_CONFIG = object() + + class _ParentStrategy: config_model_cls = NetworkParentConfigModel fabric_data = {"members": [{"fabricName": "child1"}]} @@ -127,6 +131,50 @@ def resolve(self): assert coordinator.workflow_trace[0]["event"] == "fabric_resolver_start" +@pytest.mark.parametrize("check_mode", [False, True]) +@pytest.mark.parametrize( + "config", + [ + pytest.param(_MISSING_CONFIG, id="omitted"), + pytest.param(None, id="null"), + ], +) +def test_network_workflow_rejects_missing_config_before_resolution(config, check_mode): + """Production Network workflows must not normalize missing config to [].""" + params = {"fabric_name": "fab1", "state": "overridden"} + if config is not _MISSING_CONFIG: + params["config"] = config + module = _Module(params) + module.check_mode = check_mode + coordinator = NetworkWorkflowCoordinator(module=module) + + with patch.object(coordinator, "_resolve_strategy") as resolve_strategy: + with pytest.raises(NDStateMachineError, match=r"config must be provided and cannot be null"): + coordinator.run() + + resolve_strategy.assert_not_called() + + +@pytest.mark.parametrize("check_mode", [False, True]) +def test_network_workflow_accepts_explicit_empty_config(check_mode): + """An explicit empty desired set remains valid through the Network wrapper.""" + module = _Module({"fabric_name": "fab1", "state": "overridden", "config": []}) + module.check_mode = check_mode + strategy = StandaloneNetworkStrategy( + fabric_name="fab1", + fabric_data={"managementType": "vxlanIbgp"}, + ) + coordinator = NetworkWorkflowCoordinator(module=module, strategy=strategy) + expected = {"changed": check_mode} + + with patch.object(coordinator, "_handle_standalone_workflow", return_value=expected) as handler: + result = coordinator.run() + + handler.assert_called_once() + assert handler.call_args.args[0]["config"] == [] + assert result["changed"] is check_mode + + def _mcfg_parent_orchestrator(): strategy = MulticlusterParentNetworkStrategy( fabric_name="MCFG_FAB", diff --git a/tests/unit/module_utils/test_nd_state_machine_operations.py b/tests/unit/module_utils/test_nd_state_machine_operations.py index 860db394b..88496e4ea 100644 --- a/tests/unit/module_utils/test_nd_state_machine_operations.py +++ b/tests/unit/module_utils/test_nd_state_machine_operations.py @@ -449,11 +449,13 @@ def test_overridden_deletes_non_proposed_items(): assert _names(sm.sent) == ["b", "c"] -def test_overridden_explicit_empty_config_deletes_all_existing(): +@pytest.mark.parametrize("check_mode", [False, True]) +def test_overridden_explicit_empty_config_deletes_all_existing(check_mode): """An explicit empty overridden config means delete every existing item.""" orch = _FakeOrchestrator(supports_bulk_delete=False) sm = _make_state_machine( state="overridden", + check_mode=check_mode, orchestrator=orch, existing=[_model("a", "x"), _model("b", "y")], proposed=[], @@ -461,9 +463,13 @@ def test_overridden_explicit_empty_config_deletes_all_existing(): sm.manage_state() - assert _names(orch.calls["delete"]) == ["a", "b"] assert len(sm.existing) == 0 - assert _names(sm.sent) == ["a", "b"] + if check_mode: + assert orch.calls["delete"] == [] + assert len(sm.sent) == 0 + else: + assert _names(orch.calls["delete"]) == ["a", "b"] + assert _names(sm.sent) == ["a", "b"] # ============================================================================= @@ -544,9 +550,16 @@ def test_state_machine_accepts_explicit_empty_config(state): assert len(sm.proposed) == 0 -def test_deleted_state_tolerates_null_config_as_empty(): - """Null delete config is non-destructive: it targets no proposed items.""" - module = _FakeModule(state="deleted", config=None) +@pytest.mark.parametrize( + "config", + [ + pytest.param(_MISSING, id="missing"), + pytest.param(None, id="null"), + ], +) +def test_deleted_state_tolerates_missing_or_null_config_as_empty(config): + """Missing/null delete config is non-destructive: it targets no proposed items.""" + module = _FakeModule(state="deleted", config=config) sm = NDStateMachine(module=module, model_orchestrator=_InitFakeOrchestrator) assert len(sm.proposed) == 0 diff --git a/tests/unit/modules/test_nd_manage_networks.py b/tests/unit/modules/test_nd_manage_networks.py index ae99fc07b..4f19affe6 100644 --- a/tests/unit/modules/test_nd_manage_networks.py +++ b/tests/unit/modules/test_nd_manage_networks.py @@ -11,10 +11,103 @@ __metaclass__ = type # pylint: disable=invalid-name +import json from unittest.mock import patch +import pytest +from ansible.module_utils import basic as ansible_basic +from ansible_collections.cisco.nd.plugins.module_utils.rest.sender_nd import ( + Sender, +) from ansible_collections.cisco.nd.plugins.modules import nd_manage_networks +_MISSING_CONFIG = object() + + +class _ModuleFailure(Exception): + """Capture ``AnsibleModule.fail_json`` without terminating pytest.""" + + def __init__(self, module, result): + super().__init__(result.get("msg", "module failed")) + self.module = module + self.result = result + self.controller_calls = [] + + +def _run_composed_network_module(module_args): + """ + Run Ansible parsing, the production workflow, and the real state machine. + + Only the controller transport is replaced. + """ + raw_args = json.dumps({"ANSIBLE_MODULE_ARGS": module_args}).encode() + captured = {} + controller_calls = [] + + def exit_json(module, **kwargs): + captured["module"] = module + captured["result"] = kwargs + + def fail_json(module, **kwargs): + raise _ModuleFailure(module, kwargs) + + def sender_commit(sender): + path = sender.path + verb = sender.verb.value + controller_calls.append((verb, path, sender.payload)) + if path == "/api/v1/manage/fabrics/fab1": + data = {"management": {"type": "vxlanIbgp"}} + elif path == "/api/v1/manage/fabrics?category=fabricGroup&max=10000": + data = {"fabrics": []} + elif path == "/api/v1/manage/fabrics/fab1/networks?offset=0&max=10000": + data = { + "networks": [ + {"networkName": "production"}, + {"networkName": "storage"}, + ], + "metadata": {"counts": {"total": 2, "remaining": 0}}, + } + elif verb == "POST" and path == "/api/v1/manage/fabrics/fab1/networkAttachments/query?offset=0&max=10000&includeAll=true": + data = { + "attachments": [], + "metadata": {"counts": {"total": 0, "remaining": 0}}, + } + elif verb == "POST" and path in { + "/api/v1/manage/fabrics/fab1/networkActions/deploy", + "/api/v1/manage/fabrics/fab1/networkActions/remove", + }: + data = { + "results": [ + {"networkName": "production", "status": "success"}, + {"networkName": "storage", "status": "success"}, + ], + } + else: + raise AssertionError((verb, path, sender.payload)) + sender.response = { + "RETURN_CODE": 200, + "MESSAGE": "OK", + "DATA": data, + "REQUEST_PATH": path, + "METHOD": verb, + } + + with patch.object(ansible_basic, "_ANSIBLE_ARGS", raw_args), patch.object( + nd_manage_networks.AnsibleModule, "exit_json", exit_json + ), patch.object(nd_manage_networks.AnsibleModule, "fail_json", fail_json), patch.object( + Sender, "commit", sender_commit + ): + try: + nd_manage_networks.main() + except _ModuleFailure as exc: + exc.controller_calls = list(controller_calls) + raise + + return { + **captured, + "controller_calls": controller_calls, + } + def test_nd_manage_networks_requires_pydantic_immediately_after_module_creation(): """ @@ -64,3 +157,91 @@ def fake_require_pydantic(module): ] assert events[1][1] is events[2][1] assert events[0][1].kwargs["supports_check_mode"] is True + assert "default" not in events[0][1].kwargs["argument_spec"]["config"] + + +@pytest.mark.parametrize("check_mode", [False, True]) +@pytest.mark.parametrize( + "config", + [ + pytest.param(_MISSING_CONFIG, id="omitted"), + pytest.param(None, id="null"), + ], +) +def test_nd_manage_networks_rejects_missing_overridden_config_before_query(config, check_mode): + """Omitted/null config must fail before production state discovery.""" + module_args = { + "fabric_name": "fab1", + "state": "overridden", + "_ansible_check_mode": check_mode, + "_ansible_verbosity": 0, + } + if config is not _MISSING_CONFIG: + module_args["config"] = config + + with pytest.raises(_ModuleFailure) as exc_info: + _run_composed_network_module(module_args) + + assert exc_info.value.module.params["config"] is None + assert "config must be provided and cannot be null" in exc_info.value.result["msg"] + assert exc_info.value.controller_calls == [] + + +def test_nd_manage_networks_explicit_empty_overridden_previews_existing_deletions(): + """Explicit config: [] previews delete-all through the complete workflow.""" + run = _run_composed_network_module( + { + "fabric_name": "fab1", + "state": "overridden", + "config": [], + "_ansible_check_mode": True, + "_ansible_verbosity": 0, + } + ) + + assert run["module"].params["config"] == [] + assert len(run["result"]["before"]) == 2 + assert run["result"]["after"] == [] + assert run["result"]["changed"] is True + assert run["result"]["check_mode_deploy_payloads"] == [ + {"networkNames": ["production", "storage"]}, + ] + assert len(run["controller_calls"]) == 3 + assert all(verb == "GET" for verb, _path, _payload in run["controller_calls"]) + + +def test_nd_manage_networks_explicit_empty_overridden_deletes_existing_resources(): + """Explicit config: [] performs delete-all through the complete workflow.""" + run = _run_composed_network_module( + { + "fabric_name": "fab1", + "state": "overridden", + "config": [], + "_ansible_check_mode": False, + "_ansible_verbosity": 0, + } + ) + + assert run["module"].params["config"] == [] + assert len(run["result"]["before"]) == 2 + assert run["result"]["after"] == [] + assert run["result"]["changed"] is True + assert "check_mode_deploy_payloads" not in run["result"] + mutation_calls = [ + (verb, path, payload) + for verb, path, payload in run["controller_calls"] + if path + in { + "/api/v1/manage/fabrics/fab1/networkActions/deploy", + "/api/v1/manage/fabrics/fab1/networkActions/remove", + } + ] + assert [(verb, path) for verb, path, _payload in mutation_calls] == [ + ("POST", "/api/v1/manage/fabrics/fab1/networkActions/deploy"), + ("POST", "/api/v1/manage/fabrics/fab1/networkActions/remove"), + ] + for _verb, _path, payload in mutation_calls: + assert set(payload["networkNames"]) == { + "production", + "storage", + } diff --git a/tests/unit/modules/test_nd_manage_vpc_pair.py b/tests/unit/modules/test_nd_manage_vpc_pair.py new file mode 100644 index 000000000..4aa69b436 --- /dev/null +++ b/tests/unit/modules/test_nd_manage_vpc_pair.py @@ -0,0 +1,227 @@ +# -*- coding: utf-8 -*- + +# Copyright: (c) 2026, Cisco Systems, Inc. +# GNU General Public License v3.0+ (see LICENSE or https://www.gnu.org/licenses/gpl-3.0.txt) + +"""Production-wrapper tests for ``nd_manage_vpc_pair``.""" + +from __future__ import absolute_import, annotations, division, print_function + +import json +from unittest.mock import patch + +import pytest +from ansible.module_utils import basic as ansible_basic + +from ansible_collections.cisco.nd.plugins.module_utils.nd_v2 import ( + NDModule, +) +from ansible_collections.cisco.nd.plugins.modules import nd_manage_vpc_pair + +_MISSING_CONFIG = object() + + +class _ModuleFailure(Exception): + """Capture ``AnsibleModule.fail_json`` without terminating pytest.""" + + def __init__(self, module, result): + super().__init__(result.get("msg", "module failed")) + self.module = module + self.result = result + self.controller_calls = [] + + +def _module_args(config, check_mode): + args = { + "fabric_name": "fab1", + "state": "overridden", + "force": False, + "verify": None, + "config_actions": {"save": False, "deploy": False, "type": "switch"}, + "_ansible_check_mode": check_mode, + "_ansible_verbosity": 0, + } + if config is not _MISSING_CONFIG: + args["config"] = config + return args + + +def _run_composed_vpc_pair_module(module_args): + """Run Ansible parsing, the production service, and the real state machine.""" + raw_args = json.dumps({"ANSIBLE_MODULE_ARGS": module_args}).encode() + captured = {} + controller_calls = [] + deleted_switches = set() + + def exit_json(module, **kwargs): + captured["module"] = module + captured["result"] = kwargs + + def fail_json(module, **kwargs): + raise _ModuleFailure(module, kwargs) + + def nd_request(_client, path, verb, data=None): + method = verb.value + controller_calls.append((method, path, data)) + if path == "/api/v1/manage/fabrics/fab1": + return {"fabricType": "VXLAN"} + if path.endswith("/vpcPairs"): + return { + "vpcPairs": [ + pair + for pair in [ + { + "switchId": "SWA", + "peerSwitchId": "SWB", + "useVirtualPeerLink": False, + }, + { + "switchId": "SWC", + "peerSwitchId": "SWD", + "useVirtualPeerLink": False, + }, + ] + if pair["switchId"] not in deleted_switches + ] + } + if path.endswith("/switches"): + return { + "switches": [ + { + "serialNumber": "SWA", + "vpcConfigured": True, + "vpcData": {"peerSwitchId": "SWB"}, + }, + { + "serialNumber": "SWC", + "vpcConfigured": True, + "vpcData": {"peerSwitchId": "SWD"}, + }, + ] + } + if path.endswith("/switches/SWA/vpcPair"): + if method == "PUT": + deleted_switches.add("SWA") + return {} + if "SWA" in deleted_switches: + return {} + return { + "switchId": "SWA", + "peerSwitchId": "SWB", + "useVirtualPeerLink": False, + } + if path.endswith("/switches/SWC/vpcPair"): + if method == "PUT": + deleted_switches.add("SWC") + return {} + if "SWC" in deleted_switches: + return {} + return { + "switchId": "SWC", + "peerSwitchId": "SWD", + "useVirtualPeerLink": False, + } + if path.endswith("/vpcPairConsistency"): + return {"type2Consistency": True} + if "/vpcPairOverview" in path: + return { + "overlay": { + "networkCount": {"deployed": 0}, + "vrfCount": {"deployed": 0}, + }, + "inventory": { + "syncStatus": {"pending": 0, "outOfSync": 0, "inProgress": 0}, + "vpcInterfaceCount": 0, + } + } + raise AssertionError((method, path, data)) + + with patch.object(ansible_basic, "_ANSIBLE_ARGS", raw_args), patch.object( + nd_manage_vpc_pair.AnsibleModule, "exit_json", exit_json + ), patch.object( + nd_manage_vpc_pair.AnsibleModule, "fail_json", fail_json + ), patch.object( + nd_manage_vpc_pair, "setup_logging" + ), patch.object( + NDModule, "request", nd_request + ): + try: + nd_manage_vpc_pair.main() + except _ModuleFailure as exc: + exc.controller_calls = list(controller_calls) + raise + + return { + **captured, + "controller_calls": controller_calls, + } + + +@pytest.mark.parametrize("check_mode", [False, True]) +@pytest.mark.parametrize( + "config", + [ + pytest.param(_MISSING_CONFIG, id="omitted"), + pytest.param(None, id="null"), + ], +) +def test_vpc_pair_wrapper_rejects_missing_config_before_service(config, check_mode): + """Missing overridden config must fail before state-machine construction.""" + with pytest.raises(_ModuleFailure) as exc_info: + _run_composed_vpc_pair_module(_module_args(config, check_mode)) + + assert exc_info.value.module.params["config"] is None + assert "config must be provided and cannot be null" in exc_info.value.result["msg"] + assert exc_info.value.controller_calls == [] + + +def test_vpc_pair_wrapper_explicit_empty_overridden_previews_existing_deletions(): + """Explicit config: [] reaches the real state machine safely in check mode.""" + run = _run_composed_vpc_pair_module(_module_args([], check_mode=True)) + + assert run["module"].params["config"] == [] + assert len(run["result"]["before"]) == 2 + assert run["result"]["after"] == [] + assert run["result"]["changed"] is True + assert len(run["result"]["deleted"]) == 2 + assert len(run["controller_calls"]) == 7 + assert all(method == "GET" for method, _path, _data in run["controller_calls"]) + + +def test_vpc_pair_wrapper_explicit_empty_overridden_deletes_existing_pairs(): + """Explicit config: [] performs both unpair operations in normal mode.""" + run = _run_composed_vpc_pair_module(_module_args([], check_mode=False)) + + assert run["module"].params["config"] == [] + assert len(run["result"]["before"]) == 2 + assert run["result"]["after"] == [] + assert run["result"]["current"] == [] + assert run["result"]["changed"] is True + assert len(run["result"]["deleted"]) == 2 + + put_calls = [ + (path, data) + for method, path, data in run["controller_calls"] + if method == "PUT" + ] + assert len(put_calls) == 2 + unpair_calls = { + path: data + for path, data in put_calls + } + assert unpair_calls == { + "/api/v1/manage/fabrics/fab1/switches/SWA/vpcPair": { + "vpcAction": "unPair", + "switchId": "SWA", + "peerSwitchId": "SWB", + }, + "/api/v1/manage/fabrics/fab1/switches/SWC/vpcPair": { + "vpcAction": "unPair", + "switchId": "SWC", + "peerSwitchId": "SWD", + }, + } + assert all( + method in {"GET", "PUT"} + for method, _path, _data in run["controller_calls"] + ) From e7a89389edaf11243fef1e02d73b2d44ce908bae Mon Sep 17 00:00:00 2001 From: Sivakami Sivaraman Date: Thu, 30 Jul 2026 17:09:56 +0530 Subject: [PATCH 19/22] Fix Black and pylint sanity issues --- plugins/module_utils/nd_state_machine.py | 3 +- tests/unit/modules/test_nd_manage_networks.py | 8 ++---- tests/unit/modules/test_nd_manage_vpc_pair.py | 28 ++++--------------- 3 files changed, 10 insertions(+), 29 deletions(-) diff --git a/plugins/module_utils/nd_state_machine.py b/plugins/module_utils/nd_state_machine.py index 5ee216c0a..019a530f7 100644 --- a/plugins/module_utils/nd_state_machine.py +++ b/plugins/module_utils/nd_state_machine.py @@ -37,8 +37,7 @@ def validate_config_presence(cls, state: str, config: Any) -> None: """ if state in cls.WRITE_STATES_REQUIRING_CONFIG and config is None: raise NDStateMachineError( - f"config must be provided and cannot be null for state '{state}'. " - "Use config: [] only when intentionally managing an explicit empty set." + f"config must be provided and cannot be null for state '{state}'. " "Use config: [] only when intentionally managing an explicit empty set." ) def __init__(self, module: AnsibleModule, model_orchestrator: type[NDBaseOrchestrator] | NDBaseOrchestrator): diff --git a/tests/unit/modules/test_nd_manage_networks.py b/tests/unit/modules/test_nd_manage_networks.py index 4f19affe6..b1a4759fa 100644 --- a/tests/unit/modules/test_nd_manage_networks.py +++ b/tests/unit/modules/test_nd_manage_networks.py @@ -92,11 +92,9 @@ def sender_commit(sender): "METHOD": verb, } - with patch.object(ansible_basic, "_ANSIBLE_ARGS", raw_args), patch.object( - nd_manage_networks.AnsibleModule, "exit_json", exit_json - ), patch.object(nd_manage_networks.AnsibleModule, "fail_json", fail_json), patch.object( - Sender, "commit", sender_commit - ): + with patch.object(ansible_basic, "_ANSIBLE_ARGS", raw_args), patch.object(nd_manage_networks.AnsibleModule, "exit_json", exit_json), patch.object( + nd_manage_networks.AnsibleModule, "fail_json", fail_json + ), patch.object(Sender, "commit", sender_commit): try: nd_manage_networks.main() except _ModuleFailure as exc: diff --git a/tests/unit/modules/test_nd_manage_vpc_pair.py b/tests/unit/modules/test_nd_manage_vpc_pair.py index 4aa69b436..3366e3062 100644 --- a/tests/unit/modules/test_nd_manage_vpc_pair.py +++ b/tests/unit/modules/test_nd_manage_vpc_pair.py @@ -132,19 +132,13 @@ def nd_request(_client, path, verb, data=None): "inventory": { "syncStatus": {"pending": 0, "outOfSync": 0, "inProgress": 0}, "vpcInterfaceCount": 0, - } + }, } raise AssertionError((method, path, data)) - with patch.object(ansible_basic, "_ANSIBLE_ARGS", raw_args), patch.object( - nd_manage_vpc_pair.AnsibleModule, "exit_json", exit_json - ), patch.object( + with patch.object(ansible_basic, "_ANSIBLE_ARGS", raw_args), patch.object(nd_manage_vpc_pair.AnsibleModule, "exit_json", exit_json), patch.object( nd_manage_vpc_pair.AnsibleModule, "fail_json", fail_json - ), patch.object( - nd_manage_vpc_pair, "setup_logging" - ), patch.object( - NDModule, "request", nd_request - ): + ), patch.object(nd_manage_vpc_pair, "setup_logging"), patch.object(NDModule, "request", nd_request): try: nd_manage_vpc_pair.main() except _ModuleFailure as exc: @@ -199,16 +193,9 @@ def test_vpc_pair_wrapper_explicit_empty_overridden_deletes_existing_pairs(): assert run["result"]["changed"] is True assert len(run["result"]["deleted"]) == 2 - put_calls = [ - (path, data) - for method, path, data in run["controller_calls"] - if method == "PUT" - ] + put_calls = [(path, data) for method, path, data in run["controller_calls"] if method == "PUT"] assert len(put_calls) == 2 - unpair_calls = { - path: data - for path, data in put_calls - } + unpair_calls = dict(put_calls) assert unpair_calls == { "/api/v1/manage/fabrics/fab1/switches/SWA/vpcPair": { "vpcAction": "unPair", @@ -221,7 +208,4 @@ def test_vpc_pair_wrapper_explicit_empty_overridden_deletes_existing_pairs(): "peerSwitchId": "SWD", }, } - assert all( - method in {"GET", "PUT"} - for method, _path, _data in run["controller_calls"] - ) + assert all(method in {"GET", "PUT"} for method, _path, _data in run["controller_calls"]) From c2bdb2cdc95cf0132cf09dddb70bf9e216fd7ef8 Mon Sep 17 00:00:00 2001 From: Sivakami Sivaraman Date: Sat, 1 Aug 2026 00:09:01 +0530 Subject: [PATCH 20/22] Set serialization profile in networks/vpc_pair unit tests for ansible-core 2.19 ansible-core 2.19 requires a non-empty _ANSIBLE_PROFILE to decode _ANSIBLE_ARGS when constructing AnsibleModule. Patch _ANSIBLE_PROFILE="legacy" (create=True) in the composed-module tests so they pass on 2.19 while staying compatible with 2.18. --- tests/unit/modules/test_nd_manage_networks.py | 7 ++++--- tests/unit/modules/test_nd_manage_vpc_pair.py | 9 ++++++--- 2 files changed, 10 insertions(+), 6 deletions(-) diff --git a/tests/unit/modules/test_nd_manage_networks.py b/tests/unit/modules/test_nd_manage_networks.py index b1a4759fa..4fcb1a369 100644 --- a/tests/unit/modules/test_nd_manage_networks.py +++ b/tests/unit/modules/test_nd_manage_networks.py @@ -92,9 +92,10 @@ def sender_commit(sender): "METHOD": verb, } - with patch.object(ansible_basic, "_ANSIBLE_ARGS", raw_args), patch.object(nd_manage_networks.AnsibleModule, "exit_json", exit_json), patch.object( - nd_manage_networks.AnsibleModule, "fail_json", fail_json - ), patch.object(Sender, "commit", sender_commit): + # ansible-core 2.19+ requires a serialization profile to decode _ANSIBLE_ARGS. + with patch.object(ansible_basic, "_ANSIBLE_ARGS", raw_args), patch.object(ansible_basic, "_ANSIBLE_PROFILE", "legacy", create=True), patch.object( + nd_manage_networks.AnsibleModule, "exit_json", exit_json + ), patch.object(nd_manage_networks.AnsibleModule, "fail_json", fail_json), patch.object(Sender, "commit", sender_commit): try: nd_manage_networks.main() except _ModuleFailure as exc: diff --git a/tests/unit/modules/test_nd_manage_vpc_pair.py b/tests/unit/modules/test_nd_manage_vpc_pair.py index 3366e3062..bee7d2b29 100644 --- a/tests/unit/modules/test_nd_manage_vpc_pair.py +++ b/tests/unit/modules/test_nd_manage_vpc_pair.py @@ -136,9 +136,12 @@ def nd_request(_client, path, verb, data=None): } raise AssertionError((method, path, data)) - with patch.object(ansible_basic, "_ANSIBLE_ARGS", raw_args), patch.object(nd_manage_vpc_pair.AnsibleModule, "exit_json", exit_json), patch.object( - nd_manage_vpc_pair.AnsibleModule, "fail_json", fail_json - ), patch.object(nd_manage_vpc_pair, "setup_logging"), patch.object(NDModule, "request", nd_request): + # ansible-core 2.19+ requires a serialization profile to decode _ANSIBLE_ARGS. + with patch.object(ansible_basic, "_ANSIBLE_ARGS", raw_args), patch.object(ansible_basic, "_ANSIBLE_PROFILE", "legacy", create=True), patch.object( + nd_manage_vpc_pair.AnsibleModule, "exit_json", exit_json + ), patch.object(nd_manage_vpc_pair.AnsibleModule, "fail_json", fail_json), patch.object(nd_manage_vpc_pair, "setup_logging"), patch.object( + NDModule, "request", nd_request + ): try: nd_manage_vpc_pair.main() except _ModuleFailure as exc: From e6425477acc59636981ea80c59bcd02ef2fab242 Mon Sep 17 00:00:00 2001 From: Sivakami Sivaraman Date: Wed, 5 Aug 2026 00:39:52 +0530 Subject: [PATCH 21/22] Addressing review comments --- plugins/module_utils/nd_config_collection.py | 2 +- plugins/module_utils/nd_state_machine.py | 20 +++++++------ plugins/module_utils/utils.py | 10 +++---- .../test_nd_state_machine_operations.py | 29 ++++++++++--------- 4 files changed, 32 insertions(+), 29 deletions(-) diff --git a/plugins/module_utils/nd_config_collection.py b/plugins/module_utils/nd_config_collection.py index 6e612b006..1185384d1 100644 --- a/plugins/module_utils/nd_config_collection.py +++ b/plugins/module_utils/nd_config_collection.py @@ -252,7 +252,7 @@ def to_payload_list(self, **kwargs) -> List[Dict[str, Any]]: return [item.to_payload(**kwargs) for item in self._items] @staticmethod - def from_ansible_config(data: Optional[List[Dict]], model_class: type[NDBaseModel], **kwargs) -> "NDConfigCollection": + def from_ansible_config(data: list[dict] | None, model_class: type[NDBaseModel], **kwargs) -> "NDConfigCollection": """ Create collection from Ansible config. diff --git a/plugins/module_utils/nd_state_machine.py b/plugins/module_utils/nd_state_machine.py index 019a530f7..2f585f7c5 100644 --- a/plugins/module_utils/nd_state_machine.py +++ b/plugins/module_utils/nd_state_machine.py @@ -252,11 +252,13 @@ def _manage_create_update_state(self) -> None: if self._execute_operation(self.model_orchestrator.create, item, error_msg_prefix=f"Failed to create {item.get_identifier_value()}"): successfully_sent.append(item) - # Mark as sent only for items actually pushed to the controller. In - # check mode no API call is made, so nothing is marked as sent (avoids - # false deploy triggers); the previewed 'after' state is still reflected - # in self.existing (mutated above), which is what drives 'changed'. - if not self.check_mode and successfully_sent: + # Mark successfully-processed items as sent. This stays populated in + # check mode (PR #225) so downstream config-save/deploy consumers that + # gate on len(sent) > 0 can still preview what a real run would send; + # execute_config_actions() is itself check-mode-safe (it simulates + # rather than sends). Per-item gating keeps items whose operation raised + # under ignore_errors out of 'sent'. + if successfully_sent: self.sent.add_many(successfully_sent) # Log operation @@ -299,10 +301,10 @@ def _delete_items(self, items: list[NDBaseModel]) -> None: # Batch remove from collection (single index rebuild). self.existing.delete_many([item.get_identifier_value() for item in deleted]) - # Mark as sent only for items actually pushed to the controller. In - # check mode no API call is made, so nothing is marked as sent (avoids - # false deploy triggers). - if not self.check_mode and deleted: + # Mark successfully-deleted items as sent. Stays populated in check mode + # (PR #225) so downstream config-save/deploy previews are not skipped; + # per-item gating keeps failed deletes (under ignore_errors) out of 'sent'. + if deleted: self.sent.add_many(deleted) # Log deletion diff --git a/plugins/module_utils/utils.py b/plugins/module_utils/utils.py index bd1fdfd72..a806d94c3 100644 --- a/plugins/module_utils/utils.py +++ b/plugins/module_utils/utils.py @@ -6,7 +6,7 @@ import logging from copy import deepcopy -from typing import Any, Dict, List, Set +from typing import Any from ansible_collections.cisco.nd.plugins.module_utils.endpoints.v1.manage.manage_fabrics_actions_config_save import ( EpFabricConfigSavePost, @@ -41,7 +41,7 @@ def sanitize_dict(dict_to_sanitize, keys=None, values=None, recursive=True, remo return result -def _has_perfect_matching(adjacency: List[List[int]]) -> bool: +def _has_perfect_matching(adjacency: list[list[int]]) -> bool: """Return True if every subset item can be matched to a distinct candidate. ``adjacency[i]`` holds the indices of the candidates that subset item ``i`` @@ -50,9 +50,9 @@ def _has_perfect_matching(adjacency: List[List[int]]) -> bool: consumes a candidate that a more-specific item needs. """ # candidate index -> subset item index it is currently assigned to - match_to_item: Dict[int, int] = {} + match_to_item: dict[int, int] = {} - def _try_assign(item_index: int, visited: Set[int]) -> bool: + def _try_assign(item_index: int, visited: set[int]) -> bool: for candidate_index in adjacency[item_index]: if candidate_index in visited: continue @@ -113,7 +113,7 @@ def issubset(subset: Any, superset: Any, allow_superset: bool = False) -> bool: # candidates it can match. A full matching is then required so a # less-specific item cannot greedily consume a candidate that a # more-specific item needs (relevant under allow_superset=True). - adjacency: List[List[int]] = [] + adjacency: list[list[int]] = [] for item in subset: matches = [] for index, candidate in enumerate(superset): diff --git a/tests/unit/module_utils/test_nd_state_machine_operations.py b/tests/unit/module_utils/test_nd_state_machine_operations.py index 88496e4ea..dc0d4ab7b 100644 --- a/tests/unit/module_utils/test_nd_state_machine_operations.py +++ b/tests/unit/module_utils/test_nd_state_machine_operations.py @@ -12,8 +12,9 @@ - ``_execute_operation`` returns a boolean success signal and skips the API call in check mode. -- creates/updates/deletes are added to ``sent`` only after a successful, non - check-mode operation (so check-mode runs do not produce false deploy triggers). +- creates/updates/deletes are added to ``sent`` after a successful operation + (per-item gating); ``sent`` stays populated in check mode so downstream + config-save/deploy previews are not skipped (PR #225). - under ``ignore_errors`` a failed delete leaves the item in ``existing`` and out of ``sent``; without it the failure is raised as ``NDStateMachineError``. - bulk-delete failure keeps every item, while bulk-delete success removes only @@ -219,43 +220,43 @@ def test_manage_state_invalid_state_raises(): # ============================================================================= -# check mode: API is skipped and nothing is marked sent +# check mode: API is skipped but items are still marked sent (deploy preview) # ============================================================================= -def test_check_mode_create_skips_api_and_sent(): - """Check-mode create previews the new item but issues no API call/sent.""" +def test_check_mode_create_skips_api_but_marks_sent(): + """Check-mode create previews the new item and marks it sent (no API call).""" orch = _FakeOrchestrator() sm = _make_state_machine(state="merged", check_mode=True, orchestrator=orch, proposed=[_model("a", "x")]) sm.manage_state() assert orch.calls["create"] == [] # no API call in check mode - assert len(sm.sent) == 0 # nothing marked sent -> no false deploy trigger + assert _names(sm.sent) == ["a"] # sent stays populated so deploy preview is not skipped assert sm.existing.get("a") is not None # previewed 'after' still reflects it -def test_check_mode_update_skips_api_and_sent(): - """Check-mode update previews the change but issues no API call/sent.""" +def test_check_mode_update_skips_api_but_marks_sent(): + """Check-mode update previews the change and marks it sent (no API call).""" orch = _FakeOrchestrator() sm = _make_state_machine(state="replaced", check_mode=True, orchestrator=orch, existing=[_model("a", "x")], proposed=[_model("a", "y")]) sm.manage_state() assert orch.calls["update"] == [] - assert len(sm.sent) == 0 + assert _names(sm.sent) == ["a"] assert sm.existing.get("a").value == "y" # previewed change -def test_check_mode_delete_skips_api_and_sent(): - """Check-mode delete previews the removal but issues no API call/sent.""" +def test_check_mode_delete_skips_api_but_marks_sent(): + """Check-mode delete previews the removal and marks it sent (no API call).""" orch = _FakeOrchestrator() sm = _make_state_machine(state="deleted", check_mode=True, orchestrator=orch, existing=[_model("a", "x")], proposed=[_model("a")]) sm.manage_state() assert orch.calls["delete"] == [] - assert len(sm.sent) == 0 + assert _names(sm.sent) == ["a"] assert len(sm.existing) == 0 # previewed removal @@ -465,8 +466,8 @@ def test_overridden_explicit_empty_config_deletes_all_existing(check_mode): assert len(sm.existing) == 0 if check_mode: - assert orch.calls["delete"] == [] - assert len(sm.sent) == 0 + assert orch.calls["delete"] == [] # no API call in check mode + assert _names(sm.sent) == ["a", "b"] # but the deletes are previewed as sent else: assert _names(orch.calls["delete"]) == ["a", "b"] assert _names(sm.sent) == ["a", "b"] From c3507e84d7dc5c9c428680e85e0712b7be7616d0 Mon Sep 17 00:00:00 2001 From: Sivakami Sivaraman Date: Wed, 5 Aug 2026 01:02:39 +0530 Subject: [PATCH 22/22] Fixing sanity issues --- plugins/module_utils/nd_config_collection.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugins/module_utils/nd_config_collection.py b/plugins/module_utils/nd_config_collection.py index 1185384d1..641aa63cc 100644 --- a/plugins/module_utils/nd_config_collection.py +++ b/plugins/module_utils/nd_config_collection.py @@ -2,7 +2,7 @@ # GNU General Public License v3.0+ (see LICENSE or https://www.gnu.org/licenses/gpl-3.0.txt) -from __future__ import absolute_import, division, print_function +from __future__ import absolute_import, annotations, division, print_function from copy import deepcopy from typing import Any, Dict, List, Literal, Optional