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

Request component properties not marked as required when using COMPONENT_SPLIT_REQUEST=True #243

Closed
rvinzent opened this issue Dec 30, 2020 · 10 comments

Comments

@rvinzent
Copy link

Describe the bug
When setting the COMPONENT_SPLIT_REQUEST setting to True, the generated components do not indicate that any field is required.

To Reproduce

  • COMPONENT_SPLIT_REQUEST=True in settings

This serializer produces the following components:

class RequiredSerializer(serializers.Serializer):
    read_only_property = serializers.CharField(read_only=True, required=True)
    required_property = serializers.CharField(required=True)
    Required:
      type: object
      properties:
        read_only_property:
          type: string
          readOnly: true
        required_property:
          type: string
      required:
      - read_only_property
      - required_property
    
    RequiredRequest:
      type: object
      properties:
        required_property:
          type: string

Expected behavior
In the above example RequiredRequest is missing the required properties list. It should generate

    RequiredRequest:
      type: object
      properties:
        required_property:
          type: string
      required:
      - required_property

This is causing issues with client code generation tools.

@rvinzent rvinzent changed the title Component properties not marked as required when using COMPONENT_SPLIT_REQUEST=True Request component properties not marked as required when using COMPONENT_SPLIT_REQUEST=True Dec 30, 2020
@tfranzel
Copy link
Owner

tfranzel commented Dec 31, 2020

>       assert not (read_only and required), NOT_READ_ONLY_REQUIRED
E       AssertionError: May not set both `read_only` and `required`

../venv/lib/python3.8/site-packages/rest_framework/fields.py:336: AssertionError

is invalid according to DRF and won't run. you cannot set read_only=True and required=True at the same time. Also its important to note that DRF's read_only semantics differ from the drf-spectacular/OpenApi semantics. spectacular will put them where it can automatically.

simply removing required=True on read_only_property will yield the following for me. Looks like your expected behavior as far as i can tell.

    Required:
      type: object
      properties:
        read_only_property:
          type: string
          readOnly: true
        required_property:
          type: string
      required:
      - read_only_property
      - required_property
    RequiredRequest:
      type: object
      properties:
        required_property:
          type: string
      required:
      - required_property

@rvinzent
Copy link
Author

rvinzent commented Dec 31, 2020

Oops, yea you can't explicitly set required=True, but if I remove read_only=True the parameter will appear in the generated _Request component, which is not what I want.

I only included the read only property here to show some difference between the generated Request component and the regular one, but it's not necessary to reproduce the issue. If you remove the read only field from the above example, I would still expect the required field to be marked required in the Request component spec.

class RequiredSerializer(serializers.Serializer):
    required_property = serializers.CharField(required=True)

-->

    Required:
      type: object
      properties:
        required_property:
          type: string
      required:
      - required_property
    
    RequiredRequest:
      type: object
      properties:
        required_property:
          type: string
      required:
      - required_property     # this is not included at the moment, even though it is a required field 

@rvinzent
Copy link
Author

rvinzent commented Dec 31, 2020

@tfranzel I'm going to open an issue on DRF because frankly, a read_only field does not imply the field is required=False. It turns out it's possible to optionally include required=False fields on the serialization path. DRF checks if the attribute exists when deciding to include it in the payload. I might want to indicate to DRF that the attribute must be present, even if the field is read_only.

encode/django-rest-framework#7681

@tfranzel
Copy link
Owner

tfranzel commented Jan 1, 2021

  • required_property # this is not included at the moment, even though it is a required field

i cannot confirm this statement. for me this is actually included.

i tried to compile all the cases here:

    class XSerializer(serializers.Serializer):
        plain_property = serializers.CharField()

        required_property = serializers.CharField(required=True)
        non_required_property = serializers.CharField(required=False)

        read_only_property = serializers.CharField(read_only=True)
        read_only_non_required_property = serializers.CharField(read_only=True, required=False)

        write_only_property = serializers.CharField(write_only=True)
        write_only_non_required_property = serializers.CharField(write_only=True, required=False)

which yielded for me:

    X:
      type: object
      properties:
        plain_property:
          type: string
        required_property:
          type: string
        non_required_property:
          type: string
        read_only_property:
          type: string
          readOnly: true
        read_only_non_required_property:
          type: string
          readOnly: true
      required:
      - plain_property
      - read_only_non_required_property
      - read_only_property
      - required_property
    XRequest:
      type: object
      properties:
        plain_property:
          type: string
        required_property:
          type: string
        non_required_property:
          type: string
        write_only_property:
          type: string
          writeOnly: true
        write_only_non_required_property:
          type: string
          writeOnly: true
      required:
      - plain_property
      - required_property
      - write_only_property

the required_property is included in both schema's required list, so its looks like as expected to me.

the only thing that bothers me here is read_only_non_required_property being in the response's required list, but i have not tested whether this is actually honored by DRF.

@rvinzent
Copy link
Author

rvinzent commented Jan 4, 2021

Hm, I guess my issue is more complicated than that. Thanks for looking into it! I'll dig into find the real root cause at some point

@AlexChalk
Copy link

How would I go about telling spectacular that my field is read_only=True and required=False? (Happy to open a separate issue).

@tfranzel
Copy link
Owner

tfranzel commented May 6, 2021

https://drf-spectacular.readthedocs.io/en/latest/settings.html

you can either go the proper route with COMPONENT_SPLIT_REQUEST, but that will result in more components. this is particular useful for client generation but in general will yield the most accurate description.

if you just want to fix the required issue you can simply turn this off with COMPONENT_NO_READ_ONLY_REQUIRED. the name says it all i suppose.

@dashdanw
Copy link

I tried setting COMPONENT_SPLIT_REQUEST=True and setting a field
somefield = serializersCharField(requred=False, read_only=True) is still being marked as Required in the schema, am I doing something wrong?

@tfranzel
Copy link
Owner

if you just want to fix the required issue you can simply turn this off with COMPONENT_NO_READ_ONLY_REQUIRED. the name says it all i suppose.

the answer is right there. the setting is off by default because serializers almost always contain fields on response even if they are not required. My interpretation of the required flag is that it only applies on the request.

@dashdanw
Copy link

I guess as a note, tools like schemathesis seem to use the required field to know whether a field can be expected to exist in a response

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants