veekun / pokedex

more than you ever wanted to know about Pokémon
MIT License
1.44k stars 637 forks source link

test failures with SQLAlchemy >= 1.2 #236

Open magical opened 6 years ago

magical commented 6 years ago

The Travis build has been failing recently with some odd failures that seem to be related to SQLAlchemy.

Manual testing shows that the test suite works fine with SQLAlchemy 1.1.18 (the latest 1.1.x release) but starts to fail with SQLAlchemy 1.2.0. Something must have changed in the 1.2 release.

magical commented 6 years ago

It seems the _default_language_id bindparam isn't being properly overridden, so it is defaulting to 'dummy', which breaks things.

https://github.com/veekun/pokedex/blob/master/pokedex/db/multilang.py#L171

magical commented 6 years ago

Hmm, it only seems to happen with lazyloaded columns.

magical commented 6 years ago

Minimal(ish) test case reproducing the problem: https://gist.github.com/magical/c6fb868afc1c3144197edcd95a060faa

prints test / test with SQLA 1.1.18 prints None / test with SQLA 1.2.5

magical commented 6 years ago

Looks like this is related to baked queries: http://docs.sqlalchemy.org/en/latest/changelog/migration_12.html#baked-loading-now-the-default-for-lazy-loads

magical commented 6 years ago

The reason baked queries are broken is that they bypass the Query.__iter__ method (which is where we set _default_language_id) and call a lower-level method to execute the query.

https://github.com/zzzeek/sqlalchemy/blob/39444fd45f1e57bdfe99e5eb56c67b3aad4ad88e/lib/sqlalchemy/ext/baked.py#L346 https://github.com/veekun/pokedex/blob/c3f566b/pokedex/db/multilang.py#L210

magical commented 6 years ago

The wiki suggests using a MapperOption to propagate the bindparam value through lazy loads, but notes that this approach was broken in 1.0 through 1.2.5.

https://bitbucket.org/zzzeek/sqlalchemy/wiki/UsageRecipes/GlobalFilter

magical commented 5 years ago

Updated recipe: https://github.com/sqlalchemy/sqlalchemy/wiki/FilteredQuery