diff --git a/plugins/module_utils/orchestrators/manage_acl.py b/plugins/module_utils/orchestrators/manage_acl.py index f4c37a8aa..be2b7c70f 100644 --- a/plugins/module_utils/orchestrators/manage_acl.py +++ b/plugins/module_utils/orchestrators/manage_acl.py @@ -6,7 +6,7 @@ from __future__ import annotations -from typing import Any, ClassVar +from typing import ClassVar from ansible_collections.cisco.nd.plugins.module_utils.endpoints.base import NDEndpointBaseModel from ansible_collections.cisco.nd.plugins.module_utils.endpoints.v1.manage.manage_acl import ( @@ -22,11 +22,6 @@ from ansible_collections.cisco.nd.plugins.module_utils.orchestrators.base import NDBaseOrchestrator from ansible_collections.cisco.nd.plugins.module_utils.orchestrators.types import ResponseType -# Per-item ``status`` values in a 207 Multi-Status body that count as a failure. Anything else -- -# ``success``, missing, empty, or future progress tokens -- is tolerated so informational rows do -# not surface as spurious errors. Mirrors the prefix-list orchestrator's denylist approach. -_FAILURE_STATUSES = frozenset({"failed", "failure", "error"}) - # camelCase wrapper keys used in ACL request/response bodies. _LIST_KEY = "accessControlLists" _NAMES_KEY = "accessControlListNames" @@ -47,11 +42,10 @@ class ManageAclOrchestrator(NDBaseOrchestrator[AclModel]): - bulk delete: ``POST /fabrics/{fabricName}/accessControlListActions/remove`` with ``{"accessControlListNames": [...]}``. - Because the controller answers these bulk calls with 207 (which - ``ResponseHandler`` treats as transport success), every bulk response body is - inspected per item; any entry whose ``status`` is in ``_FAILURE_STATUSES`` - raises with the offending ACL names, so partial failures are not silently - reported as success. + The controller answers these bulk calls with 207 Multi-Status even when some + items fail; ``NdV1Strategy`` inspects the per-item ``results`` array and marks + the request failed on any failing item, so partial failures surface as errors + from ``_request`` rather than being silently reported as success. The ``fabric_name`` field is read from ``rest_send.params`` (populated by ``NDStateMachine`` from the validated module params). @@ -85,31 +79,6 @@ def fabric_name(self) -> str: """ return self.rest_send.params.get("fabric_name") - @staticmethod - def _raise_on_207_failures(result: Any, operation: str) -> None: - """ - Inspect a 207 Multi-Status bulk response body. If any per-item ``status`` - is in ``_FAILURE_STATUSES``, raise with the offending ACL names and - messages so partial failures are not silently swallowed. - """ - if not isinstance(result, dict): - return - items = result.get("results") - if not isinstance(items, list) or not items: - return - failures: list[str] = [] - for item in items: - if not isinstance(item, dict): - continue - status = str(item.get("status") or "").lower() - if status not in _FAILURE_STATUSES: - continue - name = item.get("name") or "?" - message = item.get("message") or "unknown error" - failures.append(f"{name}: {message}") - if failures: - raise Exception(f"ACL {operation} reported per-item failures: {'; '.join(failures)}") - def create(self, model_instance: AclModel, **kwargs) -> ResponseType: """Create a single ACL via the bulk endpoint.""" try: @@ -191,25 +160,21 @@ def query_all(self, model_instance: AclModel = None, **kwargs) -> ResponseType: raise Exception(f"Query all failed: {e}") from e def create_bulk(self, model_instances: list[AclModel], **kwargs) -> ResponseType: - """Bulk-create ACLs in a single request and check the 207 body.""" + """Bulk-create ACLs in a single request.""" try: api_endpoint = self.create_bulk_endpoint() api_endpoint.fabric_name = self.fabric_name payload = {_LIST_KEY: [item.to_payload() for item in model_instances]} - result = self._request(path=api_endpoint.path, verb=api_endpoint.verb, data=payload) - self._raise_on_207_failures(result, "create") - return result + return self._request(path=api_endpoint.path, verb=api_endpoint.verb, data=payload) except Exception as e: raise Exception(f"Bulk create failed: {e}") from e def delete_bulk(self, model_instances: list[AclModel], **kwargs) -> ResponseType: - """Bulk-delete ACLs in a single request and check the 207 body.""" + """Bulk-delete ACLs in a single request.""" try: api_endpoint = self.delete_bulk_endpoint() api_endpoint.fabric_name = self.fabric_name payload = {_NAMES_KEY: [item.name for item in model_instances]} - result = self._request(path=api_endpoint.path, verb=api_endpoint.verb, data=payload) - self._raise_on_207_failures(result, "delete") - return result + return self._request(path=api_endpoint.path, verb=api_endpoint.verb, data=payload) except Exception as e: raise Exception(f"Bulk delete failed: {e}") from e diff --git a/plugins/module_utils/orchestrators/manage_prefix_list.py b/plugins/module_utils/orchestrators/manage_prefix_list.py index 25672c950..e48e39941 100644 --- a/plugins/module_utils/orchestrators/manage_prefix_list.py +++ b/plugins/module_utils/orchestrators/manage_prefix_list.py @@ -32,11 +32,6 @@ _QUERY_PAGE_SIZE = 100 _SCOPED_QUERY_MAX_IDENTIFIERS = 8 -# Per-item ``status`` values in a 207 Multi-Status body that count as a failure. Anything else -- -# ``success``, missing, empty, or future progress tokens -- is tolerated so informational rows do -# not surface as spurious errors. Mirrors the maintenance_mode orchestrator's denylist approach. -_FAILURE_STATUSES = frozenset({"failed", "failure", "error"}) - # Single source of truth for everything that differs between the two address families: the endpoint # classes plus the camelCase wrapper keys used in request/response bodies. Centralising this here # keeps every CRUD/bulk method address-family agnostic (no scattered ``"ipv4..." if v == "ipv4"``). @@ -83,10 +78,10 @@ class ManagePrefixListOrchestrator(NDBaseOrchestrator[PrefixListModel]): - IPv6 bulk delete: ``POST /fabrics/{fabricName}/ipv6PrefixListActions/remove`` with ``{"ipv6PrefixListNames": [...]}``. - Because the controller answers these bulk calls with 207 (which ``ResponseHandler`` - treats as transport success), every bulk response body is inspected per item; any - entry whose ``status`` is in ``_FAILURE_STATUSES`` raises with the offending prefix - list names, so partial failures are not silently reported as success. + The controller answers these bulk calls with 207 Multi-Status even when some items + fail; ``NdV1Strategy`` inspects the per-item ``results`` array and marks the request + failed on any failing item, so partial failures surface as errors from ``_request`` + rather than being silently reported as success. ``query_all`` fetches both IPv4 and IPv6 prefix lists and injects the ``ipVersion`` key into each raw API response dict so ``PrefixListModel`` @@ -278,48 +273,19 @@ def _query_all_for_version(self, version: str) -> list[dict[str, Any]]: offset += len(page) return results - @staticmethod - def _raise_on_207_failures(result: Any, operation: str) -> None: - """ - Inspect a 207 Multi-Status bulk response body. If any per-item ``status`` is in - ``_FAILURE_STATUSES``, raise with the offending prefix list names and messages so partial - failures are not silently swallowed (the controller returns 207 even when some items fail). - """ - if not isinstance(result, dict): - return - items = result.get("results") - if not isinstance(items, list) or not items: - return - failures: list[str] = [] - for item in items: - if not isinstance(item, dict): - continue - status = str(item.get("status") or "").lower() - if status not in _FAILURE_STATUSES: - continue - name = item.get("name") or "?" - message = item.get("message") or "unknown error" - failures.append(f"{name}: {message}") - if failures: - raise Exception(f"prefix list {operation} reported per-item failures: {'; '.join(failures)}") - def _bulk_create_for_version(self, version: str, items: list[PrefixListModel]) -> ResponseType: - """Send a single bulk-create request for all items of the given ip_version and check the 207 body.""" + """Send a single bulk-create request for all items of the given ip_version.""" config = self._config_for_version(version) api_endpoint = self._configure_endpoint(config["post"]()) payload = {config["list_key"]: [item.to_payload() for item in items]} - result = self._request(path=api_endpoint.path, verb=api_endpoint.verb, data=payload, operation_type=OperationType.CREATE) - self._raise_on_207_failures(result, "create") - return result + return self._request(path=api_endpoint.path, verb=api_endpoint.verb, data=payload, operation_type=OperationType.CREATE) def _bulk_delete_for_version(self, version: str, names: list[str]) -> ResponseType: - """Send a single bulk-delete request for the given prefix list names and check the 207 body.""" + """Send a single bulk-delete request for the given prefix list names.""" config = self._config_for_version(version) api_endpoint = self._configure_endpoint(config["bulk_delete"]()) payload = {config["names_key"]: names} - result = self._request(path=api_endpoint.path, verb=api_endpoint.verb, data=payload, operation_type=OperationType.DELETE) - self._raise_on_207_failures(result, "delete") - return result + return self._request(path=api_endpoint.path, verb=api_endpoint.verb, data=payload, operation_type=OperationType.DELETE) def create(self, model_instance: PrefixListModel, **kwargs) -> ResponseType: """Create a single prefix list via the bulk endpoint.""" diff --git a/plugins/module_utils/orchestrators/manage_route_map.py b/plugins/module_utils/orchestrators/manage_route_map.py index 585a00f8a..75cd97c9a 100644 --- a/plugins/module_utils/orchestrators/manage_route_map.py +++ b/plugins/module_utils/orchestrators/manage_route_map.py @@ -10,8 +10,8 @@ from ansible_collections.cisco.nd.plugins.module_utils.endpoints.base import NDEndpointBaseModel from ansible_collections.cisco.nd.plugins.module_utils.endpoints.v1.manage.manage_route_maps import ( - EpManageRouteMapsDelete, EpManageRouteMapsBulkDelete, + EpManageRouteMapsDelete, EpManageRouteMapsGet, EpManageRouteMapsListGet, EpManageRouteMapsPost, @@ -24,11 +24,6 @@ from ansible_collections.cisco.nd.plugins.module_utils.orchestrators.base import NDBaseOrchestrator from ansible_collections.cisco.nd.plugins.module_utils.orchestrators.types import ResponseType -# Per-item ``status`` values in a 207 Multi-Status body that count as a failure. Anything else -- -# ``success``, missing, empty, or future progress tokens -- is tolerated so informational rows do -# not surface as spurious errors. Mirrors the ACL orchestrator's denylist approach. -_FAILURE_STATUSES = frozenset({"failed", "failure", "error"}) - # camelCase wrapper key used in route-map list responses and bulk-create request bodies. _LIST_KEY = "routeMaps" @@ -99,28 +94,6 @@ def preflight(self, model_instances: list[RouteMapModel]) -> None: if model_instances: self.fabric_context.validate_for_mutation() - @staticmethod - def _raise_on_bulk_errors(result: ResponseType, action: str) -> None: - """Raise when a 207 bulk response contains failed per-item results.""" - if not isinstance(result, dict): - return - failures = [] - for item in result.get("results", []): - if not isinstance(item, dict): - continue - status = str(item.get("status") or "").lower() - if status in _FAILURE_STATUSES: - failures.append(item) - if not failures: - return - details = [] - for item in failures: - name = item.get("name") or "" - status = item.get("status") or "" - message = item.get("message") or "no message" - details.append(f"{name}: {status}: {message}") - raise RuntimeError(f"Route map bulk {action} failed for {', '.join(details)}") - # ------------------------------------------------------------------------- # Query helpers # ------------------------------------------------------------------------- @@ -226,9 +199,7 @@ def create_bulk(self, model_instances: list[RouteMapModel], **kwargs) -> Respons try: api_endpoint = self._configure_endpoint(self.create_bulk_endpoint()) payload = {_LIST_KEY: [item.to_payload() for item in model_instances]} - result = self._request(path=api_endpoint.path, verb=api_endpoint.verb, data=payload, operation_type=OperationType.CREATE) - self._raise_on_bulk_errors(result, "create") - return result + return self._request(path=api_endpoint.path, verb=api_endpoint.verb, data=payload, operation_type=OperationType.CREATE) except Exception as e: raise Exception(f"Bulk create failed: {e}") from e @@ -243,8 +214,6 @@ def delete_bulk(self, model_instances: list[RouteMapModel], **kwargs) -> Respons api_endpoint = self._configure_endpoint(self.delete_bulk_endpoint()) route_map_names = [item.get_identifier_value() for item in model_instances] payload = {"routeMapNames": route_map_names} - result = self._request(path=api_endpoint.path, verb=api_endpoint.verb, data=payload, operation_type=OperationType.DELETE) - self._raise_on_bulk_errors(result, "delete") - return result + return self._request(path=api_endpoint.path, verb=api_endpoint.verb, data=payload, operation_type=OperationType.DELETE) except Exception as e: raise Exception(f"Bulk delete failed: {e}") from e diff --git a/plugins/module_utils/orchestrators/subinterface_managed_interface.py b/plugins/module_utils/orchestrators/subinterface_managed_interface.py index 63b403b36..737fe222a 100644 --- a/plugins/module_utils/orchestrators/subinterface_managed_interface.py +++ b/plugins/module_utils/orchestrators/subinterface_managed_interface.py @@ -72,32 +72,6 @@ class SubinterfaceManagedInterfaceOrchestrator(NDBaseInterfaceOrchestrator[Subin supports_bulk_create: ClassVar[bool] = True supports_bulk_delete: ClassVar[bool] = True - # TODO(4.2.1) ND returns HTTP 207 Multi-Status on subinterface POST with per-item `status: "failed"` when the parent - # interface is not in routed mode (or other policy validation fails). Our RestSend response_handler treats 207 as - # success and returns the body without raising, so without this check the orchestrator would silently report - # "changed" when nothing was actually created. Remove this workaround once CiscoDevNet/ansible-nd#295 lands the - # 207-aware response handling at the RestSend layer. - @staticmethod - def _raise_on_multi_status_failures(response: ResponseType) -> None: - """ - # Summary - - Inspect a 207 Multi-Status body and raise if any item carries `status: "failed"` or `status: "error"`. - - ## Raises - - ### RuntimeError - - - If `response["results"]` contains any item with `status` in `("failed", "error")`. - """ - if not isinstance(response, dict): - return - results = response.get("results") or [] - failed = [r for r in results if isinstance(r, dict) and r.get("status") in ("failed", "error")] - if failed: - summary = "; ".join(f"{r.get('name')}: {r.get('message')}" for r in failed) - raise RuntimeError(f"ND rejected {len(failed)} interface(s): {summary}") - create_endpoint: type[NDEndpointBaseModel] = EpManageInterfacesPost update_endpoint: type[NDEndpointBaseModel] = EpManageInterfacesPut delete_endpoint: type[NDEndpointBaseModel] = NDEndpointBaseModel # unused; delete() uses bulk remove @@ -126,7 +100,6 @@ def create(self, model_instance: SubinterfaceManagedInterfaceModel, **kwargs) -> payload["switchId"] = switch_id request_body = {"interfaces": [payload]} result = self._request(path=api_endpoint.path, verb=api_endpoint.verb, data=request_body) - self._raise_on_multi_status_failures(result) self._queue_deploy(model_instance.interface_name, switch_id) return result except Exception as e: @@ -202,7 +175,6 @@ def create_bulk(self, model_instances: list[SubinterfaceManagedInterfaceModel], api_endpoint = self._configure_endpoint(self.create_bulk_endpoint(), switch_sn=switch_id) # pyright: ignore[reportOptionalCall] request_body = {"interfaces": [payload for interface_name, payload in items]} result = self._request(path=api_endpoint.path, verb=api_endpoint.verb, data=request_body) - self._raise_on_multi_status_failures(result) results.append(result) for interface_name, payload in items: self._queue_deploy(interface_name, switch_id) diff --git a/plugins/module_utils/orchestrators/subinterface_unmanaged_interface.py b/plugins/module_utils/orchestrators/subinterface_unmanaged_interface.py index 13a597189..b3270d801 100644 --- a/plugins/module_utils/orchestrators/subinterface_unmanaged_interface.py +++ b/plugins/module_utils/orchestrators/subinterface_unmanaged_interface.py @@ -60,35 +60,6 @@ class SubinterfaceUnmanagedInterfaceOrchestrator(NDBaseInterfaceOrchestrator[Sub create_bulk_endpoint: type[NDEndpointBaseModel] | None = EpManageInterfacesPost delete_bulk_endpoint: type[NDEndpointBaseModel] | None = EpManageInterfacesRemove - # TODO(4.2.1) multi-status-207-status-field-inconsistent - # ND returns HTTP 207 Multi-Status on subinterface POST with per-item `status: "failed"` when the parent - # interface is not in routed mode (or other policy validation fails). Our RestSend response_handler treats 207 as - # success and returns the body without raising, so without this check the orchestrator would silently report - # "changed" when nothing was actually created. Remove this workaround once CiscoDevNet/ansible-nd#295 lands the - # 207-aware response handling at the RestSend layer. - @staticmethod - def _raise_on_multi_status_failures(response: ResponseType) -> None: - """ - # Summary - - Inspect a 207 Multi-Status body and raise if any item carries `status: "failed"` or `status: "error"`. The - comparison is case-insensitive and whitespace-tolerant because ND is inconsistent about the casing of the - per-item `status` value across endpoints (see the `multi-status-207-status-field-inconsistent` vault note). - - ## Raises - - ### RuntimeError - - - If `response["results"]` contains any item whose `status` is `"failed"` or `"error"` (any casing). - """ - if not isinstance(response, dict): - return - results = response.get("results") or [] - failed = [r for r in results if isinstance(r, dict) and str(r.get("status") or "").strip().lower() in ("failed", "error")] - if failed: - summary = "; ".join(f"{r.get('name')}: {r.get('message')}" for r in failed) - raise RuntimeError(f"ND rejected {len(failed)} interface(s): {summary}") - def create(self, model_instance: SubinterfaceUnmanagedInterfaceModel, **kwargs) -> ResponseType: """ # Summary @@ -110,7 +81,6 @@ def create(self, model_instance: SubinterfaceUnmanagedInterfaceModel, **kwargs) payload["switchId"] = switch_id request_body = {"interfaces": [payload]} result = self._request(path=api_endpoint.path, verb=api_endpoint.verb, data=request_body) - self._raise_on_multi_status_failures(result) self._queue_deploy(model_instance.interface_name, switch_id) return result except Exception as e: @@ -186,7 +156,6 @@ def create_bulk(self, model_instances: list[SubinterfaceUnmanagedInterfaceModel] api_endpoint = self._configure_endpoint(self.create_bulk_endpoint(), switch_sn=switch_id) # pyright: ignore[reportOptionalCall] request_body = {"interfaces": [payload for interface_name, payload in items]} result = self._request(path=api_endpoint.path, verb=api_endpoint.verb, data=request_body) - self._raise_on_multi_status_failures(result) results.append(result) for interface_name, payload in items: self._queue_deploy(interface_name, switch_id) diff --git a/plugins/module_utils/rest/protocols/response_validation.py b/plugins/module_utils/rest/protocols/response_validation.py index 30a81b972..d63d14926 100644 --- a/plugins/module_utils/rest/protocols/response_validation.py +++ b/plugins/module_utils/rest/protocols/response_validation.py @@ -174,6 +174,34 @@ def is_changed(self, response: dict) -> bool: """ ... + def is_changed_on_failure(self, response: dict) -> bool: + """ + # Summary + + Report whether a failed mutation nevertheless changed controller state. + + ## Description + + A partial-success response (e.g. HTTP 207 with mixed per-item outcomes) is an aggregate failure that still mutated state. Implementations + should honour the `modified` response header when present and otherwise inspect per-item outcomes, returning `True` when any member + succeeded. This method should only be called after `is_success` has returned `False`. + + ## Parameters + + - response: Response dict with keys RETURN_CODE, MESSAGE, DATA, and any HTTP + response headers (lowercased) forwarded by the HttpAPI plugin. + + ## Returns + + - True if the failed operation changed state + - False otherwise + + ## Raises + + None + """ + ... + def extract_error_message(self, response: dict) -> Optional[str]: """ # Summary diff --git a/plugins/module_utils/rest/response_handler_nd.py b/plugins/module_utils/rest/response_handler_nd.py index f0f30b948..a20aaae69 100644 --- a/plugins/module_utils/rest/response_handler_nd.py +++ b/plugins/module_utils/rest/response_handler_nd.py @@ -206,16 +206,25 @@ def _handle_post_put_delete_response(self) -> None: """ # Summary - Handle POST, PUT, DELETE responses from the controller and set - self.result. - - - self.result is a dict containing: - - changed: - - True if RETURN_CODE in (200, 201, 202, 204, 207) and no ERROR - - False otherwise - - success: - - True if RETURN_CODE in (200, 201, 202, 204, 207) and no ERROR + Handle POST, PUT, DELETE responses from the controller and set `self.result`. + + - `self.result` is a dict containing: + - `changed`: + - True if RETURN_CODE in (200, 201, 202, 204, 207) and no embedded error + - On failure: True when the `modified` header or a per-item `success` status shows state changed (mixed + Multi-Status batches), False otherwise + - `success`: + - True if RETURN_CODE in (200, 201, 202, 204, 207) and no embedded error - False otherwise + - `retryable`: + - False when the request succeeded, or when it failed with a success-class RETURN_CODE (the application + definitively rejected the request, e.g. a Multi-Status per-item failure — replaying the identical + payload cannot succeed, so `RestSend` must not retry) + - True when the request failed with a non-success RETURN_CODE (e.g. 5xx — potentially transient) + + ## Raises + + None """ result = {} @@ -225,9 +234,15 @@ def _handle_post_put_delete_response(self) -> None: if self._strategy.is_success(self.response): result["success"] = True result["changed"] = self._strategy.is_changed(self.response) + result["retryable"] = False else: + # A failure on a success-class RETURN_CODE is an application-level rejection + # (embedded error or per-item failure): deterministic, so not retryable. + # A failure on a non-success RETURN_CODE keeps the historical retry behavior. + return_code = self.response.get("RETURN_CODE", -1) result["success"] = False - result["changed"] = False + result["changed"] = self._strategy.is_changed_on_failure(self.response) + result["retryable"] = return_code not in self._strategy.success_codes self.result = copy.copy(result) diff --git a/plugins/module_utils/rest/response_strategies/nd_v1_strategy.py b/plugins/module_utils/rest/response_strategies/nd_v1_strategy.py index a59537895..acf6f1ada 100644 --- a/plugins/module_utils/rest/response_strategies/nd_v1_strategy.py +++ b/plugins/module_utils/rest/response_strategies/nd_v1_strategy.py @@ -26,7 +26,160 @@ # pylint: enable=invalid-name -from typing import Any, Optional +from collections.abc import Iterable, Mapping +from typing import Any, Optional, TypeVar + +T = TypeVar("T") + +# Per-item status literals that mark a failure inside a Multi-Status body. ND sends these +# on HTTP 207 and, for some endpoints, on HTTP 200 as well, so the body is scanned on any +# success code. ND is also inconsistent about the literal across endpoints: "failed" (batch +# interface / switchActions/deploy), "error" (breakout), and "failure" (acl / maintenance_mode). +_MULTISTATUS_FAILURE_STATUSES = frozenset({"failed", "failure", "error"}) + +# Per-item status literal that marks a successful item in a Multi-Status body. +_MULTISTATUS_SUCCESS_STATUSES = frozenset({"success"}) + +# DATA envelope keys whose items carry a per-item `status`. Three known shapes: +# - `results` -> batch interface POST / breakout action +# - `switchIds` -> switchActions/deploy (per-switch outcome) +# - `links` -> bulk link create (POST /links) and bulk link delete (POST /linkActions/remove), +# items {linkId, message, status} with status success|failure. The GET /links list +# body rides the same `links` envelope, but its link objects carry no top-level +# `status` key, so the literal-gated scan cannot false-positive on queries. +_MULTISTATUS_ITEM_KEYS = ("results", "switchIds", "links") + +# Item keys, in priority order, used to label a failing item in an error message. +_MULTISTATUS_ITEM_LABEL_KEYS = ("name", "switchId", "serialNumber", "linkId", "id") + +# Item keys, in priority order, carrying the per-item failure detail. ND is not consistent +# across endpoints: `message` (batch interface / switchActions/deploy) and `warningMessage` +# (fabric update-group attach) are both in use. +_MULTISTATUS_ITEM_MESSAGE_KEYS = ("message", "warningMessage", "status") + + +def _get_typed_value(mapping: Mapping[str, Any], key: str, expected_type: type[T], default: T) -> T: + """ + # Summary + + Return `mapping[key]` when it is an instance of `expected_type`, else `default`. + + ## Description + + ND bodies are not schema-guaranteed: a key may be absent, or present with a `null` (or otherwise unexpected) value. `dict.get(key, default)` + only covers the absent case -- it returns `None` for an explicit `null` -- so callers that go on to index, iterate, or call `len()` need this + type-checked accessor instead. + + ## Parameters + + - mapping: The dict to read from + - key: The key to read + - expected_type: The type the value must be an instance of + - default: The value returned when the key is absent or the value has the wrong type + + ## Returns + + - The value at `key` when it is an instance of `expected_type`, otherwise `default` + + ## Raises + + None + """ + value = mapping.get(key) + return value if isinstance(value, expected_type) else default + + +def _first_non_empty(mapping: Mapping[str, Any], keys: Iterable[str]) -> str | None: + """ + # Summary + + Return the first non-empty value in `mapping` among `keys`, as a string. + + ## Description + + Keys are tried in the order given. A key whose value is absent, `None`, or stringifies to an empty (or whitespace-only) string is skipped, so an + item such as `{"name": "", "switchId": "FDO123"}` labels as `FDO123` rather than as the empty string. + + ## Parameters + + - mapping: The dict to read from + - keys: Candidate keys, in priority order + + ## Returns + + - The first non-empty value as a string, or None when every candidate key is absent or empty + + ## Raises + + None + """ + for key in keys: + value = mapping.get(key) + if value is None: + continue + text = str(value).strip() + if text: + return text + return None + + +def _multistatus_items_with_status(response: dict, statuses: frozenset[str]) -> list[dict[str, Any]]: + """ + # Summary + + Return the per-item entries in a Multi-Status body whose `status` is in `statuses`. + + ## Description + + Scans the known ND Multi-Status envelope arrays (`DATA.results[]`, `DATA.switchIds[]`, and `DATA.links[]`) and returns every item whose `status` + matches one of `statuses` (case-insensitive, whitespace-tolerant). Returns an empty list when `DATA` is not a dict, none of the arrays is + present, or no item matches. + + ## Parameters + + - response: Response dict with keys RETURN_CODE, MESSAGE, DATA, etc. + - statuses: The status literals to match (already lowercase) + + ## Returns + + - List of matching item dicts (empty list when none match) + + ## Raises + + None + """ + matched: list[dict[str, Any]] = [] + data = _get_typed_value(response, "DATA", dict, {}) + for key in _MULTISTATUS_ITEM_KEYS: + items = _get_typed_value(data, key, list, []) + matched.extend(item for item in items if isinstance(item, dict) and str(item.get("status") or "").strip().lower() in statuses) + return matched + + +def _failed_multistatus_items(response: dict) -> list[dict[str, Any]]: + """ + # Summary + + Return the per-item entries reporting a failure in a Multi-Status body. + + ## Description + + Thin wrapper around `_multistatus_items_with_status` matching the failure literals in `_MULTISTATUS_FAILURE_STATUSES` + (`failed`/`failure`/`error`). + + ## Parameters + + - response: Response dict with keys RETURN_CODE, MESSAGE, DATA, etc. + + ## Returns + + - List of failing item dicts (empty list when none fail) + + ## Raises + + None + """ + return _multistatus_items_with_status(response, _MULTISTATUS_FAILURE_STATUSES) class NdV1Strategy: @@ -50,11 +203,13 @@ class NdV1Strategy: 1. raw_response: Non-JSON response stored in DATA.raw_response 2. code/message: DATA.code and DATA.message - 3. messages array: all DATA.messages[].{code, severity, message} joined with "; " - 4. errors array: all DATA.errors[] joined with "; " - 5. Connection failure: No DATA with REQUEST_PATH and MESSAGE - 6. Non-dict DATA: Stringified DATA value - 7. Unknown: Fallback with RETURN_CODE + 3. error: Scalar DATA.error value + 4. messages array: all DATA.messages[].{code, severity, message} joined with "; " + 5. errors array: all DATA.errors[] joined with "; " + 6. Multi-Status per-item failures: DATA.results[]/DATA.switchIds[]/DATA.links[] failing items joined with "; " + 7. Connection failure: No DATA with REQUEST_PATH and MESSAGE + 8. Non-dict DATA: Stringified DATA value + 9. Unknown: Fallback with RETURN_CODE ## Raises @@ -112,6 +267,10 @@ def is_success(self, response: dict) -> bool: - Top-level `ERROR` key is present - `DATA.error` key is present + - A Multi-Status body reports a per-item failure in `DATA.results[]`, + `DATA.switchIds[]`, or `DATA.links[]` (status `failed`/`failure`/`error`). This is + checked on any success code, not only 207: ND sends per-item statuses on HTTP 200 + for some endpoints (e.g. the L3Out batch POST). ## Parameters @@ -133,8 +292,45 @@ def is_success(self, response: dict) -> bool: data = response.get("DATA") if isinstance(data, dict) and data.get("error") is not None: return False + # ND reports per-item outcomes for batch operations in DATA.results[]/DATA.switchIds[]/ + # DATA.links[] items carrying status: success|failed|failure|error. The HTTP status alone does not + # indicate every item succeeded -- ND sends these bodies on 207 and, for some endpoints, + # on plain 200 -- so any success-code response with a failing item must not be + # classified as success. See issue #295. + if _failed_multistatus_items(response): + return False return True + @staticmethod + def _format_multistatus_failure(item: dict[str, Any]) -> str: + """ + # Summary + + Format a single failing Multi-Status item as `label: message`. + + ## Description + + Labels the item by the first non-empty identifier key (`_MULTISTATUS_ITEM_LABEL_KEYS`) and appends the first non-empty detail key + (`_MULTISTATUS_ITEM_MESSAGE_KEYS`, which falls back to `status` when no message key is present). Empty and whitespace-only values are skipped + rather than used, so an item carrying `{"name": ""}` is not labelled with the empty string. When no identifier key is present, only the + detail is returned; when no detail key is present either, the generic literal `failed` is used. + + ## Parameters + + - item: A per-item dict from `DATA.results[]`, `DATA.switchIds[]`, or `DATA.links[]` + + ## Returns + + - A human-readable `label: message` string (or just the message when unlabeled) + + ## Raises + + None + """ + label = _first_non_empty(item, _MULTISTATUS_ITEM_LABEL_KEYS) + detail = _first_non_empty(item, _MULTISTATUS_ITEM_MESSAGE_KEYS) or "failed" + return f"{label}: {detail}" if label is not None else detail + def is_not_found(self, return_code: int) -> bool: """ # Summary @@ -190,6 +386,40 @@ def is_changed(self, response: dict) -> bool: return True return str(modified).lower() != "false" + def is_changed_on_failure(self, response: dict) -> bool: + """ + # Summary + + Report whether a failed mutation nevertheless changed controller state. + + ## Description + + Called for POST/PUT/DELETE responses after `is_success` has returned `False`. A Multi-Status body can mix successful and failed per-item + outcomes, so an aggregate failure does not imply nothing changed. The `modified` response header, when present and parseable, is + authoritative (mirroring `is_changed`); otherwise any per-item `status` of `success` in the recognized envelope arrays (`DATA.results[]`, + `DATA.switchIds[]`, `DATA.links[]`) reports `True`. Non-itemized failures (e.g. `DATA.error` on a 200) report `False`, preserving the + conservative historical default. + + ## Parameters + + - response: Response dict with keys RETURN_CODE, MESSAGE, DATA, and any HTTP response headers (lowercased) forwarded by the HttpAPI plugin. + + ## Returns + + - True if the `modified` header is `"true"`, or (header absent or unparseable) at least one per-item status is `success` + - False if the `modified` header is `"false"`, or no per-item status is `success` + + ## Raises + + None + """ + modified = str(response.get("modified") or "").strip().lower() + if modified == "true": + return True + if modified == "false": + return False + return len(_multistatus_items_with_status(response, _MULTISTATUS_SUCCESS_STATUSES)) > 0 + def extract_error_message(self, response: dict) -> Optional[str]: """ # Summary @@ -203,10 +433,12 @@ def extract_error_message(self, response: dict) -> Optional[str]: 1. Connection failure (no DATA) 2. Non-JSON response (raw_response in DATA) 3. code/message dict - 4. messages array with code/severity/message (all items joined) - 5. errors array (all items joined) - 6. Unknown dict format - 7. Non-dict DATA + 4. Scalar error key (DATA.error) + 5. messages array with code/severity/message (all items joined) + 6. errors array (all items joined) + 7. Multi-Status per-item failures (results[]/switchIds[]/links[], all joined) + 8. Unknown dict format + 9. Non-dict DATA ## Parameters @@ -232,34 +464,78 @@ def extract_error_message(self, response: dict) -> Optional[str]: msg = f"Connection failed for {request_path}. {message}" # Dict response data - check various ND error formats elif isinstance(response_data, dict): - # Type-narrow response_data to dict[str, Any] for pylint - # pylint: disable=unsupported-membership-test,unsubscriptable-object - data_dict: dict[str, Any] = response_data - # Raw response (non-JSON) - if "raw_response" in data_dict: - msg = "ND Error: Response could not be parsed as JSON" - # code/message format - elif "code" in data_dict and "message" in data_dict: - msg = f"ND Error {data_dict['code']}: {data_dict['message']}" - - # messages array format - if msg is None and "messages" in data_dict and len(data_dict.get("messages", [])) > 0: - parts = [] - for m in data_dict["messages"]: - if all(k in m for k in ("code", "severity", "message")): - parts.append(f"ND Error {m['code']} ({m['severity']}): {m['message']}") - if parts: - msg = "; ".join(parts) - - # errors array format - if msg is None and "errors" in data_dict and len(data_dict.get("errors", [])) > 0: - msg = f"ND Error: {'; '.join(str(e) for e in data_dict['errors'])}" - - # Unknown dict format - fallback - if msg is None: - msg = f"ND Error: Request failed with status {return_code}" + msg = self._extract_dict_error_message(response_data, response, return_code) # Non-dict response data else: msg = f"ND Error: {response_data}" return msg + + def _extract_dict_error_message(self, data_dict: dict[str, Any], response: dict, return_code: int) -> str: + """ + # Summary + + Extract an error message from a dict `DATA` body across the known ND v1 formats. + + ## Description + + Checks, in priority order: `raw_response`, `code`/`message`, the scalar `error` key, the `messages[]` array, the `errors[]` array, and + Multi-Status per-item failures (`results[]`/`switchIds[]`/`links[]`). Falls back to a generic status message when no specific format matches. + + The `messages` and `errors` arrays are read through `_get_typed_value` because ND may send either key with an explicit `null` value, which a + bare `data_dict[key]` would iterate (or `len()`) into a `TypeError` on the very path that reports an error to the user. + + ## Parameters + + - data_dict: The response `DATA` dict + - response: The full response dict (needed to scan Multi-Status envelopes) + - return_code: The response `RETURN_CODE`, used in the fallback message + + ## Returns + + - A human-readable error message string (never None) + + ## Raises + + None + """ + msg: Optional[str] = None + # Raw response (non-JSON) + if "raw_response" in data_dict: + msg = "ND Error: Response could not be parsed as JSON" + # code/message format + elif "code" in data_dict and "message" in data_dict: + msg = f"ND Error {data_dict['code']}: {data_dict['message']}" + + # Scalar error key. `is_success()` already classifies a response carrying DATA.error as a + # failure; without this branch the error ND actually sent is dropped and the user is shown + # only the generic "Request failed with status " fallback below. + if msg is None and data_dict.get("error") is not None: + msg = f"ND Error: {data_dict['error']}" + + # messages array format + if msg is None: + parts = [] + for m in _get_typed_value(data_dict, "messages", list, []): + if isinstance(m, dict) and all(k in m for k in ("code", "severity", "message")): + parts.append(f"ND Error {m['code']} ({m['severity']}): {m['message']}") + if parts: + msg = "; ".join(parts) + + # errors array format + if msg is None: + errors = _get_typed_value(data_dict, "errors", list, []) + if errors: + msg = f"ND Error: {'; '.join(str(e) for e in errors)}" + + # Multi-Status per-item failures (results[]/switchIds[]/links[]) + if msg is None: + failed_items = _failed_multistatus_items(response) + if failed_items: + parts = [self._format_multistatus_failure(item) for item in failed_items] + msg = f"ND Error: {'; '.join(parts)}" + + # Unknown dict format - fallback + if msg is None: + msg = f"ND Error: Request failed with status {return_code}" + return msg diff --git a/plugins/module_utils/rest/rest_send.py b/plugins/module_utils/rest/rest_send.py index ff771b6f9..a3f1aee6d 100644 --- a/plugins/module_utils/rest/rest_send.py +++ b/plugins/module_utils/rest/rest_send.py @@ -319,7 +319,7 @@ def _commit_normal_mode(self) -> None: """ # Summary - Call sender.commit() with retries until successful response or timeout is exceeded. + Call sender.commit() with retries until a successful response, a terminal (non-retryable) failure, or timeout. ## Raises @@ -376,6 +376,12 @@ def _commit_normal_mode(self) -> None: success = self.result_current["success"] if success is False: + if self.result_current.get("retryable", True) is False: + msg = f"{self.class_name}.{method_name}: " + msg += "Terminal failure (retryable=False). Not retrying. " + msg += f"verb {self.verb}, path {self.path}." + self.log.debug(msg) + break if self.unit_test is False: sleep(self.send_interval) timeout -= self.send_interval diff --git a/tests/unit/module_utils/fixtures/fixture_data/test_rest_send.json b/tests/unit/module_utils/fixtures/fixture_data/test_rest_send.json index 6697d25b7..f893ec180 100644 --- a/tests/unit/module_utils/fixtures/fixture_data/test_rest_send.json +++ b/tests/unit/module_utils/fixtures/fixture_data/test_rest_send.json @@ -229,5 +229,42 @@ "DATA": { "status": "success" } + }, + "test_rest_send_01100a": { + "TEST_NOTES": ["Mixed 207: one success + one failed item. Terminal - RestSend must submit exactly once."], + "RETURN_CODE": 207, + "METHOD": "POST", + "REQUEST_PATH": "/api/v1/test/multistatus", + "MESSAGE": "Multi-Status", + "DATA": { + "results": [ + {"name": "acl_new", "status": "success", "message": "created successfully"}, + {"name": "acl_seed", "status": "failed", "message": "ACL already exists"} + ] + } + }, + "test_rest_send_01100b": { + "TEST_NOTES": ["Sentinel success. Must NOT be consumed - if the retry loop replays the terminal 207, this turns the result green and fails the test."], + "RETURN_CODE": 200, + "METHOD": "POST", + "REQUEST_PATH": "/api/v1/test/multistatus", + "MESSAGE": "OK", + "DATA": {"results": [{"name": "acl_seed", "status": "success"}]} + }, + "test_rest_send_01110a": { + "TEST_NOTES": ["Transient 500. Retryable - the loop must consume the follow-up 200 in 01110b."], + "RETURN_CODE": 500, + "METHOD": "POST", + "REQUEST_PATH": "/api/v1/test/transient", + "MESSAGE": "Internal Server Error", + "DATA": {"error": "backend unavailable"} + }, + "test_rest_send_01110b": { + "TEST_NOTES": ["Recovery response consumed by the retry of 01110a."], + "RETURN_CODE": 200, + "METHOD": "POST", + "REQUEST_PATH": "/api/v1/test/transient", + "MESSAGE": "OK", + "DATA": {"results": [{"name": "acl_new", "status": "success"}]} } } diff --git a/tests/unit/module_utils/orchestrators/test_l3out.py b/tests/unit/module_utils/orchestrators/test_l3out.py index 446044493..dc234dafb 100644 --- a/tests/unit/module_utils/orchestrators/test_l3out.py +++ b/tests/unit/module_utils/orchestrators/test_l3out.py @@ -240,17 +240,19 @@ def test_l3out_00120() -> None: """ # Summary - Verify `create` detects per-item failure in 207 multi-status response. + Verify `create` detects a per-item failure in a multi-status response body. ## Test - POST returns 200 but response body has item with status "failed" - - `RuntimeError` is raised with "partially failed" message + - `RuntimeError` is raised surfacing the per-item failure message (now via the centralized + `NdV1Strategy` per-item failure detection in RestSend, which scans the body on any + success code, not only 207) ## Classes and Methods - L3OutOrchestrator.create() - - L3OutOrchestrator._validate_bulk_response() + - NdV1Strategy.is_success() """ method_name = inspect.stack()[0][3] @@ -262,7 +264,7 @@ def responses(): instance = L3OutOrchestrator(rest_send=rest_send) model = _build_l3out_model() - match = r"Create failed for .*partially failed" + match = r"Create failed for .*L3Out already exists" with pytest.raises(RuntimeError, match=match): instance.create(model) @@ -562,17 +564,19 @@ def test_l3out_00520() -> None: """ # Summary - Verify `delete_bulk` detects per-item failure in 207 multi-status response. + Verify `delete_bulk` detects a per-item failure in a multi-status response body. ## Test - POST returns 200 but response body has one item with status "failed" - - `RuntimeError` is raised with "partially failed" message + - `RuntimeError` is raised surfacing the per-item failure message (now via the centralized + `NdV1Strategy` per-item failure detection in RestSend, which scans the body on any + success code, not only 207) ## Classes and Methods - L3OutOrchestrator.delete_bulk() - - L3OutOrchestrator._validate_bulk_response() + - NdV1Strategy.is_success() """ method_name = inspect.stack()[0][3] @@ -587,7 +591,7 @@ def responses(): _build_l3out_model(name="test-l3out-static", include_config=False), ] - match = r"Bulk delete failed.*partially failed" + match = r"Bulk delete failed.*L3Out not found" with pytest.raises(RuntimeError, match=match): instance.delete_bulk(models) @@ -1183,3 +1187,82 @@ def responses(): model = L3OutModel(name="test-l3out") with does_not_raise(): instance._resolve_links(model) + + +def test_l3out_00980() -> None: + """ + # Summary + + Verify a terminal per-item attach failure is submitted exactly once (no inner or outer replay). + + ## Test + + - attach POST returns 207 with a non-not-found per-item failure + - RestSend timeout (10) exceeds send_interval (1), so only the terminal break prevents inner replay + - attach_l3outs re-raises immediately (message lacks "not found"), so the outer loop does not retry + - Exactly one response is consumed: the generator holds only one, and a replay would exhaust it with a different error + + ## Classes and Methods + + - L3OutOrchestrator.attach_l3outs() + - RestSend._commit_normal_mode() + """ + + def responses(): + yield { + "RETURN_CODE": 207, + "MESSAGE": "Multi-Status", + "DATA": {"results": [{"name": "L3Out1", "status": "failed", "message": "L3Out already attached"}]}, + } + + gen_responses = ResponseGenerator(responses()) + rest_send = _build_rest_send(gen_responses) + rest_send.timeout = 10 + rest_send.send_interval = 1 + instance = L3OutOrchestrator(rest_send=rest_send) + + match = r"already attached" + with pytest.raises(Exception, match=match): + instance.attach_l3outs([{"name": "L3Out1", "attach": True}], max_retries=2, retry_delay=0) + + +def test_l3out_00990() -> None: + """ + # Summary + + Verify the eventual-consistency outer retry still works for not-found per-item failures. + + ## Test + + - First attach POST returns 207 whose per-item failure message contains "not found" + - attach_l3outs catches the raised failure, retries the outer loop, and the second POST succeeds + - Each outer attempt submits exactly once (two responses total) + + ## Classes and Methods + + - L3OutOrchestrator.attach_l3outs() + - RestSend._commit_normal_mode() + """ + + def responses(): + yield { + "RETURN_CODE": 207, + "MESSAGE": "Multi-Status", + "DATA": {"results": [{"name": "L3Out1", "status": "failed", "message": "L3Out L3Out1 not found"}]}, + } + yield { + "RETURN_CODE": 200, + "MESSAGE": "OK", + "DATA": {"results": [{"name": "L3Out1", "status": "success"}]}, + } + + gen_responses = ResponseGenerator(responses()) + rest_send = _build_rest_send(gen_responses) + rest_send.timeout = 10 + rest_send.send_interval = 1 + instance = L3OutOrchestrator(rest_send=rest_send) + + with does_not_raise(): + result = instance.attach_l3outs([{"name": "L3Out1", "attach": True}], max_retries=1, retry_delay=0) + + assert result.get("results", [{}])[0].get("status") == "success" diff --git a/tests/unit/module_utils/orchestrators/test_manage_acl.py b/tests/unit/module_utils/orchestrators/test_manage_acl.py index cfd0b6614..3ff7ac4f4 100644 --- a/tests/unit/module_utils/orchestrators/test_manage_acl.py +++ b/tests/unit/module_utils/orchestrators/test_manage_acl.py @@ -363,7 +363,7 @@ def test_manage_acl_00070() -> None: ## Classes and Methods - ManageAclOrchestrator.create_bulk - - ManageAclOrchestrator._raise_on_207_failures + - NdV1Strategy.is_success """ def responses(): @@ -374,7 +374,7 @@ def responses(): instance = ManageAclOrchestrator(rest_send=rest_send) model = AclModel.from_config(SAMPLE_CONFIG) - with pytest.raises(Exception, match="per-item failures.*ACL-BAD"): + with pytest.raises(Exception, match="Bulk create failed.*ACL-BAD"): instance.create_bulk([model]) @@ -483,7 +483,7 @@ def test_manage_acl_00110() -> None: ## Classes and Methods - ManageAclOrchestrator.delete_bulk - - ManageAclOrchestrator._raise_on_207_failures + - NdV1Strategy.is_success """ def responses(): @@ -495,7 +495,7 @@ def responses(): instance = ManageAclOrchestrator(rest_send=rest_send) model = AclModel.from_config(config) - with pytest.raises(Exception, match="per-item failures.*ACL-BAD"): + with pytest.raises(Exception, match="Bulk delete failed.*ACL-BAD"): instance.delete_bulk([model]) diff --git a/tests/unit/module_utils/orchestrators/test_manage_policy_group_orchestrator.py b/tests/unit/module_utils/orchestrators/test_manage_policy_group_orchestrator.py index 1607fb2bc..57cf8bfd1 100644 --- a/tests/unit/module_utils/orchestrators/test_manage_policy_group_orchestrator.py +++ b/tests/unit/module_utils/orchestrators/test_manage_policy_group_orchestrator.py @@ -1967,12 +1967,15 @@ def test_manage_policy_group_orchestrator_00820() -> None: """ # Summary - Any per-switch ``failed`` status escalates to an ``Exception`` enumerating - the failed switches and their messages. + Any per-switch ``failed`` status escalates to an ``Exception`` surfacing the + failed switch and its message. For the dict/``switchIds`` body shape this is now + caught by the centralized ``NdV1Strategy`` 207/multi-status handling in RestSend + (the bare-list shape is still handled by ``_switch_deploy`` -- see 00830). ## Classes and Methods - PolicyGroupOrchestrator._switch_deploy + - NdV1Strategy.is_success """ body = { "switchIds": [ @@ -1985,7 +1988,7 @@ def test_manage_policy_group_orchestrator_00820() -> None: with pytest.raises( Exception, - match="switchActions/deploy reported 1 failed switch.*SN2.*deploy timeout", + match="SN2.*deploy timeout", ): instance._switch_deploy(["SN1", "SN2"]) diff --git a/tests/unit/module_utils/orchestrators/test_manage_prefix_list.py b/tests/unit/module_utils/orchestrators/test_manage_prefix_list.py index c759b7271..34f94477c 100644 --- a/tests/unit/module_utils/orchestrators/test_manage_prefix_list.py +++ b/tests/unit/module_utils/orchestrators/test_manage_prefix_list.py @@ -23,7 +23,7 @@ import pytest from ansible_collections.cisco.nd.plugins.module_utils.enums import HttpVerbEnum, OperationType from ansible_collections.cisco.nd.plugins.module_utils.models.manage_prefix_list.manage_prefix_list import PrefixListModel -from ansible_collections.cisco.nd.plugins.module_utils.orchestrators.manage_prefix_list import ManagePrefixListOrchestrator, _SCOPED_QUERY_MAX_IDENTIFIERS +from ansible_collections.cisco.nd.plugins.module_utils.orchestrators.manage_prefix_list import _SCOPED_QUERY_MAX_IDENTIFIERS, ManagePrefixListOrchestrator 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 @@ -833,7 +833,7 @@ def test_manage_prefix_list_00160() -> None: ## Classes and Methods - ManagePrefixListOrchestrator.create_bulk - - ManagePrefixListOrchestrator._raise_on_207_failures + - NdV1Strategy.is_success """ def responses(): @@ -845,7 +845,7 @@ def responses(): instance = ManagePrefixListOrchestrator(rest_send=rest_send) model = PrefixListModel.from_config(config) - with pytest.raises(Exception, match="per-item failures.*PL-IPV4-BAD"): + with pytest.raises(Exception, match="Bulk create failed.*ND Error: PL-IPV4-BAD"): instance.create_bulk([model]) @@ -1019,7 +1019,7 @@ def test_manage_prefix_list_00200() -> None: ## Classes and Methods - ManagePrefixListOrchestrator.delete_bulk - - ManagePrefixListOrchestrator._raise_on_207_failures + - NdV1Strategy.is_success """ def responses(): @@ -1031,7 +1031,7 @@ def responses(): instance = ManagePrefixListOrchestrator(rest_send=rest_send) model = PrefixListModel.from_config(config) - with pytest.raises(Exception, match="per-item failures.*PL-IPV4-BAD"): + with pytest.raises(Exception, match="Bulk delete failed.*ND Error: PL-IPV4-BAD"): instance.delete_bulk([model]) diff --git a/tests/unit/module_utils/orchestrators/test_manage_route_map.py b/tests/unit/module_utils/orchestrators/test_manage_route_map.py index 1834d0c9b..0033da404 100644 --- a/tests/unit/module_utils/orchestrators/test_manage_route_map.py +++ b/tests/unit/module_utils/orchestrators/test_manage_route_map.py @@ -300,7 +300,7 @@ def test_manage_route_map_orchestrator_00215() -> None: ## Classes and Methods - ManageRouteMapOrchestrator.create_bulk() - - ManageRouteMapOrchestrator._raise_on_bulk_errors() + - NdV1Strategy.is_success() """ model = _route_map_model("RM_CREATE_ONE") @@ -333,7 +333,7 @@ def test_manage_route_map_orchestrator_00220() -> None: ## Classes and Methods - ManageRouteMapOrchestrator.create_bulk() - - ManageRouteMapOrchestrator._raise_on_bulk_errors() + - NdV1Strategy.is_success() """ model = _route_map_model("RM_EXISTS") @@ -345,7 +345,7 @@ def responses(): rest_send = _build_rest_send(ResponseGenerator(responses())) instance = ManageRouteMapOrchestrator(rest_send=rest_send) - with pytest.raises(Exception, match=r"Bulk create failed: Route map bulk create failed for RM_EXISTS: failed: Route map already exists\."): + with pytest.raises(Exception, match=r"Bulk create failed.*ND Error: RM_EXISTS: Route map already exists\."): instance.create_bulk([model]) @@ -488,7 +488,7 @@ def test_manage_route_map_orchestrator_00420() -> None: ## Classes and Methods - ManageRouteMapOrchestrator.delete_bulk() - - ManageRouteMapOrchestrator._raise_on_bulk_errors() + - NdV1Strategy.is_success() """ model = RouteMapModel.from_config({"name": "RM_MISSING"}) @@ -498,7 +498,7 @@ def responses(): rest_send = _build_rest_send(ResponseGenerator(responses())) instance = ManageRouteMapOrchestrator(rest_send=rest_send) - with pytest.raises(Exception, match=r"Bulk delete failed: Route map bulk delete failed for RM_MISSING: failed: Route map not found\."): + with pytest.raises(Exception, match=r"Bulk delete failed.*ND Error: RM_MISSING: Route map not found\."): instance.delete_bulk([model]) diff --git a/tests/unit/module_utils/orchestrators/test_subinterface_managed_interface.py b/tests/unit/module_utils/orchestrators/test_subinterface_managed_interface.py index a2cf7cd25..7d0d23090 100644 --- a/tests/unit/module_utils/orchestrators/test_subinterface_managed_interface.py +++ b/tests/unit/module_utils/orchestrators/test_subinterface_managed_interface.py @@ -141,74 +141,6 @@ def test_subinterface_managed_orchestrator_00020() -> None: assert SubinterfaceManagedInterfaceOrchestrator.supports_bulk_delete is True -# ============================================================================= -# Test: _raise_on_multi_status_failures — 207 Multi-Status handling -# ============================================================================= - - -@pytest.mark.parametrize( - "response", - [ - None, - "not a dict", - {}, - {"results": []}, - {"results": [{"name": "Ethernet1/3.2", "status": "success"}]}, - ], - ids=["none", "non_dict", "empty_dict", "empty_results", "all_success"], -) -def test_subinterface_managed_orchestrator_00100(response) -> None: - """ - # Summary - - Verify `_raise_on_multi_status_failures` does NOT raise for non-dict bodies, missing/empty results, or - all-success results. - - ## Test - - - None / non-dict / empty results / all-success bodies do not raise - - ## Classes and Methods - - - SubinterfaceManagedInterfaceOrchestrator._raise_on_multi_status_failures() - """ - with does_not_raise(): - SubinterfaceManagedInterfaceOrchestrator._raise_on_multi_status_failures(response) - - -@pytest.mark.parametrize( - "status", - ["failed", "error"], - ids=["failed", "error"], -) -def test_subinterface_managed_orchestrator_00110(status) -> None: - """ - # Summary - - Verify `_raise_on_multi_status_failures` raises `RuntimeError` when any result item carries - `status: "failed"` or `status: "error"`, surfacing the per-item name and message. - - ND returns HTTP 207 Multi-Status on subinterface POST with per-item failures (e.g. parent not in routed mode) - that the RestSend layer treats as success; this guard converts those into a hard failure. - - ## Test - - - A results body with one failed item raises RuntimeError mentioning the count and message - - ## Classes and Methods - - - SubinterfaceManagedInterfaceOrchestrator._raise_on_multi_status_failures() - """ - response = { - "results": [ - {"name": "Ethernet1/3.2", "status": "success"}, - {"name": "Ethernet1/3.3", "status": status, "message": "parent not in routed mode"}, - ] - } - with pytest.raises(RuntimeError, match=r"ND rejected 1 interface\(s\).*parent not in routed mode"): - SubinterfaceManagedInterfaceOrchestrator._raise_on_multi_status_failures(response) - - # ============================================================================= # Test: query_all — happy path with filtering # ============================================================================= @@ -424,7 +356,7 @@ def test_subinterface_managed_orchestrator_00610() -> None: ## Classes and Methods - SubinterfaceManagedInterfaceOrchestrator.create() - - SubinterfaceManagedInterfaceOrchestrator._raise_on_multi_status_failures() + - NdV1Strategy.is_success() """ def responses(): diff --git a/tests/unit/module_utils/orchestrators/test_subinterface_unmanaged_interface.py b/tests/unit/module_utils/orchestrators/test_subinterface_unmanaged_interface.py index f88214a49..8f8a9ebf4 100644 --- a/tests/unit/module_utils/orchestrators/test_subinterface_unmanaged_interface.py +++ b/tests/unit/module_utils/orchestrators/test_subinterface_unmanaged_interface.py @@ -66,61 +66,6 @@ def _build_rest_send( return rest_send -# ============================================================================= -# Test: _raise_on_multi_status_failures -# ============================================================================= - - -@pytest.mark.parametrize( - "payload, expected_raise", - [ - ({"results": [{"name": "Ethernet1/3.20", "status": "success", "message": "ok"}]}, False), - ({"results": [{"name": "Ethernet1/3.20", "status": "failed", "message": "parent not routed"}]}, True), - ({"results": [{"name": "Ethernet1/3.20", "status": "error", "message": "validation"}]}, True), - ({"results": [{"name": "Ethernet1/3.20", "status": "Failed", "message": "parent not routed"}]}, True), - ({"results": [{"name": "Ethernet1/3.20", "status": " ERROR ", "message": "validation"}]}, True), - ({"results": [{"name": "Ethernet1/3.20", "status": None, "message": "no status key"}]}, False), - ({"results": []}, False), - ({}, False), - (None, False), - ("not a dict", False), - ], - ids=[ - "success", - "failed-raises", - "error-raises", - "failed-mixed-case-raises", - "error-padded-uppercase-raises", - "none-status-no-raise", - "empty-results", - "missing-results", - "none-input", - "non-dict-input", - ], -) -def test_subinterface_unmanaged_interface_00010(payload, expected_raise) -> None: - """ - # Summary - - Verify `_raise_on_multi_status_failures` behavior across the matrix. - - ## Test - - - Various 207-body shapes are passed - - `RuntimeError` is raised iff any item carries `status` of `"failed"`/`"error"` (case-insensitive, whitespace-tolerant) - - ## Classes and Methods - - - SubinterfaceUnmanagedInterfaceOrchestrator._raise_on_multi_status_failures() - """ - if expected_raise: - with pytest.raises(RuntimeError, match=r"ND rejected"): - SubinterfaceUnmanagedInterfaceOrchestrator._raise_on_multi_status_failures(payload) - else: - with does_not_raise(): - SubinterfaceUnmanagedInterfaceOrchestrator._raise_on_multi_status_failures(payload) - - # ============================================================================= # Test: create # ============================================================================= @@ -185,11 +130,13 @@ def test_subinterface_unmanaged_interface_00101(monkeypatch) -> None: ## Test - POST returns 207 with `results[0].status = "failed"` - - `_raise_on_multi_status_failures` raises; `create` wraps it in a `RuntimeError` matching `Create failed.*ND rejected` + - `_request` raises via the centralized `NdV1Strategy` 207 handling; `create` wraps it in a `RuntimeError` that + surfaces the per-item failure message ## Classes and Methods - SubinterfaceUnmanagedInterfaceOrchestrator.create() + - NdV1Strategy.is_success() """ method_name = inspect.stack()[0][3] @@ -203,7 +150,7 @@ def responses(): orchestrator = SubinterfaceUnmanagedInterfaceOrchestrator(rest_send=rest_send) model = SubinterfaceUnmanagedInterfaceModel(switch_ip="192.168.12.151", interface_name="Ethernet1/3.20") - with pytest.raises(RuntimeError, match=r"Create failed.*ND rejected"): + with pytest.raises(RuntimeError, match=r"Create failed.*Parent interface Ethernet1/3 is not in routed mode"): orchestrator.create(model) assert orchestrator._pending_deploys == [] diff --git a/tests/unit/module_utils/test_response_handler_nd.py b/tests/unit/module_utils/test_response_handler_nd.py index f3250dbcf..d70622e54 100644 --- a/tests/unit/module_utils/test_response_handler_nd.py +++ b/tests/unit/module_utils/test_response_handler_nd.py @@ -1458,6 +1458,693 @@ def test_response_handler_nd_01000(): assert "Primary error message" in instance.error_message +# ============================================================================= +# Test: Multi-Status per-item failure detection (issue #295) +# +# ND reports per-item outcomes for batch operations in a DATA envelope array whose +# items carry status: "success" | "failed" | "failure" | "error". The two known +# envelope shapes are DATA.results[] (batch interface / breakout) and +# DATA.switchIds[] (switchActions/deploy). ND sends these bodies on HTTP 207 and, +# for some endpoints, on plain HTTP 200 -- so any success-code response whose body +# contains a failing item must NOT be classified as success. +# ============================================================================= + + +def test_response_handler_nd_01200(): + """ + # Summary + + Verify 207 with a failed `DATA.results[]` item is not success. + + ## Test + + - POST with RETURN_CODE 207 and a results[] item status "failed" + sets success=False, changed=False + + ## Classes and Methods + + - NdV1Strategy.is_success() + - ResponseHandler._handle_post_put_delete_response() + - ResponseHandler.commit() + """ + instance = ResponseHandler() + instance.response = { + "RETURN_CODE": 207, + "MESSAGE": "Multi-Status", + "DATA": { + "results": [ + { + "name": "Port-channel1.999", + "status": "failed", + "message": "Sub-interface can be created only on routed interfaces", + } + ] + }, + } + instance.verb = HttpVerbEnum.POST + with does_not_raise(): + instance.commit() + assert instance.result["success"] is False + assert instance.result["changed"] is False + + +def test_response_handler_nd_01210(): + """ + # Summary + + Verify 207 with an errored `DATA.results[]` item is not success. + + ## Test + + - POST with RETURN_CODE 207 and a results[] item status "error" + (breakout-style literal) sets success=False + + ## Classes and Methods + + - NdV1Strategy.is_success() + - ResponseHandler.commit() + """ + instance = ResponseHandler() + instance.response = { + "RETURN_CODE": 207, + "MESSAGE": "Multi-Status", + "DATA": { + "results": [ + { + "name": "Ethernet1/1", + "status": "error", + "message": "Breakout not supported on this port", + } + ] + }, + } + instance.verb = HttpVerbEnum.POST + with does_not_raise(): + instance.commit() + assert instance.result["success"] is False + + +def test_response_handler_nd_01220(): + """ + # Summary + + Verify 207 with a failed `DATA.switchIds[]` item is not success. + + ## Test + + - POST with RETURN_CODE 207 and a switchIds[] item status "failed" + (switchActions/deploy shape) sets success=False + + ## Classes and Methods + + - NdV1Strategy.is_success() + - ResponseHandler.commit() + """ + instance = ResponseHandler() + instance.response = { + "RETURN_CODE": 207, + "MESSAGE": "Multi-Status", + "DATA": { + "switchIds": [ + {"switchId": "FDO1234ABCD", "status": "success"}, + {"switchId": "FDO5678WXYZ", "status": "failed", "message": "Deploy failed on peer"}, + ] + }, + } + instance.verb = HttpVerbEnum.POST + with does_not_raise(): + instance.commit() + assert instance.result["success"] is False + + +def test_response_handler_nd_01230(): + """ + # Summary + + Verify 207 whose per-item statuses all succeed remains success. + + ## Test + + - POST with RETURN_CODE 207 and results[] items all status "success" + sets success=True, changed=True (no failing item present) + - retryable is False (key present on every mutation result) + + ## Classes and Methods + + - NdV1Strategy.is_success() + - ResponseHandler.commit() + """ + instance = ResponseHandler() + instance.response = { + "RETURN_CODE": 207, + "MESSAGE": "Multi-Status", + "DATA": { + "results": [ + {"name": "Ethernet1/1", "status": "success"}, + {"name": "Ethernet1/2", "status": "success"}, + ] + }, + } + instance.verb = HttpVerbEnum.POST + with does_not_raise(): + instance.commit() + assert instance.result["success"] is True + assert instance.result["changed"] is True + assert instance.result["retryable"] is False + + +def test_response_handler_nd_01240(): + """ + # Summary + + Verify error_message aggregates failed `DATA.results[]` items. + + ## Test + + - A 207 with a failed results[] item exposes an error_message + naming the item and its per-item message + + ## Classes and Methods + + - NdV1Strategy.extract_error_message() + - ResponseHandler.error_message + """ + instance = ResponseHandler() + instance.response = { + "RETURN_CODE": 207, + "MESSAGE": "Multi-Status", + "DATA": { + "results": [ + { + "name": "Port-channel1.999", + "status": "failed", + "message": "Sub-interface can be created only on routed interfaces", + } + ] + }, + } + instance.verb = HttpVerbEnum.POST + instance.commit() + assert instance.error_message is not None + assert "Port-channel1.999" in instance.error_message + assert "Sub-interface can be created only on routed interfaces" in instance.error_message + + +def test_response_handler_nd_01250(): + """ + # Summary + + Verify error_message aggregates failed `DATA.switchIds[]` items. + + ## Test + + - A 207 with a failed switchIds[] item exposes an error_message + naming the switch and its per-item message + + ## Classes and Methods + + - NdV1Strategy.extract_error_message() + - ResponseHandler.error_message + """ + instance = ResponseHandler() + instance.response = { + "RETURN_CODE": 207, + "MESSAGE": "Multi-Status", + "DATA": { + "switchIds": [ + {"switchId": "FDO5678WXYZ", "status": "failed", "message": "Deploy failed on peer"}, + ] + }, + } + instance.verb = HttpVerbEnum.POST + instance.commit() + assert instance.error_message is not None + assert "FDO5678WXYZ" in instance.error_message + assert "Deploy failed on peer" in instance.error_message + + +def test_response_handler_nd_01260(): + """ + # Summary + + Verify a mixed 207 (one ok, one failed) fails and names only the failure. + + ## Test + + - A 207 with one success item and one failed item sets success=False + - error_message names only the failed item, not the successful one + + ## Classes and Methods + + - NdV1Strategy.is_success() + - NdV1Strategy.extract_error_message() + - ResponseHandler.commit() + """ + instance = ResponseHandler() + instance.response = { + "RETURN_CODE": 207, + "MESSAGE": "Multi-Status", + "DATA": { + "switchIds": [ + {"switchId": "FDO1111AAAA", "status": "success"}, + {"switchId": "FDO2222BBBB", "status": "failed", "message": "peer deploy failed"}, + ] + }, + } + instance.verb = HttpVerbEnum.POST + instance.commit() + assert instance.result["success"] is False + assert instance.error_message is not None + assert "FDO2222BBBB" in instance.error_message + assert "FDO1111AAAA" not in instance.error_message + + +def test_response_handler_nd_01270(): + """ + # Summary + + Verify the `failure` per-item literal (not just `failed`) is detected. + + ## Test + + - A 207 with a results[] item status "failure" sets success=False + + ## Classes and Methods + + - NdV1Strategy.is_success() + - ResponseHandler.commit() + """ + instance = ResponseHandler() + instance.response = { + "RETURN_CODE": 207, + "MESSAGE": "Multi-Status", + "DATA": {"results": [{"name": "acl-1", "status": "failure", "message": "rejected"}]}, + } + instance.verb = HttpVerbEnum.POST + with does_not_raise(): + instance.commit() + assert instance.result["success"] is False + + +def test_response_handler_nd_01280(): + """ + # Summary + + Verify a per-item status with surrounding whitespace and mixed case still counts as a failure. + + ## Test + + - A 207 with a results[] item status " Failed " (padded, mixed case) sets success=False + + ## Classes and Methods + + - NdV1Strategy.is_success() + - ResponseHandler.commit() + """ + instance = ResponseHandler() + instance.response = { + "RETURN_CODE": 207, + "MESSAGE": "Multi-Status", + "DATA": {"results": [{"name": "eth1/1", "status": " Failed ", "message": "rejected"}]}, + } + instance.verb = HttpVerbEnum.POST + with does_not_raise(): + instance.commit() + assert instance.result["success"] is False + + +def test_response_handler_nd_01290(): + """ + # Summary + + Verify a 207 whose per-item `status` is None (or a non-failure value) remains success. + + ## Test + + - A 207 with a results[] item whose status is None does not flip success + + ## Classes and Methods + + - NdV1Strategy.is_success() + - ResponseHandler.commit() + """ + instance = ResponseHandler() + instance.response = { + "RETURN_CODE": 207, + "MESSAGE": "Multi-Status", + "DATA": {"results": [{"name": "eth1/1", "status": None, "message": "no status key"}]}, + } + instance.verb = HttpVerbEnum.POST + with does_not_raise(): + instance.commit() + assert instance.result["success"] is True + + +def test_response_handler_nd_01300(): + """ + # Summary + + Verify an empty-string label key is skipped rather than used as the label. + + ## Test + + - A 207 whose failing item carries name="" falls through to the next label key (switchId) + - The message is labelled "FDO5678WXYZ: ...", not ": ..." + + ## Classes and Methods + + - NdV1Strategy._format_multistatus_failure() + - NdV1Strategy.extract_error_message() + - ResponseHandler.commit() + """ + instance = ResponseHandler() + instance.response = { + "RETURN_CODE": 207, + "MESSAGE": "Multi-Status", + "DATA": {"results": [{"name": "", "switchId": "FDO5678WXYZ", "status": "failed", "message": "Deploy failed"}]}, + } + instance.verb = HttpVerbEnum.POST + with does_not_raise(): + instance.commit() + assert instance.result["success"] is False + assert instance.error_message is not None + assert "FDO5678WXYZ: Deploy failed" in instance.error_message + assert not instance.error_message.startswith("ND Error: : ") + + +def test_response_handler_nd_01310(): + """ + # Summary + + Verify `warningMessage` is used as the per-item detail when no `message` key is present. + + ## Test + + - A 207 failing item carrying warningMessage (fabric update-group shape) surfaces that text + + ## Classes and Methods + + - NdV1Strategy._format_multistatus_failure() + - NdV1Strategy.extract_error_message() + - ResponseHandler.commit() + """ + instance = ResponseHandler() + instance.response = { + "RETURN_CODE": 207, + "MESSAGE": "Multi-Status", + "DATA": {"results": [{"name": "leaf_group", "status": "failed", "warningMessage": "Switch not found"}]}, + } + instance.verb = HttpVerbEnum.POST + with does_not_raise(): + instance.commit() + assert instance.result["success"] is False + assert instance.error_message is not None + assert "leaf_group: Switch not found" in instance.error_message + + +def test_response_handler_nd_01320(): + """ + # Summary + + Verify a failing item with neither a label key nor a detail key yields the generic literal. + + ## Test + + - A 207 failing item carrying only status="failed" surfaces "ND Error: failed" + + ## Classes and Methods + + - NdV1Strategy._format_multistatus_failure() + - ResponseHandler.commit() + """ + instance = ResponseHandler() + instance.response = { + "RETURN_CODE": 207, + "MESSAGE": "Multi-Status", + "DATA": {"results": [{"status": "failed"}]}, + } + instance.verb = HttpVerbEnum.POST + with does_not_raise(): + instance.commit() + assert instance.result["success"] is False + assert instance.error_message == "ND Error: failed" + + +# ============================================================================= +# Test: Null-valued DATA keys do not crash error-message extraction +# +# ND may send `messages` or `errors` with an explicit null value. The key is +# present, so a bare `"messages" in data_dict and len(data_dict["messages"])` +# raises TypeError on the very path that reports an error to the user. +# ============================================================================= + + +def test_response_handler_nd_01330(): + """ + # Summary + + Verify a null `DATA.messages` does not raise and falls back to the generic message. + + ## Test + + - A 500 whose DATA carries messages=None commits without raising + - error_message is the generic status fallback + + ## Classes and Methods + + - NdV1Strategy._extract_dict_error_message() + - ResponseHandler.commit() + """ + instance = ResponseHandler() + instance.response = { + "RETURN_CODE": 500, + "MESSAGE": "Internal Server Error", + "DATA": {"messages": None}, + } + instance.verb = HttpVerbEnum.POST + with does_not_raise(): + instance.commit() + assert instance.result["success"] is False + assert instance.error_message == "ND Error: Request failed with status 500" + + +def test_response_handler_nd_01340(): + """ + # Summary + + Verify a null `DATA.errors` does not raise and falls back to the generic message. + + ## Test + + - A 500 whose DATA carries errors=None commits without raising + - error_message is the generic status fallback + + ## Classes and Methods + + - NdV1Strategy._extract_dict_error_message() + - ResponseHandler.commit() + """ + instance = ResponseHandler() + instance.response = { + "RETURN_CODE": 500, + "MESSAGE": "Internal Server Error", + "DATA": {"errors": None}, + } + instance.verb = HttpVerbEnum.POST + with does_not_raise(): + instance.commit() + assert instance.result["success"] is False + assert instance.error_message == "ND Error: Request failed with status 500" + + +# ============================================================================= +# Test: DATA.error text is surfaced, not dropped +# +# is_success() classifies a response carrying DATA.error as a failure. Without a +# matching branch in the error-message extractor, the text ND actually sent was +# dropped in favour of the generic "Request failed with status " fallback. +# ============================================================================= + + +def test_response_handler_nd_01350(): + """ + # Summary + + Verify the scalar `DATA.error` value is surfaced in error_message. + + ## Test + + - A 200 whose DATA carries error="ND error occurred" sets success=False + - error_message carries the error text rather than the generic fallback + + ## Classes and Methods + + - NdV1Strategy.is_success() + - NdV1Strategy._extract_dict_error_message() + - ResponseHandler.commit() + """ + instance = ResponseHandler() + instance.response = { + "RETURN_CODE": 200, + "MESSAGE": "OK", + "DATA": {"error": "ND error occurred"}, + } + instance.verb = HttpVerbEnum.POST + with does_not_raise(): + instance.commit() + assert instance.result["success"] is False + assert instance.error_message == "ND Error: ND error occurred" + + +# ============================================================================= +# Test: links[] Multi-Status envelope (bulk link create / delete) +# +# POST /api/v1/manage/links and POST /api/v1/manage/linkActions/remove return +# HTTP 207 with {"links": [{"linkId", "message", "status"}]}, status +# success|failure. The GET /links list body rides the same `links` envelope, +# but its link objects carry no top-level `status` key, so queries must not +# false-positive. See PR #398 discussion. +# ============================================================================= + + +def test_response_handler_nd_01360(): + """ + # Summary + + Verify a 207 `links[]` envelope with a failing item is classified as failure and labelled by linkId. + + ## Test + + - A 207 links body with one success and one failure item sets success=False + - The failing item is labelled by its linkId in error_message + - The succeeding item does not appear in error_message + + ## Classes and Methods + + - NdV1Strategy.is_success() + - NdV1Strategy._format_multistatus_failure() + - ResponseHandler.commit() + """ + instance = ResponseHandler() + instance.response = { + "RETURN_CODE": 207, + "MESSAGE": "Multi-Status", + "DATA": { + "links": [ + {"linkId": "LINK-UUID-8540", "message": "LINK-UUID-8540 deleted successfully", "status": "success"}, + {"linkId": "LINK-UUID-8541", "message": "Deletion of link with id:LINK-UUID-8541 failed due to invalid linkId.", "status": "failure"}, + ] + }, + } + instance.verb = HttpVerbEnum.POST + with does_not_raise(): + instance.commit() + assert instance.result["success"] is False + assert instance.error_message is not None + assert "LINK-UUID-8541: Deletion of link with id:LINK-UUID-8541 failed due to invalid linkId." in instance.error_message + assert "LINK-UUID-8540" not in instance.error_message + + +def test_response_handler_nd_01370(): + """ + # Summary + + Verify a 207 `links[]` envelope whose items all succeed is classified as success. + + ## Test + + - A 207 links body with only success items sets success=True + + ## Classes and Methods + + - NdV1Strategy.is_success() + - ResponseHandler.commit() + """ + instance = ResponseHandler() + instance.response = { + "RETURN_CODE": 207, + "MESSAGE": "Multi-Status", + "DATA": { + "links": [ + {"linkId": "LINK-UUID-8540", "message": "LINK-UUID-8540 created successfully", "status": "success"}, + {"linkId": "LINK-UUID-8541", "message": "LINK-UUID-8541 created successfully", "status": "success"}, + ] + }, + } + instance.verb = HttpVerbEnum.POST + with does_not_raise(): + instance.commit() + assert instance.result["success"] is True + + +def test_response_handler_nd_01380(): + """ + # Summary + + Verify a failing links item carrying linkId="" yields an unlabelled message rather than ": ". + + ## Test + + - A 207 links failure item with an empty linkId (the bulk-create OpenAPI example: the link was + never created, so ND has no id to report) surfaces the message without a label prefix + + ## Classes and Methods + + - NdV1Strategy._format_multistatus_failure() + - ResponseHandler.commit() + """ + instance = ResponseHandler() + instance.response = { + "RETURN_CODE": 207, + "MESSAGE": "Multi-Status", + "DATA": { + "links": [ + {"linkId": "LINK-UUID-8540", "message": "LINK-UUID-8540 created successfully", "status": "success"}, + {"linkId": "", "message": "PTI POLICY-14240 already associated for the link LINK-UUID-15010.", "status": "failure"}, + ] + }, + } + instance.verb = HttpVerbEnum.POST + with does_not_raise(): + instance.commit() + assert instance.result["success"] is False + assert instance.error_message == "ND Error: PTI POLICY-14240 already associated for the link LINK-UUID-15010." + + +def test_response_handler_nd_01390(): + """ + # Summary + + Verify a GET-shaped `links[]` list body (link objects, no `status` key) is not a false positive. + + ## Test + + - A 200 GET /links body whose link objects carry linkId but no status sets success=True + + ## Classes and Methods + + - NdV1Strategy.is_success() + - ResponseHandler.commit() + """ + instance = ResponseHandler() + instance.response = { + "RETURN_CODE": 200, + "MESSAGE": "OK", + "DATA": { + "links": [ + {"linkId": "LINK-UUID-8540", "linkType": "lan_neighbor_link", "srcInterfaceName": "Ethernet1/2", "dstInterfaceName": "Ethernet1/9"}, + {"linkId": "LINK-UUID-48060", "linkType": "lan_planned_link", "srcInterfaceName": "Ethernet1/16", "dstInterfaceName": "Ethernet1/16"}, + ], + "meta": {"counts": {"remaining": 0, "total": 2}}, + }, + } + instance.verb = HttpVerbEnum.GET + with does_not_raise(): + instance.commit() + assert instance.result["success"] is True + assert instance.result["found"] is True + + # ============================================================================= # Test: ResponseHandler commit() can be called multiple times # ============================================================================= @@ -1494,3 +2181,317 @@ def test_response_handler_nd_01100(): instance.commit() assert instance.result["success"] is False assert instance.result["found"] is False + + +def test_response_handler_nd_01400(): + """ + # Summary + + Verify a mixed 207 POST (one success, one failed item) is classified as a terminal failure. + + ## Test + + - POST 207 with results[] holding one success and one failed item + - success is False and retryable is False (success-code response with embedded per-item failure cannot succeed on replay) + + ## Classes and Methods + + - ResponseHandler._handle_post_put_delete_response() + - ResponseHandler.commit() + """ + instance = ResponseHandler() + instance.response = { + "RETURN_CODE": 207, + "MESSAGE": "Multi-Status", + "DATA": { + "results": [ + {"name": "acl_new", "status": "success", "message": "created successfully"}, + {"name": "acl_seed", "status": "failed", "message": "ACL already exists"}, + ] + }, + } + instance.verb = HttpVerbEnum.POST + with does_not_raise(): + instance.commit() + assert instance.result["success"] is False + assert instance.result["retryable"] is False + + +def test_response_handler_nd_01410(): + """ + # Summary + + Verify a POST failing with a non-success HTTP code remains retryable. + + ## Test + + - POST returns 500 + - success is False and retryable is True (transient transport-level failures keep today's retry behavior) + - changed is False (a pure transport failure changed nothing) + + ## Classes and Methods + + - ResponseHandler._handle_post_put_delete_response() + - ResponseHandler.commit() + """ + instance = ResponseHandler() + instance.response = { + "RETURN_CODE": 500, + "MESSAGE": "Internal Server Error", + "DATA": {"error": "backend unavailable"}, + } + instance.verb = HttpVerbEnum.POST + with does_not_raise(): + instance.commit() + assert instance.result["success"] is False + assert instance.result["retryable"] is True + assert instance.result["changed"] is False + + +def test_response_handler_nd_01420(): + """ + # Summary + + Verify a fully successful POST carries retryable=False. + + ## Test + + - POST 200 with all items succeeding + - success is True and retryable is False (key present on every mutation result for a consistent shape) + + ## Classes and Methods + + - ResponseHandler._handle_post_put_delete_response() + - ResponseHandler.commit() + """ + instance = ResponseHandler() + instance.response = { + "RETURN_CODE": 200, + "MESSAGE": "OK", + "DATA": {"results": [{"name": "acl_new", "status": "success"}]}, + } + instance.verb = HttpVerbEnum.POST + with does_not_raise(): + instance.commit() + assert instance.result["success"] is True + assert instance.result["retryable"] is False + + +def test_response_handler_nd_01430(): + """ + # Summary + + Verify DATA.error on a success code is classified as a terminal failure. + + ## Test + + - POST 200 with DATA.error set (pre-existing embedded-error shape) + - success is False and retryable is False — the application definitively rejected the request + + ## Classes and Methods + + - ResponseHandler._handle_post_put_delete_response() + - ResponseHandler.commit() + """ + instance = ResponseHandler() + instance.response = { + "RETURN_CODE": 200, + "MESSAGE": "OK", + "DATA": {"error": "VRF does not exist"}, + } + instance.verb = HttpVerbEnum.POST + with does_not_raise(): + instance.commit() + assert instance.result["success"] is False + assert instance.result["retryable"] is False + + +def test_response_handler_nd_01440(): + """ + # Summary + + Verify GET results carry no retryable key (GET retry semantics are unchanged). + + ## Test + + - GET returns 500 + - result has no "retryable" key, so RestSend's .get("retryable", True) default preserves today's GET retry behavior + + ## Classes and Methods + + - ResponseHandler._handle_get_response() + - ResponseHandler.commit() + """ + instance = ResponseHandler() + instance.response = { + "RETURN_CODE": 500, + "MESSAGE": "Internal Server Error", + "DATA": {}, + } + instance.verb = HttpVerbEnum.GET + with does_not_raise(): + instance.commit() + assert instance.result["success"] is False + assert "retryable" not in instance.result + + +def test_response_handler_nd_01450(): + """ + # Summary + + Verify a mixed 207 POST reports changed=True (a member succeeded, so controller state changed). + + ## Test + + - POST 207 with one success and one failed item + - success is False, changed is True + + ## Classes and Methods + + - NdV1Strategy.is_changed_on_failure() + - ResponseHandler._handle_post_put_delete_response() + """ + instance = ResponseHandler() + instance.response = { + "RETURN_CODE": 207, + "MESSAGE": "Multi-Status", + "DATA": { + "results": [ + {"name": "acl_new", "status": "success", "message": "created successfully"}, + {"name": "acl_seed", "status": "failed", "message": "ACL already exists"}, + ] + }, + } + instance.verb = HttpVerbEnum.POST + with does_not_raise(): + instance.commit() + assert instance.result["success"] is False + assert instance.result["changed"] is True + + +def test_response_handler_nd_01460(): + """ + # Summary + + Verify an all-failed 207 POST reports changed=False. + + ## Test + + - POST 207 where every results[] item failed + - success is False, changed is False + + ## Classes and Methods + + - NdV1Strategy.is_changed_on_failure() + - ResponseHandler._handle_post_put_delete_response() + """ + instance = ResponseHandler() + instance.response = { + "RETURN_CODE": 207, + "MESSAGE": "Multi-Status", + "DATA": { + "results": [ + {"name": "acl_new", "status": "failed", "message": "invalid entry"}, + {"name": "acl_seed", "status": "failed", "message": "ACL already exists"}, + ] + }, + } + instance.verb = HttpVerbEnum.POST + with does_not_raise(): + instance.commit() + assert instance.result["success"] is False + assert instance.result["changed"] is False + + +def test_response_handler_nd_01470(): + """ + # Summary + + Verify the modified header overrides the per-item scan on failure (header says false). + + ## Test + + - Mixed 207 POST whose modified header is "false" + - changed is False even though one item succeeded (the header is authoritative) + + ## Classes and Methods + + - NdV1Strategy.is_changed_on_failure() + - ResponseHandler._handle_post_put_delete_response() + """ + instance = ResponseHandler() + instance.response = { + "RETURN_CODE": 207, + "MESSAGE": "Multi-Status", + "modified": "false", + "DATA": { + "results": [ + {"name": "acl_new", "status": "success"}, + {"name": "acl_seed", "status": "failed", "message": "ACL already exists"}, + ] + }, + } + instance.verb = HttpVerbEnum.POST + with does_not_raise(): + instance.commit() + assert instance.result["success"] is False + assert instance.result["changed"] is False + + +def test_response_handler_nd_01480(): + """ + # Summary + + Verify the modified header overrides the per-item scan on failure (header says true). + + ## Test + + - All-failed 207 POST whose modified header is "true" + - changed is True even though no item succeeded (the header is authoritative) + + ## Classes and Methods + + - NdV1Strategy.is_changed_on_failure() + - ResponseHandler._handle_post_put_delete_response() + """ + instance = ResponseHandler() + instance.response = { + "RETURN_CODE": 207, + "MESSAGE": "Multi-Status", + "modified": "true", + "DATA": {"results": [{"name": "acl_seed", "status": "failed", "message": "ACL already exists"}]}, + } + instance.verb = HttpVerbEnum.POST + with does_not_raise(): + instance.commit() + assert instance.result["success"] is False + assert instance.result["changed"] is True + + +def test_response_handler_nd_01490(): + """ + # Summary + + Verify a non-itemized embedded error (DATA.error on 200) keeps changed=False. + + ## Test + + - POST 200 with DATA.error, no modified header, no per-item envelope + - success is False, changed is False (conservative default preserved) + + ## Classes and Methods + + - NdV1Strategy.is_changed_on_failure() + - ResponseHandler._handle_post_put_delete_response() + """ + instance = ResponseHandler() + instance.response = { + "RETURN_CODE": 200, + "MESSAGE": "OK", + "DATA": {"error": "VRF does not exist"}, + } + instance.verb = HttpVerbEnum.POST + with does_not_raise(): + instance.commit() + assert instance.result["success"] is False + assert instance.result["changed"] is False diff --git a/tests/unit/module_utils/test_rest_send.py b/tests/unit/module_utils/test_rest_send.py index e4a64180b..b806d8479 100644 --- a/tests/unit/module_utils/test_rest_send.py +++ b/tests/unit/module_utils/test_rest_send.py @@ -1447,6 +1447,109 @@ def responses(): instance.commit() +def test_rest_send_01100(): + """ + # Summary + + Verify a terminal per-item failure on a success code is submitted exactly once. + + ## Test + + - POST returns 207 with a mixed success/failed results[] body (retryable=False from ResponseHandler) + - timeout (10) exceeds send_interval (1), so the loop COULD retry ~10 times + - The loop breaks after one submission: the sentinel success response in fixture 01100b is never consumed + - Final result is the 207 terminal failure, not the sentinel 200 + + ## Classes and Methods + + - RestSend.commit() + - RestSend._commit_normal_mode() + """ + method_name = inspect.stack()[0][3] + + def responses(): + yield responses_rest_send(f"{method_name}a") + yield responses_rest_send(f"{method_name}b") + + gen_responses = ResponseGenerator(responses()) + + params = {"check_mode": False} + sender = Sender() + sender.ansible_module = MockAnsibleModule() + sender.gen = gen_responses + + with does_not_raise(): + instance = RestSend(params) + instance.sender = sender + response_handler = ResponseHandler() + response_handler.response = {"RETURN_CODE": 200, "MESSAGE": "OK"} + response_handler.verb = HttpVerbEnum.GET + response_handler.commit() + instance.response_handler = response_handler + instance.unit_test = True + instance.timeout = 10 + instance.send_interval = 1 + instance.path = "/api/v1/test/multistatus" + instance.verb = HttpVerbEnum.POST + instance.payload = {"acls": ["acl_new", "acl_seed"]} + instance.commit() + + # One submission: the terminal 207 is the final response; the sentinel 200 was never consumed. + assert instance.response_current["RETURN_CODE"] == 207 + assert instance.result_current["success"] is False + assert instance.result_current["retryable"] is False + + +def test_rest_send_01110(): + """ + # Summary + + Verify a transient non-success failure still retries (guards against over-suppressing retries). + + ## Test + + - POST returns 500 (retryable=True), then 200 on the retry + - timeout (10) and send_interval (5) allow exactly two submissions + - The loop consumes both responses and the final result is the successful 200 + + ## Classes and Methods + + - RestSend.commit() + - RestSend._commit_normal_mode() + """ + method_name = inspect.stack()[0][3] + + def responses(): + yield responses_rest_send(f"{method_name}a") + yield responses_rest_send(f"{method_name}b") + + gen_responses = ResponseGenerator(responses()) + + params = {"check_mode": False} + sender = Sender() + sender.ansible_module = MockAnsibleModule() + sender.gen = gen_responses + + with does_not_raise(): + instance = RestSend(params) + instance.sender = sender + response_handler = ResponseHandler() + response_handler.response = {"RETURN_CODE": 200, "MESSAGE": "OK"} + response_handler.verb = HttpVerbEnum.GET + response_handler.commit() + instance.response_handler = response_handler + instance.unit_test = True + instance.timeout = 10 + instance.send_interval = 5 + instance.path = "/api/v1/test/transient" + instance.verb = HttpVerbEnum.POST + instance.payload = {"acls": ["acl_new"]} + instance.commit() + + assert instance.response_current["RETURN_CODE"] == 200 + assert instance.result_current["success"] is True + + # ============================================================================= # Test: RestSend.add_response() # =============================================================================