From 6c01d2f6fcdd40fb38e071cb9b9dd910d92dd92a Mon Sep 17 00:00:00 2001 From: Garrett Beatty Date: Wed, 16 Sep 2026 19:36:30 +0000 Subject: [PATCH 1/9] Sum DynamoDB ConsumedCapacity across paginated pages aws dynamodb scan/query with auto-pagination previously reported only the final page's ConsumedCapacity, undercounting the true consumed capacity of the operation (CLI-4199). Add an opt-in "aggregate_numeric_keys" paginator directive that recursively sums the numeric leaves of a response member across pages, preserving strings (e.g. TableName) and handling maps keyed by runtime-defined names such as GlobalSecondaryIndexes/LocalSecondaryIndexes. Declare ConsumedCapacity under this directive for Scan and Query. The directive is opt-in per paginator config, so no other service's pagination behavior changes. --- .../next-release/bugfix-dynamodb-49868.json | 5 + .../dynamodb/2012-08-10/paginators-1.json | 4 +- awscli/botocore/paginate.py | 62 ++++++++++ .../botocore/test_paginator_config.py | 3 + tests/functional/dynamodb/test_pagination.py | 109 ++++++++++++++++- tests/unit/botocore/test_paginate.py | 115 ++++++++++++++++++ 6 files changed, 295 insertions(+), 3 deletions(-) create mode 100644 .changes/next-release/bugfix-dynamodb-49868.json diff --git a/.changes/next-release/bugfix-dynamodb-49868.json b/.changes/next-release/bugfix-dynamodb-49868.json new file mode 100644 index 000000000000..9bc06cb55fd6 --- /dev/null +++ b/.changes/next-release/bugfix-dynamodb-49868.json @@ -0,0 +1,5 @@ +{ + "type": "bugfix", + "category": "dynamodb", + "description": "``dynamodb scan`` and ``dynamodb query`` now sum ``ConsumedCapacity`` across all pages when auto-pagination is enabled, instead of reporting only the final page. Includes nested table and secondary-index capacity (fixes CLI-4199)." +} diff --git a/awscli/botocore/data/dynamodb/2012-08-10/paginators-1.json b/awscli/botocore/data/dynamodb/2012-08-10/paginators-1.json index 8e10a0c75675..2a5e19097c11 100644 --- a/awscli/botocore/data/dynamodb/2012-08-10/paginators-1.json +++ b/awscli/botocore/data/dynamodb/2012-08-10/paginators-1.json @@ -21,7 +21,7 @@ "Count", "ScannedCount" ], - "non_aggregate_keys": [ + "aggregate_numeric_keys": [ "ConsumedCapacity" ] }, @@ -34,7 +34,7 @@ "Count", "ScannedCount" ], - "non_aggregate_keys": [ + "aggregate_numeric_keys": [ "ConsumedCapacity" ] }, diff --git a/awscli/botocore/paginate.py b/awscli/botocore/paginate.py index 24889c9600cd..920de876a89a 100644 --- a/awscli/botocore/paginate.py +++ b/awscli/botocore/paginate.py @@ -14,6 +14,7 @@ import base64 import json import logging +from copy import deepcopy from functools import partial from itertools import tee @@ -27,6 +28,31 @@ log = logging.getLogger(__name__) +def _is_summable_number(value): + # Booleans are ints in Python but should never be summed as numbers. + return isinstance(value, (int, float)) and not isinstance(value, bool) + + +def _deep_add_numeric(accumulator, new_value): + """Recursively sum the numeric leaves of ``new_value`` into ``accumulator``. + + This is used to aggregate response members that are dicts of numbers + (and nested dicts of numbers) across paginated responses, for example + DynamoDB's ``ConsumedCapacity``. Numeric leaves are summed, nested dicts + are merged recursively (which handles maps keyed by runtime-defined names + such as ``GlobalSecondaryIndexes``/``LocalSecondaryIndexes``), and any + non-numeric leaves (e.g. ``TableName``) are preserved from the first page + they appear on. Booleans are treated as non-numeric. + """ + for key, value in new_value.items(): + if isinstance(value, dict): + _deep_add_numeric(accumulator.setdefault(key, {}), value) + elif _is_summable_number(value): + accumulator[key] = accumulator.get(key, 0) + value + else: + accumulator.setdefault(key, value) + + class TokenEncoder: """Encodes dictionaries into opaque strings. @@ -201,6 +227,7 @@ def __init__( starting_token, page_size, op_kwargs, + aggregate_numeric_keys=(), ): self._method = method self._input_token = input_token @@ -214,6 +241,7 @@ def __init__( self._op_kwargs = op_kwargs self._resume_token = None self._non_aggregate_key_exprs = non_aggregate_keys + self._aggregate_numeric_keys = aggregate_numeric_keys self._non_aggregate_part = {} self._token_encoder = TokenEncoder() self._token_decoder = TokenDecoder() @@ -480,6 +508,9 @@ def result_key_iters(self): def build_full_result(self): complete_result = {} + # Running totals for members that are aggregated by recursively + # summing their numeric leaves (e.g. DynamoDB's ConsumedCapacity). + aggregate_numeric_totals = {} for response in self: page = response # We want to try to catch operation object pagination @@ -489,6 +520,23 @@ def build_full_result(self): # uses. We can remove it though once operation objects are removed. if isinstance(response, tuple) and len(response) == 2: page = response[1] + for key in self._aggregate_numeric_keys: + page_value = page.get(key) + if page_value is None: + continue + if key not in aggregate_numeric_totals: + aggregate_numeric_totals[key] = deepcopy(page_value) + elif isinstance(page_value, dict) and isinstance( + aggregate_numeric_totals[key], dict + ): + _deep_add_numeric( + aggregate_numeric_totals[key], page_value + ) + elif _is_summable_number(page_value) and _is_summable_number( + aggregate_numeric_totals[key] + ): + aggregate_numeric_totals[key] += page_value + # Any other/unexpected shape: keep the first page's value. # We're incrementally building the full response page # by page. For each page in the response we need to # inject the necessary components from the page @@ -523,6 +571,9 @@ def build_full_result(self): existing_value + result_value, ) merge_dicts(complete_result, self.non_aggregate_part) + # Overlay the recursively-summed totals last so they take precedence + # over any single-page value merged in via the non-aggregate keys. + complete_result.update(aggregate_numeric_totals) if self.resume_token is not None: complete_result['NextToken'] = self.resume_token return complete_result @@ -607,6 +658,9 @@ def __init__(self, method, pagination_config, model): ) self._result_keys = self._get_result_keys(self._pagination_cfg) self._limit_key = self._get_limit_key(self._pagination_cfg) + self._aggregate_numeric_keys = self._get_aggregate_numeric_keys( + self._pagination_cfg + ) @property def result_keys(self): @@ -618,6 +672,13 @@ def _get_non_aggregate_keys(self, config): keys.append(jmespath.compile(key)) return keys + def _get_aggregate_numeric_keys(self, config): + # These are top-level response members whose numeric leaves are + # recursively summed across pages. Unlike ``result_key`` entries they + # may be (possibly nested) dicts, and unlike ``non_aggregate_keys`` + # they are totaled rather than taken from a single page. + return tuple(config.get('aggregate_numeric_keys', [])) + def _get_output_tokens(self, config): output = [] output_token = config['output_token'] @@ -670,6 +731,7 @@ def paginate(self, **kwargs): page_params['StartingToken'], page_params['PageSize'], kwargs, + self._aggregate_numeric_keys, ) def _extract_paging_params(self, kwargs): diff --git a/tests/functional/botocore/test_paginator_config.py b/tests/functional/botocore/test_paginator_config.py index 39c4c2ba04a2..11ac27a95e2d 100644 --- a/tests/functional/botocore/test_paginator_config.py +++ b/tests/functional/botocore/test_paginator_config.py @@ -26,6 +26,7 @@ 'limit_key', 'more_results', 'non_aggregate_keys', + 'aggregate_numeric_keys', ] ) MEMBER_NAME_CHARS = set(string.ascii_letters + string.digits) @@ -325,6 +326,8 @@ def _get_all_page_output_keys(page_config): yield 'more_results', page_config['more_results'] for key in page_config.get('non_aggregate_keys', []): yield 'non_aggregate_keys', key + for key in page_config.get('aggregate_numeric_keys', []): + yield 'aggregate_numeric_keys', key def _get_list_value(page_config, key): diff --git a/tests/functional/dynamodb/test_pagination.py b/tests/functional/dynamodb/test_pagination.py index 41f5f8da5e00..53b6cddf010c 100644 --- a/tests/functional/dynamodb/test_pagination.py +++ b/tests/functional/dynamodb/test_pagination.py @@ -17,7 +17,7 @@ class TestPagination(BaseAWSCommandParamsTest): def setUp(self): - super(TestPagination, self).setUp() + super().setUp() self.first_response = { "Items": [{"Key": {"B": "MjEzNw=="}}], "Count": 1, @@ -47,3 +47,110 @@ def test_pagination_disabled_works(self): stdout, _, _ = self.run_cmd(cmd, expected_rc=0) # Ensure the base64 encoded last evaluated key is in stdout self.assertIn('"MjEzNw=="', stdout) + + +class TestConsumedCapacityAggregation(BaseAWSCommandParamsTest): + """ConsumedCapacity must be summed across pages, not taken from one page. + + See CLI-4199: auto-pagination previously reported only a single page's + ConsumedCapacity, undercounting the true cost of a Scan/Query. + """ + + def _page(self, consumed_capacity, last_key=None): + page = { + "Items": [{"Key": {"S": "item"}}], + "Count": 1, + "ScannedCount": 1, + "ConsumedCapacity": consumed_capacity, + } + if last_key is not None: + page["LastEvaluatedKey"] = last_key + return page + + def test_scan_sums_total_consumed_capacity(self): + self.parsed_responses = [ + self._page( + {"TableName": "T", "CapacityUnits": 100.0}, + last_key={"Key": {"S": "a"}}, + ), + self._page({"TableName": "T", "CapacityUnits": 102.0}), + ] + cmd = ( + 'dynamodb scan --table-name T --output json ' + '--return-consumed-capacity TOTAL' + ) + stdout, _, _ = self.run_cmd(cmd, expected_rc=0) + result = json.loads(stdout) + self.assertEqual(result["ConsumedCapacity"]["CapacityUnits"], 202.0) + self.assertEqual(result["ConsumedCapacity"]["TableName"], "T") + self.assertEqual(result["Count"], 2) + self.assertEqual(result["ScannedCount"], 2) + + def test_scan_sums_index_consumed_capacity(self): + # Index maps are keyed by a user-defined index name; the total must + # sum each index across pages regardless of that name. + self.parsed_responses = [ + self._page( + { + "TableName": "T", + "CapacityUnits": 100.0, + "Table": {"CapacityUnits": 0.0}, + "GlobalSecondaryIndexes": { + "my-index": {"CapacityUnits": 100.0} + }, + }, + last_key={"Key": {"S": "a"}}, + ), + self._page( + { + "TableName": "T", + "CapacityUnits": 102.0, + "Table": {"CapacityUnits": 0.0}, + "GlobalSecondaryIndexes": { + "my-index": {"CapacityUnits": 102.0} + }, + } + ), + ] + cmd = ( + 'dynamodb scan --table-name T --index-name my-index ' + '--output json --return-consumed-capacity INDEXES' + ) + stdout, _, _ = self.run_cmd(cmd, expected_rc=0) + cc = json.loads(stdout)["ConsumedCapacity"] + self.assertEqual(cc["CapacityUnits"], 202.0) + self.assertEqual( + cc["GlobalSecondaryIndexes"]["my-index"]["CapacityUnits"], 202.0 + ) + + def test_query_sums_total_consumed_capacity(self): + self.parsed_responses = [ + self._page( + {"TableName": "T", "CapacityUnits": 5.5}, + last_key={"Key": {"S": "a"}}, + ), + self._page({"TableName": "T", "CapacityUnits": 4.5}), + ] + cmd = ( + 'dynamodb query --table-name T --output json ' + '--key-condition-expression Id=:id ' + '--expression-attribute-values {":id":{"S":"x"}} ' + '--return-consumed-capacity TOTAL' + ) + stdout, _, _ = self.run_cmd(cmd, expected_rc=0) + result = json.loads(stdout) + self.assertEqual(result["ConsumedCapacity"]["CapacityUnits"], 10.0) + + def test_no_consumed_capacity_key_when_not_requested(self): + self.parsed_responses = [ + { + "Items": [{"Key": {"S": "a"}}], + "Count": 1, + "ScannedCount": 1, + "LastEvaluatedKey": {"Key": {"S": "a"}}, + }, + {"Items": [{"Key": {"S": "b"}}], "Count": 1, "ScannedCount": 1}, + ] + cmd = 'dynamodb scan --table-name T --output json' + stdout, _, _ = self.run_cmd(cmd, expected_rc=0) + self.assertNotIn("ConsumedCapacity", json.loads(stdout)) diff --git a/tests/unit/botocore/test_paginate.py b/tests/unit/botocore/test_paginate.py index 8c20681e618a..0549d8c5afbc 100644 --- a/tests/unit/botocore/test_paginate.py +++ b/tests/unit/botocore/test_paginate.py @@ -18,6 +18,7 @@ PaginatorModel, TokenDecoder, TokenEncoder, + _deep_add_numeric, ) from tests import mock, unittest @@ -1621,5 +1622,119 @@ def test_str_page_size(self): self.method.assert_called_with(MaxItems='1') +class TestDeepAddNumeric(unittest.TestCase): + def test_sums_numeric_leaves(self): + acc = {'CapacityUnits': 100.0} + _deep_add_numeric(acc, {'CapacityUnits': 102.5}) + self.assertEqual(acc, {'CapacityUnits': 202.5}) + + def test_recurses_into_nested_dicts(self): + acc = {'Table': {'CapacityUnits': 1.0}} + _deep_add_numeric(acc, {'Table': {'CapacityUnits': 2.0}}) + self.assertEqual(acc, {'Table': {'CapacityUnits': 3.0}}) + + def test_sums_maps_with_runtime_defined_keys(self): + # e.g. GlobalSecondaryIndexes keyed by a user-chosen index name. + acc = {'GlobalSecondaryIndexes': {'my-index': {'CapacityUnits': 5.0}}} + _deep_add_numeric( + acc, + {'GlobalSecondaryIndexes': {'my-index': {'CapacityUnits': 7.0}}}, + ) + self.assertEqual( + acc, + {'GlobalSecondaryIndexes': {'my-index': {'CapacityUnits': 12.0}}}, + ) + + def test_preserves_strings(self): + acc = {'TableName': 'T'} + _deep_add_numeric(acc, {'TableName': 'T'}) + self.assertEqual(acc, {'TableName': 'T'}) + + def test_does_not_sum_booleans(self): + acc = {'Flag': True} + _deep_add_numeric(acc, {'Flag': True}) + self.assertEqual(acc, {'Flag': True}) + + def test_adds_new_keys_from_later_pages(self): + acc = {'CapacityUnits': 1.0} + _deep_add_numeric(acc, {'CapacityUnits': 1.0, 'TableName': 'T'}) + self.assertEqual(acc, {'CapacityUnits': 2.0, 'TableName': 'T'}) + + +class TestAggregateNumericKeys(unittest.TestCase): + def setUp(self): + self.method = mock.Mock() + self.model = mock.Mock() + self.paginate_config = { + 'output_token': 'NextToken', + 'input_token': 'NextToken', + 'result_key': 'Items', + 'aggregate_numeric_keys': ['ConsumedCapacity'], + } + self.paginator = Paginator( + self.method, self.paginate_config, self.model + ) + + def test_config_parsed(self): + self.assertEqual( + self.paginator._aggregate_numeric_keys, ('ConsumedCapacity',) + ) + + def test_sums_across_pages(self): + self.method.side_effect = [ + { + 'Items': ['a'], + 'ConsumedCapacity': { + 'TableName': 'T', + 'CapacityUnits': 100.0, + 'GlobalSecondaryIndexes': { + 'my-index': {'CapacityUnits': 100.0} + }, + }, + 'NextToken': 'tok', + }, + { + 'Items': ['b'], + 'ConsumedCapacity': { + 'TableName': 'T', + 'CapacityUnits': 102.0, + 'GlobalSecondaryIndexes': { + 'my-index': {'CapacityUnits': 102.0} + }, + }, + }, + ] + result = self.paginator.paginate().build_full_result() + self.assertEqual(result['Items'], ['a', 'b']) + cc = result['ConsumedCapacity'] + self.assertEqual(cc['CapacityUnits'], 202.0) + self.assertEqual(cc['TableName'], 'T') + self.assertEqual( + cc['GlobalSecondaryIndexes']['my-index']['CapacityUnits'], 202.0 + ) + + def test_absent_when_never_returned(self): + self.method.side_effect = [ + {'Items': ['a'], 'NextToken': 'tok'}, + {'Items': ['b']}, + ] + result = self.paginator.paginate().build_full_result() + self.assertNotIn('ConsumedCapacity', result) + + def test_does_not_mutate_source_pages(self): + page = { + 'Items': ['a'], + 'ConsumedCapacity': {'CapacityUnits': 100.0}, + 'NextToken': 'tok', + } + self.method.side_effect = [ + page, + {'Items': ['b'], 'ConsumedCapacity': {'CapacityUnits': 2.0}}, + ] + self.paginator.paginate().build_full_result() + # The first page's dict must be left untouched (deepcopy on first sight). + self.assertEqual(page['ConsumedCapacity']['CapacityUnits'], 100.0) + + if __name__ == '__main__': unittest.main() From ef6a0d4b42308e3dcf26ab1d41c6c4b49faebdfe Mon Sep 17 00:00:00 2001 From: Garrett Beatty Date: Wed, 16 Sep 2026 19:51:46 +0000 Subject: [PATCH 2/9] Move DynamoDB ConsumedCapacity paginator config to sdk-extras overlay paginators-1.json is regenerated from upstream models, so hand edits there are clobbered on the next sync. Move the ConsumedCapacity aggregation config into a paginators-1.sdk-extras.json overlay (deep-merged at load time), matching the convention used across other services. This restores the generated paginators-1.json to its original state and durably (a) clears the single-page non_aggregate_keys entry and (b) declares aggregate_numeric_keys. --- .../data/dynamodb/2012-08-10/paginators-1.json | 4 ++-- .../2012-08-10/paginators-1.sdk-extras.json | 18 ++++++++++++++++++ 2 files changed, 20 insertions(+), 2 deletions(-) create mode 100644 awscli/botocore/data/dynamodb/2012-08-10/paginators-1.sdk-extras.json diff --git a/awscli/botocore/data/dynamodb/2012-08-10/paginators-1.json b/awscli/botocore/data/dynamodb/2012-08-10/paginators-1.json index 2a5e19097c11..8e10a0c75675 100644 --- a/awscli/botocore/data/dynamodb/2012-08-10/paginators-1.json +++ b/awscli/botocore/data/dynamodb/2012-08-10/paginators-1.json @@ -21,7 +21,7 @@ "Count", "ScannedCount" ], - "aggregate_numeric_keys": [ + "non_aggregate_keys": [ "ConsumedCapacity" ] }, @@ -34,7 +34,7 @@ "Count", "ScannedCount" ], - "aggregate_numeric_keys": [ + "non_aggregate_keys": [ "ConsumedCapacity" ] }, diff --git a/awscli/botocore/data/dynamodb/2012-08-10/paginators-1.sdk-extras.json b/awscli/botocore/data/dynamodb/2012-08-10/paginators-1.sdk-extras.json new file mode 100644 index 000000000000..dbfc40950964 --- /dev/null +++ b/awscli/botocore/data/dynamodb/2012-08-10/paginators-1.sdk-extras.json @@ -0,0 +1,18 @@ +{ + "merge": { + "pagination": { + "Query": { + "non_aggregate_keys": [], + "aggregate_numeric_keys": [ + "ConsumedCapacity" + ] + }, + "Scan": { + "non_aggregate_keys": [], + "aggregate_numeric_keys": [ + "ConsumedCapacity" + ] + } + } + } +} From 1d0f6d0f2b5008a9dc2c3c5f5897404f47e036d2 Mon Sep 17 00:00:00 2001 From: Garrett Beatty Date: Wed, 16 Sep 2026 19:59:00 +0000 Subject: [PATCH 3/9] Harden _deep_add_numeric against cross-page type mismatches If a response member's type differed across pages (e.g. a number where an earlier page had a string, dict, or None), _deep_add_numeric could raise mid-pagination. Guard the recurse/add paths by checking the accumulator's existing type: on mismatch, preserve the first page's value (matching the documented behavior). Also deep-copy dict leaves introduced by later pages so the aggregate never aliases a source response. Does not affect DynamoDB ConsumedCapacity (its shape is stable across pages), but makes the general aggregate_numeric_keys directive robust. --- awscli/botocore/paginate.py | 19 +++++++++++++------ tests/unit/botocore/test_paginate.py | 28 ++++++++++++++++++++++++++++ 2 files changed, 41 insertions(+), 6 deletions(-) diff --git a/awscli/botocore/paginate.py b/awscli/botocore/paginate.py index 920de876a89a..653b6113e4a3 100644 --- a/awscli/botocore/paginate.py +++ b/awscli/botocore/paginate.py @@ -45,12 +45,19 @@ def _deep_add_numeric(accumulator, new_value): they appear on. Booleans are treated as non-numeric. """ for key, value in new_value.items(): - if isinstance(value, dict): - _deep_add_numeric(accumulator.setdefault(key, {}), value) - elif _is_summable_number(value): - accumulator[key] = accumulator.get(key, 0) + value - else: - accumulator.setdefault(key, value) + if key not in accumulator: + # First time we've seen this leaf; take it as-is. + accumulator[key] = ( + deepcopy(value) if isinstance(value, dict) else value + ) + elif isinstance(value, dict) and isinstance(accumulator[key], dict): + _deep_add_numeric(accumulator[key], value) + elif _is_summable_number(value) and _is_summable_number( + accumulator[key] + ): + accumulator[key] = accumulator[key] + value + # Any type mismatch across pages (e.g. a number where an earlier page + # had a string/dict/None): keep the first page's value. class TokenEncoder: diff --git a/tests/unit/botocore/test_paginate.py b/tests/unit/botocore/test_paginate.py index 0549d8c5afbc..9470be37bb1a 100644 --- a/tests/unit/botocore/test_paginate.py +++ b/tests/unit/botocore/test_paginate.py @@ -1660,6 +1660,34 @@ def test_adds_new_keys_from_later_pages(self): _deep_add_numeric(acc, {'CapacityUnits': 1.0, 'TableName': 'T'}) self.assertEqual(acc, {'CapacityUnits': 2.0, 'TableName': 'T'}) + def test_deep_copies_new_dict_leaves(self): + # A dict leaf introduced by a later page must not alias the source. + source = {'CapacityUnits': 1.0} + acc = {} + _deep_add_numeric(acc, {'Index': source}) + acc['Index']['CapacityUnits'] += 5.0 + self.assertEqual(source['CapacityUnits'], 1.0) + + def test_type_mismatch_number_then_dict_keeps_first(self): + acc = {'k': 5.0} + _deep_add_numeric(acc, {'k': {'CapacityUnits': 1.0}}) + self.assertEqual(acc, {'k': 5.0}) + + def test_type_mismatch_string_then_number_keeps_first(self): + acc = {'k': 'T'} + _deep_add_numeric(acc, {'k': 3.0}) + self.assertEqual(acc, {'k': 'T'}) + + def test_type_mismatch_none_then_number_keeps_first(self): + acc = {'k': None} + _deep_add_numeric(acc, {'k': 3.0}) + self.assertEqual(acc, {'k': None}) + + def test_type_mismatch_none_then_dict_keeps_first(self): + acc = {'k': None} + _deep_add_numeric(acc, {'k': {'x': 1.0}}) + self.assertEqual(acc, {'k': None}) + class TestAggregateNumericKeys(unittest.TestCase): def setUp(self): From 96a5cd4ba5613ff2335bd76b445f542c710f2232 Mon Sep 17 00:00:00 2001 From: Garrett Beatty Date: Wed, 16 Sep 2026 20:31:59 +0000 Subject: [PATCH 4/9] Let aggregate_numeric_keys supersede non_aggregate_keys for a member Rather than overriding the (upstream-synced) non_aggregate_keys list in the overlay, treat a member declared in aggregate_numeric_keys as taking precedence: it is filtered out of non_aggregate handling in the Paginator. This keeps the overlay purely additive, so the upstream non_aggregate_keys list can keep receiving unrelated additions without being zeroed out. The paginator config linter is updated to mirror this precedence (a member aggregated across pages is only accounted for once), leaving the existing output-member validation intact. --- .../2012-08-10/paginators-1.sdk-extras.json | 2 - awscli/botocore/paginate.py | 23 +++++++-- .../botocore/test_paginator_config.py | 12 ++++- tests/unit/botocore/test_paginate.py | 50 +++++++++++++++++++ 4 files changed, 81 insertions(+), 6 deletions(-) diff --git a/awscli/botocore/data/dynamodb/2012-08-10/paginators-1.sdk-extras.json b/awscli/botocore/data/dynamodb/2012-08-10/paginators-1.sdk-extras.json index dbfc40950964..cdfcf094f20b 100644 --- a/awscli/botocore/data/dynamodb/2012-08-10/paginators-1.sdk-extras.json +++ b/awscli/botocore/data/dynamodb/2012-08-10/paginators-1.sdk-extras.json @@ -2,13 +2,11 @@ "merge": { "pagination": { "Query": { - "non_aggregate_keys": [], "aggregate_numeric_keys": [ "ConsumedCapacity" ] }, "Scan": { - "non_aggregate_keys": [], "aggregate_numeric_keys": [ "ConsumedCapacity" ] diff --git a/awscli/botocore/paginate.py b/awscli/botocore/paginate.py index 653b6113e4a3..21a5cc2ffa9e 100644 --- a/awscli/botocore/paginate.py +++ b/awscli/botocore/paginate.py @@ -660,14 +660,14 @@ def __init__(self, method, pagination_config, model): self._output_token = self._get_output_tokens(self._pagination_cfg) self._input_token = self._get_input_tokens(self._pagination_cfg) self._more_results = self._get_more_results_token(self._pagination_cfg) + self._aggregate_numeric_keys = self._get_aggregate_numeric_keys( + self._pagination_cfg + ) self._non_aggregate_keys = self._get_non_aggregate_keys( self._pagination_cfg ) self._result_keys = self._get_result_keys(self._pagination_cfg) self._limit_key = self._get_limit_key(self._pagination_cfg) - self._aggregate_numeric_keys = self._get_aggregate_numeric_keys( - self._pagination_cfg - ) @property def result_keys(self): @@ -676,9 +676,26 @@ def result_keys(self): def _get_non_aggregate_keys(self, config): keys = [] for key in config.get('non_aggregate_keys', []): + # A member that is aggregated across pages takes precedence over + # any non-aggregate declaration for the same member (or a path + # nested under it). This lets a member be moved to + # aggregate_numeric_keys via an overlay without having to edit the + # upstream-synced non_aggregate_keys list (and lets that list keep + # receiving unrelated upstream additions). + if self._is_aggregated(key): + continue keys.append(jmespath.compile(key)) return keys + def _is_aggregated(self, non_aggregate_key): + for aggregate_key in self._aggregate_numeric_keys: + if ( + non_aggregate_key == aggregate_key + or non_aggregate_key.startswith(f'{aggregate_key}.') + ): + return True + return False + def _get_aggregate_numeric_keys(self, config): # These are top-level response members whose numeric leaves are # recursively summed across pages. Unlike ``result_key`` entries they diff --git a/tests/functional/botocore/test_paginator_config.py b/tests/functional/botocore/test_paginator_config.py index 11ac27a95e2d..b057396f1c3c 100644 --- a/tests/functional/botocore/test_paginator_config.py +++ b/tests/functional/botocore/test_paginator_config.py @@ -324,9 +324,19 @@ def _get_all_page_output_keys(page_config): yield 'output_token', key if 'more_results' in page_config: yield 'more_results', page_config['more_results'] + aggregate_numeric_keys = page_config.get('aggregate_numeric_keys', []) for key in page_config.get('non_aggregate_keys', []): + # A member declared under aggregate_numeric_keys is aggregated across + # pages and takes precedence over any non_aggregate declaration for the + # same member (mirroring Paginator._get_non_aggregate_keys). Skip it + # here so it is only accounted for once. + if any( + key == agg or key.startswith(f'{agg}.') + for agg in aggregate_numeric_keys + ): + continue yield 'non_aggregate_keys', key - for key in page_config.get('aggregate_numeric_keys', []): + for key in aggregate_numeric_keys: yield 'aggregate_numeric_keys', key diff --git a/tests/unit/botocore/test_paginate.py b/tests/unit/botocore/test_paginate.py index 9470be37bb1a..1f7204f38f78 100644 --- a/tests/unit/botocore/test_paginate.py +++ b/tests/unit/botocore/test_paginate.py @@ -1708,6 +1708,56 @@ def test_config_parsed(self): self.paginator._aggregate_numeric_keys, ('ConsumedCapacity',) ) + def test_aggregated_key_dropped_from_non_aggregate_keys(self): + # A member in both lists is aggregated; the non_aggregate declaration + # for it (and any other non_aggregate members) is handled correctly. + config = { + 'output_token': 'NextToken', + 'input_token': 'NextToken', + 'result_key': 'Items', + 'non_aggregate_keys': ['ConsumedCapacity', 'SomethingElse'], + 'aggregate_numeric_keys': ['ConsumedCapacity'], + } + paginator = Paginator(self.method, config, self.model) + kept = [k.expression for k in paginator._non_aggregate_keys] + # ConsumedCapacity is aggregated, so it must not be treated as + # non-aggregate; unrelated members are preserved. + self.assertEqual(kept, ['SomethingElse']) + + def test_aggregated_key_drops_nested_non_aggregate_paths(self): + config = { + 'output_token': 'NextToken', + 'input_token': 'NextToken', + 'result_key': 'Items', + 'non_aggregate_keys': ['ConsumedCapacity.TableName', 'Other'], + 'aggregate_numeric_keys': ['ConsumedCapacity'], + } + paginator = Paginator(self.method, config, self.model) + kept = [k.expression for k in paginator._non_aggregate_keys] + self.assertEqual(kept, ['Other']) + + def test_aggregation_wins_when_member_in_both_lists(self): + # Even with ConsumedCapacity declared non-aggregate, the result is the + # cross-page sum, not a single page's value. + config = { + 'output_token': 'NextToken', + 'input_token': 'NextToken', + 'result_key': 'Items', + 'non_aggregate_keys': ['ConsumedCapacity'], + 'aggregate_numeric_keys': ['ConsumedCapacity'], + } + paginator = Paginator(self.method, config, self.model) + self.method.side_effect = [ + { + 'Items': ['a'], + 'ConsumedCapacity': {'CapacityUnits': 100.0}, + 'NextToken': 'tok', + }, + {'Items': ['b'], 'ConsumedCapacity': {'CapacityUnits': 102.0}}, + ] + result = paginator.paginate().build_full_result() + self.assertEqual(result['ConsumedCapacity']['CapacityUnits'], 202.0) + def test_sums_across_pages(self): self.method.side_effect = [ { From 40185cb0091d4c84bae27cd9aef8b69a2c716584 Mon Sep 17 00:00:00 2001 From: Garrett Beatty Date: Wed, 16 Sep 2026 22:18:02 +0000 Subject: [PATCH 5/9] Address Copilot review: numeric type coverage, list aliasing, validation - _is_summable_number now uses numbers.Number (not just int/float) so decimal.Decimal values aggregate; still excludes bool. (numbers.Real is intentionally NOT used: Decimal registers as Number but not Real.) - Deep-copy list leaves (not just dict) when first inserted so the aggregate never aliases a source page. - Config linter rejects nested/dotted aggregate_numeric_keys entries, which would silently never aggregate at runtime. - Add unit tests for scalar/Decimal/boolean aggregate members and list-leaf deep-copy. --- awscli/botocore/paginate.py | 11 +++-- .../botocore/test_paginator_config.py | 14 ++++++ tests/unit/botocore/test_paginate.py | 45 +++++++++++++++++++ 3 files changed, 67 insertions(+), 3 deletions(-) diff --git a/awscli/botocore/paginate.py b/awscli/botocore/paginate.py index 21a5cc2ffa9e..5e1eaa9d0805 100644 --- a/awscli/botocore/paginate.py +++ b/awscli/botocore/paginate.py @@ -14,6 +14,7 @@ import base64 import json import logging +import numbers from copy import deepcopy from functools import partial from itertools import tee @@ -29,8 +30,11 @@ def _is_summable_number(value): + # Accept any number (int, float, and decimal.Decimal, which DynamoDB + # numbers can be parsed as). numbers.Number is used rather than + # numbers.Real because Decimal registers as Number but NOT as Real. # Booleans are ints in Python but should never be summed as numbers. - return isinstance(value, (int, float)) and not isinstance(value, bool) + return isinstance(value, numbers.Number) and not isinstance(value, bool) def _deep_add_numeric(accumulator, new_value): @@ -46,9 +50,10 @@ def _deep_add_numeric(accumulator, new_value): """ for key, value in new_value.items(): if key not in accumulator: - # First time we've seen this leaf; take it as-is. + # First time we've seen this leaf. Deep-copy mutable containers so + # the aggregate never aliases (and later mutates) a source page. accumulator[key] = ( - deepcopy(value) if isinstance(value, dict) else value + deepcopy(value) if isinstance(value, (dict, list)) else value ) elif isinstance(value, dict) and isinstance(accumulator[key], dict): _deep_add_numeric(accumulator[key], value) diff --git a/tests/functional/botocore/test_paginator_config.py b/tests/functional/botocore/test_paginator_config.py index b057396f1c3c..5885e244bafc 100644 --- a/tests/functional/botocore/test_paginator_config.py +++ b/tests/functional/botocore/test_paginator_config.py @@ -171,6 +171,20 @@ def test_lint_pagination_configs( _validate_new_numeric_keys( operation_name, page_config, service_model, record_property ) + _validate_aggregate_numeric_keys(operation_name, page_config) + + +def _validate_aggregate_numeric_keys(operation_name, page_config): + # aggregate_numeric_keys must be top-level output member names. A nested + # path (e.g. "ConsumedCapacity.Table") would silently never aggregate at + # runtime (build_full_result uses page.get(key)), so reject it here. + for key in page_config.get('aggregate_numeric_keys', []): + if '.' in key: + raise AssertionError( + f"aggregate_numeric_keys entry '{key}' for operation " + f"{operation_name} must be a top-level output member name, " + "not a nested path." + ) def _validate_known_pagination_keys(page_config): diff --git a/tests/unit/botocore/test_paginate.py b/tests/unit/botocore/test_paginate.py index 1f7204f38f78..5f05072a600e 100644 --- a/tests/unit/botocore/test_paginate.py +++ b/tests/unit/botocore/test_paginate.py @@ -11,6 +11,8 @@ # ANY KIND, either express or implied. See the License for the specific # language governing permissions and limitations under the License. +from decimal import Decimal + from botocore import model from botocore.exceptions import PaginationError from botocore.paginate import ( @@ -1655,6 +1657,19 @@ def test_does_not_sum_booleans(self): _deep_add_numeric(acc, {'Flag': True}) self.assertEqual(acc, {'Flag': True}) + def test_sums_decimal_values(self): + acc = {'CapacityUnits': Decimal('100.5')} + _deep_add_numeric(acc, {'CapacityUnits': Decimal('101.5')}) + self.assertEqual(acc, {'CapacityUnits': Decimal('202.0')}) + + def test_deep_copies_new_list_leaves(self): + # A list leaf introduced by a later page must not alias the source. + source = ['a'] + acc = {} + _deep_add_numeric(acc, {'Names': source}) + acc['Names'].append('b') + self.assertEqual(source, ['a']) + def test_adds_new_keys_from_later_pages(self): acc = {'CapacityUnits': 1.0} _deep_add_numeric(acc, {'CapacityUnits': 1.0, 'TableName': 'T'}) @@ -1791,6 +1806,36 @@ def test_sums_across_pages(self): cc['GlobalSecondaryIndexes']['my-index']['CapacityUnits'], 202.0 ) + def test_sums_scalar_member_across_pages(self): + # A top-level scalar (non-dict) aggregate member is summed too. + self.method.side_effect = [ + {'Items': ['a'], 'ConsumedCapacity': 100.0, 'NextToken': 'tok'}, + {'Items': ['b'], 'ConsumedCapacity': 102.0}, + ] + result = self.paginator.paginate().build_full_result() + self.assertEqual(result['ConsumedCapacity'], 202.0) + + def test_sums_scalar_decimal_member_across_pages(self): + self.method.side_effect = [ + { + 'Items': ['a'], + 'ConsumedCapacity': Decimal('1.5'), + 'NextToken': 'tok', + }, + {'Items': ['b'], 'ConsumedCapacity': Decimal('2.5')}, + ] + result = self.paginator.paginate().build_full_result() + self.assertEqual(result['ConsumedCapacity'], Decimal('4.0')) + + def test_does_not_sum_scalar_boolean_member(self): + # A boolean must never be summed; keep the first page's value. + self.method.side_effect = [ + {'Items': ['a'], 'ConsumedCapacity': True, 'NextToken': 'tok'}, + {'Items': ['b'], 'ConsumedCapacity': True}, + ] + result = self.paginator.paginate().build_full_result() + self.assertIs(result['ConsumedCapacity'], True) + def test_absent_when_never_returned(self): self.method.side_effect = [ {'Items': ['a'], 'NextToken': 'tok'}, From 3bc5835c2581e1fa0ba7953c686eba5f8540a9e1 Mon Sep 17 00:00:00 2001 From: Garrett Beatty Date: Thu, 17 Sep 2026 13:40:03 +0000 Subject: [PATCH 6/9] Keep aggregate numeric check aligned with existing (int, float) logic Use the same numeric types as the existing result_key aggregation (int, float) in _is_summable_number rather than numbers.Number. This keeps the two aggregation paths consistent and avoids the float+Decimal addition edge case (so no _safe_numeric_add guard is needed). Strings and booleans are still excluded, which our deep-sum semantics require (preserve TableName, never sum booleans). ConsumedCapacity.CapacityUnits is a double (float), so dropping Decimal handling has no practical effect. --- awscli/botocore/paginate.py | 12 ++++++------ tests/unit/botocore/test_paginate.py | 19 ------------------- 2 files changed, 6 insertions(+), 25 deletions(-) diff --git a/awscli/botocore/paginate.py b/awscli/botocore/paginate.py index 5e1eaa9d0805..65aefaf157b6 100644 --- a/awscli/botocore/paginate.py +++ b/awscli/botocore/paginate.py @@ -14,7 +14,6 @@ import base64 import json import logging -import numbers from copy import deepcopy from functools import partial from itertools import tee @@ -30,11 +29,12 @@ def _is_summable_number(value): - # Accept any number (int, float, and decimal.Decimal, which DynamoDB - # numbers can be parsed as). numbers.Number is used rather than - # numbers.Real because Decimal registers as Number but NOT as Real. - # Booleans are ints in Python but should never be summed as numbers. - return isinstance(value, numbers.Number) and not isinstance(value, bool) + # Match the numeric types used by the existing result_key aggregation + # (int, float). Strings are intentionally excluded here (unlike that path, + # which concatenates them) so string leaves such as TableName are kept + # from the first page rather than concatenated. Booleans are ints in + # Python but should never be summed as numbers. + return isinstance(value, (int, float)) and not isinstance(value, bool) def _deep_add_numeric(accumulator, new_value): diff --git a/tests/unit/botocore/test_paginate.py b/tests/unit/botocore/test_paginate.py index 5f05072a600e..e50ed220990c 100644 --- a/tests/unit/botocore/test_paginate.py +++ b/tests/unit/botocore/test_paginate.py @@ -11,8 +11,6 @@ # ANY KIND, either express or implied. See the License for the specific # language governing permissions and limitations under the License. -from decimal import Decimal - from botocore import model from botocore.exceptions import PaginationError from botocore.paginate import ( @@ -1657,11 +1655,6 @@ def test_does_not_sum_booleans(self): _deep_add_numeric(acc, {'Flag': True}) self.assertEqual(acc, {'Flag': True}) - def test_sums_decimal_values(self): - acc = {'CapacityUnits': Decimal('100.5')} - _deep_add_numeric(acc, {'CapacityUnits': Decimal('101.5')}) - self.assertEqual(acc, {'CapacityUnits': Decimal('202.0')}) - def test_deep_copies_new_list_leaves(self): # A list leaf introduced by a later page must not alias the source. source = ['a'] @@ -1815,18 +1808,6 @@ def test_sums_scalar_member_across_pages(self): result = self.paginator.paginate().build_full_result() self.assertEqual(result['ConsumedCapacity'], 202.0) - def test_sums_scalar_decimal_member_across_pages(self): - self.method.side_effect = [ - { - 'Items': ['a'], - 'ConsumedCapacity': Decimal('1.5'), - 'NextToken': 'tok', - }, - {'Items': ['b'], 'ConsumedCapacity': Decimal('2.5')}, - ] - result = self.paginator.paginate().build_full_result() - self.assertEqual(result['ConsumedCapacity'], Decimal('4.0')) - def test_does_not_sum_scalar_boolean_member(self): # A boolean must never be summed; keep the first page's value. self.method.side_effect = [ From 272070d7044f8c7c689c800368f1cfbeeebf2c09 Mon Sep 17 00:00:00 2001 From: Garrett Beatty Date: Thu, 17 Sep 2026 15:41:50 +0000 Subject: [PATCH 7/9] Make aggregate_numeric_keys an explicit leaf-name allowlist Previously the directive summed every numeric leaf under a member, which would auto-aggregate any future numeric field DynamoDB might add under ConsumedCapacity that should not be summed. Change the directive to a map of member -> list of summable leaf field-names, so only named leaves (CapacityUnits, ReadCapacityUnits, WriteCapacityUnits) are totaled. Dynamic-key maps (GlobalSecondaryIndexes/LocalSecondaryIndexes) still work because matching is by leaf name during the recursive walk; any non-allowlisted numeric leaf is preserved from the first page. --- .../2012-08-10/paginators-1.sdk-extras.json | 20 ++-- awscli/botocore/paginate.py | 52 +++++++---- .../botocore/test_paginator_config.py | 27 ++++-- tests/unit/botocore/test_paginate.py | 92 ++++++++++++++----- 4 files changed, 138 insertions(+), 53 deletions(-) diff --git a/awscli/botocore/data/dynamodb/2012-08-10/paginators-1.sdk-extras.json b/awscli/botocore/data/dynamodb/2012-08-10/paginators-1.sdk-extras.json index cdfcf094f20b..e8a24a72681f 100644 --- a/awscli/botocore/data/dynamodb/2012-08-10/paginators-1.sdk-extras.json +++ b/awscli/botocore/data/dynamodb/2012-08-10/paginators-1.sdk-extras.json @@ -2,14 +2,22 @@ "merge": { "pagination": { "Query": { - "aggregate_numeric_keys": [ - "ConsumedCapacity" - ] + "aggregate_numeric_keys": { + "ConsumedCapacity": [ + "CapacityUnits", + "ReadCapacityUnits", + "WriteCapacityUnits" + ] + } }, "Scan": { - "aggregate_numeric_keys": [ - "ConsumedCapacity" - ] + "aggregate_numeric_keys": { + "ConsumedCapacity": [ + "CapacityUnits", + "ReadCapacityUnits", + "WriteCapacityUnits" + ] + } } } } diff --git a/awscli/botocore/paginate.py b/awscli/botocore/paginate.py index 65aefaf157b6..6394be24ffb2 100644 --- a/awscli/botocore/paginate.py +++ b/awscli/botocore/paginate.py @@ -37,16 +37,19 @@ def _is_summable_number(value): return isinstance(value, (int, float)) and not isinstance(value, bool) -def _deep_add_numeric(accumulator, new_value): - """Recursively sum the numeric leaves of ``new_value`` into ``accumulator``. +def _deep_add_numeric(accumulator, new_value, summable_leaf_names): + """Recursively sum selected numeric leaves of ``new_value`` into ``accumulator``. This is used to aggregate response members that are dicts of numbers (and nested dicts of numbers) across paginated responses, for example - DynamoDB's ``ConsumedCapacity``. Numeric leaves are summed, nested dicts - are merged recursively (which handles maps keyed by runtime-defined names - such as ``GlobalSecondaryIndexes``/``LocalSecondaryIndexes``), and any - non-numeric leaves (e.g. ``TableName``) are preserved from the first page - they appear on. Booleans are treated as non-numeric. + DynamoDB's ``ConsumedCapacity``. Nested dicts are merged recursively (which + handles maps keyed by runtime-defined names such as + ``GlobalSecondaryIndexes``/``LocalSecondaryIndexes``). + + Only numeric leaves whose key is in ``summable_leaf_names`` are summed. This + is an allowlist: any other leaf (a string like ``TableName``, or a numeric + field not named in the allowlist) is preserved from the first page it + appears on, rather than being auto-aggregated. Booleans are never summed. """ for key, value in new_value.items(): if key not in accumulator: @@ -56,13 +59,15 @@ def _deep_add_numeric(accumulator, new_value): deepcopy(value) if isinstance(value, (dict, list)) else value ) elif isinstance(value, dict) and isinstance(accumulator[key], dict): - _deep_add_numeric(accumulator[key], value) - elif _is_summable_number(value) and _is_summable_number( - accumulator[key] + _deep_add_numeric(accumulator[key], value, summable_leaf_names) + elif ( + key in summable_leaf_names + and _is_summable_number(value) + and _is_summable_number(accumulator[key]) ): accumulator[key] = accumulator[key] + value - # Any type mismatch across pages (e.g. a number where an earlier page - # had a string/dict/None): keep the first page's value. + # Everything else (non-allowlisted numeric leaf, string, or cross-page + # type mismatch): keep the first page's value. class TokenEncoder: @@ -532,7 +537,7 @@ def build_full_result(self): # uses. We can remove it though once operation objects are removed. if isinstance(response, tuple) and len(response) == 2: page = response[1] - for key in self._aggregate_numeric_keys: + for key, leaf_names in self._aggregate_numeric_keys.items(): page_value = page.get(key) if page_value is None: continue @@ -542,11 +547,13 @@ def build_full_result(self): aggregate_numeric_totals[key], dict ): _deep_add_numeric( - aggregate_numeric_totals[key], page_value + aggregate_numeric_totals[key], page_value, leaf_names ) elif _is_summable_number(page_value) and _is_summable_number( aggregate_numeric_totals[key] ): + # The whole member is a bare number (not a dict). Opting the + # member in via config is itself the allowlist decision. aggregate_numeric_totals[key] += page_value # Any other/unexpected shape: keep the first page's value. # We're incrementally building the full response page @@ -702,11 +709,18 @@ def _is_aggregated(self, non_aggregate_key): return False def _get_aggregate_numeric_keys(self, config): - # These are top-level response members whose numeric leaves are - # recursively summed across pages. Unlike ``result_key`` entries they - # may be (possibly nested) dicts, and unlike ``non_aggregate_keys`` - # they are totaled rather than taken from a single page. - return tuple(config.get('aggregate_numeric_keys', [])) + # Maps a top-level response member to the set of leaf field-names whose + # numeric values are summed (recursively, so nested/dynamic-key maps + # like GlobalSecondaryIndexes are covered). This is an allowlist: only + # the named leaves are totaled; any other leaf (strings, or numbers not + # named here) is preserved from the first page rather than summed. + # Unlike ``result_key`` these members may be (nested) dicts, and unlike + # ``non_aggregate_keys`` the named leaves are totaled across pages. + config_value = config.get('aggregate_numeric_keys', {}) + return { + member: frozenset(leaf_names) + for member, leaf_names in config_value.items() + } def _get_output_tokens(self, config): output = [] diff --git a/tests/functional/botocore/test_paginator_config.py b/tests/functional/botocore/test_paginator_config.py index 5885e244bafc..7b5028d56c78 100644 --- a/tests/functional/botocore/test_paginator_config.py +++ b/tests/functional/botocore/test_paginator_config.py @@ -175,16 +175,31 @@ def test_lint_pagination_configs( def _validate_aggregate_numeric_keys(operation_name, page_config): - # aggregate_numeric_keys must be top-level output member names. A nested - # path (e.g. "ConsumedCapacity.Table") would silently never aggregate at - # runtime (build_full_result uses page.get(key)), so reject it here. - for key in page_config.get('aggregate_numeric_keys', []): - if '.' in key: + # aggregate_numeric_keys maps a top-level output member name to a list of + # leaf field-names to sum. The member must be a top-level member (a nested + # path like "ConsumedCapacity.Table" would silently never aggregate at + # runtime, since build_full_result uses page.get(member)); leaf names are + # matched by key anywhere in the subtree, so they must be bare field names. + config_value = page_config.get('aggregate_numeric_keys', {}) + if not isinstance(config_value, dict): + raise AssertionError( + f"aggregate_numeric_keys for operation {operation_name} must be a " + "map of member name -> list of leaf field-names." + ) + for member, leaf_names in config_value.items(): + if '.' in member: raise AssertionError( - f"aggregate_numeric_keys entry '{key}' for operation " + f"aggregate_numeric_keys member '{member}' for operation " f"{operation_name} must be a top-level output member name, " "not a nested path." ) + if not isinstance(leaf_names, list) or not all( + isinstance(n, str) and '.' not in n for n in leaf_names + ): + raise AssertionError( + f"aggregate_numeric_keys['{member}'] for operation " + f"{operation_name} must be a list of bare leaf field-names." + ) def _validate_known_pagination_keys(page_config): diff --git a/tests/unit/botocore/test_paginate.py b/tests/unit/botocore/test_paginate.py index e50ed220990c..8cc74c190327 100644 --- a/tests/unit/botocore/test_paginate.py +++ b/tests/unit/botocore/test_paginate.py @@ -1623,14 +1623,18 @@ def test_str_page_size(self): class TestDeepAddNumeric(unittest.TestCase): + LEAVES = frozenset( + {'CapacityUnits', 'ReadCapacityUnits', 'WriteCapacityUnits'} + ) + def test_sums_numeric_leaves(self): acc = {'CapacityUnits': 100.0} - _deep_add_numeric(acc, {'CapacityUnits': 102.5}) + _deep_add_numeric(acc, {'CapacityUnits': 102.5}, self.LEAVES) self.assertEqual(acc, {'CapacityUnits': 202.5}) def test_recurses_into_nested_dicts(self): acc = {'Table': {'CapacityUnits': 1.0}} - _deep_add_numeric(acc, {'Table': {'CapacityUnits': 2.0}}) + _deep_add_numeric(acc, {'Table': {'CapacityUnits': 2.0}}, self.LEAVES) self.assertEqual(acc, {'Table': {'CapacityUnits': 3.0}}) def test_sums_maps_with_runtime_defined_keys(self): @@ -1639,6 +1643,7 @@ def test_sums_maps_with_runtime_defined_keys(self): _deep_add_numeric( acc, {'GlobalSecondaryIndexes': {'my-index': {'CapacityUnits': 7.0}}}, + self.LEAVES, ) self.assertEqual( acc, @@ -1647,53 +1652,63 @@ def test_sums_maps_with_runtime_defined_keys(self): def test_preserves_strings(self): acc = {'TableName': 'T'} - _deep_add_numeric(acc, {'TableName': 'T'}) + _deep_add_numeric(acc, {'TableName': 'T'}, self.LEAVES) self.assertEqual(acc, {'TableName': 'T'}) def test_does_not_sum_booleans(self): + # Even if the leaf name is allowlisted, booleans are never summed. acc = {'Flag': True} - _deep_add_numeric(acc, {'Flag': True}) + _deep_add_numeric(acc, {'Flag': True}, frozenset({'Flag'})) self.assertEqual(acc, {'Flag': True}) + def test_does_not_sum_non_allowlisted_numeric_leaf(self): + # A numeric leaf whose name is NOT in the allowlist is preserved from + # the first page, not auto-aggregated. + acc = {'SomeRate': 5.0} + _deep_add_numeric(acc, {'SomeRate': 7.0}, self.LEAVES) + self.assertEqual(acc, {'SomeRate': 5.0}) + def test_deep_copies_new_list_leaves(self): # A list leaf introduced by a later page must not alias the source. source = ['a'] acc = {} - _deep_add_numeric(acc, {'Names': source}) + _deep_add_numeric(acc, {'Names': source}, self.LEAVES) acc['Names'].append('b') self.assertEqual(source, ['a']) def test_adds_new_keys_from_later_pages(self): acc = {'CapacityUnits': 1.0} - _deep_add_numeric(acc, {'CapacityUnits': 1.0, 'TableName': 'T'}) + _deep_add_numeric( + acc, {'CapacityUnits': 1.0, 'TableName': 'T'}, self.LEAVES + ) self.assertEqual(acc, {'CapacityUnits': 2.0, 'TableName': 'T'}) def test_deep_copies_new_dict_leaves(self): # A dict leaf introduced by a later page must not alias the source. source = {'CapacityUnits': 1.0} acc = {} - _deep_add_numeric(acc, {'Index': source}) + _deep_add_numeric(acc, {'Index': source}, self.LEAVES) acc['Index']['CapacityUnits'] += 5.0 self.assertEqual(source['CapacityUnits'], 1.0) def test_type_mismatch_number_then_dict_keeps_first(self): - acc = {'k': 5.0} - _deep_add_numeric(acc, {'k': {'CapacityUnits': 1.0}}) - self.assertEqual(acc, {'k': 5.0}) + acc = {'CapacityUnits': 5.0} + _deep_add_numeric(acc, {'CapacityUnits': {'x': 1.0}}, self.LEAVES) + self.assertEqual(acc, {'CapacityUnits': 5.0}) def test_type_mismatch_string_then_number_keeps_first(self): - acc = {'k': 'T'} - _deep_add_numeric(acc, {'k': 3.0}) - self.assertEqual(acc, {'k': 'T'}) + acc = {'CapacityUnits': 'T'} + _deep_add_numeric(acc, {'CapacityUnits': 3.0}, self.LEAVES) + self.assertEqual(acc, {'CapacityUnits': 'T'}) def test_type_mismatch_none_then_number_keeps_first(self): - acc = {'k': None} - _deep_add_numeric(acc, {'k': 3.0}) - self.assertEqual(acc, {'k': None}) + acc = {'CapacityUnits': None} + _deep_add_numeric(acc, {'CapacityUnits': 3.0}, self.LEAVES) + self.assertEqual(acc, {'CapacityUnits': None}) def test_type_mismatch_none_then_dict_keeps_first(self): acc = {'k': None} - _deep_add_numeric(acc, {'k': {'x': 1.0}}) + _deep_add_numeric(acc, {'k': {'x': 1.0}}, self.LEAVES) self.assertEqual(acc, {'k': None}) @@ -1705,7 +1720,13 @@ def setUp(self): 'output_token': 'NextToken', 'input_token': 'NextToken', 'result_key': 'Items', - 'aggregate_numeric_keys': ['ConsumedCapacity'], + 'aggregate_numeric_keys': { + 'ConsumedCapacity': [ + 'CapacityUnits', + 'ReadCapacityUnits', + 'WriteCapacityUnits', + ] + }, } self.paginator = Paginator( self.method, self.paginate_config, self.model @@ -1713,7 +1734,16 @@ def setUp(self): def test_config_parsed(self): self.assertEqual( - self.paginator._aggregate_numeric_keys, ('ConsumedCapacity',) + self.paginator._aggregate_numeric_keys, + { + 'ConsumedCapacity': frozenset( + { + 'CapacityUnits', + 'ReadCapacityUnits', + 'WriteCapacityUnits', + } + ) + }, ) def test_aggregated_key_dropped_from_non_aggregate_keys(self): @@ -1724,7 +1754,7 @@ def test_aggregated_key_dropped_from_non_aggregate_keys(self): 'input_token': 'NextToken', 'result_key': 'Items', 'non_aggregate_keys': ['ConsumedCapacity', 'SomethingElse'], - 'aggregate_numeric_keys': ['ConsumedCapacity'], + 'aggregate_numeric_keys': {'ConsumedCapacity': ['CapacityUnits']}, } paginator = Paginator(self.method, config, self.model) kept = [k.expression for k in paginator._non_aggregate_keys] @@ -1738,7 +1768,7 @@ def test_aggregated_key_drops_nested_non_aggregate_paths(self): 'input_token': 'NextToken', 'result_key': 'Items', 'non_aggregate_keys': ['ConsumedCapacity.TableName', 'Other'], - 'aggregate_numeric_keys': ['ConsumedCapacity'], + 'aggregate_numeric_keys': {'ConsumedCapacity': ['CapacityUnits']}, } paginator = Paginator(self.method, config, self.model) kept = [k.expression for k in paginator._non_aggregate_keys] @@ -1752,7 +1782,7 @@ def test_aggregation_wins_when_member_in_both_lists(self): 'input_token': 'NextToken', 'result_key': 'Items', 'non_aggregate_keys': ['ConsumedCapacity'], - 'aggregate_numeric_keys': ['ConsumedCapacity'], + 'aggregate_numeric_keys': {'ConsumedCapacity': ['CapacityUnits']}, } paginator = Paginator(self.method, config, self.model) self.method.side_effect = [ @@ -1766,6 +1796,24 @@ def test_aggregation_wins_when_member_in_both_lists(self): result = paginator.paginate().build_full_result() self.assertEqual(result['ConsumedCapacity']['CapacityUnits'], 202.0) + def test_non_allowlisted_numeric_leaf_not_summed_end_to_end(self): + # A numeric field under the member that is not in the allowlist must be + # preserved from the first page, not summed. + self.method.side_effect = [ + { + 'Items': ['a'], + 'ConsumedCapacity': {'CapacityUnits': 100.0, 'SomeRate': 7.0}, + 'NextToken': 'tok', + }, + { + 'Items': ['b'], + 'ConsumedCapacity': {'CapacityUnits': 102.0, 'SomeRate': 9.0}, + }, + ] + cc = self.paginator.paginate().build_full_result()['ConsumedCapacity'] + self.assertEqual(cc['CapacityUnits'], 202.0) # allowlisted -> summed + self.assertEqual(cc['SomeRate'], 7.0) # not allowlisted -> first page + def test_sums_across_pages(self): self.method.side_effect = [ { From 9a9026860f6be4006e721cfbebc698554e4f4754 Mon Sep 17 00:00:00 2001 From: Garrett Beatty Date: Thu, 17 Sep 2026 20:19:37 +0000 Subject: [PATCH 8/9] Make aggregate_numeric_keys strict full-path allowlist with wildcard The leaf-name allowlist matched a leaf by name anywhere in the subtree, so a future field like ConsumedCapacity.AemousCapacity.CapacityUnits would be summed even though it should not be. Change the directive to a list of explicit dotted paths to the exact numeric leaves, with '*' matching only a dynamic key level (e.g. ConsumedCapacity.GlobalSecondaryIndexes.*.CapacityUnits). Only the leaf at a fully-configured path is summed; any other field (including a same-named leaf on an unconfigured path) is preserved from the first page. - paginate.py: _add_numeric_path walks an explicit path ('*' = any key at that level); config parsed as member -> list of segment-tuples. - dynamodb overlay: enumerate the exact ConsumedCapacity paths. - linter: validate paths ('*' only interior, member is top-level). - tests: _add_numeric_path unit tests incl. wildcard + unconfigured-sibling safety; e2e test that a new nested same-named leaf is not summed. --- .../2012-08-10/paginators-1.sdk-extras.json | 42 ++-- awscli/botocore/paginate.py | 119 ++++++----- .../botocore/test_paginator_config.py | 58 +++--- tests/unit/botocore/test_paginate.py | 194 +++++++++--------- 4 files changed, 223 insertions(+), 190 deletions(-) diff --git a/awscli/botocore/data/dynamodb/2012-08-10/paginators-1.sdk-extras.json b/awscli/botocore/data/dynamodb/2012-08-10/paginators-1.sdk-extras.json index e8a24a72681f..59d7be9cb0ee 100644 --- a/awscli/botocore/data/dynamodb/2012-08-10/paginators-1.sdk-extras.json +++ b/awscli/botocore/data/dynamodb/2012-08-10/paginators-1.sdk-extras.json @@ -2,22 +2,36 @@ "merge": { "pagination": { "Query": { - "aggregate_numeric_keys": { - "ConsumedCapacity": [ - "CapacityUnits", - "ReadCapacityUnits", - "WriteCapacityUnits" - ] - } + "aggregate_numeric_keys": [ + "ConsumedCapacity.CapacityUnits", + "ConsumedCapacity.ReadCapacityUnits", + "ConsumedCapacity.WriteCapacityUnits", + "ConsumedCapacity.Table.CapacityUnits", + "ConsumedCapacity.Table.ReadCapacityUnits", + "ConsumedCapacity.Table.WriteCapacityUnits", + "ConsumedCapacity.GlobalSecondaryIndexes.*.CapacityUnits", + "ConsumedCapacity.GlobalSecondaryIndexes.*.ReadCapacityUnits", + "ConsumedCapacity.GlobalSecondaryIndexes.*.WriteCapacityUnits", + "ConsumedCapacity.LocalSecondaryIndexes.*.CapacityUnits", + "ConsumedCapacity.LocalSecondaryIndexes.*.ReadCapacityUnits", + "ConsumedCapacity.LocalSecondaryIndexes.*.WriteCapacityUnits" + ] }, "Scan": { - "aggregate_numeric_keys": { - "ConsumedCapacity": [ - "CapacityUnits", - "ReadCapacityUnits", - "WriteCapacityUnits" - ] - } + "aggregate_numeric_keys": [ + "ConsumedCapacity.CapacityUnits", + "ConsumedCapacity.ReadCapacityUnits", + "ConsumedCapacity.WriteCapacityUnits", + "ConsumedCapacity.Table.CapacityUnits", + "ConsumedCapacity.Table.ReadCapacityUnits", + "ConsumedCapacity.Table.WriteCapacityUnits", + "ConsumedCapacity.GlobalSecondaryIndexes.*.CapacityUnits", + "ConsumedCapacity.GlobalSecondaryIndexes.*.ReadCapacityUnits", + "ConsumedCapacity.GlobalSecondaryIndexes.*.WriteCapacityUnits", + "ConsumedCapacity.LocalSecondaryIndexes.*.CapacityUnits", + "ConsumedCapacity.LocalSecondaryIndexes.*.ReadCapacityUnits", + "ConsumedCapacity.LocalSecondaryIndexes.*.WriteCapacityUnits" + ] } } } diff --git a/awscli/botocore/paginate.py b/awscli/botocore/paginate.py index 6394be24ffb2..a60e46b6c36a 100644 --- a/awscli/botocore/paginate.py +++ b/awscli/botocore/paginate.py @@ -37,37 +37,46 @@ def _is_summable_number(value): return isinstance(value, (int, float)) and not isinstance(value, bool) -def _deep_add_numeric(accumulator, new_value, summable_leaf_names): - """Recursively sum selected numeric leaves of ``new_value`` into ``accumulator``. - - This is used to aggregate response members that are dicts of numbers - (and nested dicts of numbers) across paginated responses, for example - DynamoDB's ``ConsumedCapacity``. Nested dicts are merged recursively (which - handles maps keyed by runtime-defined names such as - ``GlobalSecondaryIndexes``/``LocalSecondaryIndexes``). - - Only numeric leaves whose key is in ``summable_leaf_names`` are summed. This - is an allowlist: any other leaf (a string like ``TableName``, or a numeric - field not named in the allowlist) is preserved from the first page it - appears on, rather than being auto-aggregated. Booleans are never summed. +def _add_numeric_path(accumulator, page_value, segments): + """Add the numeric leaf at ``segments`` from ``page_value`` into ``accumulator``. + + ``segments`` is an explicit path within a response member, e.g. + ``('CapacityUnits',)``, ``('Table', 'CapacityUnits')``, or + ``('GlobalSecondaryIndexes', '*', 'CapacityUnits')``. A ``'*'`` segment + matches every key at that level (for maps keyed by runtime-defined names + such as index names); every other segment is matched literally. + + Only the exact leaf named by the full path is summed — this is a strict, + path-based allowlist. Intermediate keys seen for the first time (e.g. a new + index name on a later page) are seeded by deep-copy; anything not on a + configured path is left untouched (it was seeded from the first page). + Booleans and cross-page type mismatches are never summed. """ - for key, value in new_value.items(): - if key not in accumulator: - # First time we've seen this leaf. Deep-copy mutable containers so - # the aggregate never aliases (and later mutates) a source page. + if not isinstance(page_value, dict) or not isinstance(accumulator, dict): + return + seg, rest = segments[0], segments[1:] + keys = ( + list(page_value) + if seg == '*' + else ([seg] if seg in page_value else []) + ) + for key in keys: + value = page_value[key] + if rest: + if key not in accumulator: + # First time this (possibly dynamic) key appears; seed subtree. + accumulator[key] = deepcopy(value) + else: + _add_numeric_path(accumulator[key], value, rest) + elif key not in accumulator: accumulator[key] = ( deepcopy(value) if isinstance(value, (dict, list)) else value ) - elif isinstance(value, dict) and isinstance(accumulator[key], dict): - _deep_add_numeric(accumulator[key], value, summable_leaf_names) - elif ( - key in summable_leaf_names - and _is_summable_number(value) - and _is_summable_number(accumulator[key]) + elif _is_summable_number(value) and _is_summable_number( + accumulator[key] ): accumulator[key] = accumulator[key] + value - # Everything else (non-allowlisted numeric leaf, string, or cross-page - # type mismatch): keep the first page's value. + # else: non-numeric or cross-page type mismatch -> keep first value. class TokenEncoder: @@ -537,24 +546,35 @@ def build_full_result(self): # uses. We can remove it though once operation objects are removed. if isinstance(response, tuple) and len(response) == 2: page = response[1] - for key, leaf_names in self._aggregate_numeric_keys.items(): - page_value = page.get(key) + for member, paths in self._aggregate_numeric_keys.items(): + page_value = page.get(member) if page_value is None: continue - if key not in aggregate_numeric_totals: - aggregate_numeric_totals[key] = deepcopy(page_value) + if member not in aggregate_numeric_totals: + # Seed the whole member from the first page it appears on; + # later pages only add the configured leaf paths, so any + # field NOT on a path keeps this first-page value. + aggregate_numeric_totals[member] = ( + deepcopy(page_value) + if isinstance(page_value, (dict, list)) + else page_value + ) elif isinstance(page_value, dict) and isinstance( - aggregate_numeric_totals[key], dict + aggregate_numeric_totals[member], dict ): - _deep_add_numeric( - aggregate_numeric_totals[key], page_value, leaf_names - ) + for segments in paths: + if segments: + _add_numeric_path( + aggregate_numeric_totals[member], + page_value, + segments, + ) elif _is_summable_number(page_value) and _is_summable_number( - aggregate_numeric_totals[key] + aggregate_numeric_totals[member] ): - # The whole member is a bare number (not a dict). Opting the - # member in via config is itself the allowlist decision. - aggregate_numeric_totals[key] += page_value + # The whole member is a bare number (opted in as a path with + # no leaf, e.g. just "SomeCount"). + aggregate_numeric_totals[member] += page_value # Any other/unexpected shape: keep the first page's value. # We're incrementally building the full response page # by page. For each page in the response we need to @@ -709,18 +729,19 @@ def _is_aggregated(self, non_aggregate_key): return False def _get_aggregate_numeric_keys(self, config): - # Maps a top-level response member to the set of leaf field-names whose - # numeric values are summed (recursively, so nested/dynamic-key maps - # like GlobalSecondaryIndexes are covered). This is an allowlist: only - # the named leaves are totaled; any other leaf (strings, or numbers not - # named here) is preserved from the first page rather than summed. - # Unlike ``result_key`` these members may be (nested) dicts, and unlike - # ``non_aggregate_keys`` the named leaves are totaled across pages. - config_value = config.get('aggregate_numeric_keys', {}) - return { - member: frozenset(leaf_names) - for member, leaf_names in config_value.items() - } + # A list of explicit dotted paths to numeric leaves that are summed + # across pages, e.g. "ConsumedCapacity.CapacityUnits" or + # "ConsumedCapacity.GlobalSecondaryIndexes.*.CapacityUnits" (a "*" + # segment matches every key at that level, for maps keyed by + # runtime-defined names). This is a strict allowlist: only the exact + # leaf at each full path is totaled; any other field (strings, or + # numbers on a path that is not configured) is preserved from the first + # page. Parsed into member -> list of segment-tuples (after the member). + paths = {} + for path in config.get('aggregate_numeric_keys', []): + segments = path.split('.') + paths.setdefault(segments[0], []).append(tuple(segments[1:])) + return paths def _get_output_tokens(self, config): output = [] diff --git a/tests/functional/botocore/test_paginator_config.py b/tests/functional/botocore/test_paginator_config.py index 7b5028d56c78..2a23483ee5ef 100644 --- a/tests/functional/botocore/test_paginator_config.py +++ b/tests/functional/botocore/test_paginator_config.py @@ -175,30 +175,29 @@ def test_lint_pagination_configs( def _validate_aggregate_numeric_keys(operation_name, page_config): - # aggregate_numeric_keys maps a top-level output member name to a list of - # leaf field-names to sum. The member must be a top-level member (a nested - # path like "ConsumedCapacity.Table" would silently never aggregate at - # runtime, since build_full_result uses page.get(member)); leaf names are - # matched by key anywhere in the subtree, so they must be bare field names. - config_value = page_config.get('aggregate_numeric_keys', {}) - if not isinstance(config_value, dict): + # aggregate_numeric_keys is a list of explicit dotted paths to the numeric + # leaves to sum, e.g. "ConsumedCapacity.CapacityUnits" or + # "ConsumedCapacity.GlobalSecondaryIndexes.*.CapacityUnits". A "*" matches + # every key at that level and may only appear as an interior segment (not + # the top-level member, which must be a real output member, nor the leaf). + config_value = page_config.get('aggregate_numeric_keys', []) + if not isinstance(config_value, list): raise AssertionError( f"aggregate_numeric_keys for operation {operation_name} must be a " - "map of member name -> list of leaf field-names." + "list of dotted leaf paths." ) - for member, leaf_names in config_value.items(): - if '.' in member: + for path in config_value: + segments = path.split('.') + if segments[0] == '*': raise AssertionError( - f"aggregate_numeric_keys member '{member}' for operation " - f"{operation_name} must be a top-level output member name, " - "not a nested path." + f"aggregate_numeric_keys path '{path}' for operation " + f"{operation_name} must start with a top-level output member, " + "not '*'." ) - if not isinstance(leaf_names, list) or not all( - isinstance(n, str) and '.' not in n for n in leaf_names - ): + if segments[-1] == '*': raise AssertionError( - f"aggregate_numeric_keys['{member}'] for operation " - f"{operation_name} must be a list of bare leaf field-names." + f"aggregate_numeric_keys path '{path}' for operation " + f"{operation_name} must end with a leaf field-name, not '*'." ) @@ -353,20 +352,25 @@ def _get_all_page_output_keys(page_config): yield 'output_token', key if 'more_results' in page_config: yield 'more_results', page_config['more_results'] - aggregate_numeric_keys = page_config.get('aggregate_numeric_keys', []) + # aggregate_numeric_keys is a list of dotted paths; only the top-level + # member of each path is an output member to account for. + aggregate_members = { + path.split('.')[0] + for path in page_config.get('aggregate_numeric_keys', []) + } for key in page_config.get('non_aggregate_keys', []): - # A member declared under aggregate_numeric_keys is aggregated across - # pages and takes precedence over any non_aggregate declaration for the - # same member (mirroring Paginator._get_non_aggregate_keys). Skip it - # here so it is only accounted for once. + # A member that is aggregated across pages takes precedence over any + # non_aggregate declaration for the same member (mirroring + # Paginator._get_non_aggregate_keys). Skip it here so it is only + # accounted for once. if any( - key == agg or key.startswith(f'{agg}.') - for agg in aggregate_numeric_keys + key == member or key.startswith(f'{member}.') + for member in aggregate_members ): continue yield 'non_aggregate_keys', key - for key in aggregate_numeric_keys: - yield 'aggregate_numeric_keys', key + for member in aggregate_members: + yield 'aggregate_numeric_keys', member def _get_list_value(page_config, key): diff --git a/tests/unit/botocore/test_paginate.py b/tests/unit/botocore/test_paginate.py index 8cc74c190327..b29849b3a9b4 100644 --- a/tests/unit/botocore/test_paginate.py +++ b/tests/unit/botocore/test_paginate.py @@ -18,7 +18,7 @@ PaginatorModel, TokenDecoder, TokenEncoder, - _deep_add_numeric, + _add_numeric_path, ) from tests import mock, unittest @@ -1622,94 +1622,84 @@ def test_str_page_size(self): self.method.assert_called_with(MaxItems='1') -class TestDeepAddNumeric(unittest.TestCase): - LEAVES = frozenset( - {'CapacityUnits', 'ReadCapacityUnits', 'WriteCapacityUnits'} - ) - - def test_sums_numeric_leaves(self): +class TestAddNumericPath(unittest.TestCase): + def test_sums_leaf(self): acc = {'CapacityUnits': 100.0} - _deep_add_numeric(acc, {'CapacityUnits': 102.5}, self.LEAVES) + _add_numeric_path(acc, {'CapacityUnits': 102.5}, ('CapacityUnits',)) self.assertEqual(acc, {'CapacityUnits': 202.5}) - def test_recurses_into_nested_dicts(self): + def test_sums_nested_static_path(self): acc = {'Table': {'CapacityUnits': 1.0}} - _deep_add_numeric(acc, {'Table': {'CapacityUnits': 2.0}}, self.LEAVES) + _add_numeric_path( + acc, {'Table': {'CapacityUnits': 2.0}}, ('Table', 'CapacityUnits') + ) self.assertEqual(acc, {'Table': {'CapacityUnits': 3.0}}) - def test_sums_maps_with_runtime_defined_keys(self): - # e.g. GlobalSecondaryIndexes keyed by a user-chosen index name. - acc = {'GlobalSecondaryIndexes': {'my-index': {'CapacityUnits': 5.0}}} - _deep_add_numeric( + def test_wildcard_sums_dynamic_key(self): + acc = {'GSI': {'idx': {'CapacityUnits': 5.0}}} + _add_numeric_path( acc, - {'GlobalSecondaryIndexes': {'my-index': {'CapacityUnits': 7.0}}}, - self.LEAVES, + {'GSI': {'idx': {'CapacityUnits': 7.0}}}, + ('GSI', '*', 'CapacityUnits'), + ) + self.assertEqual(acc, {'GSI': {'idx': {'CapacityUnits': 12.0}}}) + + def test_wildcard_seeds_new_dynamic_key(self): + # An index first seen on a later page is seeded, not skipped. + acc = {'GSI': {'idx1': {'CapacityUnits': 5.0}}} + _add_numeric_path( + acc, + { + 'GSI': { + 'idx1': {'CapacityUnits': 1.0}, + 'idx2': {'CapacityUnits': 9.0}, + } + }, + ('GSI', '*', 'CapacityUnits'), ) self.assertEqual( acc, - {'GlobalSecondaryIndexes': {'my-index': {'CapacityUnits': 12.0}}}, - ) - - def test_preserves_strings(self): - acc = {'TableName': 'T'} - _deep_add_numeric(acc, {'TableName': 'T'}, self.LEAVES) - self.assertEqual(acc, {'TableName': 'T'}) - - def test_does_not_sum_booleans(self): - # Even if the leaf name is allowlisted, booleans are never summed. - acc = {'Flag': True} - _deep_add_numeric(acc, {'Flag': True}, frozenset({'Flag'})) - self.assertEqual(acc, {'Flag': True}) - - def test_does_not_sum_non_allowlisted_numeric_leaf(self): - # A numeric leaf whose name is NOT in the allowlist is preserved from - # the first page, not auto-aggregated. - acc = {'SomeRate': 5.0} - _deep_add_numeric(acc, {'SomeRate': 7.0}, self.LEAVES) - self.assertEqual(acc, {'SomeRate': 5.0}) - - def test_deep_copies_new_list_leaves(self): - # A list leaf introduced by a later page must not alias the source. - source = ['a'] - acc = {} - _deep_add_numeric(acc, {'Names': source}, self.LEAVES) - acc['Names'].append('b') - self.assertEqual(source, ['a']) - - def test_adds_new_keys_from_later_pages(self): - acc = {'CapacityUnits': 1.0} - _deep_add_numeric( - acc, {'CapacityUnits': 1.0, 'TableName': 'T'}, self.LEAVES - ) - self.assertEqual(acc, {'CapacityUnits': 2.0, 'TableName': 'T'}) - - def test_deep_copies_new_dict_leaves(self): - # A dict leaf introduced by a later page must not alias the source. - source = {'CapacityUnits': 1.0} - acc = {} - _deep_add_numeric(acc, {'Index': source}, self.LEAVES) - acc['Index']['CapacityUnits'] += 5.0 - self.assertEqual(source['CapacityUnits'], 1.0) - - def test_type_mismatch_number_then_dict_keeps_first(self): - acc = {'CapacityUnits': 5.0} - _deep_add_numeric(acc, {'CapacityUnits': {'x': 1.0}}, self.LEAVES) - self.assertEqual(acc, {'CapacityUnits': 5.0}) - - def test_type_mismatch_string_then_number_keeps_first(self): + { + 'GSI': { + 'idx1': {'CapacityUnits': 6.0}, + 'idx2': {'CapacityUnits': 9.0}, + } + }, + ) + + def test_does_not_touch_unconfigured_sibling(self): + # The AemousCapacity case: a numeric leaf NOT on a configured path is + # left as the first-page value, even though its leaf name matches an + # aggregated one elsewhere. + acc = { + 'CapacityUnits': 100.0, + 'AemousCapacity': {'CapacityUnits': 123.0}, + } + _add_numeric_path( + acc, + { + 'CapacityUnits': 102.0, + 'AemousCapacity': {'CapacityUnits': 123.0}, + }, + ('CapacityUnits',), + ) + self.assertEqual(acc['CapacityUnits'], 202.0) + self.assertEqual(acc['AemousCapacity'], {'CapacityUnits': 123.0}) + + def test_missing_path_in_page_is_noop(self): + acc = {'CapacityUnits': 100.0} + _add_numeric_path(acc, {}, ('CapacityUnits',)) + self.assertEqual(acc, {'CapacityUnits': 100.0}) + + def test_type_mismatch_keeps_first(self): acc = {'CapacityUnits': 'T'} - _deep_add_numeric(acc, {'CapacityUnits': 3.0}, self.LEAVES) + _add_numeric_path(acc, {'CapacityUnits': 3.0}, ('CapacityUnits',)) self.assertEqual(acc, {'CapacityUnits': 'T'}) - def test_type_mismatch_none_then_number_keeps_first(self): - acc = {'CapacityUnits': None} - _deep_add_numeric(acc, {'CapacityUnits': 3.0}, self.LEAVES) - self.assertEqual(acc, {'CapacityUnits': None}) - - def test_type_mismatch_none_then_dict_keeps_first(self): - acc = {'k': None} - _deep_add_numeric(acc, {'k': {'x': 1.0}}, self.LEAVES) - self.assertEqual(acc, {'k': None}) + def test_does_not_sum_boolean(self): + acc = {'CapacityUnits': True} + _add_numeric_path(acc, {'CapacityUnits': True}, ('CapacityUnits',)) + self.assertIs(acc['CapacityUnits'], True) class TestAggregateNumericKeys(unittest.TestCase): @@ -1720,13 +1710,10 @@ def setUp(self): 'output_token': 'NextToken', 'input_token': 'NextToken', 'result_key': 'Items', - 'aggregate_numeric_keys': { - 'ConsumedCapacity': [ - 'CapacityUnits', - 'ReadCapacityUnits', - 'WriteCapacityUnits', - ] - }, + 'aggregate_numeric_keys': [ + 'ConsumedCapacity.CapacityUnits', + 'ConsumedCapacity.GlobalSecondaryIndexes.*.CapacityUnits', + ], } self.paginator = Paginator( self.method, self.paginate_config, self.model @@ -1736,13 +1723,10 @@ def test_config_parsed(self): self.assertEqual( self.paginator._aggregate_numeric_keys, { - 'ConsumedCapacity': frozenset( - { - 'CapacityUnits', - 'ReadCapacityUnits', - 'WriteCapacityUnits', - } - ) + 'ConsumedCapacity': [ + ('CapacityUnits',), + ('GlobalSecondaryIndexes', '*', 'CapacityUnits'), + ] }, ) @@ -1754,7 +1738,7 @@ def test_aggregated_key_dropped_from_non_aggregate_keys(self): 'input_token': 'NextToken', 'result_key': 'Items', 'non_aggregate_keys': ['ConsumedCapacity', 'SomethingElse'], - 'aggregate_numeric_keys': {'ConsumedCapacity': ['CapacityUnits']}, + 'aggregate_numeric_keys': ['ConsumedCapacity.CapacityUnits'], } paginator = Paginator(self.method, config, self.model) kept = [k.expression for k in paginator._non_aggregate_keys] @@ -1768,7 +1752,7 @@ def test_aggregated_key_drops_nested_non_aggregate_paths(self): 'input_token': 'NextToken', 'result_key': 'Items', 'non_aggregate_keys': ['ConsumedCapacity.TableName', 'Other'], - 'aggregate_numeric_keys': {'ConsumedCapacity': ['CapacityUnits']}, + 'aggregate_numeric_keys': ['ConsumedCapacity.CapacityUnits'], } paginator = Paginator(self.method, config, self.model) kept = [k.expression for k in paginator._non_aggregate_keys] @@ -1782,7 +1766,7 @@ def test_aggregation_wins_when_member_in_both_lists(self): 'input_token': 'NextToken', 'result_key': 'Items', 'non_aggregate_keys': ['ConsumedCapacity'], - 'aggregate_numeric_keys': {'ConsumedCapacity': ['CapacityUnits']}, + 'aggregate_numeric_keys': ['ConsumedCapacity.CapacityUnits'], } paginator = Paginator(self.method, config, self.model) self.method.side_effect = [ @@ -1796,23 +1780,33 @@ def test_aggregation_wins_when_member_in_both_lists(self): result = paginator.paginate().build_full_result() self.assertEqual(result['ConsumedCapacity']['CapacityUnits'], 202.0) - def test_non_allowlisted_numeric_leaf_not_summed_end_to_end(self): - # A numeric field under the member that is not in the allowlist must be - # preserved from the first page, not summed. + def test_unconfigured_nested_leaf_not_summed_end_to_end(self): + # The exact concern: a NEW nested numeric field (AemousCapacity) whose + # leaf name matches an aggregated one must NOT be summed, because its + # full path is not configured. self.method.side_effect = [ { 'Items': ['a'], - 'ConsumedCapacity': {'CapacityUnits': 100.0, 'SomeRate': 7.0}, + 'ConsumedCapacity': { + 'CapacityUnits': 100.0, + 'AemousCapacity': {'CapacityUnits': 123.0}, + }, 'NextToken': 'tok', }, { 'Items': ['b'], - 'ConsumedCapacity': {'CapacityUnits': 102.0, 'SomeRate': 9.0}, + 'ConsumedCapacity': { + 'CapacityUnits': 102.0, + 'AemousCapacity': {'CapacityUnits': 123.0}, + }, }, ] cc = self.paginator.paginate().build_full_result()['ConsumedCapacity'] - self.assertEqual(cc['CapacityUnits'], 202.0) # allowlisted -> summed - self.assertEqual(cc['SomeRate'], 7.0) # not allowlisted -> first page + self.assertEqual( + cc['CapacityUnits'], 202.0 + ) # configured path -> summed + # Not on a configured path -> kept from first page, not doubled. + self.assertEqual(cc['AemousCapacity'], {'CapacityUnits': 123.0}) def test_sums_across_pages(self): self.method.side_effect = [ From fcfc1ed4a9390eb96b2b99dc7861c761f5c64092 Mon Sep 17 00:00:00 2001 From: Garrett Beatty Date: Fri, 18 Sep 2026 00:07:37 +0000 Subject: [PATCH 9/9] Address Copilot review: defensive guards for aggregate_numeric_keys - PageIterator aggregate_numeric_keys defaults to None, normalized to {} (it is a map used with .items(); a tuple default would AttributeError if a caller omits it). - _add_numeric_path returns early on empty segments (no IndexError if reused with an empty path). - Config linter rejects non-string entries and empty path segments (leading/ trailing or doubled '.'). --- awscli/botocore/paginate.py | 7 +++++-- tests/functional/botocore/test_paginator_config.py | 11 +++++++++++ tests/unit/botocore/test_paginate.py | 6 ++++++ 3 files changed, 22 insertions(+), 2 deletions(-) diff --git a/awscli/botocore/paginate.py b/awscli/botocore/paginate.py index a60e46b6c36a..7f35f24e54fc 100644 --- a/awscli/botocore/paginate.py +++ b/awscli/botocore/paginate.py @@ -52,6 +52,8 @@ def _add_numeric_path(accumulator, page_value, segments): configured path is left untouched (it was seeded from the first page). Booleans and cross-page type mismatches are never summed. """ + if not segments: + return if not isinstance(page_value, dict) or not isinstance(accumulator, dict): return seg, rest = segments[0], segments[1:] @@ -253,7 +255,7 @@ def __init__( starting_token, page_size, op_kwargs, - aggregate_numeric_keys=(), + aggregate_numeric_keys=None, ): self._method = method self._input_token = input_token @@ -267,7 +269,8 @@ def __init__( self._op_kwargs = op_kwargs self._resume_token = None self._non_aggregate_key_exprs = non_aggregate_keys - self._aggregate_numeric_keys = aggregate_numeric_keys + # Maps member -> list of segment-tuples; may be omitted by callers. + self._aggregate_numeric_keys = aggregate_numeric_keys or {} self._non_aggregate_part = {} self._token_encoder = TokenEncoder() self._token_decoder = TokenDecoder() diff --git a/tests/functional/botocore/test_paginator_config.py b/tests/functional/botocore/test_paginator_config.py index 2a23483ee5ef..b942cd032a56 100644 --- a/tests/functional/botocore/test_paginator_config.py +++ b/tests/functional/botocore/test_paginator_config.py @@ -187,7 +187,18 @@ def _validate_aggregate_numeric_keys(operation_name, page_config): "list of dotted leaf paths." ) for path in config_value: + if not isinstance(path, str) or not path: + raise AssertionError( + f"aggregate_numeric_keys entry {path!r} for operation " + f"{operation_name} must be a non-empty string path." + ) segments = path.split('.') + if any(segment == '' for segment in segments): + raise AssertionError( + f"aggregate_numeric_keys path '{path}' for operation " + f"{operation_name} has an empty segment (leading/trailing or " + "doubled '.')." + ) if segments[0] == '*': raise AssertionError( f"aggregate_numeric_keys path '{path}' for operation " diff --git a/tests/unit/botocore/test_paginate.py b/tests/unit/botocore/test_paginate.py index b29849b3a9b4..8ebe6839fc51 100644 --- a/tests/unit/botocore/test_paginate.py +++ b/tests/unit/botocore/test_paginate.py @@ -1691,6 +1691,12 @@ def test_missing_path_in_page_is_noop(self): _add_numeric_path(acc, {}, ('CapacityUnits',)) self.assertEqual(acc, {'CapacityUnits': 100.0}) + def test_empty_segments_is_noop(self): + # Defensive: an empty path must not raise (IndexError) and change nothing. + acc = {'CapacityUnits': 100.0} + _add_numeric_path(acc, {'CapacityUnits': 5.0}, ()) + self.assertEqual(acc, {'CapacityUnits': 100.0}) + def test_type_mismatch_keeps_first(self): acc = {'CapacityUnits': 'T'} _add_numeric_path(acc, {'CapacityUnits': 3.0}, ('CapacityUnits',))