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

Cyclic dependencies when using AuthorizationServer(Client, app) #8

Closed
moritz89 opened this Issue Jan 2, 2018 · 16 comments

Comments

Projects
None yet
3 participants
@moritz89

moritz89 commented Jan 2, 2018

At least in my case, when creating an AuthorizationServer, it requires the Client model. This has the requirement that the models be imported. However, since the models themselves also import __init__.py through from .. import db, login_manager, it creates a cyclic include.

Due to this constraint, the next best option is to use flask-oauthlib library. It might make sense to address this issue, either in the documentation or in the code architecturally.

@lepture

This comment has been minimized.

Owner

lepture commented Jan 4, 2018

@moritz89 I can't understand your question.

There is a demo: https://github.com/authlib/playground which is working right now.

@moritz89

This comment has been minimized.

moritz89 commented Jan 4, 2018

That is true. I'll give it a bit more context.

In the server documentation you mention that to create the server:

from authlib.flask.oauth2 import AuthorizationServer

# Client is defined above
server = AuthorizationServer(Client, app)

This code would then be placed in __init__.py or app.py. The Client code is defined in models.py or models/client.py. The models require the db, login manager, etc. and therefore import them from __init__.py.

Now we are back to my first comment, that this creates a cyclic include. The documentation for Flask-oauthlib does not have this "limitation".

@lepture

This comment has been minimized.

Owner

lepture commented Jan 4, 2018

@moritz89 I still can't get your point.

# Client is defined above

I mean "Client is defined in the above section of the documentation".

Check https://github.com/authlib/playground/blob/master/website/services/oauth2.py for a real example.

Do you mean I should improve the documentation? There is no cyclic dependency problem, it there is, you are doing it wrong.

@moritz89

This comment has been minimized.

moritz89 commented Jan 4, 2018

Sorry for the misunderstanding. The code you are referring to is 100% correct and works, hats off to you.

What I am meaning to say is, instead of defining the client above, is there a work-around, so that it is possible to place it in the app/models/oauth.py file. The reason I do not like the solution, define the client above, is that it places a model class next to the initialization code, in app/__init__.py, where it does not belong.

As a reference, when I write Flask applications I structure them as follows:

root folder/
app/
    __init__.py
    models.py
    account/
        __init__.py
        forms.py
        views.py
    models/
        __init__.py
        oauth.py
        user.py
    oauth/
        __init__.py
        forms.py
        views.py
    static/
    templates/

Now my question is, is AuthorizationServer(Client, app) mainly for example code, to be used to quickly get started? Thereby it being valid to disregard the convention of placing model classes in model files/folders?

@lepture

This comment has been minimized.

Owner

lepture commented Jan 4, 2018

@moritz89 Please check the example I provided above.

AuthorizationServer(Client, app) is a simple example. The documentation said:

It can also be initialized lazily with init_app:

https://docs.authlib.org/en/latest/flask/oauth2.html#define-server

server = AuthorizationServer(Client)
server.init_app(app)

Please check out the playground code.

https://github.com/authlib/playground/blob/master/website/services/oauth2.py

@lepture lepture closed this Jan 4, 2018

@lepture

This comment has been minimized.

Owner

lepture commented Jan 5, 2018

@moritz89 Does this fix your problem?

@moritz89

This comment has been minimized.

moritz89 commented Jan 5, 2018

Hi, thanks for your patience first of all. It is interesting that the structure, where the db object is created, is different from all other Flask projects that I know. You actually create a base class, in the model folder, from which all other models derive themselves.

What effects on the rest of the project does it have, that the db object is created in the models class?

Thanks for writing all that code the last few days. I was always referring to the locally cloned repo, where only the client, not the server was implemented.

@iabtyagi

This comment has been minimized.

iabtyagi commented Feb 10, 2018

The issue is closed but just sharing how I got it working. Might be helpful for others who are looking for a similar solution.
This is how the application is structured:

/app                # Application Module
     |-- __init__.py               # Code snippet 'B' goes here
     |
     |-- /oauth_module       # Authentication and User module
             |-- __init__.py           # Code snippet 'A' goes here
             |-- controllers.py    # Controllers implement blueprints for routes
             |-- models.py           # Define 'Token', 'Client' and other models here
     |
     |-- /some_entity_module      # another module
             |-- __init__.py
             |-- controllers.py
             |-- models.py
     |__ /static

