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

Enable sorting when batching is enabled #355

Merged
merged 7 commits into from
Sep 9, 2022
48 changes: 36 additions & 12 deletions graphene_sqlalchemy/batching.py
Original file line number Diff line number Diff line change
@@ -1,20 +1,28 @@
from asyncio import get_event_loop

import aiodataloader
import sqlalchemy
from sqlalchemy.orm import Session, strategies
from sqlalchemy.orm.query import QueryContext

from .utils import is_sqlalchemy_version_less_than

# Cache this across `batch_load_fn` calls
# This is so SQL string generation is cached under-the-hood via `bakery`
# Caching the relationship loader for each relationship prop.
RELATIONSHIP_LOADERS_CACHE = {}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We store the loaders per relationship so each item accesses the same cached result ensuring that the selectin is only triggered once


def get_batch_resolver(relationship_prop):

# Cache this across `batch_load_fn` calls
# This is so SQL string generation is cached under-the-hood via `bakery`
selectin_loader = strategies.SelectInLoader(relationship_prop, (('lazy', 'selectin'),))
def get_batch_resolver(relationship_prop):

class RelationshipLoader(aiodataloader.DataLoader):
cache = False

def __init__(self, relationship_prop, selectin_loader):
super().__init__()
self.relationship_prop = relationship_prop
self.selectin_loader = selectin_loader

async def batch_load_fn(self, parents):
"""
Batch loads the relationships of all the parents as one SQL statement.
Expand All @@ -38,8 +46,8 @@ async def batch_load_fn(self, parents):
SQLAlchemy's main maitainer suggestion.
See https://git.io/JewQ7
"""
child_mapper = relationship_prop.mapper
parent_mapper = relationship_prop.parent
child_mapper = self.relationship_prop.mapper
parent_mapper = self.relationship_prop.parent
session = Session.object_session(parents[0])

# These issues are very unlikely to happen in practice...
Expand All @@ -62,26 +70,42 @@ async def batch_load_fn(self, parents):
query_context = parent_mapper_query._compile_context()

if is_sqlalchemy_version_less_than('1.4'):
selectin_loader._load_for_path(
self.selectin_loader._load_for_path(
query_context,
parent_mapper._path_registry,
states,
None,
child_mapper
)
else:
selectin_loader._load_for_path(
self.selectin_loader._load_for_path(
query_context,
parent_mapper._path_registry,
states,
None,
child_mapper,
None
)

return [getattr(parent, relationship_prop.key) for parent in parents]

loader = RelationshipLoader()
return [getattr(parent, self.relationship_prop.key) for parent in parents]

def _get_loader(relationship_prop):
"""Retrieve the cached loader of the given relationship."""
loader = RELATIONSHIP_LOADERS_CACHE.get(relationship_prop, None)
if loader is None:
selectin_loader = strategies.SelectInLoader(
relationship_prop,
(('lazy', 'selectin'),)
)
loader = RelationshipLoader(
relationship_prop=relationship_prop,
selectin_loader=selectin_loader
)
RELATIONSHIP_LOADERS_CACHE[relationship_prop] = loader
else:
loader.loop = get_event_loop()
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not 100% sure about this. Do any of you see issues with this?

Copy link
Member

Choose a reason for hiding this comment

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

Why do we need the get_event_loop() call in the else part? Isn't that initially covered by aiodataloader already, and shouldn't the cache also get a full reset if the event loop changes (implying thread restart/change)?

aiodataloader recommends to create a new loader for each request, this would keep the loaders between different queries, right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Makse sense, I changed it

Copy link
Contributor

Choose a reason for hiding this comment

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

September update: There is also preliminary work in Graphene to vendor the DataLoader from aiodataloader into Graphene, and also change it's __init__ behavior to not ask for an even loop so early on init. Just warning because that might touch the same points we are touching here.

return loader

loader = _get_loader(relationship_prop)

async def resolve(root, info, **args):
return await loader.load(root)
Expand Down
27 changes: 19 additions & 8 deletions graphene_sqlalchemy/fields.py
Original file line number Diff line number Diff line change
Expand Up @@ -129,20 +129,31 @@ def get_query(cls, model, info, sort=None, **args):
return query


class BatchSQLAlchemyConnectionField(UnsortedSQLAlchemyConnectionField):
class BatchSQLAlchemyConnectionField(SQLAlchemyConnectionField):
erikwrede marked this conversation as resolved.
Show resolved Hide resolved
"""
This is currently experimental.
The API and behavior may change in future versions.
Use at your own risk.
"""

def wrap_resolve(self, parent_resolver):
return partial(
self.connection_resolver,
self.resolver,
get_nullable_type(self.type),
self.model,
)
@classmethod
def connection_resolver(cls, resolver, connection_type, model, root, info, **args):
if root is None:
resolved = resolver(root, info, **args)
on_resolve = partial(cls.resolve_connection, connection_type, model, info, args)
else:
relationship_prop = None
for relationship in root.__class__.__mapper__.relationships:
if relationship.mapper.class_ == model:
relationship_prop = relationship
break
resolved = get_batch_resolver(relationship_prop)(root, info, **args)
on_resolve = partial(cls.resolve_connection, connection_type, root, info, args)

if is_thenable(resolved):
return Promise.resolve(resolved).then(on_resolve)

return on_resolve(resolved)

@classmethod
def from_relationship(cls, relationship, registry, **field_kwargs):
Expand Down
18 changes: 18 additions & 0 deletions graphene_sqlalchemy/tests/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,24 @@ class Article(Base):
headline = Column(String(100))
pub_date = Column(Date())
reporter_id = Column(Integer(), ForeignKey("reporters.id"))
readers = relationship(
"Reader", secondary="articles_readers", back_populates="articles"
)


class Reader(Base):
__tablename__ = "readers"
id = Column(Integer(), primary_key=True)
name = Column(String(100))
articles = relationship(
"Article", secondary="articles_readers", back_populates="readers"
)


class ArticleReader(Base):
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Had to add some deeper nesting in the test data for more robust tests

Copy link
Collaborator

Choose a reason for hiding this comment

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

I added a Tags class in #357. Will see if there's a way to consolidate over there after this is merged.

__tablename__ = "articles_readers"
article_id = Column(Integer(), ForeignKey("articles.id"), primary_key=True)
reader_id = Column(Integer(), ForeignKey("readers.id"), primary_key=True)


class ReflectedEditor(type):
Expand Down
Loading