From 77bc0959d4df57be70463dcb227836c6a46fbf78 Mon Sep 17 00:00:00 2001 From: Joe Wakefield Date: Tue, 4 Feb 2025 22:11:45 -0500 Subject: [PATCH 01/16] Support file:// for parameter-overrides --- samcli/cli/types.py | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/samcli/cli/types.py b/samcli/cli/types.py index a3743128ade..137a8699504 100644 --- a/samcli/cli/types.py +++ b/samcli/cli/types.py @@ -6,10 +6,12 @@ import logging import re from json import JSONDecodeError +from pathlib import Path from typing import Dict, List, Optional, Union import click +from samcli.lib.config.file_manager import FILE_MANAGER_MAPPER from samcli.lib.package.ecr_utils import is_ecr_url PARAM_AND_METADATA_KEY_REGEX = """([A-Za-z0-9\\"\']+)""" @@ -97,6 +99,19 @@ def convert(self, value, param, ctx): value = (value,) if isinstance(value, str) else value for val in value: + if val.startswith('file://'): + filepath = Path(val[7:]) + if not filepath.is_file(): + return self.fail(f"{val} was not found or is a directory", param, ctx) + file_manager = FILE_MANAGER_MAPPER.get(filepath.suffix, None) + if not file_manager: + return self.fail(f"{val} uses an unsupported extension", param, ctx) + try: + parameters = file_manager.read(filepath) + except Exception as e: + return self.fail(str(e), param, ctx) + result |= parameters + continue # Add empty string to start of the string to help match `_pattern2` normalized_val = " " + val.strip() From 18d6d40feb9729f960274e293b4f30a0904d65af Mon Sep 17 00:00:00 2001 From: Joe Wakefield Date: Wed, 5 Feb 2025 11:41:27 -0500 Subject: [PATCH 02/16] Flatten nested lists and normalize parameters --- samcli/cli/types.py | 107 ++++++++++++++++++++++++++------------------ 1 file changed, 64 insertions(+), 43 deletions(-) diff --git a/samcli/cli/types.py b/samcli/cli/types.py index 137a8699504..ab22a03a2e3 100644 --- a/samcli/cli/types.py +++ b/samcli/cli/types.py @@ -10,6 +10,9 @@ from typing import Dict, List, Optional, Union import click +import ruamel.yaml + +from ruamel.yaml.comments import CommentedMap, CommentedSeq from samcli.lib.config.file_manager import FILE_MANAGER_MAPPER from samcli.lib.package.ecr_utils import is_ecr_url @@ -62,6 +65,24 @@ def _unquote_wrapped_quotes(value): return value.replace("\\ ", " ").replace('\\"', '"').replace("\\'", "'") +def _format_value(value): + if isinstance(value, bool): + return str(value).lower() + if isinstance(value, int): + return str(value) + if isinstance(value, list): + return f'{",".join(map(str, value))}' + return f'{value}' + +def _flatten_list(data): + flat_data = [] + for item in data: + if isinstance(item, list): + flat_data.extend(_flatten_list(item)) + else: + flat_data.append(item) + return flat_data + class CfnParameterOverridesType(click.ParamType): """ @@ -90,56 +111,56 @@ class CfnParameterOverridesType(click.ParamType): name = "string,list" - def convert(self, value, param, ctx): + def convert(self, values, param, ctx): result = {} # Empty tuple - if value == ("",): + if values == ("",): return result - value = (value,) if isinstance(value, str) else value - for val in value: - if val.startswith('file://'): - filepath = Path(val[7:]) - if not filepath.is_file(): - return self.fail(f"{val} was not found or is a directory", param, ctx) - file_manager = FILE_MANAGER_MAPPER.get(filepath.suffix, None) - if not file_manager: - return self.fail(f"{val} uses an unsupported extension", param, ctx) - try: - parameters = file_manager.read(filepath) - except Exception as e: - return self.fail(str(e), param, ctx) - result |= parameters - continue - # Add empty string to start of the string to help match `_pattern2` - normalized_val = " " + val.strip() + LOG.debug("Input parameters: %s", values) + + # Flatten to support YAML anchors & aliases + values = _flatten_list(values) if isinstance(values, (list, CommentedSeq)) else [values,] + LOG.debug("Flattened parameters: %s", values) + + parameters = {} + for value in values: + if isinstance(value, str): + if value.startswith('file://'): + filepath = Path(value[7:]) + if not filepath.is_file(): + self.fail(f"{value} was not found or is a directory", param, ctx) + file_manager = FILE_MANAGER_MAPPER.get(filepath.suffix, None) + if not file_manager: + self.fail(f"{value} uses an unsupported extension", param, ctx) + parameters |= self.convert(file_manager.read(filepath), param, ctx) + else: + # Legacy parameter matching + normalized_value = " " + value.strip() + for pattern in self.ordered_pattern_match: + groups = re.findall(pattern, normalized_value) + if groups: + # Group tuples can be merged into dict + parameters |= groups + break + else: + self.fail( + f"{value} is not in valid format. It must look something like '{self.__EXAMPLE_1}' or '{self.__EXAMPLE_2}'", + param, + ctx, + ) + + elif isinstance(value, CommentedMap) or isinstance(value, dict): + # e.g. YAML key-value pairs + for k, v in value.items(): + parameters[k] = _format_value(v) - try: - # NOTE(TheSriram): find the first regex that matched. - # pylint is concerned that we are checking at the same `val` within the loop, - # but that is the point, so disabling it. - pattern = next( - i - for i in filter( - lambda item: re.findall(item, normalized_val), self.ordered_pattern_match - ) # pylint: disable=cell-var-from-loop - ) - except StopIteration: - return self.fail( - "{} is not in valid format. It must look something like '{}' or '{}'".format( - val, self.__EXAMPLE_1, self.__EXAMPLE_2 - ), - param, - ctx, - ) - - groups = re.findall(pattern, normalized_val) - - # 'groups' variable is a list of tuples ex: [(key1, value1), (key2, value2)] - for key, param_value in groups: - result[_unquote_wrapped_quotes(key)] = _unquote_wrapped_quotes(param_value) + result = {} + for key, param_value in parameters.items(): + result[_unquote_wrapped_quotes(key)] = _unquote_wrapped_quotes(param_value) + LOG.debug("Output parameters: %s", result) return result From 42b5bc60f3fbaf7cedfa44d02cd6adfff52f2514 Mon Sep 17 00:00:00 2001 From: Joe Wakefield Date: Wed, 5 Feb 2025 12:13:47 -0500 Subject: [PATCH 03/16] handle empty param files and non-string yaml keys --- samcli/cli/types.py | 29 +++++++++++++++++------------ 1 file changed, 17 insertions(+), 12 deletions(-) diff --git a/samcli/cli/types.py b/samcli/cli/types.py index ab22a03a2e3..71de82efc56 100644 --- a/samcli/cli/types.py +++ b/samcli/cli/types.py @@ -65,15 +65,6 @@ def _unquote_wrapped_quotes(value): return value.replace("\\ ", " ").replace('\\"', '"').replace("\\'", "'") -def _format_value(value): - if isinstance(value, bool): - return str(value).lower() - if isinstance(value, int): - return str(value) - if isinstance(value, list): - return f'{",".join(map(str, value))}' - return f'{value}' - def _flatten_list(data): flat_data = [] for item in data: @@ -114,8 +105,9 @@ class CfnParameterOverridesType(click.ParamType): def convert(self, values, param, ctx): result = {} - # Empty tuple - if values == ("",): + # Empty tuple or empty param file + if values == ("",) or values == "" or values is None: + LOG.debug("Empty parameter set (%s)", values) return result LOG.debug("Input parameters: %s", values) @@ -126,6 +118,8 @@ def convert(self, values, param, ctx): parameters = {} for value in values: + LOG.debug("Processing parameter: %s", value) + if isinstance(value, str): if value.startswith('file://'): filepath = Path(value[7:]) @@ -154,9 +148,20 @@ def convert(self, values, param, ctx): elif isinstance(value, CommentedMap) or isinstance(value, dict): # e.g. YAML key-value pairs for k, v in value.items(): - parameters[k] = _format_value(v) + if isinstance(value, list): + parameters[str(k)] = ",".join(map(str, value)) # Collapse lists to csv + else: + parameters[str(k)] = str(v) + else: + self.fail( + "This error should never appear.", + param, + ctx, + ) result = {} + LOG.debug("Pre-unwrap parameters: %s", parameters) + for key, param_value in parameters.items(): result[_unquote_wrapped_quotes(key)] = _unquote_wrapped_quotes(param_value) From be1a66317c6858a015f191ad80e54648f4a71d1c Mon Sep 17 00:00:00 2001 From: Joe Wakefield Date: Wed, 5 Feb 2025 12:46:37 -0500 Subject: [PATCH 04/16] fix list collapse to comma delimited --- samcli/cli/types.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/samcli/cli/types.py b/samcli/cli/types.py index 71de82efc56..e6fb532dd7d 100644 --- a/samcli/cli/types.py +++ b/samcli/cli/types.py @@ -145,11 +145,11 @@ def convert(self, values, param, ctx): ctx, ) - elif isinstance(value, CommentedMap) or isinstance(value, dict): + elif isinstance(value, (dict, CommentedMap)): # e.g. YAML key-value pairs for k, v in value.items(): - if isinstance(value, list): - parameters[str(k)] = ",".join(map(str, value)) # Collapse lists to csv + if isinstance(v, (list, CommentedSeq)): + parameters[str(k)] = ",".join(map(str, v)) # Collapse lists to comma delimited else: parameters[str(k)] = str(v) else: From abf99263794893a118dd7e86fd4fbc80ea6c27f4 Mon Sep 17 00:00:00 2001 From: Joe Wakefield Date: Wed, 5 Feb 2025 13:16:49 -0500 Subject: [PATCH 05/16] handle tuples from cli parameter-overrides --- samcli/cli/types.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/samcli/cli/types.py b/samcli/cli/types.py index e6fb532dd7d..921ecf75eed 100644 --- a/samcli/cli/types.py +++ b/samcli/cli/types.py @@ -113,7 +113,7 @@ def convert(self, values, param, ctx): LOG.debug("Input parameters: %s", values) # Flatten to support YAML anchors & aliases - values = _flatten_list(values) if isinstance(values, (list, CommentedSeq)) else [values,] + values = _flatten_list(values) if isinstance(values, (tuple, list, CommentedSeq)) else (values,) LOG.debug("Flattened parameters: %s", values) parameters = {} From 27f03caa768f4599e78f00adac7cddfd7ef300d6 Mon Sep 17 00:00:00 2001 From: Joe Wakefield Date: Thu, 6 Feb 2025 10:13:58 -0500 Subject: [PATCH 06/16] Merge samconfig and cli parameter overrides --- samcli/cli/types.py | 68 ++++++++++++++++++++++++++++----------------- 1 file changed, 43 insertions(+), 25 deletions(-) diff --git a/samcli/cli/types.py b/samcli/cli/types.py index 921ecf75eed..71801c04b65 100644 --- a/samcli/cli/types.py +++ b/samcli/cli/types.py @@ -10,7 +10,6 @@ from typing import Dict, List, Optional, Union import click -import ruamel.yaml from ruamel.yaml.comments import CommentedMap, CommentedSeq @@ -65,10 +64,23 @@ def _unquote_wrapped_quotes(value): return value.replace("\\ ", " ").replace('\\"', '"').replace("\\'", "'") -def _flatten_list(data): +def _flatten_list(data: list) -> list: + """ + Recursively flattens lists and other list-like types for easy sequential processing. + This also helps with lists combined with YAML anchors & aliases. + + Parameters + ---------- + value : data + List to flatten + + Returns + ------- + Flattened list + """ flat_data = [] for item in data: - if isinstance(item, list): + if isinstance(item, (tuple, list, CommentedSeq)): flat_data.extend(_flatten_list(item)) else: flat_data.append(item) @@ -83,6 +95,8 @@ class CfnParameterOverridesType(click.ParamType): __EXAMPLE_1 = "ParameterKey=KeyPairName,ParameterValue=MyKey ParameterKey=InstanceType,ParameterValue=t1.micro" __EXAMPLE_2 = "KeyPairName=MyKey InstanceType=t1.micro" + __EXAMPLE_3 = "file://MyParams.yaml" + # Regex that parses CloudFormation parameter key-value pairs: # https://regex101.com/r/xqfSjW/2 @@ -100,26 +114,21 @@ class CfnParameterOverridesType(click.ParamType): ordered_pattern_match = [_pattern_1, _pattern_2] - name = "string,list" - - def convert(self, values, param, ctx): - result = {} - - # Empty tuple or empty param file - if values == ("",) or values == "" or values is None: + def _normalize_parameters(self, values, param, ctx): + """ + Normalizes parameter overrides into key-value pairs of strings + Later keys overwrite previous ones in case of key conflict + """ + if values in (("",), "", None) or values == {}: LOG.debug("Empty parameter set (%s)", values) - return result + return {} LOG.debug("Input parameters: %s", values) - - # Flatten to support YAML anchors & aliases - values = _flatten_list(values) if isinstance(values, (tuple, list, CommentedSeq)) else (values,) + values = _flatten_list([values]) LOG.debug("Flattened parameters: %s", values) parameters = {} for value in values: - LOG.debug("Processing parameter: %s", value) - if isinstance(value, str): if value.startswith('file://'): filepath = Path(value[7:]) @@ -128,47 +137,56 @@ def convert(self, values, param, ctx): file_manager = FILE_MANAGER_MAPPER.get(filepath.suffix, None) if not file_manager: self.fail(f"{value} uses an unsupported extension", param, ctx) - parameters |= self.convert(file_manager.read(filepath), param, ctx) + parameters |= self._normalize_parameters(file_manager.read(filepath), param, ctx) else: # Legacy parameter matching normalized_value = " " + value.strip() for pattern in self.ordered_pattern_match: groups = re.findall(pattern, normalized_value) if groups: - # Group tuples can be merged into dict parameters |= groups break else: self.fail( - f"{value} is not in valid format. It must look something like '{self.__EXAMPLE_1}' or '{self.__EXAMPLE_2}'", + f"{value} is not a valid format. It must look something like '{self.__EXAMPLE_1}', '{self.__EXAMPLE_2}', or '{self.__EXAMPLE_3}'", param, ctx, ) - elif isinstance(value, (dict, CommentedMap)): # e.g. YAML key-value pairs for k, v in value.items(): if isinstance(v, (list, CommentedSeq)): - parameters[str(k)] = ",".join(map(str, v)) # Collapse lists to comma delimited + # Collapse list values to comma delimited + parameters[str(k)] = ",".join(map(str, v)) else: parameters[str(k)] = str(v) else: self.fail( - "This error should never appear.", + f"{value} is not valid in a way the code doesn't expect", param, ctx, ) result = {} - LOG.debug("Pre-unwrap parameters: %s", parameters) - for key, param_value in parameters.items(): result[_unquote_wrapped_quotes(key)] = _unquote_wrapped_quotes(param_value) - LOG.debug("Output parameters: %s", result) return result + def convert(self, values, param, ctx): + # Merge samconfig with CLI parameter-overrides + if isinstance(values, tuple): # Tuple implies CLI + # default_map was populated from samconfig + default_map = ctx.default_map.get('parameter_overrides', {}) + LOG.debug("Default map: %s", default_map) + LOG.debug("Current values: %s", values) + # Easy merge - will flatten later + values = [default_map] + [values] + result = self._normalize_parameters(values, param, ctx) + return result + + class CfnMetadataType(click.ParamType): """ Custom Click options type to accept values for metadata parameters. From 4c740cfc025e859108dd21cb2b9174ab90ee8a26 Mon Sep 17 00:00:00 2001 From: Joe Wakefield Date: Thu, 6 Feb 2025 13:00:10 -0500 Subject: [PATCH 07/16] Lint --- samcli/cli/types.py | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-) diff --git a/samcli/cli/types.py b/samcli/cli/types.py index 71801c04b65..b3cc30f07ee 100644 --- a/samcli/cli/types.py +++ b/samcli/cli/types.py @@ -10,7 +10,6 @@ from typing import Dict, List, Optional, Union import click - from ruamel.yaml.comments import CommentedMap, CommentedSeq from samcli.lib.config.file_manager import FILE_MANAGER_MAPPER @@ -64,7 +63,8 @@ def _unquote_wrapped_quotes(value): return value.replace("\\ ", " ").replace('\\"', '"').replace("\\'", "'") -def _flatten_list(data: list) -> list: + +def _flatten_list(data: list | tuple) -> list: """ Recursively flattens lists and other list-like types for easy sequential processing. This also helps with lists combined with YAML anchors & aliases. @@ -97,7 +97,6 @@ class CfnParameterOverridesType(click.ParamType): __EXAMPLE_2 = "KeyPairName=MyKey InstanceType=t1.micro" __EXAMPLE_3 = "file://MyParams.yaml" - # Regex that parses CloudFormation parameter key-value pairs: # https://regex101.com/r/xqfSjW/2 # https://regex101.com/r/xqfSjW/5 @@ -116,8 +115,8 @@ class CfnParameterOverridesType(click.ParamType): def _normalize_parameters(self, values, param, ctx): """ - Normalizes parameter overrides into key-value pairs of strings - Later keys overwrite previous ones in case of key conflict + Normalizes parameter overrides into key-value pairs of strings + Later keys overwrite previous ones in case of key conflict """ if values in (("",), "", None) or values == {}: LOG.debug("Empty parameter set (%s)", values) @@ -130,7 +129,7 @@ def _normalize_parameters(self, values, param, ctx): parameters = {} for value in values: if isinstance(value, str): - if value.startswith('file://'): + if value.startswith("file://"): filepath = Path(value[7:]) if not filepath.is_file(): self.fail(f"{value} was not found or is a directory", param, ctx) @@ -148,7 +147,8 @@ def _normalize_parameters(self, values, param, ctx): break else: self.fail( - f"{value} is not a valid format. It must look something like '{self.__EXAMPLE_1}', '{self.__EXAMPLE_2}', or '{self.__EXAMPLE_3}'", + f"{value} is not a valid format. It must look something like " + f"'{self.__EXAMPLE_1}', '{self.__EXAMPLE_2}', or '{self.__EXAMPLE_3}'", param, ctx, ) @@ -169,16 +169,15 @@ def _normalize_parameters(self, values, param, ctx): result = {} for key, param_value in parameters.items(): - result[_unquote_wrapped_quotes(key)] = _unquote_wrapped_quotes(param_value) + result[_unquote_wrapped_quotes(key.lower())] = _unquote_wrapped_quotes(param_value) LOG.debug("Output parameters: %s", result) return result - def convert(self, values, param, ctx): # Merge samconfig with CLI parameter-overrides - if isinstance(values, tuple): # Tuple implies CLI + if isinstance(values, tuple): # Tuple implies CLI # default_map was populated from samconfig - default_map = ctx.default_map.get('parameter_overrides', {}) + default_map = ctx.default_map.get("parameter_overrides", {}) LOG.debug("Default map: %s", default_map) LOG.debug("Current values: %s", values) # Easy merge - will flatten later From 124d78f0c7952aecc92a411be484190f19efd3e1 Mon Sep 17 00:00:00 2001 From: Joe Wakefield Date: Fri, 7 Feb 2025 11:24:13 -0500 Subject: [PATCH 08/16] Add unit tests, apply related fixes --- samcli/cli/types.py | 14 ++-- tests/unit/cli/test_types.py | 143 +++++++++++++++++++++++++++++++++++ 2 files changed, 152 insertions(+), 5 deletions(-) diff --git a/samcli/cli/types.py b/samcli/cli/types.py index b3cc30f07ee..4aa29fda1cf 100644 --- a/samcli/cli/types.py +++ b/samcli/cli/types.py @@ -155,11 +155,15 @@ def _normalize_parameters(self, values, param, ctx): elif isinstance(value, (dict, CommentedMap)): # e.g. YAML key-value pairs for k, v in value.items(): - if isinstance(v, (list, CommentedSeq)): - # Collapse list values to comma delimited - parameters[str(k)] = ",".join(map(str, v)) + if v is None: + parameters[str(k)] = "" + elif isinstance(v, (list, CommentedSeq)): + # Collapse list values to comma delimited, ignore empty strings and remove extra spaces + parameters[str(k)] = ",".join(str(x).strip() for x in v if x not in (None, "")) else: parameters[str(k)] = str(v) + elif value is None: + continue else: self.fail( f"{value} is not valid in a way the code doesn't expect", @@ -169,13 +173,13 @@ def _normalize_parameters(self, values, param, ctx): result = {} for key, param_value in parameters.items(): - result[_unquote_wrapped_quotes(key.lower())] = _unquote_wrapped_quotes(param_value) + result[_unquote_wrapped_quotes(key)] = _unquote_wrapped_quotes(param_value) LOG.debug("Output parameters: %s", result) return result def convert(self, values, param, ctx): # Merge samconfig with CLI parameter-overrides - if isinstance(values, tuple): # Tuple implies CLI + if isinstance(values, tuple) and ctx: # ctx isn't set in all unit tests # default_map was populated from samconfig default_map = ctx.default_map.get("parameter_overrides", {}) LOG.debug("Default map: %s", default_map) diff --git a/tests/unit/cli/test_types.py b/tests/unit/cli/test_types.py index f6943107f6c..23e175fa2f2 100644 --- a/tests/unit/cli/test_types.py +++ b/tests/unit/cli/test_types.py @@ -150,7 +150,150 @@ def test_successful_parsing(self, input, expected): self.assertEqual(result, expected, msg="Failed with Input = " + str(input)) + @parameterized.expand( + [ + ("", "", {}, ), + (("A=1 A=2"), ("A=3 A=4",), {"A": "4"}), + (("Path=C:\\Windows\\System32"),("Path=/usr/bin",), {"Path": "/usr/bin"}), + ( + ("A=1 B=2"), + ("ParameterKey=C,ParameterValue=3", "ParameterKey=D,ParameterValue=4",), + {"A": "1", "B": "2", "C": "3", "D": "4"}, + ), + ( + ('Quoted="Hello, World!"'), + ('ParameterKey=Quoted,ParameterValue="\'Hi\'"',), + {"Quoted": "'Hi'"} + ), + ( + ("A=1 B=2"), + ("ParameterKey=A,ParameterValue=3", "ParameterKey=D,ParameterValue=4",), + {"A": "3", "B": "2", "D": "4"}, + ), + ( + ["A=1", "B=2"], + ("ParameterKey=A,ParameterValue=3", "ParameterKey=D,ParameterValue=4",), + {"A": "3", "B": "2", "D": "4"}, + ), + ( + ("A=1,2,3 B=1,2,3"), + ("ParameterKey=A,ParameterValue=2,2", "ParameterKey=D,ParameterValue=4,5,6",), + {"A": "2,2", "B": "1,2,3", "D": "4,5,6"}, + ), + ( + ("A=1 B=2 C=3"), + ("C=4",), + {"A": "1", "B": "2", "C": "4"}, + ), + ( + {"Flag": False}, + ("Flag=True",), + {"Flag": "True"} + ), + ( + ("A=1 B=2"), + "A=3 D=4", # String not tuple + {"A": "3", "D": "4"} + ), + ( + ("A=1 B=2 C=3"), + ["C=4"], # List not tuple + {"C": "4"}, + ), + ( + [[[[[[[[[[[[[[[[[[]]]]]]]]]]]], {"A": "1"}, [[[{"B": "2"}]]]]]]]]], + ("A=3",), + {"A": "3", "B": "2"}, + ), + ( + {"A": "1"}, + ("B=2",), + {"A": "1", "B": "2"}, + ), + ( + {"Spaces": "String with spaces"}, + ("Pam='Param Pam Pam Param'",), + {"Spaces": "String with spaces", "Pam": "Param Pam Pam Param"}, + ), + ( + [None, {"Empty": None}], + ("None=None",), + {"Empty": "", "None": "None"} + ), + ( + {"List": [" A ", 1, False, "", None]}, + ("OtherList='A,1,False'",), + {"List": "A,1,False", "OtherList": 'A,1,False'}, + ), + ( + {"List": [" A ", "' B '", 1, False, "", None]}, + ("List2=' A ,1,False'",), + {"List": "A,' B ',1,False", "List2": ' A ,1,False'}, + ), + ] + ) + def test_merge_default_map_parsing(self, file_overrides, cli_overrides, expected): + ctx = MagicMock() + ctx.default_map = {"parameter_overrides": file_overrides} + result = self.param_type.convert(cli_overrides, None, ctx) + self.assertEqual(result, expected, msg="Failed with Input = " + str(cli_overrides)) + + @parameterized.expand( + [ + ( + ("A=1 B=2"), + ("file://params.toml",), + {"A": "toml", "B": "toml", "Toml": "toml"}, + ), + ( + ("A=1 B=2"), + ("file://params.toml", "D=4"), + {"A": "toml", "B": "toml", "Toml": "toml", "D": "4"}, + ), + ( + ("A=1 B=2"), + ("ParameterKey=Y,ParameterValue=y", "file://params.toml", "D=4", "file://params.yaml", "A=6"), + {"A": "6", "B": "yaml", "Y": "y", "Toml": "toml", "D": "4", "Yaml": "yaml"}, + ), + ( + ("A=1 B=2"), + ("file://list.yaml",), + {"A": "a", "B": "2", "List": "1,2,3", "Yaml": "yaml"}, + ), + ( + ("A=1 B=2"), + ("file://nested.yaml",), + {"A": "yaml", "B": "yaml", "Toml": "toml", "Yaml": "yaml"}, + ), + ] + ) + def test_merge_file_parsing(self, file_overrides, cli_overrides, expected): + mock_files = { + "params.toml": 'A = "toml"\nB = "toml"\nToml = "toml"', + "params.yaml": "A: yaml\nB: yaml\nYaml: yaml", + "list.yaml": "- - - - - - A: a\n- List:\n - 1\n - 2\n - 3\n- Yaml: yaml", + "nested.yaml": "- file://params.toml\n- file://params.yaml", + } + + def mock_read_text(file_path): + file_name = file_path.name + return mock_files.get(file_name, "") + + def mock_is_file(file_path): + return file_path.name in mock_files + + with patch("pathlib.Path.is_file", new=mock_is_file), patch("pathlib.Path.read_text", new=mock_read_text): + ctx = MagicMock() + ctx.default_map = {"parameter_overrides": file_overrides} + + result = self.param_type.convert(cli_overrides, None, ctx) + print(result) + self.assertEqual(result, expected, msg="Failed with Input = " + str(cli_overrides)) + + + class TestCfnMetadataType(TestCase): + def setUp(self): self.param_type = CfnMetadataType() From 8b3782e07dde6c9225bdd9367bff5c322fb73444 Mon Sep 17 00:00:00 2001 From: Joe Wakefield Date: Sat, 8 Feb 2025 18:56:42 -0500 Subject: [PATCH 09/16] Replace |= with .updates() to support older Python --- samcli/cli/types.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/samcli/cli/types.py b/samcli/cli/types.py index 4aa29fda1cf..36d1ab16f19 100644 --- a/samcli/cli/types.py +++ b/samcli/cli/types.py @@ -136,14 +136,14 @@ def _normalize_parameters(self, values, param, ctx): file_manager = FILE_MANAGER_MAPPER.get(filepath.suffix, None) if not file_manager: self.fail(f"{value} uses an unsupported extension", param, ctx) - parameters |= self._normalize_parameters(file_manager.read(filepath), param, ctx) + parameters.update(self._normalize_parameters(file_manager.read(filepath), param, ctx)) else: # Legacy parameter matching normalized_value = " " + value.strip() for pattern in self.ordered_pattern_match: groups = re.findall(pattern, normalized_value) if groups: - parameters |= groups + parameters.update(groups) break else: self.fail( From c2aaa39ce489aca9b711f7602881a8546d3b5f9f Mon Sep 17 00:00:00 2001 From: Joe Wakefield Date: Mon, 10 Feb 2025 15:40:30 -0500 Subject: [PATCH 10/16] Remove merging samconfig and cli --- samcli/cli/types.py | 20 +------ tests/unit/cli/test_types.py | 108 ++--------------------------------- 2 files changed, 9 insertions(+), 119 deletions(-) diff --git a/samcli/cli/types.py b/samcli/cli/types.py index 36d1ab16f19..3bcc7e41ab2 100644 --- a/samcli/cli/types.py +++ b/samcli/cli/types.py @@ -113,10 +113,9 @@ class CfnParameterOverridesType(click.ParamType): ordered_pattern_match = [_pattern_1, _pattern_2] - def _normalize_parameters(self, values, param, ctx): + def convert(self, values, param, ctx): """ - Normalizes parameter overrides into key-value pairs of strings - Later keys overwrite previous ones in case of key conflict + Normalizes different parameter overrides formats into key-value pairs of strings """ if values in (("",), "", None) or values == {}: LOG.debug("Empty parameter set (%s)", values) @@ -136,7 +135,7 @@ def _normalize_parameters(self, values, param, ctx): file_manager = FILE_MANAGER_MAPPER.get(filepath.suffix, None) if not file_manager: self.fail(f"{value} uses an unsupported extension", param, ctx) - parameters.update(self._normalize_parameters(file_manager.read(filepath), param, ctx)) + parameters.update(self.convert(file_manager.read(filepath), param, ctx)) else: # Legacy parameter matching normalized_value = " " + value.strip() @@ -177,19 +176,6 @@ def _normalize_parameters(self, values, param, ctx): LOG.debug("Output parameters: %s", result) return result - def convert(self, values, param, ctx): - # Merge samconfig with CLI parameter-overrides - if isinstance(values, tuple) and ctx: # ctx isn't set in all unit tests - # default_map was populated from samconfig - default_map = ctx.default_map.get("parameter_overrides", {}) - LOG.debug("Default map: %s", default_map) - LOG.debug("Current values: %s", values) - # Easy merge - will flatten later - values = [default_map] + [values] - result = self._normalize_parameters(values, param, ctx) - return result - - class CfnMetadataType(click.ParamType): """ Custom Click options type to accept values for metadata parameters. diff --git a/tests/unit/cli/test_types.py b/tests/unit/cli/test_types.py index 23e175fa2f2..307f4554952 100644 --- a/tests/unit/cli/test_types.py +++ b/tests/unit/cli/test_types.py @@ -152,126 +152,33 @@ def test_successful_parsing(self, input, expected): @parameterized.expand( [ - ("", "", {}, ), - (("A=1 A=2"), ("A=3 A=4",), {"A": "4"}), - (("Path=C:\\Windows\\System32"),("Path=/usr/bin",), {"Path": "/usr/bin"}), ( - ("A=1 B=2"), - ("ParameterKey=C,ParameterValue=3", "ParameterKey=D,ParameterValue=4",), - {"A": "1", "B": "2", "C": "3", "D": "4"}, - ), - ( - ('Quoted="Hello, World!"'), - ('ParameterKey=Quoted,ParameterValue="\'Hi\'"',), - {"Quoted": "'Hi'"} - ), - ( - ("A=1 B=2"), - ("ParameterKey=A,ParameterValue=3", "ParameterKey=D,ParameterValue=4",), - {"A": "3", "B": "2", "D": "4"}, - ), - ( - ["A=1", "B=2"], - ("ParameterKey=A,ParameterValue=3", "ParameterKey=D,ParameterValue=4",), - {"A": "3", "B": "2", "D": "4"}, - ), - ( - ("A=1,2,3 B=1,2,3"), - ("ParameterKey=A,ParameterValue=2,2", "ParameterKey=D,ParameterValue=4,5,6",), - {"A": "2,2", "B": "1,2,3", "D": "4,5,6"}, - ), - ( - ("A=1 B=2 C=3"), - ("C=4",), - {"A": "1", "B": "2", "C": "4"}, - ), - ( - {"Flag": False}, - ("Flag=True",), - {"Flag": "True"} - ), - ( - ("A=1 B=2"), - "A=3 D=4", # String not tuple - {"A": "3", "D": "4"} - ), - ( - ("A=1 B=2 C=3"), - ["C=4"], # List not tuple - {"C": "4"}, - ), - ( - [[[[[[[[[[[[[[[[[[]]]]]]]]]]]], {"A": "1"}, [[[{"B": "2"}]]]]]]]]], - ("A=3",), - {"A": "3", "B": "2"}, - ), - ( - {"A": "1"}, - ("B=2",), - {"A": "1", "B": "2"}, - ), - ( - {"Spaces": "String with spaces"}, - ("Pam='Param Pam Pam Param'",), - {"Spaces": "String with spaces", "Pam": "Param Pam Pam Param"}, - ), - ( - [None, {"Empty": None}], - ("None=None",), - {"Empty": "", "None": "None"} - ), - ( - {"List": [" A ", 1, False, "", None]}, - ("OtherList='A,1,False'",), - {"List": "A,1,False", "OtherList": 'A,1,False'}, - ), - ( - {"List": [" A ", "' B '", 1, False, "", None]}, - ("List2=' A ,1,False'",), - {"List": "A,' B ',1,False", "List2": ' A ,1,False'}, - ), - ] - ) - def test_merge_default_map_parsing(self, file_overrides, cli_overrides, expected): - ctx = MagicMock() - ctx.default_map = {"parameter_overrides": file_overrides} - result = self.param_type.convert(cli_overrides, None, ctx) - self.assertEqual(result, expected, msg="Failed with Input = " + str(cli_overrides)) - - @parameterized.expand( - [ - ( - ("A=1 B=2"), ("file://params.toml",), {"A": "toml", "B": "toml", "Toml": "toml"}, ), ( - ("A=1 B=2"), ("file://params.toml", "D=4"), {"A": "toml", "B": "toml", "Toml": "toml", "D": "4"}, ), ( - ("A=1 B=2"), ("ParameterKey=Y,ParameterValue=y", "file://params.toml", "D=4", "file://params.yaml", "A=6"), {"A": "6", "B": "yaml", "Y": "y", "Toml": "toml", "D": "4", "Yaml": "yaml"}, ), ( - ("A=1 B=2"), - ("file://list.yaml",), - {"A": "a", "B": "2", "List": "1,2,3", "Yaml": "yaml"}, + ("file://params.yaml", "file://list.yaml",), + {"A": "a", "B": "yaml", "List": "1,2,3", "Yaml": "yaml"}, ), ( - ("A=1 B=2"), ("file://nested.yaml",), {"A": "yaml", "B": "yaml", "Toml": "toml", "Yaml": "yaml"}, ), ] ) - def test_merge_file_parsing(self, file_overrides, cli_overrides, expected): + def test_merge_file_parsing(self, inputs, expected): mock_files = { "params.toml": 'A = "toml"\nB = "toml"\nToml = "toml"', "params.yaml": "A: yaml\nB: yaml\nYaml: yaml", - "list.yaml": "- - - - - - A: a\n- List:\n - 1\n - 2\n - 3\n- Yaml: yaml", + "list.yaml": "- - - - - - A: a\n- List:\n - 1\n - 2\n - 3\n", "nested.yaml": "- file://params.toml\n- file://params.yaml", } @@ -283,12 +190,9 @@ def mock_is_file(file_path): return file_path.name in mock_files with patch("pathlib.Path.is_file", new=mock_is_file), patch("pathlib.Path.read_text", new=mock_read_text): - ctx = MagicMock() - ctx.default_map = {"parameter_overrides": file_overrides} - - result = self.param_type.convert(cli_overrides, None, ctx) + result = self.param_type.convert(inputs, None, MagicMock()) print(result) - self.assertEqual(result, expected, msg="Failed with Input = " + str(cli_overrides)) + self.assertEqual(result, expected, msg="Failed with Input = " + str(inputs)) From 2152c8bc72d261243a2381f1739759d6e74d9dd6 Mon Sep 17 00:00:00 2001 From: Joe Wakefield Date: Tue, 18 Feb 2025 08:19:57 -0500 Subject: [PATCH 11/16] Fix type hint union --- samcli/cli/types.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/samcli/cli/types.py b/samcli/cli/types.py index 3bcc7e41ab2..493dde784fc 100644 --- a/samcli/cli/types.py +++ b/samcli/cli/types.py @@ -64,7 +64,7 @@ def _unquote_wrapped_quotes(value): return value.replace("\\ ", " ").replace('\\"', '"').replace("\\'", "'") -def _flatten_list(data: list | tuple) -> list: +def _flatten_list(data: Union[list, tuple]) -> list: """ Recursively flattens lists and other list-like types for easy sequential processing. This also helps with lists combined with YAML anchors & aliases. From b5f3f6b1f1cdedceebc50bb998f737ec98f3379f Mon Sep 17 00:00:00 2001 From: Joe Wakefield Date: Wed, 19 Feb 2025 08:28:10 -0500 Subject: [PATCH 12/16] re-add accidental line deletion --- samcli/cli/types.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/samcli/cli/types.py b/samcli/cli/types.py index 493dde784fc..6978c9f0515 100644 --- a/samcli/cli/types.py +++ b/samcli/cli/types.py @@ -113,6 +113,8 @@ class CfnParameterOverridesType(click.ParamType): ordered_pattern_match = [_pattern_1, _pattern_2] + name = "string,list" + def convert(self, values, param, ctx): """ Normalizes different parameter overrides formats into key-value pairs of strings From af3c3e446f9ced7a3925104c78134471f6574e42 Mon Sep 17 00:00:00 2001 From: Joe Wakefield Date: Wed, 19 Feb 2025 13:17:28 -0500 Subject: [PATCH 13/16] Fix json schema and add tests --- samcli/cli/types.py | 3 +- schema/make_schema.py | 39 ++++++++++++- schema/samcli.json | 55 ++++++++++++++++--- tests/unit/cli/test_types.py | 1 - ...md_wrongtype_parameter_overrides_list.toml | 4 ++ ...md_wrongtype_parameter_overrides_list.yaml | 7 +++ .../parameter_overrides_features.yaml | 19 +++++++ .../schema/testdata/passing_tests/sample.toml | 4 ++ 8 files changed, 119 insertions(+), 13 deletions(-) create mode 100644 tests/unit/schema/testdata/failing_tests/buildcmd_wrongtype_parameter_overrides_list.toml create mode 100644 tests/unit/schema/testdata/failing_tests/buildcmd_wrongtype_parameter_overrides_list.yaml create mode 100644 tests/unit/schema/testdata/passing_tests/parameter_overrides_features.yaml diff --git a/samcli/cli/types.py b/samcli/cli/types.py index 6978c9f0515..9d05d579408 100644 --- a/samcli/cli/types.py +++ b/samcli/cli/types.py @@ -113,7 +113,7 @@ class CfnParameterOverridesType(click.ParamType): ordered_pattern_match = [_pattern_1, _pattern_2] - name = "string,list" + name = "list,object,string" def convert(self, values, param, ctx): """ @@ -178,6 +178,7 @@ def convert(self, values, param, ctx): LOG.debug("Output parameters: %s", result) return result + class CfnMetadataType(click.ParamType): """ Custom Click options type to accept values for metadata parameters. diff --git a/schema/make_schema.py b/schema/make_schema.py index 8e1a8e15687..3905d2b2b30 100644 --- a/schema/make_schema.py +++ b/schema/make_schema.py @@ -45,7 +45,7 @@ class SamCliParameterSchema: type: Union[str, List[str]] description: str = "" default: Optional[Any] = None - items: Optional[str] = None + items: Optional[Union[str, Dict[str, str]]] = None choices: Optional[Any] = None def to_schema(self) -> Dict[str, Any]: @@ -55,7 +55,11 @@ def to_schema(self) -> Dict[str, Any]: if self.default: param.update({"default": self.default}) if self.items: - param.update({"items": {"type": self.items}}) + if isinstance(self.items, dict): + param.update({"items": self.items}) + else: + param.update({"items": {"type": self.items}}) + if self.choices: if isinstance(self.choices, list): self.choices.sort() @@ -143,11 +147,15 @@ def format_param(param: click.core.Option) -> SamCliParameterSchema: formatted_param_types.append(param_name or "string") formatted_param_types = sorted(list(set(formatted_param_types))) # deduplicate + # Allow nested arrays of objects and strings + parameter_overrides_ref = { "$ref": "#/$defs/parameter_overrides_items" } + formatted_param: SamCliParameterSchema = SamCliParameterSchema( param.name or "", formatted_param_types if len(formatted_param_types) > 1 else formatted_param_types[0], clean_text(param.help or ""), - items="string" if "array" in formatted_param_types else None, + items=parameter_overrides_ref if param.name == "parameter_overrides" + else "string" if "array" in formatted_param_types else None, ) if param.default and param.name not in PARAMS_TO_OMIT_DEFAULT_FIELD: @@ -231,6 +239,31 @@ def generate_schema() -> dict: } schema["required"] = ["version"] schema["additionalProperties"] = False + # Allows objects and strings beneath variably nested arrays + schema["$defs"]= { + "parameter_overrides_items": { + "anyOf": [ + { + "type": "array", + "items": { "$ref": "#/$defs/parameter_overrides_items" } + }, + { + "type": "object", + "additionalProperties": { + "anyOf": [ + { "type": "string" }, + { "type": "integer" }, # Allow cooler types if it's a dict + { "type": "boolean" }, + ] + }, + }, + { + "type": "string" + }, + ] + } + } + # Iterate through packages for command and parameter information for package_name in _SAM_CLI_COMMAND_PACKAGES: commands.extend(retrieve_command_structure(package_name)) diff --git a/schema/samcli.json b/schema/samcli.json index 143f7f3031c..4a012ff3f00 100644 --- a/schema/samcli.json +++ b/schema/samcli.json @@ -13,6 +13,37 @@ "version" ], "additionalProperties": false, + "$defs": { + "parameter_overrides_items": { + "anyOf": [ + { + "type": "array", + "items": { + "$ref": "#/$defs/parameter_overrides_items" + } + }, + { + "type": "object", + "additionalProperties": { + "anyOf": [ + { + "type": "string" + }, + { + "type": "integer" + }, + { + "type": "boolean" + } + ] + } + }, + { + "type": "string" + } + ] + } + }, "patternProperties": { "^.+$": { "title": "Environment", @@ -346,11 +377,12 @@ "title": "parameter_overrides", "type": [ "array", + "object", "string" ], "description": "String that contains AWS CloudFormation parameter overrides encoded as key=value pairs.", "items": { - "type": "string" + "$ref": "#/$defs/parameter_overrides_items" } }, "skip_pull_image": { @@ -445,11 +477,12 @@ "title": "parameter_overrides", "type": [ "array", + "object", "string" ], "description": "String that contains AWS CloudFormation parameter overrides encoded as key=value pairs.", "items": { - "type": "string" + "$ref": "#/$defs/parameter_overrides_items" } }, "debug_port": { @@ -636,11 +669,12 @@ "title": "parameter_overrides", "type": [ "array", + "object", "string" ], "description": "String that contains AWS CloudFormation parameter overrides encoded as key=value pairs.", "items": { - "type": "string" + "$ref": "#/$defs/parameter_overrides_items" } }, "debug_port": { @@ -835,11 +869,12 @@ "title": "parameter_overrides", "type": [ "array", + "object", "string" ], "description": "String that contains AWS CloudFormation parameter overrides encoded as key=value pairs.", "items": { - "type": "string" + "$ref": "#/$defs/parameter_overrides_items" } }, "debug_port": { @@ -1225,11 +1260,12 @@ "title": "parameter_overrides", "type": [ "array", + "object", "string" ], "description": "String that contains AWS CloudFormation parameter overrides encoded as key=value pairs.", "items": { - "type": "string" + "$ref": "#/$defs/parameter_overrides_items" } }, "signing_profiles": { @@ -1699,11 +1735,12 @@ "title": "parameter_overrides", "type": [ "array", + "object", "string" ], "description": "String that contains AWS CloudFormation parameter overrides encoded as key=value pairs.", "items": { - "type": "string" + "$ref": "#/$defs/parameter_overrides_items" } }, "beta_features": { @@ -1982,11 +2019,12 @@ "title": "parameter_overrides", "type": [ "array", + "object", "string" ], "description": "String that contains AWS CloudFormation parameter overrides encoded as key=value pairs.", "items": { - "type": "string" + "$ref": "#/$defs/parameter_overrides_items" } }, "stack_name": { @@ -2111,11 +2149,12 @@ "title": "parameter_overrides", "type": [ "array", + "object", "string" ], "description": "String that contains AWS CloudFormation parameter overrides encoded as key=value pairs.", "items": { - "type": "string" + "$ref": "#/$defs/parameter_overrides_items" } }, "stack_name": { diff --git a/tests/unit/cli/test_types.py b/tests/unit/cli/test_types.py index 307f4554952..bd7ffabfa88 100644 --- a/tests/unit/cli/test_types.py +++ b/tests/unit/cli/test_types.py @@ -195,7 +195,6 @@ def mock_is_file(file_path): self.assertEqual(result, expected, msg="Failed with Input = " + str(inputs)) - class TestCfnMetadataType(TestCase): def setUp(self): diff --git a/tests/unit/schema/testdata/failing_tests/buildcmd_wrongtype_parameter_overrides_list.toml b/tests/unit/schema/testdata/failing_tests/buildcmd_wrongtype_parameter_overrides_list.toml new file mode 100644 index 00000000000..64e3a4805dd --- /dev/null +++ b/tests/unit/schema/testdata/failing_tests/buildcmd_wrongtype_parameter_overrides_list.toml @@ -0,0 +1,4 @@ +version = 0.1 + +[default.build.parameters] +parameter_overrides = [ 10 ] \ No newline at end of file diff --git a/tests/unit/schema/testdata/failing_tests/buildcmd_wrongtype_parameter_overrides_list.yaml b/tests/unit/schema/testdata/failing_tests/buildcmd_wrongtype_parameter_overrides_list.yaml new file mode 100644 index 00000000000..7d477dd6858 --- /dev/null +++ b/tests/unit/schema/testdata/failing_tests/buildcmd_wrongtype_parameter_overrides_list.yaml @@ -0,0 +1,7 @@ +version: 0.1 + +default: + build: + parameters: + parameter_overrides: + - 10 \ No newline at end of file diff --git a/tests/unit/schema/testdata/passing_tests/parameter_overrides_features.yaml b/tests/unit/schema/testdata/passing_tests/parameter_overrides_features.yaml new file mode 100644 index 00000000000..ede1025358d --- /dev/null +++ b/tests/unit/schema/testdata/passing_tests/parameter_overrides_features.yaml @@ -0,0 +1,19 @@ +version: 0.1 +prod-a: + deploy: + parameters: + stack_name: sam-app + region: us-east-1 + parameter_overrides: &prod_a_overrides + - Name: prod-a + - ParamA: 1 + - ParamB: True + +prod-b: + deploy: + parameters: + stack_name: sam-app-2 + region: us-east-1 + parameter_overrides: + - *prod_a_overrides + - Name: prod-b \ No newline at end of file diff --git a/tests/unit/schema/testdata/passing_tests/sample.toml b/tests/unit/schema/testdata/passing_tests/sample.toml index 4b95a2c644d..03726773f12 100644 --- a/tests/unit/schema/testdata/passing_tests/sample.toml +++ b/tests/unit/schema/testdata/passing_tests/sample.toml @@ -14,6 +14,10 @@ capabilities = "CAPABILITY_IAM" confirm_changeset = true resolve_s3 = true +[default.deploy.parameters.parameter_overrides] +A = "foo" +B = "bar" + [default.sync.parameters] watch = true From b6ba275ae5bc8e6ba45585d67e9cd263f84055a4 Mon Sep 17 00:00:00 2001 From: Joe Wakefield Date: Thu, 22 May 2025 13:59:09 -0400 Subject: [PATCH 14/16] Update comments and black linting --- samcli/cli/types.py | 67 ++++++++++++++++++++++++++++-------- schema/make_schema.py | 28 +++++++-------- tests/unit/cli/test_types.py | 6 ++-- 3 files changed, 69 insertions(+), 32 deletions(-) diff --git a/samcli/cli/types.py b/samcli/cli/types.py index 9d05d579408..a8489575121 100644 --- a/samcli/cli/types.py +++ b/samcli/cli/types.py @@ -66,17 +66,26 @@ def _unquote_wrapped_quotes(value): def _flatten_list(data: Union[list, tuple]) -> list: """ - Recursively flattens lists and other list-like types for easy sequential processing. - This also helps with lists combined with YAML anchors & aliases. + Recursively flattens nested lists, tuples, and list-like sequences into a single flat list. + + This function is the first step in normalizing inputs parsed from + various file formats (e.g., YAML, TOML, JSON), where nested sequences can + appear due to format-specific constructs such as YAML anchors/aliases, explicit + nested arrays in TOML or JSON, or user-defined nested data structures. + + By flattening these nested lists/sequences, subsequent processing can + uniformly handle all values as a simple sequential list before further + normalization (e.g., converting all values to strings). Parameters ---------- - value : data + data : list or tuple List to flatten Returns ------- - Flattened list + list + A single flat list containing all atomic elements from the nested input, preserving order. """ flat_data = [] for item in data: @@ -117,29 +126,53 @@ class CfnParameterOverridesType(click.ParamType): def convert(self, values, param, ctx): """ - Normalizes different parameter overrides formats into key-value pairs of strings + Takes parameter overrides loaded from various supported config file formats and + flattens and normalizes them into a dictionary where all keys and values are strings. + + The input `values` can be nested lists, dictionaries, or strings representing + parameter overrides, potentially including file references. This method + recursively resolves file inputs, validates formats, and standardizes output. + + Parameters + ---------- + values : Any + Input parameter overrides from config files or CLI (could be nested lists, dicts, strings). + param : click.Parameter + Parameter metadata (from Click library), used for error handling. + ctx : click.Context + Click context for error reporting. + + Returns + ------- + dict + Flattened and normalized parameter overrides as a dictionary of string keys and values. """ - if values in (("",), "", None) or values == {}: + # Handle empty or trivial input cases early + if values in [("",), "", None] or values == {}: LOG.debug("Empty parameter set (%s)", values) return {} LOG.debug("Input parameters: %s", values) + + # Flatten any nested objects to a simple list for uniform processing values = _flatten_list([values]) LOG.debug("Flattened parameters: %s", values) parameters = {} for value in values: if isinstance(value, str): + # If the string is a file reference (e.g., 'file://params.yaml') if value.startswith("file://"): - filepath = Path(value[7:]) - if not filepath.is_file(): + file_path = Path(value[7:]) + if not file_path.is_file(): self.fail(f"{value} was not found or is a directory", param, ctx) - file_manager = FILE_MANAGER_MAPPER.get(filepath.suffix, None) + file_manager = FILE_MANAGER_MAPPER.get(file_path.suffix, None) if not file_manager: self.fail(f"{value} uses an unsupported extension", param, ctx) - parameters.update(self.convert(file_manager.read(filepath), param, ctx)) + # Recursively process the file's contents and update parameters + parameters.update(self.convert(file_manager.read(file_path), param, ctx)) else: - # Legacy parameter matching + # Legacy parameter matching (e.g. "X=Y" and "ParameterKey=X,ParameterValue=Y") normalized_value = " " + value.strip() for pattern in self.ordered_pattern_match: groups = re.findall(pattern, normalized_value) @@ -154,20 +187,24 @@ def convert(self, values, param, ctx): ctx, ) elif isinstance(value, (dict, CommentedMap)): - # e.g. YAML key-value pairs + # Handle dictionary-like objects (e.g. YAML mappings) for k, v in value.items(): if v is None: + # Unset value if previously set parameters[str(k)] = "" elif isinstance(v, (list, CommentedSeq)): - # Collapse list values to comma delimited, ignore empty strings and remove extra spaces + # Join list elements into comma-separated string, strip whitespace and ignore empty entries parameters[str(k)] = ",".join(str(x).strip() for x in v if x not in (None, "")) else: + # Convert other values (numbers, booleans) to strings parameters[str(k)] = str(v) elif value is None: + # Ignore empty list items since it means both the key and value are empty continue else: + # Fail on unexpected types to avoid silent errors self.fail( - f"{value} is not valid in a way the code doesn't expect", + f"{value} is invalid in a way the code doesn't expect", param, ctx, ) @@ -175,7 +212,7 @@ def convert(self, values, param, ctx): result = {} for key, param_value in parameters.items(): result[_unquote_wrapped_quotes(key)] = _unquote_wrapped_quotes(param_value) - LOG.debug("Output parameters: %s", result) + LOG.debug("Converted %d parameters: %s", len(result), result) return result diff --git a/schema/make_schema.py b/schema/make_schema.py index 3905d2b2b30..fe98e6a42f5 100644 --- a/schema/make_schema.py +++ b/schema/make_schema.py @@ -148,14 +148,17 @@ def format_param(param: click.core.Option) -> SamCliParameterSchema: formatted_param_types = sorted(list(set(formatted_param_types))) # deduplicate # Allow nested arrays of objects and strings - parameter_overrides_ref = { "$ref": "#/$defs/parameter_overrides_items" } + parameter_overrides_ref = {"$ref": "#/$defs/parameter_overrides_items"} formatted_param: SamCliParameterSchema = SamCliParameterSchema( param.name or "", formatted_param_types if len(formatted_param_types) > 1 else formatted_param_types[0], clean_text(param.help or ""), - items=parameter_overrides_ref if param.name == "parameter_overrides" - else "string" if "array" in formatted_param_types else None, + items=( + parameter_overrides_ref + if param.name == "parameter_overrides" + else "string" if "array" in formatted_param_types else None + ), ) if param.default and param.name not in PARAMS_TO_OMIT_DEFAULT_FIELD: @@ -240,26 +243,21 @@ def generate_schema() -> dict: schema["required"] = ["version"] schema["additionalProperties"] = False # Allows objects and strings beneath variably nested arrays - schema["$defs"]= { + schema["$defs"] = { "parameter_overrides_items": { "anyOf": [ - { - "type": "array", - "items": { "$ref": "#/$defs/parameter_overrides_items" } - }, + {"type": "array", "items": {"$ref": "#/$defs/parameter_overrides_items"}}, { "type": "object", "additionalProperties": { - "anyOf": [ - { "type": "string" }, - { "type": "integer" }, # Allow cooler types if it's a dict - { "type": "boolean" }, + "anyOf": [ + {"type": "string"}, + {"type": "integer"}, # Allow cooler types if it's a dict + {"type": "boolean"}, ] }, }, - { - "type": "string" - }, + {"type": "string"}, ] } } diff --git a/tests/unit/cli/test_types.py b/tests/unit/cli/test_types.py index bd7ffabfa88..9fdb1c40aaf 100644 --- a/tests/unit/cli/test_types.py +++ b/tests/unit/cli/test_types.py @@ -149,7 +149,6 @@ def test_successful_parsing(self, input, expected): result = self.param_type.convert(input, None, None) self.assertEqual(result, expected, msg="Failed with Input = " + str(input)) - @parameterized.expand( [ ( @@ -165,7 +164,10 @@ def test_successful_parsing(self, input, expected): {"A": "6", "B": "yaml", "Y": "y", "Toml": "toml", "D": "4", "Yaml": "yaml"}, ), ( - ("file://params.yaml", "file://list.yaml",), + ( + "file://params.yaml", + "file://list.yaml", + ), {"A": "a", "B": "yaml", "List": "1,2,3", "Yaml": "yaml"}, ), ( From b01d079e55fea3215f17517bb447a92d368bafa3 Mon Sep 17 00:00:00 2001 From: Joe Wakefield Date: Wed, 2 Jul 2025 14:35:55 -0400 Subject: [PATCH 15/16] add protection against infinite recursion --- samcli/cli/types.py | 17 +++++++++++++++-- tests/unit/cli/test_types.py | 21 +++++++++++++++++++++ 2 files changed, 36 insertions(+), 2 deletions(-) diff --git a/samcli/cli/types.py b/samcli/cli/types.py index a8489575121..4f53fd6cb01 100644 --- a/samcli/cli/types.py +++ b/samcli/cli/types.py @@ -124,7 +124,7 @@ class CfnParameterOverridesType(click.ParamType): name = "list,object,string" - def convert(self, values, param, ctx): + def convert(self, values, param, ctx, seen_files=None): """ Takes parameter overrides loaded from various supported config file formats and flattens and normalizes them into a dictionary where all keys and values are strings. @@ -141,6 +141,8 @@ def convert(self, values, param, ctx): Parameter metadata (from Click library), used for error handling. ctx : click.Context Click context for error reporting. + seen_files : set + List of files processed in the current execution branch, used to detect infinite recursion Returns ------- @@ -152,6 +154,9 @@ def convert(self, values, param, ctx): LOG.debug("Empty parameter set (%s)", values) return {} + if seen_files is None: + seen_files = set() + LOG.debug("Input parameters: %s", values) # Flatten any nested objects to a simple list for uniform processing @@ -169,8 +174,16 @@ def convert(self, values, param, ctx): file_manager = FILE_MANAGER_MAPPER.get(file_path.suffix, None) if not file_manager: self.fail(f"{value} uses an unsupported extension", param, ctx) + # Recursively process the file's contents and update parameters - parameters.update(self.convert(file_manager.read(file_path), param, ctx)) + if file_path in seen_files: + self.fail(f"Infinite recursion detected in file references: {file_path}", param, ctx) + seen_files.add(file_path) + try: + nested_values = file_manager.read(file_path) + parameters.update(self.convert(nested_values, param, ctx, seen_files)) + finally: + seen_files.remove(file_path) else: # Legacy parameter matching (e.g. "X=Y" and "ParameterKey=X,ParameterValue=Y") normalized_value = " " + value.strip() diff --git a/tests/unit/cli/test_types.py b/tests/unit/cli/test_types.py index 9fdb1c40aaf..04aba0fa1e9 100644 --- a/tests/unit/cli/test_types.py +++ b/tests/unit/cli/test_types.py @@ -196,6 +196,27 @@ def mock_is_file(file_path): print(result) self.assertEqual(result, expected, msg="Failed with Input = " + str(inputs)) + def test_infinite_recursion_protection(self): + mock_files = { + "A.yaml": "- file://B.yaml", + "B.yaml": "- file://C.yaml", + "C.yaml": "- file://A.yaml", + } + + def mock_read_text(file_path): + file_name = file_path.name + return mock_files.get(file_name, "") + + def mock_is_file(file_path): + return file_path.name in mock_files + + with self.assertRaises(BadParameter) as exception, patch("pathlib.Path.is_file", new=mock_is_file), patch( + "pathlib.Path.read_text", new=mock_read_text + ): + self.param_type.convert(f"file://A.yaml", None, MagicMock()) + + self.assertIn("Infinite recursion detected in file references", str(exception.exception)) + class TestCfnMetadataType(TestCase): From 56b9c86c9bcfeb8369d0823f414180d3e468d6a5 Mon Sep 17 00:00:00 2001 From: Joe <89984070+sigJoe@users.noreply.github.com> Date: Wed, 8 Oct 2025 08:05:44 -0400 Subject: [PATCH 16/16] Update tests/unit/cli/test_types.py Co-authored-by: Roger Zhang --- tests/unit/cli/test_types.py | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/unit/cli/test_types.py b/tests/unit/cli/test_types.py index 04aba0fa1e9..b65ab3348a4 100644 --- a/tests/unit/cli/test_types.py +++ b/tests/unit/cli/test_types.py @@ -219,7 +219,6 @@ def mock_is_file(file_path): class TestCfnMetadataType(TestCase): - def setUp(self): self.param_type = CfnMetadataType()