[Django] #23647: Auto-generated PK field name is not easily configurable
Django
----------------------------------------------+--------------------
Reporter: systemsoverload | Owner: nobody
Type: New feature | Status: new
Component: Database layer (models, ORM) | Version: 1.7
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
----------------------------------------------+--------------------
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:
{{{#!python
# 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/23647>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: systemsoverload | Owner: nobody
Type: New feature | Status: new
(models, ORM) | Resolution:
Severity: Normal | Triage Stage:
Keywords: | Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by collinanderson):
* needs_better_patch: => 0
* needs_tests: => 0
* needs_docs: => 0
Comment:
Are you saying that it's too much work to do this?
{{{
class MyModel(objects):
mymodel_id = models.AutoField(primary=True)
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/23647#comment:1>
Django
Reporter: systemsoverload | Owner: nobody
Type: New feature | Status: new
(models, ORM) | Resolution:
Severity: Normal | Triage Stage:
Keywords: | Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by systemsoverload):
Replying to [comment:1 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 though. It also doesn't strike me
as particularly 'pythonic' to have implicit behavior that isn't modifiable
in any way.
--
Ticket URL: <https://code.djangoproject.com/ticket/23647#comment:2>
Django
Reporter: systemsoverload | Owner: nobody
Type: New feature | Status: new
(models, ORM) | Resolution:
Severity: Normal | Triage Stage:
Keywords: | Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by aaugustin):
I don't think it's a good trade off to create a setting for this purpose.
IMO code > settings in that case.
--
Ticket URL: <https://code.djangoproject.com/ticket/23647#comment:3>
Django
Reporter: systemsoverload | Owner: nobody
Type: New feature | Status: new
(models, ORM) | Resolution:
Severity: Normal | Triage Stage:
Keywords: | Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by 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.
{{{#!python
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
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/23647#comment:4>
Django
Reporter: systemsoverload | Owner: nobody
Type: New feature | Status: new
(models, ORM) | Resolution:
Severity: Normal | Triage Stage:
Keywords: | Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by freakboy3742):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/23647#comment:5>
Django
Reporter: systemsoverload | Owner: nobody
Type: New feature | Status: closed
Component: Database layer | Version: 1.7
Severity: Normal | Triage Stage:
Keywords: | Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by systemsoverload):
* status: new => closed
* resolution: => wontfix
Comment:
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 [comment:4 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.
>
> {{{#!python
> 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
> }}}
--
Ticket URL: <https://code.djangoproject.com/ticket/23647#comment:6>