marshmallow-code / flask-smorest

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

renaming args in api spec #120

Open mcclurem opened 4 years ago

mcclurem commented 4 years ago

This is either a feature request or a failure on my part to find documentation I'm making use of converters reasonably heavily to create endpoints with my model already extracted from my database.

class PetConverter(BaseConverter):
    def __init__(self, url_map):
        super().__init__(url_map)
        self.regex = r'[0-9]+'

    def to_python(self, value):
        return Pet.query.filter_by(id=value).first_or_404()

    def to_url(self, value):
        return str(value.id)

app.url_map.converters['pet_type'] = PetConverter
bp.register_converter(PetConverter, 'integer')

@bp.route('/pet/<pet_type:pet>')
class pet(MethodView):

   @bp.response(PetSchema, description="get pet by id")
    def get(self, pet):
        return pet

this generates api documentation that shows a url of the style get /pet/{pet}

Ideally I'd like the api documentation to show as get /pet/{pet_id}

If I change the route decorator to use /pet/<pet_type:pet_id> I'm then forced to use the variable name pet_id in my get method but it's actually a Pet object.

Perhaps the blueprint register converter could include a "reverse-formatter" argument or or callable something like:

bp.register_converter(PetConverter, 'integer', varname_mangler=lambda x: f'{x}_id')

or more similar to flask_marshmallow's url_for:

bp.register_converter(PetConverter, 'integer', varname_formatter='<var>_id')

where my assumption would be that <var> is a library defined constant stand in for the original variable name in the route so the library code is literally just:

actually_reported_varname = varname_formatter.replace('<var>', original_varname)

the callable is probably a nicer solution because it doesn't require the library constant so it's /slightly/ more flexible but either option would be a nice improvement

lafrech commented 4 years ago

I see. Your use case is legit. In fact, it looks pretty neat.

Since it is not a strictly blocking issue, I'd like to find a way to support this that would be very simple in flask-smorest. I don't think it is worth adding complicated logic.

I prefer the more flexible option using a callable. After all, this is called only once in app init code, it doesn't have to be as eye candy as if it was called on many view functions.

mcclurem commented 4 years ago

@lafrech I know you're busy but you know your library better than me. If you can point me at your best quick guess of where this might want to be implemented in smorest I'd be happy to take a stab at it

lafrech commented 3 years ago

I didn't get the time to look into this, but I think the changes in #182 allow to do this in user code.

Not saying I object to having the feature in the core.

lafrech commented 3 years ago

I didn't get the time to look into this, but I think the changes in #182 allow to do this in user code.

No it doesn't.

It shouldn't be too hard to achieve in the code.

    def register_converter(self, converter, func, name_func=None):
        """Register custom path parameter converter

        :param BaseConverter converter: Converter.
            Subclass of werkzeug's BaseConverter
        :param callable func: Function returning a parameter schema from
            a converter intance
        """
        self.converter_mapping[converter] = func
        # Register converter -> name function
        self.converter_name_mapping[converter] = name_func

    def rule_to_params(self, rule):
        """Get parameters from flask Rule"""
        params = []
        for argument in [a for a in rule.arguments if a not in rule.defaults]:
            converter = rule._converters[argument]
            param = {
                'in': 'path',
                # Use register -> name function if any
                'name': self.converter_name_mapping.get(converter, lambda x: x)(argument),
                'required': True,
            }
            # Inspired from apispec
            for converter_class in type(converter).__mro__:
                if converter_class in self.converter_mapping:
                    func = self.converter_mapping[converter_class]
                    break
            schema = func(converter)
            if self.openapi_version.major < 3:
                param.update(schema)
            else:
                param['schema'] = schema
            params.append(param)
        return params

The variables/attributes should be renamed for clarity. And the mappings should be private.

Should it be a function or just a string? PetConverter would always return pet_id, so a string would be enough for this case. Could there be a case where a function is needed?

der-joel commented 3 years ago

I ran into the same issue. A function is needed when the same converter is used for multiple schemas. E.g. an ObjectIdConverter taking an uuid and converting it to a Pet- or a Person-object depending on a parameter passed in ObjectIdConverter.__init__. In this case the documented name would also depend on this parameter.

der-joel commented 3 years ago

The suggested solution only changes the argument doc. The url seems to be unchanged (should be "{id}" instead of "{pet}"):

issue120 bug

I've come up with a solution for this using string replacement (which is quite error prone). Still works for most use cases i guess:

    def register_converter(self, converter, func, name_func=None):
        """Register custom path parameter converter

        :param BaseConverter converter: Converter.
            Subclass of werkzeug's BaseConverter
        :param callable func: Function returning a parameter schema from
            a converter intance
        """
        self.converter_mapping[converter] = func
        # Register converter -> name function
        self.converter_name_mapping[converter] = name_func

    def rule_to_params(self, rule):
        """Get parameters from flask Rule"""
        params = []
        for argument in [a for a in rule.arguments if a not in rule.defaults]:
            converter = rule._converters[argument]
            arg_name = self.converter_name_mapping.get(converter, lambda x: x)(argument)
            param = {
                'in': 'path',
                # Use register -> name function if any
                'name': arg_name,
                'required': True,
            }
            # url-rule is replaced here
            rule.rule = rule.rule.replace(f":{argument}", f":{arg_name}")
            # Inspired from apispec
            for converter_class in type(converter).__mro__:
                if converter_class in self.converter_mapping:
                    func = self.converter_mapping[converter_class]
                    break
            schema = func(converter)
            if self.openapi_version.major < 3:
                param.update(schema)
            else:
                param['schema'] = schema
            params.append(param)
        return params