Skip to content
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

Towards compatibility with SQLAlchemy 2.x #485

Merged
merged 12 commits into from
Dec 30, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 9 additions & 6 deletions .github/workflows/tests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,8 @@ jobs:
matrix:
os: ['ubuntu-latest', 'macos-latest']
python-version: ['3.7', '3.8', '3.9', '3.10', '3.11']
cratedb-version: ['5.1.1']
sqla-version: ['1.3.24', '1.4.44']
cratedb-version: ['5.1.2']
sqla-version: ['1.3.24', '1.4.45']
# To save resources, only use the most recent Python version on macOS.
exclude:
- os: 'macos-latest'
Expand All @@ -33,7 +33,7 @@ jobs:
python-version: '3.9'
- os: 'macos-latest'
python-version: '3.10'
fail-fast: true
fail-fast: false
env:
CRATEDB_VERSION: ${{ matrix.cratedb-version }}
SQLALCHEMY_VERSION: ${{ matrix.sqla-version }}
Expand All @@ -60,11 +60,14 @@ jobs:
# Report about the test matrix slot.
echo "Invoking tests with CrateDB ${CRATEDB_VERSION} and SQLAlchemy ${SQLALCHEMY_VERSION}"

# Invoke validation tasks.
# Run linter.
flake8 src bin
coverage run bin/test -vv1

# Run tests.
export SQLALCHEMY_WARN_20=1
coverage run bin/test -vvv

# Set the stage for the Codecov step.
# Set the stage for uploading the coverage report.
coverage xml

# https://github.com/codecov/codecov-action
Expand Down
4 changes: 2 additions & 2 deletions bootstrap.sh
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,8 @@

# Default variables.
BUILDOUT_VERSION=${BUILDOUT_VERSION:-2.13.7}
CRATEDB_VERSION=${CRATEDB_VERSION:-5.1.1}
SQLALCHEMY_VERSION=${SQLALCHEMY_VERSION:-1.4.44}
CRATEDB_VERSION=${CRATEDB_VERSION:-5.1.2}
SQLALCHEMY_VERSION=${SQLALCHEMY_VERSION:-1.4.45}


function print_header() {
Expand Down
11 changes: 7 additions & 4 deletions docs/by-example/sqlalchemy/advanced-querying.rst
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,11 @@ Introduction
Import the relevant symbols:

>>> import sqlalchemy as sa
>>> from sqlalchemy.ext.declarative import declarative_base
>>> from sqlalchemy.orm import sessionmaker
>>> try:
... from sqlalchemy.orm import declarative_base
... except ImportError:
... from sqlalchemy.ext.declarative import declarative_base
>>> from uuid import uuid4

Establish a connection to the database, see also :ref:`sa:engines_toplevel`
Expand Down Expand Up @@ -237,8 +240,8 @@ Let's add a task to the ``Todo`` table:
Now, let's use ``insert().from_select()`` to archive the task into the
``ArchivedTasks`` table:

>>> sel = select([Todos.id, Todos.content]).where(Todos.status == "done")
>>> ins = insert(ArchivedTasks).from_select(['id','content'], sel)
>>> sel = select(Todos.id, Todos.content).where(Todos.status == "done")
>>> ins = insert(ArchivedTasks).from_select(['id', 'content'], sel)
>>> result = session.execute(ins)
>>> session.commit()

Expand All @@ -250,7 +253,7 @@ This will emit the following ``INSERT`` statement to the database:
Now, verify that the data is present in the database:

>>> _ = connection.execute(sa.text("REFRESH TABLE archived_tasks"))
>>> pprint([str(r) for r in session.execute("SELECT content FROM archived_tasks")])
>>> pprint([str(r) for r in session.execute(sa.text("SELECT content FROM archived_tasks"))])
["('Write Tests',)"]


Expand Down
32 changes: 19 additions & 13 deletions docs/by-example/sqlalchemy/crud.rst
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,11 @@ Import the relevant symbols:
>>> import sqlalchemy as sa
>>> from datetime import datetime
>>> from sqlalchemy import delete, func, text
>>> from sqlalchemy.ext.declarative import declarative_base
>>> from sqlalchemy.orm import sessionmaker
>>> try:
... from sqlalchemy.orm import declarative_base
... except ImportError:
... from sqlalchemy.ext.declarative import declarative_base
>>> from crate.client.sqlalchemy.types import ObjectArray

Establish a connection to the database, see also :ref:`sa:engines_toplevel`
Expand All @@ -40,7 +43,7 @@ and :ref:`connect`:
Define the ORM schema for the ``Location`` entity using SQLAlchemy's
:ref:`sa:orm_declarative_mapping`:

>>> Base = declarative_base(bind=engine)
>>> Base = declarative_base()

>>> class Location(Base):
... __tablename__ = 'locations'
Expand Down Expand Up @@ -74,7 +77,7 @@ Insert a new location:

Refresh "locations" table:

>>> _ = connection.execute("REFRESH TABLE locations")
>>> _ = connection.execute(text("REFRESH TABLE locations"))

Inserted location is available:

Expand Down Expand Up @@ -175,7 +178,7 @@ The datetime and date can be set using an update statement:

Refresh "locations" table:

>>> _ = connection.execute("REFRESH TABLE locations")
>>> _ = connection.execute(text("REFRESH TABLE locations"))

Boolean values get set natively:

Expand All @@ -196,8 +199,9 @@ And verify that the date and datetime was persisted:

Update a record using SQL:

>>> result = connection.execute("update locations set kind='Heimat' where name='Earth'")
>>> result.rowcount
>>> with engine.begin() as conn:
... result = conn.execute(text("update locations set kind='Heimat' where name='Earth'"))
... result.rowcount
1

Update multiple records:
Expand All @@ -211,27 +215,29 @@ Update multiple records:

Refresh table:

>>> _ = connection.execute("REFRESH TABLE locations")
>>> _ = connection.execute(text("REFRESH TABLE locations"))

Update multiple records using SQL:

>>> result = connection.execute("update locations set flag=true where kind='Update'")
>>> result.rowcount
>>> with engine.begin() as conn:
... result = conn.execute(text("update locations set flag=true where kind='Update'"))
... result.rowcount
10

Update all records using SQL, and check that the number of documents affected
of an update without ``where-clause`` matches the number of all documents in
the table:

>>> result = connection.execute(u"update locations set kind='Überall'")
>>> result.rowcount == connection.execute("select * from locations limit 100").rowcount
>>> with engine.begin() as conn:
... result = conn.execute(text(u"update locations set kind='Überall'"))
... result.rowcount == conn.execute(text("select * from locations limit 100")).rowcount
True

>>> session.commit()

Refresh "locations" table:

>>> _ = connection.execute("REFRESH TABLE locations")
>>> _ = connection.execute(text("REFRESH TABLE locations"))

Objects can be used within lists, too:

Expand Down Expand Up @@ -282,7 +288,7 @@ Deleting a record with SQLAlchemy works like this.
>>> session.commit()
>>> session.flush()

>>> _ = connection.execute("REFRESH TABLE locations")
>>> _ = connection.execute(text("REFRESH TABLE locations"))

>>> session.query(Location).count()
23
Expand Down
5 changes: 4 additions & 1 deletion docs/by-example/sqlalchemy/getting-started.rst
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,11 @@ Introduction
Import the relevant symbols:

>>> import sqlalchemy as sa
>>> from sqlalchemy.ext.declarative import declarative_base
>>> from sqlalchemy.orm import sessionmaker
>>> try:
... from sqlalchemy.orm import declarative_base
... except ImportError:
... from sqlalchemy.ext.declarative import declarative_base

Establish a connection to the database, see also :ref:`sa:engines_toplevel`
and :ref:`connect`:
Expand Down
1 change: 0 additions & 1 deletion docs/by-example/sqlalchemy/inspection-reflection.rst
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,6 @@ Create a SQLAlchemy table object:
>>> meta = sa.MetaData()
>>> table = sa.Table(
... "characters", meta,
... autoload=True,
... autoload_with=engine)

Reflect column data types from the table metadata:
Expand Down
7 changes: 5 additions & 2 deletions docs/by-example/sqlalchemy/working-with-types.rst
Original file line number Diff line number Diff line change
Expand Up @@ -26,9 +26,12 @@ Import the relevant symbols:
>>> from datetime import datetime
>>> from geojson import Point, Polygon
>>> from sqlalchemy import delete, func, text
>>> from sqlalchemy.ext.declarative import declarative_base
>>> from sqlalchemy.orm import sessionmaker
>>> from sqlalchemy.sql import operators
>>> try:
... from sqlalchemy.orm import declarative_base
... except ImportError:
... from sqlalchemy.ext.declarative import declarative_base
>>> from uuid import uuid4
>>> from crate.client.sqlalchemy.types import Object, ObjectArray
>>> from crate.client.sqlalchemy.types import Geopoint, Geoshape
Expand Down Expand Up @@ -156,7 +159,7 @@ Update nested dictionary

Refresh and query "characters" table:

>>> _ = connection.execute("REFRESH TABLE characters")
>>> _ = connection.execute(text("REFRESH TABLE characters"))
>>> session.refresh(char_nested)

>>> char_nested = session.query(Character).filter_by(id='1234id').one()
Expand Down
6 changes: 6 additions & 0 deletions src/crate/client/sqlalchemy/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,13 @@
# with Crate these terms will supersede the license and you may use the
# software solely pursuant to the terms of the relevant commercial agreement.

from .compat.api13 import monkeypatch_add_exec_driver_sql
from .dialect import CrateDialect
from .sa_version import SA_1_4, SA_VERSION

# SQLAlchemy 1.3 does not have the `exec_driver_sql` method.
if SA_VERSION < SA_1_4:
monkeypatch_add_exec_driver_sql()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree. See also GH-501.


__all__ = [
CrateDialect,
Expand Down
Empty file.
133 changes: 133 additions & 0 deletions src/crate/client/sqlalchemy/compat/api13.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,133 @@
# -*- coding: utf-8; -*-
#
# Licensed to CRATE Technology GmbH ("Crate") under one or more contributor
# license agreements. See the NOTICE file distributed with this work for
# additional information regarding copyright ownership. Crate licenses
# this file to you under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License. You may
# obtain a copy of the License at
#
# https://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
# License for the specific language governing permissions and limitations
# under the License.
#
# However, if you have executed another commercial license agreement
# with Crate these terms will supersede the license and you may use the
# software solely pursuant to the terms of the relevant commercial agreement.

"""
Compatibility module for running a subset of SQLAlchemy 2.0 programs on
SQLAlchemy 1.3. By using monkey-patching, it can do two things:
1. Add the `exec_driver_sql` method to SA's `Connection` and `Engine`.
2. Amend the `sql.select` function to accept the calling semantics of
the modern variant.
Reason: `exec_driver_sql` gets used within the CrateDB dialect already,
and the new calling semantics of `sql.select` already get used within
many of the test cases already. Please note that the patch for
`sql.select` is only applied when running the test suite.
"""

import collections.abc as collections_abc

from sqlalchemy import exc
from sqlalchemy.sql import Select
from sqlalchemy.sql import select as original_select
from sqlalchemy.util import immutabledict


# `_distill_params_20` copied from SA14's `sqlalchemy.engine.{base,util}`.
_no_tuple = ()
_no_kw = immutabledict()


def _distill_params_20(params):
if params is None:
return _no_tuple, _no_kw
elif isinstance(params, list):
# collections_abc.MutableSequence): # avoid abc.__instancecheck__
if params and not isinstance(params[0], (collections_abc.Mapping, tuple)):
raise exc.ArgumentError(
"List argument must consist only of tuples or dictionaries"
)

return (params,), _no_kw
elif isinstance(
params,
(tuple, dict, immutabledict),
# only do abc.__instancecheck__ for Mapping after we've checked
# for plain dictionaries and would otherwise raise
) or isinstance(params, collections_abc.Mapping):
return (params,), _no_kw
else:
raise exc.ArgumentError("mapping or sequence expected for parameters")


def exec_driver_sql(self, statement, parameters=None, execution_options=None):
"""
Adapter for `exec_driver_sql`, which is available since SA14, for SA13.
"""
if execution_options is not None:
raise ValueError(
"SA13 backward-compatibility: "
"`exec_driver_sql` does not support `execution_options`"
)
args_10style, kwargs_10style = _distill_params_20(parameters)
return self.execute(statement, *args_10style, **kwargs_10style)


def monkeypatch_add_exec_driver_sql():
"""
Transparently add SA14's `exec_driver_sql()` method to SA13.
AttributeError: 'Connection' object has no attribute 'exec_driver_sql'
AttributeError: 'Engine' object has no attribute 'exec_driver_sql'
"""
from sqlalchemy.engine.base import Connection, Engine

# Add `exec_driver_sql` method to SA's `Connection` and `Engine` classes.
Connection.exec_driver_sql = exec_driver_sql
Engine.exec_driver_sql = exec_driver_sql


def select_sa14(*columns, **kw) -> Select:
"""
Adapt SA14/SA20's calling semantics of `sql.select()` to SA13.
With SA20, `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.
Derived from https://github.com/sqlalchemy/alembic/blob/b1fad6b6/alembic/util/sqla_compat.py#L557-L558
sqlalchemy.exc.ArgumentError: columns argument to select() must be a Python list or other iterable
"""
if isinstance(columns, tuple) and isinstance(columns[0], list):
if "whereclause" in kw:
raise ValueError(
"SA13 backward-compatibility: "
"`whereclause` is both in kwargs and columns tuple"
)
columns, whereclause = columns
kw["whereclause"] = whereclause
return original_select(columns, **kw)


def monkeypatch_amend_select_sa14():
"""
Make SA13's `sql.select()` transparently accept calling semantics of SA14
and SA20, by swapping in the newer variant of `select_sa14()`.
This supports the test suite of `crate-python`, because it already uses the
modern calling semantics.
"""
import sqlalchemy

sqlalchemy.select = select_sa14
sqlalchemy.sql.select = select_sa14
sqlalchemy.sql.expression.select = select_sa14
Loading