Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Schema alteration #376

Merged
merged 180 commits into from
andrewgodwin added some commits
@andrewgodwin andrewgodwin Very start of schema alteration port. Create/delete model and some te…
…sts.
8ba5bf3
@andrewgodwin andrewgodwin Add some field schema alteration methods and tests. 959a3f9
@andrewgodwin andrewgodwin Merge branch 'master' of github.com:django/django into schema-alteration
Conflicts:
	django/db/backends/postgresql_psycopg2/base.py
4a2e80f
@andrewgodwin andrewgodwin Add M2M tests and some unique support b139315
@andrewgodwin andrewgodwin Add support for unique_together c4b2a32
@andrewgodwin andrewgodwin Merge branch 'master' into schema-alteration 184cf9a
@andrewgodwin andrewgodwin Add db_table and db_tablespace handling 60873ea
@andrewgodwin andrewgodwin First stab at MySQL support cab044c
@andrewgodwin andrewgodwin All tests passing on MySQL f7955c7
@andrewgodwin andrewgodwin Test that unique constraints get ported with column rename 0b01395
@andrewgodwin andrewgodwin Add a SQlite backend. One test passes! d3d1e59
@andrewgodwin andrewgodwin db_index alteration mostly working d865503
@andrewgodwin andrewgodwin Implement primary key changing cd583d6
@andrewgodwin andrewgodwin Merge branch 'master' into schema-alteration b546e7e
@andrewgodwin andrewgodwin Add some state management methods to AppCache. 7e81213
@andrewgodwin andrewgodwin Added SQLite backend which passes all current tests d683263
@andrewgodwin andrewgodwin Repoint ForeignKeys when their to= changes. a92bae0
@andrewgodwin andrewgodwin Add M2M repointing 375178f
@andrewgodwin andrewgodwin Add check constraint support - needed a few Field changes ca9c3cd
@andrewgodwin andrewgodwin Merge branch 'master' into schema-alteration 828d691
@andrewgodwin andrewgodwin Very initial Oracle support 8413c85
@andrewgodwin andrewgodwin Stubbed-out oracle schema file 3ffbfe4
@andrewgodwin andrewgodwin Fix app loading/test interaction dbf8b93
@andrewgodwin andrewgodwin Merge remote-tracking branch 'core/master' into schema-alteration
Conflicts:
	django/db/backends/mysql/base.py
	django/db/backends/postgresql_psycopg2/base.py
9313dea
@andrewgodwin andrewgodwin More schema test fixing d0b3536
@andrewgodwin andrewgodwin Fix bug in get_indexes affecting the tests c5e2eca
@andrewgodwin andrewgodwin Python 3 compatability. 2.6 was a while back, I should learn ' as '. 06227fb
django/db/backends/__init__.py
@@ -319,6 +319,11 @@ def cursor(self):
def make_debug_cursor(self, cursor):
return util.CursorDebugWrapper(cursor, self)
+ def schema_editor(self):
+ "Returns a new instance of this backend's SchemaEditor"
+ raise NotImplementedError()
@alex Collaborator
alex added a note

Drop the parens (sorry, style nit of mine)

@andrewgodwin Owner

Having them is a style nit of mine :) There's no consistent usage in Django so far, but I can drop them if needs be.

First, it might be nice to see some consistency (over time, as changed are made). Secondly, my logic for including them (perhaps like Andrew) is that i makes t clear that we are talking about a callable. Yes, the name should do that, but some people appreciate the clues. That's about the best I can come up with for a justification.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@alex alex commented on the diff
django/db/backends/__init__.py
@@ -418,6 +423,21 @@ class BaseDatabaseFeatures(object):
# Support for the DISTINCT ON clause
can_distinct_on_fields = False
+ # Can we roll back DDL in a transaction?
+ can_rollback_ddl = False
+
+ # Can we issue more than one ALTER COLUMN clause in an ALTER TABLE?
+ supports_combined_alters = False
+
+ # What's the maximum length for index names?
+ max_index_name_length = 63
@alex Collaborator
alex added a note

Should this really be a default in the base?

@andrewgodwin Owner

Yes, these are safe defaults - having them set like this will, at worst, make migrations a bit slower on a new backend, but having them set to the other values will cause syntax errors. Given that the pattern currently involves defaults being set for everything, I think this is a good match.

@shaib Collaborator
shaib added a note

I think the sane default for max_index_name_length is property(lambda self: self.max_name_length). Is there any backend where the rules for index names are different from those of other names?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@alex alex commented on the diff
django/db/backends/mysql/introspection.py
((12 lines not shown))
return indexes
+ def get_constraints(self, cursor, table_name):
+ """
+ Retrieves any constraints or keys (unique, pk, fk, check, index) across one or more columns.
+ """
+ constraints = {}
+ # Get the actual constraint names and columns
+ name_query = """
+ SELECT kc.`constraint_name`, kc.`column_name`,
+ kc.`referenced_table_name`, kc.`referenced_column_name`
+ FROM information_schema.key_column_usage AS kc
+ WHERE
+ kc.table_schema = %s AND
+ kc.table_name = %s
+ """
@alex Collaborator
alex added a note

This probably shouldn't block this patch, but I'm starting to think it'd be cool, and seriously help readability if we replaced all these SQL strings with unmanaged models, and just used the ORM for querying...

@andrewgodwin Owner

Hmm, that's an interesting idea, but tricky to do, especially since in the PostgreSQL backend I use arrays during the query to reduce the number of queries.

@alex Collaborator
alex added a note

Yes, would definitely have to be a gradual migration (might be a good impetus to make the ORM more featureful in any event).

@shaib Collaborator
shaib added a note

On SQL Server, IIRC, you can't get all the needed info from information-schema -- you need the SQL-Server-specific system tables. That could be done with unmanaged models as well, of course, but the idea of backend-specific-unmanaged-models scares me a little.

(SQL Server is not in core, but I like to try to keep core friendly to 3rd-party backends).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
django/db/backends/mysql/introspection.py
((12 lines not shown))
return indexes
+ def get_constraints(self, cursor, table_name):
+ """
+ Retrieves any constraints or keys (unique, pk, fk, check, index) across one or more columns.
+ """
@alex Collaborator
alex added a note

Constraints and indexes really aren't the same things, does it really make sense to have them in the same method?

@akaariai Collaborator

I think there could be some room for code-reuse. I know get_indexes() has been a big pain to maintain (dropped columns, same column indexed as unique, and as part of non-unique index etc).

Perhaps add a new underlying method which returns multicolumn indexes, and multicolumn constraints alike. Something like
list of: {name: NamedTuple(cols=list(cols), unique=True, primary_key=True, constraint=True, index=True)}.

@andrewgodwin Owner

Alex: It makes sense to have them in the same method, as often they share the same namespace, and they all cross-enforce (for example, a UNIQUE constraint will also implicitly be an index). The name is perhaps the worst thing, but I didn't want get_constraints_and_indexes

Anssi: There probably is room for code reuse here, in particular I'd want to hang get_indexes off of the new get_constraints (which is that method which returns multicolumn indexes/constraints). Is there a particular reason you prefer named tuples for the return value?

@akaariai Collaborator

I just have come to like NamedTuples, and for this use case they seems to be perfect match. They have some advantages over dicts:

  • Usage is nicer - val.cols is nicer than val['cols']
  • They are immutable
  • There is no way they could contain more values than what is wanted - in effect they have a constraint on them
  • They have a little nicer printout for debugging purposes

But, this is stylistic issue and Django mostly uses dicts (because namedtuples haven't been available for long). So, there is no strong reason to use namedtuples if you don't want to.

@andrewgodwin Owner

Yeah, I'm still in the pattern of using dicts, and that function would have to build them up as dicts first anyway before converting to namedtuples (as it relies on the mutability during the process), so I'd rather keep them as dicts without a serious reason to change, and in the interests of not rewriting a good chunk of the code :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@alex alex commented on the diff
django/db/backends/mysql/schema.py
@@ -0,0 +1,26 @@
+from django.db.backends.schema import BaseDatabaseSchemaEditor
@alex Collaborator
alex added a note

Is SchemaEditor the right name for this? most of the other backend things have less action oriented names.

@andrewgodwin Owner

I think the more action-oriented name fits with the fact that you have to get an instance of it and call start/commit - the other backend things are single instance. Suggestions welcome for new names, though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
django/core/management/commands/syncdb.py
@@ -71,9 +71,11 @@ def handle_noargs(self, **options):
def model_installed(model):
opts = model._meta
converter = connection.introspection.table_name_converter
- return not ((converter(opts.db_table) in tables) or
+ # Note that if a model is unmanaged we short-circuit and never try to install it
+ return opts.managed and not ((converter(opts.db_table) in tables) or
@akaariai Collaborator

Hmmh, won't this change what is passed into the post_sync signal? This is a (minor) feature change, and this could mean the contenttypes and permissions aren't created for this model. (Haven't verified this).

@andrewgodwin Owner

You're right, it could - I had to do this to stop the tests failing (the unmanaged models in the schema tests tried to create themselves when they shouldn't have), but I can probably fix that in the test wrapper.

Thinking about this code, though, it's possible we fire post_syncdb on every run for unmanaged models. I'll check.

@akaariai Collaborator

Fixing the tests is the correct thing to do. It sounds there is some other problem if unmanaged models try to create themselves...

@andrewgodwin Owner

I think it's a bad interaction of the way the test models are set to managed during tests and then unmanaged directly afterwards. I'll look into it later and get this patched out again.

@akaariai Collaborator

I bet the reason for this problem is that you are leaking tables into the test DB. You will need to drop the tables in teardown, and this problem should go away.
EDIT: it seems you are already dropping the tables, but still I suspect table leak to be the root cause...

@andrewgodwin Owner

It's not table leak - the problem is that somehow the 'schema' test app ends up being requested by the test run of syncdb even when INSTALLED_APPS doesn't contain it. I think the problem is just in the other test, but until now the other apps have had tables existing so it hasn't tried to create them again. I'll look into it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
django/db/models/loading.py
((12 lines not shown))
+ "app_models": SortedDict((k, SortedDict(v.items())) for k, v in self.app_models.items()),
+ "app_errors": dict(self.app_errors.items()),
+ }
+
+ def restore_state(self, state):
+ """
+ Restores the AppCache to a previous state from save_state.
+ Note that the state is used by reference, not copied in.
+ """
+ self.app_store = state['app_store']
+ self.app_labels = state['app_labels']
+ self.app_models = state['app_models']
+ self.app_errors = state['app_errors']
+ self._get_models_cache.clear()
+
+ def temporary_state(self):
@akaariai Collaborator

This looks very scary to me.

If you load a model with foreign keys temporarily to the app-cache, and the foreign keys are to existing models, then the existing models will also get altered. When the state is restored, the existing models will remain altered. In general, there can be side-effects beyond the app-cache when loading/altering models, and it will be really hard to tell what those side-effects are.

To me it seems it could be better to start with an empty app-cache when using temporary model state, and then just reload (as separate model classes) the models into the app-cache. It might be possible to do this by something like:
def load_schema_models():
class Model1(models.Model):
...
# returns nothing, just introduces the models

The idea is that each time you call this, the models are recreated. I am not sure at all this works, but as the app-cache is emptied, there is nothing to return from there. So, this might work :)

The downside (assuming this works at all) is that you will not be able to import those models, you will need to get them from the temporary app-cache by get_model().

@andrewgodwin Owner

So, the AppCache changes are by FAR the nastiest part of this patch, so you're right to worry.

The basic problem is that because AppCache is a single global object, you can't just make a new one alongside (as you need to modify the main one so that lazy ForeignKey references resolve) - thus, you either need to make a new empty one, swap it in instead of the other one, load all the other apps into it, then load the new models into it, run your queries, and then swap them back. Remember that the AppCache currently shares state between all instances using the 'Borg' pattern, so this new code would have to override that behaviour in a messy way.

Given that option, and that fact this code will need to change again soon due to the app loading refactor, I opted for the state-saving approach. It works as well as can be expected and the tests give it a thorough test to make sure there's no contamination - you're right that ForeignKey references (and more importantly reverse references) persist, but there's no way to fix that without overhauling the entire models framework (something I'll look at for 1.6, but I'm not convinced about).

This AppCache stuff, incidentally, is not part of the core schema-changing codebase, and is only in there for the tests and to make the SQLite backend a bit simpler. It used to live externally as a monkey-patch, but I prefer having that code directly on the AppCache.

@akaariai Collaborator

I think we need a better way to have separate class instances in separate instances of app-cache. We probably wont get anything like this into 1.5, but that is another issue...

Really, what we want is to have an empty app-cache where we can load clones of existing class instances by something like app_cache.load_models(*models). Currently, the only way to do app-cache loading is by meta class __new__(). I think app-loading should be launched by meta.new(), but it should just launch it. The actual work should be done by some other method which could be launched by other means, too... We would also need to clone the class of the model, and this could be problematic.

If we got something like this in there would be the possibility to alter model clones in-fly whenever we want. This would be a huge bonus for testing. We could probably also use the core ORM for schema migrations, which could be a nice feature. Just unfreeze + load to temporary app-cache and give that app-cache to the developer. Could work, no?

To actually do the above is a lot of work, and seems to be closely related with the app-loading refactor. So, for the time being lets try to invent something as-simple-as-possible that could work for this feature's needs. I still do think the code as-is looks scary.

Sorry if this got a bit off-topic...

(And thanks Github for not confirming close of this form when pressing Esc, very user friendly feature)

@akaariai Collaborator

I will try to see if I can figure something a little less scary for this. I am pretty sure as-is the code will be source for strange problems. To me the managed/unmanaged flushing seems to point this out - even if you have different app-cache you are still altering the global models, and that is the scary part. The models are assumed to be immutable, and altering them might result in weird, weird bugs.

Note: my second comment above isn't a reply to Andrew's comment, it's concurrent...

@alex Collaborator
alex added a note

So, API idea:

class Meta:
    auto_register = False

causes the model not to go in the default AppCache, then we can have a cache.load_models(*classes) as @akaariai suggests. This seems sufficient to make this patch work?

@andrewgodwin Owner

Hmm, that would work - however, there still needs to be code to swap the app cache for a new temporary one, if I'm not mistaken. Am I right in thinking that the AppCache being a shared-across-all-instances class is not part of any public API and that we might be able to make it a per-instance thing?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
django/db/backends/schema.py
((47 lines not shown))
+ sql_create_fk = "ALTER TABLE %(table)s ADD CONSTRAINT %(name)s FOREIGN KEY (%(column)s) REFERENCES %(to_table)s (%(to_column)s) DEFERRABLE INITIALLY DEFERRED"
+ sql_delete_fk = "ALTER TABLE %(table)s DROP CONSTRAINT %(name)s"
+
+ sql_create_index = "CREATE INDEX %(name)s ON %(table)s (%(columns)s)%(extra)s;"
+ sql_delete_index = "DROP INDEX %(name)s"
+
+ sql_create_pk = "ALTER TABLE %(table)s ADD CONSTRAINT %(name)s PRIMARY KEY (%(columns)s)"
+ sql_delete_pk = "ALTER TABLE %(table)s DROP CONSTRAINT %(name)s"
+
+ def __init__(self, connection):
+ self.connection = connection
+
+ # State-managing methods
+
+ def start(self):
+ "Marks the start of a schema-altering run"
@alex Collaborator
alex added a note

Triple quotes for all docstrings please.

@andrewgodwin Owner

Done in ae6ffd2.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
django/db/backends/schema.py
((76 lines not shown))
+ "Tries to roll back a schema-altering run. Call instead of commit()"
+ if not self.connection.features.can_rollback_ddl:
+ raise RuntimeError("Cannot rollback schema changes on this backend")
+ self.connection.rollback()
+ self.connection.leave_transaction_management()
+
+ # Core utility functions
+
+ def execute(self, sql, params=[], fetch_results=False):
+ """
+ Executes the given SQL statement, with optional parameters.
+ """
+ # Get the cursor
+ cursor = self.connection.cursor()
+ # Log the command we're running, then run it
+ logger.info("%s; (params %r)" % (sql, params))
@alex Collaborator
alex added a note

Why does this have its own logging, instead of the usual query logging?

@andrewgodwin Owner

Because they're not normal queries - during migration runs you might well want to show only the schema-altering statements as they're being run on the database, and not e.g. the queries to the migration history table. If there's a better way of achieving that I'll happily implement it, logging is not my strong point.

@shaib Collaborator
shaib added a note

I, for one, would rather have "info" level logging at a higher level of abstraction ("altering column", "creating table" etc). While it would be nice to be able to choose which SQL is printed and which isn't, ultimately SQL here is only needed for debugging.

@andrewgodwin Owner

Indeed, which is why it's a separate thing right now as I can enable/disable it directly. If SQL generation is built in directly, there's no need for any nicely-done logging of SQL.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
django/db/backends/schema.py
((214 lines not shown))
+ sql = self.sql_create_table % {
+ "table": model._meta.db_table,
+ "definition": ", ".join(column_sqls)
+ }
+ self.execute(sql, params)
+ # Make M2M tables
+ for field in model._meta.local_many_to_many:
+ self.create_model(field.rel.through, force=True)
+
+ def delete_model(self, model, force=False):
+ """
+ Deletes a model from the database.
+ """
+ # Do nothing if this is an unmanaged or proxy model
+ if not force and (not model._meta.managed or model._meta.proxy):
+ return
@alex Collaborator
alex added a note

This seems like it should be the caller's responsibility (same applies to create_model)

@andrewgodwin Owner

Hmm, that's a good point. Any sane migration framework wouldn't try to submit changes to these, and sometimes you do want to create tables for unmanaged models, anyway. I'll change it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
django/db/backends/sqlite3/introspection.py
((15 lines not shown))
+ for index_rank, column_rank, column in cursor.fetchall():
+ if index not in constraints:
+ constraints[index] = {
+ "columns": set(),
+ "primary_key": False,
+ "unique": bool(unique),
+ "foreign_key": False,
+ "check": False,
+ "index": True,
+ }
+ constraints[index]['columns'].add(column)
+ # Get the PK
+ pk_column = self.get_primary_key_column(cursor, table_name)
+ if pk_column:
+ constraints["__primary__"] = {
+ "columns": set([pk_column]),
@alex Collaborator
alex added a note

I don't see primary mentioned anywhere else, at the minimum this needs a comment as to what that magic string is.

@andrewgodwin Owner

Comment added just above this line to explain what it is in ae6ffd2.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@alex alex commented on the diff
django/db/backends/sqlite3/schema.py
((56 lines not shown))
+ self.quote_name(temp_model._meta.db_table),
+ ', '.join([x for x, y in field_maps]),
+ ', '.join([y for x, y in field_maps]),
+ self.quote_name(model._meta.db_table),
+ ))
+ # Delete the old table
+ self.delete_model(model)
+ # Rename the new to the old
+ self.alter_db_table(model, temp_model._meta.db_table, model._meta.db_table)
+ # Run deferred SQL on correct table
+ for sql in self.deferred_sql:
+ self.execute(sql.replace(temp_model._meta.db_table, model._meta.db_table))
+ self.deferred_sql = []
+ # Fix any PK-removed field
+ if restore_pk_field:
+ restore_pk_field.primary_key = True
@alex Collaborator
alex added a note

This whole method scares me, and not in a good way. We need to find a new API for this to work, because monkey patching all this state and then cleaning up is a mess, and very very fragile.

@andrewgodwin Owner

