Fix jinjasql+sqla vs object attribute dereferencing notation at jinjasql level. (#80)

* Fix jinjasql+sqla vs object attribute dereferencing notation at jinjasql level.
* Switch to use poetry to run flake8, black. isort, safety so that CICD and local dev uses same linter versions.

Author: James Robinson

.github/workflows/main.yaml

@@ -43,32 +43,36 @@ jobs:
           python-version: ${{ matrix.python-version }}
           cache: poetry
 
-      - name: Install tox
-        run: pip install tox
+      - name: Install dependencies
+        shell: bash
+        run: poetry install
 
       - name: Install CockroachDB
         uses: ./.github/actions/setup-cockroachdb
 
       # Run all of the tests
       - name: Flake8 - Style guide enforcement
-        run: tox -e flake8
+        run: poetry run flake8 noteable_magics/ tests/ --count --show-source --statistics --benchmark
         if: ${{ matrix.python-version == 3.9 }}
 
       - name: Black - Format check
-        run: tox -e black-check
+        run: poetry run black --diff noteable_magics/ tests/
         if: ${{ matrix.python-version == 3.9 }}
 
       - name: Isort - Import format check
-        run: tox -e isort-check
+        run: poetry run isort --diff --check noteable_magics/ tests/
         if: ${{ matrix.python-version == 3.9 }}
 
       - name: Safety - Python dependency vulnerability check
-        run: tox -e safety
+        run: poetry export --without-hashes -f requirements.txt | poetry run safety check --stdin -i 51457 -i 51668
         if: ${{ matrix.python-version == 3.9 }}
 
       - name: Set env
         id: vars
         run: PY_VER=${{ matrix.python-version }}; echo ::set-output name=tox_py::${PY_VER//.}
 
+      - name: Install tox for running tests with
+        run: pip install tox
+
       - name: Pytest - Unit tests
         run: tox -e py${{ steps.vars.outputs.tox_py }}

noteable_magics/sql/run.py

@@ -112,7 +112,31 @@ def _commit(conn, config):
             pass  # not all engines can commit
 
 
-jinja_sql = JinjaSql(param_style='named')
+jinja_sql = JinjaSql(param_style='numeric')
+
+##
+# Why we use JinjaSql in numeric mode when feeding into SQLA text() expressions.
+#
+# We originally used 'named' format, which worked great in combination with SQLA text() parameter
+# syntax, up until trying to do object attribute dereferencing in jinja expressions
+# ("select id from foo where id = {{myobj.id}}"), at which time the resulting de-templated query
+# and bind params would be "select id from foo where id = :myobj.id" and {'myobj.id': 1}, which was
+# at least self-consistent until SQLA text() tries to convert that into the database driver's preferred parameter format
+# (say, pyformat) and would end up with *closing the parens too early* due to seeing the non-identifier
+# token '.': "select id from foo where id = %(myobj)s.id" with bind dict {'myobj.id': 1}, and croak
+# due to not finding exactly 'myobj' as key in the bind dict (had it made it past that, the query
+# still would have failed because the trailing '.id' would have been presented to the DB, which was never
+# the intent).
+#
+# So, to ensure that we pass only brain-dead-simple paramater expansion needs into the SQL text() expression,
+# we now drive jinja in numeric mode, so that it replaces templated variable expressions with just
+# a colon and a number ("select id from foo where id = :1"). It hands us back a list instead of a dict
+# as the bind parameters, but is easy enough to transform that list [12] into the dict of
+# string keys -> values ({'1': 12}) which SQLA's text() + conn.execute() expect for such a text() template.
+#
+# People don't much combine JinjaSQL on top of SQLA, in that they solve 'conditional composed SQL'
+# completely differently. We're special I guess?
+##
 
 
 def run(conn, sql, config, user_namespace, skip_boxing_scalar_result: bool):
@@ -123,9 +147,14 @@ def run(conn, sql, config, user_namespace, skip_boxing_scalar_result: bool):
             if first_word == "begin":
                 raise Exception("ipython_sql does not support transactions")
 
-            query, bind_params = jinja_sql.prepare_query(statement, user_namespace)
+            query, bind_list = jinja_sql.prepare_query(statement, user_namespace)
+
+            # Convert bind_list from positional list to dict per needs of a paramaterized text()
+            # construct.
+            bind_dict = {str(idx + 1): elem for (idx, elem) in enumerate(bind_list)}
+
             txt = sqlalchemy.sql.text(query)
-            result = conn.session.execute(txt, bind_params)
+            result = conn.session.execute(txt, bind_dict)
 
             _commit(conn=conn, config=config)
 

poetry.lock

@@ -56,6 +56,7 @@ setuptools = "^65.3.0"
 tox = "^3.23.1"
 managed-service-fixtures = "^0.1.5"
 requests-mock = "^1.10.0"
+safety = "^2.3.5"
 
 
 [build-system]

pyproject.toml

@@ -220,6 +220,25 @@ def test_basic_query(self, sql_magic, ipython_shell, a_value, expected_b_value):
         assert isinstance(results, int)
         assert results == expected_b_value
 
+    def test_query_dereferencing_object_attribute(self, sql_magic, ipython_shell):
+        class MyClass:
+            id: int
+
+            def __init__(self, id: int):
+                self.id = id
+
+        mc = MyClass(1)
+
+        ipython_shell.user_ns['mc'] = mc
+
+        ## jinjasql expansion including object attribute dereference!
+        results = sql_magic.execute(
+            COCKROACH_HANDLE + '\n#scalar select b from int_table where a = {{mc.id}}'
+        )
+
+        assert isinstance(results, int)
+        assert results == 2
+
     def test_insert_into(self, sql_magic, ipython_shell, capsys):
         """Create a one-off table, then test inserting into from python variables interpolated by jinja"""