Opened 6 years ago

Closed 6 years ago

#29133 closed Cleanup/optimization (fixed)

call_command() fails if a required option is passed in via **options

Reported by: Alex Tomic Owned by: Alex Tomic
Component: Core (Management commands) Version: 2.0
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

If call_command is called with a required option, e.g;

management.call_command('required_option',  "--need-me=asdf",  "--need-me-too=fdsa")
management.call_command('required_option', need_me="asdf", need_me_too="fdsa")

The first call succeeds, but the second fails with an exception:

django.core.management.base.CommandError: Error: the following arguments are required: -n/--need-me, -t/--need-me-too

The problem is due to parse_args being called in call_command without checking for potentially required arguments in **options

Change History (6)

comment:1 by Alex Tomic, 6 years ago

Owner: changed from nobody to Alex Tomic

First time contributor, pardon for any newbie mistakes in the process!

This bug has an easy workaround, but it cost me enough time today that I thought I would finally try to get my hands dirty in the code. PR has been created here: https://github.com/django/django/pull/9701

comment:2 by Tim Graham, 6 years ago

Summary: django.core.management.call_command fails if required option passed in via **optionscall_command() fails if a required option is passed in via **options
Type: UncategorizedCleanup/optimization

I'm not sure if the additional code complexity is worth supporting this. I noted this recommendation from the Python documentation, "Required options are generally considered bad form because users expect options to be optional, and thus they should be avoided when possible." An alternative could be to document the limitation.

in reply to:  2 comment:3 by Alex Tomic, 6 years ago

Replying to Tim Graham:

I'm not sure if the additional code complexity is worth supporting this. I noted this recommendation from the Python documentation, "Required options are generally considered bad form because users expect options to be optional, and thus they should be avoided when possible." An alternative could be to document the limitation.

Thanks for the quick feedback.

I figured this would be a 1-2 line fix, but it turned out to be a bit more complex than I thought. I agree it's a bit unnecessary given that the python docs recommend against this practice. Ironically, I ran up against this as I was trying to wrap a few such unwieldy management commands doing exactly that.

I don't believe it is universally agreed upon that -/-- parameters should always be optional, though, outside of the Python world. Getopt_long [ 1 ] and the Open Group utility conventions [ 2 ], for example, don't recommend against it as far as I can tell.

All that said, if this patch seems too risky to fix such a minor issue, I'd also be happy to contribute a documentation change instead.

[1] https://www.gnu.org/software/libc/manual/html_node/Getopt-Long-Options.html#Getopt-Long-Options
[2] http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap12.html e.g. Section 12.1.9

comment:4 by Tim Graham, 6 years ago

Patch needs improvement: set
Triage Stage: UnreviewedAccepted

After looking at the fix, I simplified it a bit and I think it's fine to fix this. I left comments for improvement on the PR.

comment:5 by Alex Tomic, 6 years ago

Patch needs improvement: unset

Thanks for the feedback. I've simplified the test case and supporting management command to only test what was not working before, and pushed up a new commit.

comment:6 by Tim Graham <timograham@…>, 6 years ago

Resolution: fixed
Status: assignedclosed

In a1a3e51:

Fixed #29133 -- Fixed call_command() crash if a required option is passed in options.

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