Yes, it is a mess, but improving how this works is going to take a massive overhaul to how models work (see Anssi's comments above). SQLite doesn't support schema changes anyway, so anything we do to it is going to be fragile-ish, but at least the code in there works and doesn't leak state to everything else (it did for a while, the tests caught all that).

If we can find a good solution to the models problem, though, this goes away.

@akaariai Collaborator

This stuff scares me, too. I don't see any reason why the altered model needs to be in any cache at all. Also, if the altered model contains foreign keys, or is pointed to by foreign keys, wouldn't the referenced/referencing model's ._meta then get changed, too? I believe those contain at least caches which could then mistakenly contain references to the temporary model. It is not enough to use a temporary cache. You would need to deep copy the models, too, to make this safe. Basically, it is going to be extremely hard to do proper cleanup.

If we want this patch into 1.5 all issues can't be solved. The migrations stuff is going to be more or less hacked in for 1.5, as other parts of the code doesn't yet have the support needed for this. Waiting for that to happen is going to be somewhat long wait if I am not mistaken...

So, at least we should document the crap out of every hack taken, and warn loudly about the potential pitfalls. I am afraid somebody is going to start using the dynamic cache in request processing, and it is 100% guaranteed that is going to end up ugly.

@andrewgodwin Owner

You're right in that the _meta of the other model will also be affected (plus the other class will get a ReverseSomething added to it as an attribute), so there's no way it's perfect - however, since the migrations code largely runs in a single one-off thread, that's generally not a problem. I agree we don't want people using this in code, though, especially as we want to change the way this works to have the separate caches passed around the models in future.

Perhaps a suitably large warning and a promise that we'll break it next release?

@akaariai Collaborator

Yeah, the large warning seems to be the way to go.

There are some things we could improve relatively easily for 1.5 in the beta stage. It should be possible to prevent alteration of related models by a flag in the model's Meta class, and it should be possible to create a model class without it triggering the meta.new() code (that is, the actual model creation). This way, we could do cloned_model = type(...), temp_cache.load_model(cloned_model). That is, make the loading explicit, not rely on the global loading.cache alteration.

I can work on this in the beta stages if needed.

While the code isn't too beautiful right now, if we want this into 1.5 it is more or less what we need to go with.

@andrewgodwin Owner

I like the idea of a flag preventing alteration of related models - that'll need to be improved upon later on so you can re-enable it inside a single other appcache, as that's needed for migrations, but should be fine for now. Do you think it's worth putting that in the patch now, or waiting till after the merge? Also, perhaps we can interpret auto_register = False as this flag (as it means a very similar thing)?

@akaariai Collaborator

Splitting this out to separate patch seems like a good idea to me. And, I suspect you would want to do this post-commit of this patch. This patch is already large enough.

In a perfect world the changes to app-loading would be done in completely separate patch, and then the migrations stuff would build on top of that. If you could do that splitting it would be excellent, but I am OK with just doing everything in single patch/branch.

It seems like a good idea that auto_register = False implies that no related model changes are done.

@andrewgodwin Owner

Alright - let's deal with this post-merge then. Are you suggesting that said future patch should depend on app-loading?

@akaariai Collaborator

Nah, lets just do that post-merge in a separate patch. There is no need to depend on app-loading refactor.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
django/db/models/loading.py
((24 lines not shown))
+ self.app_errors = state['app_errors']
+ self._get_models_cache.clear()
+
+ def temporary_state(self):
+ "Returns a context manager that restores the state on exit"
+ return StateContextManager(self)
+
+ def unregister_all(self):
+ """
+ Wipes the AppCache clean of all registered models.
+ Used for things like migration libraries' fake ORMs.
+ """
+ self.app_store = SortedDict()
+ self.app_labels = {}
+ self.app_models = SortedDict()
+ self.app_errors = {}
@alex Collaborator
alex added a note

At a minimum this is the wrong place for this: objects shouldn't have their own methods for monkey patching their state. I'm seriously skeptical of whether this should exist at all, it's too much of a mess and too fragile.

@andrewgodwin Owner

We should continue this discussion in Anssi's thread about the same area of code above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
django/db/models/loading.py
((40 lines not shown))
+
+
+class StateContextManager(object):
+ """
+ Context manager for locking cache state.
+ Useful for making temporary models you don't want to stay in the cache.
+ """
+
+ def __init__(self, cache):
+ self.cache = cache
+
+ def __enter__(self):
+ self.state = self.cache.save_state()
+
+ def __exit__(self, type, value, traceback):
+ self.cache.restore_state(self.state)
@alex Collaborator
alex added a note

Just use contextlib.contextmanager, you'll be much happier.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
tests/modeltests/schema/tests.py
@@ -0,0 +1,665 @@
+from __future__ import absolute_import
+import copy
+import datetime
+from django.test import TestCase
+from django.utils.unittest import skipUnless
+from django.db import connection, DatabaseError, IntegrityError
+from django.db.models.fields import IntegerField, TextField, CharField, SlugField
+from django.db.models.fields.related import ManyToManyField, ForeignKey
+from django.db.models.loading import cache
+from .models import Author, AuthorWithM2M, Book, BookWithSlug, BookWithM2M, Tag, TagUniqueRename, UniqueTest
+
+
+class SchemaTests(TestCase):
@alex Collaborator
alex added a note

This should be TransactionTestCase right?

@andrewgodwin Owner

The tests do their own transaction management, since the schema editor backend itself uses transactions and we need to check everything is working right. Not sure what improvement TransactionTestCase would give, they already run really fast.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
tests/modeltests/schema/tests.py
((75 lines not shown))
+ cursor = connection.cursor()
+ columns = dict(
+ (d[0], (connection.introspection.get_field_type(d[1], d), d))
+ for d in connection.introspection.get_table_description(
+ cursor,
+ model._meta.db_table,
+ )
+ )
+ # SQLite has a different format for field_type
+ for name, (type, desc) in columns.items():
+ if isinstance(type, tuple):
+ columns[name] = (type[0], desc)
+ # SQLite also doesn't error properly
+ if not columns:
+ raise DatabaseError("Table does not exist (empty pragma)")
+ return columns
@alex Collaborator
alex added a note

These methods seem to duplicate a lot of the actual implementation, which definitely isn't right.

@andrewgodwin Owner

Welcome to my world. I didn't want to change the API of introspection, since that can't be changed without a deprecation cycle, so I have to work round it here.

@akaariai Collaborator

The introspection API isn't part of the public API as far as I know. If it is, then skip the rest of this comment...

So, they can be changed without deprecation cycle. We should of course not change the API just for fun, but on the other hand I am very much -1 on treating any internal API like public APIs because somebody might (or is known to) use them. That is a slippery slope which leads to a situation where we can't change anything.

@andrewgodwin Owner

True - it's not documented as a public API, it's just that quite a few people use it. I can fix get_field_type to return tuples in all backends for all types and very easily add a database error in if you run get_table_description on a nonexistent table under sqlite, if people think that's in the scope of this patch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
tests/modeltests/schema/tests.py
((121 lines not shown))
+ # Create the table
+ editor = connection.schema_editor()
+ editor.start()
+ editor.create_model(Book)
+ editor.create_model(Author)
+ editor.create_model(Tag)
+ editor.commit()
+ # Check that initial tables are there
+ try:
+ list(Author.objects.all())
+ except DatabaseError as e:
+ self.fail("Author table not created: %s" % e)
+ try:
+ list(Book.objects.all())
+ except DatabaseError as e:
+ self.fail("Book table not created: %s" % e)
@alex Collaborator
alex added a note

Please drop all of these self.fail calls, they don't really add anything (a comment is much more useful) and they obscure tracebacks.

@andrewgodwin Owner

Removed in 7e8c64d.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
tests/modeltests/schema/tests.py
((44 lines not shown))
+ for model in self.models:
+ model._meta.managed = False
+ cache.restore_state(self.cache_state)
+
+ def delete_tables(self):
+ "Deletes all model tables for our models for a clean test environment"
+ cursor = connection.cursor()
+ connection.disable_constraint_checking()
+ for model in self.models:
+ # Remove any M2M tables first
+ for field in model._meta.local_many_to_many:
+ try:
+ cursor.execute(connection.schema_editor().sql_delete_table % {
+ "table": connection.ops.quote_name(field.rel.through._meta.db_table),
+ })
+ except DatabaseError:
@akaariai Collaborator

Do not swallow the error here...

@andrewgodwin Owner

Would you rather I tested for table existence first?

@akaariai Collaborator

That, or check that the error is about not having the table in the DB in the first place. The database error could be caused by multitude of other error conditions, too, and swallowing those errors could lead to silent table leak...

@andrewgodwin Owner

I've fixed this to check that the error is about the table not existing, using a slightly nasty string-comparison method. Suggested improvements to that welcome.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
django/db/backends/schema.py
((46 lines not shown))
+
+ sql_create_fk = "ALTER TABLE %(table)s ADD CONSTRAINT %(name)s FOREIGN KEY (%(column)s) REFERENCES %(to_table)s (%(to_column)s) DEFERRABLE INITIALLY DEFERRED"
+ sql_delete_fk = "ALTER TABLE %(table)s DROP CONSTRAINT %(name)s"
+
+ sql_create_index = "CREATE INDEX %(name)s ON %(table)s (%(columns)s)%(extra)s;"
+ sql_delete_index = "DROP INDEX %(name)s"
+
+ sql_create_pk = "ALTER TABLE %(table)s ADD CONSTRAINT %(name)s PRIMARY KEY (%(columns)s)"
+ sql_delete_pk = "ALTER TABLE %(table)s DROP CONSTRAINT %(name)s"
+
+ def __init__(self, connection):
+ self.connection = connection
+
+ # State-managing methods
+
+ def start(self):
@alex Collaborator
alex added a note

Should this be named begin, since you're using SQL terminology elsewhere anyways.

@andrewgodwin Owner

Not sure I like begin, as this is quite different to SQL transactions - it's also wrapping the deferred SQL stuff. Besides, all sensible databases accept START TRANSACTION too :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@alex alex commented on the diff
django/db/backends/schema.py
((142 lines not shown))
+ # Return the sql
+ return sql, params
+
+ def effective_default(self, field):
+ """
+ Returns a field's effective database default value
+ """
+ if field.has_default():
+ default = field.get_default()
+ elif not field.null and field.blank and field.empty_strings_allowed:
+ default = ""
+ else:
+ default = None
+ # If it's a callable, call it
+ if callable(default):
+ default = default()
@alex Collaborator
alex added a note

Calling a callable default in order to set a table-wide default, even if it's temporary, seems very wrong.

@andrewgodwin Owner

What else should we do? Raise an error? Not use a table-wide default for that column (and thus potentially not be able to add it)? This has caused issues in the past, certainly, when the function behind there is random.something.

@shaib Collaborator
shaib added a note

If the default is callable, then the default shouldn't stay in the table. This function may be too low-level to handle this, but it hints that once we've got the specific value, we "forget" that it was callable. I think that in this case, column_sql should return two statements -- one to create the column, and one to remove the default.

@andrewgodwin Owner

Yes, but if it's callable I'd say it might be sensible to never even use it for table creation in the first place - examples of callables people have used are random.randint and uuid.uuid4, both of which you really shouldn't make rows with.

I'd vote for the core schema system raising an error if you try to add a not-null column with a callable default, and to then make migration frameworks detect this error and prompt the user if they want to add the column with a frozen callable result instead,

@shaib Collaborator
shaib added a note

Raising an error sounds good to me (I think it is better for frameworks to detect this on their own and prompt the user when the migration is generated, not when it is run, but that is a little off topic here).

@shaib Collaborator
shaib added a note

Just a reminder that this is still unchanged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
django/db/backends/sqlite3/introspection.py
((16 lines not shown))
+ if index not in constraints:
+ constraints[index] = {
+ "columns": set(),
+ "primary_key": False,
+ "unique": bool(unique),
+ "foreign_key": False,
+ "check": False,
+ "index": True,
+ }
+ constraints[index]['columns'].add(column)
+ # Get the PK
+ pk_column = self.get_primary_key_column(cursor, table_name)
+ if pk_column:
+ # SQLite doesn't actually give a name to the PK constraint,
+ # so we invent one. This is fine, as the SQLite backend never
+ # deletes PK constraints by name.
@alex Collaborator
alex added a note

Please add to the comment that the reason it doesn't delete by name is that it uses introspection.

@andrewgodwin Owner

Done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
django/db/backends/sqlite3/schema.py
((39 lines not shown))
+ del body[field.name]
+ del mapping[field.column]
+ # Construct a new model for the new state
+ meta_contents = {
+ 'app_label': model._meta.app_label,
+ 'db_table': model._meta.db_table + "__new",
+ 'unique_together': model._meta.unique_together if override_uniques is None else override_uniques,
+ }
+ meta = type("Meta", tuple(), meta_contents)
+ body['Meta'] = meta
+ body['__module__'] = model.__module__
+ self.app_cache = AppCache()
+ cache.set_cache(self.app_cache)
+ cache.copy_from(default_cache)
+ temp_model = type(model._meta.object_name, model.__bases__, body)
+ cache.set_cache(default_cache)
@alex Collaborator
alex added a note

This doesn't seem right, you shouldn't be creating a new cache and then changing the global one. You should create the new cache, create a model with some sort of "dont add to cache" flag, and then explicitly adding it to the new cache.

@akaariai Collaborator

Problem is, currently you can't create a model without any cache if it contains references to other models. And, therein lies a danger: adding the model in to the copied cache is going to alter other models in that cache, too. But, the models themselves aren't copier. See models/fields/related.py RelatedField.contribute_to_class() for one place where such changes are done (through do_related_fields() -> contribute_to_related_class()).. I don't know if there are other such cascading changes caused by loading a model into cache. Parent models come to mind at least...

Preferably before commit, but at least before release we need a way to add a model into cache in a way that is guaranteed to not change the already existing models.

Alternatively we need to deepcopy the app-cache contents, too. But, this is an ugly approach, it can be hard to see what exactly gets copied by the deepcopy, and we might have overridden some .deepcopy methods to not work nicely with this approach (Field.deepcopy comes to mind here).

@andrewgodwin Owner

We've been discussing this on the note above, too - I don't want to use deepcopy, and I think modifying models so that auto_register = False means they don't touch anything at all would work well enough.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
django/db/models/loading.py
((22 lines not shown))
+ self._cache = cache
+
+ def __getattr__(self, attr):
+ if attr in ("_cache", "set_cache"):
+ return self.__dict__[attr]
+ return getattr(self._cache, attr)
+
+ def __setattr__(self, attr, value):
+ if attr in ("_cache", "set_cache"):
+ self.__dict__[attr] = value
+ return
+ return setattr(self._cache, attr, value)
+
+
+default_cache = AppCache()
+cache = AppCacheWrapper(default_cache)
@alex Collaborator
alex added a note

This seems like a poor idea, and probably should not exist.

@andrewgodwin Owner

I know, but it's tied to the cache-as-global-state mechanic. This'll get fixed as all the other models code gets weaned off of it slowly.

@akaariai Collaborator

Is there any possibility of passing the used cache in parameters for migrations stuff? If that would be possible, then there would be no need to change what "cache" is at all. Another possibility is to have cache = AppCache(), and then have set_dynamic_cache() and get_dynamic_cache() functions for those use cases where you need a dynamic cache. That is, the cache var would always reference the one true global installed apps cache, and when you need it you have the possibility to use an alternate cache. Not that this is too beautiful either...

So, the question is: where exactly do you need the dynamic cache references - are they really needed globally? Currently the loading.cache reference doesn't seem to be used that much, so is it really a big problem?

@andrewgodwin Owner

Unfortunately, the dynamic cache is also needed in the SQLite schema backend - because SQLite doesn't support altering tables, it uses temporary models to create new tables with the new layouts. Otherwise, I would have left out all the cache changes in this patch.

@akaariai Collaborator

But, if the model isn't registered in any cache at all, you don't need no dynamic cache at all in the SQLite backend. To me it seems you just need a clone of an existing model with alterations applied, but you don't want any other model to know about the clone. If the clone doesn't get registered in any cache, and the related models do not get altered by the clone, then you don't need any temp cache at all in the SQLite backend...

@andrewgodwin Owner

Without the clone that method was definitely polluting the cache. I think the problem was that when the new model was created, it was resolving some of the ForeignKeys and doing the resolution/modification we discussed above - perhaps that would go away when the don't-edit-other-models flag came in but, as we said further up, that might not be in this patch...

@akaariai Collaborator

Do you mind if I take a shot at the don't pollute the cache approach? That would be a pull request to your branch. I have an idea how to implement this relatively painlessly, and the easiest way to show the idea and check it works is coding it...

@andrewgodwin Owner

Be my guest - if you can get it working I'd love to include it in here.

@akaariai Collaborator

I did a little more investigation, and doing the no-pollute app-loading properly is much bigger job than what I can do before feature freeze. The basic problem is that doing it for a single model is somewhat easy (if ugly), but when you need to be able to load a set of models which have relations to each other things get complicated.

I think we should be able to have the .auto_register flag with the meaning that a model with .auto_register = False will not be loaded to cache, and will not alter other models for foreign keys or m2m tables. This could make the SQLite part less fragile. But, doing even this pre-freeze seems hard.

@andrewgodwin Owner

That's what I was concerned about - it's quite a big job, and there's no way it's going to happen in the next week.

I'm starting to be less sure this patch needs to be in 1.5 - after all, without the Field API changes (which I could feasibly do in the next week but might be a squeeze) 1.5 won't be directly usable by migrations, so perhaps it's best to wait until the 1.5 branch is forked off and then give 1.6 all of the schema alteration, field API changes and migrations-running code?

(Of course, my reason for having it in there originally was to test that it didn't break stuff, and that's been working out well so far)

@akaariai Collaborator

Well, if you feel you don't need this in 1.5, then lets postpone this. When you look at the amount of comments in this pull request it is obvious there is still some work to do... However, if you want this into 1.5 I am sure we can fix the worst parts of the cache pollution in beta stages.

Your call :)

@andrewgodwin Owner

Well, the original plan was to have this in 1.5 and develop the migrations as a separate app that could then just be dumped into contrib, but it's become apparent that migrations are going to need more extensive integration into Django, so I'm not sure what rushing this into 1.5 gains us, whereas if I merge it in just after the 1.5 branch there's loads of time to then fix up the cache stuff, change the field APIs, and get a decent migration runner all done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@shaib shaib commented on the diff
django/db/backends/__init__.py
@@ -418,6 +423,21 @@ class BaseDatabaseFeatures(object):
# Support for the DISTINCT ON clause
can_distinct_on_fields = False
+ # Can we roll back DDL in a transaction?
+ can_rollback_ddl = False
+
+ # Can we issue more than one ALTER COLUMN clause in an ALTER TABLE?
+ supports_combined_alters = False
+
+ # What's the maximum length for index names?
+ max_index_name_length = 63
+
+ # Does it support foreign keys?
+ supports_foreign_keys = True
+
+ # Does it support CHECK constraints?
+ supports_check_constraints = True
+
@shaib Collaborator
shaib added a note

To get Oracle working properly, you probably also need an entry for "int_used_for_boolean" or some such

@andrewgodwin Owner

Can we not do that further down in the value-handling functions? MySQL does similar stuff for NullBooleanFields currently without a feature flag, and it works fine.

@shaib Collaborator
shaib added a note

