Opened 9 years ago

Closed 9 years ago

#23647 closed New feature (wontfix)

Auto-generated PK field name is not easily configurable

Reported by: TJ Kells Owned by: nobody
Component: Database layer (models, ORM) Version: 1.7
Severity: Normal Keywords:
Cc: Triage Stage: Unreviewed
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Due to the way that Model inherits from ModelBase which takes a non-customizable Options class, there is no way to inject your own naming preferences into the auto generated PK field name. The most common use-case would be for auto-naming your PK fields something more standard to RBDMS like '{modelname}_id'.

I would propose either some refactoring of the Model/ModelBase to allow some hooks for customization (probably fairly difficult) OR probably more realistically how about a logging-style formatting string exposed through the settings. Something like:

# Default
AUTO_PK_NAME = 'id'

# Override
# This string just gets run through a logging-like formatter that exposes (some?) of the 
# available properties of the Option instance ie - self.model_name, self.module_name etc.
AUTO_PK_NAME = '%(module_name)s_%(model_name)s_id'

# Possibly a more modern interface setup for .format
AUTO_PK_NAME = '{module_name}_{model_name}_id'

The only area that I can see this being problematic for is that >=1.7 the built-in migrations have the column 'id' effectively hard-coded into them. The _prepare method on the options would potentially have to determine if the model is in 'user space' or not. I'm open to any solutions for working around that issue though.

Change History (6)

comment:1 by Collin Anderson, 9 years ago

Are you saying that it's too much work to do this?

class MyModel(objects):
    mymodel_id = models.AutoField(primary=True)

in reply to:  1 comment:2 by TJ Kells, 9 years ago

Replying to collinanderson:

Are you saying that it's too much work to do this?

class MyModel(objects):
    mymodel_id = models.AutoField(primary=True)

No that is currently the way we do it today. We even use AppConfig's to assert that no models getting created are missing the AutoField definition, but that seems like an awful lot of trouble for something that could probably get wrapped up into a setting with very few side-effects. Also, pre-1.7 we really struggled with newer devs who didnt understand the implication of NOT specifying that field. It also doesn't strike me as particularly 'pythonic' to have implicit behavior that isn't modifiable in any way.

Last edited 9 years ago by TJ Kells (previous) (diff)

comment:3 by Aymeric Augustin, 9 years ago

I don't think it's a good trade off to create a setting for this purpose.

IMO code > settings in that case.

comment:4 by Simon Charette, 9 years ago

I also agree that this doesn't warrant an additional setting.

You can already achieve what you're after with a ModelBase subclass with no refactoring.

from django.db import models

class AutoPKModelBase(models.ModelBase):
    def __new__(cls, name, bases, attrs):
        pk_name = "%s_id" % name.lower()
        attrs[pk_name] = models.AutoField(primary=True)
        return super(AutoPKModelBase, cls).__new__(cls, name, bases, attrs)

class MyModel(metaclass=AutoPKModelBase, models.Model):
    pass

comment:5 by Russell Keith-Magee, 9 years ago

Not only do I believe this is a bad tradeoff - I believe it has the potential to be fundamentally destructive.

Settings are project global. By having a setting, you're enforcing a behavior for *all* apps in your project - including apps that you have no involvement in developing or maintaining. That means you can affect the behaviour of third-party projects by changing the value of a setting. The potential for hard-to-track-down unintentional side effects is enormous.

Plus, what happens if you change the value of the setting *after* initially running syncdb? You've just changed the PK name for every model in a way that isn't tied to the definition of the model itself.

We're already seeing problems of this nature through AUTH_USER_MODEL - which is a system wide setting for the User model. The difference there is that the concept of a User is something that a project as a whole needs to have a policy on - primary key usage is, at best, something that a specific app needs to have a policy on (and no - that wasn't me making an argument for an application setting).

To me, this is clear example where explicit is better than implicit. If this got to a technical board vote, I'd be a strong -1.

in reply to:  4 comment:6 by TJ Kells, 9 years ago

Resolution: wontfix
Status: newclosed

I think that this is enough of a reason to close this ticket as erroneous. I'm not as up to snuff on meta magicks as I should be apparently, thanks charettes!

Replying to charettes:

I also agree that this doesn't warrant an additional setting.

You can already achieve what you're after with a ModelBase subclass with no refactoring.

from django.db import models

class AutoPKModelBase(models.ModelBase):
    def __new__(cls, name, bases, attrs):
        pk_name = "%s_id" % name.lower()
        attrs[pk_name] = models.AutoField(primary=True)
        return super(AutoPKModelBase, cls).__new__(cls, name, bases, attrs)

class MyModel(metaclass=AutoPKModelBase, models.Model):
    pass
Note: See TracTickets for help on using tickets.
Back to Top