From 2e4dd1ba77cb6eeb10df7683a57ed105d2ea30fc Mon Sep 17 00:00:00 2001 From: barredterra <14891507+barredterra@users.noreply.github.com> Date: Mon, 12 Feb 2024 18:16:47 +0100 Subject: [PATCH 1/3] refactor: setup_backup_tables --- frappe/utils/backups.py | 25 +++++++++---------------- 1 file changed, 9 insertions(+), 16 deletions(-) diff --git a/frappe/utils/backups.py b/frappe/utils/backups.py index b9fa0d1022..b69ef799b1 100644 --- a/frappe/utils/backups.py +++ b/frappe/utils/backups.py @@ -125,24 +125,17 @@ class BackupGenerator: tables.append(table) return tables - passed_tables = { - "include": get_tables(self.include_doctypes.strip().split(",")), - "exclude": get_tables(self.exclude_doctypes.strip().split(",")), - } - specified_tables = get_tables(frappe.conf.get("backup", {}).get("includes", [])) - include_tables = (specified_tables + base_tables) if specified_tables else [] + self.backup_includes = get_tables(self.include_doctypes.strip().split(",")) + self.backup_excludes = get_tables(self.exclude_doctypes.strip().split(",")) - conf_tables = { - "include": include_tables, - "exclude": get_tables(frappe.conf.get("backup", {}).get("excludes", [])), - } + if not self.ignore_conf: + backup_conf = frappe.conf.get("backup", {}) + if not self.backup_includes: + specified_tables = get_tables(backup_conf.get("includes", [])) + self.backup_includes = (specified_tables + base_tables) if specified_tables else [] - self.backup_includes = passed_tables["include"] - self.backup_excludes = passed_tables["exclude"] - - if not self.backup_includes and not self.backup_excludes and not self.ignore_conf: - self.backup_includes = self.backup_includes or conf_tables["include"] - self.backup_excludes = self.backup_excludes or conf_tables["exclude"] + if not self.backup_excludes: + self.backup_excludes = get_tables(backup_conf.get("excludes", [])) self.partial = (self.backup_includes or self.backup_excludes) and not self.ignore_conf From 2a85c4c5d9e9892916a998ce540eaebf397ae01e Mon Sep 17 00:00:00 2001 From: barredterra <14891507+barredterra@users.noreply.github.com> Date: Tue, 13 Feb 2024 11:29:30 +0100 Subject: [PATCH 2/3] refactor: no inline if-statement Co-authored-by: Akhil Narang --- frappe/utils/backups.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/frappe/utils/backups.py b/frappe/utils/backups.py index b69ef799b1..a916621616 100644 --- a/frappe/utils/backups.py +++ b/frappe/utils/backups.py @@ -131,8 +131,10 @@ class BackupGenerator: if not self.ignore_conf: backup_conf = frappe.conf.get("backup", {}) if not self.backup_includes: - specified_tables = get_tables(backup_conf.get("includes", [])) - self.backup_includes = (specified_tables + base_tables) if specified_tables else [] + if specified_tables := get_tables(backup_conf.get("includes", [])): + self.backup_includes = specified_tables + base_tables + else: + self.backup_includes = [] if not self.backup_excludes: self.backup_excludes = get_tables(backup_conf.get("excludes", [])) From 30d63b165bf8712a997b0a14ac071a8b9d651e37 Mon Sep 17 00:00:00 2001 From: barredterra <14891507+barredterra@users.noreply.github.com> Date: Tue, 13 Feb 2024 11:44:50 +0100 Subject: [PATCH 3/3] refactor: split into multiple methods --- frappe/utils/backups.py | 63 +++++++++++++++++++++++++---------------- 1 file changed, 38 insertions(+), 25 deletions(-) diff --git a/frappe/utils/backups.py b/frappe/utils/backups.py index a916621616..fa93f6d6cd 100644 --- a/frappe/utils/backups.py +++ b/frappe/utils/backups.py @@ -111,36 +111,37 @@ class BackupGenerator: dir = os.path.dirname(file_path) os.makedirs(dir, exist_ok=True) + def _set_existing_tables(self): + """Ensure self._existing_tables is set.""" + if not hasattr(self, "_existing_tables"): + self._existing_tables = frappe.db.get_tables() + def setup_backup_tables(self): - """Sets self.backup_includes, self.backup_excludes based on passed args""" - existing_tables = frappe.db.get_tables() + """Set self.backup_includes, self.backup_excludes based on include_doctypes, exclude_doctypes""" + self._set_existing_tables() - def get_tables(doctypes): - tables = [] - for doctype in doctypes: - if not doctype: - continue - table = frappe.utils.get_table_name(doctype) - if table in existing_tables: - tables.append(table) - return tables - - self.backup_includes = get_tables(self.include_doctypes.strip().split(",")) - self.backup_excludes = get_tables(self.exclude_doctypes.strip().split(",")) - - if not self.ignore_conf: - backup_conf = frappe.conf.get("backup", {}) - if not self.backup_includes: - if specified_tables := get_tables(backup_conf.get("includes", [])): - self.backup_includes = specified_tables + base_tables - else: - self.backup_includes = [] - - if not self.backup_excludes: - self.backup_excludes = get_tables(backup_conf.get("excludes", [])) + self.backup_includes = _get_tables(self.include_doctypes.strip().split(","), self._existing_tables) + self.backup_excludes = _get_tables(self.exclude_doctypes.strip().split(","), self._existing_tables) + self.set_backup_tables_from_config() self.partial = (self.backup_includes or self.backup_excludes) and not self.ignore_conf + def set_backup_tables_from_config(self): + """Set self.backup_includes, self.backup_excludes based on site config""" + if self.ignore_conf: + return + + backup_conf = frappe.conf.get("backup", {}) + self._set_existing_tables() + if not self.backup_includes: + if specified_tables := _get_tables(backup_conf.get("includes", []), self._existing_tables): + self.backup_includes = specified_tables + base_tables + else: + self.backup_includes = [] + + if not self.backup_excludes: + self.backup_excludes = _get_tables(backup_conf.get("excludes", []), self._existing_tables) + @property def site_config_backup_path(self): # For backwards compatibility @@ -474,6 +475,18 @@ download only after 24 hours.""" return recipient_list +def _get_tables(doctypes: list[str], existing_tables: list[str]) -> list[str]: + """Return a list of tables for the given doctypes that exist in the database.""" + tables = [] + for doctype in doctypes: + if not doctype: + continue + table = frappe.utils.get_table_name(doctype) + if table in existing_tables: + tables.append(table) + return tables + + @frappe.whitelist() def fetch_latest_backups(partial=False) -> dict: """Fetch paths of the latest backup taken in the last 30 days.