Skip to content

Commit f7afefb

Browse files
committed
fix(cfn-lang-ext): route Fn::ForEach dynamic-property consumers through jmespath
PACKAGEABLE_RESOURCE_ARTIFACT_PROPERTIES now contains dotted paths like Command.ScriptLocation (Glue) and Code.ImageUri (Lambda image), but four call sites in build_context, sam_integration, and language_extensions_packaging were still doing flat-key dict access: - _update_foreach_artifact_paths - _count_dynamic_properties - _get_artifact_value - detect_foreach_dynamic_properties For dotted entries those reads return None, the early skip kicks in, no SAM* mapping is generated, and the dotted artifact value never propagates back to the Fn::ForEach body. CloudFormation Mapping names and FindInMap third-arg keys also can't contain dots. Switch the four sites to _get_prop_value / _set_prop_value, derive the Mapping name and FindInMap third-arg from the leaf segment via a new _leaf_prop_name helper, and add _resolve_property_paths to skip parent paths when a more specific dotted child is present (so Lambda::Function "Code" doesn't shadow "Code.ImageUri" for image functions). Adds 6 regression tests covering Fn::ForEach over Glue::Job with templated Command.ScriptLocation and Lambda::Function with templated Code.ImageUri at both detection and Mapping-generation layers. Addresses review feedback on PR #9009.
1 parent cc97332 commit f7afefb

5 files changed

Lines changed: 358 additions & 27 deletions

File tree

samcli/commands/build/build_context.py

Lines changed: 44 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -553,6 +553,12 @@ def _update_foreach_artifact_paths(
553553
"""
554554
from samcli.lib.cfn_language_extensions.models import PACKAGEABLE_RESOURCE_ARTIFACT_PROPERTIES
555555
from samcli.lib.cfn_language_extensions.sam_integration import resolve_collection
556+
from samcli.lib.package.language_extensions_packaging import (
557+
_get_prop_value,
558+
_leaf_prop_name,
559+
_resolve_property_paths,
560+
_set_prop_value,
561+
)
556562

557563
generated_mappings: Dict[str, Dict[str, Dict[str, str]]] = {}
558564

@@ -599,11 +605,17 @@ def _update_foreach_artifact_paths(
599605
if not isinstance(properties, dict):
600606
continue
601607

602-
for prop_name in PACKAGEABLE_RESOURCE_ARTIFACT_PROPERTIES.get(resource_type, []):
603-
prop_value = properties.get(prop_name)
608+
candidate_paths = PACKAGEABLE_RESOURCE_ARTIFACT_PROPERTIES.get(resource_type, [])
609+
for prop_name in _resolve_property_paths(candidate_paths, properties):
610+
prop_value = _get_prop_value(properties, prop_name)
604611
if prop_value is None:
605612
continue
606613

614+
# CloudFormation Mapping names and FindInMap third-args must be alphanumeric,
615+
# so dotted property names (e.g. "Command.ScriptLocation") use the leaf segment
616+
# for those identifiers while the dotted form is preserved for property addressing.
617+
leaf_name = _leaf_prop_name(prop_name)
618+
607619
if contains_loop_variable(prop_value, loop_variable) and collection_values:
608620
# Determine which outer loop variables the property references
609621
referenced_outer_vars = []
@@ -622,7 +634,7 @@ def _update_foreach_artifact_paths(
622634
referenced_outer_vars=referenced_outer_vars,
623635
)
624636
if mapping_entries:
625-
mapping_name = f"SAM{prop_name}{nesting_path}"
637+
mapping_name = f"SAM{leaf_name}{nesting_path}"
626638
if dynamic_props_count.get(prop_name, 0) > 1:
627639
suffix = sanitize_resource_key_for_mapping(resource_template_key)
628640
mapping_name = f"{mapping_name}{suffix}"
@@ -637,7 +649,7 @@ def _update_foreach_artifact_paths(
637649
else:
638650
lookup_key = {"Ref": loop_variable}
639651

640-
properties[prop_name] = {"Fn::FindInMap": [mapping_name, lookup_key, prop_name]}
652+
_set_prop_value(properties, prop_name, {"Fn::FindInMap": [mapping_name, lookup_key, leaf_name]})
641653
else:
642654
expanded_key = self._build_expanded_key(
643655
resource_template_key,
@@ -648,7 +660,7 @@ def _update_foreach_artifact_paths(
648660
if expanded_key:
649661
artifact_value = self._get_artifact_value(modified_resources, expanded_key, prop_name)
650662
if artifact_value is not None:
651-
properties[prop_name] = artifact_value
663+
_set_prop_value(properties, prop_name, artifact_value)
652664

653665
# Propagate auto dependency layer references from expanded functions
654666
# to the ForEach body. Each expanded function may have Layers entries
@@ -707,6 +719,7 @@ def _count_dynamic_properties(
707719
share the same property name (e.g., two resources both with DefinitionUri).
708720
"""
709721
from samcli.lib.cfn_language_extensions.models import PACKAGEABLE_RESOURCE_ARTIFACT_PROPERTIES
722+
from samcli.lib.package.language_extensions_packaging import _get_prop_value, _resolve_property_paths
710723

711724
count: Counter = Counter()
712725
for rtk, rt in body.items():
@@ -718,8 +731,9 @@ def _count_dynamic_properties(
718731
rprops = rt.get("Properties", {})
719732
if not isinstance(rprops, dict):
720733
continue
721-
for pname in PACKAGEABLE_RESOURCE_ARTIFACT_PROPERTIES.get(rtype, []):
722-
pval = rprops.get(pname)
734+
candidate_paths = PACKAGEABLE_RESOURCE_ARTIFACT_PROPERTIES.get(rtype, [])
735+
for pname in _resolve_property_paths(candidate_paths, rprops):
736+
pval = _get_prop_value(rprops, pname)
723737
if pval is not None and contains_loop_variable(pval, loop_variable) and collection_values:
724738
count[pname] += 1
725739
return count
@@ -759,8 +773,15 @@ def _collect_dynamic_mapping_entries(
759773
760774
For nested ForEach, enumerates all outer value combinations to find
761775
the fully-expanded resource name.
776+
777+
``prop_name`` may be a jmespath dotted path; the value-dict key stored
778+
in the Mapping uses the leaf segment so it stays alphanumeric and
779+
matches the third argument of the generated Fn::FindInMap.
762780
"""
781+
from samcli.lib.package.language_extensions_packaging import _leaf_prop_name
782+
763783
mapping_entries: Dict[str, Dict[str, str]] = {}
784+
leaf_name = _leaf_prop_name(prop_name)
764785

765786
for coll_value in collection_values:
766787
if outer_context:
@@ -778,7 +799,7 @@ def _collect_dynamic_mapping_entries(
778799
expanded_key = substitute_loop_variable(resource_template_key, loop_variable, coll_value)
779800
artifact_value = self._get_artifact_value(modified_resources, expanded_key, prop_name)
780801
if artifact_value is not None:
781-
mapping_entries[coll_value] = {prop_name: artifact_value}
802+
mapping_entries[coll_value] = {leaf_name: artifact_value}
782803

783804
return mapping_entries
784805

@@ -793,9 +814,15 @@ def _collect_nested_mapping_entry(
793814
mapping_entries: Dict[str, Dict[str, str]],
794815
referenced_outer_vars: Optional[List[Tuple[str, List[str]]]] = None,
795816
) -> None:
796-
"""Enumerate outer value combinations to find expanded resource for a nested ForEach."""
817+
"""Enumerate outer value combinations to find expanded resource for a nested ForEach.
818+
819+
``prop_name`` may be a jmespath dotted path; the value-dict key uses the leaf segment.
820+
"""
797821
import itertools
798822

823+
from samcli.lib.package.language_extensions_packaging import _leaf_prop_name
824+
825+
leaf_name = _leaf_prop_name(prop_name)
799826
outer_collections = [oc[1] for oc in outer_context]
800827
outer_vars = [oc[0] for oc in outer_context]
801828

@@ -821,7 +848,7 @@ def _collect_nested_mapping_entry(
821848
mapping_key = coll_value
822849

823850
if mapping_key not in mapping_entries:
824-
mapping_entries[mapping_key] = {prop_name: artifact_value}
851+
mapping_entries[mapping_key] = {leaf_name: artifact_value}
825852

826853
def _collect_foreach_layer_mappings(
827854
self,
@@ -895,14 +922,19 @@ def _extract_nested_stack_layer_output(modified_resources: Dict, expanded_key: s
895922

896923
@staticmethod
897924
def _get_artifact_value(modified_resources: Dict, expanded_key: str, prop_name: str) -> Optional[Any]:
898-
"""Extract an artifact property value from an expanded resource, or return None."""
925+
"""Extract an artifact property value from an expanded resource, or return None.
926+
927+
``prop_name`` may be a jmespath dotted path (e.g. "Command.ScriptLocation").
928+
"""
929+
from samcli.lib.package.language_extensions_packaging import _get_prop_value
930+
899931
modified_resource = modified_resources.get(expanded_key, {})
900932
if not isinstance(modified_resource, dict):
901933
return None
902934
modified_props = modified_resource.get("Properties", {})
903935
if not isinstance(modified_props, dict):
904936
return None
905-
return modified_props.get(prop_name)
937+
return _get_prop_value(modified_props, prop_name)
906938

907939
def _copy_artifact_paths(self, original_resource: Dict, modified_resource: Dict) -> None:
908940
"""

samcli/lib/cfn_language_extensions/sam_integration.py

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -412,8 +412,10 @@ def detect_foreach_dynamic_properties(
412412
if not isinstance(properties, dict):
413413
continue
414414

415-
for prop_name in artifact_properties:
416-
prop_value = properties.get(prop_name)
415+
from samcli.lib.package.language_extensions_packaging import _get_prop_value, _resolve_property_paths
416+
417+
for prop_name in _resolve_property_paths(artifact_properties, properties):
418+
prop_value = _get_prop_value(properties, prop_name)
417419
if prop_value is not None:
418420
if contains_loop_variable(prop_value, loop_variable):
419421
dynamic_properties.append(

samcli/lib/package/language_extensions_packaging.py

Lines changed: 72 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -134,6 +134,50 @@ def _set_prop_value(props: Dict[str, Any], prop_name: str, value: Any) -> None:
134134
set_value_from_jmespath(props, prop_name, value)
135135

136136

137+
def _leaf_prop_name(prop_name: str) -> str:
138+
"""Return the last segment of a jmespath property name.
139+
140+
CloudFormation Mapping names must be alphanumeric, and the third argument of
141+
Fn::FindInMap and the keys of Mapping value-dicts must match each other as
142+
plain strings. Dotted property names (e.g. "Command.ScriptLocation") are
143+
used to *address* the property on a resource, but the *identifier* used in
144+
Mapping names and FindInMap lookups must use only the leaf segment so the
145+
generated CloudFormation is well-formed.
146+
"""
147+
return prop_name.rsplit(".", 1)[-1]
148+
149+
150+
def _resolve_property_paths(prop_names: List[str], properties: Dict[str, Any]) -> List[str]:
151+
"""Filter ``prop_names`` so a parent path is dropped when a more specific
152+
child path is present in ``properties``.
153+
154+
Some resource types declare multiple alternative artifact paths in
155+
``PACKAGEABLE_RESOURCE_ARTIFACT_PROPERTIES``. For example,
156+
``AWS::Lambda::Function`` lists ``Code`` (used for ZIP packages) and
157+
``Code.ImageUri`` (used for image packages). A given user template uses
158+
only one of these shapes, but ``Code`` is a prefix of ``Code.ImageUri``,
159+
so a naive iteration would address both paths and treat the same nested
160+
value twice. Returning only the most specific resolved path picks the
161+
correct interpretation for the user's actual property shape.
162+
"""
163+
# Sort longest-first so child paths win over their parents.
164+
sorted_paths = sorted(prop_names, key=lambda p: -p.count("."))
165+
consumed_prefixes: set = set()
166+
selected: List[str] = []
167+
for path in sorted_paths:
168+
if _get_prop_value(properties, path) is None:
169+
continue
170+
# If a more specific path under this prefix has already been selected, skip.
171+
if any(other.startswith(path + ".") for other in consumed_prefixes):
172+
continue
173+
selected.append(path)
174+
consumed_prefixes.add(path)
175+
# Preserve original ordering for callers that care.
176+
order = {p: i for i, p in enumerate(prop_names)}
177+
selected.sort(key=lambda p: order.get(p, 0))
178+
return selected
179+
180+
137181
# ---------------------------------------------------------------------------
138182
# Merge helpers
139183
# ---------------------------------------------------------------------------
@@ -330,7 +374,9 @@ def _compute_mapping_name(
330374
compatibility.
331375
"""
332376
npath = _nesting_path(prop)
333-
base_name = f"SAM{prop.property_name}{npath}"
377+
# Use the leaf segment for the Mapping name so dotted property paths (e.g.
378+
# "Command.ScriptLocation") yield well-formed alphanumeric Mapping names.
379+
base_name = f"SAM{_leaf_prop_name(prop.property_name)}{npath}"
334380
key = (npath, prop.property_name)
335381
if collision_groups.get(key, 0) <= 1:
336382
return base_name
@@ -363,6 +409,10 @@ def _generate_artifact_mappings(
363409
_validate_mapping_key_compatibility(prop)
364410

365411
mapping_name = _compute_mapping_name(prop, collision_groups)
412+
# The Mapping value-dict key and the third FindInMap argument must use
413+
# the leaf segment so dotted property paths produce well-formed
414+
# CloudFormation (Mappings can't contain dots in either name or keys).
415+
leaf_name = _leaf_prop_name(prop.property_name)
366416

367417
if mapping_name not in mappings:
368418
mappings[mapping_name] = {}
@@ -394,7 +444,7 @@ def _generate_artifact_mappings(
394444
)
395445

396446
if s3_uri:
397-
mappings[mapping_name][compound_key] = {prop.property_name: s3_uri}
447+
mappings[mapping_name][compound_key] = {leaf_name: s3_uri}
398448
else:
399449
LOG.warning(
400450
"Could not find S3 URI for %s in expanded resource %s",
@@ -421,7 +471,7 @@ def _generate_artifact_mappings(
421471
)
422472

423473
if s3_uri:
424-
mappings[mapping_name][collection_value] = {prop.property_name: s3_uri}
474+
mappings[mapping_name][collection_value] = {leaf_name: s3_uri}
425475
else:
426476
LOG.warning(
427477
"Could not find S3 URI for %s in expanded resource %s",
@@ -465,7 +515,8 @@ def _find_artifact_uri_for_resource(
465515
Find the artifact URI for a specific resource and property from the exported resources.
466516
467517
Handles all artifact property export formats (string URIs, {S3Bucket, S3Key},
468-
{Bucket, Key}, {ImageUri}).
518+
{Bucket, Key}, {ImageUri}). ``property_name`` may be a jmespath dotted path
519+
(e.g. "Command.ScriptLocation").
469520
"""
470521
resource = exported_resources.get(resource_key)
471522
if not isinstance(resource, dict):
@@ -478,7 +529,7 @@ def _find_artifact_uri_for_resource(
478529
if not isinstance(properties, dict):
479530
return None
480531

481-
artifact_uri = properties.get(property_name)
532+
artifact_uri = _get_prop_value(properties, property_name)
482533

483534
if isinstance(artifact_uri, str):
484535
return artifact_uri
@@ -531,7 +582,8 @@ def _replace_dynamic_artifact_with_findmap(
531582
Replace a dynamic artifact property value with Fn::FindInMap reference.
532583
"""
533584
if mapping_name is None:
534-
mapping_name = f"SAM{prop.property_name}{_nesting_path(prop)}"
585+
# Use the leaf segment so dotted paths produce alphanumeric Mapping names.
586+
mapping_name = f"SAM{_leaf_prop_name(prop.property_name)}{_nesting_path(prop)}"
535587

536588
current_scope = resources
537589
if prop.outer_loops:
@@ -581,13 +633,20 @@ def _replace_dynamic_artifact_with_findmap(
581633
else:
582634
lookup_key = {"Ref": prop.loop_variable}
583635

584-
properties[prop.property_name] = {
585-
"Fn::FindInMap": [
586-
mapping_name,
587-
lookup_key,
588-
prop.property_name,
589-
]
590-
}
636+
# Address the property at its dotted location (creating intermediate dicts
637+
# if needed) and use the leaf segment as the FindInMap third argument so
638+
# the lookup matches the Mapping value-dict key.
639+
_set_prop_value(
640+
properties,
641+
prop.property_name,
642+
{
643+
"Fn::FindInMap": [
644+
mapping_name,
645+
lookup_key,
646+
_leaf_prop_name(prop.property_name),
647+
]
648+
},
649+
)
591650

592651
LOG.debug(
593652
"Replaced %s in %s/%s with Fn::FindInMap reference to %s",

0 commit comments

Comments
 (0)