fix(package): rewrite AWS::Include Location buried inside language-extension functions by bnusunny · Pull Request #9030 · aws/aws-sam-cli

bnusunny · 2026-05-19T03:36:55Z

Which issue(s) does this change fix?

Why is this change necessary?

When a template uses both Transform: AWS::LanguageExtensions and contains Fn::Transform: AWS::Include buried inside a language-extension function (e.g. Fn::ToJsonString), sam package fails to rewrite the include's Location to an S3 URL. CloudFormation then rejects the deploy with The location parameter is not a valid S3 uri.

This is a regression introduced in sam-cli 1.160.0, blocking customers who package AWS::Include inside SSM Parameter Value, function environment variables, or any other dictionary-shaped property fed through Fn::ToJsonString.

How does it address the issue?

PackageContext._export previously ran expand_language_extensions before the artifact exporter walked Fn::Transform nodes. LE functions like Fn::ToJsonString json.dumps() their argument, collapsing the structural Fn::Transform subtree into a JSON-string literal. By the time the exporter ran, the include was no longer a structural dict node and was invisible to the global-transform walker.

This PR processes AWS::Include (and any other GLOBAL_TRANSFORM_EXPORTS handler) on the original template before LE expansion, mirroring CloudFormation's own server-side ordering — CFN resolves inline Fn::Transform macros before evaluating AWS::LanguageExtensions. Verified empirically against the live CFN service before pivoting to this approach.

The change is split into 5 focused commits:

refactor(package) — replace flat METADATA_EXPORT_LIST / GLOBAL_EXPORT_DICT with typed MetadataExportSpec / GlobalTransformExportSpec registries. Pure refactor; no behavior change.
feat(package) — extend merge_language_extensions_s3_uris to merge SAR Metadata exports back into LE child templates (a related Metadata-merge gap surfaced while building the registries).
fix(package) — extract _export_global_artifacts_pass to a module-level free function and call it pre-LE in both PackageContext._export (root flow) and CloudFormationStackResource.do_export (nested-stack flow). This is the Bug: The location parameter is not a valid S3 uri #9027 fix.
test(package) — unit + reproducer-style tests covering the buried-AWS::Include case from the issue, AWS::Include in Outputs, SAR metadata merge, and a nested-stack variant.
docs(cfn-lang-ext) — document the AWS::Include-before-LE processing order in docs/cfn-language-extensions.md.

What side effects does this change have?

AWS::Include resolution now runs once on the original template before LE expansion, in addition to the existing post-expansion pass. The pass is idempotent: once Location has been rewritten to s3://..., the second walk treats it as already exported and is a no-op (verified via the pre-existing is_local_file short-circuit).
The two registry refactors are behavior-preserving — the existing _export_metadata and _export_global_artifacts callers go through one-line wrappers that delegate to the new typed registries.
SAR Metadata (LicenseUrl, ReadmeUrl) in LE templates is now correctly merged back into the deploy-bound template. Previously the artifacts were uploaded to S3 but the URLs were silently dropped from the output template.
No new public API; no dependency changes; no telemetry changes.

Dynamic-Location AWS::Include inside Fn::ForEach (e.g. Location: ./swagger-${Name}.yaml) remains unsupported by sam package: a local file path with literal ${...} placeholders does not exist on disk, so is_local_file fails and the existing InvalidLocalPathError fires. CloudFormation does not substitute loop variables into Fn::Transform paths server-side either, so the limitation matches CFN's actual capability.

Mandatory Checklist

Review the generative AI contribution guidelines
Add input/output type hints to new functions/methods
Write design document if needed (Do I need to write a design document?) — N/A; bug fix mirroring CFN's existing server-side behavior.
Write/update unit tests — added unit coverage in test_artifact_exporter.py, test_package_context.py, and a new test_language_extensions_packaging.py.
Write/update integration tests — the new tests are end-to-end against _export but use mocked S3; can add a true integration test under tests/integration/package if desired.
Write/update functional tests if needed
make pr passes — make lint and black --check clean; affected unit tests (245) pass.
make update-reproducible-reqs if dependencies were changed — N/A; no dependency changes.
Write documentation — new "AWS::Include processing order" section in docs/cfn-language-extensions.md.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

aws-sam-cli-bot

Code Review Results

