Skip to content

Commit fd0bedd

Browse files
committed
fix: share Fn::ForEach and SAM-mapping helpers across modules
Two issues flagged by the bot reviewer on PR #8637: [BUG] _update_sam_mappings_relative_paths in samcli/commands/_utils/ template.py used mapping_name.startswith("SAM") to decide whether to rewrite Mapping values as relative paths. That loose check corrupts customer-authored mappings whose names happen to start with SAM as a substring — SAMPLE, SAMSUNG, SAMCustomMapping, etc. Values would be silently rewritten with relative-path prefixes. [STYLE] infra_sync_executor.py had two hardcoded resource_logical_id.startswith("Fn::ForEach::") checks instead of using the shared is_foreach_key helper the rest of the codebase adopted in this PR. Maintenance risk if the detection logic ever changes. Consolidation: - Moved the precise SAM-mapping classifier (previously _is_sam_generated_mapping in samcli/commands/deploy/exceptions.py) into samcli/lib/cfn_language_extensions/utils.py as the public is_sam_generated_mapping so both deploy/exceptions.py and commands/_utils/template.py share the same implementation. - _update_sam_mappings_relative_paths now calls is_sam_generated_mapping — SAMPLE/SAMSUNG style names are correctly ignored. - infra_sync_executor.py now calls is_foreach_key at both sites. Tests: - test_skips_sam_prefix_substring_mappings in TestUpdateSamMappingsRelativePaths verifies SAMPLE/SAMSUNG/ SAMCustomMapping are left untouched. - Existing TestIsSamGeneratedMapping and TestCreateDeployError still pass via the re-export in deploy/exceptions.py.
1 parent e016af8 commit fd0bedd

7 files changed

Lines changed: 78 additions & 56 deletions

File tree

samcli/commands/_utils/template.py

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,11 @@
1111
from botocore.utils import set_value_from_jmespath
1212

1313
from samcli.commands.exceptions import UserException
14-
from samcli.lib.cfn_language_extensions.utils import FOREACH_REQUIRED_ELEMENTS, is_foreach_key
14+
from samcli.lib.cfn_language_extensions.utils import (
15+
FOREACH_REQUIRED_ELEMENTS,
16+
is_foreach_key,
17+
is_sam_generated_mapping,
18+
)
1519
from samcli.lib.samlib.resource_metadata_normalizer import ASSET_PATH_METADATA_KEY, ResourceMetadataNormalizer
1620
from samcli.lib.utils import graphql_api
1721
from samcli.lib.utils.packagetype import IMAGE, ZIP
@@ -303,8 +307,11 @@ def _update_sam_mappings_relative_paths(mappings, original_root, new_root):
303307
return
304308

305309
for mapping_name, mapping_entries in mappings.items():
306-
# Only process SAM-generated Mappings (prefixed with "SAM")
307-
if not mapping_name.startswith("SAM"):
310+
# Only process SAM-generated Mappings (known PascalCase prefixes like
311+
# SAMCodeUri…, SAMImageUri…, SAMLayers…). Skipping with a loose
312+
# startswith("SAM") check would corrupt customer mappings that happen
313+
# to share the prefix as a substring (SAMPLE, SAMSUNG, etc.).
314+
if not is_sam_generated_mapping(mapping_name):
308315
continue
309316

310317
if not isinstance(mapping_entries, dict):

samcli/commands/deploy/exceptions.py

Lines changed: 0 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -20,48 +20,6 @@
2020
r"(?:'([^']+)'|\"([^\"]+)\"|(\S+))"
2121
)
2222

23-
# Mapping-name prefixes that SAM CLI emits for dynamic Fn::ForEach handling:
24-
# - SAM + <packageable artifact property> + <nesting path> + [resource suffix]
25-
# - SAMLayers + <nesting path> (auto dependency layer references)
26-
# See language_extensions_packaging._compute_mapping_name and build_context.
27-
# Customer-authored mappings should never start with one of these exact
28-
# PascalCase prefixes, so exact-prefix matching avoids the false positives
29-
# a regex like r"^SAM[A-Z]..." would hit on names like "SAMPLE" / "SAMSUNG".
30-
_SAM_GENERATED_MAPPING_PREFIXES: Tuple[str, ...] = (
31-
"SAMCodeUri",
32-
"SAMImageUri",
33-
"SAMContentUri",
34-
"SAMDefinitionUri",
35-
"SAMSchemaUri",
36-
"SAMBodyS3Location",
37-
"SAMDefinitionS3Location",
38-
"SAMTemplateURL",
39-
"SAMCode",
40-
"SAMContent",
41-
"SAMLayers",
42-
)
43-
44-
45-
def _is_sam_generated_mapping(mapping_name: str) -> bool:
46-
"""Check whether *mapping_name* matches the naming scheme SAM CLI uses for
47-
Mappings emitted during sam build / sam package for dynamic Fn::ForEach
48-
artifact properties. See language_extensions_packaging._compute_mapping_name.
49-
"""
50-
if not mapping_name:
51-
return False
52-
for prefix in _SAM_GENERATED_MAPPING_PREFIXES:
53-
if mapping_name == prefix:
54-
# Bare prefix with no nesting path isn't a real SAM-generated name.
55-
return False
56-
if mapping_name.startswith(prefix):
57-
# The next character must start a new PascalCase segment (upper-case
58-
# letter). Digits or lower-case letters here indicate a user name
59-
# that happens to share the prefix.
60-
nxt = mapping_name[len(prefix)]
61-
if "A" <= nxt <= "Z":
62-
return True
63-
return False
64-
6523

6624
def parse_findmap_error(error_message: str) -> Optional[Tuple[str, str]]:
6725
"""

samcli/lib/cfn_language_extensions/utils.py

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,50 @@ def is_foreach_key(key: str) -> bool:
4848
return isinstance(key, str) and key.startswith(FOREACH_PREFIX)
4949

5050

51+
# Mapping-name prefixes that SAM CLI emits for dynamic Fn::ForEach handling:
52+
# - SAM + <packageable artifact property> + <nesting path> + [resource suffix]
53+
# (see language_extensions_packaging._compute_mapping_name)
54+
# - SAMLayers + <nesting path> (see build_context — auto dependency layer refs)
55+
# Customer-authored mappings should never start with one of these exact
56+
# PascalCase prefixes, so exact-prefix matching avoids the false positives
57+
# a regex like r"^SAM[A-Z]..." would hit on names like SAMPLE / SAMSUNG.
58+
_SAM_GENERATED_MAPPING_PREFIXES: Tuple[str, ...] = (
59+
"SAMCodeUri",
60+
"SAMImageUri",
61+
"SAMContentUri",
62+
"SAMDefinitionUri",
63+
"SAMSchemaUri",
64+
"SAMBodyS3Location",
65+
"SAMDefinitionS3Location",
66+
"SAMTemplateURL",
67+
"SAMCode",
68+
"SAMContent",
69+
"SAMLayers",
70+
)
71+
72+
73+
def is_sam_generated_mapping(mapping_name: str) -> bool:
74+
"""Return True if *mapping_name* matches the naming scheme SAM CLI uses
75+
for Mappings emitted during sam build / sam package for dynamic
76+
Fn::ForEach artifact properties.
77+
78+
Customer-authored mappings that happen to start with "SAM" as a substring
79+
(e.g. SAMPLE, SAMSUNG) will NOT match because the next character after the
80+
prefix must begin a new PascalCase segment (upper-case letter).
81+
"""
82+
if not mapping_name:
83+
return False
84+
for prefix in _SAM_GENERATED_MAPPING_PREFIXES:
85+
if mapping_name == prefix:
86+
# Bare prefix with no nesting path isn't a real SAM-generated name.
87+
return False
88+
if mapping_name.startswith(prefix):
89+
nxt = mapping_name[len(prefix)]
90+
if "A" <= nxt <= "Z":
91+
return True
92+
return False
93+
94+
5195
def iter_regular_resources(template_dict: Dict) -> Iterator[Tuple[str, Dict]]:
5296
"""
5397
Yield (logical_id, resource_dict) pairs from a template's Resources section,

