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

fix(serializer): fix RecursionError without changing signature #94

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

sshishov
Copy link

@sshishov sshishov commented Aug 3, 2024

Description: fix RecursionError without changing signature

The inspect library is used instead to get the caller information. This information gives us a clue, if it is recursion call or not.

Dependencies: this is fixes the PR where additional default_to_serializer_class was added to the signature

@sshishov
Copy link
Author

sshishov commented Aug 4, 2024

Seems not working on real package, but passing tests. checking...

The `inspect` library is used instead to get the `caller` information.
This information gives us a clue, if it is recursion call or not.

Signed-off-by: Sergei Shishov <[email protected]>
@sshishov sshishov force-pushed the ss/fix/signature-issue-better-fix branch from 37c67ac to 89de758 Compare August 4, 2024 21:10
@fjsj
Copy link
Member

fjsj commented Aug 5, 2024

@sshishov thanks, I thought some frame-inspection solution could solve this, and this looks good on a first glance... but I wonder about the performance impact. Could you please try to measure what's the delay of calling currentframe().f_back.f_code.co_name at that context and report back results?

@sshishov
Copy link
Author

sshishov commented Aug 5, 2024

Hi @fjsj , the second push working for us, I have checked.

About performance penalty, how you would recommend us to test it?
I am pretty sure that penalty will be very small and almost not noticeable as there are not IO involved, all this data is already present in the context, no?

I have tested the solution and found one huge flaw which I do not how to solve in long term.

The problem is: some renderers (like XLS renderer in pandas I guess) is using serializer to get headers information and other stuff, and usually it is used in POST method. Like POST export with some paramters. Here we should have 2 serializers, one is write (for query params) and another is read for response. Unfortunately the renderer is using self.get_serializer() logic which is falling back to write serializer as the request.method is POST, and causing incorrect serializer to be used for response...

You can try to reproduce it yourself in the test...

@fjsj
Copy link
Member

fjsj commented Aug 5, 2024

I am pretty sure that penalty will be very small and almost not noticeable as there are not IO involved, all this data is already present in the context, no?

For IO, yes. The trouble is introspection in Python can have some impact (the currentframe() call). You can test performance via timeit, by making a script that instantiates GenericAPIView and call get_serializer_class.

Unfortunately the renderer is using self.get_serializer() logic which is falling back to write serializer as the request.method is POST, and causing incorrect serializer to be used for response...

Yes, that's the new behavior we introduced at #77 to fix the issues #1, #17, #44.

causing incorrect serializer to be used for response

Not clear to me what's breaking. It's the tests or your code?

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.

2 participants