Skip to content

Commit

Permalink
Fixed #29949 -- Refactored db introspection identifier converters.
Browse files Browse the repository at this point in the history
Removed DatabaseIntrospection.table_name_converter()/column_name_converter()
and use instead DatabaseIntrospection.identifier_converter().

Removed DatabaseFeatures.uppercases_column_names.

Thanks Tim Graham for the initial patch and review and Simon Charette
for the review.
  • Loading branch information
felixxm committed Nov 21, 2018
1 parent 2e47761 commit d5f4ce9
Show file tree
Hide file tree
Showing 15 changed files with 64 additions and 76 deletions.
2 changes: 1 addition & 1 deletion django/core/management/commands/migrate.py
Expand Up @@ -308,7 +308,7 @@ def sync_apps(self, connection, app_labels):

def model_installed(model):
opts = model._meta
converter = connection.introspection.table_name_converter
converter = connection.introspection.identifier_converter
return not (
(converter(opts.db_table) in tables) or
(opts.auto_created and converter(opts.auto_created._meta.db_table) in tables)
Expand Down
2 changes: 0 additions & 2 deletions django/db/backends/base/features.py
Expand Up @@ -200,8 +200,6 @@ class BaseDatabaseFeatures:
# If NULL is implied on columns without needing to be explicitly specified
implied_column_null = False

uppercases_column_names = False

# Does the backend support "select for update" queries with limit (and offset)?
supports_select_for_update_with_limit = True

Expand Down
22 changes: 7 additions & 15 deletions django/db/backends/base/introspection.py
Expand Up @@ -24,22 +24,14 @@ def get_field_type(self, data_type, description):
"""
return self.data_types_reverse[data_type]

def table_name_converter(self, name):
def identifier_converter(self, name):
"""
Apply a conversion to the name for the purposes of comparison.
Apply a conversion to the identifier for the purposes of comparison.
The default table name converter is for case sensitive comparison.
The default identifier converter is for case sensitive comparison.
"""
return name

def column_name_converter(self, name):
"""
Apply a conversion to the column name for the purposes of comparison.
Use table_name_converter() by default.
"""
return self.table_name_converter(name)

def table_names(self, cursor=None, include_views=False):
"""
Return a list of names of all tables that exist in the database.
Expand Down Expand Up @@ -83,11 +75,11 @@ def django_table_names(self, only_existing=False, include_views=True):
)
tables = list(tables)
if only_existing:
existing_tables = self.table_names(include_views=include_views)
existing_tables = set(self.table_names(include_views=include_views))
tables = [
t
for t in tables
if self.table_name_converter(t) in existing_tables
if self.identifier_converter(t) in existing_tables
]
return tables

Expand All @@ -101,10 +93,10 @@ def installed_models(self, tables):
all_models = []
for app_config in apps.get_app_configs():
all_models.extend(router.get_migratable_models(app_config, self.connection.alias))
tables = list(map(self.table_name_converter, tables))
tables = set(map(self.identifier_converter, tables))
return {
m for m in all_models
if self.table_name_converter(m._meta.db_table) in tables
if self.identifier_converter(m._meta.db_table) in tables
}

def sequence_list(self):
Expand Down
2 changes: 1 addition & 1 deletion django/db/backends/base/schema.py
Expand Up @@ -1048,7 +1048,7 @@ def _constraint_names(self, model, column_names=None, unique=None,
"""Return all constraint names matching the columns and conditions."""
if column_names is not None:
column_names = [
self.connection.introspection.column_name_converter(name)
self.connection.introspection.identifier_converter(name)
for name in column_names
]
with self.connection.cursor() as cursor:
Expand Down
1 change: 0 additions & 1 deletion django/db/backends/oracle/features.py
Expand Up @@ -28,7 +28,6 @@ class DatabaseFeatures(BaseDatabaseFeatures):
requires_literal_defaults = True
closed_cursor_error_class = InterfaceError
bare_select_suffix = " FROM DUAL"
uppercases_column_names = True
# select for update with limit can be achieved on Oracle, but not with the current backend.
supports_select_for_update_with_limit = False
supports_temporal_subtraction = True
Expand Down
33 changes: 22 additions & 11 deletions django/db/backends/oracle/introspection.py
Expand Up @@ -50,7 +50,7 @@ def get_table_list(self, cursor):
"""Return a list of table and view names in the current database."""
cursor.execute("SELECT TABLE_NAME, 't' FROM USER_TABLES UNION ALL "
"SELECT VIEW_NAME, 'v' FROM USER_VIEWS")
return [TableInfo(row[0].lower(), row[1]) for row in cursor.fetchall()]
return [TableInfo(self.identifier_converter(row[0]), row[1]) for row in cursor.fetchall()]

def get_table_description(self, cursor, table_name):
"""
Expand Down Expand Up @@ -86,13 +86,13 @@ def get_table_description(self, cursor, table_name):
internal_size, default, is_autofield = field_map[name]
name = name % {} # cx_Oracle, for some reason, doubles percent signs.
description.append(FieldInfo(
name.lower(), *desc[1:3], internal_size, desc[4] or 0,
self.identifier_converter(name), *desc[1:3], internal_size, desc[4] or 0,
desc[5] or 0, *desc[6:], default, is_autofield,
))
return description

def table_name_converter(self, name):
"""Table name comparison is case insensitive under Oracle."""
def identifier_converter(self, name):
"""Identifier comparison is case insensitive under Oracle."""
return name.lower()

def get_sequences(self, cursor, table_name, table_fields=()):
Expand All @@ -114,7 +114,11 @@ def get_sequences(self, cursor, table_name, table_fields=()):
# Oracle allows only one identity column per table.
row = cursor.fetchone()
if row:
return [{'name': row[0].lower(), 'table': table_name, 'column': row[1].lower()}]
return [{
'name': self.identifier_converter(row[0]),
'table': self.identifier_converter(table_name),
'column': self.identifier_converter(row[1]),
}]
# To keep backward compatibility for AutoFields that aren't Oracle
# identity columns.
for f in table_fields:
Expand All @@ -136,10 +140,12 @@ def get_relations(self, cursor, table_name):
user_constraints.r_constraint_name = cb.constraint_name AND
ca.position = cb.position""", [table_name])

