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

URL Converters should have access to app context #3088

Closed
tedivm opened this issue Jan 31, 2019 · 9 comments · Fixed by #3104
Closed

URL Converters should have access to app context #3088

tedivm opened this issue Jan 31, 2019 · 9 comments · Fixed by #3104
Milestone

Comments

@tedivm
Copy link

tedivm commented Jan 31, 2019

Expected Behavior

The following code should create a new URL converter that converts a specific field to a model (or None).

class ModelConverter(BaseConverter):

    def to_python(self, value):
        return models.Example.query.filter(models.Example.name == value).first()

    def to_url(self, value):
        if isinstance(value, str):
            return value
        return value.name

Actual Behavior

The converters don't have access to the app context so an error is thrown.

Bad Solution

class OrganizationConverter(BaseConverter):
    app = None
    def to_python(self, value):
        with self.app.app_context():
            return repositories.Organization.query.filter(repositories.Organization.name == value).first()

    def to_url(self, value):
        if isinstance(value, str):
            return value
        return value.name

This solution tries to get around it by creating a new app context. The problem is that the context disappears, killing the database session at the same time. This prevents SQLAlchemy features (such as lazy loading).

Environment

  • Python version: 3.6 and 3.7
  • Flask version: 1.0.2
  • SQLAlchemy version: 1.2.17
  • Flask-SQLAlchemy version: 2.3.2
  • Werkzeug version: 0.14.1

Additional Resources

This issue has also come up on stack overflow

@davidism
Copy link
Member

I've also ran into this issue with converters, but it's might not be straightforward to change at this point.

URL matching is done as part of creating the request context, not pushing the context:

self.match_request()

So we'd need to move it into RequestContext.push, after it's actually pushed so that request is populated:

_request_ctx_stack.push(self)

It's a more internal part of Flask, and I haven't really seen any discussion about overriding or using these parts, but I'd have to understand that more before making the change.

@tedivm
Copy link
Author

tedivm commented Jan 31, 2019

I managed to accomplish essentially the same thing with decorator functions (and added it on stack overflow), but it would definitely be powerful to be able to swap url parameters with their models.

@davidism
Copy link
Member

davidism commented Feb 3, 2019

Let's start with a PR that makes the change I described, and see if it affects any existing tests. I'm happy to review a PR if someone wants to contribute.

@eladm26
Copy link
Contributor

eladm26 commented Feb 24, 2019

@davidism Hi!
I created a PR for this issue and tested it, should I add you as a reviewer?
sorry, it's my PR here

@davidism
Copy link
Member

It doesn't look like you've submitted a PR.

@eladm26
Copy link
Contributor

eladm26 commented Feb 24, 2019

ok sorry, a question before I add the PR.
I added the following test and did not change the code yet:

def test_model_converters(app, client):
    from werkzeug.routing import BaseConverter

    app.config['SQLALCHEMY_TRACK_MODIFICATIONS'] = False
    app.config['SQLALCHEMY_DATABASE_URI'] = 'sqlite:////tmp/test.db'
    db = SQLAlchemy(app)

    class User(db.Model):
        id = db.Column(db.Integer, primary_key=True)
        username = db.Column(db.String(80), unique=True, nullable=False)
        email = db.Column(db.String(120), unique=True, nullable=False)

    db.create_all()
    admin = User(username='admin', email='admin@example.com')
    db.session.add(admin)

    class ModelConverter(BaseConverter):
        def to_python(self, value):
            return User.query.filter(User.username == value).first()

        def to_url(self, value):
            if isinstance(value, str):
                return value
            return value.username

    app.url_map.converters['model'] = ModelConverter

    @app.route('/<model:user_name>')
    def index(user_name):
        return user_name.email

    assert client.get('/admin').data == b'admin@example.com'

However, I did not get the out of context exception, what am I missing here?

@ThiefMaster
Copy link
Member

When using db = SQLAlchemy(app) the object is bound to the app and thus works without a context. Use this instead to avoid it:

db = SQLAlchemy()
db.init_app(app)

@eladm26
Copy link
Contributor

eladm26 commented Feb 24, 2019

@davidism this is the PR #3104

@davidism davidism added this to the 1.1.0 milestone Jun 13, 2019
@michaeltoohig
Copy link

michaeltoohig commented Sep 12, 2019

Is this well documented yet? I'm here since I read the 1.1.1 release notes and was looking for some examples of how this feature could be used. The test above by @eladm26 clears things up but it took awhile for me to find it here.

Here's the part from the release notes:

URL routing is performed after the request context is pushed. This allows custom URL converters to access current_app and request. This makes it possible to implement converters such as one that queries the database for a model based on the ID in the URL.

Thanks.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 14, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants