Skip to content

Commit 6417061

Browse files
PaulSchweizerPaulSchweizer
committed
Deprecate UnsortedSQLAlchemyConnectionField and resetting RelationshipLoader between queries
1 parent ac57fd4 commit 6417061

File tree

2 files changed

+130
-119
lines changed

2 files changed

+130
-119
lines changed

‎graphene_sqlalchemy/batching.py

Lines changed: 86 additions & 80 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,6 @@
1+
"""The dataloader uses "select in loading" strategy to load related entities."""
12
from asyncio import get_event_loop
3+
from typing import Dict
24

35
import aiodataloader
46
import sqlalchemy
@@ -7,102 +9,106 @@
79

810
from .utils import is_sqlalchemy_version_less_than
911

12+
13+
class RelationshipLoader(aiodataloader.DataLoader):
14+
cache = False
15+
16+
def __init__(self, relationship_prop, selectin_loader):
17+
super().__init__()
18+
self.relationship_prop = relationship_prop
19+
self.selectin_loader = selectin_loader
20+
21+
async def batch_load_fn(self, parents):
22+
"""
23+
Batch loads the relationships of all the parents as one SQL statement.
24+
25+
There is no way to do this out-of-the-box with SQLAlchemy but
26+
we can piggyback on some internal APIs of the `selectin`
27+
eager loading strategy. It's a bit hacky but it's preferable
28+
than re-implementing and maintainnig a big chunk of the `selectin`
29+
loader logic ourselves.
30+
31+
The approach here is to build a regular query that
32+
selects the parent and `selectin` load the relationship.
33+
But instead of having the query emits 2 `SELECT` statements
34+
when callling `all()`, we skip the first `SELECT` statement
35+
and jump right before the `selectin` loader is called.
36+
To accomplish this, we have to construct objects that are
37+
normally built in the first part of the query in order
38+
to call directly `SelectInLoader._load_for_path`.
39+
40+
TODO Move this logic to a util in the SQLAlchemy repo as per
41+
SQLAlchemy's main maitainer suggestion.
42+
See https://git.io/JewQ7
43+
"""
44+
child_mapper = self.relationship_prop.mapper
45+
parent_mapper = self.relationship_prop.parent
46+
session = Session.object_session(parents[0])
47+
48+
# These issues are very unlikely to happen in practice...
49+
for parent in parents:
50+
# assert parent.__mapper__ is parent_mapper
51+
# All instances must share the same session
52+
assert session is Session.object_session(parent)
53+
# The behavior of `selectin` is undefined if the parent is dirty
54+
assert parent not in session.dirty
55+
56+
# Should the boolean be set to False? Does it matter for our purposes?
57+
states = [(sqlalchemy.inspect(parent), True) for parent in parents]
58+
59+
# For our purposes, the query_context will only used to get the session
60+
query_context = None
61+
if is_sqlalchemy_version_less_than('1.4'):
62+
query_context = QueryContext(session.query(parent_mapper.entity))
63+
else:
64+
parent_mapper_query = session.query(parent_mapper.entity)
65+
query_context = parent_mapper_query._compile_context()
66+
67+
if is_sqlalchemy_version_less_than('1.4'):
68+
self.selectin_loader._load_for_path(
69+
query_context,
70+
parent_mapper._path_registry,
71+
states,
72+
None,
73+
child_mapper,
74+
)
75+
else:
76+
self.selectin_loader._load_for_path(
77+
query_context,
78+
parent_mapper._path_registry,
79+
states,
80+
None,
81+
child_mapper,
82+
None,
83+
)
84+
return [
85+
getattr(parent, self.relationship_prop.key) for parent in parents
86+
]
87+
88+
1089
# Cache this across `batch_load_fn` calls
1190
# This is so SQL string generation is cached under-the-hood via `bakery`
1291
# Caching the relationship loader for each relationship prop.
13-
RELATIONSHIP_LOADERS_CACHE = {}
92+
RELATIONSHIP_LOADERS_CACHE: Dict[
93+
sqlalchemy.orm.relationships.RelationshipProperty, RelationshipLoader
94+
] = {}
1495

1596

