Opened 3 years ago

Last modified 2 years ago

#33685 new New feature

Support using service names for tests on PostgreSQL.

Reported by: Shane Ambler Owned by: nobody
Component: Testing framework Version: 4.0
Severity: Normal Keywords:
Cc: Hasan Ramezani, Carlton Gibson Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

In django 4.0 the postgresql DATABASE settings were changed to allow use of a .pg_service.conf and .pgpass file being defined in the OPTIONS value.

Using the old config works fine in both runserver and test.

Using the new config works for runserver but fails for test.

In settings I have :-

DATABASES = {
    'default': {
        'ENGINE': 'django.db.backends.postgresql',
        'OPTIONS': {
            'service': 'yfsm_pgdb',
            'passfile': '.yfsm_pgpass',
        },
    },
}

I have the user password entry allowing login to any database (using *) I have also tried adding an entry with test_dbname. I also tried adding an entry in .pg_service.conf as [test_yfsm_pgdb]

running tests I see django.db.utils.OperationalError: fe_sendauth: no password supplied indicating that the password file is not used.

It would appear that when testing, the new OPTIONS settings are ignored.

Change History (18)

comment:1 by Shane Ambler, 3 years ago

Looking at the pull request for the OPTIONS change, I find that I can use the service name django_test. This does not appear to be documented anywhere.

While I wouldn't initially agree with using one login for testing all projects, use of this pg service name needs to be documented.

comment:2 by Shane Ambler, 3 years ago

Component: Testing frameworkDocumentation

comment:3 by Shane Ambler, 3 years ago

Component: DocumentationTesting framework

comment:4 by Shane Ambler, 3 years ago

I failed to get my setting file right when I checked that, the use of the django_test service name is part of the django project tests.

The use of postgresql service name still appears broken with testing

comment:5 by Mariusz Felisiak, 3 years ago

Resolution: invalid
Status: newclosed

If you want to user PostgreSQL's password files, all required databases must be included, i.e. postgres, dbname, test_dbname, e.g. the following configuration works for me

localhost:5432:postgres:postgres_username:SeCrEtPassworD
localhost:5432:dbname:postgres_username:SeCrEtPassworD
localhost:5432:test_dbname:postgres_username:SeCrEtPassworD

settings.py:

DATABASES = {
    'default': {
        'ENGINE': 'django.db.backends.postgresql',
        'HOST': 'localhost',
        'PORT': 5432,
        'NAME': 'dbname',
        'USER': 'postgres_username',
        'OPTIONS': {
            'passfile': '.custompgpass',
        }
    },
}

in reply to:  5 ; comment:6 by Shane Ambler, 3 years ago

Replying to Mariusz Felisiak:

If you want to user PostgreSQL's password files, all required databases must be included, i.e. postgres, dbname, test_dbname, e.g. the following configuration works for me

localhost:5432:postgres:postgres_username:SeCrEtPassworD
localhost:5432:dbname:postgres_username:SeCrEtPassworD
localhost:5432:test_dbname:postgres_username:SeCrEtPassworD

Yes, I tried both the test_dbname and * to match any dbname and it still fails.

settings.py:

DATABASES = {
    'default': {
        'ENGINE': 'django.db.backends.postgresql',
        'HOST': 'localhost',
        'PORT': 5432,
        'NAME': 'dbname',
        'USER': 'postgres_username',
        'OPTIONS': {
            'passfile': '.custompgpass',
        }
    },
}

While you are maintaining both old and new config variations, all will continue to work for you.

If you remove the old, no longer documented, settings of HOST PORT NAME USER then your setup will work for runserver but not test

The pg docs https://docs.djangoproject.com/en/4.0/ref/databases/#postgresql-notes-1 only documents the new method of specifying service and passfile in the OPTIONS - this documented setup does not support running tests with postgresql databases.

comment:7 by Shane Ambler, 3 years ago

Resolution: invalid
Status: closednew

in reply to:  6 comment:8 by Mariusz Felisiak, 3 years ago

Summary: Using new postgresql settings fails when running testsTests crash when using only a service name on PostgreSQL.
Triage Stage: UnreviewedAccepted

While you are maintaining both old and new config variations, all will continue to work for you.

If you remove the old, no longer documented, settings of HOST PORT NAME USER then your setup will work for runserver but not test
The pg docs https://docs.djangoproject.com/en/4.0/ref/databases/#postgresql-notes-1 only documents the new method of specifying service and passfile in the OPTIONS ...

Using ['OPTIONS']['service'] and ['OPTIONS']['passfile'] is not a "new" or only supported way for the DATABASES settings. It's a PostgreSQL-specific option that you may use that's why it's documented only in the "PostgreSQL notes". Using HOST, PORT, NAME, USER, and PASSWORD is still fully supported for PostgreSQL, see docs.

... this documented setup does not support running tests with postgresql databases.

NAME is used in many places for test setup, including cloning a test database, that's why it doesn't work with only service specified.

comment:9 by Shane Ambler, 3 years ago

Replying to Mariusz Felisiak:

