tfranzel / drf-spectacular

Sane and flexible OpenAPI 3 schema generation for Django REST framework.
https://drf-spectacular.readthedocs.io
BSD 3-Clause "New" or "Revised" License
2.33k stars 259 forks source link

Model fields that contain a `default` value are not marked as required in response API #1299

Open johnthagen opened 3 days ago

johnthagen commented 3 days ago

Describe the bug

Using the default Django User model, creating a Serializer and ViewSet from this leads to many of the fields being optional in the response model that should always be "required"/assured to be there.

Consider:

from django.contrib.auth.models import User
from rest_framework.serializers import ModelSerializer

class UserSerializer(ModelSerializer):
    class Meta:
        model = User
        fields = "__all__"

And

from django.contrib.auth.models import User
from rest_framework.mixins import (
    CreateModelMixin,
    ListModelMixin,
    RetrieveModelMixin,
    UpdateModelMixin,
)
from rest_framework.permissions import AllowAny

class UserViewSet(
    CreateModelMixin, RetrieveModelMixin, UpdateModelMixin, ListModelMixin, GenericViewSet
):
    queryset = User.objects.all()
    serializer_class = UserSerializer
    permission_classes = [AllowAny]
    filter_backends = []

This creates a schema where many of the fields (such as is_staff) are optional even for the User response type:

Screenshot 2024-09-25 at 11 53 20 AM

But I'm not sure why they would be marked optional in the response, as my API will always return a bool for them? I see in the Model definitions they have default values, perhaps that is related?

class AbstractUser(AbstractBaseUser, PermissionsMixin):
    ...
    is_staff = models.BooleanField(
        _("staff status"),
        default=False,
        help_text=_("Designates whether the user can log into this admin site."),
    )

I'm still not sure if this is a bug, or some kind of expected behaviour for this model.

The problem this causes is that in generated clients such as Typescript the code is:


  /**
   * Designates whether the user can log into this admin site.
   */
  is_staff?: boolean;

And the ? denotes it is optional and all clients then have to do a null check even through the API will always return a value for this field.

In a generated Python client, this looks like the following, with an extra Unset state:

is_staff: Union[Unset, bool] = UNSET

Environment

tfranzel commented 3 days ago

Hi @johnthagen,

One OpenAPI component is not able to fully cover what a DRF serializer can do. What you see is a carefully crafted compromise that would work both for the request and response. If you want to have a better representation, you need to enable COMPONENT_SPLIT_REQUEST as the docs suggest:

https://drf-spectacular.readthedocs.io/en/latest/client_generation.html#component-issues

This is especially important for client generation due to the stated reasons. You will then get 2 components that together very closely resemble what the serializer actually does.

Regarding the required flag, the field is marked as required if the serializer field is marked (or auto-generated) as required or if the field is readOnly. You have to realize that DRF's required field deals with the request side of things. It is not statement about the response. We can only make one simplification for readOnly fields, where making the field required it is not a regression on requests.

Can't really say anything about the specific generator target, but with the COMPONENT_SPLIT_REQUEST, most generator issues go away.

johnthagen commented 3 days ago

@tfranzel Thanks for this response.

If you want to have a better representation, you need to enable COMPONENT_SPLIT_REQUEST as the docs suggest

Sorry, I should have also mentioned my drf-spectacular settings, as I always have that set to True due to the important rationale in the docs (this setting is very important!).

SPECTACULAR_SETTINGS = {
    "COMPONENT_SPLIT_REQUEST": True,
    # drf-spectacular-sidecar settings
    "SWAGGER_UI_DIST": "SIDECAR",
    "SWAGGER_UI_FAVICON_HREF": "SIDECAR",
    "REDOC_DIST": "SIDECAR",
    ...
}

In my first post, I tried to simplify my example by not including the generated UserRequest object, but I did generate it locally. Here are both:

Screenshot 2024-09-26 at 8 09 34 AM

I think this will help summarize the crux of my question. If I swap the built in User model for my own:

from django.db import models

class MyUser(models.Model):
    username = models.TextField()
    is_staff = models.BooleanField()
    is_superuser = models.BooleanField()

class UserSerializer(ModelSerializer):
    class Meta:
        model = MyUser
        fields = "__all__"

Then the generated OpenAPI spec is correctly generated as I expect. The response type in the API guarantees that is_staff and is_superuser will always be present.

Screenshot 2024-09-26 at 8 24 08 AM

This is the expected behaviour for my typical Serializers and ViewSets, but I'm trying to figure out why the built in User model does not exhibit this behavior, especially on the generated response type (User in the API).

johnthagen commented 3 days ago

Okay, I seem to have located the cause. It is the default in the Models field, as I suspected.

If I change to:

class MyUser(models.Model):
    username = models.TextField()
    is_staff = models.BooleanField(default=True)
    is_superuser = models.BooleanField()

is_staff is no longer set as required in the response object.

    User:
      type: object
      properties:
        id:
          type: integer
          readOnly: true
        username:
          type: string
        is_staff:
          type: boolean
        is_superuser:
          type: boolean
      required:
      - id
      - is_superuser
      - username
Screenshot 2024-09-26 at 8 29 15 AM

To me, this makes sense for the request type (e.g., a user does not need to provide is_staff because Django will put in the default if the user does not provide it).

But for the response type, DRF will always return the is_staff field, so it should be marked required, right?

Perhaps there is some logic with regards to looking at default values in Model fields that drf-spectacular should differientiate between response and request processing?


I think that I personally have not run into this issue before because I just checked our codebase and we do not mark any of our Model fields with a default=. But we do use the built in User from Django, and it has default= set, so this is why it finally surfaced.