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

Are Serializer and BaseSerializer supported? #432

Closed
rfleschenberg opened this issue Jun 17, 2021 · 9 comments
Closed

Are Serializer and BaseSerializer supported? #432

rfleschenberg opened this issue Jun 17, 2021 · 9 comments
Labels
bug Something isn't working fix confirmation pending issue has been fixed and confirmation from issue reporter is pending

Comments

@rfleschenberg
Copy link

Hi,

I am using DRF's serializers.Serializer and serializers.BaseSerializer in a few places. I am running into errors because spectacular tries to access attributes on the serializer that are not present.

for field in serializer.fields.values():
throws AttributeError because my BaseSerializer subclass does not have fields.

model = field.parent.Meta.model
throws AttributeError because my Serializer subclass does not have Meta.

  • Is it intended that spectacular only supports ModelSerializer subclasses, or am I maybe missing something here?
  • Is there a good workaround for this?
  • As a last resort solution, is there an API / pattern that allows me to manually define the schema instead of relying on inspection? I checked https://drf-spectacular.readthedocs.io/en/latest/customization.html, but I am not sure what the right approach would be in this case. A custom preprocessing hook maybe?
@tfranzel
Copy link
Owner

Hi, drf-spectacular supports both Serializer and ModelSerializer, i.e. subclasses of those and anything included in DRF. having the fields attr is a assumptions that holds true for those.

writing your own serializer based on BaseSerializer apparently breaks some of the assumptions we made. pretty certain that your deviation requires writing a OpenApiSerializerExtension. We did that for example for rest_polymorphic (code). from there you can call back into the AutoSchema and reuse parts of spectacular.

if that is too much trouble you can annotate manually written raw schemas with extend_schema(operation=XXX) but that is a matter of last resort usually.

i don't think writing a preprocessing hook will get you far in this case.

if you need more advice i would need more insight into what your custom serializer baseclass is doing.

@rfleschenberg
Copy link
Author

Hi,

thanks for your reply. You're right that Serializer instances have the fields attribute, but they don't have Meta. I didn't find anything in the code that guards against the AttributeError before calling field.parent.Meta.model. Maybe I somehow ended up in a code path I wasn't supposed to. I have to find the time to reproduce this again, so I can provide the traceback, or ideally an MRE.

@tfranzel
Copy link
Owner

you end up in a codepath that uses Meta, because you have used a PrimaryKeyRelatedField that usually only makes sense on a ModelSerializer (where there must be a Meta by definition).

under the hood this field is connected to the model either via source or the field name. Not sure if that fields even works in absence of a model.

it is true that we do not guard against every "misusage". we sometimes just assume we get a valid and working serializer, i.e. a serializer that properly works in the API. There are a lot of ways to break serializers and we would have otherwise have 200+ more asserts in the AutoSchema. its a trade-off.

@tfranzel
Copy link
Owner

tfranzel commented Jul 1, 2021

closing this issue for now. feel free to comment if anything is missing or not working and we will follow-up.

@tfranzel tfranzel closed this as completed Jul 1, 2021
@Chadys
Copy link

Chadys commented Aug 23, 2021

@tfranzel I'm actually hitting this issue with a ReadOnlyField. When the field is of this type, we are executing this model = field.parent.Meta.model code path which is failing for a simple Serializer. A ReadOnlyField is pretty generic and does not make sense only for ModelSerializer. I hit this problem because I override TokenRefreshSerializer from rest_framework_simplejwt. I know that there is an OpenApiSerializerExtension to handle rest_framework_simplejwt's serializers, and I'm ok with having to use extend_schema_serializer for my usecase, since I know getting any information out of a ReadOnlyField outside of a ModelSerializer is near to impossible. But a more explicit message should be given instead of just crashing on every Serializer or ListSerializer that uses a ReadOnlyField. I would actually prefer no error at all, and just for the ReadOnlyField to be displayed without any details if it's not inside a ModelSerializer, so the user can add a extend_schema_serializer only if they want to.

@Chadys
Copy link

Chadys commented Aug 23, 2021

For anyone stumbling on the exact same issue as me, where I customize rest_framework_simplejwt's serializers and that makes drf-spectacular crash on ReadOnlyField, the simpler and cleaner way to resolve this is:

class YourCustomTokenObtainPairSerializerExtension(TokenObtainPairSerializerExtension):
    target_class = "your_app_name.serializers.YourCustomTokenObtainPairSerializer"

class YourCustomTokenRefreshSerializerExtension(TokenRefreshSerializerExtension):
    target_class = "your_app_name.serializers.YourCustomTokenRefreshSerializer"

But I still advocate for at least a better error message since that doesn't seem like a serializer's misusage to me 😄

@tfranzel
Copy link
Owner

I have revisited this issue and I was too quick to reject the use case. It is in fact possible to use ReadOnlyField outside of ModelSerializer, but I would call that an uncommon usage pattern. Most of the time I would assume CharField(read_only=True) is more appropriate as ReadOnlyField is in fact made for accessing the models. Nonetheless if it works for DRF, we must handle the case.

@Chadys I added a check to prevent the exception and warn on the situation. Warning resolution via decoration however is not that easy here. I would suggest following the new warning advice 😄

@tfranzel tfranzel added bug Something isn't working fix confirmation pending issue has been fixed and confirmation from issue reporter is pending labels Aug 25, 2021
@AdeelK2
Copy link

AdeelK2 commented Dec 22, 2022

Hey guys, recently I'm working on a project and have faced the same issue:

AttributeError: 'MuscleGroupAltSerializer' object has no attribute 'fields'

class MuscleGroupAltSerializer(serializers.BaseSerializer):
    def to_representation(self, obj):
        if obj is not None:
            if ('client_version' in self.context and
                self.context['client_version'] >= VERSION_FEATURE_SUPPORT['musclegroup_alt']):
                return MuscleGroupListSerializer(obj).data
            else:
                return obj.name
        else:
            return []

which is used in another serializer like: musclegroup = MuscleGroupAltSerializer(many=True)

Any suggestions will be appreciated.

@tfranzel
Copy link
Owner

@AdeelK2 so internally we check for isinstance(BaseSerializer) and consider it valid usage, which has a technical reason behind it. To be 100% correct, we would need to require isinstance(Serializer) instead, since it has the fields attribute which we make use of. That is why there is this unpleasant AttributeError.

We do allow BaseSerializer but then require that you take care of the fields attribute yourself. ListSerializer are unaffected. We basically expect serializers to have fields by default. You can go around this requirement by writing a OpenApiSerializerExtension.

Even if we catch that AttributeError, you wouldn't get too far anyway. It would be basically impossible to introspect what you do in to_representation. A custom to_representation almost always means writing a OpenApiSerializerExtension.

If this is a one off, I would recommend you to use @extend_schema(responses=MuscleGroupListSerializer) on the endpoint method OR @extend_schema_view(...) on the view to circumvent the attempt to parse MuscleGroupAltSerializer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working fix confirmation pending issue has been fixed and confirmation from issue reporter is pending
Projects
None yet
Development

No branches or pull requests

4 participants