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

RFC: Use classes for plugins #207

Closed
lafrech opened this issue May 8, 2018 · 20 comments

Comments

@lafrech
Copy link
Member

commented May 8, 2018

I think things would be simpler if plugins were classes.

I read in the comments that the plugin system is copied on Sphinx. I don't know why Sphinx does it like this, but I don't see the benefit in apispec.

Since plugins are only referred to by their path as string, there is no direct access to them. So their configuration can be a bit awkward:

1/ Mutating a global variable in the plugin file.

2/ Calling a function in the plugin file that will have to store either in a global in the file (which is equivalent to case 1 above) or somewhere else (like map_to_swagger_type stores the mapping in the field's __swagger_field_mapping attribute).

3/ Acting upon the spec object. This is the case for schema_name_resolver, which is used only in marshmallow plugin, so shouldn't be exposed to spec.

Likewise, storage is more complicated as it is done in the spec object. The list of schema definitions is stored in spec.plugins['apispec.ext.marshmallow']['refs'] which is the reason why the spec object is passed to all functions in the plugin.

With plugin classes, the plugin object would keep its storage internal. It wouldn't need globals. It could be configured at instantiation before (or even after, if needed) being passed to the spec object.

A plugin would just have to define such methods:

class MyApispecPlugin(BaseApispecPlugin):
    def definition_helper(...):
        [...]

    def path_helper(...):
        [...]

    def operation_helper(...):
        [...]

Of course, BaseApispecPlugin would provide no-op methods so that child classes would only add needed methods.

Then, no need to store stuff in the spec object. Variables would be plugin class attributes, modifiable by direct access or plugin object method calls (this is plugin implementation detail). No need to expose plugin internal details to spec (schema_name_resolver). No need to add attributes to the field classes (schema -> swagger type mapping can be stored in a plugin object attribute).

Here's what setup would look like:

# Instantiate and configure plugin
ma_plugin = apispec.ext.marshmallow.MarshmallowPlugin(
    schema_name_resolver = my_func
)

# Create an APISpec
spec = APISpec(
    title='Swagger Petstore',
    version='1.0.0',
    plugins=[
        apispec.ext.flask.FlaskPlugin(),  # No specific config to do beforehand
        ma_plugin,  # Configured above
    ],
)

@sloria, is there a reason I'm missing for the current plugin interface?

This refactor would solve part of #206.

@sloria

This comment has been minimized.

Copy link
Member

commented May 8, 2018

I quite like this idea. It would certainly be more ergonomic to to configure plugins at runtime this way.

Honestly, the only reasons I went with Sphinx-style plugins from the start were that Sphinx is a mature library (so users may be accustomed to it) and it was simple to implement. I'm certainly not tied to the approach.

If we went for this, I think I'd prefer if plugins were just an interface and eschew a base class.

class MyApispecPlugin:
    def definition_helper(...):
        [...]

    def path_helper(...):
        [...]

    def operation_helper(...):
        [...]

Is there a use case for passing spec to the plugins? If so, then we can do what Flask does:

from apispec.ext.marshmallow import MarshmallowPlugin

MarshmallowPlugin(spec)

# or

ma = MarshmallowPlugin()
ma.init_spec(spec)
@lafrech

This comment has been minimized.

Copy link
Member Author

commented May 9, 2018

Good.

If there is no base class, that means each plugin must provide the three helpers, even if they just pass, or the core must check their existence before calling them.

Passing spec is a good idea and so is the use of init_spec. In fact, I was wondering how to pass the openapi major version, and passing the whole spec is more future-proof: no need to break the API when a new parameter is needed.

@lafrech

This comment has been minimized.

Copy link
Member Author

commented May 9, 2018

I've been giving it a try. The transition of the plugin into plugin objects is relatively easy.

However, I noticed the register_*_helper interface in the core. I didn't realized before it was not limited to plugins. Like, anyone can register a function in there from his code without a plugin file.

If we move to plugin objects, and we store the spec in the plugin objects, we don't want to pass the spec object when calling the helper methods. This is one of the benefits of the change.

But then this breaks custom non-plugin helpers passed directly through the register_*_helpers interface as they don't receive the spec object anymore. This means that those helpers should be passed as plugin objects as well. This could be a bit of an overhead, but not a very big deal.

And there's the specific case of response_helpers. The register function expects a method and a status code. It is currently not used in plugins provided with apispec, but I suppose you added them because you had a use case for them. The interface could be changed to pass the method and the status code to a plugin object reponse_helper method.

class MyApispecPlugin:

    def response_helper(self, method, status_code, **kwargs):
        """One response helper function with multiple cases"""
@sloria

This comment has been minimized.

Copy link
Member

commented May 9, 2018

However, I noticed the register_*_helper interface in the core. I didn't realized before it was not limited to plugins. Like, anyone can register a function in there from his code without a plugin file.

These were originally intended for use within plugins' setup functions. Class-based plugins obviate them, so perhaps we remove or deprecate them.

And there's the specific case of response_helpers. The register function expects a method and a status code. It is currently not used in plugins provided with apispec, but I suppose you added them because you had a use case for them. The interface could be changed to pass the method and the status code to a plugin object reponse_helper method.

I don't recall if I was targeting a specific use case when I added response_helpers. Perhaps I was planning to add the ability to pass marshmallow schemas as response formats, but I can't be sure. I think passing method and status_code makes sense, though.

@lafrech

This comment has been minimized.

Copy link
Member Author

commented May 9, 2018

Here's what I've done so far: https://github.com/Nobatek/apispec/tree/dev_207_plugin_classes

I'm still uncomfortable with the separation between ext.marshmallow.MarshmallowPlugin and ext.marshmallow.swagger.Swagger Methods from one object call methods from the other. This was already the case before, hence the circular import issues.

A few swagger tests are broken. The whole test_swagger.py test suite should be reworked to pass Swagger or MarshmallowPlugin or APISpec objects using fixtures, to avoid sharing instances across tests. I'd rather have a clearer mind about those objects before modifying all the tests.

I kept the register_*_helper functions. They can be deprecated.

TODO:

  • Sort things in Swagger and MarshmallowPlugin, decide who holds the schema references dict
  • Remove spec object from swagger methods
  • Add openapi major version as Swagger object attribute written at init
  • Rename Swagger to something more meaningful and up-to-date like OpenAPIConverter.
  • Decide which format to use to pass the OpenAPI version to Swagger object (string, distutils.version, tuple,...)
  • Make FIELD_MAPPING an attribute of Swagger and use map_to_swagger_type to modify it rather than write in the schemas.
  • Fix swagger tests
  • Double-check what is class method and what is module function in marshmallow/swagger extensions
  • Decide whether or not to provide a base class for plugin classes
  • Fix get_schema_class/resolve_schema_class redundancy
  • Check tests / add missing tests for new stuff
  • Trigger deprecation warnings when using deprecated interface (register_*_helper functions, plugins by path as string, schema_name_resolver passed to spec)
  • Last but not least, document the changes...
@sloria

This comment has been minimized.

Copy link
Member

commented May 10, 2018

Just gave your branch a glance, and it looks pretty good so far. I'll try to take a closer look over the weekend.

It'd be great if we could deprecate the old plugin API (rather than remove it) in order to give downstream packages a chance to update. Not sure how much complexity that will involve, though. I'll defer to you on that.

@lafrech

This comment has been minimized.

Copy link
Member Author

commented May 10, 2018

I added backward compatibility. Of course, any lib using swagger ext functions directly will break, but it is still possible to add plugins by their path as string, and to pass schema_name_resolver to spec. Deprecation warnings still to be added.

@sloria

This comment has been minimized.

Copy link
Member

commented May 10, 2018

Awesome. Looks like flask-apispec uses the swagger module, so that will need to be updated. Are you just moving those functions to apispec.ext.marshmallow?

Also, do we want to allow passing string paths to the new plugin classes?

spec = APISpec(
    plugins=[
        'apispec.ext.marshmallow.MarshmallowPlugin',    
    ]
)

Could be convenient for the common use case.

@lafrech

This comment has been minimized.

Copy link
Member Author

commented May 10, 2018

Awesome. Looks like flask-apispec uses the swagger module, so that will need to be updated. Are you just moving those functions to apispec.ext.marshmallow?

No. I'm cramming them into a Swagger class (or OpenAPIConverter, or any better name). This class has attributes:

  • list of schema references (or should it be in MarshmallowPlugin?)
  • openapi version

This indeed breaks code that uses functions from swagger.py file, such as flask-apispec or our flask-rest-api but hopefully, it should be an easy fix: just instantiate the class and use it.

Current design is wrong because spec is transmitted all along the chain, unless you don't need the schema references so you don't care about the spec. Except now, we also need the openapi version potentially everywhere. We could pass the version, and the refs, and next, what? Making all those class methods makes it practical for that.

Using those functions directly is wrong because when not passing a spec object, version defaults to OpenAPI 2. And to specify version 3, you need to create a spec with openapi version set to 3, which is ridiculous. At least that's my understanding. I'm not claiming I'm 100% sure about everything, here. Hopefully, it'll be clearer when I'm done.

Overall, it will be breaking, not backward compatible, but easy to fix from client side. And since it is a breaking change, I'd rather be sure it is correct before releasing.

Also, do we want to allow passing string paths to the new plugin classes?

I'd rather not.

  • Little benefit: you don't need to import + instantiate a class. That's a small gain. I don't think it's worth the added complexity (which is small as well, I admit).
  • Currently, I keep backward compatibility by checking whether input is a string (old interface) or an object (new interface). Doing what you suggest makes it harder to discriminate old/new.
  • Two ways to achieve the same thing. Shrug. When I do that, I can almost feel the presence of a mini @deckar01 floating above my shoulder telling me not to do it, and I know he's right.
  • Plugin classes may receive parameters, such as the schema_name_resolver callable passed to MarshmallowPlugin, so this won't work anymore (unless we add another layer of complexity).
@lafrech

This comment has been minimized.

Copy link
Member Author

commented May 10, 2018

With the latest commits, it is now clearer why the use of classes is nice. No spec object being passed all the way in swagger functions and openapi version accessible by each swagger conversion function. The job is not done yet, though (TODO list above updated).

test_swagger.py test functions tests are broken. We need a fixture to create the plugin objects and the spec. I didn't try to fix those tests one by one. I'd rather get the fixture right before modifying all test functions' signature.

@sloria

This comment has been minimized.

Copy link
Member

commented May 10, 2018

Sounds good. I buy your arguments for avoiding the string imports. YAGNI. Or--more accurately--WWdD (What Would @deckar01 Do?).

@lafrech

This comment has been minimized.

Copy link
Member Author

commented May 10, 2018

I fixed the failing tests. There's probably still room for improvement.

Also, totally unrelated, I added a pytest flag (--skip-node) allowing to skip node tests. Sometimes I want to run tests and I don't have node installed, and the two failing tests clutter the terminal output. I can remove this and/or discuss it in another PR. The flag defaults to False and does not affect CI tests. Edit: I removed this from the branch and proposed it in #210.

@lafrech

This comment has been minimized.

Copy link
Member Author

commented May 11, 2018

Pushing branch to this repo to get CI tests. All tests pass except check_api validation. I'll investigate when I get the time.

@lebouquetin, @inkhey, this change impacts schema autoreferencing code. Also, the way to pass the schema name resolver callback would be modified. Feedback welcome.

@lafrech

This comment has been minimized.

Copy link
Member Author

commented May 16, 2018

I think I'm getting close to it.

A few points remain open to discussion:

  • Rename Swagger to something more meaningful and up-to-date like OpenAPIConverter
  • Decide which format to use to pass the OpenAPI version to Swagger object (string, distutils.version, tuple,...)
  • Decide whether or not to provide a base class for plugin classes

And a few things to do:

  • Check tests / add missing tests for new stuff
  • Trigger deprecation warnings when using deprecated interface (register_*_helper functions, plugins by path as string, schema_name_resolver passed to spec)
  • Last but not least, document the changes...

Note that due/thanks to parametrization, some tests are run four times: OpenAPI v2/v3 x deprecated/new interface. I don't think it is an issue. It is fast anyway. And the deprecated interface will be dropped at some point.

@sloria

This comment has been minimized.

Copy link
Member

commented May 27, 2018

Rename Swagger to something more meaningful and up-to-date like OpenAPIConverter

+1. I think OpenAPIConverter is a fine name.

Decide which format to use to pass the OpenAPI version to Swagger object (string, distutils.version, tuple,...)

I think a string would be the most user-friendly, but I suppose that would mean that the version would have to be parsed in many different places, which could be a pain. I delegate to you on that decision.

Decide whether or not to provide a base class for plugin classes

This is a matter of taste. I have seen a few recent examples of "interface sans inheritance" in the wild (e.g. Falcon and apistar, and Flask plugins) and quite like the DX of it. Though to be honest, I can't give a strong argument for doing it that way except that I like the aesthetics.

@lafrech

This comment has been minimized.

Copy link
Member Author

commented May 27, 2018

+1. I think OpenAPIConverter is a fine name.

I was about to leave it as Swagger. I can do the renaming. And change the file name accordingly.

I think a string would be the most user-friendly, but I suppose that would mean that the version would have to be parsed in many different places, which could be a pain. I delegate to you on that decision.

I introduced an OpenAPIVersion object that is supposed to make this easier. I'm not a fan of distutils LooseVersion. Semantic versioning can be complicated. I thought about adding a dependency to a semver parsing library, but I figured that OpenAPIVersion object was enough for the moment.

This is a matter of taste. I have seen a few recent examples of "interface sans inheritance" in the wild (e.g. Falcon and apistar) and quite like the DX of it. Though to be honest, I can't give a strong argument for doing it that way except that I like the aesthetics.

OK, I'll try the base class approach and see if it is actually better.

@lafrech

This comment has been minimized.

Copy link
Member Author

commented May 27, 2018

Also, I'm not perfectly happy about the plugin initialization. I introduced a init_spec mechanism "à la Flask" but still, the init is sort of a chicken and egg situation.

A plugin is instantiated then passed to the spec object, and then the spec object is passed to the plugin's init_spec method. Between __init__ and init_spec, the plugin is in a broken state because it has no spec object. I don't think it matters in practice but it looks awkward. If used in a wrong way, it would raise weird "None had no [...] attribute" exceptions. We could add a getter and a setter around the plugin's spec attribute but I'm not sure it's worth it. I don't see why someone would use a plugin without passing it a spec.

Anyway, maybe you'll have a better idea for this.

@lafrech lafrech referenced this issue May 28, 2018
3 of 3 tasks complete
@sloria

This comment has been minimized.

Copy link
Member

commented May 28, 2018

Hm, I don't think you need handle malformed plugin instances that don't have a spec. Unlike Flask plugins, users should not need to use plugin instances after they are instantiated. In fact, users shouldn't have to call init_spec themselves.

# YES
spec = APISpec(
    plugins= [
        MarshmallowPlugin()
    ]
)

# NO
ma = MarshmallowPlugin()  # malformed (no spec)
ma.init_app(spec)
@lafrech

This comment has been minimized.

Copy link
Member Author

commented May 28, 2018

I added a commit to provide a BasePlugin class. I think it looks better this way.

However, it revealed an unrelated issue affecting response helpers. Looks like the description: some data string in the test docstrings is not correctly parsed into a dictionary (see test failures).

@lafrech

This comment has been minimized.

Copy link
Member Author

commented May 28, 2018

Hm, I don't think you need handle malformed plugin instances that don't have a spec. Unlike Flask plugins, users should not need to use plugin instances after they are instantiated. In fact, users shouldn't have to call init_spec themselves.

Yes, I agree. So there is no problem, then. init_spec is called automatically when instantiating ApiSpec with the plugin instance.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.