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

avoid creating a new app context when using json argument in test client #2900

Closed
mbarakaja opened this issue Sep 2, 2018 · 5 comments

Comments

@mbarakaja
Copy link

commented Sep 2, 2018

At this moment, when a Flask test client method uses the json argument, a new application context is created in the line below.

with app.app_context():

The problem is that when the above created context goes out of scope, anything registered to teardown_appcontext is called. This is the case of Flask-SQLAlchemy, which remove the current session when that happens.

This is problematic is some tests scenarios.

I am using PostgreSQL SAVEPOINT to rollback any changes made by a test case. My approach is similar to this example provided by SQLAlchemy here, so I'm not going to put all my code here, just a shallow example.

I have a class that creates a nested transaction and rollback everything when needed.

client = app.test_client()
trn = TransactionManager(database)

with app.app_context():
    trn.start() # start a new nested transaction

    client.post('/products', data=json.dumps({'name': 'Cake'}))
    client.get('/products')
    
    trn.end() # Rollback before the session is removed.

The above works well because everything happens inside the same transaction and each test client interaction can see changes made to the database by previous client calls.

But using the json argument, the POST call will first create an application context, which immediately will go out of scope making Flask-SQLAlchemy's teardown_appcontext registered function remove the current session before the next client call, making each database interaction in a different transaction.

Why do we need an application context to dump the JSON string?

kwargs['data'] = json_dumps(kwargs.pop('json'))

I don't understand, but if the the above logic is necessary I think the code should check first if there is an application context pushed already using has_app_context.

Am I doing something wrong or I am missing something?

@kumy

This comment has been minimized.

Copy link

commented Sep 20, 2018

Hi, I passed hours trying to find why my blended objects were detached from the session just after using get() function. I finally reached to the same problematic line. Removing the context creation or replacing it with test_request_context fixed my problem.

What do Flask gurus think about the current implementation?

flask/flask/testing.py

Lines 81 to 86 in c92001d

# push a context so flask.json can use app's json attributes
with app.app_context():
kwargs['data'] = json_dumps(kwargs.pop('json'))
if 'content_type' not in kwargs:
kwargs['content_type'] = 'application/json'

kumy added a commit to geokrety/flask that referenced this issue Sep 20, 2018

@garenchan

This comment has been minimized.

Copy link
Contributor

commented Sep 22, 2018

I think this may be better than using test_request_context:

from flask import current_app

try:
    push_ctx = current_app is not app
    if push_ctx:
        ctx = app.app_context()
        ctx.push()

    kwargs['data'] = json_dumps(kwargs.pop('json'))
finally:
    if push_ctx:
        ctx.pop()
@mbarakaja

This comment has been minimized.

Copy link
Author

commented Sep 22, 2018

@kumy Did you test your code?.

The Flask test client and test_request_context use make_test_environ_builder under the hood. So, probably your code is going to end up in a recursive call.

@kumy

This comment has been minimized.

Copy link

commented Sep 22, 2018

@mbarakaja Yes, it is currently deployed in my dev environment and it works.
I also just tested code from @garenchan and it's also working in my use case.

garenchan added a commit to garenchan/flask that referenced this issue Sep 23, 2018

Don't use app_context context manager while accessing configuration
If we just want to access configuration, it's important to not use push
and pop methods of the actual app context object since that would mean
the cleanup handlers will be called.

Fix issue pallets#2900.

garenchan added a commit to garenchan/flask that referenced this issue Sep 23, 2018

Don't use app_context context manager while accessing configuration.
If we just want to access configuration, it's important to not use push
and pop methods of the actual app context object since that would mean
the cleanup handlers will be called.

Fix issue pallets#2900.
@soundstripe

This comment has been minimized.

Copy link

commented Nov 4, 2018

I’m in the exact same situation. The double app context teardown is making it difficult to roll back sessions in test environments. Once for jsonify and once again for the actual request.

@miguelgrinberg miguelgrinberg added question and removed question labels Jan 13, 2019

@davidism davidism added this to the 1.0.3 milestone May 13, 2019

@davidism davidism changed the title Consider avoiding creating a new app context when using json argument in test client avoid creating a new app context when using json argument in test client May 17, 2019

@davidism davidism closed this May 17, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.