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

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

Open
juur opened this issue Mar 25, 2024 · 5 comments
Labels

Comments

@juur
Copy link
Contributor

juur commented Mar 25, 2024

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
Copy link
Contributor Author

juur commented Mar 25, 2024

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

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

the validate method ignores missing fields!

@juur juur closed this as completed Mar 25, 2024
@juur juur reopened this Mar 26, 2024
@juur
Copy link
Contributor Author

juur commented Mar 26, 2024

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.

@lafrech
Copy link
Member

lafrech commented Apr 29, 2024

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)

@juur
Copy link
Contributor Author

juur commented Apr 29, 2024

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 ??

@lafrech
Copy link
Member

lafrech commented Apr 29, 2024

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.

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

No branches or pull requests

2 participants