Unexpected TypeError from `Composable.as_string` called with no arguments

[fw-cc]

Hi all,

This is my first issue here so apologies if things aren't formatted properly.

I'm currently using psycopg 3.1.17 on Python 3.11 on Ubuntu 20.04.

My current direct usecase for this library involves sanitising and building text SQL queries, below is a mock example:

>>> example_query = sql.SQL("SELECT * FROM foo_table WHERE example_col = {bar_value}")
>>> example_query
SQL('SELECT * FROM foo_table WHERE example_col = {bar_value}')
>>> example_query.format(bar_value="formatted_bar_value")
Composed([SQL('SELECT * FROM foo_table WHERE example_col = '), Literal('formatted_bar_value')])
>>> example_query.format(bar_value="formatted_bar_value").as_string()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: Composable.as_string() missing 1 required positional argument: 'context'
>>> example_query.format(bar_value="formatted_bar_value").as_string(None)
"SELECT * FROM foo_table WHERE example_col = 'formatted_bar_value'"

I would have expected Composable.as_string called without arguments to default to context=None as context is defined as Optional in signatures and documentation, instead it errors unless None is provided for the context argument. This same behaviour exists with Composed objects as they inherit from Composable.

Assuming this would align with the style and intention of the library's text SQL API design, I believe this is a simple case of changing the to_string method context argument from:
... context: Optional[AdaptContext] ... to ... context: Optional[AdaptContext] = None ....

Many thanks for maintaining this library, it's incredibly useful.

[dvarrazzo]

Hello,

I will review this aspect, thank you very much for pointing out an eventual inconsistencies.

I seem to remember that certain sql.Composable subclasses use libpq functionalities that require to pass a connection object; for sure in psycopg2 passing a working context is mandatory. I have tried to relax this need, and I can't remember up to what extent.

We should check first if every Composable subclass supports a None as an argument. After that, surely we can use None as default so that you can call as_string() without argument.

[dvarrazzo]

One of the problem, maybe the only one, just checked, is:

>>> psycopg.sql.Identifier("hello").as_string(None)
ValueError: a connection is necessary for Identifier

that is probably what stopped so consider as_string() without arguments a good idea.

This requirement comes because PQescapeIdentifier() takes a connection; other functions such as PQescapeString()/PQescapeStringConn() exist in variants that take or not take a connection (but the one without connection is somewhat deprecated).

So, if we wanted to support None as argument for all Composible's subclasses .as_string() we should implement in Python an escape_identifier_no_conn() function, which is probably not too difficult, but must be done.

[dvarrazzo]

There was recently some grumpiness, which I don't want to link to avoid giving grumpiness a platform, in psycopg2, of someone complaining about as_string() taking a mandatory connection.

Because in psycopg 3 this is close to be reached, and an escaping identifier function is totally doable, I've set this for completion in Psycopg 3.2.

For reference, this is the implementation libpq function that we ought to reproduce. It doesn't need to be in C, because I don't expect the path of escaping identifiers to be particularly hot. A Python implementation would be ok as long as we have tests that prove that the results are what expected, char by char, with the behaviour with, let's say, escaping in the presence of an utf8 encoding connection.

[fw-cc]

That's a very sensible reason for this behaviour, thanks for such a quick response.

For my personal use-case I'm building and sanitising queries with psycopg's text query API but executing them elsewhere, and I believe everywhere I need to use None when generating strings will be supported by the library as is.

I would be interested in contributing to assist with this issue if you believe it feasible, though I do feel the need to mention this is slightly outside of my area of expertise. Please let me know what you think would be best.

Thanks again