Code

Opened 5 years ago

Closed 23 months ago

#10200 closed Bug (fixed)

loaddata command does not raise CommandError on errors

Reported by: lgs Owned by: nobody
Component: Core (Management commands) Version: master
Severity: Normal Keywords: exit status
Cc: yishaibeeri Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

When there is an error in a fixture being loaded by the command loaddata it prints an error description into stderr and them simple return. I think it would be better to raise a CommandError instead of simple exiting the handle method. My reasons are:

  • You can't easily know the succesfullnes of a loaddata command programmatically. For example, we have a buildbot that load some fixtures and we don't know if it passes this step or not since the exit code is always 0 no matter the data is successfully loaded or not.
  • AFAIK is the standard Django mechanism to report command errors
  • The code that catch this type of exceptions does already print its message to stderr, so we do not lose that functionality.

I'm attaching a patch that fixes this issues with existing tests already fixed.

Attachments (2)

scream-if-error-in-loaddata-command.diff (7.7 KB) - added by lgs 5 years ago.
10200-2.diff (15.8 KB) - added by claudep 2 years ago.
Raise CommandError in loaddata

Download all attachments as: .zip

Change History (18)

comment:1 Changed 5 years ago by lgs

  • Has patch set
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

comment:2 Changed 5 years ago by anonymous

  • milestone post-1.0 deleted

Milestone post-1.0 deleted

comment:3 Changed 5 years ago by jacob

  • Triage Stage changed from Unreviewed to Design decision needed

This is a backwards-incompatible change, so we need a very good reason to make this change. I'm not sure your reasons get that far, but I'll leave this open for some more discussion.

comment:4 Changed 5 years ago by lgs

What harm could do this patch to users other than alerting them that they used to have bad fixtures and now they know it?

The only real difference this patch introduces is that loaddata stops trying to load fixtures as soon as one of them is rotten. I think this is a good thing since the previous behaviour could hide obscure bugs in your data.

comment:5 Changed 5 years ago by pmocek

I noticed what is likely a related problem. When the loaddata
command is used with the auto-generated manage.py, bad syntax
results in silent failure and a return code indicating success.

The built-in usage help shows that the fixture argument is
mandatory:

Usage: manage.py loaddata [options] fixture [fixture ...]

However, manage.py does not report an error when a fixture is not
specified:

$ ./manage.py loaddata
$ echo $?
0

Only when the verbosity level is increased from 0 to 2 is an error
reported, and the return code still indicates success:

$ ./manage.py loaddata --verbosity=1 ; echo $?
0
$ ./manage.py loaddata --verbosity=2 ; echo $?
No fixtures found.
0

Typically, silence implies successful completion. This problem is
particularly troublesome because the syntax for loaddata differs
significantly from that of dumpdata. It took
outside assistance
for me -- new to Django but with a considerable amount of experience
using CLI utilities -- to determine why the following commands
completed with apparent success but left my database empty:

$ ./manage.py dumpdata >data.json
$ ./manage.ph flush
$ ./manage.py loaddata <data.json

comment:6 Changed 5 years ago by anonymous

  • Cc pmocek-django-tickets@… added

comment:7 Changed 5 years ago by pmocek

  • Cc pmocek-django-tickets@… removed

(noticed I receive e-mail updates after commenting regardless of having my address in the CC field)

comment:8 Changed 3 years ago by SmileyChris

  • Severity set to Normal
  • Type set to Bug

comment:9 Changed 3 years ago by julien

  • Patch needs improvement set

#6397, #11481 and #10849 were closed as dupes. See also related issue in #12007.

Also, the tests need to be updated to use unittest.

comment:10 Changed 3 years ago by jacob

  • Easy pickings unset
  • Resolution set to duplicate
  • Status changed from new to closed
  • UI/UX unset

#16026 would solve this one in a quite elegant way. Marking this one a duplicate of that.

comment:11 Changed 3 years ago by yishaibeeri

  • Cc yishaibeeri added
  • Component changed from Core (Serialization) to Core (Management commands)
  • Keywords exit status added
  • Resolution duplicate deleted
  • Status changed from closed to reopened
  • Triage Stage changed from Design decision needed to Accepted
  • Version changed from 1.0 to SVN

Reopening, as the fix for #16026 did not, eventually, fix this issue. In trunk (as of rev 17029) the exit status of a failed loaddata invocation (e.g., bad value in a fixture causing a DatabaseError) is still 0.

comment:12 Changed 2 years ago by ramiro

  • Needs tests set

Actually, loaddata code in its current status is such that all error code paths lead to:

  • An error is printed to stderr
  • The handle() method returns

So, the only (beneficial IMHO) change the modification proposed by this ticket would introduce would be that the Unix process exit code changes from zero to a non-zero value.

I suspect the backward-incompatibility reservations Jacob had back then were related to the fact that raising CommandError would mean exiting from the nested loops and aborting execution of the command; but the evolution of the code during the last three years has resulted in precisely that behavior already

But I'm not sure. Will try to confirm with him.

Last edited 2 years ago by ramiro (previous) (diff)

comment:13 Changed 2 years ago by anonymous

Any news about this ?

I've just hit this. We have a buildbot and when the loaddata fails, we cannot easily detect it. So, having run "set -e" in my bash doesn't help in this case, and then tests are run. The tests fail and the error was that loaddata failed, very hidden in the output.

If the exit code of the command could be just like any standard unix command (!=0 on error), that will be great and made life for sysadmins like me happier :)

Btw, we are using django 1.3.1 in case it matters

comment:14 Changed 2 years ago by claudep

When #18387 will be committed, we should be able to easily make a clean patch for this one.

Changed 2 years ago by claudep

Raise CommandError in loaddata

comment:15 Changed 2 years ago by claudep

  • Needs tests unset
  • Patch needs improvement unset

Here's an updated patch for this issue.

comment:16 Changed 23 months ago by Claude Paroz <claude@…>

  • Resolution set to fixed
  • Status changed from reopened to closed

In [6fd1950a4e29c1b5ed071a880db64103715ead51]:

Fixed #10200 -- Raised CommandError when errors happen in loaddata.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.