Skip to content

Conversation

@sharbov
Copy link

@sharbov sharbov commented Oct 21, 2018

Fixes #719 .

This is an initial version to see if I'm on the right track,
I'll be happy to hear some feedback,
then I'll add the tests.

FYI @dtkav

@sharbov sharbov force-pushed the add_plugins_support branch from 9f1caf9 to 8426b06 Compare October 21, 2018 15:56
will be passed the framework's request context.
:type pass_context_arg_name: str | None
:param plugins: list of plugin classes
:type plugins: list

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
:type plugins: list
:type plugins: list | None

will be passed the framework's request context.
:type pass_context_arg_name: str | None
:param plugins: list of plugin classes
:type plugins: list

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
:type plugins: list
:type plugins: list | None

logger.debug('... Adding uri parsing decorator (%r)', uri_parsing_decorator)
function = uri_parsing_decorator(function)

for plugins_decorator in self.__plugins_decorator:

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the for loop?
self.__plugins_decorator yields once.

@dtkav
Copy link
Collaborator

dtkav commented Oct 27, 2018

I think this PR as-is is significantly less flexible than overriding AbstractApi.function().
If you override AbstractApi.function() you can insert user code anywhere, or even replace middleware, but the proposed PR forces any plugins to run before uri parsing and validation. This seems somewhat arbitrary to me.
Django has a pretty good middleware concept (https://docs.djangoproject.com/en/2.1/topics/http/middleware/#activating-middleware), but it's more holistic. (example below)

MIDDLEWARE = [
    'django.middleware.security.SecurityMiddleware',
    'django.contrib.sessions.middleware.SessionMiddleware',
    'django.middleware.common.CommonMiddleware',
    'django.middleware.csrf.CsrfViewMiddleware',
    'django.contrib.auth.middleware.AuthenticationMiddleware',
    'django.contrib.messages.middleware.MessageMiddleware',
    'django.middleware.clickjacking.XFrameOptionsMiddleware',
]

I think the plugin concept is cool, but IMHO it should aim to basically replace the entire AbstractApi.function() method.

What do you think? Do you like the django-style approach?

BTW thanks for making the PR, I think it is perfect for starting concrete discussion around a plugin architecture!

@sharbov
Copy link
Author

sharbov commented Oct 28, 2018

Hey @dtkav

Thanks for the review.

I see your point about the lack of flexibility in the current PR.
At the time I felt like refactring the entire AbstractApi.function would be a too drastic change.

The Django middleware seems interesting, and a similar principle might apply here as well,
each of the decorators used in the function can be viewed as a kind of plugin / middleware.

Would you like me to have another go at it or would you rather wait for more input on this?

@cognifloyd
Copy link
Contributor

cognifloyd commented Dec 11, 2019

I prefer the approach in #760 for making pluggable deserialization/validation. I think the same approach will extend well to serialization.

What other kinds of plugins does this PR enable that content-type based plugins might not?

@RobbeSneyders
Copy link
Member

Closing due to lack of contributor feedback.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Plugin Mechanism

6 participants