Towards compatibility with SQLAlchemy 2.x #485
Conversation
amotl
force-pushed
the
amo/doctests-refactoring
branch
2 times, most recently
from
6090d53 to
5dc0e04
Compare
amotl
force-pushed
the
amo/sa-mitigate-deprecation-warnings
branch
2 times, most recently
from
810c3df to
386e246
Compare
amotl
force-pushed
the
amo/sa-mitigate-deprecation-warnings
branch
from
386e246 to
7ef37c1
Compare
amotl
force-pushed
the
amo/sa-mitigate-deprecation-warnings
branch
from
1429658 to
a4454d5
Compare
amotl
force-pushed
the
amo/sa-mitigate-deprecation-warnings
branch
from
fe32a5e to
b177b1e
Compare
amotl
force-pushed
the
amo/sa-mitigate-deprecation-warnings
branch
2 times, most recently
from
be93172 to
639636f
Compare
|
|
||
| # SQLAlchemy 1.3 does not have the `exec_driver_sql` method. | ||
| if SA_VERSION < SA_1_4: | ||
| monkeypatch_add_exec_driver_sql() |
There was a problem hiding this comment.
Do we already know how much longer we intend to support SQLAlchemy 1.3? According to SQLAlchemy's version policy, 1.3 will become EOL as soon as 2.0 becomes stable. Given that maintaining compatibility with 1.3 here reaches a certain complexity, do we want to keep it much longer?
There was a problem hiding this comment.
Hi. The most recent discussion about EOL of SA13 can be found at sqlalchemy/sqlalchemy#8631 (comment). I think we will support it for a while, and I am happy that the changes have not been too much intrusive, other than needing a few lines in compat/api13.py.
We discussed different alternatives on maintenance of the code within this package with @seut, regarding version deprecations and such, but all other alternatives would need more efforts, especially if we would need to deprecate support for SA13 earlier, because this might mean we would have to keep a maintenance branch or such. See also GH-494.
There was a problem hiding this comment.
Nevertheless, I think it will not hurt to add a runtime warning for the next release, which will give users a notice that dropping support for SA13 will be around the corner.
There was a problem hiding this comment.
The 1.3 support is quite nicely encapsulated, indeed. Well done! For now (while 1.3 is still supported upstream) it's a good solution. Once 1.3 reached EOL, I would still vote for removing it shortly after to eliminate the need for monkey patching (which always feels a bit risky to me, especially in tests).
There was a problem hiding this comment.
+1 for removing 1.3 support. Given that crate-python is mostly stable (bugfixes are rare) we can point users of SQLAlchemy 1.3 to an older version of crate-python and make sure to update the version constraints for the next crate-python release. pip should then pick the right version, no?
Most notable changes: - Declarative becomes a first class API, so it is now located at `sqlalchemy.orm.declarative_base`. - The `Session` no longer supports "bound metadata" when it resolves the engine to be used for connectivity. This means that an Engine object **must** be passed to the constructor now. - `connection.execute` will now only accept SQL statements wrapped in `text()`. - When needing to run a driver-level SQL statement, the method `connection.exec_driver_sql` must be used. - Library-level "Autocommit" aka. "implicit commit" removed from both Core and ORM. - `select()` no longer accepts varied constructor arguments, only the "generative" style of `select()` will be supported. The list of columns / tables to select from should be passed positionally. - The `autoload` option to `sa.MetaData` has been deprecated. `autoload_with` takes place, it is already there and supported. https://docs.sqlalchemy.org/en/20/changelog/migration_20.html
Don't use `autoload_with` on table `mytable`, because it will never be persisted to a database, so it can not be inspected.
This reverts commit b20faba.
"Implicit" and "Connectionless" execution, and "bound metadata" have been removed beginning with SQLAlchemy 2.0 [1]. Earlier and contemporary versions of SQLAlchemy had the ability to associate an `Engine` with a `MetaData` object. This allowed a number of so-called "connectionless" execution patterns. That is no longer possible. Instead, the association with an `Engine` object has to be concluded differently. On this very spot, in the context of the `dict_test` test cases, the most easy fix was to move it to the invocation of the `compile()` method of the `selectable` instance, which is now returned by the `sqlalchemy.sql.*` primitives: expression = selectable.compile(bind=self.engine) This is needed, because otherwise, when not associating `Engine` with `MetaData` properly, `CrateDialect` would be bypassed, and the "paramstyle" [2] of SQLAlchemy's `DefaultDialect` would be used, which is actually "named" [3], as originally reflected per b20faba. This is probably wrong, because the CrateDB Python driver uses the "qmark" paramstyle [4]. [1] https://docs.sqlalchemy.org/en/20/changelog/migration_20.html#implicit-and-connectionless-execution-bound-metadata-removed [2] https://docs.sqlalchemy.org/en/20/core/engines.html#sqlalchemy.create_engine.params.paramstyle [3] https://github.com/sqlalchemy/sqlalchemy/blob/rel_2_0_0b4/lib/sqlalchemy/engine/default.py#L204 [4] https://github.com/crate/crate-python/blob/0.29.0/src/crate/client/__init__.py#L36
By binding the insert clause to an engine, use the "qmark" paramstyle again, instead of falling back to the "named" paramstyle of the default dialect.
Resolve import of `declarative_base`. For SA13, it is in `sqlalchemy.ext.declarative`, while it is in `sqlalchemy.orm` on more recent versions of SA.
Note that this is on a best-effort basis. SQLAlchemy 2.0 will be mostly compatible with most SQLAlchemy 1.3 code which is not using any of the deprecated functionalities, modulo a few aspects. > It is possible to have code that is compatible with 2.0 and 1.3 at the > same time, you would just need to ensure you use the subset of > features and APIs that are common to both. This patch adds a small compatibility layer, which activates on SA13 only, and monkey-patches two spots where anomalies have been spotted.
amotl
force-pushed
the
amo/sa-mitigate-deprecation-warnings
branch
from
639636f to
19dbf7d
Compare
About
Based on information from the migration guide referenced below, this patch brings the code base a bit closer to SQLAlchemy 2.0.
-- SQLAlchemy migration to 2.0 Step Two - Turn on RemovedIn20Warnings
Details
sqlalchemy.orm.declarative_base.Sessionno longer supports "bound metadata" when it resolves the engine to be used for connectivity. This means that an Engine object must be passed to the constructor now.connection.executewill now only accept SQL statements wrapped intext().connection.exec_driver_sqlmust be used.with engine.begin() as conn:.select()no longer accepts varied constructor arguments, only the "generative" style ofselect()will be supported. The list of columns / tables to select from should be passed positionally.autoloadoption tosa.MetaDatahas been deprecated.autoload_withtakes place, it is already there and supported.References
SQLALCHEMY_WARN_20=1sqlalchemy-cratedb#111Notes
df3d04c is a bit unrelated to the changes regarding SQLAlchemy. However, those improvements, finally resolving a few leftover socket leaks in the test suite, have been made while working on mitigating the SA warnings, so that's why it's here. I think it will not harm the review process too much.
Compatibility
With #497, this patch also retains compatibility with SQLAlchemy 1.3.