Implement policy-aware deep merge semantics for nested attributes in state=merged

[gmicol]

Summary

The Orchestrator framework does not consistently perform true merge semantics for nested attributes when state=merged. Direct nested NDBaseModel fields are partially supported, but lists, dictionaries, lists of NDNestedModel, and lists of dictionaries are treated as atomic values. When a user supplies only part of a nested collection, the framework can replace the entire existing nested collection instead of merging the requested subset.

This can be misleading and destructive for users who expect state=merged to preserve unspecified nested configuration.

Current Behavior

NDStateMachine uses exclude_unset=True for state=merged diffs, then calls NDConfigCollection.merge(), which delegates to NDBaseModel.merge().

NDBaseModel.merge():

• only applies fields explicitly set by the proposed model
• recursively merges only when both current and proposed values are NDBaseModel
• otherwise assigns the proposed value directly

The diff helper issubset() recursively compares dictionaries, but lists require full-list equality with the same length. This means a proposed list containing one nested item is not treated as a subset of an existing list containing multiple items.

Affected examples include:

• FabricUpdateGroupModel.install_image_data.install_package_names
• FabricUpdateGroupModel.update_report_checks
• LocalUserModel.security_domains
• interface trunk VLAN mapping lists
• fabric models with nested list/dict collections

Problem

For state=merged, users reasonably expect omitted nested properties to remain unchanged. Today that is only reliable for direct nested model fields. Collection-like nested fields are replaced as a single property.

Example problem shape:

state: merged
config:
  - name: existing_object
    nested_collection:
      - key: one_item_to_update
        value: new_value

If the controller already has other entries in nested_collection, the framework may send only the proposed list and drop the omitted entries.

Suggested Fix

Implement a policy-aware deep merge engine shared by model diff and merge logic.

Recommended design:

1. Add merge policy metadata to NDBaseModel, for example:

merge_policies: ClassVar[dict[str, MergePolicy]] = {
    "security_domains": MergePolicy.list_keyed("name"),
    "update_report_checks": MergePolicy.list_keyed("report_check_name"),
    "install_package_names": MergePolicy.list_union(),
}

2. Supported policies should include:

   • replace: current default, safest fallback
   • model_deep: recursive model merge
   • dict_deep: recursive dictionary merge
   • list_replace: atomic list replacement
   • list_union: merge primitive list values without duplicates
   • list_keyed: merge list entries by identifier/key
   • possibly list_model_identifier: use nested model get_identifier_value() where available

3. Align diff behavior with merge behavior:

   • get_diff(..., exclude_unset=True) should use the same policy tree as merge()
   • a proposed keyed-list item that already matches one existing item should be no_diff
   • a proposed keyed-list item that differs should be changed
   • omitted list entries should not force replacement under state=merged

4. Cache merge plans per model class:

   • inspect Pydantic fields once
   • avoid repeated full serialization where possible
   • validate the merged model once after applying changes

5. Make module generation require explicit merge policy decisions for nested collection fields.

Alternatives Considered

Option A: Generic recursive merge for all dicts/lists

This is easy to implement but risky. Lists do not have universal semantics: some are sets, some are ordered lists, some are keyed collections, and some must be replaced atomically. A generic list merge could silently produce invalid ND payloads.

Option B: JSON Merge Patch style semantics

This gives a known merge model for dictionaries, but JSON Merge Patch treats arrays as replacement values. That does not solve keyed nested collections without extra policy metadata.

Option C: Policy-aware merge engine

This is the recommended approach. It is more work, but it keeps default behavior safe and lets each model declare the intended semantics for nested collections.

Risks

• Changing merge behavior globally can alter idempotency and payloads across many modules.
• Keyed list merge requires reliable identifiers for nested entries.
• Some ND APIs treat list order as meaningful.
• Some APIs require full nested payloads even for partial updates.
• Deep merge can add runtime cost if implemented with repeated dumps/validation.
• Diff and merge must be updated together; changing only merge can still produce false changes.

Test Plan

• Add model-level tests for direct nested models, nested dicts, primitive lists, keyed lists of NDNestedModel, and lists of dicts.
• Add state-machine tests showing state=merged preserves unspecified nested list entries.
• Add negative tests for fields declared as replace.
• Add module-specific regression tests for at least:
  • local user security domains
  • fabric update group install image/report checks
  • trunk VLAN mapping entries
• Add performance benchmarks for large configs and nested lists before enabling broad defaults.

Acceptance Criteria

• state=merged preserves unspecified nested values for fields declared mergeable.
• Existing replace semantics remain available and are the default for ambiguous fields.
• Diff and merge use the same policy rules.
• Runtime impact is measured and acceptable for large module configs.
• Future modules have a documented way to declare nested merge behavior.

[mikewiebe]

Just added this note while I was re-reviewing PR 287. Just adding a bread crumb to make sure we come back and verify after we complete the work in this issue

PR #287 adds nd_manage_community_list and nd_manage_extended_community_list, and both modules also need regression coverage when this issue is resolved.

Their entries fields are keyed collections where sequence_number provides the natural entry identity. With the current atomic-list merge behavior, a partial state: merged declaration replaces the full list and can remove entries that the user did not mention.

Please include both standard and extended community lists in the #386 module-level test matrix. At minimum, verify that:

• updating one existing sequence preserves other unmentioned sequences;
• adding a new sequence preserves all existing sequences;
• repeating the same partial merge is idempotent;
• state: replaced still performs intentional whole-list replacement; and
• diff and outgoing payload behavior follow the same keyed-list merge policy.

A concrete regression case is an existing list with sequences 10 and 20 followed by a merged task containing only an update to sequence 10. The result must retain sequence 20.

[mikewiebe]

PR #294 is the shared implementation point for this issue.

The relevant behavior is centralized in the shared model and state-machine utilities: list comparison can treat a proposed list as a subset of the existing list, while NDBaseModel.merge() currently assigns an explicitly supplied list wholesale. Fixing only individual resource modules would leave the same comparison-versus-construction mismatch available to every other list-bearing model.

When #386 is implemented, please make the shared comparison and payload-construction paths use the same field policy:

• keyed union/update for collections that support partial state: merged intent;
• authoritative whole-list replacement only for fields explicitly defined as atomic; and
• production-model regression tests proving that unmentioned list members survive a partial merge while state: replaced retains replacement semantics.

PR #294 does not need to resolve this as part of its current scope if the work is intentionally deferred, but #386 should treat its shared utility code as the primary implementation location and include downstream module coverage.