Skip to content

feat(globals): add per-property merge strategy for list properties#3945

Open
vicheey wants to merge 8 commits into
developfrom
feat/globals-merge-strategy
Open

feat(globals): add per-property merge strategy for list properties#3945
vicheey wants to merge 8 commits into
developfrom
feat/globals-merge-strategy

Conversation

@vicheey

@vicheey vicheey commented Jun 23, 2026

Copy link
Copy Markdown
Contributor

Summary

Fixes #3939

Adds a per-property merge strategy to the Globals merge engine. Today, all list-type properties concatenate when both Globals and resource-level values exist. This is incorrect for properties like Architectures (a function runs on one architecture — local should replace, not append).

Changes

New module: samtranslator/plugins/globals/merge_strategy.py

  • MergeOp enum: CONCATENATE, REPLACE, MERGE_BY_KEY
  • MergeRule frozen dataclass with validation
  • merge_by_key() factory function

Modified: samtranslator/plugins/globals/globals.py

  • Path-aware recursion: _do_merge tracks the current property path through dict recursion
  • Schema lookup at LIST+LIST nodes only — DICT always deep-merges, PRIMITIVE always local-wins (unchanged)
  • CUSTOM_STRATEGIES dict declares per-property behavior using dot-notation paths
  • _merge_by_key() method for future use (Tags merge-by-key)

Day-1 scope: Function.Architectures: REPLACE

Design

The schema uses dot-notation paths (e.g., VpcConfig.SecurityGroupIds) to support nested properties. Properties not listed in CUSTOM_STRATEGIES default to CONCATENATE — identical to today's behavior. This makes the change fully backward-compatible.

How to extend

Add entries to CUSTOM_STRATEGIES in globals.py:

CUSTOM_STRATEGIES: dict[str, MergeRule] = {
    "Function.Architectures": REPLACE,
    # Future:
    # "Function.Tags": merge_by_key("Key"),
    # "Function.VpcConfig.SecurityGroupIds": REPLACE,
}

Tests

  • Unit tests for MergeRule types and validation (12 cases)
  • Integration tests in GlobalPropertiesTestCases covering REPLACE, MERGE_BY_KEY, nested paths, and multi-strategy
  • End-to-end translator fixture (globals_merge_strategy_architectures.yaml) proving REPLACE behavior across all 3 partitions
  • All 4533 existing tests pass unchanged (backward compatibility proof)
  • Coverage: 95.62%

Add a strategy pattern to the Globals merge engine that allows declaring
per-property merge behavior via a dot-notation schema. Day-1 scope:
Function.Architectures uses REPLACE (local list wins entirely).

