strawberry-graphql / strawberry-sqlalchemy

A SQLAlchemy Integration for strawberry-graphql
MIT License
91 stars 26 forks source link

feat: initial relay integration support #65

Closed bellini666 closed 10 months ago

bellini666 commented 11 months ago

TODO:

botberry commented 11 months ago

Thanks for adding the RELEASE.md file!

Here's a preview of the changelog:


Initial relay connection/node support using Strawberry's relay integration.

codecov-commenter commented 11 months ago

Codecov Report

Merging #65 (d86b98f) into main (bf75e07) will increase coverage by 6.26%. Report is 1 commits behind head on main. The diff coverage is 89.69%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #65 +/- ## ========================================== + Coverage 78.45% 84.71% +6.26% ========================================== Files 10 14 +4 Lines 789 1590 +801 Branches 118 260 +142 ========================================== + Hits 619 1347 +728 - Misses 141 191 +50 - Partials 29 52 +23 ```
codspeed-hq[bot] commented 11 months ago

CodSpeed Performance Report

Merging #65 will not alter performance

Comparing bellini666:relay_integration (9ef9785) with main (af6a48c)

Summary

✅ 1 untouched benchmarks

erikwrede commented 11 months ago

hey everyone, I still need some time to fully review this. Will get back when I have done some more testing

bellini666 commented 11 months ago

Hey @erikwrede , @mattalbr

I just got back from djangocon and I'll get back to this PR during the weekend now.

One thing I was wondering here is regarding the fact that you need to have a session to query sqlalchemy objects.

Because of that I had to add a sessionmaker attribute to the fields, and I notice that the data loaders require one as well.

I was thinking about a way to avoid that and I have a proposal. Let me know what you think and I can implement that in this PR:

The idea is to use contextvars to provide a session that will be used during the execution of a query. Not only the relay fields I'm defining here can use it, but the dataloaders can also use it instead of requiring a bind argument.

Something like:

current_session = contextvars.ContextVar("current-session")

class SessionProviderExtension(SchemaExtension):
    def __init__(self, sessionmaker):
        self.sessionmaker = sessionmaker

    def on_execute(self):
        token = current_session.set(self.sessionmaker())
        try:
            yield
        finally:
            current_session.reset(token)

Of course we can also make the current_session be started lazely (create it when first accessing it)

What do you think?

bellini666 commented 10 months ago

@mattalbr @erikwrede think I addressed all of the comments in this PR

Let me know if there's anything else missing to be able to merge this

bellini666 commented 10 months ago

lgtm from my side, pending agreement on this from you:

Thank you! @mattalbr let me know if you want any extra changes in this