I added it to South not too long ago, and it allowed me to factor out things for Oracle and SQL Server (and I might have included MySql, had I been aware that there's a similar issue there).
https://bitbucket.org/andrewgodwin/south/changeset/9425a7823355e1907c2bd9f9e2e0c7e1845c6dcf

@andrewgodwin Owner

That patch looks to me like it might be better represented as a small override to effective_default, perhaps? I'm keen not to add too many things to DatabaseFeatures...

@shaib Collaborator
shaib added a note

Trade-off. Flag vs. repetition between backends (unless I'm missing something). Your call.

@andrewgodwin Owner

No, that's about how I assessed it too. I'd like to keep it explicit in the backends for now - once the Oracle one is ported over and written it might be the case that we switch it to a feature flag, but that's not too hard to do.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@shaib shaib commented on the diff
django/db/backends/schema.py
@@ -0,0 +1,637 @@
@shaib Collaborator
shaib added a note

The file name is a little surprising. To parallel "creation.py" I would expect something like "alteration.py". "schema_change.py" or "schema_editing.py" also work. "schema.py", to me, hints mostly at a generic, python-level representation of the schema.

@andrewgodwin Owner

It used to be called alteration.py, oddly enough, but the idea is that this will eventually replace creation.py so it doesn't really have to parallel it. I didn't want something too long, and I think what it does is immediately obvious once you open the file and see "SchemaEditor".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@shaib shaib commented on the diff
django/db/backends/schema.py
((32 lines not shown))
+ sql_alter_column = "ALTER TABLE %(table)s %(changes)s"
+ sql_alter_column_type = "ALTER COLUMN %(column)s TYPE %(type)s"
+ sql_alter_column_null = "ALTER COLUMN %(column)s DROP NOT NULL"
+ sql_alter_column_not_null = "ALTER COLUMN %(column)s SET NOT NULL"
+ sql_alter_column_default = "ALTER COLUMN %(column)s SET DEFAULT %(default)s"
+ sql_alter_column_no_default = "ALTER COLUMN %(column)s DROP DEFAULT"
+ sql_delete_column = "ALTER TABLE %(table)s DROP COLUMN %(column)s CASCADE"
+ sql_rename_column = "ALTER TABLE %(table)s RENAME COLUMN %(old_column)s TO %(new_column)s"
+
+ sql_create_check = "ALTER TABLE %(table)s ADD CONSTRAINT %(name)s CHECK (%(check)s)"
+ sql_delete_check = "ALTER TABLE %(table)s DROP CONSTRAINT %(name)s"
+
+ sql_create_unique = "ALTER TABLE %(table)s ADD CONSTRAINT %(name)s UNIQUE (%(columns)s)"
+ sql_delete_unique = "ALTER TABLE %(table)s DROP CONSTRAINT %(name)s"
+
+ sql_create_fk = "ALTER TABLE %(table)s ADD CONSTRAINT %(name)s FOREIGN KEY (%(column)s) REFERENCES %(to_table)s (%(to_column)s) DEFERRABLE INITIALLY DEFERRED"
@shaib Collaborator
shaib added a note

(style nit) I prefer it when lines this long are split.

@andrewgodwin Owner

I hate it when long lines are split, since all modern editors auto-wrap nicely, and it makes the line harder to read for me if it's split.

@shaib Collaborator
shaib added a note

http://www.python.org/dev/peps/pep-0008/#maximum-line-length

I find 80 limiting as well, but passing the github code-window width is a nuisance to reviewers.

@charettes Collaborator

FYI pep-0008 was revisited and allows up to 99 chars.

@andrewgodwin Owner

Plus the Django style guide says "Don’t limit lines of code to 79 characters if it means the code looks significantly uglier or is harder to read". I think this counts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
django/db/backends/schema.py
((52 lines not shown))
+
+ sql_create_pk = "ALTER TABLE %(table)s ADD CONSTRAINT %(name)s PRIMARY KEY (%(columns)s)"
+ sql_delete_pk = "ALTER TABLE %(table)s DROP CONSTRAINT %(name)s"
+
+ def __init__(self, connection):
+ self.connection = connection
+
+ # State-managing methods
+
+ def start(self):
+ """
+ Marks the start of a schema-altering run.
+ """
+ self.deferred_sql = []
+ self.connection.commit_unless_managed()
+ self.connection.enter_transaction_management()
@shaib Collaborator
shaib added a note

Shouldn't the database feature for transactional DDL be consulted about this?

In current databases, where transactional DDL is not supported, DDL generates an implicit commit, which would bring Django's account of the transaction state out of sync with the database. Things may break when somebody uses the API in a way which includes both DDL and DML.

Further, I think it could be reasonable for a database engine which doesn't support transactional DDL, to flag DDL commands under a transaction as errors (although I'm not aware of any engine behaving this way). With current code, the backend for such an engine would then need to override start(); this is not terrible in itself, but start() does other stuff as well.

So, even if you think it unnecessary to check for can_rollback_ddl here, I suggest that the lines controlling the database transaction and its Django management be taken out into their own, separately overridable methods.

@andrewgodwin Owner

I'd say checking for transactional DDL support here makes complete sense - there's no point starting a transaction if they won't do anything, is there. I'll add it in and wrap the check round the transaction methods (I don't want to add too many overrideable sub-methods if I don't have to - it's an easy change to do later if we need it)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@shaib shaib commented on the diff
django/db/backends/schema.py
((105 lines not shown))
+ def column_sql(self, model, field, include_default=False):
+ """
+ Takes a field and returns its column definition.
+ The field must already have had set_attributes_from_name called.
+ """
+ # Get the column's type and use that as the basis of the SQL
+ db_params = field.db_parameters(connection=self.connection)
+ sql = db_params['type']
+ params = []
+ # Check for fields that aren't actually columns (e.g. M2M)
+ if sql is None:
+ return None
+ # Optionally add the tablespace if it's an implicitly indexed column
+ tablespace = field.db_tablespace or model._meta.db_tablespace
+ if tablespace and self.connection.features.supports_tablespaces and field.unique:
+ sql += " %s" % self.connection.ops.tablespace_sql(tablespace, inline=True)
@shaib Collaborator
shaib added a note

Some databases are very picky about the order of clauses in statements. I think it better to write this function as
1. Build an object representing the required SQL features, and
2. Pass it to another method ("format_column_sql") for combining them.

@andrewgodwin Owner

Do you have some examples of which ones are picky and how? Things are built in a certain order already, so it might be possible to have a less complex way of ensuring the order works for the picky cases.

@shaib Collaborator
shaib added a note

3 months this waited for me to have time...

Oracle, for one, insists on having DEFAULT values specified before constraints such as NULL, UNIQUE or PRIMARY KEY.
http://docs.oracle.com/cd/B19306_01/server.102/b14200/statements_7002.htm#CEGEDHJE
Oracle does not seem to support column-specific collation orders.

Postgres doesn't care about DEFAULT (treats default as one more constraint), seems to want COLLATE before any other clause.

MSSQL's documentation (http://msdn.microsoft.com/en-us/library/ms174979.aspx) implies that it wants the NULL before the DEFAULT, but I just checked with a 2008 and it doesn't seem to care. By the same docs, it wants the collation to come before the NULL or DEFAULT, and it does care that COLLATE come before NULL (but not default).

MySql's documentation (http://dev.mysql.com/doc/refman/5.5/en/create-table.html) implies that it wants NULL before DEFAULT and UNIQUE/PRIMARY KEY later, I don't have a mysql on hand to check. It implies more strongly that character collation definitions need to come before any of those (treated as part of the data type);

Sqlite (http://www.sqlite.org/lang_createtable.html) ignores most options, and seems to not care much about order of clauses.

All in all, it seems like there can be an "agreed order" that fits all currently supported databases (and even MSSQL, which is not currently in core), but I think structuring things the way I suggested will make this more future-proof and 3rd-party-backend friendly.

@andrewgodwin Owner

Alright, you're probably right - it's best not to rely on everything being sane in future (who knows what other DBs will do). Perhaps just a simple dict-like structure would work?

@andrewgodwin Owner

Right. Me and Alex discussed this last night and I'll probably just go with a mutable object rather than a dict, but the idea will be the same regardless.

@shaib Collaborator
shaib added a note

Yes, sounds good to me.

@carljm Owner
carljm added a note

Is this still in the plans?

@andrewgodwin Owner

It's currently bumped to "when it's needed", as this code is currently working fine and passing tests on all four backends (some reordering is possible in the overrideable SQL fragments anyway)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
django/db/backends/schema.py
((123 lines not shown))
+ # Oracle treats the empty string ('') as null, so coerce the null
+ # option whenever '' is a possible value.
+ if (field.empty_strings_allowed and not field.primary_key and
+ self.connection.features.interprets_empty_strings_as_nulls):
+ null = True
+ if null:
+ sql += " NULL"
+ else:
+ sql += " NOT NULL"
+ # Primary key/unique outputs
+ if field.primary_key:
+ sql += " PRIMARY KEY"
+ elif field.unique:
+ sql += " UNIQUE"
+ # If we were told to include a default value, do so
+ default_value = self.effective_default(field)
@shaib Collaborator
shaib added a note

Shouldn't this line be also under the "if" below it?

@andrewgodwin Owner

Yep, it should be, I'll change that.

@andrewgodwin Owner

Wait, no, it's fine there - at worst it'll set it to null when it's already null.

@shaib Collaborator
shaib added a note

I commented under the impression that effective_default() may call a potentially expensive (and perhaps even state-changing) callable, which isn't necessary if include_default is false. If we're dropping the callable execution, then it does't really matter.

@andrewgodwin Owner

Ah, right, I see - I think I was misreading the diff location. You're right that given the other change, this becomes a lot less expensive.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
django/db/backends/schema.py
((189 lines not shown))
+ "name": self._create_index_name(model, [field.column], suffix=""),
+ "table": self.quote_name(model._meta.db_table),
+ "columns": self.quote_name(field.column),
+ "extra": "",
+ }
+ )
+ # FK
+ if field.rel and self.connection.features.supports_foreign_keys:
+ to_table = field.rel.to._meta.db_table
+ to_column = field.rel.to._meta.get_field(field.rel.field_name).column
+ self.deferred_sql.append(
+ self.sql_create_fk % {
+ "name": '%s_refs_%s_%x' % (
+ field.column,
+ to_column,
+ abs(hash((model._meta.db_table, to_table)))
@shaib Collaborator
shaib added a note

Don't you want to use DatabaseCreation._digest() here? But more importantly, when you're already including two column names, it's surprisingly easy to go over Oracle's 30-character limit; whatever name comes out, you need to pass it through shortening, so it makes little sense to shorten the table names specifically.

@andrewgodwin Owner

Well, create_index_name already shortens names so they fit within whatever the limit is set to in the features. Plus, I don't want this module to depend on creation (as that's going away eventually), so that's why it's not used. Precise matching of names with the old code is not important, as anything which changes indexes/constraints does a search for them by columns and type first.

@shaib Collaborator
shaib added a note

The value generated by the """'%s_refs_%s_%x' % ... """ expression is used, as is, as a name in an SQL statement. If it goes over 30, nothing will stop it (except an Oracle "Invalid Identifer" or some such).

@andrewgodwin Owner

Good point - I've changed the FK names to use create_index_name so this is all solved in one place.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
django/db/backends/schema.py
((255 lines not shown))
+ self.execute(
+ self.sql_delete_unique % {
+ "table": self.quote_name(model._meta.db_table),
+ "name": constraint_names[0],
+ },
+ )
+ # Created uniques
+ for fields in news.difference(olds):
+ columns = [model._meta.get_field_by_name(field)[0].column for field in fields]
+ self.execute(self.sql_create_unique % {
+ "table": self.quote_name(model._meta.db_table),
+ "name": self._create_index_name(model, columns, suffix="_uniq"),
+ "columns": ", ".join(self.quote_name(column) for column in columns),
+ })
+
+ def alter_db_table(self, model, old_db_table, new_db_table):
@shaib Collaborator
shaib added a note

What is the model doing here?

@andrewgodwin Owner

Providing the field objects, which themselves provide the column names for the fields specified in the unique_together (as, for example, an FK will have a different column name)

@shaib Collaborator
shaib added a note

but this is just a table rename... what am I missing?

@andrewgodwin Owner

Sorry, misread the diff again, you're referring to the bottom line. It's not needed here, but I wanted to ensure that model was available to all operations, as that's something that the SQLite backend has proved is useful (for example, if renaming a table involved making a new one, copying data over, and deleting it from the old one on some backend), and this is the level of API it's really hard to change once it's public.

We also might need the model for things like the database router, the tablespace, or some other DB option.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
django/db/backends/schema.py
((302 lines not shown))
+ return
+ # Check constraints can go on the column SQL here
+ db_params = field.db_parameters(connection=self.connection)
+ if db_params['check']:
+ definition += " CHECK (%s)" % db_params['check']
+ # Build the SQL and run it
+ sql = self.sql_create_column % {
+ "table": self.quote_name(model._meta.db_table),
+ "column": self.quote_name(field.column),
+ "definition": definition,
+ }
+ self.execute(sql, params)
+ # Drop the default if we need to
+ # (Django usually does not use in-database defaults)
+ if not keep_default and field.default is not None:
+ sql = self.sql_alter_column % {
@shaib Collaborator
shaib added a note

Bug: This sql string is never used.

@andrewgodwin Owner

Fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
django/db/backends/schema.py
((335 lines not shown))
+ "column": self.quote_name(field.column),
+ "to_table": self.quote_name(to_table),
+ "to_column": self.quote_name(to_column),
+ }
+ )
+
+ def delete_field(self, model, field):
+ """
+ Removes a field from a model. Usually involves deleting a column,
+ but for M2Ms may involve deleting a table.
+ """
+ # Special-case implicit M2M tables
+ if isinstance(field, ManyToManyField) and field.rel.through._meta.auto_created:
+ return self.delete_model(field.rel.through)
+ # Get the column's definition
+ definition, params = self.column_sql(model, field)
@shaib Collaborator
shaib added a note

Is there no cheaper way to find out if the field has a column?

In particular, if you accept my suggestion above re defaults, you might find that calling column_sql adds a deferred statement to remove the default of a column which, by then, wouldn't exist.

@andrewgodwin Owner

I've made this a lot cheaper by getting the type directly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
django/db/backends/schema.py
((371 lines not shown))
+ old_type = old_db_params['type']
+ new_db_params = new_field.db_parameters(connection=self.connection)
+ new_type = new_db_params['type']
+ if old_type is None and new_type is None and (old_field.rel.through and new_field.rel.through and old_field.rel.through._meta.auto_created and new_field.rel.through._meta.auto_created):
+ return self._alter_many_to_many(model, old_field, new_field, strict)
+ elif old_type is None or new_type is None:
+ raise ValueError("Cannot alter field %s into %s - they are not compatible types (probably means only one is an M2M with implicit through model)" % (
+ old_field,
+ new_field,
+ ))
+ # Has unique been removed?
+ if old_field.unique and not new_field.unique:
+ # Find the unique constraint for this field
+ constraint_names = self._constraint_names(model, [old_field.column], unique=True)
+ if strict and len(constraint_names) != 1:
+ raise ValueError("Found wrong number (%s) of constraints for %s.%s" % (
@shaib Collaborator
shaib added a note

Cryptic error message. How about "Couldn't find single unique constraint for %s.%s (there are %d)"

@andrewgodwin Owner

These error messages generally only appear in tests (when strict is on), so having the number in there is really useful - still, I'll change the message to have type in there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
django/db/backends/mysql/schema.py
@@ -0,0 +1,26 @@
+from django.db.backends.schema import BaseDatabaseSchemaEditor
+
+
+class DatabaseSchemaEditor(BaseDatabaseSchemaEditor):
+
+ sql_rename_table = "RENAME TABLE %(old_table)s TO %(new_table)s"
+
+ sql_alter_column_null = "MODIFY %(column)s %(type)s NULL"
+ sql_alter_column_not_null = "MODIFY %(column)s %(type)s NULL"
@shaib Collaborator
shaib added a note

should be "... %(type)s NOT NULL", I think.

@andrewgodwin Owner

Yep, you're right there! Fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
django/db/backends/schema.py
((482 lines not shown))
+ },
+ [new_default],
+ ))
+ # Nullability change?
+ if old_field.null != new_field.null:
+ if new_field.null:
+ actions.append((
+ self.sql_alter_column_null % {
+ "column": self.quote_name(new_field.column),
+ "type": new_type,
+ },
+ [],
+ ))
+ else:
+ actions.append((
+ self.sql_alter_column_null % {
@shaib Collaborator
shaib added a note

Should be self.sql_alter_column_not_null, I think.

@andrewgodwin Owner

Yep, have fixed this and added a test to catch it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
django/db/backends/schema.py
((512 lines not shown))
+ "table": self.quote_name(model._meta.db_table),
+ "changes": sql,
+ },
+ params,
+ )
+ # Added a unique?
+ if not old_field.unique and new_field.unique:
+ self.execute(
+ self.sql_create_unique % {
+ "table": self.quote_name(model._meta.db_table),
+ "name": self._create_index_name(model, [new_field.column], suffix="_uniq"),
+ "columns": self.quote_name(new_field.column),
+ }
+ )
+ # Added an index?
+ if not old_field.db_index and new_field.db_index and not old_field.unique and not new_field.unique:
@shaib Collaborator
shaib added a note

Does this behave correctly if I drop a unique constraint and add an index instead?

@andrewgodwin Owner

It does now - have just fixed it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
django/db/backends/schema.py
((579 lines not shown))
+ "column": self.quote_name(new_field.column),
+ "check": new_db_params['check'],
+ }
+ )
+
+ def _alter_many_to_many(self, model, old_field, new_field, strict):
+ """
+ Alters M2Ms to repoint their to= endpoints.
+ """
+ # Rename the through table
+ self.alter_db_table(old_field.rel.through, old_field.rel.through._meta.db_table, new_field.rel.through._meta.db_table)
+ # Repoint the FK to the other side
+ self.alter_field(
+ new_field.rel.through,
+ old_field.rel.through._meta.get_field_by_name(old_field.m2m_reverse_field_name())[0],
+ new_field.rel.through._meta.get_field_by_name(new_field.m2m_reverse_field_name())[0],
@shaib Collaborator
shaib added a note

If it's indeed the 'm2m_reverse_field_name()' that's needed here, this is worth a comment.

@andrewgodwin Owner

Comment added.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
django/db/backends/schema.py
((592 lines not shown))
+ new_field.rel.through,
+ old_field.rel.through._meta.get_field_by_name(old_field.m2m_reverse_field_name())[0],
+ new_field.rel.through._meta.get_field_by_name(new_field.m2m_reverse_field_name())[0],
+ )
+
+ def _create_index_name(self, model, column_names, suffix=""):
+ """
+ Generates a unique name for an index/unique constraint.
+ """
+ # If there is just one column in the index, use a default algorithm from Django
+ if len(column_names) == 1 and not suffix:
+ return truncate_name(
+ '%s_%s' % (model._meta.db_table, BaseDatabaseCreation._digest(column_names[0])),
+ self.connection.ops.max_name_length()
+ )
+ # Else generate the name for the index by South
@shaib Collaborator
shaib added a note

