refactor(cfn-lang-ext): consolidate copy helpers and relocate prop-name helpers#9017
Open
bnusunny wants to merge 1 commit into
Open
refactor(cfn-lang-ext): consolidate copy helpers and relocate prop-name helpers#9017bnusunny wants to merge 1 commit into
bnusunny wants to merge 1 commit into
Conversation
…me helpers PR #9009 (issue #9005) left two near-duplicate copy helpers in the language- extensions code: _copy_artifact_uris_for_type in samcli/lib/package/ language_extensions_packaging.py and _copy_artifact_paths in samcli/commands/ build/build_context.py. They were 90% identical and differed only in the optional dynamic_prop_keys skip-set. Bot review #1 on that PR caught the exact bug class this duplication enables — a flat-key .get() slipping into one helper while the other had been jmespath-aware. Promote the four prop-name helpers (_get_prop_value, _set_prop_value, _leaf_prop_name, _resolve_property_paths) — until now underscore-private helpers in language_extensions_packaging.py imported across packages by samcli/commands/build and samcli/lib/cfn_language_extensions/sam_integration — into a properly-named module samcli/lib/cfn_language_extensions/property_paths with no leading underscore. Add a fifth helper, copy_artifact_properties, that consolidates the two duplicated copy implementations into one. Add a contract test that asserts every entry in PACKAGEABLE_RESOURCE_ ARTIFACT_PROPERTIES round-trips through set_prop_value / get_prop_value and that every leaf is alphanumeric (a precondition for use in CloudFormation Mapping names and Fn::FindInMap third-arg keys). Future additions to the canonical packageable-resource list are now caught at the contract layer rather than at user-template execution time. Mechanical changes: - new samcli/lib/cfn_language_extensions/property_paths.py - new tests/unit/lib/cfn_language_extensions/test_property_paths.py - delete _copy_artifact_uris_for_type from samcli/lib/package/language_extensions_packaging.py; collapse its two in-file callers (_copy_artifact_uris and _update_foreach_with_s3_uris) to call copy_artifact_properties directly - delete BuildContext._copy_artifact_paths from samcli/commands/build/ build_context.py; inline the single call site - update inline imports in build_context.py and sam_integration.py to point at the new module - rename test_copy_artifact_uris_for_type_* to test_copy_artifact_properties_* in tests/unit/commands/package/test_package_context.py and update function-import sites in three test files - delete duplicate get_prop_value / set_prop_value tests from test_package_context.py (now in test_property_paths.py) Verification: - baseline 9191 passed, 25 skipped → after 9203 passed, 25 skipped (+12) - 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
8 tasks
reedham-aws
reviewed
May 15, 2026
| # Regular resource - copy updated paths from modified template | ||
| modified_resource = modified_resources.get(resource_key, {}) | ||
| self._copy_artifact_paths(resource_value, modified_resource) | ||
| from samcli.lib.cfn_language_extensions.property_paths import copy_artifact_properties |
Contributor
There was a problem hiding this comment.
We should import everything at the top level.
Contributor
There was a problem hiding this comment.
I guess that's not the pattern that is already being used here, though. We can just keep it like this then, although I think it should be changed.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Follow-up to #9009 (issue #9005). Closes a structural divergence the bot reviews on that PR repeatedly caught: two near-duplicate copy helpers — `_copy_artifact_uris_for_type` in `samcli/lib/package/language_extensions_packaging.py` and `_copy_artifact_paths` in `samcli/commands/build/build_context.py` — that differed only in their optional `dynamic_prop_keys` skip-set. The first round of bot review on #9009 caught the exact bug class this duplication enables: a flat-`.get()` slipping into one helper while the other had been jmespath-aware.
Changes
What's NOT in this PR
Test plan
Why now
PR #9009 went through three rounds of bot review, each catching a real silent-corruption bug:
Bugs 2 and 3 were structural: two separate hand-maintained lists that needed to track the canonical packageable-property registry. The fixes for those — deriving the prefix list from the canonical dict, keying collision detection on the leaf — landed in #9009 itself. Bug 1's fix (route `_copy_artifact_paths` through the new jmespath helpers) also landed in #9009, but the duplication between the two copy helpers remained, leaving the failure mode latent. This PR closes that.