search

vintasoftware / drf-rw-serializers

Generic views, viewsets and mixins that extend the Django REST Framework ones adding separated serializers for read and write operations
MIT License
177 stars 28 forks source link

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

Open sshishov opened 1 month ago

sshishov commented 1 month ago

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 commented 1 month ago

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

fjsj commented 1 month ago

@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 commented 1 month ago

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 commented 1 month ago

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 https://github.com/vintasoftware/drf-rw-serializers/pull/77 to fix the issues https://github.com/vintasoftware/drf-rw-serializers/issues/1, https://github.com/vintasoftware/drf-rw-serializers/issues/17, https://github.com/vintasoftware/drf-rw-serializers/issues/44.

causing incorrect serializer to be used for response

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