Skip to content

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

Closed
vmarkovtsev wants to merge 1 commit into
getsentry:masterfrom
vmarkovtsev:repr-mapping-serialization
Closed

Do not serialize mappings with custom __repr__ as dict-s#1300
vmarkovtsev wants to merge 1 commit into
getsentry:masterfrom
vmarkovtsev:repr-mapping-serialization

Conversation

@vmarkovtsev

Copy link
Copy Markdown
Contributor

Fixes #1296

@vmarkovtsev

Copy link
Copy Markdown
Contributor Author

@sl0thentr0py can you please review this 🙏

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

Copy link
Copy Markdown
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
Copy Markdown
Contributor 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
Copy Markdown
Contributor 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

sl0thentr0py commented Jan 25, 2022

Copy link
Copy Markdown
Member

@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
Copy Markdown
Contributor Author

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

@sl0thentr0py

Copy link
Copy Markdown
Member

ok I'll make another PR then

@sl0thentr0py

Copy link
Copy Markdown
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