Uh oh!
There was an error while loading. Please reload this page.
Add serializer to response wrapper - #145
Conversation
4e4ac70 to
e6869b9Comparee6869b9 to
0bc370dComparematez0
commented
Dec 11, 2023
Updated the author's and committer's email address. |
matez0
commented
Jan 19, 2024
While the README states that "Requires Python > 3.6", the build was run for Python 3.6 as well. |
a9769b7 to
ea783b5Compareea783b5 to
ade758aCompareade758a to
d87f5d0Compared87f5d0 to
2287b32Comparematez0
commented
May 1, 2024
Rename decorator and argument name for more readability. |
2287b32 to
bb08bcbComparematez0
commented
May 1, 2024
Rebased onto the top of main. |
bb08bcb to
5f9fec1Compare5f9fec1 to
9c69e60Comparematez0
commented
Dec 23, 2024
Rebased on the top of main. |
It can be useful, when the response fully or partially need to be stored in a database in a JSON serialized form. From python 3.8, the `_unwrap` method could be replaced with using singledispatchmethod.
9c69e60 to
2573f14Comparematez0
commented
Apr 2, 2025
Rebased on the top of main. |
There was a problem hiding this comment.
Thanks for this @matez0 — clean implementation, thorough tests (the circular-reference handling with JSON Pointer–style $ref is a nice touch). Two suggestions before merge, both about the attribute-walking strategy in _unwrap_object:
1. dir(data) invokes property getters and walks inherited attributes
return {
key: cls._unwrap(getattr(data, key), paths_by_id, path + [key])
for key in dir(data)
if not key.startswith('__')
and not cls._is_function(getattr(data, key))
}
A few concerns with dir():
Property getters fire on serialization. If a consumer subclasses ResponseWrapper (or wraps an object) with a @Property that hits a DB, raises, or has side effects, .dict() will trigger it — and getattr runs twice here (once in the filter, once in the value), so the side effect happens twice.
Inherited / class-level attributes are included. dir() returns everything visible on the class, not just the instance.
Performance. Two getattr calls per attribute.
Could we use vars(data) (i.e. data.dict) instead? It walks instance attributes only, never invokes descriptors, and only requires a single getattr/dict lookup per key.
Heads-up that this is a behavior change: test_serialize_object declares ObjAttr attributes as class variables, which vars(instance) won't see. If we want to keep that test passing as-is, we'd need to either (a) move them to init, or (b) merge type(data).dict in for non-ResponseWrapper instances.
2. Single-underscore attributes leak through the filter
The filter is not key.startswith('__'), so conventional Python privates (_internal_state, _cached_token, etc.) are serialized. The SDK doesn't currently use _-prefixed sensitive fields, but it's a sharp edge for future code or downstream subclasses.
Two options:
- (a) tighten the filter — single underscores are conventionally private
if not key.startswith('_')
Or if there's a reason to include them:
- (b) keep the current behavior, document it
def dict(self):
"""
Serializes the instance to a dictionary recursively.
...
Note: attributes prefixed with a single underscore are included in the output.
If you store sensitive data on_-prefixed attributes, filter the result before logging or persisting.
"""
Aside
Worth a one-liner in the docstring that responses may contain payment-sensitive fields (PAN, CVV tokens, OAuth tokens) and that callers should treat .dict() output the same as the underlying response — i.e. don't log/persist without redaction. Not a code issue, just an ergonomics nudge so .dict() doesn't make accidental logging too easy.
Tests are great either way — really appreciate the circular-reference coverage.



Implements #144