Reviewed: 9ffa5ed..f05f2d4
Files: 8
Comments: 1

Comments on lines outside the diff:

[samcli/lib/package/artifact_exporter.py:339] [GENERAL] The default value for the metadata_to_export constructor parameter changes from frozenset(METADATA_EXPORT_LIST) to tuple(METADATA_EXPORTS), but the class-level annotation just above still declares:

class Template:
   ...
   metadata_to_export: frozenset

After this PR, self.metadata_to_export is assigned a tuple of MetadataExportSpec instances, not a frozenset of exporter classes. The annotation is now both wrong about the container type (tuple vs frozenset) and silent about the new element type (MetadataExportSpec vs Type[Resource]), which will mislead future readers and is incompatible with strict mypy.

Suggest updating the annotation to match the new shape, e.g.:

from typing import Sequence
...
class Template:
   ...
   metadata_to_export: Sequence["MetadataExportSpec"]

(or Tuple[MetadataExportSpec, ...] if you want to lock in tuple-ness). Same thing applies to resources_to_export: frozenset if the registry refactor is later extended there, but for this PR only metadata_to_export drifts.

…T with typed registries Migrate two flat package exporter registries to typed dataclass form: - METADATA_EXPORT_LIST (list of Resource subclasses) -> METADATA_EXPORTS (List[MetadataExportSpec]). Each spec captures both the metadata_type ('AWS::ServerlessRepo::Application') and the per-property exporter classes that handle LicenseUrl/ReadmeUrl. Template._export_metadata now dispatches via a metadata_type lookup instead of a per-class RESOURCE_TYPE filter. - GLOBAL_EXPORT_DICT (dict keyed by Fn::Transform) -> GLOBAL_TRANSFORM_EXPORTS (List[GlobalTransformExportSpec]). Each spec carries a discriminator callable (e.g. _is_aws_include for matching AWS::Include) plus the handler. _export_global_artifacts now dispatches through the discriminator so future Fn::Transform variants can register without touching the walker. The typed shape is the contract a follow-up commit will read from to process AWS::Include before language-extension expansion.

Extend merge_language_extensions_s3_uris with a registry-driven Metadata pass that copies rewritten property values (LicenseUrl, ReadmeUrl) from the exported template back into the original (Fn::ForEach-preserving) template after LE expansion. Without this pass, when a child stack uses Transform: AWS::LanguageExtensions and declares AWS::ServerlessRepo::Application metadata, sam package silently dropped the License/Readme S3 URLs from the merged output — they were uploaded but never wired into the template the user deploys. Implementation iterates METADATA_EXPORTS (the registry added in the prior commit) so future metadata types pick up merge support without touching the merge walker.

#9027) Closes #9027. Symptom: when a template uses both Transform: AWS::LanguageExtensions and contains Fn::Transform: AWS::Include buried inside an LE function (e.g. Fn::ToJsonString), sam package fails to rewrite the include's Location to an S3 URL. CloudFormation then rejects the deploy with 'The location parameter is not a valid S3 uri.' Root cause: PackageContext._export ran expand_language_extensions BEFORE the artifact exporter walked Fn::Transform nodes. LE functions like Fn::ToJsonString json.dumps() their argument, collapsing the structural Fn::Transform subtree into a JSON-string literal. By the time the exporter ran, the include was no longer a structural dict node and was invisible to the global-transform walker. Fix: process AWS::Include (and any other GLOBAL_TRANSFORM_EXPORTS handler) on the original template BEFORE LE expansion runs, mirroring CloudFormation's own server-side transform ordering — CFN resolves inline Fn::Transform macros before evaluating AWS::LanguageExtensions. Implementation: - Extract Template._export_global_artifacts to a module-level _export_global_artifacts_pass(template_dict, uploader, template_dir). The instance method becomes a one-line delegate so existing callers keep working. - Call _export_global_artifacts_pass on the original template before expand_language_extensions in PackageContext._export (root flow) and in CloudFormationStackResource.do_export (nested-stack child flow). Dynamic-Location AWS::Include inside Fn::ForEach (e.g. Location: ./swagger-${Name}.yaml) is not supported by sam package: a local file path with literal ${...} placeholders does not exist on disk, so is_local_file fails and the existing InvalidLocalPathError fires — which is the right user-facing failure. CloudFormation does not substitute loop variables into Fn::Transform paths server-side either, so the limitation matches CFN's actual capability.

