fix: correct index syncing (backport #18262) (#20841)

* refactor: guard clause (cherry picked from commit 6de41a7) # Conflicts: # frappe/database/mariadb/schema.py * fix: correct index re-syncing The implementation of syncing unique and non-unique index depended on index names which used to be different before because of that there's tendency to incorrectly identify index. This PR adds a separate util for checking if a column has index without relying on naming convention. It just goes and checks if there's any index with that column in it, hence far more reliable. (cherry picked from commit cbe4b59) # Conflicts: # frappe/database/mariadb/database.py # frappe/database/mariadb/schema.py * fix: index column should be first (cherry picked from commit da561c2) * chore: resolve conflict * chore: expand walrus * fix: py3.7 compat --------- Co-authored-by: Ankush Menat <ankush@frappe.io>
frappe · Jun 1, 2023 · 6d69c05 · 6d69c05
1 parent 1597a20
commit 6d69c05
Show file tree

Hide file tree

Showing 2 changed files with 47 additions and 37 deletions.
diff --git a/frappe/database/mariadb/database.py b/frappe/database/mariadb/database.py
@@ -277,6 +277,7 @@ def get_table_columns_description(self, table_name):
 				where table_name="{table_name}"
 					and column_name=columns.column_name
 					and NON_UNIQUE=1
+					and Seq_in_index = 1
 					limit 1
 			), 0) as 'index',
 			column_key = 'UNI' as 'unique'
@@ -295,6 +296,35 @@ def has_index(self, table_name, index_name):
 			)
 		)
 
+	def get_column_index(self, table_name: str, fieldname: str, unique: bool = False):
+		"""Check if column exists for a specific fields in specified order.
+
+		This differs from db.has_index because it doesn't rely on index name but columns inside an
+		index.
+		"""
+
+		indexes = self.sql(
+			f"""SHOW INDEX FROM `{table_name}`
+				WHERE Column_name = "{fieldname}"
+					AND Seq_in_index = 1
+					AND Non_unique={int(not unique)}
+				""",
+			as_dict=True,
+		)
+
+		# Same index can be part of clustered index which contains more fields
+		# We don't want those.
+		for index in indexes:
+			clustered_index = self.sql(
+				f"""SHOW INDEX FROM `{table_name}`
+					WHERE Key_name = "{index.Key_name}"
+						AND Seq_in_index = 2
+					""",
+				as_dict=True,
+			)
+			if not clustered_index:
+				return index
+
 	def add_index(self, doctype, fields, index_name=None):
 		"""Creates an index with given fields if not already created.
 		Index name will be `fieldname1_fieldname2_index`"""

diff --git a/frappe/database/mariadb/schema.py b/frappe/database/mariadb/schema.py
@@ -68,45 +68,25 @@ def alter(self):
 
 		for col in self.add_index:
 			# if index key does not exists
-			if not frappe.db.has_index(self.table_name, col.fieldname + "_index"):
-				add_index_query.append("ADD INDEX `{}_index`(`{}`)".format(col.fieldname, col.fieldname))
+			if not frappe.db.get_column_index(self.table_name, col.fieldname, unique=False):
+				add_index_query.append(f"ADD INDEX `{col.fieldname}_index`(`{col.fieldname}`)")
 
 		for col in {*self.drop_index, *self.drop_unique}:
-			if col.fieldname != "name":  # primary key
-				current_column = self.current_columns.get(col.fieldname.lower())
-				unique_constraint_changed = current_column.unique != col.unique
-				if unique_constraint_changed and not col.unique:
-					# nosemgrep
-					unique_index_record = frappe.db.sql(
-						"""
-						SHOW INDEX FROM `{0}`
-						WHERE Key_name=%s
-						AND Non_unique=0
-					""".format(
-							self.table_name
-						),
-						(col.fieldname),
-						as_dict=1,
-					)
-					if unique_index_record:
-						drop_index_query.append("DROP INDEX `{}`".format(unique_index_record[0].Key_name))
-				index_constraint_changed = current_column.index != col.set_index
-				# if index key exists
-				if index_constraint_changed and not col.set_index:
-					# nosemgrep
-					index_record = frappe.db.sql(
-						"""
-						SHOW INDEX FROM `{0}`
-						WHERE Key_name=%s
-						AND Non_unique=1
-					""".format(
-							self.table_name
-						),
-						(col.fieldname + "_index"),
-						as_dict=1,
-					)
-					if index_record:
-						drop_index_query.append("DROP INDEX `{}`".format(index_record[0].Key_name))
+			if col.fieldname == "name":
+				continue
+
+			current_column = self.current_columns.get(col.fieldname.lower())
+			unique_constraint_changed = current_column.unique != col.unique
+			if unique_constraint_changed and not col.unique:
+				unique_index = frappe.db.get_column_index(self.table_name, col.fieldname, unique=True)
+				if unique_index:
+					drop_index_query.append(f"DROP INDEX `{unique_index.Key_name}`")
+
+			index_constraint_changed = current_column.index != col.set_index
+			if index_constraint_changed and not col.set_index:
+				index_record = frappe.db.get_column_index(self.table_name, col.fieldname, unique=False)
+				if index_record:
+					drop_index_query.append(f"DROP INDEX `{index_record.Key_name}`")
 
 		try:
 			for query_parts in [add_column_query, modify_column_query, add_index_query, drop_index_query]: