bug_fix(1477): type handling in _add_sql_comment #3113
Conversation
|
|
| @@ -22,6 +22,8 @@ def _add_sql_comment(sql, **meta) -> str: | |||
| """ | |||
| meta.update(**_add_framework_tags()) | |||
| comment = _generate_sql_comment(**meta) | |||
| # converting to str to handle any type errors | |||
| sql = str(sql) | |||
There was a problem hiding this comment.
I think the type of sql should be str here, and we should fix the callers.
There was a problem hiding this comment.
Is this the statement = _add_sql_comment( function?
There was a problem hiding this comment.
Hi @aryabharat thanks for working on this!
Yes, and there are also 2 other util callers in the SQLAlchemy instrumentor and the Django instrumentor. These also support sqlcommenting, though not consistently with the way the psycopg2 instrumentor does. Fixing all 3 spots (DB-API, SQLAlchemy, Django) would be great with added testing.
There was a problem hiding this comment.
Hi @tammy-baylis-swi thanks for the reply.
Just one question, on handling the composable object
For handling SQL inputs:
If the input is a psycopg2.sql.Composable object (not a string),
convert it to a string using the as_string() method.
Example:
if isinstance(sql_input, sql.Composable):
sql_string = sql_input.as_string(connection)
Should i handle like this?
There was a problem hiding this comment.
Yes, it's generally good if you can follow the psycopg2 docs and it fixes the type errors.
Description
This PR fixes an issue with SQL comment generation, the current implementation fails when processing non-string SQL queries, particularly those from psycopg2's SQL utilities.
Modified _add_sql_comment() function to handles different SQL query types by converting incoming sql queries to strings
Preserves the original query if comment generation fails
Fixes #1477
Type of change
Please delete options that are not relevant.