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

[POC] Added Support for AsyncSession in SQLAlchemy 1.4+ #1413

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

zikphil
Copy link

@zikphil zikphil commented Nov 28, 2021

Im adding a PoC for AsyncSession support in the latest sqlalchemy-oso module.

2 functions are added: authorized_async_sessionmaker and async_scoped_session, both are expecting AsyncSessions as opposed to normal Session.

SQLAlchemy converts all code to Sync when time comes to process signals so there shouldn't be a need for any modifications there. I have tested with the following code from your tutorial and got the expected output:

https://gist.github.com/zikphil/9b0c7d70b789796bcbcc56528e41ea51

Feel free to adapt this code as you see fit.

@github-actions
Copy link

github-actions bot commented Nov 28, 2021

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@zikphil
Copy link
Author

zikphil commented Nov 28, 2021

I have read the CLA Document and I hereby sign the CLA

@gj
Copy link
Member

gj commented Dec 1, 2021

Hi @zikphil thanks for the contribution! I'm sorry it's taken us a few days to get to it — we've been pretty heads-down on getting ready for tomorrow's release. I'll make sure this PR gets reviewed ASAP

Copy link
Contributor

@gkaemmer gkaemmer left a comment

Choose a reason for hiding this comment

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

Thanks for this, this is awesome -- glad its not too bad to get this working.

To get this merged, I think we'll need to do some testing in sqlalchemy 1.3, which we need to continue supporting. Right now I'm pretty sure there are a few ways this breaks compatibility.

Do you think you'd have time to do that testing? We'd love to get this out in the next release (in two weeks)

languages/python/sqlalchemy-oso/sqlalchemy_oso/session.py Outdated Show resolved Hide resolved
languages/python/sqlalchemy-oso/sqlalchemy_oso/session.py Outdated Show resolved Hide resolved
@zikphil
Copy link
Author

zikphil commented Dec 7, 2021

Please review the new implementation. I have split it out in separate files and added a condition for SQLAlchemy => 1.4

@zikphil zikphil requested a review from gkaemmer December 9, 2021 14:05
Copy link
Contributor

@gkaemmer gkaemmer left a comment

Choose a reason for hiding this comment

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

I think from an implementation standpoint this looks good to me. Do you think you could add a few tests, one for each of async_authorized_sessionmaker and async_scoped_session? I would think you could copy some tests verbatim from test_sqlalchemy.py and add @pytest.mark.async to make them asynchronous.

You'd also probably need to skip the test file during the sqlalchemy 1.3 tests (which the test suite runs in parallel with tox). You should be able to use pytest.mark.skip with allow_module_level=True to do that (see: https://docs.pytest.org/en/latest/how-to/skipping.html#skip).

Lemme know if you need any tips on that. Thanks again for this contribution -- very excited to have this in.

@alexhafner
Copy link
Contributor

Hi @zikphil - do you have a chance to move forward with this PR? We had used it with a handful of calls so far and it worked fine, would be great to have it operating on top of oso 0.26.0   😇

@gj gj marked this pull request as draft October 12, 2022 14:07
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.

5 participants