relations = {}
for row in cursor.fetchall():
relations[row[0].lower()] = (row[2].lower(), row[1].lower())
return relations
return {
self.identifier_converter(field_name): (
self.identifier_converter(rel_field_name),
self.identifier_converter(rel_table_name),
) for field_name, rel_table_name, rel_field_name in cursor.fetchall()
}

def get_key_columns(self, cursor, table_name):
cursor.execute("""
Expand All @@ -150,8 +156,10 @@ def get_key_columns(self, cursor, table_name):
JOIN user_cons_columns rcol
ON rcol.constraint_name = c.r_constraint_name
WHERE c.table_name = %s AND c.constraint_type = 'R'""", [table_name.upper()])
return [tuple(cell.lower() for cell in row)
for row in cursor.fetchall()]
return [
tuple(self.identifier_converter(cell) for cell in row)
for row in cursor.fetchall()
]

def get_constraints(self, cursor, table_name):
"""
Expand Down Expand Up @@ -186,6 +194,7 @@ def get_constraints(self, cursor, table_name):
GROUP BY user_constraints.constraint_name, user_constraints.constraint_type
""", [table_name])
for constraint, columns, pk, unique, check in cursor.fetchall():
constraint = self.identifier_converter(constraint)
constraints[constraint] = {
'columns': columns.split(','),
'primary_key': pk,
Expand Down Expand Up @@ -213,6 +222,7 @@ def get_constraints(self, cursor, table_name):
GROUP BY cons.constraint_name, rcols.table_name, rcols.column_name
""", [table_name])
for constraint, columns, other_table, other_column in cursor.fetchall():
constraint = self.identifier_converter(constraint)
constraints[constraint] = {
'primary_key': False,
'unique': False,
Expand Down Expand Up @@ -240,6 +250,7 @@ def get_constraints(self, cursor, table_name):
GROUP BY ind.index_name, ind.index_type
""", [table_name])
for constraint, type_, columns, orders in cursor.fetchall():
constraint = self.identifier_converter(constraint)
constraints[constraint] = {
'primary_key': False,
'unique': False,
Expand Down
4 changes: 2 additions & 2 deletions django/db/models/query.py
Expand Up @@ -1347,7 +1347,7 @@ def __init__(self, raw_query, model=None, query=None, params=None,

def resolve_model_init_order(self):
"""Resolve the init field names and value positions."""
converter = connections[self.db].introspection.column_name_converter
converter = connections[self.db].introspection.identifier_converter
model_init_fields = [f for f in self.model._meta.fields if converter(f.column) in self.columns]
annotation_fields = [(column, pos) for pos, column in enumerate(self.columns)
if column not in self.model_fields]
Expand Down Expand Up @@ -1469,7 +1469,7 @@ def columns(self):
@cached_property
def model_fields(self):
"""A dict mapping column names to model field names."""
converter = connections[self.db].introspection.table_name_converter
converter = connections[self.db].introspection.identifier_converter
model_fields = {}
for field in self.model._meta.fields:
name, column = field.get_attname_column()
Expand Down
2 changes: 1 addition & 1 deletion django/db/models/sql/query.py
Expand Up @@ -92,7 +92,7 @@ def clone(self, using):
def get_columns(self):
if self.cursor is None:
self._execute_query()
converter = connections[self.using].introspection.column_name_converter
converter = connections[self.using].introspection.identifier_converter
return [converter(column_meta[0])
for column_meta in self.cursor.description]

Expand Down
7 changes: 7 additions & 0 deletions docs/releases/2.2.txt
Expand Up @@ -312,6 +312,13 @@ backends.
* ``sql_create_fk`` is replaced with ``sql_foreign_key_constraint``,
``sql_constraint``, and ``sql_create_constraint``.

* ``DatabaseIntrospection.table_name_converter()`` and
``column_name_converter()`` are removed. Third party database backends may
need to instead implement ``DatabaseIntrospection.identifier_converter()``.
In that case, the constraint names that
``DatabaseIntrospection.get_constraints()`` returns must be normalized by
``identifier_converter()``.

Admin actions are no longer collected from base ``ModelAdmin`` classes
----------------------------------------------------------------------

Expand Down
6 changes: 3 additions & 3 deletions tests/backends/tests.py
Expand Up @@ -80,7 +80,7 @@ def test_bad_parameter_count(self):
"An executemany call with too many/not enough parameters will raise an exception (Refs #12612)"
with connection.cursor() as cursor:
query = ('INSERT INTO %s (%s, %s) VALUES (%%s, %%s)' % (
connection.introspection.table_name_converter('backends_square'),
connection.introspection.identifier_converter('backends_square'),
connection.ops.quote_name('root'),
connection.ops.quote_name('square')
))
Expand Down Expand Up @@ -217,7 +217,7 @@ def create_squares_with_executemany(self, args):

def create_squares(self, args, paramstyle, multiple):
opts = Square._meta
tbl = connection.introspection.table_name_converter(opts.db_table)
tbl = connection.introspection.identifier_converter(opts.db_table)
f1 = connection.ops.quote_name(opts.get_field('root').column)
f2 = connection.ops.quote_name(opts.get_field('square').column)
if paramstyle == 'format':
Expand Down Expand Up @@ -303,7 +303,7 @@ def test_unicode_fetches(self):
'SELECT %s, %s FROM %s ORDER BY %s' % (
qn(f3.column),
qn(f4.column),
connection.introspection.table_name_converter(opts2.db_table),
connection.introspection.identifier_converter(opts2.db_table),
qn(f3.column),
)
)
Expand Down
4 changes: 0 additions & 4 deletions tests/constraints/tests.py
Expand Up @@ -48,8 +48,6 @@ def test_database_constraint(self):
def test_name(self):
constraints = get_constraints(Product._meta.db_table)
expected_name = 'price_gt_discounted_price'
if connection.features.uppercases_column_names:
expected_name = expected_name.upper()
self.assertIn(expected_name, constraints)


Expand Down Expand Up @@ -87,6 +85,4 @@ def test_model_validation(self):
def test_name(self):
constraints = get_constraints(Product._meta.db_table)
expected_name = 'unique_name'
if connection.features.uppercases_column_names:
expected_name = expected_name.upper()
self.assertIn(expected_name, constraints)
5 changes: 1 addition & 4 deletions tests/gis_tests/gis_migrations/test_operations.py
Expand Up @@ -63,12 +63,9 @@ def set_up_test_model(self, force_raster_creation=False):
self.current_state = self.apply_operations('gis', ProjectState(), operations)

def assertGeometryColumnsCount(self, expected_count):
table_name = 'gis_neighborhood'
if connection.features.uppercases_column_names:
table_name = table_name.upper()
self.assertEqual(
GeometryColumns.objects.filter(**{
GeometryColumns.table_name_col(): table_name,
'%s__iexact' % GeometryColumns.table_name_col(): 'gis_neighborhood',
}).count(),
expected_count
)
Expand Down
2 changes: 1 addition & 1 deletion tests/inspectdb/tests.py
Expand Up @@ -184,7 +184,7 @@ def test_special_column_name_introspection(self):
out = StringIO()
call_command('inspectdb', table_name_filter=special_table_only, stdout=out)
output = out.getvalue()
base_name = 'field' if connection.features.uppercases_column_names else 'Field'
base_name = connection.introspection.identifier_converter('Field')
self.assertIn("field = models.IntegerField()", output)
self.assertIn("field_field = models.IntegerField(db_column='%s_')" % base_name, output)
self.assertIn("field_field_0 = models.IntegerField(db_column='%s__')" % base_name, output)
Expand Down
41 changes: 15 additions & 26 deletions tests/schema/tests.py
Expand Up @@ -85,7 +85,7 @@ def tearDown(self):

def delete_tables(self):
"Deletes all model tables for our models for a clean test environment"
converter = connection.introspection.table_name_converter
converter = connection.introspection.identifier_converter
with connection.schema_editor() as editor:
connection.disable_constraint_checking()
table_names = connection.introspection.table_names()
Expand Down Expand Up @@ -1868,9 +1868,6 @@ def test_remove_db_index_doesnt_remove_custom_indexes(self):
table_name=AuthorWithIndexedName._meta.db_table,
column_names=('name',),
)
if connection.features.uppercases_column_names:
author_index_name = author_index_name.upper()
db_index_name = db_index_name.upper()
try:
AuthorWithIndexedName._meta.indexes = [index]
with connection.schema_editor() as editor:
Expand Down Expand Up @@ -1908,8 +1905,6 @@ def test_order_index(self):
with connection.schema_editor() as editor:
editor.add_index(Author, index)
if connection.features.supports_index_column_ordering:
if connection.features.uppercases_column_names:
index_name = index_name.upper()
self.assertIndexOrder(Author._meta.db_table, index_name, ['ASC', 'DESC'])
# Drop the index
with connection.schema_editor() as editor:
Expand Down Expand Up @@ -2122,12 +2117,14 @@ def get_field(*args, field_class=IntegerField, **kwargs):
field = get_field()
table = model._meta.db_table
column = field.column
identifier_converter = connection.introspection.identifier_converter

