SearchVector() can return query strings that are unsafe to combine.

[Patryk Zawadzki]

As the function specifically calls connection.ops.compose_sql, the returned sql will have all parameters inlined.

An unintended consequence is that if you pass it a value that contains a percent sign, like Value("10% OFF"), the resulting sql will have the % character inlined. Such values will result in a ProgrammingError as soon as you attempt to combine the SearchVector with any expression that relies on params.

Depending on whether you use psycopg2 or psycopg 3, the resulting error will tell you that there are not enough params to format the query template or that there is an unescaped % in the query.

[David Sanders]

Tentatively accepting, I'm seeing errors but only when the args to SearchVector are outside that which is documented to accept.

Felix & Simon will likely be able to clear things up as the arguments to SearchVector aren't "meant" to contain %: ​https://docs.djangoproject.com/en/4.2/ref/contrib/postgres/search/#changing-the-search-configuration & ​https://docs.djangoproject.com/en/4.2/ref/contrib/postgres/search/#weighting-queries

So, to clarify, is this the behaviour you're seeing?

There's no issue if you filter by the search vector like so:

Foo.objects.annotate(search=SearchVector("text")).filter(search=Value("10% off"))

It's when one of the arguments to SearchVector contain a percent that it becomes an issue:

Foo.objects.annotate(search=SearchVector("text", config="%")).filter(search=Value("10% off"))

...

django/db/models/query.py:373: in __repr__
    data = list(self[: REPR_OUTPUT_SIZE + 1])
django/db/models/query.py:397: in __iter__
    self._fetch_all()
django/db/models/query.py:1883: in _fetch_all
    self._result_cache = list(self._iterable_class(self))
django/db/models/query.py:90: in __iter__
    results = compiler.execute_sql(
django/db/models/sql/compiler.py:1560: in execute_sql
    cursor.execute(sql, params)
django/db/backends/utils.py:67: in execute
    return self._execute_with_wrappers(
django/db/backends/utils.py:80: in _execute_with_wrappers
    return executor(sql, params, many, context)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

self = <django.db.backends.utils.CursorWrapper object at 0x106e1bb20>
sql = 'SELECT "ticket_34459_foo"."id", "ticket_34459_foo"."text", to_tsvector(\'%\'::regconfig, COALESCE("ticket_34459_foo"....tsvector(\'%\'::regconfig, COALESCE("ticket_34459_foo"."text", \'\')) @@ (plainto_tsquery(%s::regconfig, %s)) LIMIT 21'
params = ('%', '10% off')
ignored_wrapper_args = (False, {'connection': <DatabaseWrapper vendor='postgresql' alias='default'>, 'cursor': <django.db.backends.utils.CursorWrapper object at 0x106e1bb20>})

    def _execute(self, sql, params, *ignored_wrapper_args):
        self.db.validate_no_broken_transaction()
        with self.db.wrap_database_errors:
            if params is None:
                # params default might be backend specific.
                print(sql)
                return self.cursor.execute(sql)
            else:
>               return self.cursor.execute(sql, params)
E               IndexError: tuple index out of range

django/db/backends/utils.py:90: IndexError

[Mariusz Felisiak]

Regression in 09ffc5c1212d4ced58b708cbbf3dfbfb77b782ca.

[Patryk Zawadzki]

David, I’m not sure if it’s “supported” or not, but my project uses SearchVector with expressions such as Value instead of column names so I see a lot of percent signs passed as the corpus of text, not the config or weight. We calculate some of the values we index in Python code and combine strings with different weights.

[Mariusz Felisiak]

We use compose_sql() in different places. Unfortunately, it's sometimes used together with uncomposed SQL statements causing issues when binding parameters. IMO, we should escape % when we know that the parameters are bound later, e.g.

django/contrib/postgres/search.py

@@ -146 +146 @@
 
         # These parameters must be bound on the client side because we may
         # want to create an index on this expression.
-        sql = connection.ops.compose_sql(sql, config_params + params + extra_params)
+        sql = connection.ops.compose_sql(
+            sql, config_params + params + extra_params, quote_params=True
+        )
         return sql, []
 
 
@@ -321 +323 @@
         if self.options:
             options_params.append(
                 ", ".join(
-                    connection.ops.compose_sql(f"{option}=%s", [value])
+                    connection.ops.compose_sql(
+                        f"{option}=%s", [value], quote_params=True
+                    )
                     for option, value in self.options.items()
                 )
             )

django/db/backends/postgresql/operations.py

@@ -201 +201 @@
             return name  # Quoting once is enough.
         return '"%s"' % name
 
-    def compose_sql(self, sql, params):
+    def quote_value(self, value):
+        if isinstance(value, str):
+            value = value.replace("%", "%%")
+        return value
+
+    def compose_sql(self, sql, params, quote_params=False):
+        if quote_params and params:
+            params = [self.quote_value(param) for param in params]
         return mogrify(sql, params, self.connection)
 
     def set_time_zone_sql(self):

Patryk, does it work for you?

[David Sanders]

I've confirmed Simon's patch works with this test:

tests/postgres_tests/test_search.py

@@ -161 +161 @@
         )
         self.assertNotIn("COALESCE(COALESCE", str(searched.query))
 
+    def test_values_with_percent(self):
+        searched = Line.objects.annotate(
+            search=SearchVector(Value("This week everything is 10% off"))
+        ).filter(search="10 % off")
+        self.assertEqual(len(searched), 9)
+
 
 class SearchConfigTests(PostgreSQLSimpleTestCase):
     def test_from_parameter(self):

[Mariusz Felisiak]

Replying to David Sanders:

I've confirmed Simon's patch works with this test

Thanks :+1: I'm not Simon ;)

[Phil Gyford]

This is probably the same issue, but wanted to check it wasn't only tangentially related and so missed.

If I have a model with a field like:

search_document = SearchVectorField(null=True)

Then I try to update that for object 123 by doing:

MyModel.objects.filter(pk=123).update(search_document=SearchVector(Value('10%')))

then with psycopg2 I get "IndexError: tuple index out of range".

and with psycopg3 I get "ProgrammingError: only '%s', '%b', '%t' are allowed as placeholders, got '%"

If I omit the % then there's no error.

[Florian Apolloner]

I am a bit confused, shouldn't compose_sql (and then mogrify) do that automatically? ie is this a psycopg bug?

[Florian Apolloner]

Unfortunately, it's sometimes used together with uncomposed SQL statements causing issues when binding parameters. IMO, we should escape % when we know that the parameters are bound later, e.g.

Ugh, we should probably not use compose_sql then

[Mariusz Felisiak]

Replying to Florian Apolloner:

Unfortunately, it's sometimes used together with uncomposed SQL statements causing issues when binding parameters. IMO, we should escape % when we know that the parameters are bound later, e.g.

Ugh, we should probably not use compose_sql then

Escaping % sounds like a reasonable compromise to me, we already do this in schema editors, e.g. in ​MySQL or ​PostgreSQL. Also, we avoid side-effects by providing the quote_params argument.

[Florian Apolloner]

Yes, but the schema editor has generally been a thing where we know we do suboptimal things and we have the generated SQL under control. Here we have (in the worst case) user input and it simply feels like opening a can of worms if we are not able to distinguish between parameters and the sql iteself clearly.

[Mariusz Felisiak]

Replying to Florian Apolloner:

Yes, but the schema editor has generally been a thing where we know we do suboptimal things and we have the generated SQL under control. Here we have (in the worst case) user input and it simply feels like opening a can of worms if we are not able to distinguish between parameters and the sql iteself clearly.

Alternatively, we can remove compose_sql() from SearchVector (it was added in ​f83ee2a23ee28a271a50a005b62259d735fbea3f) but looks unnecessary now, see draft ​PR. We can add test for an index with SearchVector to confirm this.

[Florian Apolloner]

Lovely, if that passes the testsuite that would be certainly preferred. Might be that follow up changes made this unnecessary

[Mariusz Felisiak <felisiak.mariusz@…>]

In 4bf4222:

Fixed #34459 -- Fixed SearchVector() crash for parameters with % symbol.

Thanks Patryk Zawadzki for the report.

Regression in 09ffc5c1212d4ced58b708cbbf3dfbfb77b782ca.

[Mariusz Felisiak <felisiak.mariusz@…>]

In db49def5:

[4.2.x] Fixed #34459 -- Fixed SearchVector() crash for parameters with % symbol.

Thanks Patryk Zawadzki for the report.

Regression in 09ffc5c1212d4ced58b708cbbf3dfbfb77b782ca.

Backport of 4bf4222010fd8e413963c6c873e4088614332ef9 from main