Skip to content

Commit 645f0ec

Browse files
committed
fix(golden): preserve Fn::ForEach in pinned outputs via BuildContext merge
The harness was producing pinned expected.{build,package}.yaml files that showed the LE-EXPANDED template (AlphaFunction/BetaFunction as siblings), but real `sam build` and `sam package` preserve the original Fn::ForEach structure with merged artifact paths inside the body — that's the design in PR #8637. run_build_pipeline now delegates the LE merge to ``BuildContext._get_template_for_output`` (invoked via a __new__ shell since the method only reads ``self.*`` for other methods on the same class). run_package_pipeline mirrors PackageContext's LE flow: ``_export_global_artifacts_pass`` → LE expand → rewrite artifacts → ``merge_language_extensions_s3_uris`` to copy URIs back into the original ForEach body. The SAM transform is skipped on the LE branch — the body keeps ``AWS::Serverless::Function`` with ``CodeUri`` (matching real sam build / sam package output, where the SAM transform runs server-side at deploy time). Update the corresponding unit test assertions to verify ``Fn::ForEach::*`` is preserved instead of expanded.
1 parent 59bc246 commit 645f0ec

3 files changed

Lines changed: 195 additions & 27 deletions

File tree

tests/golden/harness.py

Lines changed: 144 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,17 @@
3636
matches the user-visible behavior of ``sam build`` on such a template
3737
(the LE construct is preserved verbatim for CloudFormation server-side
3838
expansion).
39+
- When ``language_extensions=True`` and the template uses LE: the SAM
40+
transform is *also* skipped, and the build / package outputs preserve
41+
``Fn::ForEach::*`` with merged artifact values inside the body. This mirrors
42+
what real ``sam build`` and ``sam package`` write to disk (PR #8637): the
43+
SAM transform runs server-side at deploy time, so the build/package
44+
artifacts keep ``AWS::Serverless::Function`` (with ``CodeUri``) — they
45+
are not pre-transformed to ``AWS::Lambda::Function`` (with ``Code``).
46+
The merge itself delegates to
47+
``BuildContext._get_template_for_output`` (build) and
48+
``merge_language_extensions_s3_uris`` (package), invoked via lightweight
49+
shims so the harness does not duplicate the merge logic.
3950
4051
Known limitations (deferred to follow-up PRs / corpus growth):
4152
@@ -66,6 +77,7 @@
6677

6778
import copy
6879
import hashlib
80+
import types
6981
from pathlib import Path
7082
from typing import Any, Dict, List, Optional, Tuple
7183

@@ -74,6 +86,7 @@
7486
from samcli.lib.cfn_language_extensions.sam_integration import expand_language_extensions
7587
from samcli.lib.cfn_language_extensions.utils import is_foreach_key
7688
from samcli.lib.intrinsic_resolver.intrinsics_symbol_table import IntrinsicsSymbolTable
89+
from samcli.lib.providers.provider import get_full_path
7790
from samcli.lib.utils.resources import (
7891
RESOURCES_WITH_IMAGE_COMPONENT,
7992
RESOURCES_WITH_LOCAL_PATHS,
@@ -284,24 +297,112 @@ def _run_sam_transform(template: Dict[str, Any], parameter_values: Dict[str, Any
284297
return translated
285298

286299

300+
def _build_artifacts_map(template: Dict[str, Any]) -> Dict[str, str]:
301+
"""Build the ``artifacts: Dict[str, str]`` map BuildContext expects.
302+
303+
Keys are ``<stack_path>/<resource_id>`` full paths (with empty stack_path
304+
this collapses to just ``<resource_id>``); values are the placeholder
305+
string standing in for an on-disk built artifact location. We only
306+
populate entries for resources that actually carry a packageable artifact
307+
property — this matches BuildContext's check for whether to merge an
308+
expanded resource back into the original ForEach body.
309+
"""
310+
artifacts: Dict[str, str] = {}
311+
seen: set = set()
312+
for resource_id, _prop_path, _resource in _walk_artifact_properties(template):
313+
if resource_id in seen:
314+
continue
315+
seen.add(resource_id)
316+
artifacts[get_full_path("", resource_id)] = BUILT_ARTIFACT_PLACEHOLDER
317+
return artifacts
318+
319+
320+
def _merge_back_into_original(
321+
original_template: Dict[str, Any],
322+
modified_template: Dict[str, Any],
323+
artifacts: Dict[str, str],
324+
) -> Dict[str, Any]:
325+
"""Call ``BuildContext._get_template_for_output`` without instantiating a
326+
full BuildContext.
327+
328+
The method only invokes ``self.*`` for *other* methods on the same class
329+
(audited in PR 1). The ``stack`` parameter only reads three attributes:
330+
``original_template_dict``, ``parameters``, ``stack_path``. So we use a
331+
``BuildContext.__new__`` shell as ``self`` and a SimpleNamespace as the
332+
stack. No instance state is touched.
333+
"""
334+
# Lazy import: keep the build_context imports out of the harness
335+
# module-load path (it pulls in click, telemetry, etc.).
336+
from samcli.commands.build.build_context import BuildContext
337+
338+
stack = types.SimpleNamespace(
339+
original_template_dict=original_template,
340+
parameters={},
341+
stack_path="",
342+
)
343+
shell = BuildContext.__new__(BuildContext)
344+
# mypy: SimpleNamespace is structurally compatible with the three Stack
345+
# attributes _get_template_for_output reads (audited above), but it isn't
346+
# an actual Stack instance so the static type check rejects it.
347+
return BuildContext._get_template_for_output(
348+
shell,
349+
stack=stack, # type: ignore[arg-type]
350+
modified_template=modified_template,
351+
artifacts=artifacts,
352+
)
353+
354+
287355
def run_build_pipeline(template_path: Path, language_extensions: bool) -> Dict[str, Any]:
288356
"""In-process equivalent of `sam build` for one template.
289357
290-
1. Load the template.
291-
2. Run LE expansion (no-op if disabled or template has no LE transform).
292-
3. Run the SAM transform on the expanded template.
293-
4. Replace local artifact paths with BUILT_ARTIFACT_PLACEHOLDER.
358+
For non-LE templates (and LE-disabled): run SAM transform on the loaded
359+
template and rewrite local artifact paths to BUILT_ARTIFACT_PLACEHOLDER.
360+
361+
For templates that use LanguageExtensions and have it enabled:
362+
1. LE-expand to get a flat resource list. This is the modified-template
363+
lookup source for the merge.
364+
2. Skip the SAM transform on this LE branch. Real ``sam build`` does
365+
the same: the transform runs server-side at deploy time so the
366+
``.aws-sam/build/template.yaml`` keeps ``AWS::Serverless::Function``
367+
(and its ``CodeUri``). Running the SAM transform here would also
368+
break ``BuildContext._update_original_template_paths``, which keys
369+
its packageable-property lookup off the *original* ForEach body's
370+
resource type — a transformed modified template would carry
371+
``Code`` while the original body still has ``CodeUri``.
372+
3. Build the ``artifacts`` map from the LE-expanded resource ids.
373+
4. Replace local artifact paths in the LE-expanded copy with
374+
BUILT_ARTIFACT_PLACEHOLDER.
375+
5. Call ``BuildContext._get_template_for_output`` with the original
376+
(still ForEach-shaped) template and the modified expanded copy.
377+
This is the same code path real ``sam build`` uses to produce
378+
``.aws-sam/build/template.yaml`` and merges the placeholder values
379+
back into the ForEach body.
294380
"""
295381
template = _load_template(template_path)
296382

297383
parameter_values = dict(IntrinsicsSymbolTable.DEFAULT_PSEUDO_PARAM_VALUES)
298384

299385
le_result = expand_language_extensions(template, parameter_values=parameter_values, enabled=language_extensions)
300-
expanded = copy.deepcopy(le_result.expanded_template)
301386

302-
transformed = _run_sam_transform(expanded, parameter_values)
303-
_replace_local_artifact_paths(transformed)
304-
return transformed
387+
if not le_result.had_language_extensions:
388+
# No LE (either disabled or template doesn't use it): the transformed
389+
# template is what gets written to disk. Rewrite paths in place.
390+
expanded = copy.deepcopy(le_result.expanded_template)
391+
transformed = _run_sam_transform(expanded, parameter_values)
392+
_replace_local_artifact_paths(transformed)
393+
return transformed
394+
395+
# LE path: do NOT run SAM transform — see docstring above. Build the
396+
# artifacts map from the LE-expanded ids, rewrite paths to the
397+
# placeholder, then delegate the merge to BuildContext.
398+
expanded = copy.deepcopy(le_result.expanded_template)
399+
artifacts = _build_artifacts_map(expanded)
400+
_replace_local_artifact_paths(expanded)
401+
return _merge_back_into_original(
402+
original_template=le_result.original_template,
403+
modified_template=expanded,
404+
artifacts=artifacts,
405+
)
305406

306407

307408
class GoldenS3Uploader:
@@ -348,24 +449,50 @@ def run_package_pipeline(
348449
) -> Dict[str, Any]:
349450
"""In-process equivalent of `sam package` for one template + build output.
350451
351-
1. Walk packageable artifact properties on the build output and replace
352-
each local path with an s3://golden-bucket/<sha256> URI.
353-
2. Run _export_global_artifacts_pass on the rewritten template to
354-
handle any AWS::Include structural nodes.
355-
3. Return the final dict.
452+
For non-LE inputs: walk packageable artifact properties on the build
453+
output and replace each local path with an s3://golden-bucket/<sha256>
454+
URI; then run ``_export_global_artifacts_pass`` for AWS::Include.
455+
456+
For LE inputs (the build output preserves ``Fn::ForEach::*``): mirror
457+
``PackageContext._export_with_language_extensions`` —
458+
1. Run ``_export_global_artifacts_pass`` on a copy of the build_output
459+
so AWS::Include nodes resolve before LE expansion collapses
460+
structural Fn::Transform subtrees into JSON-string literals.
461+
2. LE-expand that copy to get a flat resource view.
462+
3. Walk the expanded resources, rewriting each artifact property to an
463+
s3://golden-bucket/... URI (or {S3Bucket,S3Key} dict for Lambdas).
464+
4. Call ``merge_language_extensions_s3_uris(original=build_output,
465+
exported=expanded-with-s3-uris)`` to copy the URIs back into the
466+
original ForEach body. The result preserves ``Fn::ForEach::*``.
356467
"""
357468
from samcli.lib.package.artifact_exporter import _export_global_artifacts_pass
469+
from samcli.lib.package.language_extensions_packaging import merge_language_extensions_s3_uris
358470

359471
template_dir = str(template_path.parent)
360472
uploader = GoldenS3Uploader(template_dir)
361473

362-
# Walk build_output's artifact properties and rewrite local paths.
363474
pkg = copy.deepcopy(build_output)
364-
_rewrite_artifacts_to_s3(pkg, uploader, template_dir)
365475

366-
# AWS::Include pass on the rewritten template.
476+
# AWS::Include pass before LE expansion (mirrors PackageContext._export_with_language_extensions).
367477
_export_global_artifacts_pass(pkg, uploader, template_dir)
368-
return pkg
478+
479+
le_result = expand_language_extensions(pkg, parameter_values=None, enabled=True)
480+
if not le_result.had_language_extensions:
481+
# Plain template (or LE disabled at build time): the build output is
482+
# already a flat resource list. Rewrite artifacts in place and return.
483+
_rewrite_artifacts_to_s3(pkg, uploader, template_dir)
484+
return pkg
485+
486+
# LE path: rewrite artifacts on the expanded copy, then merge back into the
487+
# original (build_output) which still has Fn::ForEach::* intact.
488+
expanded = copy.deepcopy(le_result.expanded_template)
489+
_rewrite_artifacts_to_s3(expanded, uploader, template_dir)
490+
491+
return merge_language_extensions_s3_uris(
492+
original_template=pkg,
493+
exported_template=expanded,
494+
dynamic_properties=le_result.dynamic_artifact_properties,
495+
)
369496

370497

371498
def _rewrite_artifacts_to_s3(template: Dict[str, Any], uploader, template_dir: str) -> None:

tests/golden/test_harness_build.py

Lines changed: 28 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -16,13 +16,36 @@ def test_build_sam_case_replaces_codeuri_with_placeholder():
1616
assert func["Properties"]["Code"] == "<<BUILT_ARTIFACT>>"
1717

1818

19-
def test_build_le_case_expands_foreach():
19+
def test_build_le_case_preserves_foreach():
20+
"""LE templates must keep Fn::ForEach::* in the output.
21+
22+
Real ``sam build`` produces a template that preserves the original
23+
``Fn::ForEach`` structure so CloudFormation can re-expand it server-side
24+
at deploy time. Inside the body, the artifact property is rewritten to
25+
the build placeholder. The body resource type stays
26+
``AWS::Serverless::Function`` because the SAM transform also runs
27+
server-side, not at build time. See PR #8637.
28+
"""
2029
case = CASES_ROOT / "language_extensions" / "foreach_static_zip"
2130
result = run_build_pipeline(case / "template.yaml", language_extensions=True)
22-
assert "AlphaFunction" in result["Resources"]
23-
assert "BetaFunction" in result["Resources"]
24-
# ForEach key gone
25-
assert not any(k.startswith("Fn::ForEach") for k in result["Resources"])
31+
32+
# ForEach key preserved at the top of Resources.
33+
foreach_keys = [k for k in result["Resources"] if k.startswith("Fn::ForEach")]
34+
assert foreach_keys, "Fn::ForEach::* must survive the build pipeline for LE templates"
35+
36+
# The expanded resource ids must NOT appear at the top level — that would
37+
# mean the ForEach got expanded away, which is exactly the bug PR #8637
38+
# fixed. The harness must mirror real-world output.
39+
assert "AlphaFunction" not in result["Resources"]
40+
assert "BetaFunction" not in result["Resources"]
41+
42+
# Body is still a Serverless::Function (SAM transform runs server-side),
43+
# and its CodeUri now points at the placeholder.
44+
foreach_value = result["Resources"][foreach_keys[0]]
45+
body = foreach_value[2]
46+
body_resource = next(iter(body.values()))
47+
assert body_resource["Type"] == "AWS::Serverless::Function"
48+
assert body_resource["Properties"]["CodeUri"] == "<<BUILT_ARTIFACT>>"
2649

2750

2851
def test_build_le_case_skipped_when_disabled():

tests/golden/test_harness_package.py

Lines changed: 23 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -17,14 +17,32 @@ def test_package_sam_case_rewrites_code_to_s3_uri():
1717
assert "S3Key" in code
1818

1919

20-
def test_package_le_case_rewrites_each_expanded_codeuri():
20+
def test_package_le_case_preserves_foreach_with_s3_uri():
21+
"""LE templates must keep Fn::ForEach::* through the package pipeline.
22+
23+
The body's artifact property (CodeUri here) should be rewritten to an
24+
S3 URI string so that, at deploy time, CloudFormation can re-expand the
25+
ForEach and each iteration receives the same packaged artifact. See
26+
``merge_language_extensions_s3_uris``.
27+
"""
2128
case = CASES_ROOT / "language_extensions" / "foreach_static_zip"
2229
build_out = run_build_pipeline(case / "template.yaml", language_extensions=True)
2330
pkg_out = run_package_pipeline(case / "template.yaml", build_out)
24-
for fn_id in ("AlphaFunction", "BetaFunction"):
25-
code = pkg_out["Resources"][fn_id]["Properties"]["Code"]
26-
assert isinstance(code, dict)
27-
assert code.get("S3Bucket") == "golden-bucket"
31+
32+
foreach_keys = [k for k in pkg_out["Resources"] if k.startswith("Fn::ForEach")]
33+
assert foreach_keys, "Fn::ForEach::* must survive the package pipeline for LE templates"
34+
# No expanded ids at the top level.
35+
assert "AlphaFunction" not in pkg_out["Resources"]
36+
assert "BetaFunction" not in pkg_out["Resources"]
37+
38+
foreach_value = pkg_out["Resources"][foreach_keys[0]]
39+
body = foreach_value[2]
40+
body_resource = next(iter(body.values()))
41+
code_uri = body_resource["Properties"]["CodeUri"]
42+
# Single shared S3 URI for the whole loop (static collection, no
43+
# per-iteration Mappings needed).
44+
assert isinstance(code_uri, str)
45+
assert code_uri.startswith("s3://golden-bucket/")
2846

2947

3048
def test_package_cfn_case_rewrites_raw_code_path():

0 commit comments

Comments
 (0)