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

call as_view in methodresolver #1552

Merged
merged 5 commits into from
Jun 24, 2022

Conversation

bluebrown
Copy link
Contributor

@bluebrown bluebrown commented Jun 11, 2022

Signed-off-by: Nico Braun rainbowstack@gmail.com

Fixes #1549

Warning
this is a breaking change
users will be required to change any search method to a get method, merging get by id and get collection into a single handler

The primary reason for this change is to be able to use method decorators as described here: https://flask.palletsprojects.com/en/2.1.x/views/#decorating-views

Changes proposed in this pull request:

  • change the MethodResolver to call as_view on the classes

Signed-off-by: Nico Braun <rainbowstack@gmail.com>
@bluebrown
Copy link
Contributor Author

I did not really find out how to write a test for this. All tests have passed. I think this kind of functionality is currently not covered. Like there is no app created to see what the methods return. Or perhaps, I overlooked it.

@bluebrown
Copy link
Contributor Author

We, should probably also update the documentation on this.

@coveralls
Copy link

coveralls commented Jun 11, 2022

Pull Request Test Coverage Report for Build 2489119200

  • 22 of 28 (78.57%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.07%) to 94.27%

Changes Missing Coverage Covered Lines Changed/Added Lines %
connexion/resolver.py 22 28 78.57%
Totals Coverage Status
Change from base Build 2416536603: -0.07%
Covered Lines: 2665
Relevant Lines: 2827

💛 - Coveralls

Copy link
Member

@RobbeSneyders RobbeSneyders left a comment

Choose a reason for hiding this comment

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

Thanks @bluebrown.

  • The changes look good to me in general. I left one small comment on the example.
  • We should indeed update the docs on this. Could you have a look?
  • As mentioned in the issue, it would be good to still offer a resolver with the old behavior, but we can mark it deprecated.
  • The example currently doesn't run on main since we're in the middle of a large refactoring (Move functionality into pluggable ASGI middleware stack #1489), but the resolver itself does seem to work correctly. I submitted a PR which fixes the blocking issue (Use resolver in security middleware #1553).
  • The current resolver tests indeed only check the generated operation_id, which is unchanged. I'll have a deeper look at the tests later to see how we can best add a test for the changed behavior.

class PetsView(MethodView):
""" Create Pets service
"""
method_decorators = []
decorators = [example_decorator]
pets = {}

def post(self):
Copy link
Member

Choose a reason for hiding this comment

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

This needs a body argument to work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have updated the example to use the body param in post and put

Signed-off-by: Nico Braun <rainbowstack@gmail.com>
Signed-off-by: Nico Braun <rainbowstack@gmail.com>
Signed-off-by: Nico Braun <rainbowstack@gmail.com>
@bluebrown
Copy link
Contributor Author

  • added a way to use the old resolver as DeprecatedMethodViewResolver
  • added a class_arguments parameter to the new MethodViewResolver to allow dependency injection
  • updated the tests to test both resolvers with the existing tests via parameterized fixture
  • updated the example app
  • updated the docs

Signed-off-by: Nico Braun <rainbowstack@gmail.com>
Copy link
Member

@RobbeSneyders RobbeSneyders left a comment

Choose a reason for hiding this comment

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

Thanks for the changes @bluebrown.
I left one comment, but overall changes look great.

I still need to find the time to have a look at how we can add an additional test for the resulting behavior,

@@ -192,7 +192,8 @@ def post(self):
return ...
"""

def __init__(self, *args, **kwargs):
def __init__(self, *args, class_arguments=None, **kwargs):
self.class_arguments = class_arguments or {}
Copy link
Member

Choose a reason for hiding this comment

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

Could we name this a bit more specific eg. view_arguments?
It would also be good to document it here as an argument on top of the documentation you already added.

Copy link
Contributor Author

@bluebrown bluebrown Jun 17, 2022

Choose a reason for hiding this comment

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

I think class_arguments is more fitting because the arguments for the view function are also called class_args and class_kwargs. That's because these arguments are passed to the class when instantiating it.

Copy link
Contributor Author

@bluebrown bluebrown Jun 17, 2022

Choose a reason for hiding this comment

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

I will try to add a comment to the init function. I had actually one already with example usage, but the pre-commit hook didn't like it.

Copy link
Member

@RobbeSneyders RobbeSneyders left a comment

Choose a reason for hiding this comment

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

Thanks @bluebrown. I'll merge this one and submit a test as a separate PR as we need to rebase on main anyway to make it work. I'll tag you there.

@RobbeSneyders RobbeSneyders merged commit 3c6e13c into spec-first:main Jun 24, 2022
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.

support method view decorator
3 participants