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

Support Sanic #1250

Closed
wants to merge 15 commits into from
Closed

Support Sanic #1250

wants to merge 15 commits into from

Conversation

ioggstream
Copy link
Contributor

Fixes #496 .

Changes proposed in this pull request:

  • support Sanic
  • creating a sanic_api.py and sanic_app.py modules
  • add relevant tests

@kigawas
Copy link
Contributor

kigawas commented Oct 26, 2020

Is it ready to merge? Looks good to me

@@ -11,5 +11,5 @@
"options",
"head",
"patch",
"trace"
# "trace" # FIXME: rpolli
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kigawas Sanic does not support trace. Ideas welcome!

Copy link
Contributor

Choose a reason for hiding this comment

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

Trace method is rather rare, do we really need it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's in connexion code, so we should ask them. @dtkav

@@ -11,7 +11,7 @@
logging.basicConfig(level=logging.DEBUG)

TEST_FOLDER = pathlib.Path(__file__).parent
FIXTURES_FOLDER = TEST_FOLDER / 'fixtures'
FIXTURES_FOLDER = TEST_FOLDER / "fixtures"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kigawas iiuc we both like black but I'd limit this kind of changes on connexion repo :)

Copy link
Contributor

Choose a reason for hiding this comment

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

@ioggstream
Copy link
Contributor Author

@kigawas still there are tests failing, and more should be added.
A discussion with the connexion community should be started to check they are interested in supporting sanic.

@kigawas
Copy link
Contributor

kigawas commented Oct 26, 2020

@kigawas still there are tests failing, and more should be added.
A discussion with the connexion community should be started to check they are interested in supporting sanic.

Lots of errors like TypeError: Can't instantiate abstract class SanicApi with abstract methods make_security_handler_factory

Maybe it's better to add flake8, black and mypy to make life easier

@kigawas
Copy link
Contributor

kigawas commented Oct 27, 2020

@kigawas still there are tests failing, and more should be added.
A discussion with the connexion community should be started to check they are interested in supporting sanic.

--- a/connexion/apis/abstract.py
+++ b/connexion/apis/abstract.py
@@ -37,7 +37,8 @@ class AbstractAPI(metaclass=AbstractAPIMeta):
     def __init__(self, specification, base_path=None, arguments=None,
                  validate_responses=False, strict_validation=False, resolver=None,
                  auth_all_paths=False, debug=False, resolver_error_handler=None,
-                 validator_map=None, pythonic_params=False, pass_context_arg_name=None, options=None):
+                 validator_map=None, pythonic_params=False, pass_context_arg_name=None, options=None,
+                 ):
         """
         :type specification: pathlib.Path | dict
         :type base_path: str | None
@@ -101,6 +102,8 @@ class AbstractAPI(metaclass=AbstractAPIMeta):
         logger.debug('pass_context_arg_name: %s', pass_context_arg_name)
         self.pass_context_arg_name = pass_context_arg_name

+        self.security_handler_factory = self.make_security_handler_factory(pass_context_arg_name)
+
         if self.options.openapi_spec_available:
             self.add_openapi_json()
             self.add_openapi_yaml()
@@ -143,6 +146,11 @@ class AbstractAPI(metaclass=AbstractAPIMeta):
         Adds a 404 error handler to authenticate and only expose the 404 status if the security validation pass.
         """

