fix: do not mutate the caller's template during transform - #3974
fix: do not mutate the caller's template during transform#3974Adityaj0 wants to merge 2 commits into
Conversation
![aws-sam-tooling-bot[bot]](https://avatars.githubusercontent.com/b/1290?s=60&v=4){kind=small}
| # dicts and strings are copied. The Py27Dict/Py27UniStr wrappers that function | ||
| # installs carry hash-ordering state that logical ID generation depends on, and | ||
| # deep-copying them does not preserve it. | ||
| input_fragment = copy.deepcopy(input_fragment) |
There was a problem hiding this comment.
[BUG] The copy fixes the template, but the very next line still mutates the caller's other input. to_py27_compatible_template rewrites parameter_values in place (py27hash_fix.py):
if parameter_values:
for key, val in parameter_values.items():
parameter_values[key] = convertto_py27_type(val)So after transform(template, params, loader) returns, the caller's params dict has had its values replaced with Py27UniStr/Py27Dict/Py27LongInt wrappers. This is caller-visible: Py27UniStr.__repr__ renders as u'value', and Py27Dict iterates in Python 2 hash order rather than insertion order, so a caller that logs or iterates its own parameter values after transforming sees different results than before the call.
Note that Translator.translate does not have this problem — SamParameterValues.__init__ already does copy.deepcopy(parameter_values) — so transform() is the only place the caller's dict is touched, and copying it at the same boundary is consistent with what the rest of the stack already does:
input_fragment = copy.deepcopy(input_fragment)
parameter_values = copy.deepcopy(parameter_values)The new tests all pass {} as parameter_values, so this path is not covered; a case with a non-empty parameter value would catch it.
`Translator.translate()` documents that it returns "a copy of the
template with SAM resources replaced", but the parser and plugins edit
the template in place: the `Globals` section is deleted from it, merged
global properties are written into resource properties, and generated
API definition bodies are written into explicit
`AWS::Serverless::Api`/`HttpApi` resources.
Callers that keep the template around, or transform it more than once,
see the damage. A second transform of the same object fails outright:
InvalidDocumentException: Event with id [Api] is invalid.
API method "get" defined multiple times for path "/x".
Take a copy at the `transform()` boundary. The copy is deliberately
taken *before* `to_py27_compatible_template()` runs, so that only plain
dicts and strings are copied: the Py27Dict/Py27UniStr wrappers that
function installs carry hash-ordering state that logical ID generation
depends on, and deep-copying them does not preserve it. Copying after
that point changes generated logical IDs.
On a 246 KiB, 200-function template the copy costs 3.2ms against a
1924ms transform -- 0.2%.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
8715820 to
d8997eb
Compare
|
Good catch — fixed. `transform()` now also deep-copies `parameter_values` before `to_py27_compatible_template()`, at the same boundary as `input_fragment`, matching what `SamParameterValues.init` already does for `Translator.translate()`. Added `test_parameter_values_are_not_modified`, which passes a non-empty `parameter_values` dict (str, int, and nested dict values) through a template with an explicit API resource (the only path that exercises the mutation) and asserts both value equality and exact type (`Py27UniStr` subclasses `str`, so a plain `==` check wouldn't have caught this). Confirmed it fails on the previous commit and passes now. Full suite: `tests/translator tests/plugins tests/parser` — 2578 passed (same 5 pre-existing, unrelated SAR-timing failures as before). |
|
|
||
| def _managed_policy_loader(): | ||
| loader = MagicMock() | ||
| loader.load.return_value = {"AWSLambdaBasicExecutionRole": "arn:aws:iam::aws:policy/service-role/AWSLambdaBasicExecutionRole"} |
There was a problem hiding this comment.
[GENERAL] This line is 130 characters. pyproject.toml sets black's line-length = 120, and CI runs make pr (.github/workflows/build.yml:34), whose format-check target runs black --check setup.py samtranslator tests integration bin schema_source. Black will want to split this dict literal, so --check exits non-zero and the build fails before the new tests ever run.
def managedpolicy_loader():
loader = MagicMock()
loader.load.return_value = {
"AWSLambdaBasicExecutionRole": "arn:aws:iam::aws:policy/service-role/AWSLambdaBasicExecutionRole"
}
return loaderNote on the earlier review comment about parameter_values: I confirmed to_py27_compatible_template does mutate it in place (samtranslator/utils/py27hash_fix.py:94-96), and the diff now adds parameter_values = copy.deepcopy(parameter_values) alongside the template copy, so that issue is resolved and not re-raised. I also checked the remaining caller-owned input, managed_policy_loader: the map returned by get_managed_policy_map() is only read (role_constructor.py:35), so no third mutation path remains in transform().
|
Addressed the formatting comment on |
|
|
||
| # The generated DefinitionBody used to be written into the caller's | ||
| # AWS::Serverless::Api resource. | ||
| self.assertEqual(template, expected) |
There was a problem hiding this comment.
[GENERAL] The template tests assert value equality only, which cannot detect the wrapper-type substitution that to_py27_compatible_template() performs on the caller's template. Py27Dict subclasses dict and Py27UniStr subclasses str without overriding eq, so assertEqual(template, expected) passes even when template["Resources"] has been replaced by a Py27Dict and its keys by Py27UniStr (py27hash_fix.py, to_py27_compatible_template).
This matters for the exact alternative implementation the PR description discusses and rejects: if the input_fragment copy is ever moved to after to_py27_compatible_template() (or into translate()), the caller's template still gets Resources/Parameters/API Properties swapped for Py27Dict — caller-visible via a different repr and Python 2 hash-order iteration — and this suite would not fail. test_parameter_values_are_not_modified already guards against precisely this with explicit type checks; the template tests lack the equivalent.
def test_explicit_api_template_is_not_modified(self):
template = copy.deepcopy(TEMPLATE_WITH_EXPLICIT_API)
expected = copy.deepcopy(template)
transform(template)
# The generated DefinitionBody used to be written into the caller's
# AWS::Serverless::Api resource.
self.assertEqual(template, expected)
# Py27Dict/Py27UniStr compare equal to dict/str, so assert the types too.
self.assertIs(type(template["Resources"]), dict)
self.assertIs(type(template["Resources"]["Api"]["Properties"]), dict)1 participant
Issue #, if available
#3973
Description of changes
Translator.translate()documents that it returns "a copy of the template", but the parser and plugins edit the template in place:Globalsis deleted from it, merged global properties are written into resource properties, and generated API definition bodies are written into explicitAWS::Serverless::Api/HttpApiresources. A caller that transforms the same template twice getsInvalidDocumentException: API method "get" defined multiple times for path "/x"on the second call.This takes a copy at the
transform()boundary.The copy is taken before
to_py27_compatible_template()on purpose. That function installsPy27Dict/Py27UniStrwrappers whose hash-ordering state logical ID generation depends on, andcopy.deepcopydoes not preserve it — putting the copy at the top oftranslate()instead changes generated logical IDs, whichtest_transform_feature_toggle_0_feature_toggle_api_open_api_version_overridecatches. Copying while the template is still plain dicts and strings avoids that.Scope: this fixes the public
transform()entry point.Translator.translate()called directly still mutates its argument; correcting that needs__deepcopy__support on the Py27 types, which I've left alone.Description of how you validated changes
Four tests in
tests/translator/test_transform_does_not_mutate_input.py, one per mutation path (Globals, implicit API event, explicitApi, and the double-transform failure for both REST and HTTP APIs). All four fail ondevelopand pass with this change.Existing suites:
tests/translator2174 passed, andtests/translator tests/plugins tests/parser2577 passed. Five failures intests/plugins/application/test_serverless_app_plugin.pyare pre-existing ondevelop(timing-based SAR tests) and fail identically without this change.Overhead of the copy on a 246 KiB, 200-function template: 3.2 ms against a 1924 ms transform, 0.2%.
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.