bikeshedder / tusker

PostgreSQL migration management tool
The Unlicense
208 stars 17 forks source link

Sql Alchemy and unescaped JSON #18

Closed RichardNeill closed 2 years ago

RichardNeill commented 2 years ago

One of my schema functions contains the line:

json = '{ "type": "Category", "data": {"empty":0} }';

followed by:

INSERT INTO points (series_id, value, json) VALUES (theseries.id, value, json);

which is fine when the function is created (in psql), or executed. Note that the value of "empty" is intentionally "integer zero".

However, when running tusker, I get:

Error executing SQL file schema_functions.sql: (sqlalchemy.exc.InvalidRequestError) A value is required for bind parameter '0' (Background on this error at: https://sqlalche.me/e/14/cd3x)

I can make this error go away, if I change the function line to:

json = '{ "type": "Category", "data": {"empty":"0"} }';

(i.e "string-zero", which of course is not what I intend).

I think that Tusker is failing to correctly escape that line when it hands it off to sql-alchemy, which in turn is (mis)interpreting the 0 as a bind-parameter rather than a literal integer.

Additionally, the error message was very hard to track down, because it didn't print the line-number within the schema_functions.sql file, so I had to manually delete/undelete sections of a 2000-line script to locate the offending line. So please do print the line-numbers where possible.

Thanks for your help.

bikeshedder commented 2 years ago

I finally came around looking at this. The fix was simple once I figured out what's going on:

SQLAlchemy did parse the :<param> as a parameter and tried to be smart about it. It looks like a SQLAlchemy bug in a way as it doesn't treat string literals as such and seams to perform a simple pattern matching even inside of string constants.

I just replaced the cursor.execute(text(...)) by a simple cursor.exec_driver_sql(...) which circumvents all that SQLAlchemy logic we don't actually need.

bikeshedder commented 2 years ago

I just released Tusker 0.4.6 which includes this fix: https://pypi.org/project/tusker/0.4.6/

bikeshedder commented 2 years ago

Big whoops. I had to yank the 0.4.6 release because it contained parts of #19 by accident.

0.4.7 is the proper release with that bugfix: https://pypi.org/manage/project/tusker/release/0.4.7/