Skip to content

Add serializer to response wrapper#145

Open
matez0 wants to merge 2 commits intocheckout:mainfrom
matez0:add-serializer-to-response-wrapper
Open

Add serializer to response wrapper#145
matez0 wants to merge 2 commits intocheckout:mainfrom
matez0:add-serializer-to-response-wrapper

Conversation

@matez0
Copy link
Copy Markdown

@matez0 matez0 commented Dec 7, 2023

Note:
From python 3.8, the _unwrap method could be replaced with using functools.singledispatchmethod.

Implements #144

@matez0 matez0 force-pushed the add-serializer-to-response-wrapper branch from 4e4ac70 to e6869b9 Compare December 7, 2023 16:50
@armando-rodriguez-cko armando-rodriguez-cko self-assigned this Dec 7, 2023
@matez0 matez0 force-pushed the add-serializer-to-response-wrapper branch from e6869b9 to 0bc370d Compare December 11, 2023 19:18
@matez0
Copy link
Copy Markdown
Author

matez0 commented Dec 11, 2023

Updated the author's and committer's email address.

@armando-rodriguez-cko armando-rodriguez-cko linked an issue Jan 12, 2024 that may be closed by this pull request
@matez0
Copy link
Copy Markdown
Author

matez0 commented Jan 19, 2024

While the README states that "Requires Python > 3.6", the build was run for Python 3.6 as well.
Since the walrus operator only introduced in Python 3.8, I still should not have used it.
The commit avoiding to use walrus operator could be squashed (fixed up) with the commit adding the serializer.
Should I do it?

@matez0 matez0 force-pushed the add-serializer-to-response-wrapper branch from a9769b7 to ea783b5 Compare February 3, 2024 11:22
@matez0 matez0 force-pushed the add-serializer-to-response-wrapper branch from ea783b5 to ade758a Compare February 23, 2024 11:16
@matez0 matez0 force-pushed the add-serializer-to-response-wrapper branch from ade758a to d87f5d0 Compare March 19, 2024 23:02
@matez0 matez0 force-pushed the add-serializer-to-response-wrapper branch from d87f5d0 to 2287b32 Compare May 1, 2024 21:03
@matez0
Copy link
Copy Markdown
Author

matez0 commented May 1, 2024

Rename decorator and argument name for more readability.

@matez0 matez0 force-pushed the add-serializer-to-response-wrapper branch from 2287b32 to bb08bcb Compare May 1, 2024 21:06
@matez0
Copy link
Copy Markdown
Author

matez0 commented May 1, 2024

Rebased onto the top of main.

@matez0 matez0 force-pushed the add-serializer-to-response-wrapper branch from bb08bcb to 5f9fec1 Compare June 30, 2024 10:03
@matez0 matez0 force-pushed the add-serializer-to-response-wrapper branch from 5f9fec1 to 9c69e60 Compare December 23, 2024 13:48
@matez0
Copy link
Copy Markdown
Author

matez0 commented Dec 23, 2024

Rebased on the top of main.

matez0 added 2 commits April 2, 2025 15:30
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.
@matez0 matez0 force-pushed the add-serializer-to-response-wrapper branch from 9c69e60 to 2573f14 Compare April 2, 2025 13:30
@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud Bot commented Apr 2, 2025

@matez0
Copy link
Copy Markdown
Author

matez0 commented Apr 2, 2025

Rebased on the top of main.

@david-ruiz-cko david-ruiz-cko self-requested a review May 6, 2026 07:21
Copy link
Copy Markdown
Contributor

@david-ruiz-cko david-ruiz-cko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

https://github.com/checkout/checkout-sdk-python/blob/2573f14/checkout_sdk/checkout_response.py#L57-L65

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

Add serializer to response wrapper

3 participants