bug_fix(1477): type handling in _add_sql_comment

[aryabharat]

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.

• Bug fix (non-breaking change which fixes an issue)

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: aryabharat / name: Bharat Arya (d375a60)

[xrmx]

Thanks for the PR, could you please add a test so we are sure that the code is fixing the reported issue?

[Kludex]

I think the type of sql should be str here, and we should fix the callers.

[aryabharat]

Is this the statement = _add_sql_comment( function?

[tammy-baylis-swi]

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.

[aryabharat]

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?

[tammy-baylis-swi]

Yes, it's generally good if you can follow the psycopg2 docs and it fixes the type errors.

[aryabharat]

I have update the code, can you please check if this is the fine then will add the test case.

[tammy-baylis-swi]

Sorry @aryabharat I must have mixed up PRs in haste. Using that psycopg2 utility will work within Psycopg2Instrumentor, but adding the usage/dependency might not work for the places where the changes are being made (DB-API, SQLAlchemy, Django). Please could you try these instead:

For SQLAlchemy, their documentation says to use str(), so please switch to that. It's like the change you originally proposed, but in the Sqlalchemy instrumentor instead.

For DB-API and Django it can depend on the db driver being used. (DB-API instrumentor more of a shared utility rather; Django is an ORM). Sometimes it is psycopg2 but user could be using mysqlclient instead, for example. Using str() might work just as well, but should be tested for different cases.

[aryabharat]

Hi @tammy-baylis-swi , thanks for your help and sorry for the delay.

What I have done for SQLAlchemy is to use str() directly.

However, in the case of DB-API and Django with psycopg2, we cannot use str(), as it returns an unformatted query. So, I handled the psycopg2 case by checking the function as_string().

For mysqlclient, I handled it by using str() only.

Please let me, if you have any inputs.