[RFC] Deprecate Blueprint.doc decorator

[lafrech]

The Blueprint.doc decorator is meant to add documentation that can't be inferred from the code or added by dedicated decorators. It allows the user to pass manual documentation that will be deep-merged with the automatically generated documentation. Unfortunately, it is not the silver bullet one would expect. It suffers from a few intrinsic issues.

We could try to play smart and improve the way the structure received by doc is used, but I'd rather add dedicated decorators or other explicit means to allow the user to pass documentation in a structured fashion, with a clear and thin abstraction layer.

I'd like to use this issue to list known issues about Blueprint.doc and use cases that should be covered by other means. The goal being to make it useless, and to deprecate it if possible.

Known issues

• It can't really document arguments as deep-merging lists doesn't work so it conflicts with auto-generated documentation.
• When adding extra documentation for a response, the user must use the same type for the status code than the one used in the response decorator (Support for http.HTTPStatus #60 (comment)).
• It doesn't handle the OASv2 / v3 transparently, while the other decorators are pretty good at it, allowing the user to change the OAS version with a configuration parameter using the same codebase. For instance, it doesn't add ['content']['application/json'] when using v3 which makes it more verbose to the user.

Use cases

• Add summary/description: Those can be passed in the view function docstring.
• Add examples: Can now be achieved with response/arguments decorators.
• Document specific API features (e.g. security information). Since this is quite specific, I think it could be done by a custom decorator defined in user code. Typically, when using a decorator to manage accesses to resources (generally from a flask third-party library), this decorator should be wrapped in a custom decorator that adds the information to the docs (see Add authorization to header? #36 (comment) for an example). This means the _apidoc attribute mechanism becomes public API.
• Document / add extra-information to parameters. The only real need was for path parameters. Documentation for path parameters can be passed in Blueprint.route. (See No way to add description to path parameters #23)
• Document / add extra-information to responses. We could add a specific decorator for that. If we assume a view function can only have one normal response and other responses are errors (or error-likes such as redirections or "not modified"), then that decorator could be called "error". (See Error response schema #44)

[dougthor42]

It's likely that most of these issues would be solved by #49, no? Users could simply set docstring_sep=None on the blueprint and then whatever they add to the function docstring will be included in description.

I think that doing so satisfies the "Document specific API features" use case and the "Document parameters / responses" use case.

Granted, I don't know much about the goals here, so it may be completely off base. Do things like specific API features or extra info to responses need to be easily accessible such as being a key in a dict? For example, is this needed?

{'summary': "Summary line.",
 'description': "Long description",
 'api_feature': "Some API feature",
}

or can it just be in the description?

{'summary': "Summary line.",
 'description': "Long description\n\nSome info about an API feature",
}

[lafrech]

@dougthor42, nope. flask-rest-api and apispec mean to actually use the features defined in the OpenAPI spec. Cramming all information in the "description" field in an unstructured way defeats the point of the spec. For instance, it does not allow tools to display the features properly.

Besides, you don't need to use sep=None if you don't use the default --- separator in the docstring. (BTW, thanks for your work on the PR. I'll try to get back to you soon with answers to you questions.)

[aiham]

We are using the doc decorator to specify an operationId which is consumed by openapi-generator for generating client method names, unique to each path not just the route:

@blp.doc(operationId="getSomeData")
def get(self):
    return someData

@blp.doc(operationId="setSomeData")
def post(self, newData):
    someData.update(newData)

will generate a client method like:

const someData = await myApi.getSomeData();
await myApi.setSomeData(otherData);

[lafrech]

@aiham This looks like a legit use case.

I didn't know about that OAS feature. We could try to add a feature to provide default unique IDs based on blueprint/route/endpoint/method if that is deemed useful.

But anyway, users may want to force their own operationId. Here are the alternatives I can think of:

• Parse operationId from docstring. I don't like that because you can't create it programatically.
• Take an operationId base string in route and add lowercase method at the end on each method if route is called on a MethodView. I don't like that because it forces the end of the string. And I'm not sure it is easy to achieve.
• Create a Blueprint.operation_id decorator. It may look a bit too much for such an anecdotal feature. But it wouldn't be a performance issue since the decorator would just complete the docs and not wrap the view function.

[lafrech]

Obviously, another option is to keep the doc decorator as it is and document what it should not be used for. It sounds a bit cheap and dirty, but it could be the most pragmatic and future-proof choice.

[aiham]

Auto generating operationId probably goes against the original purpose of the field which was meant for customising the method name that openapi-generator is already auto generating. But I wouldn't be against adding an auto generated operationId if the user explicitly requested one, I'm sure it would be useful outside the scope of openapi-generator.

I think keeping Blueprint.doc is probably the way to go, because injecting custom fields is actually a feature of OpenAPI: Specification Extensions. Extensions are especially useful for new spec-consuming tools that don't exist yet or if someone wants to add some app-specific metadata to an operation. An example is the official swagger-node library which relies on the x-swagger-router-controller field.

We are also using Blueprint.doc for the operation-specific security rules (unless you can suggest a better way to do this?):

@blp.doc(security=[{"mySecurityScheme":[]}])

I think these are examples that you can't necessarily guarantee the whole spec has been implemented by this library, but there's also the case of new fields being added to the spec in the future that you can't predict. It would be better to let library users extend it rather than restrict them. If that is via a differently named decorator I'd be happy with that too.

Also one comment on the known issue:

It can't really document arguments as deep-merging lists doesn't work so it conflicts with auto-generated documentation.

Maybe whatever is specified with Blueprint.doc (or a different decorator) should take precedence over anything auto generated. This allows library users to purposefully override something that they disagree with. For example, the official swagger-core Java annotations library explicitly allows you to override auto generated documentation: https://github.com/swagger-api/swagger-core/wiki/Annotations-1.5.X#apiparam

While swagger-core scans these annotations by default, the @ApiParam can be used to add more details on the parameters or change the values as they are read from the code.

These are just my thoughts, hopefully you get some value from them. Thanks for flask-rest-api.

[lafrech]

Thanks for the valuable feedback, really.

Yeah, I guess trying to cover every feature explicitly in this lib is a chimera. Ultimately, we're probably gonna keep doc. In its current form, it is deep merged with auto doc, and as it comes as the second argument of the merge, it takes precedence.

Another limitation of doc is that due to its lack of explicit structure, it can only be injected as is, therefore it does not abstract OAS version like the rest of the lib does. In the other decorators, the doc is generated correctly once the OAS version is specified.

The issues with arguments is that they are a list. You can deep merge dicts but not lists. So introducing an argument in there erases all auto documented arguments rather than adding to them. I get your point with taking precedence, but erasing all auto documented arguments (i.e. having to recreate them manually) just to add or complete a single argument is really a pity. We could try to fix this specific issue by adding a specific logic for this case, but that would be just the beginning of a whole series of alike troubles. My stance on this is to not use it to tweak response/arguments. Rather than trying to prevent this, we could just document it and let the user know what he is doing. I'm still not comfortable with this.

Regarding the security schema, my advice would be to modify the decorator you're using to enforce it so that it also documents what it does. => DRYer resources.

I agree about operationId. If someone wants to do it, I don't think I'll object to an auto-generation feature, but it should be overridable.

[aiham]

Regarding the security schema, my advice would be to modify the decorator you're using to enforce it so that it also documents what it does. => DRYer resources.

Yes, the decorator which enforces access control is the one that calls blp.doc(security=[{"mySecurityScheme":[]}]) automatically. I'm just confirming this is the only way to add security to the operation's schema.

[lafrech]

Ah, OK. We do the same but instead of calling doc, we manipulate the _apispec attribute directly. As I wrote above, making this the recommended way would require this attribute to be public API. Or proxied. Which is more or less what doc does...

[mjpieters]

Idea: dynamically create decorators for OpenAPI documentation keys.

• Only support keys valid for an operation, where those are not already supported by other, existing methods (exiting decorators or other sources of OpenAPI information).
• Support arbitrary keys as long as they start with x_.
• Expects a single argument to store as the value for the named key, for objects or arrays you can use a dictionary or list.

For example, to set an operation ID, mark the operation as deprecated, and include code samples, you'd use:

@blp.route('/')
class Pets(MethodView):
    @blp.arguments(TagsSchema, location='query')
    @blp.response(200, PetSchema(many=True))
    @blp.operation.operation_id("findPetsByTags")
    @blp.operation.deprecated(True)
    @blp.operation.x_code_samples([dict(lang="Python", source="import requests\n\n...")])
    def get(self, tags):
        """Finds Pets by tags

        Multiple tags can be provided with comma separated strings. Use tag1,
        tag2, tag3 for testing.

       """

The name operation documents what type of OpenAPI object this applies to. To segregate this by OpenAPI version, include an optional oas2, oas30 or oas31 name in the expression:

    @blp.oas2.consumes(...)  # only applies to OpenAPI v2
    @blp.oas30.oas31.callbacks(...)   # only applies to OpenAPI v3.0 and v3.1

For operation keys that have existing methods, you could throw an exception, or raise a warning, and in the exception or warning message include information about better ways to set the specific key (e.g. Instead of using @blp.operation.summary, give your function a docstring. The first line automatically becomes the operation summary. See [documentation link]). For invalid key-names you can redirect the developer to the x- vendor prefix.

These attributes just produce a generic decorator function with the target OpenAPI key and version information as a closure.

Yes, this is just the same thing as the @blp.help() decorator, with the keyword arguments turned into attributes. The advantage is individual decorators can be re-used across a project, mixed and matched with other such decorators, and would visually better document what they are for. Object traversal via attributes also gives you a richer language to include specific OAS versions. The additional ideas, to automatically reject or at least sternly warn about non-standard keys will help avoid producing invalid OpenAPI output and re-educate the developer, but those could be implemented using the current doc() decorator.

The decorator is not intended to support merging.

[lafrech]

Sorry for the delay.

Thanks to user feedback and own experience, my mind is moving towards keeping this decorator in a form or another.

As I wrote above, for the security decorator case, the best thing to do IMHO is to override the decorator to let it auto-document (currently by writing in _apidoc attribute, hopefully someday with a better API). This might apply to many other use cases, but ultimately, there will always be cases needing manual doc.

My main concern is how to merge with autodoc, especially for the parameters case (because it is a list).

I haven't been giving it much thought since then and I don't expect a solution to come out of the blue.

@mjpieters the feature you describe above sounds neat. I don't see myself implementing this any soon. High work / benefit ratio. And the whole thing is not in my priorities. But thanks for sharing a clear specification to build upon.

Meanwhile, this issue is 2 year old and the status quo still stands.

---

BTW, this makes me think of another design issue.

Theoretically, view functions can be decorated by calling the decorator as a function rather than with the decorator syntax, and one may even decorate the same view func with two different unnested decorators.

def view_func(self):
    return stuff

# View func decorated with decorator 1
blp.route('/path_1')(decorator_1(view_func))

# View func decorated with decorator 2
blp.route('/path_2')(decorator_2(view_func))

Because of this, I always deepcopy the doc structure to avoid two resources docs being mixed up. To do so, even if the decorator doesn't affect the function, only the doc, I still need to wrap the function in another function.

flask-smorest/flask_smorest/blueprint.py

Lines 220 to 232 in 16f7677

	def decorator(func):
	@wraps(func)
	def wrapper(*f_args, **f_kwargs):
	return func(*f_args, **f_kwargs)
	# The deepcopy avoids modifying the wrapped function doc
	wrapper._apidoc = deepcopy(getattr(wrapper, '_apidoc', {}))
	wrapper._apidoc['manual_doc'] = deepupdate(
	deepcopy(wrapper._apidoc.get('manual_doc', {})), kwargs)
	return wrapper
	return decorator

The deepcopy in itself is not an issue, but the wrapping adds to the call stack. I don't think it matters as long as the number of nested decorators is limited, but it matters more when decorators pile up. In any case, the impact on performance is probably negligible. I always thought it was a pity to waste this just for the corner case described above, but I couldn't come up with a better solution.

[mjpieters]

I always thought it was a pity to waste this just for the corner case described above, but I couldn't come up with a better solution.

Perhaps you could substitute the wrapper with functools.partialmethod()? That would avoid the callstack entry:

from functools import partialmethod, wraps


 def decorator(func): 
  
     wrapper = wraps(func)(partialmethod(func))
  
     # The deepcopy avoids modifying the wrapped function doc 
     wrapper._apidoc = deepcopy(getattr(wrapper, '_apidoc', {})) 
     wrapper._apidoc['manual_doc'] = deepupdate( 
         deepcopy(wrapper._apidoc.get('manual_doc', {})), kwargs) 
     return wrapper 

Use partialmethod, not partial, so the decorated result can still be bound to instances like a function can.

[mjpieters]

Never mind; a partialmethod can't itself easily be decorated (it can't be called directly, it must be bound).

[lafrech]

Perhaps this SO post or this one would help. Keeping this for later.