Skip to content

Commit fd39aff

Browse files
Alex Wangwangyb-A
authored andcommitted
fix(plugin): exclude payloads from info equality
Addresses the medium-severity Codex review comment on #616. The payload fields joined the generated __eq__ and __hash__, so the widening was not additive. execution_input holds arbitrary deserialized JSON, and a dict or list value made a previously hashable InvocationStartInfo raise TypeError on hash(); both fields also made infos built from the earlier field set compare unequal to infos carrying a payload. Both effects were reproduced first. Set compare=False, hash=False on execution_input and execution_result. Identity fields still drive equality, so payload-only differences now compare equal -- payloads are incidental data on what is otherwise an event record. Adds tests for hashability across dict, list, nested and scalar payloads, for equality against the prior field set, and for the field declarations themselves. Note OperationInfo.result / OperationInfo.error from #625 remain in compare. They are hashable types so they do not break hash(), but the equality asymmetry with these fields is worth a maintainer decision. Refs #616
1 parent 34c4185 commit fd39aff

2 files changed

Lines changed: 93 additions & 2 deletions

File tree

packages/aws-durable-execution-sdk-python/src/aws_durable_execution_sdk_python/plugin.py

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -180,6 +180,8 @@ class InvocationInfo:
180180
default=None,
181181
kw_only=True,
182182
repr=False,
183+
compare=False,
184+
hash=False,
183185
metadata={"experimental": True},
184186
)
185187
"""EXPERIMENTAL: The deserialized execution input, when available.
@@ -193,6 +195,11 @@ class InvocationInfo:
193195
secrets, possibly megabytes -- into logs. Read the attribute explicitly to
194196
record it.
195197
198+
Excluded from ``__eq__`` and ``__hash__`` so adding it stays additive. The
199+
value is arbitrary deserialized JSON, so a dict or list payload would make a
200+
previously hashable info unhashable, and comparisons against infos built
201+
from the earlier field set would start returning False.
202+
196203
Defaults to ``None`` only when the field is not populated (a hook info built
197204
without it); ``durable_execution()`` always populates it with the
198205
deserialized input payload, which is ``{}`` when the payload is empty.
@@ -216,6 +223,8 @@ class InvocationEndInfo(InvocationInfo):
216223
default=None,
217224
kw_only=True,
218225
repr=False,
226+
compare=False,
227+
hash=False,
219228
metadata={"experimental": True},
220229
)
221230
"""EXPERIMENTAL: The serialized execution result, when available.
@@ -224,9 +233,10 @@ class InvocationEndInfo(InvocationInfo):
224233
large payload. Mirrors the JS SDK's ``InvocationEndInfo.executionResult``.
225234
``None`` on failure or suspend.
226235
227-
Excluded from ``repr`` for the same reason as
236+
Excluded from ``repr``, ``__eq__`` and ``__hash__`` for the same reasons as
228237
:attr:`InvocationInfo.execution_input`: hook infos are logged wholesale by
229-
instrumentation, and the result is customer data.
238+
instrumentation, and adding the field should not change how existing infos
239+
compare.
230240
"""
231241

232242
@classmethod

packages/aws-durable-execution-sdk-python/tests/plugin_test.py

Lines changed: 81 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -161,6 +161,87 @@ def test_payload_fields_are_declared_non_repr(self):
161161
self.assertFalse(end_fields["execution_input"].repr)
162162
self.assertFalse(end_fields["execution_result"].repr)
163163

164+
def test_payload_fields_do_not_break_hashability(self):
165+
"""A payload must not make a previously hashable info unhashable.
166+
167+
``execution_input`` holds arbitrary deserialized JSON, so a dict or list
168+
value would otherwise propagate into the generated ``__hash__`` and
169+
raise ``TypeError``.
170+
"""
171+
base = {
172+
"request_id": "req-1",
173+
"execution_arn": "arn:test",
174+
"is_first_invocation": True,
175+
}
176+
177+
for payload in ({"k": "v"}, ["a", "b"], {"nested": {"deep": [1, 2]}}, "plain"):
178+
info = InvocationStartInfo(**base, execution_input=payload)
179+
# Must not raise, and must match the payload-free hash.
180+
self.assertEqual(hash(InvocationStartInfo(**base)), hash(info))
181+
182+
end_info = InvocationEndInfo(
183+
**base,
184+
status=InvocationStatus.SUCCEEDED,
185+
execution_input={"k": "v"},
186+
execution_result='{"big": "payload"}',
187+
)
188+
self.assertEqual(
189+
hash(InvocationEndInfo(**base, status=InvocationStatus.SUCCEEDED)),
190+
hash(end_info),
191+
)
192+
193+
def test_payload_fields_are_excluded_from_equality(self):
194+
"""Adding the payload fields must not change how infos compare.
195+
196+
Infos built from the earlier field set still compare equal to infos
197+
carrying a payload, so this widening stays additive for callers.
198+
"""
199+
base = {
200+
"request_id": "req-1",
201+
"execution_arn": "arn:test",
202+
"is_first_invocation": True,
203+
}
204+
205+
self.assertEqual(
206+
InvocationStartInfo(**base),
207+
InvocationStartInfo(**base, execution_input={"k": "v"}),
208+
)
209+
# Two different payloads also compare equal -- payloads are incidental
210+
# data, not part of the info's identity.
211+
self.assertEqual(
212+
InvocationStartInfo(**base, execution_input={"a": 1}),
213+
InvocationStartInfo(**base, execution_input={"b": 2}),
214+
)
215+
self.assertEqual(
216+
InvocationEndInfo(**base, status=InvocationStatus.SUCCEEDED),
217+
InvocationEndInfo(
218+
**base,
219+
status=InvocationStatus.SUCCEEDED,
220+
execution_input={"k": "v"},
221+
execution_result='"result"',
222+
),
223+
)
224+
# Identity fields still drive inequality.
225+
self.assertNotEqual(
226+
InvocationStartInfo(**base, execution_input={"k": "v"}),
227+
InvocationStartInfo(
228+
**{**base, "request_id": "req-2"}, execution_input={"k": "v"}
229+
),
230+
)
231+
232+
def test_payload_fields_are_declared_non_compare(self):
233+
"""Pin the declarations, not just the observed behaviour."""
234+
start_fields = {f.name: f for f in fields(InvocationStartInfo)}
235+
end_fields = {f.name: f for f in fields(InvocationEndInfo)}
236+
237+
for holder, name in (
238+
(start_fields, "execution_input"),
239+
(end_fields, "execution_input"),
240+
(end_fields, "execution_result"),
241+
):
242+
self.assertFalse(holder[name].compare, name)
243+
self.assertIs(holder[name].hash, False, name)
244+
164245
def test_payload_fields_are_marked_experimental(self):
165246
plugin_info_types = (
166247
OperationInfo,

0 commit comments

Comments
 (0)