From 073daae6c1aaca1b2af6a2368cccc3a92671a0f7 Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Fri, 30 Sep 2022 11:21:31 +0530 Subject: [PATCH 1/3] refactor: guard clause (cherry picked from commit 6de41a78e94ffa5d18b0ffddc5d71c23fd1198fb) --- frappe/database/mariadb/schema.py | 72 ++++++++++++++++--------------- 1 file changed, 37 insertions(+), 35 deletions(-) diff --git a/frappe/database/mariadb/schema.py b/frappe/database/mariadb/schema.py index 99297fbab2..472cc5dfa0 100644 --- a/frappe/database/mariadb/schema.py +++ b/frappe/database/mariadb/schema.py @@ -88,41 +88,43 @@ class MariaDBTable(DBTable): 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 `{}` - 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(f"DROP INDEX `{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 `{}` - 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(f"DROP INDEX `{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: + # nosemgrep + unique_index_record = frappe.db.sql( + """ + SHOW INDEX FROM `{}` + 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(f"DROP INDEX `{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 `{}` + 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(f"DROP INDEX `{index_record[0].Key_name}`") try: for query_parts in [add_column_query, modify_column_query, add_index_query, drop_index_query]: From 10e1eed077af6dc332aad2622bfc78b3e09447a8 Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Fri, 30 Sep 2022 12:16:07 +0530 Subject: [PATCH 2/3] 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 cbe4b591000599e92f14f6451e0a88d28d5b89ab) --- frappe/database/mariadb/database.py | 31 +++++++++++++++++++++++++ frappe/database/mariadb/schema.py | 36 +++++------------------------ 2 files changed, 37 insertions(+), 30 deletions(-) diff --git a/frappe/database/mariadb/database.py b/frappe/database/mariadb/database.py index 3fc241454e..34233d7724 100644 --- a/frappe/database/mariadb/database.py +++ b/frappe/database/mariadb/database.py @@ -319,6 +319,37 @@ class MariaDBDatabase(MariaDBConnectionUtil, MariaDBExceptionUtil, Database): ) ) + def get_column_index( + self, table_name: str, fieldname: str, unique: bool = False + ) -> frappe._dict | None: + """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: str, fields: list, index_name: str = 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 index 472cc5dfa0..6a85089190 100644 --- a/frappe/database/mariadb/schema.py +++ b/frappe/database/mariadb/schema.py @@ -84,7 +84,7 @@ class MariaDBTable(DBTable): for col in self.add_index: # if index key does not exists - if not frappe.db.has_index(self.table_name, col.fieldname + "_index"): + 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: @@ -94,37 +94,13 @@ class MariaDBTable(DBTable): 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 `{}` - 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(f"DROP INDEX `{unique_index_record[0].Key_name}`") + if unique_index := frappe.db.get_column_index(self.table_name, col.fieldname, unique=True): + drop_index_query.append(f"DROP INDEX `{unique_index.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 `{}` - 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(f"DROP INDEX `{index_record[0].Key_name}`") + if index_record := frappe.db.get_column_index(self.table_name, col.fieldname, unique=False): + 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]: From dd7faf5bdc7777810b2928021df899fb49889ce9 Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Fri, 30 Sep 2022 13:03:59 +0530 Subject: [PATCH 3/3] fix: index column should be first (cherry picked from commit da561c237d2bd2d8ffd5cb047c74fa95dcd6baef) --- frappe/database/mariadb/database.py | 1 + 1 file changed, 1 insertion(+) diff --git a/frappe/database/mariadb/database.py b/frappe/database/mariadb/database.py index 34233d7724..1df9877eb1 100644 --- a/frappe/database/mariadb/database.py +++ b/frappe/database/mariadb/database.py @@ -301,6 +301,7 @@ class MariaDBDatabase(MariaDBConnectionUtil, MariaDBExceptionUtil, Database): 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'