Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

string queries and ORDER BY clause #442

Open
rsurgiewicz opened this issue Jan 14, 2022 · 12 comments
Open

string queries and ORDER BY clause #442

rsurgiewicz opened this issue Jan 14, 2022 · 12 comments

Comments

@rsurgiewicz
Copy link

Hi there,

fist of all: big kudos for the module! Guys you've done an amazing piece of work here!

I have a problem or possibly a bug.
postgres: When using parameterized text based query order by clause parameters are not working (are ignored)

My simplified query:
SELECT * FROM "sourceTable" WHERE 1=1 ORDER BY :orderBy DESC LIMIT :topn

then I use:
await database.fetch_all(query=QRY, values = {"orderBy": "startTime","topn":1}

and debug says:
DEBUG:databases:Query: SELECT * FROM "sourceTable" WHERE 1=1 ORDER BY $1 DESC LIMIT $2; Args: ('startTime', 1)
but results are not ordered at all.

One can provide a non-existing column and query is still execuded, e.g.,
DEBUG:databases:Query: SELECT * FROM "rfb_active_tests" WHERE 1=1 ORDER BY $1 LIMIT $2; Args: ('who_ate_the_cookies_from_the_cookie_jar ', 1)
and results are the same.

I've tried several approaches including ASC or DESC in the orderBy parameter itself or by providing a Tuple, but apparently none worked.

Unfortunately I have to use text SQL queries.
Is it a bug or am I doing something wrong?

How shall I pass the order by parameters (REST param) to avoid e.g SQLInjections then?

Thanks

@aminalaee
Copy link
Member

Hi,

Can you provide a minimal example where we can see what is the current output and what is the expected output?

It will be much easier and faster to help.

@rsurgiewicz
Copy link
Author

rsurgiewicz commented Jan 14, 2022

thank you for quick reply:

here is data table:

id startTime avgResult
zX2Rr0Vc1E 2021-12-09 13:49:48.141000 6428932
DSN54vThdq 2021-12-09 13:54:00.265000 7088159
GGUVUITrgU 2021-12-09 14:11:46.921000 4661406
r3p8RVu3SB 2021-12-10 11:27:43.446000 21975032.5

python:

        QRY = """
                SELECT id FROM sourcetable WHERE 1=1 ORDER BY :orderBy DESC LIMIT :topN;
                """

        QRY_PARAM_DICT = {
            "orderBy": '"startTime"',
            "topN": 1
        }
        output = await database.fetch_all(query=QRY, values = QRY_PARAM_DICT)

debug:
DEBUG:databases:Query: SELECT id FROM sourcetable WHERE 1=1 ORDER BY $1 DESC LIMIT $2; Args: ('"startTime"', 1)

Result:
id = zX2Rr0Vc1E

id startTime avgResult
zX2Rr0Vc1E 2021-12-09 13:49:48.141000 6428932

Expected result:
id = r3p8RVu3SB

id startTime avgResult
r3p8RVu3SB 2021-12-10 11:27:43.446000 21975032.5

@aminalaee
Copy link
Member

Sorry I forgot to ask, which database driver are you using?

@rsurgiewicz
Copy link
Author

psycopg2 (psycopg2-binary 2.9.3 to be more precise)

@aminalaee
Copy link
Member

I'm not sure if it's really related to this issue, but Databases is supposed to be used with an async db driver like asyncpg or aiopg for PostgreSQL.

@collerek
Copy link
Member

It might be related to #139. I know that in some cases when you pass parameters outside of columns scope (like where clause or order clause) databases passes all values to columns instead of splitting them by name etc.

@aminalaee
Copy link
Member

I just tested that fix but that doesn't solve the issue.

@rsurgiewicz
Copy link
Author

rsurgiewicz commented Jan 17, 2022

Did you try to replicate the issue?

Meanwhile, I've tried with asyncpg driver, but I observe same misbehavior.

@aminalaee
Copy link
Member

yes, that fails on other drivers too, I've tried sqlite.

@rsurgiewicz
Copy link
Author

rsurgiewicz commented Jan 17, 2022

Ok, I see.

Looking on the bright side; at least it's replicable.

If there's a sql-injection-safe workaround that could be used, please let me know. thx

@aminalaee
Copy link
Member

I think this is the behaviour of bindparams in SQLAlchemy, which is correct. Databases uses bindparams to set query parameters. An example from SQLAlchemy:

stmt = text("SELECT id, name FROM user WHERE name=:name "
            "AND timestamp=:timestamp")

stmt = stmt.bindparams(name='jack',
            timestamp=datetime.datetime(2012, 10, 8, 15, 12, 5))

The bindparams does not set columns, it sets column properties. I thought about passing literal columns instead, something like this:

from sqlalchemy.sql import literal_column

result = await db.fetch_all("SELECT * FROM foo ORDER BY :c", values={"c": literal_column('bar')})

But the aiosqliue doesn't handle that, and that's I think expected.

As a workaround you might be able to get SQLAlchemy query expressions to do this, I know it's not always the case:

from sqlalchemy import select

stmt = select(X).order_by(x)
result = await db.fetch_all(stmt)

What do you think?

@rsurgiewicz
Copy link
Author

rsurgiewicz commented Jan 17, 2022

Thanks @aminalaee,

I've tried the sqlalchemy select approach.

Even though it produces a list of databases.backends.postgres.Record objects, but when I try to read the data from it or convert it using either dict(onerecord) or {**onerecord} I get KeyError e.g.,

File .... site-packages/databases/backends/postgres.py", line 139, in __getitem__
    idx, datatype = self._column_map[key]
KeyError: 'id'

(id is column name in query)

---- EDIT -----
Oh I see, with sqlalchemy than I need to implement the ORM based approach.
Unfortunately this is not applicable here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants