strawberry-graphql / strawberry-sqlalchemy

A SQLAlchemy Integration for strawberry-graphql
MIT License
91 stars 26 forks source link

Enable integration testing with postgres and check in failing test with secondary relationship #20

Closed mattalbr closed 1 year ago

mattalbr commented 1 year ago

I tried reasonably hard (~1 day) to fix the actual bug, but I don't understand sqlalchemy's relationship inspection well enough.

The local remote pairs on the Employee.departments.property is:

[(Column('e_id', Integer(), table=<employee>, primary_key=True, nullable=False), Column('employee_id', Integer(), ForeignKey('employee.e_id'), table=<employee_department_join_table>, primary_key=True, nullable=False)), (Column('d_id', Integer(), table=<department>, primary_key=True, nullable=False), Column('department_id', Integer(), ForeignKey('department.d_id'), table=<employee_department_join_table>, primary_key=True, nullable=False))]

So our strategy of filtering on in_(tuple([getattr(self, local.key) for local, _ in local_remote_pairs])) falls through.

local_columns more helpfully shows just e_id, so maybe there's a way we can join on the local entity and filter on that.

I think if we instead passed the key as only local_columns and changed the query to:

related_model = relationship.entity.entity
self_model = relationship.parent.entity
query = (
    select(
        related_model, self_model
    )
    .join(relationship.secondary, relationship.secondaryjoin)
    .join(self_model, relationship.primaryjoin)
    .filter(tuple_(*[column for column in relationship.local_columns]).in_(keys))
)

and changed our grouping to be on getattr(row[1], local.key)

it should work, but I'm not sure if there are corner cases with composite foreign keys, which my team doesn't use.

mattalbr commented 1 year ago

Checks in the test that demonstrates #19

mattalbr commented 1 year ago

Haven't lost track of this just got busy at work! Looking to pick it back up next week, thanks for the comments.

botberry commented 1 year ago

Hi, thanks for contributing to Strawberry 🍓!

We noticed that this PR is missing a RELEASE.md file. We use that to automatically do releases here on GitHub and, most importantly, to PyPI!

So as soon as this PR is merged, a release will be made 🚀.

Here's an example of RELEASE.md:

Release type: patch

Description of the changes, ideally with some examples, if adding a new feature.

Release type can be one of patch, minor or major. We use semver, so make sure to pick the appropriate type. If in doubt feel free to ask :)

codecov-commenter commented 1 year ago

Welcome to Codecov :tada:

Once merged to your default branch, Codecov will compare your coverage reports and display the results in this comment.

Thanks for integrating Codecov - We've got you covered :open_umbrella:

codspeed-hq[bot] commented 1 year ago

CodSpeed Performance Report

Merging #20 will not alter performance

Comparing mattalbr:secondary (2aabbfe) with main (8341ab5)

Summary

✅ 1 untouched benchmarks