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

Backend for async SQLAlchemy ORM #121

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

Backend for async SQLAlchemy ORM #121

wants to merge 10 commits into from

Conversation

il-s
Copy link

@il-s il-s commented Nov 25, 2021

Hi, I liked the idea of ​​your project and needed to make a fastapi router that will work with SQLAlchemy in asynchronous mode.

Also this answers issue number #87

@vercel
Copy link

vercel bot commented Nov 25, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/flortz/fastapi-crudrouter/EAS4myHY5LcKjZqzoaV1XjS28kmP
✅ Preview: https://fastapi-crudrouter-git-fork-il-s-master-flortz.vercel.app

@michaeltoohig
Copy link

Wow, I was just working on this myself after abandoning ormar in favor of using async sqlalchemy but yours looks great and I hope it is merged quickly.

@awtkns
Copy link
Owner

awtkns commented Nov 30, 2021

Hi @il-s thanks for this! Sorry for the delay, I've been quite busy.

I am totally on board with adding support for this, however I have a couple questions:

  1. Instead of a brand new implementation, do you think it would be better to simply have async=true as an optional parameter?
  2. For this to be merged, the implementation will need to be added to the test suite. Did you need help with that?

Thanks again for this 🚀

@il-s
Copy link
Author

il-s commented Nov 30, 2021

@awtkns
ok i will try to implement option 1 (with async = true parameter)

@il-s
Copy link
Author

il-s commented Nov 30, 2021

for the test, I used db_func like so:

from sqlalchemy.ext.asyncio import AsyncSession, create_async_engine
from sqlalchemy.orm import declarative_base, sessionmaker

Base = declarative_base()

class AsyncDatabaseSession:
    def __init__(self):
        self._session = None
        self._engine = None
    def __getattr__(self, name):
        return getattr(self._session, name)
    async def init(self):
        self._engine = create_async_engine(
            "postgresql+asyncpg://postgres:postgres@localhost/postgres",
            echo=True,
        )

        self._session = sessionmaker(
            self._engine, expire_on_commit=True, class_=AsyncSession
        )()

main_session = AsyncDatabaseSession()

async def get_db():
    yield main_session._session
    await main_session._session.commit()

`

@il-s
Copy link
Author

il-s commented Dec 4, 2021

@awtkns, need review!

@awtkns
Copy link
Owner

awtkns commented Dec 5, 2021

HI @il-s, looking good!

I am hoping to get this in and released tomorrow! I just need to add it to the test framework and write some documentation for it 🚀

@il-s
Copy link
Author

il-s commented Jan 28, 2022

@awtkns
Any ideas on how to do a merge?

@JonasKs
Copy link

JonasKs commented Feb 9, 2022

Hi @awtkns , any status on this?

try:
(model,) = (
await db.execute(
select(self.db_model).where(self.db_model.id == item_id)
Copy link

Choose a reason for hiding this comment

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

Just fyi... my team noticed this doesn't work with PK's not named "id". I think this code might work better:

Suggested change
select(self.db_model).where(self.db_model.id == item_id)
select(self.db_model).where(getattr(self.db_model, self._pk) == item_id)

@vercel
Copy link

vercel bot commented Jan 11, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
fastapi-crudrouter ✅ Ready (Inspect) Visit Preview Jan 11, 2023 at 7:08AM (UTC)

Copy link

@VolDr VolDr left a comment

Choose a reason for hiding this comment

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

Any updates?

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.

6 participants