-
Notifications
You must be signed in to change notification settings - Fork 75
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
Support SQLAlchemy 2.0 #84
base: master
Are you sure you want to change the base?
Conversation
b5de041
to
984d5d3
Compare
@tomviner FYI I rebased on the latest master to get the MariaDB pin to pass the GH workflows |
Thanks for your PR, and rebase. The tests for SQLA 1.4 are failing. Would you like to take a look at fixing them? |
@tomviner no problem I think I see the issue. Glad we’ve got good tests covering it! I can also take a look at how to add SQLAlchemy 2 to CI |
984d5d3
to
976b5a9
Compare
This property is removed in SQLAlchemy 2.x
… .gitignore This allows Tox to find all Python versions when using pyenv
drop_all() makes the 'bind' positional argument required
516ef7d
to
8328dd8
Compare
Specify compatible versions of SQLAlchemy and SQLAlchemy-Utils for Python, SQLAlchemy versions
fe6188f
to
a0dea62
Compare
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.
@tomviner sorry it took me a bit but I have a fix up!
I also included a lot of changes to Tox and how tests run in CI. Probably the most impactful change is that with all the modifications I made to tox.ini, we can remove the Tox environment compatibility from the GitHub workflow config. The result is that we now have SQLAlchemy 2 running in CI and an easy to way to run it locally!
There are still a few test failures around loads and joins in SQLAlchemy 2. It will likely take me another week to get some bandwidth to fix these additional tests. Hopefully there's a straightforward fix!
# sqlalchemylatest (i.e. > 2.0.0) is not yet supported | ||
# for any version of python | ||
|
||
- {python: '3.7', tox: "py37-sqlalchemy1.0"} | ||
- {python: '3.7', tox: "py37-sqlalchemy1.1"} | ||
- {python: '3.7', tox: "py37-sqlalchemy1.2"} | ||
- {python: '3.7', tox: "py37-sqlalchemy1.3"} | ||
- {python: '3.7', tox: "py37-sqlalchemy1.4"} | ||
|
||
- {python: '3.8', tox: "py38-sqlalchemy1.0"} | ||
- {python: '3.8', tox: "py38-sqlalchemy1.1"} | ||
- {python: '3.8', tox: "py38-sqlalchemy1.2"} | ||
- {python: '3.8', tox: "py38-sqlalchemy1.3"} | ||
- {python: '3.8', tox: "py38-sqlalchemy1.4"} | ||
|
||
- {python: '3.9', tox: "py39-sqlalchemy1.0"} | ||
- {python: '3.9', tox: "py39-sqlalchemy1.1"} | ||
- {python: '3.9', tox: "py39-sqlalchemy1.2"} | ||
- {python: '3.9', tox: "py39-sqlalchemy1.3"} | ||
- {python: '3.9', tox: "py39-sqlalchemy1.4"} | ||
|
||
# python3.10 with sqlalchemy <= 1.1 errors with: | ||
# AttributeError: module 'collections' has no attribute 'MutableMapping' | ||
- {python: '3.10', tox: "py310-sqlalchemy1.2"} | ||
- {python: '3.10', tox: "py310-sqlalchemy1.3"} | ||
- {python: '3.10', tox: "py310-sqlalchemy1.4"} |
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.
No longer necessary since this is now fully encoded in tox.ini.
3.7.17 | ||
3.8.18 | ||
3.9.18 | ||
3.10.13 | ||
3.11.7 | ||
3.12.1 |
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 locally use pyenv and thought this would be useful to specify. Happy to remove if this muddies the waters for tooling.
'mysql': ['mysql-connector-python-rf==2.2.2'], | ||
'postgresql': ['psycopg2==2.8.4'], | ||
'mysql': ['mysql-connector-python-rf>=2.2.2'], | ||
'postgresql': ['psycopg2>=2.8.4'], |
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 don't exactly recall the Python version that was building wheels. I didn't think the versions were all that important for validating proper execution. I can look into this deeper if necessary.
sqlalchemy_filters/models.py
Outdated
return orm_descriptor.extension_type == symbol('HYBRID_PROPERTY') | ||
# SQLAlchemy 2 treats extension_type as an enum, not a symbol(). Enum is at sqlalchemy.ext.hybrid.HybridExtensionType | ||
|
||
return str(orm_descriptor.extension_type) in ("symbol('HYBRID_PROPERTY')", 'HybridExtensionType.HYBRID_PROPERTY') |
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.
This is a little bit hacky but I was trying to optimize for performance. Other things I thought of:
- Use
isinstance()
to see iform_descriptor.extension_type
is an Enum and then use.name
to compare.isinstance()
can be really slow in a hot path, and this gets invoked a lot for all properties on queried models - Try/except import since
HybridExtensionType
only exists in 2+. Kind of obtuse, I thought the same "hot path" reason to exclude this one too
Happy to change for a different opinion on the right way to check this.
@@ -131,7 +131,7 @@ def connection(db_uri, db_engine_options, is_postgresql): | |||
|
|||
yield connection | |||
|
|||
Base.metadata.drop_all() | |||
Base.metadata.drop_all(engine) |
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.
SA 2.x compatibility
tox.ini
Outdated
@@ -10,11 +19,14 @@ extras = | |||
mysql | |||
postgresql | |||
deps = | |||
{py37,py38,py39,py310}: sqlalchemy-utils~=0.37.8 | |||
!sqlalchemy2,!sqlalchemylatest: sqlalchemy-utils~=0.37.8 |
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 really wanted !sqlalchemy{2,latest}
to work but it didn't. Alas.
4e5a3a1
to
526375f
Compare
Simplifies tox expressions and doesn't change anything fundamental about this library. 0.38.3 dropped support for Python 3.4 and 3.5 which sqlalchemy-filters never supported in the first place
Is there an ETA on this? |
@tomviner is there any work open that I can support with in order to release this? |
Any updates? |
I there anything I could help with in order to merge this ? |
Fixes #83.
I am going to be running these in production soon and I figured I would go ahead and contribute back.
My process was to take the fork from @bodik who did most of the heavy lifting (thanks @bodik!) and then applied fixes to other SQLAlchemy 2.0 issues. My 1 change so far fixes this exception:
Not exactly sure where to go from here, I haven't yet investigated the testing ecosystem of this library but probably we want to get SQLAlchemy 2.0 into the test suite.
@bodik @tomviner what do you think?