SA20: Fix SqlAlchemy tests to use "qmark" paramstyle again

[amotl]

Dear @hammerhead,

regarding your recent patch #486 about the fix to SqlAlchemyDictTypeTest, I learned more about the details while working on #488. Based on the rationale provided below, I think 30d6ea4 is the right fix at this place, to retain the "qmark" paramstyle being sent to the driver. It makes sense to look at this very commit only, in order to not get distracted by the other changes induced by reverting your original commit b20faba.

Within #490, I made the same mistake on overlooking this detail at the SqlAlchemyInsertFromSelectTest, and erroneously followed the updated output from the reported diff, effectively accepting a change to the "named" paramstyle. efca25e gets it right again, by also binding the insert clause to an engine.

With kind regards,
Andreas.

Rationale

"Implicit" and "Connectionless" execution, and "bound metadata", have been removed beginning with SQLAlchemy 2.0 ¹.

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 conducted 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" ² of SQLAlchemy's DefaultDialect would be used, which is actually the "named" paramstyle ³, as originally reflected per b20faba.

This is probably wrong, because the CrateDB Python driver uses the "qmark" paramstyle ⁴.

Footnotes

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 ↩

[amotl]

@hammerhead: Oh, I forgot to keep this one when reverting your change. Apologies. Maybe you can add it to GH-485 again, with a descriptive commit message, which outlines that there was an actual programming error here?

[hammerhead]

Done: 1429658

I checked it a bit more in detail, and found that parameters in Cursor.execute differ: Without my change, it receives a BindParameter ((BindParameter('%(4541705392 param)s', 'Trillian', type_=NullType()), '1')). With my change, it receives a Tuple (('Trillian', '1')).

Both variants seem to carry the same information, but the current implementation fails to handle the BindParameter correctly when attempting to serialize it to JSON for the HTTP call. So it might also be considered a missing feature that what the test was previously trying to do would fail (potentially used to work earlier?).

[hammerhead]

The fix is rather trivial:

diff --git a/src/crate/client/http.py b/src/crate/client/http.py
index e932f73..ac03ae8 100644
--- a/src/crate/client/http.py
+++ b/src/crate/client/http.py
@@ -52,6 +52,7 @@ from crate.client.exceptions import (
     DigestNotFoundException,
     ProgrammingError,
 )
+from sqlalchemy.sql.expression import BindParameter
 
 
 logger = logging.getLogger(__name__)
@@ -93,6 +94,9 @@ class CrateJsonEncoder(json.JSONEncoder):
                        (delta.seconds + delta.days * 24 * 3600) * 1000.0)
         if isinstance(o, date):
             return calendar.timegm(o.timetuple()) * 1000
+        if isinstance(o, BindParameter):
+            return o.value
+
         return json.JSONEncoder.default(self, o)

But I was unable to produce a test case yet that would expose the issue or generally asses how much of a problem this is.

[amotl]

Hi Niklas,

this is intriguing. Thank you for sharing your findings. Based on this information, I am thinking about if 1429658 should better be handed in separately, maybe together with the other fix you shared above, and two integration test cases (without using mocking), which exercise and verify that both variants work.

But I was unable to produce a test case yet that would expose the issue.

GH-491 may come to the rescue for finding an appropriate place for those integration tests, so this is probably the right chance to spawn it. I will stage a corresponding PR where your fixes can then be picked into.

With kind regards,
Andreas.