-
-
Notifications
You must be signed in to change notification settings - Fork 22
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: relationships loading #105
Conversation
add test case
This is a cool idea! You are on a roll this week! Is it possible to get the tests working for 3.8? |
from advanced_alchemy.repository._util import get_instrumented_attr, wrap_sqlalchemy_exception | ||
from advanced_alchemy.repository.typing import ModelT | ||
from advanced_alchemy.utils.deprecation import deprecated | ||
|
||
if TYPE_CHECKING: | ||
from collections import abc | ||
from datetime import datetime | ||
from typing import Self |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This may need to be typing_extensions
for us to get 3.8 support?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, just fixed it, but EllipsisType
does not seem to available before 3.10
@@ -169,6 +193,9 @@ async def add( | |||
""" | |||
with wrap_sqlalchemy_exception(): | |||
instance = await self._attach_to_session(data) | |||
if self._load: | |||
await self._flush_or_commit(auto_commit=True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the auto_commit
follow what was sent in from the method or actually be True
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it should stay True
since the following self._refresh_with_load()
emits a select to get back the newly inserted rows with loaded relationships.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just thinking through this a bit more, and it's still not totally clear.
We definitely need a flush so that the inserted row is loaded into relationship. As long as it isn't a new session though, we don't have to commit to make that happen. However, i'm not quite following why the commit is necessary.
Is there a simple use case that I can walk through to visualize why the commit over a flush is needed?
Actually, it comes from topsport-com-au/starlite-saqlalchemy#304, after some iterations ;) |
This is a great addition @gazorby! However, I have concerns about the interface. The pipeline style doesn't really fit in with the rest of the repositories, as it's the only place this would be used. Furthermore, the Another concern is the "blanket keyword argument" style, which isn't great for type checking (and testing), and is something that - in this case - SQLAlchemy would do better than we here. I would propose that for now, the interface for this stays functional, which could maybe look something like this: # List author and load all their books
authors = await AuthorRepo(session=session).get_one(name="J.R.R Tolkien", load=Author.books)
assert all(isinstance(book, BookModel) for book in author.books)
author = await AuthorRepo(session=session).get_one(
name="J.R.R Tolkien",
load=[Author.books, BookModel.publisher]
)
assert all(isinstance(book, BookModel) for book in author.books)
assert all(isinstance(book.publisher, PublisherModel) for book in author.books)
# use loaders from sqla
author = await AuthorRepo(session=session).get_one(
name="J.R.R Tolkien",
load=subqueryload(Author.books)
)
assert all(isinstance(book, BookModel) for book in author.books) |
Hi @gazorby I wasn't sure if you'd be continuing with this, so I'd made my own, admittedly lazier POC here #130 - I've just now noticed your recent activity. If we take for example the relationship of result = session.execute(
select(Author).options(selectinload(Author.books).selectinload(Book.chapters).load_only(Chapter.name)
) This gives us the opportunity to emit a more lightweight query in the context of the book hierarchy if the I also often make use of # find out about authors and only their books containing "foo" in the title
select(Author).options(selectinload(Author.books.any(Book.title.ilike("%foo%"))) The native approach makes a lot of sense to me here, let me know your thoughts? |
I refactored my codebase with a new implementation exposing the API @provinzkraut suggested, which I also agree with, and brings several benefits over my previous iteration, not least being fully typed and much lighter. So now I can use plain SQLAlchemy loaders or pass a nested list of relationships and let the repository generate the loaders: author_repo.get(id, load=Author.books)
author_repo.get(id, load=[(Author.books, Book.chapter), Author.publisher]
author_repo.get(id, load=selectinload(Author.books).selectinload(Book.chapters).load_only(Chapter.name)) Will update the PR when I have some time |
supplanted by #157 |
Pull Request Checklist
Description
Expose SQLAlchemy relationship loading techniques through the repository interface.
The idea is to have SQLAlchemy loading styles more integrated into the repository API, via a single
.load()
repository method that sets the relationships to load on the repository model.Here is the proposed API:
It also makes relationship loading more "composable" since
SQLAlchemyLoad
are standalone objects that can be reused.Close Issue(s)