marshmallow-code / flask-smorest

DB agnostic framework to build auto-documented REST APIs with Flask and marshmallow
https://flask-smorest.readthedocs.io
MIT License
651 stars 72 forks source link

Inconsistent order of parameters when using rule.arguments from werkzeug inside FlaskPlugin #541

Closed ddorian closed 1 year ago

ddorian commented 1 year ago

I'm using this with flask-smorest. In https://flask-smorest.readthedocs.io/en/latest/openapi.html#enforce-order-in-openapi-specification-file, you say to use "ordered = True" so they're ordered and I can do snapshot-testing on the openapi json output.

But in this line:https://github.com/marshmallow-code/apispec/blob/dev/src/apispec/ext/marshmallow/common.py#L54, schema._declared_fields is not an ordered dict, but a simple dict. The schema.fields is ordered though.

Do you have any idea how to fix this even with classes instead of just objects?

See https://github.com/marshmallow-code/flask-smorest/issues/541 for correct issue

Regards, Dorian

lafrech commented 1 year ago

Hi. I don't understand. Simple dicts are ordered. In fact, since the latest marshmallow version, ordered shouldn't even be needed.

Do you think you could investigate and provide a reproducible case we can look at?

(Note to self: bump marshmallow dependency version and remove note about ordering from the docs.)

ddorian commented 1 year ago

Yeah, looks like that code is not the issue.

See the code below. And execute it a couple of times. And you will see that the order of parameters (not inside the get, those outside the get, at the end of the json), will randomly shuffle:

import json

import marshmallow as ma
from flask import Flask
from flask.views import MethodView
from flask_smorest import Api, Blueprint
from marshmallow import fields

app = Flask(__name__)
app.config["API_TITLE"] = "My API"
app.config["API_VERSION"] = "v1"
app.config["OPENAPI_VERSION"] = "3.0.2"
api = Api(app)
api.ERROR_SCHEMA = None

blp = Blueprint("pets", "pets", url_prefix="/pets", description="Operations on pets")

class BaseSchema(ma.Schema):
    class Meta:
        ordered = True

class ProjectPathArgs(BaseSchema):
    project_id = fields.Integer(
        data_key="project_id",
        required=True,
        metadata={"description": "ID of the project to retrieve."},
    )

class UploadIdPathArgs(ProjectPathArgs):
    upload_id = fields.String(data_key="upload_id", required=True)

@blp.route("/project/<project_id>/upload/<upload_id>/complete")
class Pets(MethodView):
    @blp.arguments(UploadIdPathArgs, location="path")
    @blp.response(204)
    def get(self, args):
        """List pets"""
        return {}

api.register_blueprint(blp)

# print(api.spec.to_dict())
with app.app_context():
    # print(api._openapi_json().json)
    print(json.dumps(api.spec.to_dict()["paths"], indent=2))

exit()
ddorian commented 1 year ago

So sometimes it's one.txt and sometimes it's two.txt

lafrech commented 1 year ago

The order should be deterministic. I've been working in apispec and flask-smorest to ensure that.

Marking it as bug. Further investigation should tell whether it is here in apispec or in flask-smorest.

Thanks for reporting.

ddorian commented 1 year ago

I think the problem is in https://github.com/marshmallow-code/flask-smorest/blob/master/flask_smorest/spec/plugins.py#L113

The rule.arguments is a set and has random ordering.

lafrech commented 1 year ago

But this is only about path arguments from the flask rule, it shouldn't be related to marshmallow Schema instance vs. class.

I'm afraid we can't do much about this since it is in flask, unless we force an arbitrary order, e.g. alphabetical.

ddorian commented 1 year ago

But this is only about path arguments from the flask rule,

Yes, and only this is wrong.

it shouldn't be related to marshmallow Schema instance vs. class.

It's not. My original hunch was wrong. The schema is always consistent (since dicts are ordered on Python 3.7+).

I'm afraid we can't do much about this since it is in flask, unless we force an arbitrary order, e.g. alphabetical.

We can use rule._trace, replacing the line in https://github.com/marshmallow-code/flask-smorest/blob/master/flask_smorest/spec/plugins.py#L113C79-L113C79 with:

        for argument in [a for a0, a in rule._trace if a not in rule.defaults and a0 is True]:

Looks like a0=True above is only done on a valid argument https://github.com/pallets/werkzeug/blob/47c6bd568efd0870d413c27be316c2b4eee74486/src/werkzeug/routing/rules.py#L624

ddorian commented 1 year ago

The change to use rule._trace worked fine for my 100KB openapi.json file for the whole project.


Since I always use a schema for the path arguments, in this case I'm duplicating the arguments (once from the schema, once from the FlaskPlugin that it does automatically).

So in my case I can just disable FlaskPlugin and be done, doing this:

class MyFlaskPlugin(FlaskPlugin):
    def rule_to_params(self, *args, **kwargs):
        return []

api = Api(app, spec_kwargs=dict(flask_plugin=MyFlaskPlugin()))
ddorian commented 1 year ago

I think another alternative would be to add a schema argument to the Blueprint.route() method https://github.com/marshmallow-code/flask-smorest/blob/ea52d1d35c8f081ca6903b083e307eef6c8ebece/flask_smorest/blueprint.py#L108 so that would be used before doing the automatic parameters from rule.arguments.

And I think you can transfer this issue to flask_smorest.

lafrech commented 1 year ago

So our options are

I think we can live with the underscore attribute. We could even ask werkzeug for an API (public method) to get arguments in order.

Would you like to propose a PR?