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 for path-arguments duplicates input #220

Open
der-joel opened this issue Jan 29, 2021 · 7 comments
Open

Schema for path-arguments duplicates input #220

der-joel opened this issue Jan 29, 2021 · 7 comments

Comments

@der-joel
Copy link

der-joel commented Jan 29, 2021

Using a schema to validate/process the path arguments seems to pass the argument twice.
The data-dict of the marshmallow schema is passed as an argument and the value is passed as keyword-argument (probably by vanilla flask).

Note: This can be prevented by passing as_kwargs = True to the decorator

Code-Example:

from flask.views import MethodView
from marshmallow import Schema
from marshmallow.fields import String
from flask_smorest import Blueprint

bp = Blueprint("pets", "pets", description="pet related operations", url_prefix="/api/pets")

class PetIdSchema(Schema):
    pet_id = String(required=True)

@bp.route("/<string:pet_id>")
class SinglePet(MethodView):
    """route for operations on a single pet"""

    @bp.arguments(PetIdSchema(), location="path")
    def get(self, *args, **kwargs):
        """get information on this pet"""
        print(f"args: {args} | kwargs: {kwargs}")
        # prints: args: ({'pet_id': '600aed28e65f74c3a97f6f58'},) | kwargs: {'pet_id': '600aed28e65f74c3a97f6f58'}
        pet_id = args[0]
        return Pet.get_by_id(pet_id)
@lafrech
Copy link
Member

lafrech commented Jan 29, 2021

Use of Schema for path parameters is poorly documented.

See this conversation: #219.

Better doc would certainly not hurt.

Thinking out loud, perhaps we could catch the parameter injected by Flask from @arguments and solely rely on the schema. Might be worth investigating, as it could improve user experience.

@sirosen
Copy link
Contributor

sirosen commented Feb 14, 2021

Thinking out loud, perhaps we could catch the parameter injected by Flask from @arguments and solely rely on the schema. Might be worth investigating, as it could improve user experience.

I'm thinking about how this could be done, either in flask-smorest or in webargs.

We could check request.view_args and filter any matching keys out of keyword args when location="path", use_kwargs=False. No path parameters would be passed through multiple stacked Blueprint.arguments decorators in that case.

Another option is for Blueprint.arguments to implicitly pass use_kwargs=True if location="path" and no use_kwargs parameter is explicitly passed.

I'm not sure if these are good ideas vs. docs and maybe a warning about path params specifically. Fancy special-case behaviors might be harder to understand or document.

@hellbe
Copy link

hellbe commented Jun 5, 2023

Ran into another issue relating to this. In the generated OpenAPI specification the path parameter is documented on the path object but it seems like the OpenAPI standard expects it to be documented on the method(s): https://swagger.io/docs/specification/describing-parameters/#path-parameters

I can create the desired output for the method by adding an argument schema with location="path" but a validator still complains about the unexpected parameters for the path. Any advice on how to get rid of that output or, even better, change so the path arguments output is added to the method(s) instead?

@lafrech
Copy link
Member

lafrech commented Jun 5, 2023

@hellbe see the "common parameters" part of that page:

Common Parameters for All Methods of a Path
Parameters shared by all operations of a path can be defined on the path level instead of the operation level. Path-level parameters are inherited by all operations of that path. A typical use case are the GET/PUT/PATCH/DELETE operations that manipulate a resource accessed via a path parameter.

@hellbe
Copy link

hellbe commented Jun 5, 2023

@lafrech yes I am well aware and not suggesting to change that, but the fact that OpenAPI/Swagger spec still expects those parameters to be described for each method. I.e. the definition should not change, the generated documentation should.

@lafrech
Copy link
Member

lafrech commented Jun 5, 2023

My understanding is that parameters shared by all operations of a path can be defined on the path level instead of the operation level so our implementation conforms to the spec, doesn't it?

Are you saying that swagger-ui or another tool complains about it?

@hellbe
Copy link

hellbe commented Jun 5, 2023

@lafrech you are correct - looked more closely and also confirmed here: https://swagger.io/specification/#path-item-object

For some reason the validator I was using is complaining so I assumed it was out of spec. Sorry about the confusion!

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