Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle SimpleLazyObject in payloads #297

Merged
merged 3 commits into from
Dec 18, 2018
Merged

Conversation

rokob
Copy link
Contributor

@rokob rokob commented Nov 27, 2018

This needs some cleanup, but the basic idea is to handle SimpleLazyObject via a custom JSONEncoder subclass

Fixes #295

@brianr
Copy link
Member

brianr commented Nov 28, 2018

I'm wondering if we would want this same behavior for other things that aren't json serializable. Like... is there ever a case where we would want _serialize_payload to raise an error instead of returning something that's at least partially serialized / sorta useful?

@rokob
Copy link
Contributor Author

rokob commented Nov 28, 2018

Yeah that is a good point. Instead of throwing a TypeError, I will just put a placeholder.

@rokob
Copy link
Contributor Author

rokob commented Nov 28, 2018

So I should have tried to write the test first, I don't know why rollbar/lib/transforms/serializable.py does not handle this.

@rokob
Copy link
Contributor Author

rokob commented Nov 28, 2018

Specifically, the payload gets built here:

payload = _build_payload(data)

where _build_payload (https://github.com/rollbar/pyrollbar/blob/487a78c427c8021d46f2d211e34c500593bcb706/rollbar/__init__.py#L1314,L1327) calls _transform which applies the transforms to each key/value:

obj = transforms.transform(obj, transform, key=key)

One of those transforms is a serializer transform which turns everything into a bare Python type, falling back to repr(x) or str(type(x)) depending on what happens. So I have no idea how an object can end up in the payload and make it past that transform to the json encoder. Unless someone removes/modifies the transforms. This is still useful as a fallback as apparently this is possible, but I can't seem to get it to happen in a test.

@rokob rokob merged commit b9caf5d into master Dec 18, 2018
@rokob rokob deleted the serialize-simple-lazy-object branch December 18, 2018 22:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants