Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Migration failure with Django 1.7.2 and sqlite database when using a through model. #285

Closed
lpomfrey opened this issue Jan 6, 2015 · 25 comments

Comments

@lpomfrey
Copy link
Contributor

commented Jan 6, 2015

I'm not entirely sure whether this is an issue with taggit or Django itself, but since I can only reproduce it with taggit I'm reporting it here. Feel free to close as a Django issue if you think this isn't anything to do with taggit.

When using a through model with taggit and Django 1.7.2, migrations always fail with an OperationalError. This only occurs (as far as I know) with the sqlite backend, PostgreSQL, at least, is fine.

To reproduce

Create a simple project with an app with the following models.py

from django.db import models
from taggit.managers import TaggableManager
from taggit.models import TaggedItemBase


class BugTag(TaggedItemBase):

    content_object = models.ForeignKey('Bug')
    note = models.CharField(max_length=50)


class Bug(models.Model):

    name = models.CharField(max_length=100)
    tags = TaggableManager(through=BugTag)

Make your initial migrations with manage.py makemigrations <appname> then run with manage.py migrate.

The migrate command will fail with an exception like:

    Operations to perform:
  Apply all migrations: sessions, admin, auth, contenttypes, taggit, bug
Running migrations:
  Applying bug.0001_initial...Traceback (most recent call last):
  File "./manage.py", line 7, in <module>
    execute_from_command_line(sys.argv)
  File "/home/username/.virtualenvs/sqlitebug/local/lib/python2.7/site-packages/django/core/management/__init__.py", line 385, in execute_from_command_line
    utility.execute()
  File "/home/username/.virtualenvs/sqlitebug/local/lib/python2.7/site-packages/django/core/management/__init__.py", line 377, in execute
    self.fetch_command(subcommand).run_from_argv(self.argv)
  File "/home/username/.virtualenvs/sqlitebug/local/lib/python2.7/site-packages/django/core/management/base.py", line 288, in run_from_argv
    self.execute(*args, **options.__dict__)
  File "/home/username/.virtualenvs/sqlitebug/local/lib/python2.7/site-packages/django/core/management/base.py", line 338, in execute
    output = self.handle(*args, **options)
  File "/home/username/.virtualenvs/sqlitebug/local/lib/python2.7/site-packages/django/core/management/commands/migrate.py", line 161, in handle
    executor.migrate(targets, plan, fake=options.get("fake", False))
  File "/home/username/.virtualenvs/sqlitebug/local/lib/python2.7/site-packages/django/db/migrations/executor.py", line 68, in migrate
    self.apply_migration(migration, fake=fake)
  File "/home/username/.virtualenvs/sqlitebug/local/lib/python2.7/site-packages/django/db/migrations/executor.py", line 102, in apply_migration
    migration.apply(project_state, schema_editor)
  File "/home/username/.virtualenvs/sqlitebug/local/lib/python2.7/site-packages/django/db/migrations/migration.py", line 108, in apply
    operation.database_forwards(self.app_label, schema_editor, project_state, new_state)
  File "/home/username/.virtualenvs/sqlitebug/local/lib/python2.7/site-packages/django/db/migrations/operations/fields.py", line 37, in database_forwards
    field,
  File "/home/username/.virtualenvs/sqlitebug/local/lib/python2.7/site-packages/django/db/backends/sqlite3/schema.py", line 177, in add_field
    self._remake_table(model, create_fields=[field])
  File "/home/username/.virtualenvs/sqlitebug/local/lib/python2.7/site-packages/django/db/backends/sqlite3/schema.py", line 145, in _remake_table
    self.quote_name(model._meta.db_table),
  File "/home/username/.virtualenvs/sqlitebug/local/lib/python2.7/site-packages/django/db/backends/schema.py", line 103, in execute
    cursor.execute(sql, params)
  File "/home/username/.virtualenvs/sqlitebug/local/lib/python2.7/site-packages/django/db/backends/utils.py", line 81, in execute
    return super(CursorDebugWrapper, self).execute(sql, params)
  File "/home/username/.virtualenvs/sqlitebug/local/lib/python2.7/site-packages/django/db/backends/utils.py", line 65, in execute
    return self.cursor.execute(sql, params)
  File "/home/username/.virtualenvs/sqlitebug/local/lib/python2.7/site-packages/django/db/utils.py", line 94, in __exit__
    six.reraise(dj_exc_type, dj_exc_value, traceback)
  File "/home/username/.virtualenvs/sqlitebug/local/lib/python2.7/site-packages/django/db/backends/utils.py", line 65, in execute
    return self.cursor.execute(sql, params)
  File "/home/username/.virtualenvs/sqlitebug/local/lib/python2.7/site-packages/django/db/backends/sqlite3/base.py", line 485, in execute
    return Database.Cursor.execute(self, query, params)
django.db.utils.OperationalError: table bug_bug__new has no column named tags

The SQL causing the issue is:

SELECT name FROM sqlite_master
    WHERE type in ('table', 'view') AND NOT name='sqlite_sequence'
    ORDER BY name
BEGIN
CREATE TABLE "bug_bug" ("id" integer NOT NULL PRIMARY KEY AUTOINCREMENT, "name" varchar(100) NOT NULL)
CREATE TABLE "bug_bugtag" ("id" integer NOT NULL PRIMARY KEY AUTOINCREMENT, "note" varchar(50) NOT NULL, "content_object_id" integer NOT NULL REFERENCES "bug_bug" ("id"), "tag_id" integer NOT NULL REFERENCES "taggit_tag" ("id"))
CREATE TABLE "bug_bug__new" ("id" integer NOT NULL PRIMARY KEY AUTOINCREMENT, "name" varchar(100) NOT NULL)
INSERT INTO "bug_bug__new" ("tags", "id", "name") SELECT NULL, "id", "name" FROM "bug_bug"

Django 1.7.1 doesn't exhibit the issue, and it's present in both the develop branch of taggit and the latest, 0.12.2, release.

gasman added a commit to wagtail/wagtail that referenced this issue Jan 6, 2015
…h sqlite jazzband/django-taggit#285 - pin sqlite tox tests to 1.7.1 for now
@outime

This comment has been minimized.

Copy link

commented Jan 7, 2015

Experienced this as well, had to downgrade to 1.7.1 so I could run my tests.

@frewsxcv

This comment has been minimized.

Copy link
Collaborator

commented Jan 8, 2015

I can also confirm this issue with Taggit 0.12.2 and Django 1.7.2

@frewsxcv

This comment has been minimized.

Copy link
Collaborator

commented Jan 8, 2015

This Django commit is the commit that broke tests for django-taggit.

@frewsxcv

This comment has been minimized.

Copy link
Collaborator

commented Jan 8, 2015

Specifically with this section

    # Add in any created fields
    for field in create_fields:
      body[field.name] = field
-     # If there's a default, insert it into the copy map
-     if field.has_default():
+     # Choose a default and insert it into the copy map
+     if not isinstance(field, ManyToManyField):
         mapping[field.column] = self.quote_value(
           self.effective_default(field)
         )

With Taggit 0.12.2:

  • field.has_default() == False
  • not isinstance(field, ManyToManyField) == True
@coldmind

This comment has been minimized.

Copy link

commented Jan 8, 2015

Manytomany field can not have a default.
If you take a look inside of effective default, there also is a check for field.has_default

@frewsxcv

This comment has been minimized.

Copy link
Collaborator

commented Jan 8, 2015

@coldmind: There's difference though. mapping gets set to different things after that block executes

With Django 1.7.1:

(pdb) mapping
{u'id': u'"id"', u'name': u'"name"'}

With Django 1.7.2:

(pdb) mapping
{u'tags': 'NULL', u'id': u'"id"', u'name': u'"name"'}
@frewsxcv

This comment has been minimized.

Copy link
Collaborator

commented Jan 8, 2015

...and as a result, tags gets added to this SQL statement here which is where this crash happens

@coldmind

This comment has been minimized.

Copy link

commented Jan 8, 2015

@frewsxcv what do you thing is wrong in my commit? It was a bug, sqlite3 not always used effective default, but it should use.
Your TaggableManager has a behavior like ManyToManyField, right? Because problem is that tags receives NULL as default value.
The way I see it can be resolved to check smth like this if field.get_internal_type() != "ManyToManyField": instead of if not isinstance(field, ManyToManyField):, and you will override internal type for TaggableManager, if it will not break something.

@coldmind

This comment has been minimized.

Copy link

commented Jan 8, 2015