Freudian slip...

@andrewgodwin Owner

Ahem. Fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@shaib shaib commented on the diff
django/db/backends/schema.py
((599 lines not shown))
+ Generates a unique name for an index/unique constraint.
+ """
+ # If there is just one column in the index, use a default algorithm from Django
+ if len(column_names) == 1 and not suffix:
+ return truncate_name(
+ '%s_%s' % (model._meta.db_table, BaseDatabaseCreation._digest(column_names[0])),
+ self.connection.ops.max_name_length()
+ )
+ # Else generate the name for the index by South
+ table_name = model._meta.db_table.replace('"', '').replace('.', '_')
+ index_unique_name = '_%x' % abs(hash((table_name, ','.join(column_names))))
+ # If the index name is too long, truncate it
+ index_name = ('%s_%s%s%s' % (table_name, column_names[0], index_unique_name, suffix)).replace('"', '').replace('.', '_')
+ if len(index_name) > self.connection.features.max_index_name_length:
+ part = ('_%s%s%s' % (column_names[0], index_unique_name, suffix))
+ index_name = '%s%s' % (table_name[:(self.connection.features.max_index_name_length - len(part))], part)
@shaib Collaborator
shaib added a note

What if len(part)>max_index_name_length? Say, Oracle, and len(column_names[0])>21?

@andrewgodwin Owner

Good point. Have added a second shortening pass that if this result is too long, just use a hash of it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
tests/modeltests/schema/tests.py
((40 lines not shown))
+ def tearDown(self):
+ # Delete any tables made for our models
+ self.delete_tables()
+ # Rollback anything that may have happened
+ connection.rollback()
+ connection.leave_transaction_management()
+ cache.set_cache(default_cache)
+ cache.app_models['schema'] = {} # One M2M gets left in the old cache
+
+ def delete_tables(self):
+ "Deletes all model tables for our models for a clean test environment"
+ cursor = connection.cursor()
+ connection.disable_constraint_checking()
+ for model in self.models:
+ # Remove any M2M tables first
+ for field in model._meta.local_many_to_many:
@shaib Collaborator
shaib added a note

Don't you want to limit this to auto-created tables?

@andrewgodwin Owner

Why? This is only the tests, and we don't want any tables from the schema app in there when we're done.

@shaib Collaborator
shaib added a note

Because this causes your non-existent-table errors below... You delete the non-auto M2M tables once as M2M and once as models.

@andrewgodwin Owner

But the models that are deleted are from a set list, and none of them are through models for M2M fields, so that will never be the case, or at least as far as I can tell.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@ptone
Collaborator

Andrew - it's occurred to me that this may not address the situation when someone starts a project then switches to a swapped user model - I am NOT talking about the data migration fore user data (people are own for that) - but raising the issue of what, if anything, needs to be done when a model._meta.swapped goes from False to True from one migration to the next.

Not really being familiar with the core approach here, I don't even know if anything needs to be done, just realizing that this was probably developed before _meta.swapped was introduced and pointing that out. cc @freakboy3742

@freakboy3742

@ptone - @andrewgodwin is at least peripherally aware of the problem; I've already raised a bug on South's tracker about integration with AUTH_USER_MODEL.

Fully swapping the User model is a bit of a nightmare; On the South ticket, I suggested that a perfectly acceptable first pass is to disallow this - i.e., initial sync records the AUTH_USER_MODEL in use, and future migrations won't allow it to be changed. This is what we suggest in the docs anyway, so I don't see any problem with enforcing it.

@andrewgodwin

Yes, Russ has filed this in the South tracker already and I've not been able to take a good look at it yet. I suspect the first pass will be that we just moan, though I'd like to do something more intelligent (spit out a skeleton migration for swapping the tables out and preserving data, and letting them finish it off, perhaps).

andrewgodwin added some commits
@andrewgodwin andrewgodwin Merge branch 'master' into schema-alteration
Conflicts:
	django/db/backends/__init__.py
	django/db/models/fields/related.py
	django/db/models/options.py
6a632e0
@andrewgodwin andrewgodwin Merge remote-tracking branch 'core/master' into schema-alteration
Conflicts:
	django/db/models/loading.py
	django/db/models/options.py
b62e823
@andrewgodwin andrewgodwin Merge branch 'master' into schema-alteration
Conflicts:
	django/db/backends/__init__.py
	django/db/backends/mysql/base.py
	django/db/backends/oracle/base.py
	django/db/backends/oracle/creation.py
	django/db/backends/postgresql_psycopg2/base.py
	django/db/backends/sqlite3/base.py
	django/db/models/fields/related.py
7f3678d
@andrewgodwin andrewgodwin Fix schema editor interaction with new transactions 6e21a59
@charettes

The callable check could be moved inside the field.has_default() conditional.

@alex
Collaborator

This definitely needs a test.

Agreed.

@gavinwahl

This looks like a perfect opportunity for dependency injection. Have you considered passing the loader and recorder instances in as arguments? If you wanted to go all the way, you could even remove the dependency on connection and instead pass in a schema editor factory (it would normally be the connection.schema_editor function).

I'd rather not do that, since I'm treating the interface between this and Loader/Recorder as pretty specific - it's trivial to subclass this and change the classes used if needs be, and I like my method signatures nice and clean.

The schema_editor idea also won't work, alas - schema_editor auto-encapsulates things in a transaction and the design requires one transaction per migration, so it's necessary to be able to make many schema editors on demand. On top of that, the connection really is the first-class object required here - not only is schema_editor used, but also .features and .introspection.

Fair enough

@gavinwahl

Why is this method in this class? The rest of the class does generic dependency-graph manipulation, which could have other uses. This one method is very specific to migrations though. Could MigrationGraph inherit from a more generic DependencyGraph?

The rest of the class is already built assuming that it's dealing with migrations, though - see things like leaf_nodes - and without wanting to introduce class hierarchies for no reason, this fits reasonably well here. This isn't just a graph of arbitrary edges and nodes - it's specifically one of Migrations, with live, runnable Migration instances inside.

South has a bit too much indirection and classes-for-everything, so I'm trying to simplify on the design here - no point prematurely optimising code layout. It's trivial to move this method if needed as the design becomes more solid.

@fatbusinessman

I may be reading this wrong, but it looks to me like you'll end up with unpredictable results when renaming a column while adding a column with the same signature. In that situation, you should probably explode and ask the user what to do.

Imagine I have a model like so:

class Grease(models.Model):
    which_one_do_i_want = models.CharField(max_length=255)
    i_got_chills = models.BooleanField()

And I change the model to be like this, then attempt to generate a migration:

class Grease(models.Model):
    which_one_do_i_want = models.CharField(max_length=255)
    theyre_multiplying = models.BooleanField()
    and_im_losing_control = models.BooleanField()

Where is my data from "i_got_chills" now?

P.S. I hate that song and now can't get it out of my head. Ooh ooh ooh.

Jonty: In the real version it'll prompt you about this (and possibly do slightly less strict matching, i.e. ignoring changes to defaults). That'll be done via the Questioner classes.

Then everything is lovely and I shall have a beer to celebrate.

andrewgodwin added some commits
@andrewgodwin andrewgodwin Prompt about renames rather than doing them automatically cca4070
@andrewgodwin andrewgodwin Remove EmailField max_length default removal in deconstruct() 48493cf
@andrewgodwin andrewgodwin Autodetect ForeignKeys and add dependencies/split on circulars e2d7e83
@andrewgodwin andrewgodwin Merge remote-tracking branch 'core/master' into schema-alteration
Conflicts:
	django/db/backends/__init__.py
	django/db/models/fields/related.py
	tests/field_deconstruction/tests.py
7a47ba6
@andrewgodwin andrewgodwin Merge branch 'master' into schema-alteration b1e0ec0
@andrewgodwin andrewgodwin Fix M2M interaction with transactions 310cdf4
@andrewgodwin andrewgodwin Add unique_together altering operation 67dcea7
@andrewgodwin andrewgodwin Autodetection of unique_together changes 3b20af3
@andrewgodwin andrewgodwin Support for index_together in schema backends 6a8cfbf
@andrewgodwin andrewgodwin Fix combined alters on PostgreSQL f343cbf
@andrewgodwin andrewgodwin Fix some bad test running under PostgreSQL 9ef715d
@andrewgodwin andrewgodwin Fix get_constraints to do multi-column indexes properly on pg dbd3e77
@andrewgodwin andrewgodwin Fix index_together test 2202e3f
@andrewgodwin andrewgodwin Add AlterIndexTogether operation 61ff46c
@andrewgodwin andrewgodwin Make get_constraints return columns in order 3a6580e
@andrewgodwin andrewgodwin Merge branch 'master' into schema-alteration
Conflicts:
	django/db/backends/mysql/introspection.py
	django/db/backends/oracle/creation.py
	django/db/backends/postgresql_psycopg2/creation.py
	django/db/models/base.py
	django/db/models/loading.py
03ec321
@andrewgodwin andrewgodwin Make multi-app-cache tests work again 52eb19b
@andrewgodwin andrewgodwin Make migrate command recognise prefixes and 'zero'. 162f7b9
@andrewgodwin andrewgodwin Add tests for the migrate command and fix a bug they exposed 00276e0
@andrewgodwin andrewgodwin Small start to migrations documentation 06103c8
@andrewgodwin andrewgodwin More migration docs, and conversion of all easy syncdb references f8297f6
@andrewgodwin andrewgodwin Add test for creating M2Ms a758c9c
django/core/management/commands/migrate.py
((42 lines not shown))
+ for app_name in settings.INSTALLED_APPS:
+ try:
+ import_module('.management', app_name)
+ except ImportError as exc:
+ # This is slightly hackish. We want to ignore ImportErrors
+ # if the "management" module itself is missing -- but we don't
+ # want to ignore the exception if the management module exists
+ # but raises an ImportError for some reason. The only way we
+ # can do this is to check the text of the exception. Note that
+ # we're a bit broad in how we check the text, because different
+ # Python implementations may not use the same text.
+ # CPython uses the text "No module named management"
+ # PyPy uses "No module named myproject.myapp.management"
+ msg = exc.args[0]
+ if not msg.startswith('No module named') or 'management' not in msg:
+ raise
@alex Collaborator
alex added a note

Since you're moving this around: there's no a ahelper for this nonsense logic in django.utils.module_loading, please use it :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
django/core/management/commands/migrate.py
((162 lines not shown))
+ # Note that if a model is unmanaged we short-circuit and never try to install it
+ return not ((converter(opts.db_table) in tables) or
+ (opts.auto_created and converter(opts.auto_created._meta.db_table) in tables))
+
+ manifest = SortedDict(
+ (app_name, list(filter(model_installed, model_list)))
+ for app_name, model_list in all_models
+ )
+
+ create_models = set([x for x in itertools.chain(*manifest.values())])
+ emit_pre_sync_signal(create_models, self.verbosity, self.interactive, connection.alias)
+
+ # Create the tables for each model
+ if self.verbosity >= 1:
+ self.stdout.write(" Creating tables...\n")
+ with transaction.commit_on_success_unless_managed(using=connection.alias):
@alex Collaborator
alex added a note

Is this even a thing anymore?

@andrewgodwin Owner

Well, this guarantees that a commit is issued even if it's nested, which atomic() doesn't do. I'll port over the logic from that function and just do it with atomic.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
django/db/backends/schema.py
@@ -0,0 +1,666 @@
+import sys
+import hashlib
+import operator
+from django.db.backends.creation import BaseDatabaseCreation
+from django.db.backends.util import truncate_name
+from django.utils.log import getLogger
+from django.db.models.fields.related import ManyToManyField
+from django.db.transaction import atomic
+from django.utils.six.moves import reduce
@alex Collaborator
alex added a note

Please order the imports alpahebtically and put a newline between stdlib and django.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
django/db/migrations/recorder.py
((7 lines not shown))
+ """
+ Deals with storing migration records in the database.
+
+ Because this table is actually itself used for dealing with model
+ creation, it's the one thing we can't do normally via syncdb or migrations.
+ We manually handle table creation/schema updating (using schema backend)
+ and then have a floating model to do queries with.
+
+ If a migration is unapplied its row is removed from the table. Having
+ a row in the table always means a migration is applied.
+ """
+
+ class Migration(models.Model):
+ app = models.CharField(max_length=255)
+ name = models.CharField(max_length=255)
+ applied = models.DateTimeField(default=datetime.datetime.utcnow)
@brosner Collaborator
brosner added a note

When I run migrate Django spews out naive datetime objects being used due to USE_TZ = True. Is this something that be changed to use Django's timezone.now function?

@andrewgodwin Owner

You're entirely right. Fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@shaib shaib commented on the diff
django/db/migrations/recorder.py
((7 lines not shown))
+ """
+ Deals with storing migration records in the database.
+
+ Because this table is actually itself used for dealing with model
+ creation, it's the one thing we can't do normally via syncdb or migrations.
+ We manually handle table creation/schema updating (using schema backend)
+ and then have a floating model to do queries with.
+
+ If a migration is unapplied its row is removed from the table. Having
+ a row in the table always means a migration is applied.
+ """
+
+ class Migration(models.Model):
+ app = models.CharField(max_length=255)
+ name = models.CharField(max_length=255)
+ applied = models.DateTimeField(default=now)
@shaib Collaborator
shaib added a note

In certain situations -- mostly trying to salvage a database botched by user errors -- I would have found it useful to know which migrations were run for real, and which faked.
My first thought was to add a flag for this here, but that would give me only half of the story at best (backwards migrations and backwards fake migrations are just as important in these situations).
I don't have a concrete, full suggeation for improvement, but perhaps some sort of "migration run history" log should be kept in a way that is accessible. Not necessarily in the database, of course.

@andrewgodwin Owner

Yes, I agree it would be nice - mostly to save some users from themselves - but definitely more of a logging thing than a database table thing. Django doesn't really have a great way to do persistent logs, though...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
django/core/management/commands/flush.py
@@ -7,7 +7,8 @@
from django.core.management import call_command
from django.core.management.base import NoArgsCommand, CommandError
from django.core.management.color import no_style
-from django.core.management.sql import sql_flush, emit_post_sync_signal
+from django.core.management.sql import sql_flush, emit_post_migrate_signal
+from django.utils.importlib import import_module
@timgraham Owner

unused import?

@andrewgodwin Owner

No, it's used, see line 47.

@andrewgodwin Owner

Oh, no, see what you mean now. Fixed.

@timgraham Owner

Ok, I think it's just a bad merge resolution since it looks like your change is adding this back. It's been replaced with from importlib import import_module on master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
django/core/management/commands/makemigrations.py
((14 lines not shown))
+
+
+class Command(BaseCommand):
+ option_list = BaseCommand.option_list + (
+ make_option('--empty', action='store_true', dest='empty', default=False,
+ help='Make a blank migration.'),
+ )
+
+ help = "Creates new migration(s) for apps."
+ usage_str = "Usage: ./manage.py makemigrations [--empty] [app [app ...]]"
+
+ def handle(self, *app_labels, **options):
+
+ self.verbosity = int(options.get('verbosity'))
+ self.interactive = options.get('interactive')
+ self.style = color_style()
@timgraham Owner

BaseCommand.init sets up self.style -- is there a reason to override it here?

@andrewgodwin Owner

Nope. Removed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
django/core/management/commands/migrate.py
@@ -0,0 +1,247 @@
+from optparse import make_option
+from collections import OrderedDict
+import itertools
+import traceback
+
+from django.conf import settings
+from django.core.management import call_command
+from django.core.management.base import BaseCommand, CommandError
+from django.core.management.color import color_style, no_style
+from django.core.management.sql import custom_sql_for_model, emit_post_migrate_signal, emit_pre_migrate_signal
+from django.db import connections, router, transaction, models, DEFAULT_DB_ALIAS
+from django.db.migrations.executor import MigrationExecutor
+from django.db.migrations.loader import AmbiguityError
+from django.utils.importlib import import_module
@timgraham Owner

this has been deprecated on master, use from importlib import import_module instead

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@timgraham timgraham commented on the diff
django/core/management/commands/migrate.py
((24 lines not shown))
+ make_option('--database', action='store', dest='database',
+ default=DEFAULT_DB_ALIAS, help='Nominates a database to synchronize. '
+ 'Defaults to the "default" database.'),
+ make_option('--fake', action='store_true', dest='fake', default=False,
+ help='Mark migrations as run without actually running them'),
+ )
+
+ help = "Updates database schema. Manages both apps with migrations and those without."
+
+ def handle(self, *args, **options):
+
+ self.verbosity = int(options.get('verbosity'))
+ self.interactive = options.get('interactive')
+ self.show_traceback = options.get('traceback')
+ self.load_initial_data = options.get('load_initial_data')
+ self.test_database = options.get('test_database', False)
@timgraham Owner

unused?

@andrewgodwin Owner

It's for future use - I suspect there'll be a need for migrate to know if it's being used for testing or not. It's passed in as a parameter in db/backends/creation.py, line 343.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
django/core/management/commands/migrate.py
((26 lines not shown))
+ 'Defaults to the "default" database.'),
+ make_option('--fake', action='store_true', dest='fake', default=False,
+ help='Mark migrations as run without actually running them'),
+ )
+
+ help = "Updates database schema. Manages both apps with migrations and those without."
+
+ def handle(self, *args, **options):
+
+ self.verbosity = int(options.get('verbosity'))
+ self.interactive = options.get('interactive')
+ self.show_traceback = options.get('traceback')
+ self.load_initial_data = options.get('load_initial_data')
+ self.test_database = options.get('test_database', False)
+
+ self.style = color_style()
@timgraham Owner

as above

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@timgraham timgraham commented on the diff
django/core/management/commands/migrate.py
((86 lines not shown))
+
+ # Print some useful info
+ if self.verbosity >= 1:
+ self.stdout.write(self.style.MIGRATE_HEADING("Operations to perform:"))
+ if run_syncdb:
+ self.stdout.write(self.style.MIGRATE_LABEL(" Synchronize unmigrated apps: ") + (", ".join(executor.loader.unmigrated_apps) or "(none)"))
+ if target_app_labels_only:
+ self.stdout.write(self.style.MIGRATE_LABEL(" Apply all migrations: ") + (", ".join(set(a for a, n in targets)) or "(none)"))
+ else:
+ if targets[0][1] is None:
+ self.stdout.write(self.style.MIGRATE_LABEL(" Unapply all migrations: ") + "%s" % (targets[0][0], ))
+ else:
+ self.stdout.write(self.style.MIGRATE_LABEL(" Target specific migration: ") + "%s, from %s" % (targets[0][1], targets[0][0]))
+
+ # Run the syncdb phase.
+ # If you ever manage to get rid of this, I owe you many, many drinks.
@timgraham Owner

comment is unclear

@andrewgodwin Owner

Unclear how? If someone does manage to remove the rest of the syncdb code and make Django fully migrations-only, I'll definitely buy them a drink.

@timgraham Owner

comment was more clear once I saw that "sync_apps" is a huge, ugly function :-)

@carljm Owner
carljm added a note

Is there a technical challenge to doing so, or is it just a policy question? I mean, it doesn't seem like it would be that hard to just say "migrations are now how you make anything happen in the database, period." and remove the syncdb code and force people to always make migrations. I kind of think that's a good idea, personally, though it would definitely require discussion, and could just as well be done in a later release.

@andrewgodwin Owner

That's what I want to do eventually, yes, but it's too much to accomplish in one release as it would break a hell of a lot more stuff.

@carljm Owner
carljm added a note

Yup, agreed - better left to a later release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@timgraham timgraham commented on the diff
django/db/backends/creation.py
((10 lines not shown))
verbosity=max(verbosity - 1, 0),
interactive=False,
database=self.connection.alias,
- load_initial_data=False)
+ load_initial_data=False,
+ test_database=True)
@timgraham Owner

used? (commented above)

@andrewgodwin Owner

See comment above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
django/db/backends/mysql/introspection.py
((57 lines not shown))
+ for table, non_unique, index, colseq, column in [x[:5] for x in cursor.fetchall()]:
+ if index not in constraints:
+ constraints[index] = {
+ 'columns': SortedSet(),
+ 'primary_key': False,
+ 'unique': False,
+ 'index': True,
+ 'check': False,
+ 'foreign_key': None,
+ }
+ constraints[index]['index'] = True
+ constraints[index]['columns'].add(column)
+ # Convert the sorted sets to lists
+ for constraint in constraints.values():
+ constraint['columns'] = list(constraint['columns'])
+ # Return
@timgraham Owner

comment needed?

@andrewgodwin Owner

Where? This function seems pretty well commented to me.

@timgraham Owner

"# Return"

@andrewgodwin Owner

Oh, you mean "this comment isn't needed" instead of "another comment needed?". I'll remove it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
django/db/backends/schema.py
((76 lines not shown))
+ atomic(self.connection.alias, self.connection.features.can_rollback_ddl).__exit__(None, None, None)
+ else:
+ # Continue propagating exception
+ return None
+
+ # Core utility functions
+
+ def execute(self, sql, params=[], fetch_results=False):
+ """
+ Executes the given SQL statement, with optional parameters.
+ """
+ # Get the cursor
+ cursor = self.connection.cursor()
+ # Log the command we're running, then run it
+ logger.debug("%s; (params %r)" % (sql, params))
+ #print("%s; (params %r)" % (sql, params))
@timgraham Owner

remove debug?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
django/db/backends/schema.py
((68 lines not shown))
+ self.deferred_sql = []
+ atomic(self.connection.alias, self.connection.features.can_rollback_ddl).__enter__()
+ return self
+
+ def __exit__(self, exc_type, exc_value, traceback):
+ if exc_type is None:
+ for sql in self.deferred_sql:
+ self.execute(sql)
+ atomic(self.connection.alias, self.connection.features.can_rollback_ddl).__exit__(None, None, None)
+ else:
+ # Continue propagating exception
+ return None
+
+ # Core utility functions
+
+ def execute(self, sql, params=[], fetch_results=False):
@timgraham Owner

fetch_results kwarg doesn't appear to be used

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
django/db/migrations/autodetector.py
((378 lines not shown))
+ return value
+ except ValueError:
+ pass
+ result = input("Please select a valid option: ")
+
+ def ask_initial(self, app_label):
+ "Should we create an initial migration for the app?"
+ # Don't ask for django.contrib apps
+ app = cache.get_app(app_label)
+ if app.__name__.startswith("django.contrib"):
+ return False
+ # If it was specified on the command line, definitely true
+ if app_label in self.specified_apps:
+ return True
+ # Now ask
+ return self._boolean_input("Do you want to enable migrations for app '%s'?" % app_label)
@apollo13 Owner
apollo13 added a note

Please add [y/n] and mark (with uppercase) what the default is. (For _boolean_input in general, not just this one)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@apollo13
Owner

I just created a migration and the file shows:

    dependencies = [(u'testb', '0001_initial')]

-- That doesn't work on 3.2, you might also want to import unicode_literals, depending on whether you want text or bytes for everything.

EDIT:// The fields also have unicode markers from time to time:

            fields = [(u'id', models.AutoField(verbose_name=u'ID', serialize=False, auto_created=True, primary_key=True),), ('char', models.CharField(max_length=256),), ('fk', models.ForeignKey(to=u'testb.TestB', to_field=u'id'),)],
@apollo13
Owner

Is there any way to not get asked everytime if you want to enable migrations for an app?

@apollo13
Owner

I get migrations which are getting unapplied without me asking for it:

florian@apollo13:~/.virtualenvs/522125f0c8c708a8/migrationtest$ ./manage.py migrate
Operations to perform:
  Synchronize unmigrated apps: sessions, admin, messages, testc, auth, staticfiles, contenttypes
  Apply all migrations: testa, testb
Synchronizing apps without migrations:
  Creating tables...
    Creating table django_admin_log
    Creating table auth_permission
    Creating table auth_group_permissions
    Creating table auth_group
    Creating table auth_user_groups
    Creating table auth_user_user_permissions
    Creating table auth_user
    Creating table django_content_type
    Creating table django_session
    Creating table testc_test
  Installing custom SQL...
  Installing indexes...
Installed 0 object(s) from 0 fixture(s)
Running migrations:
  Applying testb.0001_initial... OK
  Applying testa.0001_initial... OK
  Applying testa.0002_auto... OK
  Unapplying testa.0002_auto... OK
  Unapplying testa.0001_initial... OK

You just installed Django's auth system, which means you don't have any superusers defined.
Would you like to create one now? (yes/no): no
florian@apollo13:~/.virtualenvs/522125f0c8c708a8/migrationtest$ ./manage.py migrate
Operations to perform:
  Synchronize unmigrated apps: sessions, admin, messages, testc, auth, staticfiles, contenttypes
  Apply all migrations: testa, testb
Synchronizing apps without migrations:
  Creating tables...
  Installing custom SQL...
  Installing indexes...
Installed 0 object(s) from 0 fixture(s)
Running migrations:
  Applying testa.0001_initial... OK
  Applying testa.0002_auto... OK
  Unapplying testa.0002_auto... OK
  Unapplying testa.0001_initial... OK

Ping me in IRC if you need details.

@andrewgodwin
Owner

@apollo13 That's weird, I'll look into it.

As for the asking every time, that's something that still needs to be resolved (current solution works, but is REALLY ANNOYING). The proposal I like most is having them enabled in the new app template, and then having this just assume they're disabled unless migrations directory is present.

django/db/migrations/autodetector.py
((17 lines not shown))
+ as it's likely that changes interact (for example, you can't
+ add a ForeignKey without having a migration to add the table it
+ depends on first). A user interface may offer single-app usage
+ if it wishes, with the caveat that it may not always be possible.
+ """
+
+ def __init__(self, from_state, to_state, questioner=None):
+ self.from_state = from_state
+ self.to_state = to_state
+ self.questioner = questioner or MigrationQuestioner()
+
+ def changes(self):
+ """
+ Returns a dict of migration plans which will achieve the
+ change from from_state to to_state. The dict has app labels
+ as kays and a list of migrations as values.
@timgraham Owner

keys*

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@apollo13
Owner

@andrewgodwin more details (from irc):

<apollo13> andrewgodwin: are you around? testa has a fk to testb, testb changes are "stable" so to say
<apollo13> andrewgodwin: also, since django itself asks "Would you like to create one now? (yes/no): no"  -- we might wanna change [y/n] to stay consistent? [sry]

and migrating manually works:

florian@apollo13:~/.virtualenvs/522125f0c8c708a8/migrationtest$ ./manage.py migrate testa 0001_initial
Operations to perform:
  Target specific migration: 0001_initial, from testa
Running migrations:
  Applying testa.0001_initial... OK
@apollo13
Owner

Trying to migrate apps not in INSTALLED_APPS shows a confusing error:

florian@apollo13:~/.virtualenvs/522125f0c8c708a8/migrationtest$ ./manage.py migrate fsdgdshdsfdsgdfshgds 0001_initial
CommandError: App 'fsdgdshdsfdsgdfshgds' does not have migrations (you cannot selectively sync unmigrated apps)

Something like "this app doesn't exist" would be better.

django/db/migrations/graph.py
((1 lines not shown))
+from django.utils.datastructures import SortedSet
+from django.db.migrations.state import ProjectState
+
+
+class MigrationGraph(object):
+ """
+ Represents the digraph of all migrations in a project.
+
+ Each migration is a node, and each dependency is an edge. There are
+ no implicit dependencies between numbered migrations - the numbering is
+ merely a convention to aid file listing. Every new numbered migration
+ has a declared dependency to the previous number, meaning that VCS
+ branch merges can be detected and resolved.
+
+ Migrations files can be marked as replacing another set of migrations -
+ this is to support the "squash" feature. The graph handler isn't resposible
@timgraham Owner

