Opened 12 months ago

Closed 12 months ago

Last modified 12 months ago

#34486 closed Bug (fixed)

SearchHeadline crashes without an active connection.

Reported by: Scott Macpherson Owned by: Scott Macpherson
Component: contrib.postgres Version: 4.2
Severity: Release blocker Keywords:
Cc: Florian Apolloner, Daniele Varrazzo Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Following an upgrade to Django 4.2 and psycopg3, any use of SearchVector has started returning AttributeError "'NoneType' object has no attribute 'pgconn'".

It seems to be because the instance of DatabaseWrapper passed in to as_sql doesn't have a connection: DatabaseOperations.compose_sql calls mogrify, which in turn calls .connection on the DatabaseWrapper, which returns NoneType.

I've verified that calling connection.connect() in search.py immediately before the compose_sql line resolves this, but I don't know nearly enough to know if this is the real root problem, or where in the stack the connection is supposed to be created if it is.

Attachments (1)

traceback.txt (6.0 KB ) - added by Scott Macpherson 12 months ago.
Traceback

Download all attachments as: .zip

Change History (16)

by Scott Macpherson, 12 months ago

Attachment: traceback.txt added

Traceback

comment:1 by Tim Graham, 12 months ago

Type: UncategorizedBug

I guess it's fixed by 4bf4222010fd8e413963c6c873e4088614332ef9 which removed compose_sql()?

comment:2 by Scott Macpherson, 12 months ago

That commit does indeed resolve this issue. While checking that out I discovered that SearchHeadline.as_sql suffers from the same issue. Should I log a separate bug for that?

in reply to:  2 comment:3 by Mariusz Felisiak, 12 months ago

Resolution: duplicate
Status: newclosed

Replying to Scott Macpherson:

While checking that out I discovered that SearchHeadline.as_sql suffers from the same issue. Should I log a separate bug for that?

Does it? In SearchHeadline.as_sql() composed options are passed in params, so it should not be affected.

Duplicate of #34459.

comment:4 by Scott Macpherson, 12 months ago

The trouble doesn't seem to be with the structure of the call to compose_sql, but it's called on a DatabaseOperations object with a DatabaseWrapper that doesn't have a connection. When mogrify retrieves that connection it gets a NoneType back.

I can't work out why the tests in SearchHeadlineTests don't capture this - could it have something to do with the way connections are reused in the tests, but not for a cold request?

…
  File "/Users/scott/Development/python_venvs/zerosleeps/lib/python3.11/site-packages/django/contrib/postgres/search.py", line 320, in as_sql
    ", ".join(
    ^
  File "/Users/scott/Development/python_venvs/zerosleeps/lib/python3.11/site-packages/django/contrib/postgres/search.py", line 321, in <genexpr>
    connection.ops.compose_sql(f"{option}=%s", [value])
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/scott/Development/python_venvs/zerosleeps/lib/python3.11/site-packages/django/db/backends/postgresql/operations.py", line 205, in compose_sql
    return mogrify(sql, params, self.connection)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/scott/Development/python_venvs/zerosleeps/lib/python3.11/site-packages/django/db/backends/postgresql/psycopg_any.py", line 21, in mogrify
    return ClientCursor(connection.connection).mogrify(sql, params)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/scott/Development/python_venvs/zerosleeps/lib/python3.11/site-packages/psycopg/cursor.py", line 672, in __init__
    super().__init__(connection)
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/scott/Development/python_venvs/zerosleeps/lib/python3.11/site-packages/psycopg/cursor.py", line 65, in __init__
    self._pgconn = connection.pgconn
                   ^^^^^^^^^^^^^^^^^

Exception Type: AttributeError at /search/
Exception Value: 'NoneType' object has no attribute 'pgconn'

in reply to:  4 comment:5 by Mariusz Felisiak, 12 months ago

Replying to Scott Macpherson:

I can't work out why the tests in SearchHeadlineTests don't capture this - could it have something to do with the way connections are reused in the tests, but not for a cold request?

I'm not sure how it is possible that you reach this point without a connection. Can you provide a regression test or a small project that reproduces the issue?

comment:6 by Scott Macpherson, 12 months ago

I've knocked up a little project which demonstrates the issue:

https://github.com/scottmacpherson/postgres_search

comment:7 by Mariusz Felisiak, 12 months ago

Cc: Florian Apolloner Daniele Varrazzo added
Resolution: duplicate
Severity: NormalRelease blocker
Status: closednew
Summary: SearchVector.as_sql causes AttributeErrorSearchHeadline crashes without an active connection.
Triage Stage: UnreviewedAccepted

Great catch! We should ensure an active connection in mogrify() with psycopg version 3, e.g.

  • django/db/backends/postgresql/psycopg_any.py

    diff --git a/django/db/backends/postgresql/psycopg_any.py b/django/db/backends/postgresql/psycopg_any.py
    index 579104dead..1fe6b15caf 100644
    a b try:  
    1818    TSTZRANGE_OID = types["tstzrange"].oid
    1919
    2020    def mogrify(sql, params, connection):
    21         return ClientCursor(connection.connection).mogrify(sql, params)
     21        with connection.cursor() as cursor:
     22            return ClientCursor(cursor.connection).mogrify(sql, params)
    2223
    2324    # Adapters.
    2425    class BaseTzLoader(TimestamptzLoader):

Would you like to prepare a patch (via GitHub PR)? a regression test is required.

comment:8 by Scott Macpherson, 12 months ago

I'd be happy to. Are you able to give me some pointers on replicating the behaviour we're seeing from inside TestCase?

I've been mucking about with TransactionTestCase rather than TestCase, and I can get things into a state where connection.connection inside mogrify returns an idle connection rather than one that's in a transaction, but that's not far enough to replicate this.

in reply to:  8 comment:9 by Mariusz Felisiak, 12 months ago

Replying to Scott Macpherson:

I'd be happy to. Are you able to give me some pointers on replicating the behaviour we're seeing from inside TestCase?

I've been mucking about with TransactionTestCase rather than TestCase, and I can get things into a state where connection.connection inside mogrify returns an idle connection rather than one that's in a transaction, but that's not far enough to replicate this.

Since the real issue is in compose_sql(), I'd add a backend-specific test, e.g.

  • tests/backends/postgresql/tests.py

    diff --git a/tests/backends/postgresql/tests.py b/tests/backends/postgresql/tests.py
    index 9c4512be24..82df344139 100644
    a b class Tests(TestCase):  
    420420        with self.assertRaisesMessage(NotSupportedError, msg):
    421421            connection.check_database_version_supported()
    422422        self.assertTrue(mocked_get_database_version.called)
     423
     424    def test_compose_sql_no_connection(self):
     425        new_connection = connection.copy()
     426        # compose_sql() should open a new connection.
     427        try:
     428            self.assertEqual(new_connection.ops.compose_sql("SELECT %s", ["test"]), "SELECT 'test'")
     429        finally:
     430            new_connection.close()

comment:10 by Scott Macpherson, 12 months ago

Owner: set to Scott Macpherson
Status: newassigned

comment:11 by Scott Macpherson, 12 months ago

Thanks for your help - you made it too easy! Pull request 16760 created: https://github.com/django/django/pull/16760.

backends.postgres has one new regression test - that suite had one skip before and the same skip after this change. tests.postgres_tests produces the same output before and after this change as well.

comment:12 by Scott Macpherson, 12 months ago

Has patch: set
Last edited 12 months ago by Mariusz Felisiak (previous) (diff)

comment:13 by Mariusz Felisiak, 12 months ago

Triage Stage: AcceptedReady for checkin

comment:14 by Mariusz Felisiak <felisiak.mariusz@…>, 12 months ago

Resolution: fixed
Status: assignedclosed

In 53aee470:

Fixed #34486 -- Fixed DatabaseOperations.compose_sql() crash with no existing database connection on PostgreSQL.

Regression in 09ffc5c1212d4ced58b708cbbf3dfbfb77b782ca.

comment:15 by Mariusz Felisiak <felisiak.mariusz@…>, 12 months ago

In 090d5cc:

[4.2.x] Fixed #34486 -- Fixed DatabaseOperations.compose_sql() crash with no existing database connection on PostgreSQL.

Regression in 09ffc5c1212d4ced58b708cbbf3dfbfb77b782ca.

Backport of 53aee470d5b35e2708864d5221d2b5655e10c091 from main

Note: See TracTickets for help on using tickets.
Back to Top