Opened 4 weeks ago

Closed 3 weeks ago

#35429 closed Cleanup/optimization (fixed)

Add argparse choices to --database options

Reported by: Adam Johnson Owned by: Jae Hyuck Sa
Component: Core (Management commands) Version: dev
Severity: Normal Keywords:
Cc: Mariusz Felisiak Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: yes UI/UX: no

Description

Many management commands take a --database option to select which database to operate on. Currently, this is unvalidated, causing crashes when a bad name is typed:

$ ./manage.py migrate --database deflaut
Traceback (most recent call last):
  ...
  File "/.../django/core/management/commands/migrate.py", line 100, in handle
    self.check(databases=[database])
  ...
  File "/.../django/db/models/fields/__init__.py", line 442, in _check_backend_specific_checks
    errors.extend(connections[alias].validation.check_field(self, **kwargs))
                  ~~~~~~~~~~~^^^^^^^
  File "/.../django/utils/connection.py", line 61, in __getitem__
    raise self.exception_class(f"The connection '{alias}' doesn't exist.")
django.utils.connection.ConnectionDoesNotExist: The connection 'deflaut' doesn't exist.

We can add argparse’s choices for validation:

--- django/core/management/commands/migrate.py
+++ django/core/management/commands/migrate.py
@@ -47,6 +47,7 @@ def add_arguments(self, parser):
         parser.add_argument(
             "--database",
             default=DEFAULT_DB_ALIAS,
+            choices=tuple(connections),
             help=(
                 'Nominates a database to synchronize. Defaults to the "default" '
                 "database."

The failure is then graceful, and lists the available options:

$ ./manage.py migrate --database deflaut
usage: manage.py migrate ...
manage.py migrate: error: argument --database: invalid choice: 'deflaut' (choose from 'default')

Change History (13)

comment:1 by Simon Charette, 4 weeks ago

Triage Stage: UnreviewedAccepted

comment:2 by Jae Hyuck Sa , 4 weeks ago

Last edited 4 weeks ago by Jae Hyuck Sa (previous) (diff)

comment:3 by Jae Hyuck Sa , 4 weeks ago

Owner: changed from nobody to Jae Hyuck Sa
Status: newassigned

comment:4 by Jae Hyuck Sa , 4 weeks ago

Has patch: set

comment:5 by Jae Hyuck Sa , 4 weeks ago

Last edited 4 weeks ago by Jae Hyuck Sa (previous) (diff)

comment:6 by Adam Johnson, 4 weeks ago

Patch needs improvement: set

in reply to:  6 comment:7 by Jae Hyuck Sa , 4 weeks ago

Replying to Adam Johnson:

As you told me, I applied them to the all relevant database options.

Last edited 4 weeks ago by Jae Hyuck Sa (previous) (diff)

comment:8 by Jae Hyuck Sa , 4 weeks ago

Patch needs improvement: unset

in reply to:  description comment:9 by Mariusz Felisiak, 4 weeks ago

Replying to Adam Johnson:

We can add argparse’s choices for validation:

--- django/core/management/commands/migrate.py
+++ django/core/management/commands/migrate.py
@@ -47,6 +47,7 @@ def add_arguments(self, parser):
         parser.add_argument(
             "--database",
             default=DEFAULT_DB_ALIAS,
+            choices=tuple(connections),
             help=(
                 'Nominates a database to synchronize. Defaults to the "default" '
                 "database."

Can we? Do we really want to iterate over connections? As far as I'm aware, this will force initialization of all database connections. I don't think it's acceptable. IMO it's not worth doing.

comment:10 by Simon Charette, 4 weeks ago

Do we really want to iterate over connections? As far as I'm aware, this will force initialization of all database connections

I know it's confusing (I had to check myself to make sure when accepting the ticket) but connections.__iter__ doesn't create any connections it simply iterates over settings.DATABASES.

comment:11 by Mariusz Felisiak, 4 weeks ago

Cc: Mariusz Felisiak added

comment:12 by Sarah Boyce, 3 weeks ago

Triage Stage: AcceptedReady for checkin

comment:13 by Sarah Boyce <42296566+sarahboyce@…>, 3 weeks ago

Resolution: fixed
Status: assignedclosed

In 4a76ac0:

Fixed #35429 -- Added argparse choices to --database options.

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