Opened 3 weeks ago
Last modified 13 days ago
#37365 assigned Bug
models.E007 misses duplicate columns when one db_column is explicitly quoted
| Reported by: | Matthew Schinckel | Owned by: | Md. Saikat Islam |
|---|---|---|---|
| Component: | Core (System checks) | Version: | 6.1 |
| Severity: | Normal | Keywords: | db_column quoting system-checks |
| Cc: | Triage Stage: | Accepted | |
| Has patch: | yes | Needs documentation: | no |
| Needs tests: | no | Patch needs improvement: | no |
| Easy pickings: | no | UI/UX: | no |
Description
The models.E007 system check can miss duplicate database columns when one db_column is explicitly quoted and another is not. The strings differ, but the configured backend can render them as the same SQL identifier.
This issue appears to be mostly the same across different backends.
Reproduction
This test uses the configured backend's own identifier quoting. It does not query the database.
from django.db import connection, models from django.test import SimpleTestCase from django.test.utils import isolate_apps class QuotedColumnTests(SimpleTestCase): @isolate_apps() def test_quoted_column_collision(self) -> None: quoted_name = connection.ops.quote_name("id") class Example(models.Model): key = models.IntegerField(primary_key=True, db_column=quoted_name) score = models.IntegerField(db_column="id") class Meta: app_label = "example" self.assertIn("models.E007", {error.id for error in Example.check()})
Actual result: Example.check() returns [], so the assertion fails.
Expected result: models.E007, because both fields map to the same physical column. Using the unquoted string "id" for both fields correctly produces that error.
Backend examples
The equivalent spellings depend on the backend:
- SQLite:
idand"id"both render as"id". Source - MySQL:
idand`id`both render as`id`. The quoted spelling must use backticks; double quotes would represent a different identifier. Source - Oracle:
idand"id"both render as"ID", because Django's Oracle quote_name() uppercases its result. Source - PostgreSQL:
idand"id"both render as"id". Source
Cause
Model._check_column_name_clashes() compares the original field.column strings without accounting for backend identifier handling. It accepts declarations that would generate duplicate column names.
The desired behaviour is to recognise equivalent identifiers and report the collision, rather than permit duplicate columns. Any fix should account for backend-specific rules rather than simply stripping double quotes.
Versions checked
Runtime reproduction: Django 5.2.17, Python 3.11.6, with SQLite and PostgreSQL backend configurations. The checks do not query a database.
6.1.1 appears to have the same issue based on inspection of its released source. The shared check and the four backends' quote_name() implementations are unchanged from 5.2.17. MySQL and Oracle findings are source verification only; no integration tests were run against those databases.
Change History (8)
comment:1 by , 3 weeks ago
| Owner: | set to |
|---|---|
| Status: | new → assigned |
| Triage Stage: | Unreviewed → Accepted |
comment:2 by , 3 weeks ago
On a second thought.
db_column = "id" ---------------------------------------------------> gives you database column name id
db_column = connection.ops.quote_name("id") ---> gives you database column name "id"
So they are not duplicate and should not raise duplicate column name error. But we have duplicate column name error in database level when we try to run migration.
Also django doc specifies:
... Django quotes column and table names behind the scenes.
So they are quoted before being executed.
follow-up: 4 comment:3 by , 3 weeks ago
Yeah, I was going to work through a solution but ran out of day.
comment:4 by , 3 weeks ago
I am already done with the solution. I was going to open a PR. I’d appreciate it if you could share your thoughts on my reasoning.
comment:5 by , 3 weeks ago
| Has patch: | set |
|---|
comment:7 by , 2 weeks ago
A note for you Saikat, about triaging. Often new reporters aren't familiar with the owner field. It's better to ask them if they want to submit a PR and leave the owner field blank until they respond with a No or silence. Otherwise assigning to yourself might appear as poaching.
comment:8 by , 13 days ago
Got it, thanks for pointing that out. I'll make sure to follow this process moving forward.
Thanks for reporting this issue. I was able to reproduce it after inspection.
After digging into the code I found the reason. When we normally assign a string value as db_column, django saves it as it is (
"id"), but when we useconnections.quoate_namedjangos makes it quoated'"id"'. As a result in the_check_column_name_clashes(django/db/models/base.py) method when we compare using... column_name = f.column # Ensure the column name is not already in use. if column_name and column_name in used_column_names: errors.append(...) ....This could be solved using
column_name = connection.ops.quote_name(f.column)so that we can treat"id"and'"id"'as same in duplicacy check.but this is not good when querying data using sql if we assign a db_column name like "multi word column name". This should be quoated.
So another solution i found is to update the field db_column name in init as quated.
in
django/db/models/fields/__init__.pysince
quote_namefollows quoting once rule. So this should be good to go. I will be following second solution since it will update the db_column name.