Snippet A: (goes in app/oauth_module/__init__.py)

    from authlib.flask.oauth2 import AuthorizationServer
    from .models import Client
    oauthserver = AuthorizationServer(Client)

Snippet B: (goes in app/__init__.py)

    app = Flask(__name__)
    db = SQLAlchemy(app)
    from app.oauth_module import oauthserver
    oauthserver.init_app(app)
    # register all blueprints
    from app.oauth_module.controllers import oauth_blueprint
    app.register_blueprint(oauth_blueprint)
    # ....
    db.create_all()

The oauth_module defines blueprints for the routes in its controllers.py
Something like:

from app.oauth_module import oauthserver
from flask import Blueprint
# ..
# Define the grant classes
# ..
oauthserver.register_grant_endpoint(PasswordGrant)
# ..
oauth_blueprint = Blueprint('oauth_blueprint', __name__)

@oauth_blueprint.route('/oauth/token', methods=['POST'])
def issue_token():
    return oauthserver.create_token_response()
@lepture

This comment has been minimized.

Owner

lepture commented Feb 10, 2018

@moritz89 @iabtyagi

I finally got it. You are putting db in the root your_project/__init__.py. I always make my_project/__init__.py clean with this:

my_project/
  __init__.py
  models/
    __init__.py
    base.py
    other_models.py

Here is how it looks in models:

# models/base.py
db = SQLAlchemy()

# models/__init__.py
from .base import db

And then importing it to the root:

# my_project/__init__.py

from my_project.models import db

db.init_app(app)

I'm wondering where did you learnt your Flask directory structure. Flask is very flexible, there is no certain folder structure for it. But putting your db in the root __init__.py is not a good idea, it will sometime create a cyclic dependency.

lepture added a commit that referenced this issue Feb 10, 2018

A breaking change to initialize Client model lazily.
Not everyone has a good folder structure, there are chances
that people don't know how to prevent a cyclic dependency.
With this change, people don't need to init Client directly,
they can initilize it in `init_app`.

Inspired by the issue: #8
@iabtyagi

This comment has been minimized.

iabtyagi commented Feb 10, 2018

@lepture Completely agree on Flask being very flexible. Its flexibility makes it very powerful in the hands of experienced. But people new to Flask or, as in my case, new to Python Web ecosystem, need some structure to begin with. So, I followed this article https://www.digitalocean.com/community/tutorials/how-to-structure-large-flask-applications

Thanks for the commit. And thanks for the sample structure, will try to use it in my application.

@lepture

This comment has been minimized.

Owner

lepture commented Feb 10, 2018

Damn. That tutorial is a really bad example. For large application, we are using the factory pattern:

http://flask.pocoo.org/docs/0.12/patterns/appfactories/

@moritz89

This comment has been minimized.

moritz89 commented Feb 10, 2018

Will definitely have a look at the best practices. I used the user management scaffolding provided by https://github.com/hack4impact/flask-base. What is your impression of their structure and have you come across any other project that does it better?

@lepture

This comment has been minimized.

Owner

lepture commented Feb 10, 2018

@moritz89 that’s why you have a cyclic dependency issue. It is a bad practice to put those things in the root init file then importing them backwards. Instead, the root init should be a holy place to import other things. This is not just a tip for Flask, but for every language. Keep the root as root.

@lepture

This comment has been minimized.

Owner

lepture commented Feb 10, 2018

As I mentioned above, if you read the official guide on the factory guide, there is an example:

def create_app(config_filename):
    app = Flask(__name__)
    app.config.from_pyfile(config_filename)

    from yourapplication.model import db
    db.init_app(app)
@lepture

This comment has been minimized.

Owner

lepture commented Feb 10, 2018

Actually authlib playground has a better folder structure, you can learn something from it.

@lepture

This comment has been minimized.

Owner

lepture commented Apr 21, 2018

@moritz89 @iabtyagi I've written a post on folder structure: https://lepture.com/en/2018/structure-of-a-flask-project

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