fix: do not mutate the caller's template during transform - #3974
Conversation
`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>
| # 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.
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.