+    @staticmethod
+    @abc.abstractmethod
+    def make_security_handler_factory(pass_context_arg_name):
+        """ Create SecurityHandlerFactory to create all security check handlers """
+
     def add_operation(self, path, method):
         """
         Adds one operation to the api.

Solution

index d2050b1..feb095d 100644
--- a/connexion/apis/flask_api.py
+++ b/connexion/apis/flask_api.py
@@ -9,6 +9,7 @@ from connexion.handlers import AuthErrorHandler
 from connexion.jsonifier import Jsonifier
 from connexion.lifecycle import ConnexionRequest, ConnexionResponse
 from connexion.utils import is_json_mimetype, yamldumper
+from connexion.security import FlaskSecurityHandlerFactory
 from werkzeug.local import LocalProxy

 logger = logging.getLogger('connexion.apis.flask_api')
@@ -16,6 +17,11 @@ logger = logging.getLogger('connexion.apis.flask_api')

 class FlaskApi(AbstractAPI):

+    @staticmethod
+    def make_security_handler_factory(pass_context_arg_name):
+        """ Create default SecurityHandlerFactory to create all security check handlers """
+        return FlaskSecurityHandlerFactory(pass_context_arg_name)
+
     def _set_base_path(self, base_path):
         super(FlaskApi, self)._set_base_path(base_path)
         self._set_blueprint()

@kigawas
Copy link
Contributor

kigawas commented Oct 27, 2020

@ioggstream

We are actively using this PR and it works pretty well, but there also come several bugs like

  1. For the value in url query, only the last character or number will get passed into handlers (like abc=123&def=456, you'll only get {'abc': '3', 'def': '6'})

  2. For uuid arguments, an extra dot appears (like you'll get .dc1332bd-c104-4d6f-b3ee-8ea83dccb028 instead of dc1332bd-c104-4d6f-b3ee-8ea83dccb028. Probably this is a connexion bug)

@ioggstream
Copy link
Contributor Author

@ioggstream

We are actively using this PR and it works pretty well, but there also come several bugs ...
That's great! Happy that it works...

I am happy to review and merge your patches, but we really need to engage with connexion folks to continue :)

  1. For the value in url query, only the last character or number will get passed into handlers (like abc=123&def=456, you'll only get {'abc': '3', 'def': '6'})
  2. For uuid arguments, an extra dot appears

@kigawas
Copy link
Contributor

kigawas commented Oct 27, 2020

I am happy to review and merge your patches, but we really need to engage with connexion folks to continue :)

@zalando @hjacobs

I apologise if it bothers you, but it'd be really nice to support sanic 😃

@kigawas
Copy link
Contributor

kigawas commented Nov 6, 2020

This pr would fix most of the errors mentioned above, except one.

ioggstream#4

Here is the pytest result (for only tests/sanic/test_sanic_bootstrap.py)

=============================================================================== short test summary info ================================================================================
FAILED tests/sanic/test_sanic_bootstrap.py::test_app_with_different_uri_parser - AssertionError: assert ['d'] == ['a', 'b', 'c']
===================================================================== 1 failed, 42 passed, 2654 warnings in 10.84s =====================================================================

For full log: https://gist.github.com/kigawas/b26a4905d4f7118bd70ed8c532fd2321

@@ -5,3 +5,5 @@
# concrete
from .flask_security_handler_factory import FlaskSecurityHandlerFactory # NOQA
from .aiohttp_security_handler_factory import AioHttpSecurityHandlerFactory # NOQA

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kigawas can we remove this space?

@@ -3,8 +3,6 @@
import functools
import logging

import aiohttp
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kigawas can you PR this separately to the main connexion branch so that we don't mess with files we are not going to change?

Copy link
Contributor

Choose a reason for hiding this comment

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

Since the sanic dependencies do not include aiohttp, it's better to remove this to avoid potential import error

Copy link
Contributor

Choose a reason for hiding this comment

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

#1320

pr proposed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kigawas I understand, but let's push on #1320 and remove this change from this PR so we don't have to rebase :)

Copy link
Contributor

Choose a reason for hiding this comment

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

No worries, I'll update later

@Tzvika-m
Copy link

Is this still active?
Would be super useful (:
Thanks!

@kigawas
Copy link
Contributor

kigawas commented Mar 2, 2021

Is this still active?
Would be super useful (:
Thanks!

This PR should work itself, but the maintenance of this repo is not active I guess

@ioggstream
Copy link
Contributor Author

Hi folks, if there is some interest in the connexion community we can continue to work on it....

@RobbeSneyders
Copy link
Member

Connexion 3.0 will support ASGI frameworks out of the box. See #1395

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 for sanic/starlette framework or async/await
4 participants