From d7aa017d83b416187b54ad38400475fd86d80671 Mon Sep 17 00:00:00 2001 From: Mike Bayer Date: Wed, 4 Sep 2019 18:46:53 -0400 Subject: [PATCH] Strip special chars in anonymized bind names Characters that interfere with "pyformat" or "named" formats in bound parameters, namely ``%, (, )`` and the space character, as well as a few other typically undesirable characters, are stripped early for a :func:`.bindparam` that is using an anonymized name, which is typically generated automatically from a named column which itself includes these characters in its name and does not use a ``.key``, so that they do not interfere either with the SQLAlchemy compiler's use of string formatting or with the driver-level parsing of the parameter, both of which could be demonstrated before the fix. The change only applies to anonymized parameter names that are generated and consumed internally, not end-user defined names, so the change should have no impact on any existing code. Applies in particular to the psycopg2 driver which does not otherwise quote special parameter names, but also strips leading underscores to suit Oracle (but not yet leading numbers, as some anon parameters are currently entirely numeric/underscore based); Oracle in any case continues to quote parameter names that include special characters. Fixes: #4837 Change-Id: I21cb654c3e4ef786114160b8b4295242720bf3f9 --- doc/build/changelog/unreleased_13/4837.rst | 20 ++++++++ lib/sqlalchemy/sql/elements.py | 8 +++- test/dialect/oracle/test_compiler.py | 8 ++-- test/sql/test_compiler.py | 54 ++++++++++++++++++++++ 4 files changed, 85 insertions(+), 5 deletions(-) create mode 100644 doc/build/changelog/unreleased_13/4837.rst diff --git a/doc/build/changelog/unreleased_13/4837.rst b/doc/build/changelog/unreleased_13/4837.rst new file mode 100644 index 0000000000..def5aaa5ec --- /dev/null +++ b/doc/build/changelog/unreleased_13/4837.rst @@ -0,0 +1,20 @@ +.. change:: + :tags: bug, sql + :tickets: 4837 + + Characters that interfere with "pyformat" or "named" formats in bound + parameters, namely ``%, (, )`` and the space character, as well as a few + other typically undesirable characters, are stripped early for a + :func:`.bindparam` that is using an anonymized name, which is typically + generated automatically from a named column which itself includes these + characters in its name and does not use a ``.key``, so that they do not + interfere either with the SQLAlchemy compiler's use of string formatting or + with the driver-level parsing of the parameter, both of which could be + demonstrated before the fix. The change only applies to anonymized + parameter names that are generated and consumed internally, not end-user + defined names, so the change should have no impact on any existing code. + Applies in particular to the psycopg2 driver which does not otherwise quote + special parameter names, but also strips leading underscores to suit Oracle + (but not yet leading numbers, as some anon parameters are currently + entirely numeric/underscore based); Oracle in any case continues to quote + parameter names that include special characters. diff --git a/lib/sqlalchemy/sql/elements.py b/lib/sqlalchemy/sql/elements.py index bc6f51b8c6..3caa380ee4 100644 --- a/lib/sqlalchemy/sql/elements.py +++ b/lib/sqlalchemy/sql/elements.py @@ -1227,7 +1227,13 @@ class BindParameter(roles.InElementRole, ColumnElement): if unique: self.key = _anonymous_label( - "%%(%d %s)s" % (id(self), key or "param") + "%%(%d %s)s" + % ( + id(self), + re.sub(r"[%\(\) \$]+", "_", key).strip("_") + if key is not None + else "param", + ) ) else: self.key = key or _anonymous_label("%%(%d param)s" % id(self)) diff --git a/test/dialect/oracle/test_compiler.py b/test/dialect/oracle/test_compiler.py index 2edb68e888..a6ea7d2b17 100644 --- a/test/dialect/oracle/test_compiler.py +++ b/test/dialect/oracle/test_compiler.py @@ -177,14 +177,14 @@ class CompileTest(fixtures.TestBase, AssertsCompiledSQL): self.assert_compile( s, "SELECT anon_1.col1, anon_1.col2 FROM " - "(SELECT /*+ FIRST_ROWS([POSTCOMPILE__ora_frow_1]) */ " + "(SELECT /*+ FIRST_ROWS([POSTCOMPILE_ora_frow_1]) */ " "anon_2.col1 AS col1, " "anon_2.col2 AS col2, ROWNUM AS ora_rn FROM (SELECT " "sometable.col1 AS col1, sometable.col2 AS " "col2 FROM sometable) anon_2 WHERE ROWNUM <= " "[POSTCOMPILE_param_1]) anon_1 WHERE ora_rn > " "[POSTCOMPILE_param_2]", - checkparams={"_ora_frow_1": 10, "param_1": 30, "param_2": 20}, + checkparams={"ora_frow_1": 10, "param_1": 30, "param_2": 20}, dialect=oracle.OracleDialect(optimize_limits=True), ) @@ -263,13 +263,13 @@ class CompileTest(fixtures.TestBase, AssertsCompiledSQL): s = select([t]).with_for_update().limit(10).order_by(t.c.col2) self.assert_compile( s, - "SELECT /*+ FIRST_ROWS([POSTCOMPILE__ora_frow_1]) */ " + "SELECT /*+ FIRST_ROWS([POSTCOMPILE_ora_frow_1]) */ " "anon_1.col1, anon_1.col2 FROM (SELECT " "sometable.col1 AS col1, sometable.col2 AS " "col2 FROM sometable ORDER BY " "sometable.col2) anon_1 WHERE ROWNUM <= [POSTCOMPILE_param_1] " "FOR UPDATE", - checkparams={"param_1": 10, "_ora_frow_1": 10}, + checkparams={"param_1": 10, "ora_frow_1": 10}, dialect=oracle.OracleDialect(optimize_limits=True), ) diff --git a/test/sql/test_compiler.py b/test/sql/test_compiler.py index ebc0fc6318..c789a879ad 100644 --- a/test/sql/test_compiler.py +++ b/test/sql/test_compiler.py @@ -3040,6 +3040,60 @@ class BindParameterTest(AssertsCompiledSQL, fixtures.TestBase): eq_(len(set(pp)), total_params, "%s %s" % (len(set(pp)), len(pp))) eq_(len(set(pp.values())), total_params) + def test_bind_anon_name_no_special_chars(self): + for paramstyle in "named", "pyformat": + dialect = default.DefaultDialect() + dialect.paramstyle = paramstyle + + for name, named, pyformat in [ + ("%(my name)s", ":my_name_s_1", "%(my_name_s_1)s"), + ("myname(foo)", ":myname_foo_1", "%(myname_foo_1)s"), + ( + "this is a name", + ":this_is_a_name_1", + "%(this_is_a_name_1)s", + ), + ("_leading_one", ":leading_one_1", "%(leading_one_1)s"), + ("3leading_two", ":3leading_two_1", "%(3leading_two_1)s"), + ("$leading_three", ":leading_three_1", "%(leading_three_1)s"), + ("%(tricky", ":tricky_1", "%(tricky_1)s"), + ("5(tricky", ":5_tricky_1", "%(5_tricky_1)s"), + ]: + t = table("t", column(name, String)) + expr = t.c[name] == "foo" + + self.assert_compile( + expr, + "t.%s = %s" + % ( + dialect.identifier_preparer.quote(name), + named if paramstyle == "named" else pyformat, + ), + dialect=dialect, + checkparams={named[1:]: "foo"}, + ) + + def test_bind_anon_name_special_chars_uniqueify_one(self): + # test that the chars are escaped before doing the counter, + # otherwise these become the same name and bind params will conflict + t = table("t", column("_3foo"), column("4%foo")) + + self.assert_compile( + (t.c["_3foo"] == "foo") & (t.c["4%foo"] == "bar"), + 't._3foo = :3foo_1 AND t."4%foo" = :4_foo_1', + checkparams={"3foo_1": "foo", "4_foo_1": "bar"}, + ) + + def test_bind_anon_name_special_chars_uniqueify_two(self): + + t = table("t", column("_3foo"), column("4(foo")) + + self.assert_compile( + (t.c["_3foo"] == "foo") & (t.c["4(foo"] == "bar"), + 't._3foo = :3foo_1 AND t."4(foo" = :4_foo_1', + checkparams={"3foo_1": "foo", "4_foo_1": "bar"}, + ) + def test_bind_as_col(self): t = table("foo", column("id")) -- 2.47.2