flask-smorest icon indicating copy to clipboard operation
flask-smorest copied to clipboard

Schema validation of mandatory fields on a blueprint with a HTTP PATCH method containing only modified fields

Open juur opened this issue 10 months ago • 5 comments

I have a simple CRUD class (based on flask.views.MethodView). The Schema class is based on marshmallow_sqlalchemy.SQLAlchemyAutoSchema.

 @_bp.arguments(AccountGroupModelSchema)
 @_bp.response(200, AccountGroupModelSchema)
 def patch(self, updated, account_group_id):
     """Update an existing object."""
     item = AccountGroup.query.get_or_404(account_group_id)

     item.name = updated.name or item.name

     db.session.commit()
     return item

When submitting a HTTP PATCH, the JSON payload contains only those fields being modified, e.g. { "name": "new_name" }. This means the Schema validation fails, because mandatory fields are not present.

Is there anyway to handle validating the fields that are present AND rejecting any fields that are unknown but NOT failing validation for missing (as not being modified) fields in a PATCH? Or do I just have to take the JSON directly, update the entity, then validate prior to updated in the ORM?

juur avatar Mar 25 '24 11:03 juur

I figured it out, if i replace the decorator with this:

@_bp.arguments(AccountGroupModelSchema(partial=True))

the validate method ignores missing fields!

juur avatar Mar 25 '24 13:03 juur

Whilst partial=True does permit PATCH to work with validation, it creates another problem within apispec causing errors like this to appear:

apispec/ext/marshmallow/openapi.py:135: UserWarning: Multiple schemas resolved to the name Account. The name has been modified. Either manually add each of the schemas with a different name or provide a custom schema_name_resolver.

I assume this is because apispec considers the partial Schema to be different to the non-partial one. Therefore I'm back in the situation where just sending a single field in a PATCH doesn't work perfectly.

juur avatar Mar 26 '24 10:03 juur

It is fine. Just craft a custom name resolver to provide a different name for partial schemas.

def resolver(schema):
    # This is the default name resolver from apispec
    schema_cls = resolve_schema_cls(schema)
    name = schema_cls.__name__
    if name.endswith("Schema"):
        name = name[:-6] or name
    # Except it appends Partial to partial schemas.
    if isinstance(schema, ma.Schema) and schema.partial:
        name = f"{name}Partial"
    return name


class Api(flask_smorest.Api):
    """Api class"""

    def __init__(self, app=None, *, spec_kwargs=None):
        spec_kwargs = spec_kwargs or {}
        spec_kwargs["marshmallow_plugin"] = MarshmallowPlugin(
            schema_name_resolver=resolver
        )
        super().__init__(app=app, spec_kwargs=spec_kwargs)

lafrech avatar Apr 29 '24 20:04 lafrech

Shouldn't flask-smorest "just work" for the (very common) RESTful use of PATCH whereby only modified attributes are submitted?

I don't really understand the above code enough to know quite how it works, but would (in my example above) I also need a AccountGroupModelSchemaPartial schema, thus doubling the number of schemas I have ??

juur avatar Apr 29 '24 21:04 juur

I didn't test that code but I think it should work with the partial=True change you proposed above. It will create two schemas in the doc but you only use one in your code (with or without partial).

I'm not sure patch is so commonly used. I've been investigating about it a while back and doing it this way doesn't allow to remove a field, for instance, hence the use of complicated things like json-patch. In my APIs, I don't provide PATCH, just PUT. The client GETs an item, modifies it, then PUTs. But there's no point arguing about whether or not it is common. I'd be happy if it could be supported out-of-the-box even if it uncommon, except in this case it seems kind of complicated to do and requires assumptions about how the PATCH is performed that I don't want to make.

I think the solution proposed above is the way to go. Admittedly, it could be documented better.

lafrech avatar Apr 29 '24 21:04 lafrech