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

Add plugin system for Blueprints and Api #436

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Aesonus
Copy link

@Aesonus Aesonus commented Dec 31, 2022

Introduces a plugin system to add functionality using composition instead of inheritance. This requires the plugin to be registered on the blueprint. See test_plugin.py for more details.

Introduces a plugin system to add functionality using composition
instead of inheritance. This requires the plugin to be registered on the
blueprint. See test_plugin.py for more details.
@Aesonus
Copy link
Author

Aesonus commented Dec 31, 2022

Hello,
I found myself reading through the discussion over in issue #71 and thought I might take a crack at making a simpler way for users to add their own decorators and apidoc without having to rely on Blueprint.doc. This PR aims to get the ball rolling in that direction by moving away from adding more mixin types on Blueprint and moving towards a compositional pattern. Granted, this PR does still rely on public access to func._apidoc on decorator callable to add API spec, but that aside, this would be a start.

I also cast a vote for calling plugins for this purpose "Smore Plugins".

@codecov
Copy link

codecov bot commented Jan 23, 2023

Codecov Report

Base: 99.87% // Head: 99.64% // Decreases project coverage by -0.24% ⚠️

Coverage data is based on head (0080f52) compared to base (ab5f6c5).
Patch coverage: 93.10% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #436      +/-   ##
==========================================
- Coverage   99.87%   99.64%   -0.24%     
==========================================
  Files          13       15       +2     
  Lines         823      852      +29     
  Branches      180      186       +6     
==========================================
+ Hits          822      849      +27     
- Misses          0        1       +1     
- Partials        1        2       +1     
Impacted Files Coverage Δ
flask_smorest/plugin/abc.py 85.71% <85.71%> (ø)
flask_smorest/plugin/built_in.py 94.73% <94.73%> (ø)
flask_smorest/__init__.py 100.00% <100.00%> (ø)
flask_smorest/blueprint.py 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@lafrech
Copy link
Member

lafrech commented Jan 23, 2023

Thanks for the contribution and sorry for not answering any sooner.

Please allow me a little time to give this the attention it deserves. (Don't worry about test coverage at this stage.)

@Aesonus
Copy link
Author

Aesonus commented Jan 24, 2023

Not a problem at all. Thanks.

@lafrech
Copy link
Member

lafrech commented Jan 29, 2023

I think I get the point. But it's not clear to me what it allows that can't be done without it. Admittedly, documenting a security decorator is something users have been wondering how to do and I have never really taken the time to lay this down. But I'm not sure this plugin interface is required and how much it helps.

You may be interested in what I do in bemserver-api. This is the way I recommend over using blp.doc.

In extensions package, there's a smorest module, in which I subclass Api to document the security scheme and I subclass Blueprint to add the login_required method and add a doc callback. I agree this is not really obvious, it deserves a better interface and a plugin interface such as this one seems to be a step in the right direction. But it still exposes non-obvious stuff. For instance, register_method_docs's signature may require users to understand the internals to an extend that they could do what I'm doing in bemserver-api without the plugin interface.

I realize this answer might not be satisfying but unless we manage to produce a very simple interface (is this even feasible?) I'm tempted to cowardly leave things as is (and point advanced users to real-life examples).

I understand your point about compositing vs. subclassing. But there are so many advanced features that need subclassing that I always recommend to subclass when starting a new app, just to make it easier on the long run. And I find it nicer in the resource pages to have all features as blp methods (totally opinionated, I admit).

This is not a definitive no. Just thinking out loud that this might not be needed (and I'd rather not add uneeded API surface).

@Aesonus
Copy link
Author

Aesonus commented Feb 2, 2023

Thank you for taking the time to look this over and give your thoughts.

I think I get the point. But it's not clear to me what it allows that can't be done without it.

In all honesty, nothing at this point. I did not really intend for this to add functionality but rather to reduce the amount of knowledge a user may need about Blueprint to extend functionality using subclassing. While I was writing this PR, I had to take a good look at the source code to figure out how all the docs were generated, leading me to the Blueprint._prepare_doc_cbks attribute. This would require users wanting to extend the functionality knowing about the internal workings of Blueprint and Api. That brings me to your other point.

In extensions package, there's a smorest module, in which I subclass Api to document the security scheme and I subclass Blueprint to add the login_required method and add a doc callback. I agree this is not really obvious, it deserves a better interface and a plugin interface such as this one seems to be a step in the right direction. But it still exposes non-obvious stuff. For instance, register_method_docs's signature may require users to understand the internals to an extend that they could do what I'm doing in bemserver-api without the plugin interface.

I can agree somewhat, however, what you do in bemserver-api would require the user to understand that Blueprint relies on Blueprint._prepare_doc_cbks to add the additional api from _apidoc to the spec. They would need to know to append the _prepare_auth_doc method to Blueprint._prepare_doc_cbks and also to extend the Api class as well to document the security schema. What I have submitted here eliminates those extra steps. While I agree that this plugin system does the same thing bemserver-api does, I am of the opinion that what I have done reduces the number of steps to add additional functionality, and is particularly advantageous when adding more than one extension.

For instance, if I was wanting to have security schema and also give each endpoint an operationId using decorators, I would end up adding more methods to the same extended Blueprint class, violating the single responsibility principle. By separating each additional extension to its own class, it avoids that problem. That being said, it does take the decorators out of the Blueprint class. That could be an easy fix using __getattr__ in the Blueprint class to proxy to the plugin decorator method, and registering plugins on a class attribute rather than an instance attribute, such that every Blueprint instance can use the registered plugins. I may revise this pull request to reflect that.

I realize this answer might not be satisfying but unless we manage to produce a very simple interface (is this even feasible?) I'm tempted to cowardly leave things as is (and point advanced users to real-life examples).

I think it is feasible, though it is not clear to me how at this point. I'll have to get back to you on that.

In any case, I appreciate your response. It certainly reveals quite a bit about the paradigms used. I will see what else I can come up with. I really enjoy this project (I use it a lot), so I am happy to help make it better.

@lafrech
Copy link
Member

lafrech commented Feb 2, 2023

Thank you for the nice feedback. And for the constructive help. This is definitely not a no go and I'd be happy if we could go further and actually provide a nice interface for the reasons you explained above.

I don't have enough time to put into this but you seem to have a good vision about it now and I'll try to be as reactive as can be.

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

Successfully merging this pull request may close these issues.

2 participants