The engine remains fully backward-compatible: properties not listed in
CUSTOM_STRATEGIES default to CONCATENATE (today's behavior). Path-aware
recursion tracks the current property path and consults the schema only
at LIST+LIST merge nodes.

New module: samtranslator/plugins/globals/merge_strategy.py
- MergeOp enum: CONCATENATE, REPLACE, MERGE_BY_KEY
- MergeRule frozen dataclass with validation
- merge_by_key() factory function

Resolves: #3939
@vicheey vicheey changed the title feat(globals): add per-property merge strategy with dot-notation schema feat(globals): add per-property merge strategy for list properties Jun 23, 2026

@aws-sam-tooling-bot aws-sam-tooling-bot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review Results

Reviewed: 90ed07e..46a50d4
Files: 8
Comments: 1

Comment thread samtranslator/plugins/globals/globals.py
…cate keys

Guard against duplicate key values in global_list (produces duplicate
replacements) and local_list (produces duplicate appends) by checking
seen_keys before appending in both passes.

Adds regression tests for both cases.
@vicheey

vicheey commented Jun 23, 2026

Copy link
Copy Markdown
Contributor Author

This PR takes inspiration from #3940 (by @allenheltondev) and expands the approach into a general-purpose framework with a strategy pattern that supports:

  • REPLACE — local list wins entirely (same outcome as fix Globals.Function Architectures override behavior #3940 for Architectures)
  • MERGE_BY_KEY — deduplicate list-of-dicts by a named key field (enables future Tags merge-by-key)
  • Nested paths — dot-notation schema keys (e.g. VpcConfig.SecurityGroupIds) work at any depth, not just top-level

The framework is extensible via CUSTOM_STRATEGIES — adding new per-property merge behavior is a one-line dict entry rather than per-resource conditional logic.

cc @allenheltondev — thank you for the original fix and for raising #3939.

@vicheey vicheey marked this pull request as ready for review June 23, 2026 07:24
@vicheey vicheey requested a review from a team as a code owner June 23, 2026 07:24
…irements.Architectures

Extends CUSTOM_STRATEGIES with a nested dot-path rule proving the
framework works at 2+ levels of dict recursion. Adds test cases to
both the dedicated merge strategy fixture and the existing
capacity_provider_global_with_functions fixture.

@aws-sam-tooling-bot aws-sam-tooling-bot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review Results

Reviewed: 90ed07e..69940b4
Files: 4 (source + tests; output JSON fixtures and YAML inputs are skipped)
Comments: 1

:param local_list: Local list of dicts
:param key: The dict key to match on
:return: Merged list
"""

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[BUG] _merge_by_key produces inconsistent results when local_list contains duplicate key values, depending on whether global_list happens to contain the same key.

local_by_key = {item[key]: item for item in local_list if isinstance(item, dict) and key in item}

The dict comprehension causes the last local entry to win for any given key. Then in Pass 2, the loop iterates local_list directly and appends the first non-seen entry. The two passes disagree:

  • Global has no override for the duplicate key → Pass 2 wins → first local entry kept.
  • Global has an override for the duplicate key → Pass 1 wins via local_by_key → last local entry kept.

Trace with key="Key":

# Case A: no global override
global_list = []
local_list  = [{"Key": "env", "Value": "a"}, {"Key": "env", "Value": "b"}]
# result -> [{"Key": "env", "Value": "a"}]  (first wins)

# Case B: global has the same key
global_list = [{"Key": "env", "Value": "g"}]
local_list  = [{"Key": "env", "Value": "a"}, {"Key": "env", "Value": "b"}]
# result -> [{"Key": "env", "Value": "b"}]  (last wins, via local_by_key)

The existing test list_with_merge_by_key_deduplicates_local_duplicates covers Case A and asserts first-wins; Case B is untested and silently flips the precedence. Recommend picking one rule and using it in both passes — e.g. build local_by_key by skipping already-present keys (if item[key] not in local_by_key) so first-wins is consistent across both branches, or by iterating local_by_key.values() in Pass 2 so last-wins is consistent.

This is dormant for now (no CUSTOM_STRATEGIES entry uses MERGE_BY_KEY), but the test suite locks in the inconsistent behavior, which will be harder to change once Tags merge-by-key is enabled.

vicheey added 5 commits June 23, 2026 18:22
…sses

local_by_key dict comprehension kept last entry per key, but Pass 2
kept first non-seen entry. When global had the same key as duplicate
locals, Pass 1 used last-wins (via local_by_key) while Pass 2 used
first-wins — inconsistent behavior depending on whether global
contained the key.

Fix: build local_by_key with first-entry-wins (skip already-present
keys) so both passes agree. Adds regression test for Case B.
…ties

Adds a new merge strategy that replaces at the key level (only local's
keys survive) but deep-merges values when both global and local share
the same key. No parameters needed — existing recursion handles all
value types (dicts deep-merge, scalars local-win, lists concatenate).

Registered for CapacityProvider.ManagedResourceTags: when local sets
Tags, global Propagate is dropped; when both have Tags, values merge.
Empty local {} inherits global (falsy guard, matches SAM precedent).
…urceTags

- New MergeOp: local key-set wins, shared keys deep-merge values
- Intercept DICT+DICT nodes in _do_merge when strategy registered
- Add CapacityProvider.ManagedResourceTags to CUSTOM_STRATEGIES
- Fix ruff PLC0206 (.items()) and mypy no-untyped-call
- Update translator fixtures: CpOverrideStrategyDropsGlobalKey added,
  CpExplicitTagsConflictWithGlobal removed (no longer an error)
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.

Globals.Function Architectures should be overridden by resource-level Architectures

1 participant