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

Error when running migration script: more than one primary key #115

Closed
lofidevops opened this issue Mar 10, 2017 · 14 comments
Closed

Error when running migration script: more than one primary key #115

lofidevops opened this issue Mar 10, 2017 · 14 comments
Labels
Milestone

Comments

@lofidevops
Copy link

lofidevops commented Mar 10, 2017

Steps to reproduce:

  • Add the example LdapGroup class to existing Django site
  • Run makemigrations
  • Run migrate
  • Run runserver
  • Log in as admin, visit LdapGroup entity

What should happen:

  • Migration script for LdapGroup class created
  • Migration script completes successfully
  • Login successful
  • LdapGroup contains groups on LDAP server

What happens instead:

  • Migration script for LdapGroup class created (as expected)
  • Error when running migrate (see below)
  • Cannot proceed to runserver or visit admin app
  Applying ldapregister.0003_ldapgroup...Traceback (most recent call last):
  File "/home/david/.local/share/virtualenvs/purist_web/lib/python3.5/site-packages/django/db/backends/utils.py", line 62, in execute
    return self.cursor.execute(sql)
  File "/home/david/.local/share/virtualenvs/purist_web/lib/python3.5/site-packages/django/db/backends/sqlite3/base.py", line 335, in execute
    return Database.Cursor.execute(self, query)
sqlite3.OperationalError: table "ldapregister_ldapgroup" has more than one primary key

Configuration:

settings.py (points to Online LDAP Test Server)

DATABASES = {
    'ldap': {
        'ENGINE': 'ldapdb.backends.ldap',
        'NAME': 'ldap://ldap.forumsys.com',
        'USER': 'cn=read-only-admin,dc=example,dc=com',
        'PASSWORD': 'password',
        'TLS': False,
    },
    'default': {
        'ENGINE': 'django.db.backends.sqlite3',
        'NAME': os.path.join(BASE_DIR, 'db.sqlite3'),
    },
}

DATABASE_ROUTERS = ['ldapdb.router.Router']

models.py

class LdapGroup(ldapdb.models.Model):
    """
    Class for representing an LDAP group entry.
    """

    # LDAP meta-data
    base_dn = "dc=example,dc=com"
    object_classes = ['groupOfUniqueNames']

    # posixGroup attributes
    gid = IntegerField(db_column='gidNumber', unique=True)
    name = CharField(db_column='cn', max_length=200, primary_key=True)
    members = ListField(db_column='memberUid')

    def __str__(self):
        return self.name

    def __unicode__(self):
        return self.name

admin.py

class LdapGroupAdmin(admin.ModelAdmin):
    exclude = ['dn', 'objectClass']
    list_display = ['gid', 'name']

admin.site.register(LdapGroup, LdapGroupAdmin)

0003_ldapgroup.py (autogenerated)

# -*- coding: utf-8 -*-
# Generated by Django 1.10.6 on 2017-03-10 14:24
from __future__ import unicode_literals

from django.db import migrations, models
import ldapdb.models.fields


class Migration(migrations.Migration):

    dependencies = [
        ('ldapregister', '0002_auto_20170310_1340'),
    ]

    operations = [
        migrations.CreateModel(
            name='LdapGroup',
            fields=[
                ('dn', models.CharField(max_length=200, primary_key=True, serialize=False)),
                ('gid', ldapdb.models.fields.IntegerField(db_column='gidNumber', unique=True)),
                ('name', ldapdb.models.fields.CharField(db_column='cn', max_length=200, primary_key=True, serialize=False)),
                ('members', ldapdb.models.fields.ListField(db_column='memberUid')),
            ],
            options={
                'abstract': False,
            },
        ),
    ]
@lofidevops
Copy link
Author

lofidevops commented Mar 10, 2017

The following workaround results in a working admin app:

  1. Run makemigrations to generate the migration script
  2. Edit the migration script: remove the primary key flag from 'name', keep the primary key flag on 'dn'
  3. Run migrate
  4. Keep the primary key on 'name' in the model
  5. Run runserver