…n LE templates Three end-to-end tests that exercise the package_context._export-equivalent flow on language-extension templates: - test_le_template_with_top_level_aws_include_merges_location verifies AWS::Include in Outputs gets its Location rewritten to s3:// after the pre-LE pass. - test_le_template_with_serverless_repo_metadata_merges_license_url verifies SAR LicenseUrl/ReadmeUrl in Metadata get merged back. - TestPackageContextIssue9027 reproduces the user template from #9027 (Fn::ToJsonString over Fn::Transform: AWS::Include buried inside AWS::SSM::Parameter) and asserts the buried Location is rewritten to s3://. Locks down the regression.

…e extensions Add a section explaining that sam package processes Fn::Transform: AWS::Include before language-extension expansion, mirroring CloudFormation's server-side transform ordering. This means AWS::Include Location rewrites work correctly even when the include lives buried inside language-extension functions like Fn::ToJsonString or Fn::ForEach bodies.

bnusunny · 2026-05-19T03:58:55Z

Thanks for the catch — fixed in the rewritten a69b86e8d → 1783eda47 (the registry refactor commit). The class-level annotation now reads:

from typing import Sequence
...
class Template:
    metadata_to_export: Sequence[MetadataExportSpec]

Sequence keeps the door open for either a tuple or a list at the call site (the default is tuple(METADATA_EXPORTS)); the element type is now explicit.

resources_to_export: frozenset left as-is for this PR — its element type didn't change in this refactor, only metadata_to_export drifted. Happy to migrate it in a follow-up if preferred.

make lint and black --check still clean; 245 affected unit tests pass.

PR #9033 made AWS::LanguageExtensions local processing opt-in. Two API changes broke three tests on this branch after the merge from develop: - expand_language_extensions() now requires a keyword-only `enabled` arg - PackageContext reads self._language_extensions_enabled, set in __init__ Pass enabled=True to expand_language_extensions in the two artifact-exporter tests, and set _language_extensions_enabled on the PackageContext stub that bypasses __init__ via __new__. All three tests exercise templates with Transform: AWS::LanguageExtensions, so enabling LE is the intended behavior.

…lude Move os, Destination/Uploaders, and yaml_parse to module-level imports; drop the redundant inline tempfile/PackageContext (already at module level). Addresses inline-imports review comment on #9030.

…age_extensions imports Move the two inline imports inside _export() to module scope. No behavior change; this is preparation for the upcoming _export() split which references these from a new branch method.

…branch) New private method that mirrors pre-1.160.0 sam-cli behavior: a tight Template.export() pipeline with no LE machinery. Unused by _export() until the dispatcher is cut over in a follow-up commit. Also adds two structural-gate smoke tests that stay red until the dispatcher is wired up — they assert that the off path never invokes expand_language_extensions or the pre-LE _export_global_artifacts_pass.

New private method containing the existing #9027 ordering: pre-LE include pass, LE expansion, Template.export(), merge, Mappings. Unused by _export() until the dispatcher is cut over in the next commit.

… flag _export() becomes a thin dispatcher: read the template, branch on self._language_extensions_enabled, dump. The two branch methods added in the prior two commits now own the actual work. Treats --language-extensions as a hard correctness boundary: when off, no LE machinery is invoked at all (no expand_language_extensions, no pre-LE _export_global_artifacts_pass, no merge, no Mappings). Template.export() still handles AWS::Include for non-LE templates via its own internal _export_global_artifacts pass — that path has worked since long before the #9027 fix and is unchanged by this refactor. Public surface (PackageContext._export(template_path, use_json) -> str) is preserved; all existing _export tests pass unchanged.

…context use site The hoist in 756c502 moved `expand_language_extensions` to a module-level import in `package_context.py`. The use-site binding is now captured at module load, so existing tests that patched the source module `samcli.lib.cfn_language_extensions.sam_integration.expand_language_extensions` no longer intercept calls from `_export()`. Repoint two patches at the use-site `samcli.commands.package.package_context.expand_language_extensions`: - `test_phase_separation.py::test_package_context_export_calls_expand_language_extensions` was failing intermittently in CI depending on test-collection order. - `test_package_context.py::test_off_path_does_not_invoke_expand_language_extensions` is retargeted for consistency now that the inline-import shadow is gone (post-Task-4 cutover); the dead `had_language_extensions=False` scaffolding can also go since the off path no longer calls expand. Other source-module patches in `test_artifact_exporter.py` are correct as-is — those tests cover `Template.export()` paths whose use site is the artifact_exporter module.

