Opened 3 years ago

Closed 2 years ago

Last modified 2 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: 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 3 years ago.
show warning if there isn't any file for a label.
18990_tests.path (4.9 KB) - added by d1ffuz0r 3 years ago.
Some modifications and tests for the path
18990_tests.patch (4.9 KB) - added by d1ffuz0r 3 years ago.

Download all attachments as: .zip

Change History (18)

comment:1 Changed 3 years 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 3 years ago by dirigeant

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

comment:2 Changed 3 years ago by dirigeant

  • Cc dirigeant added
  • Has patch set

Please check attached patch for an easy fix.

comment:3 Changed 3 years ago by claudep

  • Needs tests set

Changed 3 years ago by d1ffuz0r

Some modifications and tests for the path

Changed 3 years ago by d1ffuz0r

comment:4 Changed 3 years ago by d1ffuz0r

  • Cc d1fffuz0r@… added

bump

comment:5 follow-up: Changed 3 years 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 3 years 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 3 years ago by d1ffuz0r (previous) (diff)

comment:7 Changed 2 years ago by senko

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

comment:8 Changed 2 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 2 years 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 2 years ago by senko

Updated the pull request according to the comments by derega.

comment:11 Changed 2 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 2 years ago by oinopion

  • Triage Stage changed from Accepted to Ready for checkin

Looks good to me.

comment:13 Changed 2 years 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 2 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 2 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