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

SlugRelatedField with DataclassSerializer #897

Closed
HansAarneLiblik opened this issue Dec 14, 2022 · 12 comments
Closed

SlugRelatedField with DataclassSerializer #897

HansAarneLiblik opened this issue Dec 14, 2022 · 12 comments
Labels
bug Something isn't working fix confirmation pending issue has been fixed and confirmation from issue reporter is pending

Comments

@HansAarneLiblik
Copy link

HansAarneLiblik commented Dec 14, 2022

Describe the bug
With version 0.25.0, our schema generation started to fail, because we use SlugRelatedField with DataclassSerializer not ModelSerializer, and therefore do not have field.parent.Meta.model field, but have provided queryset attr to SlugRelatedField constructor, which should be used to detect the type instead

Source of bug: 0307ff9#diff-4d081c8c817b2246109e52886d6b1f078858a2d5c99ea6e694796e39a7d96d77R662
#893

To Reproduce

from dataclasses import dataclass

from django.db import models
from rest_framework import serializers
from rest_framework_dataclasses.serializers import DataclassSerializer

class MyModel(models.Model):
    name = models.CharField(max_length=200, unique=True)
    value = models.IntegerField()


@dataclass()
class MyBase:
    obj: MyModel


class MyBaseSerializer(DataclassSerializer):
    slug_name = serializers.SlugRelatedField(slug_field="name", source="obj", queryset=MyModel.objects)

    class Meta:
        dataclass = MyBase
        fields = ["slug_name"]

AttributeError: type object 'Meta' has no attribute 'model'

Expected behavior
model to be determined from provided queryset
model = field.queryset.model if hasattr(field, "queryset") else getattr(field.parent.Meta, "model", None)

or perhaps try to deduct from dataclass field type ?

@tfranzel
Copy link
Owner

of course there are multiple ways of using this field. 😞 I had a gut feeling this may produce issues. will look into it.

@tfranzel tfranzel added the bug Something isn't working label Dec 14, 2022
@tfranzel
Copy link
Owner

tfranzel commented Dec 14, 2022

@HansAarneLiblik please review and test #899. hopefully this will do the trick.

@HansAarneLiblik
Copy link
Author

@tfranzel Seems to work perfectly !

tfranzel added a commit that referenced this issue Dec 15, 2022
utilize queryset for SlugRelatedField #897
@HansAarneLiblik
Copy link
Author

Not related to this, but my test between 0.24 and 0.25 showed that schema generation speed has drastically decreased. If needed I can generate some numbers

@tfranzel
Copy link
Owner

Well, i'm pretty sure this is due to your requested feature #866

I found that these two lines for getting the source are quite expensive: https://github.com/tfranzel/drf-spectacular/pull/889/files#diff-d93b8eca9df4d268a9d1ae33010b6119414e6a61a5d736e39a1650f8975a72edR114-R115

It doubled the test suite execution time by itself. However, I added a cache which brought it down to the previous performance. I thought 1000 cache size is a fair guestimate for number of serializer/views in an API.

Please provide some number so we can further improve this

@HansAarneLiblik
Copy link
Author

330kb yaml 13.6k lines
77 Errors
20 Warnings (We're working on them, 😿 )

0.24.2:
1.25s per API request

0.25.0:
initial: 6-8 seconds
Cached 1.25s

NOTE: These values about double when running Django in debug mode

About the results - I love that it shows the file path, but it only does it for a few cases. Most of our errors don't have it.

(Of the ~100 errors, these following 8 are the only cases they exist)

image

@tfranzel
Copy link
Owner

330kb yaml 13.6k lines

😅 thats a hefty one

Cached 1.25s

sounds good and it looks like the cache size is sufficient.

initial: 6-8 seconds

That is to be expected I suppose unless there is a more efficient stdlib method for getting the file and line number, which I couldn't find.

NOTE: These values about double when running Django in debug mode

Not sure why that is exactly. We don't do anything special for debug=True.

About the results - I love that it shows the file path, but it only does it for a few cases. Most of our errors don't have it.

That is also mostly expected. There is no path for warn/errors that are happening outside of the actual view parsing, e.g. preprocessing, postprocessing, setup. Those happen outside of any add_trace_message decoration.

I'm considering turning off the filepath for SpectacularSchemaView altogether due to the performance hit. Imho bug tracing should happen through the CLI command anyway, where it would be available.

@tfranzel
Copy link
Owner

@HansAarneLiblik however if those msg are unable to guess serializer, they should be wrapped in the add_trace_message for the view. That would mean those 2 lines fail silently for some reasons. You would need to investigate why.

@tfranzel
Copy link
Owner

I think I found the issue regarding missing paths. You are using a lot of @api_view decorators and due to DRF magic and wrapping, the actual line number is lost. However we exit early and don't use path which we would have.

@HansAarneLiblik
Copy link
Author

Indeed we're using mostly @apiview() decorator

@tfranzel
Copy link
Owner

tfranzel commented Dec 15, 2022

@HansAarneLiblik what do you think about only showing the path (and line if available) in CLI only (./manage.py spectacular --file /dev/null --color). This would have no performance penality for the actual API.

@HansAarneLiblik
Copy link
Author

Sure, Seems like a reasonable way to go !

tfranzel added a commit that referenced this issue Dec 16, 2022
CLI will run with line numbers, Schema view will not.
@tfranzel tfranzel added the fix confirmation pending issue has been fixed and confirmation from issue reporter is pending label Dec 16, 2022
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

2 participants