samcli/lib/deploy/deployer.py

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -34,9 +34,9 @@
3434
DeployStackOutPutFailedError,
3535
DeployStackStatusMissingError,
3636
MissingMappingKeyError,
37-
_is_sam_generated_mapping,
3837
parse_findmap_error,
3938
)
39+
from samcli.lib.cfn_language_extensions.utils import is_sam_generated_mapping
4040
from samcli.lib.deploy.utils import DeployColor, FailureMode
4141
from samcli.lib.package.local_files_utils import get_uploaded_s3_object_name, mktempfile
4242
from samcli.lib.package.s3_uploader import S3Uploader
@@ -123,7 +123,7 @@ def _create_deploy_error(stack_name: str, error_message: str) -> Exception:
123123
# failures (e.g. a RegionMap lookup) must pass through with the
124124
# raw CloudFormation message — the SAM-specific re-package guidance
125125
# would be misleading otherwise.
126-
if _is_sam_generated_mapping(mapping_name):
126+
if is_sam_generated_mapping(mapping_name):
127127
return MissingMappingKeyError(
128128
stack_name=stack_name,
129129
missing_key=missing_key,

samcli/lib/sync/infra_sync_executor.py

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717
from samcli.commands.build.build_context import BuildContext
1818
from samcli.commands.deploy.deploy_context import DeployContext
1919
from samcli.commands.package.package_context import PackageContext
20+
from samcli.lib.cfn_language_extensions.utils import is_foreach_key
2021
from samcli.lib.providers.provider import ResourceIdentifier
2122
from samcli.lib.providers.sam_stack_provider import is_local_path
2223
from samcli.lib.telemetry.event import EventTracker
@@ -284,7 +285,7 @@ def _auto_skip_infra_sync(
284285

285286
# The recursive template check for Nested stacks
286287
for resource_logical_id in current_template.get("Resources", {}):
287-
if resource_logical_id.startswith("Fn::ForEach::"):
288+
if is_foreach_key(resource_logical_id):
288289
continue
289290

290291
resource_dict = current_template.get("Resources", {}).get(resource_logical_id, {})
@@ -397,7 +398,7 @@ def _sanitize_template(
397398
built_resource_dict = None
398399

399400
for resource_logical_id in resources:
400-
if resource_logical_id.startswith("Fn::ForEach::"):
401+
if is_foreach_key(resource_logical_id):
401402
continue
402403

403404
resource_dict = resources.get(resource_logical_id, {})

tests/unit/commands/_utils/test_template_language_extensions.py

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -118,6 +118,18 @@ def test_skips_non_sam_mappings(self):
118118
# Should be unchanged
119119
self.assertEqual(mappings["RegionMap"]["us-east-1"]["AMI"], "ami-12345")
120120

121+
def test_skips_sam_prefix_substring_mappings(self):
122+
"""User-authored mappings like SAMPLE / SAMSUNG must not be treated as SAM-generated."""
123+
mappings = {
124+
"SAMPLE": {"key1": {"CodeUri": "services/users"}},
125+
"SAMSUNG": {"key2": {"CodeUri": "services/orders"}},
126+
"SAMCustomMapping": {"key3": {"CodeUri": "services/products"}},
127+
}
128+
_update_sam_mappings_relative_paths(mappings, "/old", "/new")
129+
self.assertEqual(mappings["SAMPLE"]["key1"]["CodeUri"], "services/users")
130+
self.assertEqual(mappings["SAMSUNG"]["key2"]["CodeUri"], "services/orders")
131+
self.assertEqual(mappings["SAMCustomMapping"]["key3"]["CodeUri"], "services/products")
132+
121133
def test_skips_non_dict_mapping_entries(self):
122134
mappings = {"SAMCodeUriFunctions": "not a dict"}
123135
_update_sam_mappings_relative_paths(mappings, "/old", "/new")

tests/unit/commands/deploy/test_exceptions.py

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -160,7 +160,7 @@ def test_missing_mapping_key_error_explains_foreach_constraint(self):
160160

161161

162162
class TestIsSamGeneratedMapping(TestCase):
163-
"""Tests for the _is_sam_generated_mapping classifier.
163+
"""Tests for the is_sam_generated_mapping classifier.
164164
165165
The classifier gates whether Deployer wraps a CloudFormation
166166
Fn::FindInMap failure as MissingMappingKeyError (which emits
@@ -170,7 +170,7 @@ class TestIsSamGeneratedMapping(TestCase):
170170
"""
171171

172172
def test_sam_generated_names_match(self):
173-
from samcli.commands.deploy.exceptions import _is_sam_generated_mapping
173+
from samcli.lib.cfn_language_extensions.utils import is_sam_generated_mapping
174174

175175
for name in (
176176
"SAMCodeUriServices",
@@ -179,10 +179,10 @@ def test_sam_generated_names_match(self):
179179
"SAMImageUriFunctionsApi",
180180
):
181181
with self.subTest(name=name):
182-
self.assertTrue(_is_sam_generated_mapping(name))
182+
self.assertTrue(is_sam_generated_mapping(name))
183183

184184
def test_user_mapping_names_do_not_match(self):
185-
from samcli.commands.deploy.exceptions import _is_sam_generated_mapping
185+
from samcli.lib.cfn_language_extensions.utils import is_sam_generated_mapping
186186

187187
for name in (
188188
"RegionMap",
@@ -200,9 +200,9 @@ def test_user_mapping_names_do_not_match(self):
200200
"",
201201
):
202202
with self.subTest(name=name):
203-
self.assertFalse(_is_sam_generated_mapping(name))
203+
self.assertFalse(is_sam_generated_mapping(name))
204204

205205
def test_none_input_is_false(self):
206-
from samcli.commands.deploy.exceptions import _is_sam_generated_mapping
206+
from samcli.lib.cfn_language_extensions.utils import is_sam_generated_mapping
207207

208-
self.assertFalse(_is_sam_generated_mapping(None))
208+
self.assertFalse(is_sam_generated_mapping(None))

0 commit comments

Comments
 (0)