with connection.schema_editor() as editor:
editor.create_model(model)
editor.add_field(model, field)

constraint_name = "CamelCaseIndex"
constraint_name = 'CamelCaseIndex'
expected_constraint_name = identifier_converter(constraint_name)
editor.execute(
editor.sql_create_index % {
"table": editor.quote_name(table),
Expand All @@ -2138,22 +2135,20 @@ def get_field(*args, field_class=IntegerField, **kwargs):
"condition": "",
}
)
if connection.features.uppercases_column_names:
constraint_name = constraint_name.upper()
self.assertIn(constraint_name, self.get_constraints(model._meta.db_table))
self.assertIn(expected_constraint_name, self.get_constraints(model._meta.db_table))
editor.alter_field(model, get_field(db_index=True), field, strict=True)
self.assertNotIn(constraint_name, self.get_constraints(model._meta.db_table))
self.assertNotIn(expected_constraint_name, self.get_constraints(model._meta.db_table))

constraint_name = "CamelCaseUniqConstraint"
constraint_name = 'CamelCaseUniqConstraint'
expected_constraint_name = identifier_converter(constraint_name)
editor.execute(editor._create_unique_sql(model, [field.column], constraint_name))
if connection.features.uppercases_column_names:
constraint_name = constraint_name.upper()
self.assertIn(constraint_name, self.get_constraints(model._meta.db_table))
self.assertIn(expected_constraint_name, self.get_constraints(model._meta.db_table))
editor.alter_field(model, get_field(unique=True), field, strict=True)
self.assertNotIn(constraint_name, self.get_constraints(model._meta.db_table))
self.assertNotIn(expected_constraint_name, self.get_constraints(model._meta.db_table))

if editor.sql_foreign_key_constraint:
constraint_name = "CamelCaseFKConstraint"
constraint_name = 'CamelCaseFKConstraint'
expected_constraint_name = identifier_converter(constraint_name)
fk_sql = editor.sql_foreign_key_constraint % {
"column": editor.quote_name(column),
"to_table": editor.quote_name(table),
Expand All @@ -2170,11 +2165,9 @@ def get_field(*args, field_class=IntegerField, **kwargs):
"constraint": constraint_sql,
}
)
if connection.features.uppercases_column_names:
constraint_name = constraint_name.upper()
self.assertIn(constraint_name, self.get_constraints(model._meta.db_table))
self.assertIn(expected_constraint_name, self.get_constraints(model._meta.db_table))
editor.alter_field(model, get_field(Author, CASCADE, field_class=ForeignKey), field, strict=True)
self.assertNotIn(constraint_name, self.get_constraints(model._meta.db_table))
self.assertNotIn(expected_constraint_name, self.get_constraints(model._meta.db_table))

def test_add_field_use_effective_default(self):
"""
Expand Down Expand Up @@ -2491,11 +2484,7 @@ def test_alter_field_add_index_to_integerfield(self):
new_field.set_attributes_from_name('weight')
with connection.schema_editor() as editor:
editor.alter_field(Author, old_field, new_field, strict=True)

expected = 'schema_author_weight_587740f9'
if connection.features.uppercases_column_names:
expected = expected.upper()
self.assertEqual(self.get_constraints_for_column(Author, 'weight'), [expected])
self.assertEqual(self.get_constraints_for_column(Author, 'weight'), ['schema_author_weight_587740f9'])

# Remove db_index=True to drop index.
with connection.schema_editor() as editor:
Expand Down

0 comments on commit d5f4ce9

Please sign in to comment.