Fixed #29501 -- Allowed dbshell to pass options to underlying tool.

[adamchainz]

Ticket

[adamchainz]

Requested your review @claudep because you've reviewed a few dbshell PR's in the past :)

[claudep]

Looks great, added some comments.

[carltongibson]

Looks good. Couple of comments.

Aside: it seems sqlite3 has no way to run a command and exit: --cmd pops into the shell after running the supplied command. (That's not a problem with this PR.) Scrub that: it turns out not using --cmd at all works as expected. Doh.

[adamchainz]

I think this is ready for merge?

[felixxm]

@adamchainz I will take a look in this week 👀 , please be patient.

[felixxm]

@adamchainz Thanks for this patch 👍 I pushed edits to docs. What do you think about suppressing subprocess' exceptions?

[ngnpope]

I think suppressing the exception makes sense. Django is passing the arguments through and the error is raised by the external process which has already thrown messages to the console.

I was going to say reraising was not needed, but if scripted it could be handy to know which exact command failed.

[adamchainz]

Sorry about my impatience @felixxm 🙈

Agree with both that suppressing the exception makes sense. It already breaks some workflows before passing options through.

The reraise of CommandError would work, but I'm not sure it's necessary. I've instead changed the PR right now to use this pattern:

proc = subprocess.run(...)
sys.exit(proc.returncode)

This means Django is "transparent" around the command - it won't create any output but still exits with the same code. It's much more like its behaviour using os.exec* which was removed in bf5382c due to windows concerns. And then dbshell can (again) be a complete drop-in replacement in scripts.

[felixxm]

This means Django is "transparent" around the command - it won't create any output but still exits with the same code. It's much more like its behaviour using os.exec* which was removed in bf5382c due to windows concerns. And then dbshell can (again) be a complete drop-in replacement in scripts.

This approach breaks usage with call_command(), I think we need to stay with suppression subprocess.CalledProcessError's (or re-raise them as CommandError's).

[adamchainz]

This approach breaks usage with call_command(), I think we need to stay with suppression subprocess.CalledProcessError's (or re-raise them as CommandError's).

I am not sure use of dbshell with call_command is common. It would not have worked Django< 1.6 at least due to use of os.exec* previously, since that replaces the process and stops Python executing entirely. And anyway like this, one can catch SystemExit instead of CommandError.

I think it's important to preserve the return code, especially when adding the ability to use custom flags. Which return code was used can carry extra meaning. I've made the sys.exit conditional on the return code not being 0 so at least testing with except SystemExit catches abnormal conditions only.

[adamchainz]

I've also changed to FileNotFoundError in dbshell.py, since Python 3 differentiates between different kinds of OSError, and that's the one relating to missing commands.

[carltongibson]

Management commands are expected to raise CommandError:

raising CommandError exception (with a sensible description of the error) is the preferred way to indicate that something has gone wrong in the execution of a command.

I think we need to stick with that.

[adamchainz]

Okay done. To preserve the return code I added the returncode argument to CommandError, which also seems like it could be useful for other scripting sitautions.

[felixxm]

@adamchainz Thanks 👍 Please reorganize commits, we shouldn't mix (multiple) cleanups with a new feature.

[adamchainz]

Apologies for my unsplit commits. I think I've done it with good enough naming now.

[felixxm]

@adamchainz Thanks 👍 I added tests for missing dbshell executable (in a separate commit), added test for CommandError.returncode, and pushed minor edits.

[adamchainz]

All LGTM