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

Add quart compatibility. #189

Closed
wants to merge 10 commits into from
Closed

Add quart compatibility. #189

wants to merge 10 commits into from

Conversation

Ristellise
Copy link
Contributor

@Ristellise Ristellise commented Oct 9, 2019

This PR adds:

  • quart_compat.py [A simple test module with 1 function only.]
  • async_compat.py [Conditional import]
  • Quart compatibility fixes to get it working with it.

If this PR gets accepted, this would fix issues with quart with flask-security.
This does not affect any flask functionality as far as I'm aware.
If I need to fix/change/lint anything please let me know.

- quart_compat.py [A simple test modules with 1 function only.]
- Quart compatibility fixes to get it working with it.
Copy link
Member

@jwag956 jwag956 left a comment

Choose a reason for hiding this comment

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

Looks pretty good to me. A couple small points.

Also - I don't see any tests - so that might mean in the future things get broken?

@@ -78,7 +103,6 @@ def users_create(identity, password, active):
@roles.command("create")
@click.argument("name")
@click.option("-d", "--description", default=None)
@click.option("-p", "--permissions")
Copy link
Member

Choose a reason for hiding this comment

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

This is a new capability in Flask-Security.. so shouldn't be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll fix that. Code was mainly copy pasted from an older source version.

@@ -612,7 +612,7 @@ def default_want_json(req):
if req.is_json:
return True
# TODO should this handle json sub-types?
if req.accept_mimetypes.best == "application/json":
if req.accept_mimetypes["application/json"] >= 1.0:
Copy link
Member

Choose a reason for hiding this comment

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

Not sure this is quite the same condition - I realize that quart doesn't fully support werkzeug - however if you support http spec - you probably need to support it. In any case - if you can't - lets make this another conditiponal.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After adding this, I think your right. Problem is that quart doesnt have a .best in their function.

We could try to polyfill or make our on method though...

@Ristellise
Copy link
Contributor Author

Ristellise commented Oct 9, 2019

Also - I don't see any tests - so that might mean in the future things get broken?

Probably but the only thing that would break the entire system would be quart_compat.py check. which is unlikely unless they renamed flask_patch to something else.

EDIT2: The python2.7 tests has been bugging me, I dont think we have asyncio back then. With this PR, Python 2.7 should stop working though...

@jwag956
Copy link
Member

jwag956 commented Oct 9, 2019

For now - I am following Pallets lead on keeping py2.7 support. I don't know when they will drop it - hopefully soon!
Until then - I would like to keep it working - so could you add a if quart and PY3: around async code? you don't support 2.7 - so it shouldn't matter.

@Ristellise
Copy link
Contributor Author

so could you add a if quart and PY3: around async code? you don't support 2.7 - so it shouldn't matter.

Not really. The issue is that async keyword causes a syntax error on all python 2 versions...

On second thoughts doing a try and except could work...

- py.27 tests fixed hopefully
- best property attribute filling
- We try and catch a attribute error for default_wants_json
- PY3 Test
@Ristellise
Copy link
Contributor Author

Ristellise commented Oct 10, 2019

Try and catch doesnt seem to work on python 2.7 tests...

We COULD try this: https://stackoverflow.com/a/57023749 (An exec a hardcoded script source) But at that point, "Is it worth it?"

- Python 2 compatibility... hope this time it works.
- Implement our own best function if it doesn't exist [Quart backwards compatibility.]
Copy link
Member

@jwag956 jwag956 left a comment

Choose a reason for hiding this comment

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

Sorry for all the hassle. Here is an experiment I did which I think is a lot cleaner:

Add a new file async_compat.py:

from flask import current_app
from werkzeug.local import LocalProxy

_security = LocalProxy(lambda: current_app.extensions["security"])

_datastore = LocalProxy(lambda: _security.datastore)
async def _commit(response=None):
    _datastore.commit()
    return response

In views.py:


if PY3:
    import async_compat  # noqa: F401
else:
    def _commit(response=None):
        _datastore.commit()
        return response

And finally - in pytest.ini add:

    async_compat.py ALL

@jwag956
Copy link
Member

jwag956 commented Nov 1, 2019

I cloned your PR, fixed coverage issues and merged...

Thanks!

@jwag956 jwag956 closed this Nov 1, 2019
@Ristellise
Copy link
Contributor Author

Thanks again for all your work. :)

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.

None yet

2 participants