[Django] #33413: Errors with db_collation – no propagation to foreignkeys
Django
-------------------------------------+-------------------------------------
Reporter: typonaut | Owner: nobody
Type: Bug | Status: new
Component: Database | Version: 3.2
layer (models, ORM) |
Severity: Normal | Keywords:
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
Using `db_collation` with a `pk` that also has referenced `fk`s in other
models causes `foreign key constraint` errors in MySQL.
With the following models:
{{{
class Account(models.Model):
id = ShortUUIDField(primary_key=True, db_collation='utf8_bin',
db_index=True, max_length=22)
…
class Address(models.Model):
id = ShortUUIDField(primary_key=True, db_collation='utf8_bin',
db_index=True, max_length=22)
account = models.OneToOneField(Account, on_delete=models.CASCADE)
…
class Profile(models.Model):
id = ShortUUIDField(primary_key=True, db_collation='utf8_bin',
db_index=True, max_length=22)
…
account = models.ForeignKey('Account', verbose_name=_('account'),
null=True, blank=True, on_delete=models.CASCADE)
…
etc
}}}
Where `Account.id` has been changed from `models.BigAutoField` if
`makemigrations` is run then it produces `sqlmigrate` output like this:
{{{
ALTER TABLE `b_manage_account` MODIFY `id` varchar(22) COLLATE `utf8_bin`;
ALTER TABLE `b_manage_address` MODIFY `account_id` varchar(22) NOT NULL;
ALTER TABLE `b_manage_profile` MODIFY `account_id` varchar(22) NULL;
ALTER TABLE `b_manage_address` ADD CONSTRAINT
`b_manage_address_account_id_7de0ae37_fk` FOREIGN KEY (`account_id`)
REFERENCES `b_manage_account` (`id`);
ALTER TABLE `b_manage_profile` ADD CONSTRAINT
`b_manage_profile_account_id_ec864dcc_fk` FOREIGN KEY (`account_id`)
REFERENCES `b_manage_account` (`id`);
}}}
With this SQL the `ADD CONSTRAINT` queries fail. This is because the
`COLLATE` should also be present in the `b_manage_address.account_id` and
`b_manage_profile.account_id` modification statements. Like this:
{{{
ALTER TABLE `b_manage_account` MODIFY `id` varchar(22) COLLATE `utf8_bin`;
ALTER TABLE `b_manage_address` MODIFY `account_id` varchar(22) NOT NULL
COLLATE `utf8_bin;
ALTER TABLE `b_manage_profile` MODIFY `account_id` varchar(22) NULL
COLLATE `utf8_bin;
ALTER TABLE `b_manage_address` ADD CONSTRAINT
`b_manage_address_account_id_7de0ae37_fk` FOREIGN KEY (`account_id`)
REFERENCES `b_manage_account` (`id`);
ALTER TABLE `b_manage_profile` ADD CONSTRAINT
`b_manage_profile_account_id_ec864dcc_fk` FOREIGN KEY (`account_id`)
REFERENCES `b_manage_account` (`id`);
}}}
In the latter case the `ADD CONSTRAINT` statements run without error. The
collation of the `pk` must match the collation of the `fk` otherwise an
error will occur.
--
Ticket URL: <https://code.djangoproject.com/ticket/33413>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: typonaut | Owner: nobody
Type: Bug | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Simon Charette):
* component: Database layer (models, ORM) => Migrations
* stage: Unreviewed => Accepted
Comment:
[https://github.com/django/django/blob/806efe912b846c1fde250c9321d8334b7517cd56/django/db/backends/base/schema.py#L217-L219
It seems like] this should be addressable by defining a
`ForeignKey.db_collation` property that proxies
`self.target_field.db_column`
{{{#!diff
diff --git a/django/db/models/fields/related.py
b/django/db/models/fields/related.py
index 11407ac902..f82f787f5c 100644
--- a/django/db/models/fields/related.py
+++ b/django/db/models/fields/related.py
@@ -1043,6 +1043,10 @@ def db_type(self, connection):
def db_parameters(self, connection):
return {"type": self.db_type(connection), "check":
self.db_check(connection)}
+ @property
+ def db_collation(self):
+ return getattr(self.target_field, 'db_collation', None)
+
def convert_empty_strings(self, value, expression, connection):
if (not value) and isinstance(value, str):
return None
}}}
I do wonder if it would be better to have `'collation': self.db_collation`
returned in `CharField` and `TextField.db_params` instead and adapt
`ForeignKey.db_params` to simply proxy `self.target_feld.db_params` and
adapt the schema editor to branch of `params['collation']` instead as
`db_collation` is pretty much a ''text fields'' option that ''related
fields'' should know nothing about.
--
Ticket URL: <https://code.djangoproject.com/ticket/33413#comment:1>
Django
Reporter: typonaut | Owner: nobody
Type: Bug | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Old description:
New description:
With the following models:
etc
}}}
--
Comment (by typonaut):
fixed a couple of typos.
--
Ticket URL: <https://code.djangoproject.com/ticket/33413#comment:2>
Django
Reporter: typonaut | Owner: nobody
Type: Bug | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* cc: Tom Carrick (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/33413#comment:3>
Django
Reporter: typonaut | Owner: David Wobrock
Type: Bug | Status: assigned
Component: Migrations | Version: 3.2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by David Wobrock):
* cc: David Wobrock (added)
* needs_better_patch: 0 => 1
* has_patch: 0 => 1
* status: new => assigned
* owner: nobody => David Wobrock
Comment:
Hi there,
I started looking into this and here is a draft PR
https://github.com/django/django/pull/15629.
Plainly adding `db_collation` and making the `ForeignKey` object aware it
through a property works when creating the Models from scratch.
However, this won't make the FK object aware of changes to the related PK
field, meaning that changing/adding/removing a `db_collation`.
From my understanding, we should add it to the `db_parameters` so that
changes to the target db_collation will be reflected on the FK field and
its constraints.
--
Ticket URL: <https://code.djangoproject.com/ticket/33413#comment:4>
Django
Reporter: typonaut | Owner: David Wobrock
Type: Bug | Status: assigned
Component: Migrations | Version: 3.2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by David Wobrock):
* needs_better_patch: 1 => 0
Comment:
I tried an approach to change how we alter fields to take into account the
db_collation for FK fields, in
`db.backends.base.schema.BaseDatabaseSchemaEditor._alter_field`.
Tell if you think that can work :)
--
Ticket URL: <https://code.djangoproject.com/ticket/33413#comment:5>
Django
Reporter: typonaut | Owner: David Wobrock
Type: Bug | Status: assigned
Component: Migrations | Version: 3.2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* needs_better_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/33413#comment:6>
Django
Reporter: typonaut | Owner: David Wobrock
Type: Bug | Status: assigned
Component: Migrations | Version: 3.2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by David Wobrock):
* needs_better_patch: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/33413#comment:7>
Django
Reporter: typonaut | Owner: David Wobrock
Type: Bug | Status: assigned
Component: Migrations | Version: 3.2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"87da2833384b7acd99a778a78e07ea6640f54f35" 87da2833]:
{{{
#!CommitTicketReference repository=""
revision="87da2833384b7acd99a778a78e07ea6640f54f35"
Refs #33413 -- Added collation to CharField/TextField's db_parameters.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/33413#comment:8>
Django
Reporter: typonaut | Owner: David Wobrock
Type: Bug | Status: assigned
Component: Migrations | Version: 3.2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* needs_better_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/33413#comment:9>
Django
Reporter: typonaut | Owner: David Wobrock
Type: Bug | Status: assigned
Component: Migrations | Version: 3.2
Severity: Normal | Resolution:
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* needs_better_patch: 1 => 0
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/33413#comment:10>
Django
Reporter: typonaut | Owner: David Wobrock
Component: Migrations | Version: 3.2
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak <felisiak.mariusz@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"aca9bb2a12b8985d0cb736d380db97439010cd98" aca9bb2a]:
{{{
#!CommitTicketReference repository=""
revision="aca9bb2a12b8985d0cb736d380db97439010cd98"
Fixed #33413 -- Made migrations propage collations to related fields.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/33413#comment:11>