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.

Sign up for GitHub

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

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

Merged
merged 3 commits into from
Dec 22, 2022
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
88e4846
Revert "SA20: Address deprecation warnings in SqlAlchemyDictTypeTest"
amotl Dec 21, 2022
01b5a09
SA20: Fix SqlAlchemyDictTypeTest
amotl Dec 21, 2022
efca25e
SA20: Fix SqlAlchemyInsertFromSelectTest
amotl Dec 22, 2022
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
24 changes: 13 additions & 11 deletions src/crate/client/sqlalchemy/tests/dict_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,21 +37,22 @@


class SqlAlchemyDictTypeTest(TestCase):

def setUp(self):
self.engine = sa.create_engine('crate://')
metadata = sa.MetaData()
self.mytable = sa.Table('mytable',
metadata,
self.mytable = sa.Table('mytable', metadata,
sa.Column('name', sa.String),
sa.Column('data', Craty))

def assertSQL(self, expected_str, actual_expr):
def assertSQL(self, expected_str, selectable):
actual_expr = selectable.compile(bind=self.engine)
self.assertEqual(expected_str, str(actual_expr).replace('\n', ''))

def test_select_with_dict_column(self):
mytable = self.mytable
self.assertSQL(
"SELECT mytable.data[:data_1] AS anon_1 FROM mytable",
"SELECT mytable.data['x'] AS anon_1 FROM mytable",
select(mytable.c.data['x'])
)

Expand All @@ -60,7 +61,7 @@ def test_select_with_dict_column_where_clause(self):
s = select(mytable.c.data).\
where(mytable.c.data['x'] == 1)
self.assertSQL(
"SELECT mytable.data FROM mytable WHERE mytable.data[:data_1] = :param_1",
"SELECT mytable.data FROM mytable WHERE mytable.data['x'] = ?",
s
)

Expand All @@ -70,7 +71,7 @@ def test_select_with_dict_column_nested_where(self):
s = s.where(mytable.c.data['x']['y'] == 1)
self.assertSQL(
"SELECT mytable.name FROM mytable " +
"WHERE mytable.data[:data_1][:param_1] = :param_2",
"WHERE mytable.data['x']['y'] = ?",
s
)

Expand All @@ -79,7 +80,7 @@ def test_select_with_dict_column_where_clause_gt(self):
s = select(mytable.c.data).\
where(mytable.c.data['x'] > 1)
self.assertSQL(
"SELECT mytable.data FROM mytable WHERE mytable.data[:data_1] > :param_1",
"SELECT mytable.data FROM mytable WHERE mytable.data['x'] > ?",
s
)

Expand All @@ -89,18 +90,19 @@ def test_select_with_dict_column_where_clause_other_col(self):
s = s.where(mytable.c.data['x'] == mytable.c.name)
self.assertSQL(
"SELECT mytable.name FROM mytable " +
"WHERE mytable.data[:data_1] = mytable.name",
"WHERE mytable.data['x'] = mytable.name",
s
)

def test_update_with_dict_column(self):
mytable = self.mytable
stmt = mytable.update().\
where(mytable.c.name == 'Arthur Dent').\
values({mytable.c.data['x']: "Trillian"})

values({
"data['x']": "Trillian"
})
Comment on lines -100 to +103
Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

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

self.assertSQL(
"UPDATE mytable SET data[:data_1]=:param_1 WHERE mytable.name = :name_1",
"UPDATE mytable SET data['x'] = ? WHERE mytable.name = ?",
stmt
)

Expand Down
5 changes: 2 additions & 3 deletions src/crate/client/sqlalchemy/tests/insert_from_select_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,8 +75,7 @@ def test_insert_from_select_triggered(self):
ins = insert(self.character_archived).from_select(['name', 'age'], sel)
self.session.execute(ins)
self.session.commit()
# TODO: Verify if this is correct.
self.assertSQL(
"INSERT INTO characters_archive (name, age) SELECT characters.name, characters.age FROM characters WHERE characters.status = :status_1",
ins
"INSERT INTO characters_archive (name, age) SELECT characters.name, characters.age FROM characters WHERE characters.status = ?",
ins.compile(bind=self.engine)
)