Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#27858 closed Cleanup/optimization (fixed)

Stop read-only management commands from attempting to create a django_migrations table

Reported by: Marti Raudsepp Owned by: Marti Raudsepp
Component: Migrations Version: 1.10
Severity: Normal Keywords: makemigrations
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description (last modified by Tim Graham)

In multiple different projects now, I've needed to connect Django to legacy databases that aren't under Django's control. Even when setting "manage = False" in all affected models and configuring the DB router to never allow migrate in the legacy database, Django "makemigrations" still attempts to create the django_migrations table, causing permission errors in my use case.

The pull request changes MigrationRecorder is so that for read-only operations, if the django_migrations table doesn't exist, it's assumed that no migrations have been applied, instead of trying to create it. This applies to all migration commands.

Django has always had the problem of being "opinionated", meaning there's often fighting involved if you don't exactly follow The True Django Way. :) This patch is a small step in making Django more flexible.

Change History (10)

comment:1 by Marti Raudsepp, 8 years ago

Description: modified (diff)

comment:2 by Tim Graham, 8 years ago

I'm unsure about the steps to reproduce a problem here. According to #27054 it looks like if you configure Django properly, that table shouldn't be created. Do you have some configuration where that's not the case? (or is your patch meant to eliminate the need for such configuration?)

comment:3 by Marti Raudsepp, 8 years ago

Description: modified (diff)

D'oh! You are right of course, it was a logic error in my allow_migrate method. That fixes my use case, thanks!

However, I still think the patch has some merit. It has always felt wrong to me that operations that should be entirely read-only, like makemigrations or runserver, go and start creating tables. Do you agree?

comment:4 by Tim Graham, 8 years ago

Cc: Andrew Godwin added

I can't think of any problems with that change. Andrew, what do you think?

Side note: #23808 reported the issue of runserver creating the migrations table and the resolution was to document the behavior.

in reply to:  4 comment:5 by Andrew Godwin, 8 years ago

Replying to Tim Graham:

I can't think of any problems with that change. Andrew, what do you think?

Seems like a good idea to me, and the PR is quite small which is nice.

comment:6 by Tim Graham, 8 years ago

Cc: Andrew Godwin removed
Description: modified (diff)
Summary: makemigrations shouldn't attempt to create django_migrations tableStop read-only management commands from attempting to create a django_migrations table
Triage Stage: UnreviewedAccepted
Type: New featureCleanup/optimization

comment:7 by Tim Graham, 8 years ago

Patch needs improvement: set

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

Resolution: fixed
Status: assignedclosed

In fda55c71:

Fixed #27858 -- Prevented read-only management commands from creating the django_migrations table.

MigrationRecorder now assumes that if the django_migrations table
doesn't exist, then no migrations are applied.

Reverted documentation change from refs #23808.

comment:9 by Tim Graham <timograham@…>, 8 years ago

In 24d7fe49:

Refs #27858 -- Fixed typo in MigrationRecorder.applied_migrations() comment.

comment:10 by Marti Raudsepp, 8 years ago

Thanks! And sorry! I always intended to get back to this, but kept procrastinating. :(

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