Using ['OPTIONS']['service'] and ['OPTIONS']['passfile'] is not a "new" or only supported way for the DATABASES settings.

It looks like it has been available in "main" for a bit over a year, but it is still "new" as it has only been available in one public release. Yes, the old (or generic) method still works and will work properly if it is the only config docs that you find.

Using ['DATABASE']['NAME'] and ['DATABASE']['USER'] will work for runserver as well as test and they will override any service file settings.

We shouldn't need to duplicate login details in two places to get basic functionality.

I see having tests fail on the first tutorial as a big entry barrier for new users.

comment:10 by Mariusz Felisiak, 3 years ago

Severity: NormalRelease blocker

It looks like it has been available in "main" for a bit over a year, but it is still "new" as it has only been available in one public release.

Right, good point, it's a new feature so this ticket should be marked as a release blocker.

comment:11 by Shane Ambler, 3 years ago

Just a suggestion for whoever looks into this :

I expect that the service name is being passed to psycopg2, making altering the dbname awkward, in that case I think prepending 'test_' to the service name would be a viable solution. Then the docs would show two service entries to support testing in the example.

in reply to:  11 comment:12 by Mariusz Felisiak, 3 years ago

Replying to Shane Ambler:

Just a suggestion for whoever looks into this :

I expect that the service name is being passed to psycopg2, making altering the dbname awkward, in that case I think prepending 'test_' to the service name would be a viable solution. Then the docs would show two service entries to support testing in the example.

Agreed, I was thinking about the same solution. Unfortunately it's quite tricky to implement this duality :|

comment:13 by Mariusz Felisiak, 3 years ago

Cc: Hasan Ramezani Carlton Gibson added

I've tried to implement this but all mechanism related with creating/cloning test databases are based on the database name. It's feasible, however, my draft patch is quite complex and the risk of introducing regressions in a stable version is too great, IMO. I'd document that service names cannot be currently used for testing purposes and treat this as a new feature request.

For example:

  • docs/ref/databases.txt

    diff --git a/docs/ref/databases.txt b/docs/ref/databases.txt
    index c270f56942..ca6362a7a6 100644
    a b password from the `password file`_, you must specify them in the  
    165165    Support for connecting by a service name, and specifying a password file
    166166    was added.
    167167
     168.. warning::
     169
     170    Using a service name for testing purposes is not supported. This
     171    :ticket:`may be implemented later <33685>`.
     172
    168173Optimizing PostgreSQL's configuration
    169174-------------------------------------
    170175

comment:14 by Hasan Ramezani, 3 years ago

however, my draft patch is quite complex and the risk of introducing regressions in a stable version is too great,

Agree with you.

I'd document that service names cannot be currently used for testing purposes and treat this as a new feature request.

Sound good to me.

comment:15 by Mariusz Felisiak, 3 years ago

Severity: Release blockerNormal
Summary: Tests crash when using only a service name on PostgreSQL.Support using service names for tests on PostgreSQL.
Type: BugNew feature

comment:16 by GitHub <noreply@…>, 3 years ago

In 64748016:

Refs #33685 -- Doc'd that using PostgreSQL's service names for testing purposes is not supported.

comment:17 by Mariusz Felisiak <felisiak.mariusz@…>, 3 years ago

In de9c08c0:

[4.0.x] Refs #33685 -- Doc'd that using PostgreSQL's service names for testing purposes is not supported.

Backport of 647480166bfe7532e8c471fef0146e3a17e6c0c9 from main

comment:18 by Joshua Lusk, 2 years ago

Was messing around with this and found the immediate issue to be that the .pop() call in get_connection_params() was destructive to self.settings_dict, since assigning settings_dict = self.settings_dict means both still reference the same underlying dictionary. This means that after _nodb_cursor is done creating the test database, acquiring the normal cursor will fail since ['OPTIONS']['service'] has been removed from [OPTIONS] entirely. I was able to get tests to work by removing the destructive call to .pop() and specifying service as None specifically in the conn_params for this case:

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

    diff --git a/django/db/backends/postgresql/base.py b/django/db/backends/postgresql/base.py
    index 2758c402ab..be1c05f313 100644
    a b class DatabaseWrapper(BaseDatabaseWrapper):  
    194194            }
    195195        elif settings_dict["NAME"] is None:
    196196            # Connect to the default 'postgres' db.
    197             settings_dict.get("OPTIONS", {}).pop("service", None)
    198             conn_params = {"database": "postgres", **settings_dict["OPTIONS"]}
     197            conn_params = {"database": "postgres", "service": None, **settings_dict["OPTIONS"]}
    199198        else:
    200199            conn_params = {**settings_dict["OPTIONS"]}
    201200

But to Mariusz Felisiak's point:

I've tried to implement this but all mechanism related with creating/cloning test databases are based on the database name. It's feasible, however, my draft patch is quite complex and the risk of introducing regressions in a stable version is too great, IMO. I'd document that service names cannot be currently used for testing purposes and treat this as a new feature request.

There might be more to this than meets the eye, especially if their are code paths other than the instance of DatabaseWrapper created by _nodb_cursor that use this branch of get_connection_params() that I am not aware of.

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