…tches Hoist expand_language_extensions, IntrinsicsSymbolTable, generate_and_apply_artifact_mappings, and merge_language_extensions_s3_uris to module-level imports in artifact_exporter.py — preparation for the LE / non-LE structural-gate split of CloudFormationStackResource.do_export. Module-level binding requires existing tests that patched samcli.lib.cfn_language_extensions.sam_integration.expand_language_extensions (the source module) to repoint at the use-site samcli.lib.package.artifact_exporter.expand_language_extensions, since the use-site name is captured at module load and source-module patches no longer intercept once the inline import is gone. Same lesson as PR #9030 commit 45dea4b for package_context.

…method LE-off branch for CloudFormationStackResource: path-based Template construction with no pre-LE pass, no parameter_values, no deepcopy. Method is unused until the dispatcher rewrite — wiring lands in the follow-up commit. Adds TestDoExportLanguageExtensionsStructuralGate with two red structural-gate assertions that the dispatcher rewrite will turn green.

Lifts the existing LE-expansion body from CloudFormationStackResource.do_export into a dedicated method (resource_id, template_path, parent_dir, abs_template_path, resource_dict) -> Dict. No behavior change yet — the dispatcher in the follow-up commit will route LE-on calls into it. Threads resource_dict so the branch can read nested-stack Parameters without the dispatcher passing them separately. Returns the exported template dict so the dispatcher owns the final yaml_dump + upload tail.

… flag CloudFormationStackResource.do_export is now a thin dispatcher that: 1) does the early-exit guards (None / S3 URL / non-local file) 2) routes to _do_export_with_language_extensions or _do_export_without_language_extensions based on self.language_extensions_enabled 3) owns the final yaml_dump + upload + property mutation tail. The off path is now structurally LE-free: no expand_language_extensions call, no _export_global_artifacts_pass, no IntrinsicsSymbolTable pseudo-param plumbing, no parent_parameter_values, no copy.deepcopy. Template.export() runs its own internal _export_global_artifacts so AWS::Include still resolves on the off path. The structural-gate tests added in the previous commit are now green. Existing LE tests that rely on language_extensions_enabled = True now set the flag explicitly (where they previously depended on the LE machinery running unconditionally as a passthrough).

Black collapsed multi-line calls and signatures whose single-line form fits under the 120-char limit. No behavior change.

Adds a CFN-parity helper that builds the parameter_values dict for a nested child stack: pseudo-params (with parent pseudo-name overrides honored) plus parent-rebound values via the nested-stack Parameters property. Non-pseudo parent names are not copied; child Defaults are read by the LE expander itself via parsed_template.parameters. Helper is unused production code in this commit. Wired into CloudFormationStackResource._do_export_with_language_extensions in the next commit.

CloudFormationStackResource._do_export_with_language_extensions used to bulk-copy the parent stack's full parameter_values into the child, so a parent's `Foo=parent_foo` could silently shadow an unrelated child Parameter named Foo. This diverged from CloudFormation's nested-stack contract (only parent-rebound names reach the child) and from the non-LE path (which threads no parameters at all). Replace the merge block with _build_child_parameter_values, which returns pseudo-params (with parent pseudo-name overrides honored) plus parent-rebound values from the nested-stack Parameters property. Child template Defaults still resolve via the LE expander's parsed_template fallback (resolvers/fn_ref.py:_resolve_parameter). Adds end-to-end coverage: - non-pseudo parent param does not leak into child's parameter_values - child Default still resolves via resolver fallback after the change

…cstrings Code-review polish for the LE parent-param scoping fix: - drop call-site comment block at _do_export_with_language_extensions that duplicated _build_child_parameter_values's docstring - shorten the two new test docstrings in TestCloudFormationStackResourceChildExpansion to match neighbor style (no internal module paths)

@patch

Move imports introduced by the previous two commits from inside test bodies up to the module-level import block: - _build_child_parameter_values, IntrinsicsSymbolTable - yaml_dump (samcli.yamlhelper), shutil, inspect (stdlib) None of these symbols are @patch targets, so hoisting does not affect any mock binding (use-site patches in artifact_exporter remain correct).

