Convert to using psycopg.sql

[amjith]

Description

Ref: #134
Use psycopg.sql to construct SQL statements.

Opening this PR to start a discussion.

[amjith]

@dvarrazzo This statement fails because .format() is only defined in the SQL class. But the result of the __add__() method is of the parent class type Composable. I'm not really sure how to circumvent this.

[dvarrazzo]

In this specific case you can attach the pattern to the WHERE bit:

query += SQL("WHERE d.datname ~ {}").format(schema)

should do (schema is wrapped automatically in a Literal).

More in general, you can create the main skeleton of the query and inject the variable parts as parameter, instead of chaining a sequence. In this case it should be (untested):

    params = {}
    query = SQL('''
        SELECT d.datname as "Name",
        pg_catalog.pg_get_userbyid(d.datdba) as "Owner",
        pg_catalog.pg_encoding_to_char(d.encoding) as "Encoding",
        d.datcollate as "Collate",
        d.datctype as "Ctype",
        pg_catalog.array_to_string(d.datacl, E'\n') AS "Access privileges
        {verbose_fields}
        FROM pg_catalog.pg_database d
        {verbose_tables}
        {pattern_where}
        ORDER BY 1
        "''')

    if verbose:
        params["verbose_fields"] = SQL(''',
            CASE WHEN pg_catalog.has_database_privilege(d.datname, 'CONNECT')
                    THEN pg_catalog.pg_size_pretty(pg_catalog.pg_database_size(d.datname))
                    ELSE 'No Access'
            END as "Size",
            t.spcname as "Tablespace",
            pg_catalog.shobj_description(d.oid, 'pg_database') as "Description"''')
        params["verbose_tables"] = SQL("""
            JOIN pg_catalog.pg_tablespace t on d.dattablespace = t.oid
            """)
    else:
        params["verbose_fields"] = SQL("")
        params["verbose_tables"] = SQL("")

    if pattern:
        _, schema = sql_name_pattern(pattern)
        params["schema_pattern"] = schema

        params["pattern_where"] = SQL("""
        WHERE d.datname ~ {}
        """).format(schema)
    else:
        params["pattern_where"] = SQL("")

    cur.execute(query.format(**params))

[amjith]

Thank you! That makes sense. I've updated the PR with the "correct" implementation for this function.

I'll update the rest of the commands later today.

[codecov-commenter]

Codecov Report

❗ No coverage uploaded for pull request base (main@3c3aae2). Click here to learn what that means.
Patch has no changes to coverable lines.

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #137   +/-   ##
=======================================
  Coverage        ?   67.32%           
=======================================
  Files           ?        6           
  Lines           ?     1212           
  Branches        ?        0           
=======================================
  Hits            ?      816           
  Misses          ?      396           
  Partials        ?        0           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[j-bennet]

Not sure, but I think this should be different now:

WHERE d.datname ~ {schema_pattern}

[dvarrazzo]

Seems correct, as well as using Literal(schema) in the params.

[j-bennet]

Same here, I may be mistaken, but from the docs looks like this should now be:

sql += SQL("WHERE r.rolname ~ {rolname}")

vs

sql += SQL("WHERE r.rolname ~ %(rolname)s")

[dvarrazzo]

If you use the {name} parameters, you have to use .format() to pass it a value, which must be a Composible instance. If it's not, it will be wrapped in a Literal (in psycopg) and adapted as per normal rules (in psycopg2 it will raise an exception, FYI).

If you use a %s or %(name)s placeholder, it will not be touched by format, but then you will pass the values to replace on execute.

So, the full picture should be one of the two:

cur.execute(sql.SQL("WHERE rolename = {rolename}").format(rolename=myrolename))
cur.execute(sql.SQL("WHERE rolename = %(rolename)s"), {"rolename": myrolename})

[j-bennet]

Nice work, especially with added tests.

[j-bennet]

Calling .format twice, once to log, and once to execute, could save that formatted instance and reuse.

[j-bennet]

Same, calling .format twice in a row.