LdapPerson defines 2 PKs, migrations fail

[mk2301]

accounts.models.LdapPerson defines 2 PKs, one in the implementation and one in its base class.

tapir/accounts/models.py

class LdapPerson(ldapdb.models.Model):

    # [...]

    uid = ldapdb_fields.CharField(db_column="uid", max_length=200, primary_key=True)

    # [...]

LdapPerson inherits from ldabdb.models.Model:

class Model(django.db.models.base.Model):
 
    dn = ldapdb_fields.CharField(max_length=200, primary_key=True)

If you change the uid field to not be a primary key, the following function of ldabdb.models.Model fails generating the dn (distingushed name), as it looks for fields which are defined with primary_key=True.

 def build_rdn(self):
        """
        Build the Relative Distinguished Name for this entry.
        """
        bits = []
        for field in self._meta.fields:
            if field.db_column and field.primary_key:
                bits.append("%s=%s" % (field.db_column,
                                       getattr(self, field.name)))
        if not len(bits):
            raise Exception("Could not build Distinguished Name")
        return '+'.join(bits)

If you leave it and execute makemigrations, it generates a migration for changing the uid field. Executing migrate fails then, because it is not possible to define multiple PK fields.

So the logic of the LDAP module requires to define a second PK, but database-wise this is not possible. I don't really know what the solution is here. Maybe we need to override the build_rdn() function to check for a property that doesn't break the DB instead of primary_key

Workaround:
Delete the new migrations file in accounts/migrations manually after makemigrations.

[mk2301]

@Theophile-Madet :

Hey, yeah it's quite annoying.
It is a know bug of ypthon-ldapdb : django-ldapdb/django-ldapdb#115 that actually has been fixed in 2021 : django-ldapdb/django-ldapdb#201
But there has been no release of python-ldapdb since then.

[mk2301]

Maybe we can just release our own version based on the current version including the fix. Probably this is the easiest solution without breaking current functionality? Otherwise we would have to rework a lot when switching to another module I assume

Or we just extend and override the build_rdn() function in tapir.
But I'm not sure what the implications are when we do that

[mk2301]

probably obsolete after migration to keycloak