Closed danilocastelhano1 closed 4 years ago
tfranzel
commented
4 years ago hey! it is nearly impossible to guess what went wrong there. all i can see that it is probably a heavily nested serializer (which should be ok) and that the assert got hit by something that was not a serializer. this should be a very unlikely event. i would need some kind of serializer example or reproduction on how to provoke the error.
jayvdb
commented
4 years ago I just hit this.
I suggest that assert is never used without a second arg which explains the failed assertion. There are code quality tools which can enforce this. If you want these assertions to cause issues to be raised, it helps to have assertion messages let people raise bugs which can be used to identify the problem.
Since this component is py36+, it could also use should_be instead of assert, which auto-voodoo creates useful assertion error messages.
And as usual, I would prefer these are warnings. In my case it is a custom lazy-ily defined serializer and it could be safely ignored as inspecting it wont work anyway. The following worked for me.
- assert is_serializer(serializer)
+ if not is_serializer(serializer):
+ returncertainly something to consider. i did not anticipate this being hit, except for very rare cases. i'll look into it whether a more meaningful assert message or a non-fatal error/warn is more appropriate.
should_be looks nice but i don't want to add 2 deps for this one line.
@danilocastelhano1 which library with customized serializer logic did you use?
tfranzel
commented
4 years ago @jayvdb i had another look and i remember now. that assertion is supposed to be just that,a is a basic assumption that "must" hold true. (Almost) all callers of that function ensure that assumption and if not throw a warning and do a fallback. The only way i can see to get there unchecked is by either using PolymorphicSerializer or PolymorphicProxySerializer.
can you elaborate on how you got that error? that would be most helpful. i might have missed a case somewhere.
danilocastelhano1
commented
4 years ago @tfranzel i use docusign library, there is a big serializer in there
tfranzel
commented
4 years ago @danilocastelhano1 i see.
PolymorphicSerializer or PolymorphicProxySerializer in combination with that serializer? @extend_schema there. danilocastelhano1
commented
4 years ago in docusign, I use customizable serializers, I believe this is what is causing the exception, if I change my serializer to one that is: MySerializer (serializers.ModelSerializer), it works correctly, but if I create a customizable serializer: eg: MyCustomSerializer ( serializers.Serializer), generate the mentioned error.
tfranzel
commented
4 years ago ahh i understand, but customized serializers should never provoke this error, as long as they are based on serializers.BaseSerializer. that includes serializers.Serializer!
pretty much every test we have is using this, where it has never been a problem. example: class LikeSerializer(serializers.Serializer):
also the check is pretty trivial:
are you sure you are actually using rest_framework.serializers.Serializer as base class? something must be different. i'm a bit puzzled.
danilocastelhano1
commented
4 years ago i think i found the error, it is in:
my_field = ListSerializer(required=False, child=serializers.CharField(required=False))when i use ListSerializer, gives the error above, ListSerializer belongs to rest_framework->serializers.py i'm using this version of djangorestframework==3.11.0
jayvdb
commented
4 years ago can you elaborate on how you got that error? that would be most helpful. i might have missed a case somewhere.
You have missed the case where the serializer doesnt inherit from those classes. :-) https://github.com/peopledoc/django-formidable/blob/master/formidable/serializers/child_proxy.py is the one I encountered a few days ago.
tfranzel
commented
4 years ago @danilocastelhano1 ... ahh that makes absolute sense! now we are getting to the bottom of it. DRF automatically translates XSerializer(many=True) to ListSerializer(child=XSerializer()) internally. so the assumption is that child needs to be a serializer and not a field. i have to check if what you are doing is actually officially alllowed by DRF. i'm not so sure.
https://www.django-rest-framework.org/api-guide/serializers/#listserializer The ListSerializer class provides the behavior for serializing and validating multiple objects at once. You won't typically need to use ListSerializer directly, but should instead simply pass many=True when instantiating a serializer.
tfranzel
commented
4 years ago @jayvdb well i wouldn't call that missed. that is on purpose. DRF required the Serializer interface from objects that are supposed to be "serializers". an arbitrary object will not do here. it is a safeguard against crazy and literally every non-fringe DRF app implements that interface. There is a lot of magic going on in Views and Serializers that we rely on. Same goes for DRF itself.
We have PolymorphicProxySerializer (not derived from BaseSerializer) which does similar proxy magic. special things that deviate from the norm require a Extensions like PolymorphicProxySerializerExtension. Otherwise all bets are off. We have to draw a line somewhere :smile:
danilocastelhano1
commented
4 years ago I think it is official, because in drf-yasg works good, and in drf-spectacular gives the error
You can create an serializer: class TestSerializer(serializers.Serializer):
field1 = serializers.CharField(required=False)
# Works fine!
field2 = serializers.IntegerField(required=False)
# Error in fied 3!
field3 = ListSerializer(required=False, child=serializers.CharField(required=False))and a viewset: class TestViewSet(viewsets.GenericViewSet, mixins.ListModelMixin, mixins.RetrieveModelMixin): queryset = Test.objects.all() permission_classes = [IsAuthenticated] serializer_class = TestSerializer
tfranzel
commented
4 years ago @danilocastelhano1 well that was a surprise! i validated that it works on the API as you said. today i learnt something new :smile: ListSerializer accepts Field as child, which means it is indeed a bug. still i think its unusual usage. i'll come up with a bugfix. thanks for digging in.
danilocastelhano1
commented
4 years ago Glad to help and improve the tool. tks.
tfranzel
commented
4 years ago @danilocastelhano1 can you check if that works for you? close if issue is resolved.
fwiw, my confusion came from the fact that there is a dedicated class for exactly that purpose: serializers.ListField. that would have worked right off the bat. However, DRF code mentions that they are closely related, but since your usage does also work, we will support both cases.
jayvdb
commented
4 years ago Otherwise all bets are off. We have to draw a line somewhere
I agree. I just wish you didnt force failure on my entire API because of one part of it that I dont want or expect to be exposed in the OpenAPI.
tfranzel
commented
4 years ago @jayvdb lets move your problem to another issue (#126). they hit the same assert but because of different reasons.
danilocastelhano1
commented
4 years ago morning guys, how can i test? by pip install drf-spectacular? first i ran pip uninstall drf-spectacular then i ran pip install drf-spectacular so the error persists, i noted that file: openapi.py do not changed, in github the file (https://github.com/tfranzel/drf-spectacular/blob/master/drf_spectacular/openapi.py) has 943 lines vs 933 when i ran pip install drf-spectacular
tfranzel
commented
4 years ago morning!
pip uninstall drf-spectacular
pip install git+https://github.com/tfranzel/drf-spectacular should do it. then you have the current master state
danilocastelhano1
commented
4 years ago Nice @tfranzel Woked good thanks a lot!
i have an serializer who give me an exception, follow my traceback: