From 40a2daf84c2fd82bbf8ca6266831a90a98567f2b Mon Sep 17 00:00:00 2001 From: David Arnold Date: Thu, 6 Apr 2023 20:47:20 -0500 Subject: [PATCH 1/4] chore: give cli invocations of dbtools a dedicated interface --- frappe/commands/utils.py | 71 ++++++------------- frappe/database/__init__.py | 73 +++++++++++++++++++ frappe/database/db_manager.py | 47 ++++++------ frappe/database/mariadb/setup_db.py | 4 +- frappe/database/postgres/setup_db.py | 44 ++++++------ frappe/utils/backups.py | 102 ++++++++++----------------- 6 files changed, 183 insertions(+), 158 deletions(-) diff --git a/frappe/commands/utils.py b/frappe/commands/utils.py index 715c079af3..c257812452 100644 --- a/frappe/commands/utils.py +++ b/frappe/commands/utils.py @@ -2,17 +2,16 @@ import json import os import subprocess import sys -from shutil import which import click import frappe +from frappe import _ from frappe.commands import get_site, pass_context from frappe.coverage import CodeCoverage from frappe.exceptions import SiteNotSpecifiedError from frappe.utils import cint, update_progress_bar -find_executable = which # backwards compatibility EXTRA_ARGS_CTX = {"ignore_unknown_options": True, "allow_extra_args": True} @@ -465,19 +464,11 @@ def database(context, extra_args): Enter into the Database console for given site. """ site = get_site(context) - if not site: - raise SiteNotSpecifiedError frappe.init(site=site) - if frappe.conf.db_type == "mariadb": - _mariadb(extra_args=extra_args) - elif frappe.conf.db_type == "postgres": - _psql(extra_args=extra_args) + _enter_console(extra_args=extra_args) -@click.command( - "mariadb", - context_settings=EXTRA_ARGS_CTX, -) +@click.command("mariadb", context_settings=EXTRA_ARGS_CTX) @click.argument("extra_args", nargs=-1) @pass_context def mariadb(context, extra_args): @@ -485,10 +476,9 @@ def mariadb(context, extra_args): Enter into mariadb console for a given site. """ site = get_site(context) - if not site: - raise SiteNotSpecifiedError frappe.init(site=site) - _mariadb(extra_args=extra_args) + frappe.conf.db_type = "mariadb" + _enter_console(extra_args=extra_args) @click.command("postgres", context_settings=EXTRA_ARGS_CTX) @@ -500,42 +490,27 @@ def postgres(context, extra_args): """ site = get_site(context) frappe.init(site=site) - _psql(extra_args=extra_args) + frappe.conf.db_type = "postgres" + _enter_console(extra_args=extra_args) -def _mariadb(extra_args=None): - mariadb = which("mariadb") or which("mysql") - command = [ - mariadb, - "--port", - str(frappe.conf.db_port), - "-u", - frappe.conf.db_name, - f"-p{frappe.conf.db_password}", - frappe.conf.db_name, - "-h", - frappe.conf.db_host, - "--pager=less -SFX", - "--safe-updates", - "-A", - ] - if extra_args: - command += list(extra_args) - os.execv(mariadb, command) +def _enter_console(extra_args=None): + from frappe.database import get_command - -def _psql(extra_args=None): - psql = which("psql") - - host = frappe.conf.db_host - port = frappe.conf.db_port - env = os.environ.copy() - env["PGPASSWORD"] = frappe.conf.db_password - conn_string = f"postgresql://{frappe.conf.db_name}@{host}:{port}/{frappe.conf.db_name}" - psql_cmd = [psql, conn_string] - if extra_args: - psql_cmd = psql_cmd + list(extra_args) - subprocess.run(psql_cmd, check=True, env=env) + bin, args, bin_name = get_command( + host=frappe.conf.db_host, + port=frappe.conf.db_port, + user=frappe.conf.db_name, + password=frappe.conf.db_password, + db_name=frappe.conf.db_name, + extra=list(extra_args) if extra_args else [], + ) + if not bin: + frappe.throw( + _("{} not found in PATH! This is required to access the console.").format(bin_name), + exc=frappe.ExecutableNotFound, + ) + os.execv(bin, [bin] + args) @click.command("jupyter") diff --git a/frappe/database/__init__.py b/frappe/database/__init__.py index 76ad24b6e6..7119685720 100644 --- a/frappe/database/__init__.py +++ b/frappe/database/__init__.py @@ -3,6 +3,7 @@ # Database Module # -------------------- +from shutil import which from frappe.database.database import savepoint @@ -50,3 +51,75 @@ def get_db(host=None, user=None, password=None, port=None): import frappe.database.mariadb.database return frappe.database.mariadb.database.MariaDBDatabase(host, user, password, port=port) + + +def get_command( + host=None, port=None, user=None, password=None, db_name=None, extra=None, dump=False +): + import frappe + + if frappe.conf.db_type == "postgres": + if dump: + bin, bin_name = which("pg_dump"), "pg_dump" + else: + bin, bin_name = which("psql"), "psql" + + host = frappe.utils.esc(host, "$ ") + user = frappe.utils.esc(user, "$ ") + db_name = frappe.utils.esc(db_name, "$ ") + + conn_string = str + if password: + password = frappe.utils.esc(password, "$ ") + conn_string = f"postgresql://{user}:{password}@{host}:{port}/{db_name}" + else: + conn_string = f"postgresql://{user}@{host}:{port}/{db_name}" + + command = [conn_string] + + if extra: + command.extend(extra) + + else: + if dump: + bin, bin_name = which("mariadb-dump") or which("mysqldump"), "mariadb-dump" + else: + bin, bin_name = which("mariadb") or which("mysql"), "mariadb" + + host = frappe.utils.esc(host, "$ ") + user = frappe.utils.esc(user, "$ ") + db_name = frappe.utils.esc(db_name, "$ ") + + command = [ + f"--user={user}", + f"--host={host}", + f"--port={port}", + ] + + if password: + password = frappe.utils.esc(password, "$ ") + command.append(f"--password={password}") + + if dump: + command.extend( + [ + "--single-transaction", + "--quick", + "--lock-tables=false", + ] + ) + else: + command.extend( + [ + "--pager='less -SFX'", + "--safe-updates", + "--no-auto-rehash", + ] + ) + + command.append(db_name) + + if extra: + command.extend(extra) + + return bin, command, bin_name diff --git a/frappe/database/db_manager.py b/frappe/database/db_manager.py index 6431217585..94b106b788 100644 --- a/frappe/database/db_manager.py +++ b/frappe/database/db_manager.py @@ -1,4 +1,5 @@ import frappe +from frappe import _ class DbManager: @@ -49,37 +50,37 @@ class DbManager: return self.db.sql("SHOW DATABASES", pluck=True) @staticmethod - def restore_database(target, source, user, password): - import os + def restore_database(verbose, target, source, user, password): from shutil import which - from frappe.utils import make_esc + from frappe.database import get_command + from frappe.utils import execute_in_shell - esc = make_esc("$ ") pv = which("pv") - mariadb_cli = which("mariadb") or which("mysql") + + command = [] if pv: - pipe = f"{pv} {source} |" - source = "" - else: - pipe = "" - source = f"< {source}" - - if pipe: + command.extend([f"{pv}", f"{source}", "|"]) + source = [] print("Restoring Database file...") + else: + source = ["<", f"{source}"] - command = "{pipe} {mariadb_cli} -u {user} -p{password} -h{host} -P{port} {target} {source}" - command = command.format( - pipe=pipe, - user=esc(user), - password=esc(password), - host=esc(frappe.conf.db_host), - target=esc(target), - source=source, + bin, args, bin_name = get_command( + host=frappe.conf.db_host, port=frappe.conf.db_port, - mariadb_cli=mariadb_cli, + user=user, + password=password, + db_name=target, ) - - os.system(command) + if not bin: + frappe.throw( + _("{} not found in PATH! This is required to restore the database.").format(bin_name), + exc=frappe.ExecutableNotFound, + ) + command.append(bin) + command.extend(args) + command.extend(source) + execute_in_shell(" ".join(command), check_exit_code=True, verbose=verbose) frappe.cache.delete_keys("") # Delete all keys associated with this site. diff --git a/frappe/database/mariadb/setup_db.py b/frappe/database/mariadb/setup_db.py index 87a7f6d343..f2caf05463 100644 --- a/frappe/database/mariadb/setup_db.py +++ b/frappe/database/mariadb/setup_db.py @@ -97,7 +97,9 @@ def import_db_from_sql(source_sql=None, verbose=False): db_name = frappe.conf.db_name if not source_sql: source_sql = os.path.join(os.path.dirname(__file__), "framework_mariadb.sql") - DbManager(frappe.local.db).restore_database(db_name, source_sql, db_name, frappe.conf.db_password) + DbManager(frappe.local.db).restore_database( + verbose, db_name, source_sql, db_name, frappe.conf.db_password + ) if verbose: print("Imported from database %s" % source_sql) diff --git a/frappe/database/postgres/setup_db.py b/frappe/database/postgres/setup_db.py index 9d1d045b04..e838a4bfa0 100644 --- a/frappe/database/postgres/setup_db.py +++ b/frappe/database/postgres/setup_db.py @@ -1,6 +1,7 @@ import os import frappe +from frappe import _ def setup_database(force, source_sql=None, verbose=False): @@ -39,12 +40,9 @@ def bootstrap_database(db_name, verbose, source_sql=None): def import_db_from_sql(source_sql=None, verbose=False): from shutil import which - from subprocess import PIPE, run - # we can't pass psql password in arguments in postgresql as mysql. So - # set password connection parameter in environment variable - subprocess_env = os.environ.copy() - subprocess_env["PGPASSWORD"] = str(frappe.conf.db_password) + from frappe.database import get_command + from frappe.utils import execute_in_shell # bootstrap db if not source_sql: @@ -52,27 +50,33 @@ def import_db_from_sql(source_sql=None, verbose=False): pv = which("pv") - _command = ( - f"psql {frappe.conf.db_name} " - f"-h {frappe.conf.db_host} -p {str(frappe.conf.db_port)} " - f"-U {frappe.conf.db_name}" - ) + command = [] if pv: - command = f"{pv} {source_sql} | " + _command + command.extend([f"{pv}", f"{source_sql}", "|"]) + source = [] + print("Restoring Database file...") else: - command = _command + f" -f {source_sql}" + source = ["-f", f"{source_sql}"] - print("Restoring Database file...") - if verbose: - print(command) + bin, args, bin_name = get_command( + host=frappe.conf.db_host, + port=frappe.conf.db_port, + user=frappe.conf.db_name, + password=frappe.conf.db_password, + db_name=frappe.conf.db_name, + ) - restore_proc = run(command, env=subprocess_env, shell=True, stdout=PIPE) - - if verbose: - print( - f"\nSTDOUT by psql:\n{restore_proc.stdout.decode()}\nImported from Database File: {source_sql}" + if not bin: + frappe.throw( + _("{} not found in PATH! This is required to restore the database.").format(bin_name), + exc=frappe.ExecutableNotFound, ) + command.append(bin) + command.extend(args) + command.extend(source) + execute_in_shell(" ".join(command), check_exit_code=True, verbose=verbose) + frappe.cache.delete_keys("") # Delete all keys associated with this site. def get_root_connection(root_login=None, root_password=None): diff --git a/frappe/utils/backups.py b/frappe/utils/backups.py index 11f90fe07c..eebccae5f6 100644 --- a/frappe/utils/backups.py +++ b/frappe/utils/backups.py @@ -16,7 +16,7 @@ from cryptography.fernet import Fernet # imports - module imports import frappe import frappe.utils -from frappe import conf +from frappe import _, conf from frappe.utils import cint, get_file_size, get_url, now, now_datetime # backup variable for backwards compatibility @@ -362,37 +362,14 @@ class BackupGenerator: with open(site_config_backup_path, "w") as n, open(site_config_path) as c: n.write(c.read()) - def get_db_dump_exeuctable(self) -> str: - db_exc, exists = None, False - - if self.db_type == "mariadb": - if mariadb_dump_path := which("mariadb-dump"): - exists = bool(mariadb_dump_path) - db_exc = "mariadb-dump" - else: - # Fallback to mysqldump if mariadb-dump is not available. - db_exc = "mysqldump" - exists = bool(which(db_exc)) - elif self.db_type == "postgres": - db_exc = "pg_dump" - exists = bool(which(db_exc)) - - if not exists: - frappe.throw( - f"{db_exc} not found in PATH! This is required to take a backup.", - exc=frappe.ExecutableNotFound, - ) - return db_exc - def take_dump(self): import frappe.utils from frappe.utils.change_log import get_app_branch - db_exc = self.get_db_dump_exeuctable() gzip_exc = which("gzip") if not gzip_exc: frappe.throw( - "`gzip` not found in PATH! This is required to take a backup.", exc=frappe.ExecutableNotFound + _("gzip not found in PATH! This is required to take a backup."), exc=frappe.ExecutableNotFound ) database_header_content = [ @@ -400,11 +377,6 @@ class BackupGenerator: "", ] - # escape reserved characters - args = frappe._dict( - [item[0], frappe.utils.esc(str(item[1]), "$ ")] for item in self.__dict__.copy().items() - ) - if self.backup_includes: backup_info = ("Backing Up Tables: ", ", ".join(self.backup_includes)) elif self.backup_excludes: @@ -423,54 +395,52 @@ class BackupGenerator: generated_header = "\n".join(f"-- {x}" for x in database_header_content) + "\n" - with gzip.open(args.backup_path_db, "wt") as f: + with gzip.open(self.backup_path_db, "wt") as f: f.write(generated_header) - if self.db_type == "postgres": + # Remember process of this shell and kill it if mysqldump exits w/ non-zero code + def wrap(cmd): + ret = ["self=$$;", "("] + ret.extend(cmd) + ret.extend(["||", "kill", "$self", ")", "|", gzip_exc, ">>", self.backup_path_db]) + return ret + + cmd = [] + extra = [] + if self.db_type == "mariadb": if self.backup_includes: - args["include"] = " ".join([f"--table='public.\"{table}\"'" for table in self.backup_includes]) + extra.extend([f"'{x}'" for x in self.backup_includes]) elif self.backup_excludes: - args["exclude"] = " ".join( - [f"--exclude-table-data='public.\"{table}\"'" for table in self.backup_excludes] - ) + extra.extend([f"--ignore-table='{self.db_name}.{table}'" for table in self.backup_excludes]) - cmd_string = ( - "self=$$; " - "( {db_exc} postgres://{user}:{password}@{db_host}:{db_port}/{db_name}" - " {include} {exclude} || kill $self ) | {gzip} >> {backup_path_db}" - ) - - else: + elif self.db_type == "postgres": if self.backup_includes: - args["include"] = " ".join([f"'{x}'" for x in self.backup_includes]) + extra.extend([f"--table='public.\"{table}\"'" for table in self.backup_includes]) elif self.backup_excludes: - args["exclude"] = " ".join( - [f"--ignore-table='{self.db_name}.{table}'" for table in self.backup_excludes] - ) + extra.extend([f"--exclude-table-data='public.\"{table}\"'" for table in self.backup_excludes]) - cmd_string = ( - # Remember process of this shell and kill it if mysqldump exits w/ non-zero code - "self=$$; " - " ( {db_exc} --single-transaction --quick --lock-tables=false -u {user}" - " -p{password} {db_name} -h {db_host} -P {db_port} {include} {exclude} || kill $self ) " - " | {gzip} >> {backup_path_db}" - ) + from frappe.database import get_command - command = cmd_string.format( - user=args.user, - password=args.password, - db_exc=db_exc, - db_host=args.db_host, - db_port=args.db_port, - db_name=args.db_name, - backup_path_db=args.backup_path_db, - exclude=args.get("exclude", ""), - include=args.get("include", ""), - gzip=gzip_exc, + bin, args, bin_name = get_command( + host=self.db_host, + port=self.db_port, + user=self.user, + password=self.password, + db_name=self.db_name, + extra=extra, + dump=True, ) + if not bin: + frappe.throw( + _("{} not found in PATH! This is required to take a backup.").format(bin_name), + exc=frappe.ExecutableNotFound, + ) + cmd.append(bin) + cmd.extend(args) + command = " ".join(wrap(cmd)) if self.verbose: - print(command.replace(args.password, "*" * 10) + "\n") + print(command.replace(frappe.utils.esc(self.password, "$ "), "*" * 10) + "\n") frappe.utils.execute_in_shell(command, low_priority=True, check_exit_code=True) From 5b33608fe32109e16d19af2e2eed255717b0c9af Mon Sep 17 00:00:00 2001 From: David Arnold Date: Wed, 15 Nov 2023 11:21:16 +0100 Subject: [PATCH 2/4] fix: shell escaping in execute_in_shell helper --- frappe/database/__init__.py | 2 +- frappe/database/db_manager.py | 3 ++- frappe/database/postgres/setup_db.py | 3 ++- frappe/utils/__init__.py | 5 +++++ 4 files changed, 10 insertions(+), 3 deletions(-) diff --git a/frappe/database/__init__.py b/frappe/database/__init__.py index 7119685720..0330db78fc 100644 --- a/frappe/database/__init__.py +++ b/frappe/database/__init__.py @@ -111,7 +111,7 @@ def get_command( else: command.extend( [ - "--pager='less -SFX'", + "--pager=less -SFX", "--safe-updates", "--no-auto-rehash", ] diff --git a/frappe/database/db_manager.py b/frappe/database/db_manager.py index 94b106b788..4de52c07d0 100644 --- a/frappe/database/db_manager.py +++ b/frappe/database/db_manager.py @@ -51,6 +51,7 @@ class DbManager: @staticmethod def restore_database(verbose, target, source, user, password): + import shlex from shutil import which from frappe.database import get_command @@ -80,7 +81,7 @@ class DbManager: exc=frappe.ExecutableNotFound, ) command.append(bin) - command.extend(args) + command.append(shlex.join(args)) command.extend(source) execute_in_shell(" ".join(command), check_exit_code=True, verbose=verbose) frappe.cache.delete_keys("") # Delete all keys associated with this site. diff --git a/frappe/database/postgres/setup_db.py b/frappe/database/postgres/setup_db.py index e838a4bfa0..4f1095536a 100644 --- a/frappe/database/postgres/setup_db.py +++ b/frappe/database/postgres/setup_db.py @@ -39,6 +39,7 @@ def bootstrap_database(db_name, verbose, source_sql=None): def import_db_from_sql(source_sql=None, verbose=False): + import shlex from shutil import which from frappe.database import get_command @@ -73,7 +74,7 @@ def import_db_from_sql(source_sql=None, verbose=False): exc=frappe.ExecutableNotFound, ) command.append(bin) - command.extend(args) + command.append(shlex.join(args)) command.extend(source) execute_in_shell(" ".join(command), check_exit_code=True, verbose=verbose) frappe.cache.delete_keys("") # Delete all keys associated with this site. diff --git a/frappe/utils/__init__.py b/frappe/utils/__init__.py index 48b492a3f4..8df7f52b60 100644 --- a/frappe/utils/__init__.py +++ b/frappe/utils/__init__.py @@ -443,9 +443,14 @@ def unesc(s, esc_chars): def execute_in_shell(cmd, verbose=False, low_priority=False, check_exit_code=False): # using Popen instead of os.system - as recommended by python docs + import shlex import tempfile from subprocess import Popen + if type(cmd) is list: + # ensure it's properly escaped; only a single string argument executes via shell + cmd = shlex.join(cmd) + with (tempfile.TemporaryFile() as stdout, tempfile.TemporaryFile() as stderr): kwargs = {"shell": True, "stdout": stdout, "stderr": stderr} From 9228cd70f81d13ba27490e11d78ac8c8688874c7 Mon Sep 17 00:00:00 2001 From: David Arnold Date: Fri, 17 Nov 2023 09:30:53 +0100 Subject: [PATCH 3/4] fix: code style --- frappe/utils/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frappe/utils/__init__.py b/frappe/utils/__init__.py index 8df7f52b60..6d6162138e 100644 --- a/frappe/utils/__init__.py +++ b/frappe/utils/__init__.py @@ -447,7 +447,7 @@ def execute_in_shell(cmd, verbose=False, low_priority=False, check_exit_code=Fal import tempfile from subprocess import Popen - if type(cmd) is list: + if isinstance(cmd, list): # ensure it's properly escaped; only a single string argument executes via shell cmd = shlex.join(cmd) From 283e91fd43084f6e065329f3597e1ada5f0e3ac9 Mon Sep 17 00:00:00 2001 From: David Arnold Date: Fri, 17 Nov 2023 10:00:07 +0100 Subject: [PATCH 4/4] fix: minor fixes Co-authored-by: Akhil Narang --- frappe/database/__init__.py | 1 - frappe/database/db_manager.py | 4 ++-- frappe/database/postgres/setup_db.py | 4 ++-- 3 files changed, 4 insertions(+), 5 deletions(-) diff --git a/frappe/database/__init__.py b/frappe/database/__init__.py index 0330db78fc..1918a54727 100644 --- a/frappe/database/__init__.py +++ b/frappe/database/__init__.py @@ -68,7 +68,6 @@ def get_command( user = frappe.utils.esc(user, "$ ") db_name = frappe.utils.esc(db_name, "$ ") - conn_string = str if password: password = frappe.utils.esc(password, "$ ") conn_string = f"postgresql://{user}:{password}@{host}:{port}/{db_name}" diff --git a/frappe/database/db_manager.py b/frappe/database/db_manager.py index 4de52c07d0..68cd39f2f5 100644 --- a/frappe/database/db_manager.py +++ b/frappe/database/db_manager.py @@ -62,11 +62,11 @@ class DbManager: command = [] if pv: - command.extend([f"{pv}", f"{source}", "|"]) + command.extend([pv, source, "|"]) source = [] print("Restoring Database file...") else: - source = ["<", f"{source}"] + source = ["<", source] bin, args, bin_name = get_command( host=frappe.conf.db_host, diff --git a/frappe/database/postgres/setup_db.py b/frappe/database/postgres/setup_db.py index 4f1095536a..7d56bd24c0 100644 --- a/frappe/database/postgres/setup_db.py +++ b/frappe/database/postgres/setup_db.py @@ -54,11 +54,11 @@ def import_db_from_sql(source_sql=None, verbose=False): command = [] if pv: - command.extend([f"{pv}", f"{source_sql}", "|"]) + command.extend([pv, source_sql, "|"]) source = [] print("Restoring Database file...") else: - source = ["-f", f"{source_sql}"] + source = ["-f", source_sql] bin, args, bin_name = get_command( host=frappe.conf.db_host,