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

[Blueprint] DRF writable nested serializers #1225

Open
vabene1111 opened this issue Apr 22, 2024 · 6 comments
Open

[Blueprint] DRF writable nested serializers #1225

vabene1111 opened this issue Apr 22, 2024 · 6 comments

Comments

@vabene1111
Copy link

DRF writable nested serializers is a somewhat popular DRF plugin that simplifies writing trough nested serializer relations (see https://github.com/beda-software/drf-writable-nested)

Problem

The problem is, that the default id field is marked as readonly in the schema or, when using COMPONENT_SPLIT_REQUEST, completely omitted. Which is correct from a schema perspective as far as I can judge it.
This leads some frontend clients to remove the id field from the request body (which is also perfectly reasonable) but the nested object will also have the id removed which means that the nested serializer is no longer capable of deciding if the object should be created or updated.

In practice this means nested objects get stripped of their IDs before being send to the endpoint and the nested object will be re-created on every request as it treats the object as new.

possible solution / workaround

The following code is something I came up with in order to solve this issue for my project. I tried to make it as general and robust as possible but as I am not a schema expert. It also sacrifices schema accuracy to a degree and has a potential for certain edge cases to cause errors (especially if a model/schema should be named Request or contain the word).

Maybe it helps someone searching for this or if you deem it good enough can be provided as a blueprint with maybe a warning. I am also happy to work with someone who understands the whole schema generation a bit better to implement/test further improvements.

# custom processing for schema
# reason: DRF writable nested needs ID's to decide if a nested object should be created or updated
# the API schema/client make ID's read only by default and strips them entirely in request objects (with COMPONENT_SPLIT_REQUEST enabled)
# change request objects (schema ends with Request) and response objects (just model name) to the following:
#   response objects: id field is required but read only
#   request objects: id field is optional but writable/included

# WARNING: COMPONENT_SPLIT_REQUEST must be enabled, if not schemas might be wrong

def custom_postprocessing_hook(result, generator, request, public):
    for c in result['components']['schemas'].keys():
        # handle schemas used by the client to do requests on the server
        if c.strip().endswith('Request'):
            # check if request schema has a corresponding response schema to avoid changing request schemas for models that end with the word Request
            response_schema = None
            if c.strip().replace('Request', '') in result['components']['schemas'].keys():
                response_schema = c.strip().replace('Request', '')
            elif c.strip().startswith('Patched') and c.strip().replace('Request', '').replace('Patched', '', 1) in result['components']['schemas'].keys():
                response_schema = c.strip().replace('Request', '').replace('Patched', '', 1)

            # if response schema exist update request schema to include writable, optional id
            if response_schema and 'id' in result['components']['schemas'][response_schema]['properties']:
                if 'id' not in result['components']['schemas'][c]['properties']:
                    result['components']['schemas'][c]['properties']['id'] = {'readOnly': False, 'type': 'integer'}
                # this is probably never the case but make sure ID is not required anyway
                if 'required' in result['components']['schemas'][c] and 'id' in result['components']['schemas'][c]['required']:
                    result['components']['schemas'][c]['required'].remove('id')
        # handle all schemas returned by the server to the client
        else:
            if 'properties' in result['components']['schemas'][c] and 'id' in result['components']['schemas'][c]['properties']:
                # make ID field not read only so it's not stripped from the request on the client
                result['components']['schemas'][c]['properties']['id']['readOnly'] = True
                # make ID field required because if an object has an id it should also always be returned
                if 'required' not in result['components']['schemas'][c]:
                    result['components']['schemas'][c]['required'] = ['id']
                else:
                    result['components']['schemas'][c]['required'].append('id')

    return result

conclusion

Thank you for this awesome library and all the work you have done.
If you are not interested please just close it, people with the same problem searching for it will likely find it if needed.

@vabene1111
Copy link
Author

ok so while the solution above is the most correct it poses some challenges with the mix of response and request types on the frontend. For my personal need I switched to not splitting the objects and basically just making ID's not required using this hook.

If I encounter any other improvements or issues I will post here

# custom processing for schema
# reason: DRF writable nested needs ID's to decide if a nested object should be created or updated
# the API schema/client make ID's read only by default and strips them entirely in request objects (with COMPONENT_SPLIT_REQUEST enabled)
# change the schema to make IDs optional but writable so they are included in the request

def custom_postprocessing_hook(result, generator, request, public):
    for c in result['components']['schemas'].keys():
        # handle schemas used by the client to do requests on the server
        if 'properties' in result['components']['schemas'][c] and 'id' in result['components']['schemas'][c]['properties']:
            # make ID field not read only so it's not stripped from the request on the client
            result['components']['schemas'][c]['properties']['id']['readOnly'] = False
            # make ID field not required
            if 'required' in result['components']['schemas'][c] and 'id' in result['components']['schemas'][c]['required']:
                result['components']['schemas'][c]['required'].remove('id')

    return result

@tfranzel
Copy link
Owner

Hi @vabene1111,

interesting! So you are saying drf-writable-nested serializers behave differently depending on their location inside the payload (root vs nested). That is indeed not covered by spectacular, as we assume serializers behave identical regardless of usage location.

I would have probably gone with your first approach:

  • Leave response as is.
  • Relax request object by leaving in the id and making it optional&writable (thus covering both root/non-root cases)

I would have chosen a different way though. I would have likely created a subclass CustomAutoSchema and modified _postprocess_serializer_schema I think this would be a lot easier than reconstructing removed stuff in a hook after the fact.

Alternatively (and even preferrable) a OpenApiSerializerExtension might have been possible too, if their generated serializers have some kind of identifying subclass, mixin or something.

ok so while the solution above is the most correct it poses some challenges with the mix of response and request

yes, depending on the client target, this may pose problems. However, this is a general problem not limited to this particular plugin.

I'm short on time right now, so I cannot do this by myself. Would you like to look into one of those options? CustomAutoSchema would be a blueprint. The extension could even be elevated to full support (if there are no other blockers). Let me know.

@vabene1111
Copy link
Author

Hi, thanks for your feedback.

interesting! So you are saying drf-writable-nested serializers behave differently depending on their location inside the payload (root vs nested). That is indeed not covered by spectacular, as we assume serializers behave identical regardless of usage location.

The problem is the loss of context. On the root view the ID comes from the ID parameter of the get request /api/someview/id. The nested objects don't have that context so if a nested object is part of the request the serializer cannot know if the nested object is just a new object that doesn't have an ID yet or if its an existing object in the database that has an ID that was just stripped from it by the client.

If you recommend an extension or CustomAutoSchema I will try to look into it. I need support for this for my open source project which is consuming most of my time but I also want a proper solution for it so having it as a blueprint or even officially supported would be awesome, so I will try to find some time too look into it.

For now at least people have a starting point from this issue.

@tfranzel
Copy link
Owner

The problem is the loss of context. On the root view the ID comes from the ID parameter of ...

Yes, I got that. I was merely stating it is uncommon that a DRF serializer behaves differently based on the location i.e. context.

I think the popularity of the lib makes native support worth considering. I think they do work with mixins a lot, so the extension route might be viable.

No worries, give it a try when you have the time. Happy to give feedback on a PR, ideally also early in the process, so I can potentially give some direction.

@vabene1111
Copy link
Author

Ok, perfect, thank you! I will look into making this an extension, having it natively supported would be awesome. I will let you know once I have anything to show for

@vabene1111
Copy link
Author

Ok so I currently have this very simple partial solution which produces non required ID fields

class DRFWritableNestedExtension(OpenApiSerializerFieldExtension):
    target_class = 'rest_framework.fields.IntegerField'
    
    def map_serializer_field(self, auto_schema, direction):
        if self.target.field_name == 'id':
            return {'type': 'integer', 'required': False}

What I am unsure if I can use a FieldExtension to override readOnly because if I set on the type ({'type': 'integer', 'required': False, 'readOnly': False}) the schema still shows readOnly: true. Is the FieldExtension correct for that or do I need to override on the serializer level?

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

2 participants