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

ManyToManyFields on Page subclasses have the wrong through table name created during migrations #89

Closed
sjdines opened this issue Sep 1, 2015 · 5 comments
Labels

Comments

@sjdines
Copy link
Contributor

sjdines commented Sep 1, 2015

Using Django==1.7.10 or 'Django==1.8.4.there appears to to be an issue where the through table for aManyToManyField` gets made with an incorrect name.

Using the following model structure in a base project:

from django.db import models
from fluent_pages.models import Page


class BasicPage(Page):
    product_category = models.ManyToManyField(
        'sites.Site'
    )

    class Meta:
        verbose_name = 'Basic page'
        verbose_name_plural = 'Basic pages'

I ran ./manage.py makemigrations <app_name> which generated a migration which looks sensible:

# -*- coding: utf-8 -*-
from __future__ import unicode_literals

from django.db import models, migrations


class Migration(migrations.Migration):

    dependencies = [
        ('fluent_pages', '0002_auto_20150901_0528'),
        ('sites', '0001_initial'),
    ]

    operations = [
        migrations.CreateModel(
            name='BasicPage',
            fields=[
                ('urlnode_ptr', models.OneToOneField(parent_link=True, auto_created=True, primary_key=True, serialize=False, to='fluent_pages.UrlNode')),
                ('product_category', models.ManyToManyField(to='sites.Site')),
            ],
            options={
                'db_table': 'pagetype_fluent_test_basicpage',
                'verbose_name': 'Basic page',
                'verbose_name_plural': 'Basic pages',
            },
            bases=('fluent_pages.page',),
        ),
    ]

So all is migrated and I go into the admin and try to save some many to many data:

OperationalError at /admin/fluent_pages/page/add/
no such table: fluent_test_basicpage_product_category

After fishing around it appears that during the Django life cycle it does not have that name except when in the migration process the CreateModel.database_forwards function gets the app state at which point the URLNodeMetaClass.__new__ table changes have been applied to it which can be seen by putting a debug set trace at after the model retrieval in CreateModel.database_forwards and printing model._meta.get_field('product_category').rel.through._meta.db_table. If you then run in shell mode (./manage.py shell) and import your model and do the same you will notice a different table name.

The output for each of these are:
Migration --> u'pagetype_fluent_test_basicpage_product_category'
Shell --> u'fluent_test_basicpage_product_category'

I am happy to help look into this issue but if anyone has an ideas or knowledge that would be awesome!

As a work around I guess you could manually create the through table manually and set the db table on it? I have not tested this though.

@sjdines
Copy link
Contributor Author

sjdines commented Sep 1, 2015

For reference I worked around this by setting the db_table attribute on the ManyToManyField e.g:

class BasicPage(Page):
    product_category = models.ManyToManyField(
        'sites.Site',
        db_table='pagetype_fluent_test_basicpage_product_category',
    )

    class Meta:
        verbose_name = 'Basic page'
        verbose_name_plural = 'Basic pages'

@mrmachine
Copy link
Contributor

This just bit us again.

I wonder if it's worth the metaclass magic to prefix database table names, as it seems that has also caused trouble indirectly in #103 by letting Django think db_table had been explicitly set, when to the end user it was not set by them, but only set magically by fluent.

I think most people don't know or care what their DB table names are, and if they do, they would explicitly define them on their models.

If fluent were to remove this behaviour, I think Django should be able to detect the removal of db_table in a migration and automatically rename tables in user's existing projects -- they'd just need to run makemigrations?

The work-around is easy enough, once you know what the cause us. The problem is, every time a new developer hits this problem, they may spend a lot of time trying to debug it before they arrive at this GitHub issue and find the work-around.

@vdboor
Copy link
Contributor

vdboor commented May 17, 2016

Yes, I'm in favor of such change, given that Django can handle this. It would make the next release Django 1.7+ only, which is fine with me.

I wonder, do we still need a "1.0" release that also supports Django 1.6 (for transition) and then release 1.1 that is Django 1.7+ only - or are you in favor of just an 1.0 release that is Django 1.7+ only?

@cogat
Copy link

cogat commented Nov 25, 2016

(Just bit us one more time)

@cogat
Copy link

cogat commented Nov 25, 2016

NB To work around this, give your model a db_table that doesn't start with your app_label. This will prevent Fluent from prepending pagetypes_.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants