graphql-python / graphene-sqlalchemy

Graphene SQLAlchemy integration
http://docs.graphene-python.org/projects/sqlalchemy/en/latest/
MIT License
980 stars 226 forks source link

feat(async): add support for async sessions #350

Closed jendrikjoe closed 1 year ago

jendrikjoe commented 2 years ago

This is my initial suggestion on how to support async sessions in graphene-sqlalchemy as discussed in https://github.com/graphql-python/graphene-sqlalchemy/issues/324 πŸ™‚ It is not completely done yet, but I thought I would already open it to give you a chance for some feedback. Sorry as well for all the style changes. My IDE ran black over it. I can revert those changes, however I couldn't find anything in the contribution guidelines regarding the style, so I thought I keep it for the time being πŸ€·β€β™‚οΈ As mentioned in the original issue: Async only supports joined and selectin loads and no lazy ones. This should be stated somewhere in the docs, I assume πŸ˜‰

Open points:

erikwrede commented 2 years ago

Thank you! I'll have a look at this in the next days.

codecov-commenter commented 2 years ago

Codecov Report

Base: 97.09% // Head: 96.24% // Decreases project coverage by -0.84% :warning:

Coverage data is based on head (1e857e0) compared to base (2edeae9). Patch coverage: 90.74% of modified lines in pull request are covered.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #350 +/- ## ========================================== - Coverage 97.09% 96.24% -0.85% ========================================== Files 7 9 +2 Lines 722 879 +157 ========================================== + Hits 701 846 +145 - Misses 21 33 +12 ``` | [Impacted Files](https://codecov.io/gh/graphql-python/graphene-sqlalchemy/pull/350?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=graphql-python) | Coverage Ξ” | | |---|---|---| | [graphene\_sqlalchemy/batching.py](https://codecov.io/gh/graphql-python/graphene-sqlalchemy/pull/350/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=graphql-python#diff-Z3JhcGhlbmVfc3FsYWxjaGVteS9iYXRjaGluZy5weQ==) | `95.34% <75.00%> (ΓΈ)` | | | [graphene\_sqlalchemy/types.py](https://codecov.io/gh/graphql-python/graphene-sqlalchemy/pull/350/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=graphql-python#diff-Z3JhcGhlbmVfc3FsYWxjaGVteS90eXBlcy5weQ==) | `93.45% <76.47%> (-1.95%)` | :arrow_down: | | [graphene\_sqlalchemy/fields.py](https://codecov.io/gh/graphql-python/graphene-sqlalchemy/pull/350/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=graphql-python#diff-Z3JhcGhlbmVfc3FsYWxjaGVteS9maWVsZHMucHk=) | `98.58% <100.00%> (-0.58%)` | :arrow_down: | | [graphene\_sqlalchemy/utils.py](https://codecov.io/gh/graphql-python/graphene-sqlalchemy/pull/350/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=graphql-python#diff-Z3JhcGhlbmVfc3FsYWxjaGVteS91dGlscy5weQ==) | `93.85% <100.00%> (ΓΈ)` | | | [graphene\_sqlalchemy/\_\_init\_\_.py](https://codecov.io/gh/graphql-python/graphene-sqlalchemy/pull/350/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=graphql-python#diff-Z3JhcGhlbmVfc3FsYWxjaGVteS9fX2luaXRfXy5weQ==) | `100.00% <0.00%> (ΓΈ)` | | Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=graphql-python). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=graphql-python)

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

erikwrede commented 2 years ago

Looks like everything is coming together, I'll check the new changes out soon! Thank you for all the effort πŸ™‚

@Fred7b do you also have time for a full review? If I recall correctly, u originally created the issue so I would appreciate the input from you! Formatting will be solved by black soon, so no need to care for that on the review.

jendrikjoe commented 2 years ago

A polite ping πŸ™‚

erikwrede commented 2 years ago

@jendrikjoe sorry, I've been focused on other issues. My plan is to merge those first, release a new beta with the other issues and then immediately release another beta including async session support so both versions can be tried out, in case async session support still has some bugs we didn't find. If there's anything left to change I'll let you know in the next days :)

jendrikjoe commented 2 years ago

@erikwrede no worries and thanks for explaining πŸ€— The release plan makes a lot of sense and take all the time you need for it πŸ‘ Just wanted to make sure that the MR is not forgotten πŸ˜‰

erikwrede commented 2 years ago

Quick Update: Release of this is pushed back until #355 is merged. We're reformatting connection fields and it makes sense to merge that first and then add async sessions later. Should require minor reformatting here once #355 is done. I definitely want to get this integrated afterwards. Sorry to keep you waiting, thanks for all the patience! πŸ™‚

jendrikjoe commented 2 years ago

No worries @erikwrede πŸ€— Just ping me whenever I should resolve conflicts here πŸ‘

erikwrede commented 2 years ago

Hey @jendrikjoe Sorry it took so long! The consolidation is done and ready to be addressed. Should only be causing minor conflicts. LMK if you need any help πŸ™‚

jendrikjoe commented 2 years ago

Thanks for pinging me πŸ‘ Will try resolve the conflicts later today πŸ™‚

erikwrede commented 2 years ago

Great!

I noticed that some of the original tests are still modified to use async methods. I'd prefer that entire part to be separate - leave the sync tests and Schema resolvers as is and create separate object types with async resolvers and corresponding tests. Otherwise a test failure will not tell wether the failure hints at a fundamental problem or problems with the async extension. What do you think?

jendrikjoe commented 2 years ago

I parametrize the schema now to have a synchronous and an asynchronous for all relevant tests. This way every test reports with which schema it failed, while still avoiding to copy all the tests. Or are you referring to the changes I made on the lazy attributes of the SQLModels?

I still have to investigate why the batching tests using get_full_relay_schema are failing for me. Presumably, because I separated the batching models from the others, so the lazy attribute is not set on them.

I as well moved some assert statements in the batching tests. I had some bugs in them, which led to the result of querying the schema having an error and then of course wrong sql statement counts, too. However, first testing that no errors occur when calling the schema, before counting the SQL statements made the debugging a lot easier and the error more apparent.

jendrikjoe commented 2 years ago

Will try to figure as well what the issues with the session fixture is πŸ™ˆ

jendrikjoe commented 2 years ago

Should be all working now πŸ₯³