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

Don't import concrete security factories in init #1377

Merged
merged 3 commits into from
Jul 7, 2021

Conversation

Ruwann
Copy link
Member

@Ruwann Ruwann commented Jul 3, 2021

The concrete SecurityHandlerFactory implementations require the corresponding libraries to be installed.
When importing them in security.__init__.py, this can raise an ImportError.

For example, when running the helloworld example which uses FlaskApp and shouldn't require aiohttp:

$ python hello.py 
Traceback (most recent call last):
  File "hello.py", line 10, in <module>
    app = connexion.FlaskApp(__name__, port=9090, specification_dir='swagger/')
  File "/home/ruwan/projects/connexion/connexion/__init__.py", line 22, in _required_lib
    raise exc
  File "/home/ruwan/projects/connexion/connexion/__init__.py", line 30, in <module>
    from .apis.flask_api import FlaskApi, context  # NOQA
  File "/home/ruwan/projects/connexion/connexion/apis/flask_api.py", line 13, in <module>
    from connexion.security import FlaskSecurityHandlerFactory
  File "/home/ruwan/projects/connexion/connexion/security/__init__.py", line 8, in <module>
    from .aiohttp_security_handler_factory import AioHttpSecurityHandlerFactory  # NOQA
  File "/home/ruwan/projects/connexion/connexion/security/aiohttp_security_handler_factory.py", line 3, in <module>
    import aiohttp
ModuleNotFoundError: No module named 'aiohttp'

This PR addresses this by not importing the concrete classes in security.__init__.py. Another option is to do something similar to connexion.__init__.py, but I opted for this approach because the SecurityHandlerFactory isn't exposed directly to the end user, but happy to change it so this also uses that approach.

@coveralls
Copy link

coveralls commented Jul 3, 2021

Coverage Status

Coverage increased (+0.004%) to 97.081% when pulling b69d0d6 on Ruwann:bugfix/aiohttp-security into 1012721 on zalando:main.

@RobbeSneyders
Copy link
Member

Good catch! Not a big preference for one solution, but I do like the cleaner imports by importing them in __init__.

I think we should update our tests to prevent this from happening again. We can group tests by marking them and run separate test suites for aiohttp & Flask with only the necessary imports installed.

@Ruwann
Copy link
Member Author

Ruwann commented Jul 6, 2021

Thanks! I have updated the PR to use the approach of catching the ImportErrors in security.__init__.py.

I have also opened issue #1389 to discuss how to best set up the testing suites for optional dependencies, as there are different ways of doing this.

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.

Ok sounds good!
Left one small comment.

connexion/security/__init__.py Outdated Show resolved Hide resolved
@Ruwann Ruwann merged commit c014299 into spec-first:main Jul 7, 2021
@Ruwann Ruwann deleted the bugfix/aiohttp-security branch July 7, 2021 18:53
@hjacobs hjacobs added this to the 2.8 milestone Jul 8, 2021
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.

4 participants