Code

Opened 22 months ago

Closed 14 months ago

Last modified 14 months ago

#18990 closed Bug (fixed)

'manage.py loaddata' does not report if the loading file does not exist

Reported by: sergzach Owned by: senko
Component: Core (Management commands) Version: 1.4
Severity: Normal Keywords: loaddata
Cc: dirigeant, d1fffuz0r@… Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

In the next case the file ProductCategory2.json does not exist but the loaddata utility does not report about it. It reports that 0 objects installed from 0 fixtures instead.

v11162:/www/lavka/dargent# python manage.py loaddata /root/models/ProductCategory2.json
Installed 0 object(s) from 0 fixture(s)

There are files in /root/lavka/:

v11162:/www/lavka/dargent# ls /root/models/
Country.json  Producer.json  ProductCategory.json

Attachments (3)

18990.patch (1.5 KB) - added by dirigeant 22 months ago.
show warning if there isn't any file for a label.
18990_tests.path (4.9 KB) - added by d1ffuz0r 21 months ago.
Some modifications and tests for the path
18990_tests.patch (4.9 KB) - added by d1ffuz0r 21 months ago.

Download all attachments as: .zip

Change History (18)

comment:1 Changed 22 months ago by ptone

  • Component changed from Core (Other) to Core (Management commands)
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

While the "0 fixture(s)" is accurate - I agree a more helpful message/error could be displayed.

Changed 22 months ago by dirigeant

show warning if there isn't any file for a label.

comment:2 Changed 22 months ago by dirigeant

  • Cc dirigeant added
  • Has patch set

Please check attached patch for an easy fix.

comment:3 Changed 22 months ago by claudep

  • Needs tests set

Changed 21 months ago by d1ffuz0r

Some modifications and tests for the path

Changed 21 months ago by d1ffuz0r

comment:4 Changed 21 months ago by d1ffuz0r

  • Cc d1fffuz0r@… added

bump

comment:5 follow-up: Changed 21 months ago by ptone

  • Patch needs improvement set

I think we only want to raise an error when a single fixture file was specified no valid fixture files were found in a dir - which I believe means checking at this point:

if os.path.isabs(fixture_name):
    fixture_dirs = [fixture_name]

Since we have specified a single file - and it does not exist - raising an actual CommandError is probably what is desired. The tests should check that this is raised using assertRaises.

comment:6 in reply to: ↑ 5 Changed 21 months ago by d1ffuz0r

Hmm. Every time the commands "syncdb" and "flush" trying to load the 'initial_data' fixture, how be with it. Ignore? Because that be generate the errors. Setup "--no-initial-data" default value to False can break backward compatibilities

django/core/management/commands/syncdb.py:

        # Load initial_data fixtures (unless that has been disabled)
        if load_initial_data:
            call_command('loaddata', 'initial_data', verbosity=verbosity,
                         database=db, skip_validation=True)


Sorry for my English

Last edited 21 months ago by d1ffuz0r (previous) (diff)

comment:7 Changed 14 months ago by senko

  • Owner changed from nobody to senko
  • Status changed from new to assigned

comment:8 Changed 14 months ago by senko

  • Needs tests unset
  • Patch needs improvement unset

@ptone: that would still hide an error when you specify a single fixture (but not an absolute path), eg: ./manage.py loaddata myfixture.json (it's not an absolute path and wouldn't register here).

Reading the source, it seems to me that the name "initial_data" is special to Django (syncdb and flush hardcode it), so a solution I tried was to not raise the CommandError if the fixture name is "initial_data". I don't like the part that it's now hardcoded in another place - is there a good place to put this in, except settings (since we probably don't want it to be user-settable).

Another possible solution would be to raise the error unconditionally and catch it in flush/syncdb, but that would catch *any* CommandError loaddata might raise, which is again probably not what we want.

Based on the above, I think this approach makes sense - here's a pull request with a fix and updated tests for it: https://github.com/django/django/pull/1134

Please let me know if this makes sense.

comment:9 Changed 14 months ago by derega

PR 1134 breaks current functionality. If the list of files is generated with a script this now bails out on first not found file. It might be a good idea, but nonetheless it is backward incompatible change.

I think it would be better to just list the not found files and continue.

comment:10 Changed 14 months ago by senko

Updated the pull request according to the comments by derega.

comment:11 Changed 14 months ago by senko

Another good comment by derega via IRC: the output from the warnings looks a bit unfriendly:

(django-dev)[tachyon:djangotest]$ python manage.py loaddata asd.json as.json
/home/senko/.virtualenvs/django-dev/lib/python2.7/django/core/management/commands/loaddata.py:173: UserWarning: No fixture named 'asd' found.
  warnings.warn("No fixture named '%s' found." % fixture_name)

/home/senko/.virtualenvs/django-dev/lib/python2.7/django/core/management/commands/loaddata.py:173: UserWarning: No fixture named 'as' found.
  warnings.warn("No fixture named '%s' found." % fixture_name)

Installed 0 object(s) from 0 fixture(s)

comment:12 Changed 14 months ago by oinopion

  • Triage Stage changed from Accepted to Ready for checkin

Looks good to me.

comment:13 Changed 14 months ago by Senko Rasic <senko.rasic@…>

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

In cc3b3ba93a7bfdd2ece739e97e36150a719acd3e:

Fixed #18990: Loaddata now complains if fixture doesn't exist

The fixture named "initial_data" is exceptional though; if it
doesn't exist, the error is not raised. This allows syncdb and
flush management commands to attempt to load it without causing
an error if it doesn't exist.

comment:14 Changed 14 months ago by Senko Rasic <senko.rasic@…>

In c44a2c40fe0ed79b0fa00233a204d41e9c677750:

Fixed #18990 -- Loaddata now complains if fixture doesn't exist

If the fixture doesn't exist, loaddata will output a warning.

The fixture named "initial_data" is exceptional though; if it
doesn't exist, the warning is not emitted. This allows syncdb and
flush management commands to attempt to load it without causing
spurious warnings.

Thanks to Derega, ptone, dirigeant and d1ffuz0r for contributions
to the ticket.

comment:15 Changed 14 months ago by Andrew Godwin <andrew@…>

In 7a99d1e167f81c5fcd1b9f7f548c5d6959cef2ef:

Merge pull request #1134 from senko/ticket_18990

Fixed #18990: Loaddata now complains if fixture doesn't exist

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.