Is there supposed to be a migration script? (Django complains if I don't create one.)

@lofidevops
Copy link
Author

lofidevops commented Mar 15, 2017

This leads me to ask: should my ldapdb models be triggering the creation of migration scripts, or am I doing something wrong?

@lofidevops lofidevops changed the title django.db.utils.OperationalError: more than one primary key Error when running migrate: more than one primary key Mar 15, 2017
@lofidevops lofidevops changed the title Error when running migrate: more than one primary key Error when running migration script: more than one primary key Mar 15, 2017
@rbarrois
Copy link
Contributor

@kwill The answer is "don't create migrations"; Django should not complain about them.

The django-ldapdb-provided router will properly specify that no migrations should be created; however, it seems that Django decides to create them with makemigrations even if the router states that no migration should be created.

The fix would be to never call makemigrations your_ldap_app.

@rbarrois rbarrois added the docs label Mar 20, 2017
@lofidevops
Copy link
Author

Thanks for the info. I have other models in your_ldap_app so it's possible I'll need to make and run migrations in the future. Could there be a bug in makemigrations itself?

@rbarrois
Copy link
Contributor

Oww.

That's an option I'll have to consider — maybe I could be able to bypass the migration generator and either not generate any migration code, or silently skip the migration methods.

I'll add this to the todo-list for version 0.9.0.

@rbarrois rbarrois added this to the 0.9.0 milestone Mar 20, 2017
@moonwolf-github
Copy link

moonwolf-github commented Aug 31, 2017

Any progress with this? Although workaround works i think it's not a best idea to manually edit migration files (;)

With one exception: next manage.py makemigrations will add edited field again.

@MarcPuckett
Copy link

Is this a sqlite3 specific problem?

@ghost
Copy link

ghost commented Feb 1, 2018

@MarcPuckett its been a while, if I recall correctly, I ran into this issue with a mysql database backend.

@jasongabler
Copy link

I'm not sure if I'm addressing the same issue, but I think I ran into this before I realized that I needed to run the migrations. When I'd just runserver and execute a view, I would get errors about multiple PKs. So I went through the django-ldapdb code and saw that the base model has dn (distinguishedName) already designated as a PK, from ldapdb/models/base.py

image

At first I grimaced and figured I could hack my way around this, as I wanted to make my organizations uid field the PK. But then, that's really just a unique field for my org. After all, Isn't distinguishedName the PK for LDAP by definition? I concluded that in the LDAP universe this is correct and I never have to worry about the ORM having a PK because this package silently takes care of it for me. Am I missing something here?

@DreamerDDL
Copy link

How about adding Options.managed = false to ldapdb.Model class ?

class Meta:
    abstract = True
    managed = False

It solve problem with more than one primary key for me.
Django Model Meta options

@Ecno92
Copy link

Ecno92 commented Jul 10, 2019

@DreamerDDL I've tried this and I kept hitting issues.

The approach that I'm using now is by overriding the router with my own implementation which has a different version of is_ldap_model.

The current version checks for an attribute called base_dn. This is however not present when executing migrations as Django introduces fake migration objects in some cases during migration.

I also could not find any identifier on the fake model object which clearly identifies the fact that it is an ldap model. That's why I've implemented is_ldap_model like this:

from ldapdb.models import Model as LdapModel
from django.apps import apps

def is_ldap_model(model):
    real_model = apps.get_model(
        model._meta.app_label,
        model._meta.model_name
    )

    return issubclass(real_model, LdapModel)

This implementation depends on the current models present in the codebase. This may have implications. However I don't see a better solution right now.
Once I know for sure that I've found a correct approach I will open a PR.

Also important to know is that the if the router says that Django should not allow migrations it will still generate migrations. It only will skip them in execution!

@Chilipp
Copy link

Chilipp commented Dec 14, 2020

@rbarrois, I am new to django-ldapdb but find it quite nice. However I am also getting the issue mentioned here and the fix mentioned by @Ecno92 in #115 (comment) works well! Is there a reason why this has not been implemented yet within the last year? Or is it only me who is still having this issue? I'm happy to create the necessary PR if you're okay with it.

@Chilipp
Copy link

Chilipp commented Dec 14, 2020

actually I just saw that @dzen already made a PR #201. Any chance to get this merged?

@Natureshadow
Copy link
Member

Fixed in #201

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

9 participants