1697
def get_batch_resolver(relationship_prop):
17-
18-
class RelationshipLoader(aiodataloader.DataLoader):
19-
cache = False
20-
21-
def __init__(self, relationship_prop, selectin_loader):
22-
super().__init__()
23-
self.relationship_prop = relationship_prop
24-
self.selectin_loader = selectin_loader
25-
26-
async def batch_load_fn(self, parents):
27-
"""
28-
Batch loads the relationships of all the parents as one SQL statement.
29-
30-
There is no way to do this out-of-the-box with SQLAlchemy but
31-
we can piggyback on some internal APIs of the `selectin`
32-
eager loading strategy. It's a bit hacky but it's preferable
33-
than re-implementing and maintainnig a big chunk of the `selectin`
34-
loader logic ourselves.
35-
36-
The approach here is to build a regular query that
37-
selects the parent and `selectin` load the relationship.
38-
But instead of having the query emits 2 `SELECT` statements
39-
when callling `all()`, we skip the first `SELECT` statement
40-
and jump right before the `selectin` loader is called.
41-
To accomplish this, we have to construct objects that are
42-
normally built in the first part of the query in order
43-
to call directly `SelectInLoader._load_for_path`.
44-
45-
TODO Move this logic to a util in the SQLAlchemy repo as per
46-
SQLAlchemy's main maitainer suggestion.
47-
See https://git.io/JewQ7
48-
"""
49-
child_mapper = self.relationship_prop.mapper
50-
parent_mapper = self.relationship_prop.parent
51-
session = Session.object_session(parents[0])
52-
53-
# These issues are very unlikely to happen in practice...
54-
for parent in parents:
55-
# assert parent.__mapper__ is parent_mapper
56-
# All instances must share the same session
57-
assert session is Session.object_session(parent)
58-
# The behavior of `selectin` is undefined if the parent is dirty
59-
assert parent not in session.dirty
60-
61-
# Should the boolean be set to False? Does it matter for our purposes?
62-
states = [(sqlalchemy.inspect(parent), True) for parent in parents]
63-
64-
# For our purposes, the query_context will only used to get the session
65-
query_context = None
66-
if is_sqlalchemy_version_less_than('1.4'):
67-
query_context = QueryContext(session.query(parent_mapper.entity))
68-
else:
69-
parent_mapper_query = session.query(parent_mapper.entity)
70-
query_context = parent_mapper_query._compile_context()
71-
72-
if is_sqlalchemy_version_less_than('1.4'):
73-
self.selectin_loader._load_for_path(
74-
query_context,
75-
parent_mapper._path_registry,
76-
states,
77-
None,
78-
child_mapper
79-
)
80-
else:
81-
self.selectin_loader._load_for_path(
82-
query_context,
83-
parent_mapper._path_registry,
84-
states,
85-
None,
86-
child_mapper,
87-
None
88-
)
89-
return [getattr(parent, self.relationship_prop.key) for parent in parents]
98+
"""get the resolve function for the given relationship."""
9099

91100
def _get_loader(relationship_prop):
92101
"""Retrieve the cached loader of the given relationship."""
93102
loader = RELATIONSHIP_LOADERS_CACHE.get(relationship_prop, None)
94-
if loader is None:
103+
if loader is None or loader.loop != get_event_loop():
95104
selectin_loader = strategies.SelectInLoader(
96-
relationship_prop,
97-
(('lazy', 'selectin'),)
105+
relationship_prop, (('lazy', 'selectin'),)
98106
)
99107
loader = RelationshipLoader(
100108
relationship_prop=relationship_prop,
101-
selectin_loader=selectin_loader
109+
selectin_loader=selectin_loader,
102110
)
103111
RELATIONSHIP_LOADERS_CACHE[relationship_prop] = loader
104-
else:
105-
loader.loop = get_event_loop()
106112
return loader
107113

108114
loader = _get_loader(relationship_prop)

‎graphene_sqlalchemy/fields.py

Lines changed: 44 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@
1414
from .utils import EnumValue, get_query
1515

1616

17-
class UnsortedSQLAlchemyConnectionField(ConnectionField):
17+
class SQLAlchemyConnectionField(ConnectionField):
1818
@property
1919
def type(self):
2020
from .types import SQLAlchemyObjectType
@@ -37,13 +37,45 @@ def type(self):
3737
)
3838
return nullable_type.connection
3939

40+
def __init__(self, type_, *args, **kwargs):
41+
nullable_type = get_nullable_type(type_)
42+
if "sort" not in kwargs and nullable_type and issubclass(nullable_type, Connection):
43+
# Let super class raise if type is not a Connection
44+
try:
45+
kwargs.setdefault("sort", nullable_type.Edge.node._type.sort_argument())
46+
except (AttributeError, TypeError):
47+
raise TypeError(
48+
'Cannot create sort argument for {}. A model is required. Set the "sort" argument'
49+
" to None to disabling the creation of the sort query argument".format(
50+
nullable_type.__name__
51+
)
52+
)
53+
elif "sort" in kwargs and kwargs["sort"] is None:
54+
del kwargs["sort"]
55+
super(SQLAlchemyConnectionField, self).__init__(type_, *args, **kwargs)
56+
4057
@property
4158
def model(self):
4259
return get_nullable_type(self.type)._meta.node._meta.model
4360

4461
@classmethod
45-
def get_query(cls, model, info, **args):
46-
return get_query(model, info.context)
62+
def get_query(cls, model, info, sort=None, **args):
63+
query = get_query(model, info.context)
64+
if sort is not None:
65+
if not isinstance(sort, list):
66+
sort = [sort]
67+
sort_args = []
68+
# ensure consistent handling of graphene Enums, enum values and
69+
# plain strings
70+
for item in sort:
71+
if isinstance(item, enum.Enum):
72+
sort_args.append(item.value.value)
73+
elif isinstance(item, EnumValue):
74+
sort_args.append(item.value)
75+
else:
76+
sort_args.append(item)
77+
query = query.order_by(*sort_args)
78+
return query
4779

4880
@classmethod
4981
def resolve_connection(cls, connection_type, model, info, args, resolved):
@@ -90,43 +122,16 @@ def wrap_resolve(self, parent_resolver):
90122
)
91123

92124

93-
# TODO Rename this to SortableSQLAlchemyConnectionField
94-
class SQLAlchemyConnectionField(UnsortedSQLAlchemyConnectionField):
125+
# TODO Remove in next major version
126+
class UnsortedSQLAlchemyConnectionField(SQLAlchemyConnectionField):
95127
def __init__(self, type_, *args, **kwargs):
96-
nullable_type = get_nullable_type(type_)
97-
if "sort" not in kwargs and issubclass(nullable_type, Connection):
98-
# Let super class raise if type is not a Connection
99-
try:
100-
kwargs.setdefault("sort", nullable_type.Edge.node._type.sort_argument())
101-
except (AttributeError, TypeError):
102-
raise TypeError(
103-
'Cannot create sort argument for {}. A model is required. Set the "sort" argument'
104-
" to None to disabling the creation of the sort query argument".format(
105-
nullable_type.__name__
106-
)
107-
)
108-
elif "sort" in kwargs and kwargs["sort"] is None:
109-
del kwargs["sort"]
110-
super(SQLAlchemyConnectionField, self).__init__(type_, *args, **kwargs)
111-
112-
@classmethod
113-
def get_query(cls, model, info, sort=None, **args):
114-
query = get_query(model, info.context)
115-
if sort is not None:
116-
if not isinstance(sort, list):
117-
sort = [sort]
118-
sort_args = []
119-
# ensure consistent handling of graphene Enums, enum values and
120-
# plain strings
121-
for item in sort:
122-
if isinstance(item, enum.Enum):
123-
sort_args.append(item.value.value)
124-
elif isinstance(item, EnumValue):
125-
sort_args.append(item.value)
126-
else:
127-
sort_args.append(item)
128-
query = query.order_by(*sort_args)
129-
return query
128+
super(UnsortedSQLAlchemyConnectionField, self).__init__(type_, *args, **kwargs)
129+
warnings.warn(
130+
"UnsortedSQLAlchemyConnectionField is deprecated and will be removed in the next "
131+
"major version. Use SQLAlchemyConnectionField instead and set `sort = None` "
132+
"if you want to disable sorting.",
133+
DeprecationWarning,
134+
)
130135

131136

132137
class BatchSQLAlchemyConnectionField(SQLAlchemyConnectionField):

0 commit comments

Comments
 (0)