refactor(cfn-lang-ext): consolidate copy helpers and relocate prop-name helpers#9017
refactor(cfn-lang-ext): consolidate copy helpers and relocate prop-name helpers#9017bnusunny 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
| # 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 |
There was a problem hiding this comment.
We should import everything at the top level.
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.
roger-zhangg
left a comment
There was a problem hiding this comment.
LGTM. Clean separation of concerns: consolidates _copy_artifact_uris_for_type / _copy_artifact_paths duplication into a single copy_artifact_properties, and promotes the underscore-private jmespath helpers into a properly public property_paths module.
Verified:
- No behavioral changes — the consolidated helper has the same logic as the union of both predecessors (optional
foreach_key/dynamic_prop_keyskwargs for the merge path, omitted for the simpler build-time path). - Tests properly adapted — existing test coverage is preserved with only the call-site renames (
_copy_artifact_uris_for_type->copy_artifact_properties), duplicate low-level tests moved to the newtest_property_paths.py, and new contract tests added (round-trip + alphanumeric-leaf assertions on every entry inPACKAGEABLE_RESOURCE_ARTIFACT_PROPERTIES). - Import sites correctly updated — all 6 inline import blocks in
build_context.pyand the top-level import insam_integration.pypoint at the new module with the public names.
One note: both this PR and #9018 are in CONFLICTING state with develop. After resolving merge conflicts with the base branch, whichever of #9017 / #9018 merges second will also need a trivial rebase against the other (they both modify import blocks in build_context.py and language_extensions_packaging.py). Despite the PR #9018 description saying merge order doesn't matter, merging #9017 first is slightly cleaner — #9018 still imports _leaf_prop_name from language_extensions_packaging, which this PR removes.
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.