Skip to content

Add RestSend framework, enums, and shared unit test infrastructure#185

Open
allenrobel wants to merge 37 commits intond42_integrationfrom
nd42_rest_send
Open

Add RestSend framework, enums, and shared unit test infrastructure#185
allenrobel wants to merge 37 commits intond42_integrationfrom
nd42_rest_send

Conversation

@allenrobel
Copy link
Collaborator

@allenrobel allenrobel commented Mar 2, 2026

Summary

Adds the RestSend HTTP request framework, enums.py, and the shared unit test infrastructure for the ND collection. This PR is one of two companion PRs targeting nd42_integration:

  • This PR nd42_rest_send — RestSend framework, enums.py, and all shared test infrastructure (merge first)
  • Add Log class and logging config #184 nd42_loggingLog class, logging config, and test_log.py

Note: Imports of Log (log.py) will show as broken until nd42_logging (#184) is merged into nd42_integration. The unit tests in this PR also have a dependency on log.py (via common_utils) and will not run until both branches are merged.


TODO

  1. 2026-02-03 - Add unit tests for sender_file.py as well.

Plugin files added

File Description
plugins/module_utils/enums.py HttpVerbEnum and OperationType enums — required by RestSend, Results, ResponseHandler, and Sender
plugins/module_utils/rest_send.py RestSend — orchestrates HTTP requests via a pluggable Sender, with retries, timeout, and unit-test mode
plugins/module_utils/results.py Results — tracks changed/failed state and diff output across a task's lifetime
plugins/module_utils/protocol_response_handler.py ResponseHandlerProtocol — structural protocol for response handler implementations
plugins/module_utils/response_handler_nd.py ResponseHandler — ND-specific response handler; normalises HTTP status codes and extracts error messages
plugins/module_utils/protocol_sender.py SenderProtocol — structural protocol for sender implementations
plugins/module_utils/sender_nd.py Sender — ND-specific sender using Ansible's httpapi connection plugin
plugins/module_utils/pydantic_compat.py Pydantic v1/v2 compatibility shim used by Results
plugins/module_utils/protocol_response_validation.py ResponseValidationStrategy protocol for version-specific ND API response validation
plugins/module_utils/response_validation_nd_v1.py ND API v1 implementation of ResponseValidationStrategy

Shared unit test infrastructure added

File Description
plugins/module_utils/enums.py (see above)
tests/unit/__init__.py Package marker
tests/unit/module_utils/__init__.py Package marker
tests/unit/module_utils/common_utils.py does_not_raise() and other shared test helpers
tests/unit/module_utils/fixtures/load_fixture.py JSON fixture loader
tests/unit/module_utils/mock_ansible_module.py Lightweight AnsibleModule mock
tests/unit/module_utils/response_generator.py Generator-based mock response sequencer
tests/unit/module_utils/sender_file.py File-based Sender for replaying canned HTTP responses in tests

Unit tests added

File Tests
tests/unit/module_utils/test_rest_send.py RestSend class
tests/unit/module_utils/test_response_handler_nd.py ResponseHandler class
tests/unit/module_utils/test_sender_nd.py Sender class
tests/unit/module_utils/fixtures/fixture_data/test_rest_send.json Canned HTTP responses for RestSend tests

Merge order

  1. Merge this PR → nd42_integration (provides enums.py and shared test infrastructure needed by nd42_logging)
  2. Merge nd42_logging (Add Log class and logging config #184) → nd42_integration

Test plan

  • After merging both PRs into nd42_integration: python -m pytest tests/unit/module_utils/ passes cleanly
  • tox -e linters passes (black, isort, pylint, mypy)
  • ansible-test sanity --docker passes

🤖 Generated with Claude Code

Plugin files:
- plugins/module_utils/rest_send.py: RestSend class for HTTP request handling
- plugins/module_utils/results.py: Results class for tracking task state
- plugins/module_utils/protocol_response_handler.py: ResponseHandlerProtocol
- plugins/module_utils/response_handler_nd.py: ND-specific ResponseHandler
- plugins/module_utils/protocol_sender.py: SenderProtocol
- plugins/module_utils/sender_nd.py: ND-specific Sender (httpapi connection)
- plugins/module_utils/pydantic_compat.py: Pydantic v1/v2 compatibility shim
- plugins/module_utils/protocol_response_validation.py: ResponseValidationStrategy protocol
- plugins/module_utils/response_validation_nd_v1.py: ND v1 API response validation

Unit tests:
- tests/unit/module_utils/test_rest_send.py
- tests/unit/module_utils/test_response_handler_nd.py
- tests/unit/module_utils/test_sender_nd.py
- tests/unit/module_utils/fixtures/fixture_data/test_rest_send.json

Note: imports of Log and enums (log.py, enums.py) are intentionally broken in
this branch. Both will resolve once nd42_logging is merged into nd42_integration.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Moved from nd42_logging to this branch so that enums.py (a required
dependency of the RestSend framework) is co-located with the code that
uses it.

Plugin files:
- plugins/module_utils/enums.py: HttpVerbEnum and OperationType enums

Shared unit test infrastructure:
- tests/unit/__init__.py
- tests/unit/module_utils/__init__.py
- tests/unit/module_utils/common_utils.py: does_not_raise() and shared helpers
- tests/unit/module_utils/fixtures/load_fixture.py: JSON fixture loader
- tests/unit/module_utils/mock_ansible_module.py: lightweight AnsibleModule mock
- tests/unit/module_utils/response_generator.py: generator-based mock response sequencer
- tests/unit/module_utils/sender_file.py: file-based Sender for replaying canned responses

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@allenrobel allenrobel mentioned this pull request Mar 2, 2026
3 tasks
@allenrobel allenrobel changed the title Add RestSend framework: RestSend, Results, ResponseHandler, Sender Add RestSend framework, enums, and shared unit test infrastructure Mar 2, 2026
plugins/module_utils/nd_v2.py demonstrates how to wire together the
RestSend framework (RestSend, Sender, ResponseHandler) and Smart
Endpoints in a module context. Included as a working example for
teams building new modules against nd42_integration.

Note: imports Log from nd42_logging and endpoint classes from
nd42_smart_endpoints; will have broken imports until all three
branches are merged into nd42_integration.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
allenrobel and others added 5 commits March 3, 2026 09:27
ResponseHandler now delegates status code checks and error message
extraction to an injected ResponseValidationStrategy (defaulting to
NdV1Strategy) instead of hardcoding the logic. This removes the
duplicate error extraction code and the three hardcoded class constants,
and exposes a `validation_strategy` property for future v2 support.

Unit tests updated to reflect the removed class constants and to cover
the new `validation_strategy` property.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Black reformats `from __future__ import (absolute_import, ...)` by
removing the parentheses. Add `# fmt: off` / `# fmt: on` inside the
existing `# isort: off` / `# isort: on` blocks to prevent this.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
sphinx-notfound-page ; python_version >= '3.5' # docs build requires python 3+
straight.plugin ; python_version >= '3.5' # needed for hacking/build-ansible.py which will host changelog generation and requires python 3+ No newline at end of file
sphinx
python_version >= "3.5" # docs build requires python 3+
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need those anymore if we only support 3.10+ anyway?

PYDANTIC_IMPORT_ERROR = None # pylint: disable=invalid-name


def require_pydantic(module) -> None:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As we discussed yesterday I would adapt this to raise an exception and not use module / fail_json in here specifically so we keep it all in the module itself.

Is there a clever way for us to know this is executed from main() and just raised the exception without forcing it to call this method in every module main()?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @lhercot , there's no clever solution here that reduces total boilerplate, rather just one that distributes it differently. The current approach is pragmatic for Ansible. The module parameter is a mild coupling but it gives us missing_required_lib's installation instructions, the traceback, and proper exit formatting for free.

There may be a way to use a @requires_pydantic decorator on main(), which would cleaner than an explicit call, but would require main() to accept an instantiated ansible_module as an argument, which isn't really standard Ansible practice (I'm not sure this would even work). Can we explore this in a separate PR (in the interest of time)?

  import functools

  def requires_pydantic(func):
      """Decorator for module main() functions that require pydantic."""
      @functools.wraps(func)
      def wrapper(module, *args, **kwargs):
          if not HAS_PYDANTIC:
              from ansible.module_utils.basic import missing_required_lib
              module.fail_json(msg=missing_required_lib("pydantic"), exception=PYDANTIC_IMPORT_ERROR)
          return func(module, *args, **kwargs)
      return wrapper

  Then in each module:

  from ansible_collections.cisco.nd.plugins.module_utils.common.pydantic_compat import requires_pydantic

  @requires_pydantic
  def main(module):
      # pydantic is guaranteed available here
      ...

"""
...

def is_success(self, return_code: int) -> bool:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need to expand the signature here to support analyzing the whole response for success.
We have had cases where the API returns a 200 or other "successful" code but the answer contains an error message or code.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a good call but I think we have what we need for that in def extract_error_message below?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mikewiebe I think @lhercot is right here. While extract_error_message does do what it says on the tin, it's not actually integrated into the determination of success. Callers would need to reference extract_error_message separately, which is fragile.

Addressed with commit: d22f1e7

This also removes ad hoc handling in ResponseHandler, making NdV1Strategy the single authority determining success/failure.

return self._response_data

@property
def result(self) -> list[dict[str, Any]]:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't this be plural if it returns a list?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC, my thinking at the time (couple years ago) was that there is a single list, hence result -> singular. But, I can change this if you feel strongly. Would be a breaking change for any existing modules in progress, but better to address now rather than later if plural makes more sense here. Let me know.

Comment on lines +394 to +399
method_name: str = "add_response_data"
if not isinstance(value, dict):
msg = f"{self.class_name}.{method_name}: "
msg += f"instance.add_response_data must be a dict. Got {value}"
raise TypeError(msg)
self._response_data.append(copy.deepcopy(value))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we also put this data in the _tasks data so we have a consistent view of all data irrespective if they come from Task or from Legacy use of _response_data?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to contradict the previous comment to deprecate add_response_data (which I just committed).

Two options then:

  1. Deprecate and remove — keep the deprecation notices we just added, and callers should migrate to using register_api_call() to store response data in _tasks.
  2. Integrate into _tasks — remove the deprecation notices and instead have add_response_data() also store data in the task pipeline, giving a unified view.

Option 1 seems cleaner architecturally — _response_data is a parallel data path that bypasses the Pydantic-validated _tasks lifecycle. Rather than bridging two systems, it's simpler to migrate callers to the standard path.

What was the use-case you envisioned for add_response_data assuming you don't want to deprecate it?

allenrobel and others added 10 commits March 9, 2026 07:51
The explicit error_codes set {405, 409} was never called by the response
handler, which already uses else-branches to catch anything outside
success/not-found codes. This matches the original httpapi/nd.py behavior
(anything not in success codes is an error) and avoids silently missing
real errors like 500s from misconfigured API gateways.

Fixes: #185 (comment)

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Previously, extract_error_message() only used the first element of the
messages and errors arrays. Now all items are joined with "; " so no
error detail is silently dropped when the API returns multiple entries.

Addresses review comment: #185 (comment)

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Some ND API endpoints return a successful HTTP status code (e.g. 200)
while embedding an error in the response body. The previous is_success
signature only accepted a return_code int, making it impossible to
detect these cases without ad-hoc checks scattered in ResponseHandler.

- Change protocol and NdV1Strategy.is_success signature to accept the
  full response dict instead of a bare return_code int
- NdV1Strategy.is_success now checks both the status code and embedded
  error indicators (top-level ERROR key and DATA.error)
- Remove ad-hoc embedded-error guards from ResponseHandler._handle_post_put_delete_response;
  the strategy is now the single authority on success (SRP)

Closes: #185 (comment)

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
ND API endpoints may include a modified header (forwarded as a lowercase
key by the HttpAPI plugin) with string values "true" or "false" to
explicitly signal whether a PUT/POST operation mutated any controller
state. Previously this header was ignored in the new infrastructure,
meaning changed was always True for any successful mutation.

- Add is_changed(response) to the ResponseValidationStrategy protocol
- Implement in NdV1Strategy: returns False only when the modified header
  is explicitly "false"; defaults to True when the header is absent,
  preserving existing behaviour for DELETE and other verbs that do not
  send the header
- ResponseHandler._handle_post_put_delete_response now calls is_changed
  to set changed independently of success

Closes: #185 (comment)

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ards

Remove python_version >= "3.5" environment markers since the collection
requires Python >= 3.11 already. Also fix sphinx-notfound-page package
name which was broken into "sphinx - notfound - page".

Ref: #185 (comment)

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Per reviewer feedback at #185 (comment),
the previous names (TaskResultData, CurrentTaskData, register_task_result) were
linked to the Ansible playbook task concept, causing confusion since these
structures represent a single REST API call, not an Ansible task.

- TaskResultData     -> ApiCallResult      (immutable, frozen Pydantic model)
- CurrentTaskData    -> PendingApiCall     (mutable staging model)
- register_task_result() -> register_api_call()

Lifecycle is now: PendingApiCall -> ApiCallResult -> FinalResultData

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Remove _changed and _failed set attributes from Results and derive them
on demand from the ApiCallResult entries in self._tasks via set
comprehensions. This eliminates duplicated state without changing
behaviour.

Ref: #185 (comment)

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Mark the legacy response_data property and add_response_data() method
as deprecated, noting they may be removed in a future release.

Ref: #185 (comment)

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Remove the separate `_response_data` list and derive `response_data`
from `self._tasks` via the existing `response` property, eliminating
duplicate data storage. Make `add_response_data()` a no-op since both
it and `response_data` are deprecated and unused outside results.py.

Ref: #185 (comment)

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
jeetugangwar11 pushed a commit to jeetugangwar11/ansible-nd that referenced this pull request Mar 11, 2026
The explicit error_codes set {405, 409} was never called by the response
handler, which already uses else-branches to catch anything outside
success/not-found codes. This matches the original httpapi/nd.py behavior
(anything not in success codes is an error) and avoids silently missing
real errors like 500s from misconfigured API gateways.

Fixes: CiscoDevNet#185 (comment)

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
jeetugangwar11 pushed a commit to jeetugangwar11/ansible-nd that referenced this pull request Mar 11, 2026
Previously, extract_error_message() only used the first element of the
messages and errors arrays. Now all items are joined with "; " so no
error detail is silently dropped when the API returns multiple entries.

Addresses review comment: CiscoDevNet#185 (comment)

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
jeetugangwar11 pushed a commit to jeetugangwar11/ansible-nd that referenced this pull request Mar 11, 2026
Some ND API endpoints return a successful HTTP status code (e.g. 200)
while embedding an error in the response body. The previous is_success
signature only accepted a return_code int, making it impossible to
detect these cases without ad-hoc checks scattered in ResponseHandler.

- Change protocol and NdV1Strategy.is_success signature to accept the
  full response dict instead of a bare return_code int
- NdV1Strategy.is_success now checks both the status code and embedded
  error indicators (top-level ERROR key and DATA.error)
- Remove ad-hoc embedded-error guards from ResponseHandler._handle_post_put_delete_response;
  the strategy is now the single authority on success (SRP)

Closes: CiscoDevNet#185 (comment)

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
jeetugangwar11 pushed a commit to jeetugangwar11/ansible-nd that referenced this pull request Mar 11, 2026
ND API endpoints may include a modified header (forwarded as a lowercase
key by the HttpAPI plugin) with string values "true" or "false" to
explicitly signal whether a PUT/POST operation mutated any controller
state. Previously this header was ignored in the new infrastructure,
meaning changed was always True for any successful mutation.

- Add is_changed(response) to the ResponseValidationStrategy protocol
- Implement in NdV1Strategy: returns False only when the modified header
  is explicitly "false"; defaults to True when the header is absent,
  preserving existing behaviour for DELETE and other verbs that do not
  send the header
- ResponseHandler._handle_post_put_delete_response now calls is_changed
  to set changed independently of success

Closes: CiscoDevNet#185 (comment)

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
jeetugangwar11 pushed a commit to jeetugangwar11/ansible-nd that referenced this pull request Mar 11, 2026
…ards

Remove python_version >= "3.5" environment markers since the collection
requires Python >= 3.11 already. Also fix sphinx-notfound-page package
name which was broken into "sphinx - notfound - page".

Ref: CiscoDevNet#185 (comment)

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
jeetugangwar11 pushed a commit to jeetugangwar11/ansible-nd that referenced this pull request Mar 11, 2026
Per reviewer feedback at CiscoDevNet#185 (comment),
the previous names (TaskResultData, CurrentTaskData, register_task_result) were
linked to the Ansible playbook task concept, causing confusion since these
structures represent a single REST API call, not an Ansible task.

- TaskResultData     -> ApiCallResult      (immutable, frozen Pydantic model)
- CurrentTaskData    -> PendingApiCall     (mutable staging model)
- register_task_result() -> register_api_call()

Lifecycle is now: PendingApiCall -> ApiCallResult -> FinalResultData

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
jeetugangwar11 pushed a commit to jeetugangwar11/ansible-nd that referenced this pull request Mar 11, 2026
Remove _changed and _failed set attributes from Results and derive them
on demand from the ApiCallResult entries in self._tasks via set
comprehensions. This eliminates duplicated state without changing
behaviour.

Ref: CiscoDevNet#185 (comment)

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
jeetugangwar11 pushed a commit to jeetugangwar11/ansible-nd that referenced this pull request Mar 11, 2026
Mark the legacy response_data property and add_response_data() method
as deprecated, noting they may be removed in a future release.

Ref: CiscoDevNet#185 (comment)

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
jeetugangwar11 pushed a commit to jeetugangwar11/ansible-nd that referenced this pull request Mar 11, 2026
Remove the separate `_response_data` list and derive `response_data`
from `self._tasks` via the existing `response` property, eliminating
duplicate data storage. Make `add_response_data()` a no-op since both
it and `response_data` are deprecated and unused outside results.py.

Ref: CiscoDevNet#185 (comment)

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
mikewiebe
mikewiebe previously approved these changes Mar 11, 2026
Copy link
Collaborator

@mikewiebe mikewiebe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Store request-side data alongside response data for a complete API
interaction record, and add a verbosity_level for future output filtering.

### plugins/module_utils/rest/results.py
- **Imports**: Added `field_validator` from pydantic_compat, `HttpVerbEnum` from enums
- **`ApiCallResult`**: Added `path: str`, `verb: str`, `payload: Optional[dict]`,
  `verbosity_level: int` (1-6). Added `field_validator("verb", mode="before")`
  to coerce `HttpVerbEnum` -> `str`
- **`FinalResultData`**: Added `path: list[str]`, `verb: list[str]`,
  `payload: list[Optional[dict]]`, `verbosity_level: list[int]`
- **`PendingApiCall`**: Added `path: str = ""`, `verb: HttpVerbEnum = HttpVerbEnum.GET`,
  `payload: Optional[dict] = None`, `verbosity_level: int = Field(default=3, ge=1, le=6)`
- **`Results.register_api_call()`**: Passes new fields to `ApiCallResult(...)`,
  deep-copies payload
- **`Results.build_final_result()`**: Aggregates new fields into `FinalResultData`
- **New properties**: `path`/`path_current`, `verb`/`verb_current`,
  `payload`/`payload_current`, `verbosity_level`/`verbosity_level_current`
  -- all with type validation on setters

### plugins/module_utils/rest/rest_send.py
- Added `_committed_payload` instance variable and `committed_payload`
  read-only property
- Saves `copy.deepcopy(self._payload)` to `_committed_payload` before
  clearing payload in both `_commit_normal_mode()` and `_commit_check_mode()`

### tests/unit/module_utils/test_results.py (new)
- Tests for `PendingApiCall` new field defaults and validation
- Tests for `ApiCallResult` new fields, verb coercion, frozen immutability
- Tests for all `Results` current-task property getters/setters with
  type/value error cases
- Tests for `register_api_call()` capturing new fields (including
  deep-copy verification)
- Tests for aggregate properties across multiple tasks
- Tests for `build_final_result()` including new fields in output

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
mikewiebe
mikewiebe previously approved these changes Mar 11, 2026
Copy link
Collaborator

@mikewiebe mikewiebe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

The Results class tracks data per API call, not per Ansible task.
Updated docstrings on the new aggregate properties (path, verb, payload,
verbosity_level) and FinalResultData attribute descriptions to use
accurate terminology and avoid confusion during code review.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants