added support and test cases for psycopg3 #1737
Conversation
|
there's typos in tile and description: it's 'psycopg' and not 'psycorg' |
| @@ -205,7 +214,7 @@ def _record(self, method, sql, params): | |||
| # case where Django can start a transaction before the first query | |||
| # executes, so in that case logger.current_transaction_id() will | |||
| # generate a new transaction ID since one does not already exist. | |||
| final_conn_status = conn.status | |||
There was a problem hiding this comment.
Seems like the old logic was based on connection status,
https://www.psycopg.org/docs/extensions.html#connection-status-constants
however according to the new api in psycopg3 connection.info.status we only have 2 states
https://www.psycopg.org/psycopg3/docs/api/pq.html#psycopg.pq.ConnStatus
Since STATUS_IN_TRANSACTION is never in any of the options of connection.info.status the test tests.panels.test_sql.SQLPanelMultiDBTestCase.test_transaction_status will always fail. How should we based the logic for the new states of psycopg3? should we rely on conn.info.transaction_status instead of connection.info.status
…status as it works with pyscorg3
Co-authored-by: Matthias Kestenholz <mk@feinheit.ch>
tox.ini
Outdated
| py{310}-dj{40}-{sqlite} | ||
| py{310,311}-dj{41,42,main}-{sqlite,postgresql,postgis,mysql} | ||
| py{310,311}-dj{40,41}-{sqlite,legacy_postgresql,postgis,mysql} |
There was a problem hiding this comment.
Now you're adding back many Django 4.0 test runs, those have been removed on purpose in #1719. That's not better.
There was a problem hiding this comment.
my bad, should be good this time
There was a problem hiding this comment.
It seems that github actions still tries to run python3.8 with django 4.2a
https://github.com/jazzband/django-debug-toolbar/actions/runs/4113633936/jobs/7102476809#step:9:26
can you help adjust the test to prevent that?
tests/panels/test_sql.py
Outdated
| if is_psycopg3: | ||
| list( | ||
| PostgresJSON.objects.raw( | ||
| "SELECT * FROM tests_postgresjson WHERE field ->> 'key' = ANY(%s)", | ||
| [["a", "b'"]], | ||
| ) | ||
| ) |
There was a problem hiding this comment.
Sorry, we had a misunderstanding. This block does not fulfill the purpose of the test. If psycopg3 doesn't support tuples as a parameter, this test should fail expectedly. However, it doesn't look like there's a expectedFailureIf decorator, so maybe we update the skipUnless to include confirming it's psycopg2?
There was a problem hiding this comment.
was not sure how to check if we have psycopg3
so i relied on the method used with django to detect psycopg version
https://github.com/django/django/blob/main/django/db/backends/postgresql/base.py#L24
and combined it with self.skipTest
let me know if there is a better way to skip the test
There was a problem hiding this comment.
@nofalx can you enable maintainers to edit the PR? I'd like to push a change. It should be on the right side near the notifications button. https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/allowing-changes-to-a-pull-request-branch-created-from-a-fork
|
closed in favor for #1739 as the option "Allow edits by maintainers |
Description
Add support for psycopg3 while still keeping support for psycopg2
Fixes #1736
Checklist:
docs/changes.rst.tests.panels.test_sql.SQLPanelMultiDBTestCase.test_transaction_status