Opened 4 years ago

Closed 4 years ago

Last modified 4 years 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: Türker Sezer, 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 Türker Sezer 4 years ago.
show warning if there isn't any file for a label.
18990_tests.path (4.9 KB) - added by Roman Gladkov 4 years ago.
Some modifications and tests for the path
18990_tests.patch (4.9 KB) - added by Roman Gladkov 4 years ago.

Download all attachments as: .zip

Change History (18)

comment:1 Changed 4 years ago by Preston Holmes

Component: Core (Other)Core (Management commands)
Triage Stage: UnreviewedAccepted

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

Changed 4 years ago by Türker Sezer

Attachment: 18990.patch added

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

comment:2 Changed 4 years ago by Türker Sezer

Cc: Türker Sezer added
Has patch: set

Please check attached patch for an easy fix.

comment:3 Changed 4 years ago by Claude Paroz

Needs tests: set

Changed 4 years ago by Roman Gladkov

Attachment: 18990_tests.path added

Some modifications and tests for the path

Changed 4 years ago by Roman Gladkov

Attachment: 18990_tests.patch added

comment:4 Changed 4 years ago by Roman Gladkov

Cc: d1fffuz0r@… added

bump

comment:5 Changed 4 years ago by Preston Holmes

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 4 years ago by Roman Gladkov

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 4 years ago by Roman Gladkov (previous) (diff)

comment:7 Changed 4 years ago by senko

Owner: changed from nobody to senko
Status: newassigned

comment:8 Changed 4 years 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 4 years ago by Ilkka Hakkari

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 4 years ago by senko

Updated the pull request according to the comments by derega.

comment:11 Changed 4 years 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 4 years ago by Tomek Paczkowski

Triage Stage: AcceptedReady for checkin

Looks good to me.

comment:13 Changed 4 years ago by Senko Rasic <senko.rasic@…>

Resolution: fixed
Status: assignedclosed

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 4 years 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 4 years 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

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