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

Do not serialize mappings with custom __repr__ as dict-s #1300

Closed

Conversation

vmarkovtsev
Copy link

Fixes #1296

@vmarkovtsev
Copy link
Author

@sl0thentr0py can you please review this 🙏

@sl0thentr0py sl0thentr0py self-requested a review January 18, 2022 20:48
@sl0thentr0py
Copy link
Member

Hey @vmarkovtsev, thx for the contribution!
Sadly this broke some tests, will investigate and get back to you.

@vmarkovtsev vmarkovtsev force-pushed the repr-mapping-serialization branch from 780bee9 to 8032cee Compare January 24, 2022 09:09
@vmarkovtsev
Copy link
Author

I have fixed some tests and the style, but the CI does not trigger again.

@vmarkovtsev vmarkovtsev force-pushed the repr-mapping-serialization branch from 8032cee to 9621427 Compare January 24, 2022 09:38
@vmarkovtsev vmarkovtsev force-pushed the repr-mapping-serialization branch from 9621427 to e1791a9 Compare January 25, 2022 16:37
@vmarkovtsev
Copy link
Author

As I see, the only test that's failing is the serialization of QueryDict in Django that has a custom __repr__ definition.
We can have a global list of class exclusions where the Django integration can append QueryDict, WDYT?

@sl0thentr0py
Copy link
Member

sl0thentr0py commented Jan 25, 2022

@vmarkovtsev yes I think we want to preserve older behavior for classes such as QueryDict here. I'm not sure what other things out there in the wild this will affect. Not sure what I think of another exclusion list tbh.

The other option here is to give you a custom def __sentry_repr__ that if available on any class will automatically short circuit the serializer and use that instead. Would that work for you?

@vmarkovtsev
Copy link
Author

Yes, __sentry_repr__ should work. Let's go that path then 👍

@sl0thentr0py
Copy link
Member

ok I'll make another PR then

@sl0thentr0py
Copy link
Member

closing in favor of #1322

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.

Support custom repr() defined in classes inheriting from Mapping
2 participants