responsible*

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
django/test/runner.py
@@ -267,7 +267,7 @@ def setup_databases(verbosity, interactive, **kwargs):
# Second pass -- actually create the databases.
old_names = []
mirrors = []
-
+
@timgraham Owner

bad whitespace change

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
django/utils/datastructures.py
@@ -236,6 +236,36 @@ def clear(self):
super(SortedDict, self).clear()
self.keyOrder = []
+class SortedSet(object):
+ """
+ A set which keeps the ordering of the inserted items.
+ Currently backs onto SortedDict.
@timgraham Owner

SortedDict is deprecated in master, can we make it use OrderedDict instead?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@timgraham timgraham commented on the diff
docs/ref/django-admin.txt
@@ -572,6 +570,48 @@ Use the ``--keep-pot`` option to prevent django from deleting the temporary
.pot file it generates before creating the .po file. This is useful for
debugging errors which may prevent the final language files from being created.
+makemigrations [<appname>]
+--------------------------
+
+.. django-admin:: makemigrations
+
@timgraham Owner

.. versionadded:: 1.7 for this and migrate

@andrewgodwin Owner

Done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
docs/ref/django-admin.txt
@@ -572,6 +570,48 @@ Use the ``--keep-pot`` option to prevent django from deleting the temporary
.pot file it generates before creating the .po file. This is useful for
debugging errors which may prevent the final language files from being created.
+makemigrations [<appname>]
+--------------------------
+
+.. django-admin:: makemigrations
+
+Creates new migrations based on the changes detected to your models.
+Migrations, their relationship with apps and more are covered in depth in
+:doc:`the migrations documentation</topics/migrations>`.
+
+Providing one or more app names as arguments will limit the migrations created
+to the app specified and any dependencies needed (the table at the other end
@timgraham Owner

app(s) specified

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
docs/ref/django-admin.txt
@@ -572,6 +570,48 @@ Use the ``--keep-pot`` option to prevent django from deleting the temporary
.pot file it generates before creating the .po file. This is useful for
debugging errors which may prevent the final language files from being created.
+makemigrations [<appname>]
+--------------------------
+
+.. django-admin:: makemigrations
+
+Creates new migrations based on the changes detected to your models.
+Migrations, their relationship with apps and more are covered in depth in
+:doc:`the migrations documentation</topics/migrations>`.
+
+Providing one or more app names as arguments will limit the migrations created
+to the app specified and any dependencies needed (the table at the other end
+of a ForeignKey, for example)
@timgraham Owner

add `` around ForeignKey
missing period at end of sentence

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
docs/ref/django-admin.txt
((15 lines not shown))
+of a ForeignKey, for example)
+
+.. django-admin-option:: --empty
+
+The ``--empty`` option will cause ``makemigrations`` to output an empty
+migration for the specified apps, for manual editing. This option is only
+for advanced users and should not be used unless you are familiar with
+the migration format, migration operations and the dependencies between
+your migrations.
+
+migrate [<appname> [<migrationname>]]
+-------------------------------------
+
+.. django-admin:: migrate
+
+Synchronises the database state with the current set of models and migrations.
@timgraham Owner

docs use "Synchronize" with a z

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
docs/ref/django-admin.txt
((7 lines not shown))
+.. django-admin:: makemigrations
+
+Creates new migrations based on the changes detected to your models.
+Migrations, their relationship with apps and more are covered in depth in
+:doc:`the migrations documentation</topics/migrations>`.
+
+Providing one or more app names as arguments will limit the migrations created
+to the app specified and any dependencies needed (the table at the other end
+of a ForeignKey, for example)
+
+.. django-admin-option:: --empty
+
+The ``--empty`` option will cause ``makemigrations`` to output an empty
+migration for the specified apps, for manual editing. This option is only
+for advanced users and should not be used unless you are familiar with
+the migration format, migration operations and the dependencies between
@timgraham Owner

add comma before "and"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
docs/ref/django-admin.txt
((19 lines not shown))
+The ``--empty`` option will cause ``makemigrations`` to output an empty
+migration for the specified apps, for manual editing. This option is only
+for advanced users and should not be used unless you are familiar with
+the migration format, migration operations and the dependencies between
+your migrations.
+
+migrate [<appname> [<migrationname>]]
+-------------------------------------
+
+.. django-admin:: migrate
+
+Synchronises the database state with the current set of models and migrations.
+Migrations, their relationship with apps and more are covered in depth in
+:doc:`the migrations documentation</topics/migrations>`.
+
+The behaviour of this command changes depending on the arguments provided:
@timgraham Owner

"behavior" for consistency

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
docs/ref/signals.txt
((17 lines not shown))
+-----------