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

[mattalbr]

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]

Checks in the test that demonstrates #19

[patrick91]

didn't know about this 😊

I wonder if it might best to use github actions for that? or it is better to use this since it does work locally too?

[mattalbr]

At my company we run it locally with no problem, and isn't too heavy weight (test setup takes on the order of 1s)! Just need to have postgres installed on the device, which I'll add to our devcontainer once we have a devcontainer. testing.postgresql is basically just a convenient wrapper around postgres to make it easy (and performant) to use in tests. We also then use github actions to run a testing docker container, which of course has postgres installed.

Speaking of github actions, it appears none ran for this PR, so my money is on this library not having any CI/CD setup yet. It looks like strawberry core uses devcontainer and github actions, which is awesome because my company does the same so I'm familiar with both. In the next PR, I'm going to try to just copy over .devcontainer and .github from strawberiy-grpahql/strawberry, some config files from the root directory, make a few tweaks, and see if that'll get us CI/CD for ~free

[patrick91]

ok! sounds good to me 😊

the strawberry devcontainer might not be up-to-date, I don't really use it personally :)

also we did migrate all our tests to nox, which makes it easier to test multiple dependencies 😊

[erikwrede]

Great first steps! 🚀
I believe in the long run we should really leverage SQLAlchemy's internals for relationship loading. All the edge cases are already covered there, and it will be less effort to adjust in the future. For reference:
https://github.com/graphql-python/graphene-sqlalchemy/blob/master/graphene_sqlalchemy/batching.py

[erikwrede]

No, we need a parametrized conftest that dynamically creates session and engine based on SQLAlchemy version.

[mattalbr]

Done

[erikwrede]

Can we make this fixture driver-agnostic? I'd prefer to have it so we can easily add sqlite and mysql. Especially sqlite is important for quickly running the tests. Postgres should not be required to develop locally on this library, but we should just provide the Postgres Pipeline in the repo.

[erikwrede]

/cc @patrick91 do you think we can get all the botberry pre release stuff in here without much effort?

[mattalbr]

What do you think of this @erikwrede? engine is now parameterized on DB type, and dynamically requests the appropriate fixture. For now, it's just postgresql but sqlite should be an easy add.

In the future, we might even be able to skip postgresql tests if postgresql isn't installed.

If you had a different idea about how to do this, I'm open!

[mattalbr]

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

[botberry]

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]

Welcome to Codecov 🎉

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 ☂️

[codspeed-hq]

CodSpeed Performance Report

Merging #20 will not alter performance

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

Summary

✅ 1 untouched benchmarks