fix(idempotency): is_missing_idempotency_key iterates dict keys instead of values - #8391
Open
Adityaj0 wants to merge 1 commit into
Open
fix(idempotency): is_missing_idempotency_key iterates dict keys instead of values#8391Adityaj0 wants to merge 1 commit into
Adityaj0 wants to merge 1 commit into
Conversation
…ad of values
is_missing_idempotency_key iterated `data` directly for dict input, which
walks its keys, not its values. For a dict whose values are all None but
whose keys are ordinary non-None strings -- exactly what a JMESPath
multi-select expression like '{user: headers.user_id, order: body.order_id}'
produces when the referenced event fields are absent -- this returns False
("not missing") when it should return True.
With raise_on_no_idempotency_key=True, the safety check that's supposed to
raise IdempotencyKeyError in this situation silently doesn't fire. With the
default False, no warning is emitted and the persistence layer hashes the
all-None dict into a real idempotency key, so unrelated invocations that
both fail to populate those fields collapse onto the same idempotency key
and get incorrectly deduplicated against each other.
The existing test only covered a dict of {None: None} (None as the key),
which happens to still pass under the old key-iterating behavior and so
never caught this. Iterate data.values() for dict input instead, and add
a test covering the realistic non-None-keys/all-None-values case.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.



Issue number: closes #8390
Summary
Changes
is_missing_idempotency_keyiterateddatadirectly fordictinput:for x in dataover adictwalks its keys, not its values. This is correct forlist/tuple, but wrong fordict— a dict produced by a JMESPath multi-select expression (e.g.event_key_jmespath="{user: headers.user_id, order: body.order_id}") over fields that are all absent from the event resolves to{"user": None, "order": None}: ordinary non-Nonekeys, all-Nonevalues. The key-based check incorrectly reports this as "not missing."Consequence: with
raise_on_no_idempotency_key=True, the safety check silently doesn't fire. With the defaultFalse, no warning is emitted and the all-None dict gets hashed into a real idempotency key, causing unrelated invocations that both fail to populate those fields to collapse onto the same key and get incorrectly deduplicated.Fix: iterate
data.values()fordictinput.Added
is_missing_idempotency_key({"user": None, "order": None})andis_missing_idempotency_key({"user": "abc"})cases to the existingtest_is_missing_idempotency_keyintests/functional/idempotency/_boto3/test_idempotency.py. The existing test only covered{None: None}(Noneas the key), which happens to still pass under the buggy key-iterating code — that's why it never caught this. Confirmed the new assertion fails against the pre-fix code and passes with the fix:(
_redisexcluded from my local run only because of an unrelated missingmultiprocessdependency in my environment, not related to this change.)User experience
Before: a dict-shaped idempotency key extraction (multi-select JMESPath) whose values are all missing is silently treated as present, either bypassing
raise_on_no_idempotency_keyentirely or generating a real (and collision-prone, since it's a constant hash) idempotency key from empty data.After: correctly detected as missing, matching the existing behavior for list/tuple/scalar inputs.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.