refactor(cfn-lang-ext): extract shared Mapping-name primitives for Fn::ForEach#9018
refactor(cfn-lang-ext): extract shared Mapping-name primitives for Fn::ForEach#9018bnusunny wants to merge 1 commit into
Conversation
…::ForEach Follow-up to #9009 (issue #9005). Closes the second class of structural divergence the bot reviews on that PR repeatedly caught: the Mapping name, Fn::FindInMap lookup key, and compound Mapping table key for nested Fn::ForEach were each constructed inline in two places — once in the build- time pipeline (samcli/commands/build/build_context.py) and once in the package-time pipeline (samcli/lib/package/language_extensions_packaging.py). Bot review #2 caught the missing prefix list; bot review #3 caught the collision counter keyed on the dotted path. Both bugs were duplicate string-construction logic drifting between sites. This PR extracts the three computations into a single module: samcli/lib/cfn_language_extensions/foreach_mapping_helpers.py with three pure functions: - compute_mapping_name(leaf, nesting_path, *, has_collision, resource_template_key) - compute_lookup_key(loop_variable, referenced_outer_vars) - compute_compound_mapping_key(outer_values, inner_value) Both pipelines now call these helpers; neither constructs the strings inline. The cross-pipeline equivalence test in tests/unit/lib/cfn_language_extensions/test_foreach_mapping_helpers.py asserts that build's collision-counting input and package's collision- counting input produce byte-identical Mapping names for the same logical inputs — locking in the structural guarantee. Mechanical changes: - samcli/lib/cfn_language_extensions/foreach_mapping_helpers.py (new) — three pure helpers + module docstring + __all__. - tests/unit/lib/cfn_language_extensions/test_foreach_mapping_helpers.py (new) — 19 tests including parameterized construction cases and three cross-pipeline equivalence cases. - samcli/lib/package/language_extensions_packaging.py — _compute_mapping_name body becomes a 3-line delegation; _replace_dynamic_artifact_with_findmap's default mapping_name and inline lookup_key construction become helper calls; _generate_artifact_mappings's inline compound-key construction becomes a helper call. - samcli/commands/build/build_context.py — _update_foreach_artifact_paths's inline mapping_name and lookup_key constructions become helper calls; _collect_nested_mapping_entry's inline compound-key construction becomes a helper call. Collapses two if/else branches in the process. Removes the now-unused sanitize_resource_key_for_mapping import. What this PR does NOT do: - It does not unify orchestration. The build walker stays a recursive walker; the package detect-then-emit pipeline stays a detect-then-emit pipeline. They just compute Mapping identifiers through one shared source of truth. - It does not touch auto-dependency-layer Mapping emission (_collect_foreach_layer_mappings). That stays in build_context.py. - It does not change DynamicArtifactProperty or PACKAGEABLE_RESOURCE_ARTIFACT_PROPERTIES. No detection changes. Verification: - baseline 9191 passed, 25 skipped → after 9211 passed, 25 skipped (+20) - targeted: 2052 passed across cfn_language_extensions, package, build - ruff check, mypy, black --check all clean - end-to-end #9005 repro (sam package against language-extensions/tc-026-nested-application) still rewrites Properties.Location to an https://s3... URL - leaf-collision regression tests from #9009 commit 5e81820 still pass
roger-zhangg
left a comment
There was a problem hiding this comment.
LGTM. This cleanly extracts the three Mapping-identifier computations (mapping name, lookup key, compound table key) into a shared foreach_mapping_helpers module, eliminating the structural divergence risk between build-time and package-time pipelines.
Verified:
- No behavioral changes — the extracted helpers produce byte-identical outputs to the inline constructions they replace. The
compute_mapping_name/compute_lookup_key/compute_compound_mapping_keyfunctions are pure, well-documented, and cover all branches (no-collision vs collision, no-outer-vars vs compound join, etc.). - Cross-pipeline equivalence tests — the
TestCrossPipelineEquivalenceclass is a valuable structural safeguard; it asserts both pipelines produce the same Mapping name for equivalent inputs and will fail loudly if a future change drifts one side. - Package-side
_compute_mapping_name— correctly becomes a thin delegation wrapper (retains itscollision_groupsCounter plumbing, which is pipeline-specific orchestration, while the raw name construction delegates to the shared helper). - Build-side simplification — the two if/else branches for
lookup_keyand the inline compound-key construction in_collect_nested_mapping_entrycollapse cleanly into single helper calls.
One note: both this PR and #9017 are in CONFLICTING state with develop. While the PR description says merge order with #9017 doesn't matter, this PR still imports _leaf_prop_name from language_extensions_packaging (line ~835 of build_context.py). PR #9017 removes that function and replaces it with leaf_prop_name in the new property_paths module. So merging #9017 first is the cleaner path — after that, the rebase for this PR would just update that one import to from samcli.lib.cfn_language_extensions.property_paths import leaf_prop_name.
Summary
Follow-up to #9009 (issue #9005). Closes the second class of structural divergence the bot reviews on that PR repeatedly caught: the Mapping name, Fn::FindInMap lookup key, and compound Mapping table key for nested Fn::ForEach were each constructed inline in two places — once in the build-time pipeline (`samcli/commands/build/build_context.py`) and once in the package-time pipeline (`samcli/lib/package/language_extensions_packaging.py`). Bot review #2 caught a missing prefix list; bot review #3 caught a collision counter keyed on the dotted path. Both bugs were duplicate string-construction logic drifting between sites.
This PR extracts the three computations into a single module so neither pipeline can drift from the other again.
Changes
What this PR does NOT do
Test plan
Relationship to PR #9017
Independent. PR #9017 consolidates the copy helpers and relocates the prop-name helpers into a public module. PR B (this one) extracts the Mapping-name primitives. They touch different functions in the same files; the merge order doesn't matter.