@patch

Address review feedback on PR #9030: inline imports should live at module scope, and on-the-fly file writes for LE child templates should use checked-in fixtures instead of tempfile.mkdtemp() + yaml_dump(). Hoist inline imports (LanguageExtensionResult, _resolve_nested_stack_parameters, yaml_dump, shutil, the packageable_resources block) to the module-level import block. None are @patch targets, so use-site patches remain correct. Add tests/unit/lib/package/test_data/ following the precedent in tests/unit/lib/intrinsic_resolver/test_data, with TEST_DATA_PATH constant defined as Path(__file__).resolve().parent / "test_data". Convert the LE parent-param test methods, the buried-AWS::Include test, the expansion-error-handling pair, and the structural-gate pair to read their child templates (and the export-events.json include target) from the fixture tree. The on-the-fly tempdir + yaml_dump scaffolding is removed entirely along with the corresponding _write_child / _write_minimal_child helpers. No production code change; all 110 unit tests in test_artifact_exporter still pass.

…xture Replace the on-the-fly tempfile + open()+write() scaffolding in TestPackageContextBuriedAWSInclude.setUp with a checked-in fixture under tests/unit/commands/package/test_data/buried_aws_include/. Mirrors the TEST_DATA_PATH pattern already in tests/unit/lib/package/test_data and tests/unit/lib/intrinsic_resolver/test_data. setUp now resolves template_path from TEST_DATA_PATH; the export-events include target lives next to template.yaml so the relative Location in the template still resolves at sam-package time. No production code change; all 146 tests in test_package_context still pass.

valerena · 2026-05-28T00:58:13Z

+    specs_by_key: Dict[str, list] = {}
+    for spec in GLOBAL_TRANSFORM_EXPORTS:
+        specs_by_key.setdefault(spec.template_key, []).append(spec)


this doesn't depend on the parameters of this method.. should we just have this as a global variable like GLOBAL_TRANSFORM_EXPORTS? (not needed, but to consider)

bnusunny requested a review from a team as a code owner May 19, 2026 03:36

github-actions Bot added area/package sam package command pr/internal labels May 19, 2026

aws-sam-cli-bot reviewed May 19, 2026

View reviewed changes

bnusunny added 5 commits May 18, 2026 20:57

bnusunny force-pushed the fix/9027-le-package-merge branch from f05f2d4 to 38c5b70 Compare May 19, 2026 03:58

bnusunny requested review from roger-zhangg and vicheey May 19, 2026 16:01

roger-zhangg reviewed May 19, 2026

View reviewed changes

Comment thread tests/unit/commands/package/test_package_context.py Outdated

roger-zhangg reviewed May 19, 2026

View reviewed changes

Comment thread tests/unit/commands/package/test_package_context.py Outdated

Denny-g6labs mentioned this pull request May 20, 2026

Bug: The location parameter is not a valid S3 uri #9027

Closed

bnusunny added 13 commits May 20, 2026 18:48

Merge branch 'develop' into fix/9027-le-package-merge

e0ec117

test(package): hoist inline imports in TestPackageContextBuriedAWSInc…

8109508

…lude Move os, Destination/Uploaders, and yaml_parse to module-level imports; drop the redundant inline tempfile/PackageContext (already at module level). Addresses inline-imports review comment on #9030.

refactor(package): hoist InvalidSamDocumentException and expand_langu…

756c502

…age_extensions imports Move the two inline imports inside _export() to module scope. No behavior change; this is preparation for the upcoming _export() split which references these from a new branch method.

refactor(package): add _export_with_language_extensions (on-path branch)

3841151

New private method containing the existing #9027 ordering: pre-LE include pass, LE expansion, Template.export(), merge, Mappings. Unused by _export() until the dispatcher is cut over in the next commit.

chore(package): apply make format to do_export refactor

f7e9ba8

Black collapsed multi-line calls and signatures whose single-line form fits under the 120-char limit. No behavior change.

bnusunny requested a review from roger-zhangg May 21, 2026 05:23

bnusunny requested a review from aws-sam-cli-bot May 21, 2026 05:23

bnusunny added 3 commits May 21, 2026 10:01

bnusunny requested a review from valerena May 21, 2026 17:41