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

feat: support sqlalchemy select API #229

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

novag
Copy link
Contributor

@novag novag commented Jan 5, 2025

Support the sqlalchemy select API in KeysetConnections. This is especially important for the async interface since async sqlalchemy only supports the select API.

Description

The change is backwards compatible so it supports the old style query as well as the select API for synchronous sessions. For async sessions only the select API is supported since there is no sqlalchemy async query support.

Currently, this PR is blocked by the fact that the Select type returned by the select() function does not inherit from Iterable as the Query type id which make the linter fail. I'm not too familiar with the internals of strawberry yet, so I'd appreciate any feedback on how to best tackle this.

There's also still stuff to do to be fully asynchronous. In field.py there are two run_sync calls I did not look into yet. Also I have not checked whether StrawberrySQLAlchemyAsyncQuery is really still needed.

Examples:

@strawberry.type
class Query:
    @connection(
        KeysetConnection[Employee],
        sessionmaker=async_sessionmaker,
    )
    async def async_employees(self) -> Iterable[Employee]:
        return select(models.Employee).order_by(models.Employee.id)

    @connection(
        KeysetConnection[Employee],
        sessionmaker=sync_sessionmaker,
    )
    async def sync_employees(self) -> Iterable[Employee]:
        return return models.session.query(models.Employee).order_by(models.Employee.id)

Types of Changes

  • Core
  • Bugfix
  • New feature
  • Enhancement/optimization
  • Documentation

Issues Fixed or Closed by This PR

Checklist

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • I have tested the changes and verified that they work and don't break anything (as well as I can manage).

Summary by Sourcery

New Features:

  • Support the SQLAlchemy select API when resolving connections.

Copy link
Contributor

sourcery-ai bot commented Jan 5, 2025

Reviewer's Guide by Sourcery

This PR adds support for the SQLAlchemy select API in resolvers, enabling compatibility with asynchronous sessions and providing a more modern approach to querying. It maintains backward compatibility with the existing query style for synchronous sessions.

Sequence diagram for async resolver with select API

sequenceDiagram
    participant Client
    participant Resolver
    participant AsyncSession
    participant SQLAlchemy

    Client->>Resolver: Request data
    Resolver->>AsyncSession: Execute select query
    AsyncSession->>SQLAlchemy: select_page()
    SQLAlchemy-->>AsyncSession: Return results
    AsyncSession-->>Resolver: Return paginated data
    Resolver-->>Client: Return connection response
Loading

Class diagram for updated resolver types

classDiagram
    class KeysetConnection {
        +resolve_connection(nodes, info, before, after, first, last)
        +resolve_nodes(session, nodes)
        +resolve_nodes_async(session, nodes)
    }

    class StrawberrySQLAlchemyAsyncQuery {
        +session: AsyncSession
        +query: Callable[[], Select]
        +iterator: Iterator
        +limit: int
        +offset: int
        +__aiter__()
        +__anext__()
    }

    note for StrawberrySQLAlchemyAsyncQuery "Updated to support Select API"
    note for KeysetConnection "Modified to handle both Query and Select types"
Loading

State diagram for query resolution flow

stateDiagram-v2
    [*] --> CheckSessionType
    CheckSessionType --> AsyncSession: is async
    CheckSessionType --> SyncSession: is sync

    AsyncSession --> SelectAPI: must use select()
    SelectAPI --> ExecuteQuery

    SyncSession --> QueryChoice
    QueryChoice --> QueryAPI: use query()
    QueryChoice --> SelectAPI: use select()
    QueryAPI --> ExecuteQuery

    ExecuteQuery --> ReturnResults
    ReturnResults --> [*]
Loading

File-Level Changes

Change Details Files
Use select API instead of query when resolving
  • Updated the resolve_connection method to handle both Query and Select objects.
  • Removed the StrawberrySQLAlchemyAsyncQuery class as it's no longer needed.
  • Modified the resolve_nodes and resolve_nodes_async functions to work with the select API.
  • Updated type hints to reflect the changes from Query to Select.
  • Refactored the query building logic to use the select API.
  • Added logic to handle different session types (sync and async).
src/strawberry_sqlalchemy_mapper/relay.py
src/strawberry_sqlalchemy_mapper/field.py
Add release notes
  • Added a new release note indicating the addition of SQLAlchemy select API support.
  • Specified the release type as "minor".
RELEASE.md

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time. You can also use
    this command to specify where the summary should be inserted.

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

@botberry
Copy link
Member

botberry commented Jan 5, 2025

Thanks for adding the RELEASE.md file!

Here's a preview of the changelog:


Support SQLAlchemy select API when resolving.

Copy link
Contributor

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

Hey @novag - I've reviewed your changes - here's some feedback:

Overall Comments:

  • Please add tests to cover the new select API functionality, particularly focusing on async use cases
  • Documentation should be updated to explain the new select API usage and its differences from the query API
Here's what I looked at during the review
  • 🟡 General issues: 1 issue found
  • 🟢 Security: all looks good
  • 🟢 Testing: all looks good
  • 🟢 Complexity: all looks good
  • 🟢 Documentation: all looks good

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

RELEASE.md Outdated Show resolved Hide resolved
@novag novag marked this pull request as draft January 5, 2025 16:41
@novag novag marked this pull request as ready for review January 5, 2025 16:41
Copy link
Contributor

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

We have skipped reviewing this pull request. It looks like we've already reviewed the commit d611352 in this pull request.

@Ckk3 Ckk3 self-assigned this Jan 7, 2025
@Ckk3
Copy link
Contributor

Ckk3 commented Jan 9, 2025

Thanks for your contribution, @novag !
I dont review it yet but I see that unit tests action doesn't run as expected, I dont know why this happened but I'll add a new commit with a comment to see if it will run now.

@codecov-commenter
Copy link

codecov-commenter commented Jan 9, 2025

Codecov Report

Attention: Patch coverage is 77.77778% with 8 lines in your changes missing coverage. Please review.

Project coverage is 84.90%. Comparing base (2133fd7) to head (918ea6c).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #229      +/-   ##
==========================================
- Coverage   85.51%   84.90%   -0.62%     
==========================================
  Files          16       16              
  Lines        1629     1636       +7     
  Branches      139      147       +8     
==========================================
- Hits         1393     1389       -4     
  Misses        173      173              
- Partials       63       74      +11     

Copy link

codspeed-hq bot commented Jan 9, 2025

CodSpeed Performance Report

Merging #229 will not alter performance

Comparing novag:select (918ea6c) with main (2133fd7)

Summary

✅ 1 untouched benchmarks

@Ckk3
Copy link
Contributor

Ckk3 commented Jan 9, 2025

I see that some tests failed, including the one I added a TODO comment.
Please fix when you can @novag , also you can add new tests if you want.

@novag novag force-pushed the select branch 3 times, most recently from adec54d to cc36132 Compare January 10, 2025 09:06
@novag
Copy link
Contributor Author

novag commented Jan 10, 2025

I was unpacking the StrawberrySQLAlchemyAsyncQuery too late for an assert I introduced. Now the SQLAlchemy 1.4 test still fails because 1.4 did not have the Select API. Do we want to continue to support 1.4? If so, I would conditionally import the select type and create an empty dummy select type if the test fails under 1.4.

Before I go ahead and write more tests, I would also like to discuss the general approach regarding the return of the Select type. Would it perhaps make sense to force the user to always return a StrawberrySQLAlchemyAsyncQuery? Then we are able to return the correct iterable type, but it is a bit cumbersome.

Sorry for all the pushes, when fixing some warnings I created a linter error that I didn't realize because for some reason the lint check wasn't run locally via pre-commit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants