-
-
Notifications
You must be signed in to change notification settings - Fork 33
Add support to secondary tables relationships #218
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
base: main
Are you sure you want to change the base?
Changes from 23 commits
de06188
0cd732d
401cd65
6f644e3
ca9bc1c
eb852ce
5770379
be77996
fb6a580
0fb61bb
03a5438
a575650
beaa3f9
8a65328
9d76061
91c24c5
1cd8df4
e96f179
4b6516b
9b079d4
4baa7ae
33d7758
2a53474
d04af46
3f7f13d
ff3e419
6752231
0cd68d2
0745c64
a8c03a5
276ffd5
f3e2149
94ae7b1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| Release type: patch | ||
|
|
||
| Add support for secondary table relationships in SQLAlchemy mapper, addressing a bug and enhancing the loader to handle these relationships efficiently. |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -11,6 +11,7 @@ | |||||||||||||||||||||
| Tuple, | ||||||||||||||||||||||
| Union, | ||||||||||||||||||||||
| ) | ||||||||||||||||||||||
| from strawberry_sqlalchemy_mapper.exc import InvalidLocalRemotePairs | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| from sqlalchemy import select, tuple_ | ||||||||||||||||||||||
| from sqlalchemy.engine.base import Connection | ||||||||||||||||||||||
|
|
@@ -45,12 +46,16 @@ def __init__( | |||||||||||||||||||||
| "One of bind or async_bind_factory must be set for loader to function properly." | ||||||||||||||||||||||
| ) | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| async def _scalars_all(self, *args, **kwargs): | ||||||||||||||||||||||
| async def _scalars_all(self, *args, disabled_optimization_to_secondary_tables=False, **kwargs): | ||||||||||||||||||||||
| if self._async_bind_factory: | ||||||||||||||||||||||
| async with self._async_bind_factory() as bind: | ||||||||||||||||||||||
| if disabled_optimization_to_secondary_tables is True: | ||||||||||||||||||||||
|
||||||||||||||||||||||
| if disabled_optimization_to_secondary_tables is True: | |
| if disabled_optimization_to_secondary_tables: |
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.
Updated! Thank you
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.
updated!
Outdated
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.
nitpick:
| if disabled_optimization_to_secondary_tables is True: | |
| if disabled_optimization_to_secondary_tables: |
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.
Updated!
Outdated
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.
polish: for ruff/black this parenthesis should be closed in the next line. I think you forgot to pre-commit install =P (ditto for the lines below)
Also, we are probably missing a lint check in here which runs ruff/black/etc (and maybe migrate to ruff formatter instead of black soon)
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.
Sorry about that, I see now that pre-commit dont run due to some updates that dont work with dev container python version (3.8).
I updated that imports and now i'm fixing all the erros ;)
sourcery-ai[bot] marked this conversation as resolved.
Show resolved
Hide resolved
Outdated
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.
nitpick: total personal preference, so feel free to ignore. I feel like those 2 _buil functions could be defined in the module and query could be defined with the if directly, like query = _build_normal_relationship_query(related_model, relationship, keys) if relationship.secondary is None else _build_relationship_with_secondary_table_query(related_model, relationship, keys)
Even better to avoid *args in there as mypy/pyright can validate them
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.
Updated!
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.
suggestion:
| if relationship.secondary is not None: | |
| # We need to retrieve values from both the self_model and related_model. To achieve this, we must disable the default SQLAlchemy optimization that returns only related_model values. This is necessary because we use the keys variable to match both related_model and self_model. | |
| rows = await self._scalars_all(query, disabled_optimization_to_secondary_tables=True) | |
| else: | |
| rows = await self._scalars_all(query) | |
| # We need to retrieve values from both the self_model and related_model. | |
| # To achieve this, we must disable the default SQLAlchemy optimization | |
| # that returns only related_model values. This is necessary because we | |
| # use the keys variable to match both related_model and self_model. | |
| rows = await self._scalars_all(query, disabled_optimization_to_secondary_tables=relationship.secondary is not None) |
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.
Thanks! Updated!
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -82,6 +82,7 @@ | |||||||||||||||||
| from strawberry_sqlalchemy_mapper.exc import ( | ||||||||||||||||||
| HybridPropertyNotAnnotated, | ||||||||||||||||||
| InterfaceModelNotPolymorphic, | ||||||||||||||||||
| InvalidLocalRemotePairs, | ||||||||||||||||||
| UnsupportedAssociationProxyTarget, | ||||||||||||||||||
| UnsupportedColumnType, | ||||||||||||||||||
| UnsupportedDescriptorType, | ||||||||||||||||||
|
|
@@ -154,7 +155,8 @@ def from_type(cls, type_: type, *, strict: Literal[True]) -> Self: ... | |||||||||||||||||
|
|
||||||||||||||||||
| @overload | ||||||||||||||||||
| @classmethod | ||||||||||||||||||
| def from_type(cls, type_: type, *, strict: bool = False) -> Optional[Self]: ... | ||||||||||||||||||
| def from_type(cls, type_: type, *, | ||||||||||||||||||
| strict: bool = False) -> Optional[Self]: ... | ||||||||||||||||||
|
|
||||||||||||||||||
| @classmethod | ||||||||||||||||||
| def from_type( | ||||||||||||||||||
|
|
@@ -165,7 +167,8 @@ def from_type( | |||||||||||||||||
| ) -> Optional[Self]: | ||||||||||||||||||
| definition = getattr(type_, cls.TYPE_KEY_NAME, None) | ||||||||||||||||||
| if strict and definition is None: | ||||||||||||||||||
| raise TypeError(f"{type_!r} does not have a StrawberrySQLAlchemyType in it") | ||||||||||||||||||
| raise TypeError( | ||||||||||||||||||
| f"{type_!r} does not have a StrawberrySQLAlchemyType in it") | ||||||||||||||||||
| return definition | ||||||||||||||||||
|
|
||||||||||||||||||
|
|
||||||||||||||||||
|
|
@@ -228,11 +231,12 @@ class StrawberrySQLAlchemyMapper(Generic[BaseModelType]): | |||||||||||||||||
|
|
||||||||||||||||||
| def __init__( | ||||||||||||||||||
| self, | ||||||||||||||||||
| model_to_type_name: Optional[Callable[[Type[BaseModelType]], str]] = None, | ||||||||||||||||||
| model_to_interface_name: Optional[Callable[[Type[BaseModelType]], str]] = None, | ||||||||||||||||||
| extra_sqlalchemy_type_to_strawberry_type_map: Optional[ | ||||||||||||||||||
| Mapping[Type[TypeEngine], Type[Any]] | ||||||||||||||||||
| ] = None, | ||||||||||||||||||
| model_to_type_name: Optional[Callable[[ | ||||||||||||||||||
| Type[BaseModelType]], str]] = None, | ||||||||||||||||||
| model_to_interface_name: Optional[Callable[[ | ||||||||||||||||||
| Type[BaseModelType]], str]] = None, | ||||||||||||||||||
| extra_sqlalchemy_type_to_strawberry_type_map: Optional[Mapping[Type[TypeEngine], Type[Any]] | ||||||||||||||||||
| ] = None, | ||||||||||||||||||
| ) -> None: | ||||||||||||||||||
| if model_to_type_name is None: | ||||||||||||||||||
| model_to_type_name = self._default_model_to_type_name | ||||||||||||||||||
|
|
@@ -295,7 +299,8 @@ def _edge_type_for(self, type_name: str) -> Type[Any]: | |||||||||||||||||
| """ | ||||||||||||||||||
| edge_name = f"{type_name}Edge" | ||||||||||||||||||
| if edge_name not in self.edge_types: | ||||||||||||||||||
| lazy_type = StrawberrySQLAlchemyLazy(type_name=type_name, mapper=self) | ||||||||||||||||||
| lazy_type = StrawberrySQLAlchemyLazy( | ||||||||||||||||||
| type_name=type_name, mapper=self) | ||||||||||||||||||
| self.edge_types[edge_name] = edge_type = strawberry.type( | ||||||||||||||||||
| dataclasses.make_dataclass( | ||||||||||||||||||
| edge_name, | ||||||||||||||||||
|
|
@@ -314,14 +319,15 @@ def _connection_type_for(self, type_name: str) -> Type[Any]: | |||||||||||||||||
| connection_name = f"{type_name}Connection" | ||||||||||||||||||
| if connection_name not in self.connection_types: | ||||||||||||||||||
| edge_type = self._edge_type_for(type_name) | ||||||||||||||||||
| lazy_type = StrawberrySQLAlchemyLazy(type_name=type_name, mapper=self) | ||||||||||||||||||
| lazy_type = StrawberrySQLAlchemyLazy( | ||||||||||||||||||
| type_name=type_name, mapper=self) | ||||||||||||||||||
| self.connection_types[connection_name] = connection_type = strawberry.type( | ||||||||||||||||||
| dataclasses.make_dataclass( | ||||||||||||||||||
| connection_name, | ||||||||||||||||||
| [ | ||||||||||||||||||
| ("edges", List[edge_type]), # type: ignore[valid-type] | ||||||||||||||||||
| ], | ||||||||||||||||||
| bases=(relay.ListConnection[lazy_type],), # type: ignore[valid-type] | ||||||||||||||||||
| bases=(relay.ListConnection[lazy_type],), # type: ignore[valid-type] | ||||||||||||||||||
| ) | ||||||||||||||||||
| ) | ||||||||||||||||||
| setattr(connection_type, _GENERATED_FIELD_KEYS_KEY, ["edges"]) | ||||||||||||||||||
|
|
@@ -387,7 +393,7 @@ def _convert_relationship_to_strawberry_type( | |||||||||||||||||
| if relationship.uselist: | ||||||||||||||||||
| # Use list if excluding relay pagination | ||||||||||||||||||
| if use_list: | ||||||||||||||||||
| return List[ForwardRef(type_name)] # type: ignore | ||||||||||||||||||
| return List[ForwardRef(type_name)] # type: ignore | ||||||||||||||||||
|
|
||||||||||||||||||
| return self._connection_type_for(type_name) | ||||||||||||||||||
| else: | ||||||||||||||||||
|
|
@@ -451,7 +457,8 @@ def _get_association_proxy_annotation( | |||||||||||||||||
| strawberry_type.__forward_arg__ | ||||||||||||||||||
| ) | ||||||||||||||||||
| else: | ||||||||||||||||||
| strawberry_type = self._connection_type_for(strawberry_type.__name__) | ||||||||||||||||||
| strawberry_type = self._connection_type_for( | ||||||||||||||||||
| strawberry_type.__name__) | ||||||||||||||||||
| return strawberry_type | ||||||||||||||||||
|
|
||||||||||||||||||
| def make_connection_wrapper_resolver( | ||||||||||||||||||
|
|
@@ -500,13 +507,29 @@ async def resolve(self, info: Info): | |||||||||||||||||
| if relationship.key not in instance_state.unloaded: | ||||||||||||||||||
| related_objects = getattr(self, relationship.key) | ||||||||||||||||||
| else: | ||||||||||||||||||
| relationship_key = tuple( | ||||||||||||||||||
| [ | ||||||||||||||||||
| getattr(self, local.key) | ||||||||||||||||||
| for local, _ in relationship.local_remote_pairs or [] | ||||||||||||||||||
| if local.key | ||||||||||||||||||
| ] | ||||||||||||||||||
| ) | ||||||||||||||||||
| if relationship.secondary is None: | ||||||||||||||||||
| relationship_key = tuple( | ||||||||||||||||||
| [ | ||||||||||||||||||
| getattr(self, local.key) | ||||||||||||||||||
| for local, _ in relationship.local_remote_pairs or [] | ||||||||||||||||||
| if local.key | ||||||||||||||||||
| ] | ||||||||||||||||||
|
||||||||||||||||||
| [ | |
| getattr(self, local.key) | |
| for local, _ in relationship.local_remote_pairs or [] | |
| if local.key | |
| ] | |
| getattr(self, local.key) | |
| for local, _ in relationship.local_remote_pairs or [] | |
| if local.key |
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.
Updated!
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.
suggestion: ditto
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.
Updated!
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.
thought: maybe call this
enable_and haveTrueas the default?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 dont want to do this because it removes optimizations that we only need to remove when we need to pick up secondary tables values, so if the default is True we will lose peformance in queries that dont need it.
But I agree that this var name aren't good enought, so I will change the name to
query_secondary_tablesand refactor the function.