Opened 6 years ago

Closed 6 years ago

Last modified 6 years ago

#23699 closed Bug (fixed)

Migrate in unittest still loads initial data

Reported by: tony-zhu Owned by: nobody
Component: Core (Management commands) Version: 1.7
Severity: Normal Keywords:
Cc: Andrew Godwin 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

The migrate command in Django 1.7 does not load the initial_data fixtures as claims in the release note.

However it still loads the initial_data in unittests, which crashes the unittest when using some libraries (Django-helpdesk in my case).

Here is what I found:

When initializing unit tests, the
create_test_db in db/backends/creation calls the migration command with test_flush=True

The migrate command checks the test_flush in line 135. Then calls the flush command which loads the initial data.

Adding "load_initial_data=False" to the migrate command fixed it in my local test environment

Here is the section changed:

        # The test runner requires us to flush after a syncdb but before migrations,
        # so do that here.
        if options.get("test_flush", False):
            call_command(
                'flush',
                verbosity=max(self.verbosity - 1, 0),
                interactive=False,
                database=db,
                reset_sequences=False,
                inhibit_post_migrate=True,
                load_initial_data=False,
            )

Change History (10)

comment:1 Changed 6 years ago by Tim Graham

This looks like a duplicate of #23077, in which case it looks like it can't be fixed. The suggestion is simply to remove the initial data directory when you add migrations to your application. Will this work for you? Perhaps more documentation is needed.

comment:2 in reply to:  1 Changed 6 years ago by tony-zhu

It is a duplicate ticket.

But the 1.7 release does break the apps which have initial_data and want to be compatible with both Django 1.6+South and Django 1.7.

My pull request https://github.com/tony-zhu/django/commit/f533a949af2da4d2e8a59edade03772c7b74d0a4 at least makes the "flush" command do what it claims doing, that is only load initial_data for unmigrated apps.

My local test shows it will keep the behavior of unmigrated app as the old syncdb and ignore the initial_data for migrated apps. Which I think might be a better solution for the users to adapt the new migration framework.

comment:3 Changed 6 years ago by Tim Graham

Cc: Andrew Godwin added

Yes, I don't understand Andrew's rationale for doing nothing on the other ticket, "there is no way to prevent people mentioning models that are migrated in fixtures in unmigrated apps (Django doesn't segregate models during fixture loading)". Why do we need to prevent that? Adding him just to be sure.

comment:4 Changed 6 years ago by Andrew Godwin

The problem I was encountering was that people would have one initial_data migration for a project containing entries for all models in a project - not just the ones in the specific app (as it's common to just "dumpdata" to a single fixture file)

In particular, while you can make flush only load the initial_data from apps without migrations, you cannot prevent people from putting instances of models with migrations into those initial_data files.

Thus, while you can modify flush to only load unmigrated apps, it's of limited use in actually preventing the error occurring in the initial ticket. That said, I don't see why it can't be fixed, and then documented as such, it might be a little less surprising.

comment:5 Changed 6 years ago by Tim Graham

Has patch: set
Needs tests: set
Triage Stage: UnreviewedAccepted

comment:6 in reply to:  4 Changed 6 years ago by tony-zhu

Replying to andrewgodwin:

The problem I was encountering was that people would have one initial_data migration for a project containing entries for all models in a project - not just the ones in the specific app (as it's common to just "dumpdata" to a single fixture file)

In particular, while you can make flush only load the initial_data from apps without migrations, you cannot prevent people from putting instances of models with migrations into those initial_data files.

Thus, while you can modify flush to only load unmigrated apps, it's of limited use in actually preventing the error occurring in the initial ticket. That said, I don't see why it can't be fixed, and then documented as such, it might be a little less surprising.

I understand that one app could load initial data for other apps with migrations. There is no way we could prevent that. The only solution might be abandon the whole loaddata command for Django 1.7+ apps.

However, my propose flush command change does make sense for some reusable Django apps that could be dropped to any existing Django project. In my case, it is Django-helpdesk (https://github.com/rossp/django-helpdesk), which people just pip install it and put it into installed apps. Supporting both Django 1.6+South and Django 1.7+ will require such apps to have both old initial_data and the new migration based initial data.

comment:7 Changed 6 years ago by Tim Graham

Needs tests: unset
Triage Stage: AcceptedReady for checkin

comment:8 Changed 6 years ago by Tim Graham <timograham@…>

Resolution: fixed
Status: newclosed

In dd1ea70779adff294140eddb0229c382e12f32f3:

Fixed #23699 -- Prevented flush from loading initial data for apps with migrations.

comment:9 Changed 6 years ago by Tim Graham <timograham@…>

In 5cc8180a651b11f00a0a05296f8db15db1e48cdc:

[1.7.x] Fixed #23699 -- Prevented flush from loading initial data for apps with migrations.

Backport of dd1ea70779 from master.

comment:10 Changed 6 years ago by Tim Graham <timograham@…>

In abee4f718e6f08b523fc61fb812ef88d13a2e4b2:

[1.7.x] Fixed stable/1.7.x test failures from refs #23699.

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