Add support to secondary tables relationships #218
Conversation
Reviewer's Guide by SourceryThis PR implements support for secondary table relationships in SQLAlchemy by modifying the loader logic to handle both standard and secondary table relationships correctly. The implementation includes disabling SQLAlchemy's default optimization when dealing with secondary tables to ensure proper data retrieval from both self_model and related_model. ER Diagram for Secondary Table RelationshipserDiagram
EMPLOYEE {
INTEGER id PK
STRING name
STRING role
}
DEPARTMENT {
INTEGER id PK
STRING name
}
BUILDING {
INTEGER id PK
STRING name
}
EMPLOYEE ||--o{ DEPARTMENT : "belongs to"
EMPLOYEE ||--o{ BUILDING : "works in"
EMPLOYEE_DEPARTMENT_JOIN_TABLE {
INTEGER employee_id PK
INTEGER department_id PK
}
EMPLOYEE_BUILDING_JOIN_TABLE {
INTEGER employee_id PK
INTEGER building_id PK
}
EMPLOYEE ||--o{ EMPLOYEE_DEPARTMENT_JOIN_TABLE : ""
DEPARTMENT ||--o{ EMPLOYEE_DEPARTMENT_JOIN_TABLE : ""
EMPLOYEE ||--o{ EMPLOYEE_BUILDING_JOIN_TABLE : ""
BUILDING ||--o{ EMPLOYEE_BUILDING_JOIN_TABLE : ""
Class Diagram for SQLAlchemy ModelsclassDiagram
class Employee {
+Integer id
+String name
+String role
+Department department
+Building building
}
class Department {
+Integer id
+String name
+List~Employee~ employees
}
class Building {
+Integer id
+String name
+List~Employee~ employees
}
Employee --> Department : "secondary"
Employee --> Building : "secondary"
File-Level Changes
Assessment against linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #218 +/- ##
==========================================
+ Coverage 85.51% 87.27% +1.75%
==========================================
Files 16 17 +1
Lines 1629 2137 +508
Branches 139 152 +13
==========================================
+ Hits 1393 1865 +472
- Misses 173 203 +30
- Partials 63 69 +6 |
CodSpeed Performance ReportMerging #218 will not alter performanceComparing Summary
|
|
Thanks for adding the Here's a preview of the changelog: Add support for secondary table relationships in SQLAlchemy mapper, addressing a bug and enhancing the loader to handle these relationships efficiently. |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @Ckk3 - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟡 Testing: 1 issue found
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| @@ -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): | |||
There was a problem hiding this comment.
thought: maybe call this enable_ and have True as the default?
| if self._async_bind_factory: | ||
| async with self._async_bind_factory() as bind: | ||
| if disabled_optimization_to_secondary_tables is True: |
There was a problem hiding this comment.
nitpick:
| if disabled_optimization_to_secondary_tables is True: | |
| if disabled_optimization_to_secondary_tables: |
| return (await bind.scalars(*args, **kwargs)).all() | ||
| else: | ||
| assert self._bind is not None | ||
| if disabled_optimization_to_secondary_tables is True: |
There was a problem hiding this comment.
nitpick:
| if disabled_optimization_to_secondary_tables is True: | |
| if disabled_optimization_to_secondary_tables: |
|
|
||
| if not relationship.local_remote_pairs: | ||
| raise InvalidLocalRemotePairs( | ||
| f"{related_model.__name__} -- {self_model.__name__}") |
There was a problem hiding this comment.
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)
| def _build_query(*args): | ||
| return _build_normal_relationship_query(*args) if relationship.secondary is None else _build_relationship_with_secondary_table_query(*args) | ||
|
|
||
| query = _build_query(related_model, relationship, keys) |
There was a problem hiding this comment.
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
| 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) |
There was a problem hiding this comment.
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) |
| [ | ||
| getattr(self, local.key) | ||
| for local, _ in relationship.local_remote_pairs or [] | ||
| if local.key | ||
| ] |
There was a problem hiding this comment.
praise: you can pass the iterator to the tuple directly, no need to create a list for that
| [ | |
| 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, str(local_remote_pairs_secondary_table_local.key)), | ||
| ] |
|
|
||
|
|
||
| # TODO Investigate this test | ||
| @pytest.mark.skip("This test is currently failing because the Query with relay.ListConnection generates two DepartmentConnection, which violates the schema's expectations. After investigation, it appears this issue is related to the Relay implementation rather than the secondary table issue. We'll address this later. Additionally, note that the `result.data` may be incorrect in this test.") |
There was a problem hiding this comment.
thought: strange...
I know that this lib will generate automatic connections for relations, and that indeed could conflict with the departments you are defining. But you don't have a departments relation inside Employee
Maybe the secondary table is generating such connection with that name? If so, is that correct/expected?
Fixes #19
Description
This PR fixes the relationships involving secondary tables by primarily updating the logic in
loader.py.Notably, a new flag,
disabled_optimization_to_secondary_tables, has been introduced in theself._scalars_allfunction. This flag is required to disable SQLAlchemy's default optimization, which retrieves only therelated_modelvalues. In our case, this optimization must be disabled to retrieve values for both theself_modeland therelated_model.This adjustment is necessary because the
keysvariable is used to match data from both theself_modeland therelated_model. Please review the implementation of this flag and its impact on the overall query behavior.IMPORTANT
During development, I discovered an issue that causes the
ModelConnectiontype to be duplicated when we have relationship. After investigation, it seems this problem is related to the Relay implementation rather than the secondary table logic.To address this, I will create a separate issue. I’ve included a failing test in this PR,
test_query_with_secondary_table_with_values_list, which demonstrates the issue.Types of Changes
Issues Fixed or Closed by This PR
Checklist
Summary by Sourcery
Add support for secondary table relationships in the SQLAlchemy mapper, addressing a bug and enhancing the loader to handle these relationships efficiently. Include comprehensive tests to ensure the correct implementation of these features.
New Features:
Bug Fixes:
Enhancements:
Tests:
Summary by Sourcery
Add support for secondary table relationships in the SQLAlchemy mapper, addressing a bug and enhancing the loader to handle these relationships efficiently. Include comprehensive tests to ensure the correct implementation of these features.
New Features:
Bug Fixes:
Enhancements:
Tests: