Repository: crate/crate-python

Issue:
SQLAlchemy: Ignore `FOR UPDATE` clauses
## About

At https://github.com/crate-workbench/mlflow-cratedb/issues/7, we discovered that CrateDB does not support `SELECT ... FOR UPDATE` statements.

## Details

mlflow-cratedb [has a monkeypatch](https://github.com/crate-workbench/mlflow-cratedb/blob/v0.1.1/mlflow_cratedb/patch/crate_python.py#L31-L45), which adds a visitor to the CrateDB SQLAlchemy dialect to just omit that clause.

## Proposal

The monkeypatch needs to be converted into a real patch, might be supported by a corresponding software tests to verify the `FOR UPDATE` clause does not get rendered into the SQL statement, and a bit of documentation may also be added - one sentence is probably enough.

## Followup

Determine if this is an applicable candidate to raise on the crate/crate issue tracker.


Discussion:
amotl: Dear @surister,

after bringing in GH-582 and GH-583, I am going to wrap this up, if you don't have any objections. Users are waiting for another release, and my intention was only to bring in a few small maintenance updates and little improvements like this.

Your contribution was a perfect fit for that, thank you again.

With kind regards,
Andreas.

Patch:
diff --git a/CHANGES.txt b/CHANGES.txt
index 808430c..5947185 100644
--- a/CHANGES.txt
+++ b/CHANGES.txt
@@ -17,6 +17,7 @@ Unreleased
   constraints
 - DBAPI: Properly raise ``IntegrityError`` exceptions instead of
   ``ProgrammingError``, when CrateDB raises a ``DuplicateKeyException``.
+- SQLAlchemy: Ignore SQL's ``FOR UPDATE`` clause. Thanks, @surister.
 
 .. _urllib3 v2.0 migration guide: https://urllib3.readthedocs.io/en/latest/v2-migration-guide.html
 .. _urllib3 v2.0 roadmap: https://urllib3.readthedocs.io/en/stable/v2-roadmap.html
diff --git a/src/crate/client/sqlalchemy/compiler.py b/src/crate/client/sqlalchemy/compiler.py
index cdc0772..767ad63 100644
--- a/src/crate/client/sqlalchemy/compiler.py
+++ b/src/crate/client/sqlalchemy/compiler.py
@@ -309,3 +309,10 @@ class CrateCompiler(compiler.SQLCompiler):
         Generate OFFSET / LIMIT clause, PostgreSQL-compatible.
         """
         return PGCompiler.limit_clause(self, select, **kw)
+
+    def for_update_clause(self, select, **kw):
+        # CrateDB does not support the `INSERT ... FOR UPDATE` clause.
+        # See https://github.com/crate/crate-python/issues/577.
+        warnings.warn("CrateDB does not support the 'INSERT ... FOR UPDATE' clause, "
+                      "it will be omitted when generating SQL statements.")
+        return ''


Test patch:
diff --git a/src/crate/client/sqlalchemy/tests/compiler_test.py b/src/crate/client/sqlalchemy/tests/compiler_test.py
index 44cb16c..9c08154 100644
--- a/src/crate/client/sqlalchemy/tests/compiler_test.py
+++ b/src/crate/client/sqlalchemy/tests/compiler_test.py
@@ -43,7 +43,7 @@ from crate.client.test_util import ParametrizedTestCase
 from crate.testing.settings import crate_host
 
 
-class SqlAlchemyCompilerTest(ParametrizedTestCase):
+class SqlAlchemyCompilerTest(ParametrizedTestCase, ExtraAssertions):
 
     def setUp(self):
         self.crate_engine = sa.create_engine('crate://')
@@ -257,6 +257,34 @@ class SqlAlchemyCompilerTest(ParametrizedTestCase):
             mock.call(mock.ANY, 'INSERT INTO mytable (name) VALUES (?)', ('foo_4', ), None),
         ])
 
+    def test_for_update(self):
+        """
+        Verify the `CrateCompiler.for_update_clause` method to
+        omit the clause, since CrateDB does not support it.
+        """
+
+        with warnings.catch_warnings(record=True) as w:
+
+            # By default, warnings from a loop will only be emitted once.
+            # This scenario tests exactly this behaviour, to verify logs
+            # don't get flooded.
+            warnings.simplefilter("once")
+
+            selectable = self.mytable.select().with_for_update()
+            _ = str(selectable.compile(bind=self.crate_engine))
+
+            selectable = self.mytable.select().with_for_update()
+            statement = str(selectable.compile(bind=self.crate_engine))
+
+        # Verify SQL statement.
+        self.assertEqual(statement, "SELECT mytable.name, mytable.data \nFROM mytable")
+
+        # Verify if corresponding warning is emitted, once.
+        self.assertEqual(len(w), 1)
+        self.assertIsSubclass(w[-1].category, UserWarning)
+        self.assertIn("CrateDB does not support the 'INSERT ... FOR UPDATE' clause, "
+                      "it will be omitted when generating SQL statements.", str(w[-1].message))
+
 
 FakeCursor = MagicMock(name='FakeCursor', spec=Cursor)