Just checked that, migrations passed.
Please test your project, where you will set

    def get_internal_type(self):
        return 'ManyToManyField'

for TaggableManager. If all will be OK, I will create PR for django to fix this issue.

@coldmind

This comment has been minimized.

Copy link

commented Jan 8, 2015

I opened django/django#3865 anyway, because it should be in that way.

@lpomfrey

This comment has been minimized.

Copy link
Contributor Author

commented Jan 9, 2015

I've created PR #286 to add the get_internal_type method to TaggableManager.

@MarkusH

This comment has been minimized.

Copy link

commented Jan 27, 2015

I just committed a patch (PR) to Django's 1.7 branch that uses the get_internal_type(). This patch will be available in Django 1.7.4.

@shacker

This comment has been minimized.

Copy link

commented Feb 25, 2015

The issue is unfortunately still present in Django 1.7.4.

@frewsxcv

This comment has been minimized.

Copy link
Collaborator

commented Feb 25, 2015

@shacker That's because #286 hasn't merged yet

@outime

This comment has been minimized.

Copy link

commented Feb 27, 2015

This issue doesn't exist anymore in Django 1.7.5 (https://docs.djangoproject.com/en/1.7/releases/1.7.5/) as the regression has been fixed: https://code.djangoproject.com/ticket/24236

Edit: someone applied a patch in the project I'm working on and I overlooked that, my apologies for the false hopes.

@gasman

This comment has been minimized.

Copy link
Contributor

commented Feb 27, 2015

@outime Is that with or without the patch from #286 applied? 1.7.5 still fails with the OperationalError for me (running the unit tests for https://github.com/torchbox/wagtail under sqlite).

@lpomfrey

This comment has been minimized.

Copy link
Contributor Author

commented Feb 27, 2015

@outime I think that only applies to sub-classes of ManyToManyField which TaggableManager isn't. In any case, 1.7.5 doesn't fix the issue without without the #286 patch.

@MarkusH

This comment has been minimized.

Copy link

commented Feb 27, 2015

This still requires #286 to be merged

@frewsxcv

This comment has been minimized.

Copy link
Collaborator

commented Feb 27, 2015

I asked both @alex and @apollo13 about this and they both claim they no longer maintain this library. Not sure what to suggest :-/ Maybe time for a fork?

@lpomfrey

This comment has been minimized.

Copy link
Contributor Author

commented Feb 27, 2015

@frewsxcv I don't mind taking over maintenance as long as no one else wants to or has any objections. But, maybe commit access to this repository and the pypi project would be better so it could stay in the same place and keep its name?

@alex

This comment has been minimized.

Copy link
Collaborator

commented Feb 27, 2015

I'd like to find a way to have a maintainer under the current repo/pypi
project/etc.

On Fri, Feb 27, 2015 at 1:22 PM, Luke Pomfrey notifications@github.com
wrote:

@frewsxcv https://github.com/frewsxcv I don't mind taking over
maintenance as long as no one else wants to or has any objections. But,
maybe commit access to this repository and the pypi project would be better
so it could stay in the same place and keep its name?


Reply to this email directly or view it on GitHub
#285 (comment).

"I disapprove of what you say, but I will defend to the death your right to
say it." -- Evelyn Beatrice Hall (summarizing Voltaire)
"The people's good is the highest law." -- Cicero
GPG Key fingerprint: 125F 5C67 DFE9 4084

@lpomfrey

This comment has been minimized.

Copy link
Contributor Author

commented Feb 27, 2015

@alex As I said I've no problem taking over maintenance, providing no one has any objections. If it helps my username is the same on pypi as here.

@SpencerTachick

This comment has been minimized.

Copy link

commented Mar 3, 2015

I just encountered this issue on the latest version of django. I can't add any fields to a model without encountering this. I was able to get it to work yesterday on a column I added, but I'm not sure how...

frewsxcv added a commit that referenced this issue Mar 3, 2015
Add get_internal_type to TaggableManager for #285
@gasman

This comment has been minimized.

Copy link
Contributor

commented Mar 4, 2015

#286 is now merged and released (and I can confirm it works for me) - presumably this can be closed now?

@lpomfrey

This comment has been minimized.

Copy link
Contributor Author

commented Mar 4, 2015

Fixed with the merge of #286

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
9 participants
You can’t perform that action at this time.