From af1cc27988f16bde56b4b07487b72002a8788236 Mon Sep 17 00:00:00 2001 From: Divyansh Singh Date: Thu, 2 Mar 2023 23:55:16 +0530 Subject: [PATCH 1/8] feat: add dump-restore support for sqlite databases --- borgmatic/config/schema.yaml | 25 ++++++ borgmatic/hooks/dispatch.py | 2 + borgmatic/hooks/dump.py | 7 +- borgmatic/hooks/sqlite.py | 122 +++++++++++++++++++++++++++ docs/how-to/backup-your-databases.md | 15 +++- tests/end-to-end/test_database.py | 3 + tests/unit/hooks/test_sqlite.py | 75 ++++++++++++++++ 7 files changed, 244 insertions(+), 5 deletions(-) create mode 100644 borgmatic/hooks/sqlite.py create mode 100644 tests/unit/hooks/test_sqlite.py diff --git a/borgmatic/config/schema.yaml b/borgmatic/config/schema.yaml index d7acc1c1..6f399ed2 100644 --- a/borgmatic/config/schema.yaml +++ b/borgmatic/config/schema.yaml @@ -931,6 +931,31 @@ properties: mysqldump/mysql commands (from either MySQL or MariaDB). See https://dev.mysql.com/doc/refman/8.0/en/mysqldump.html or https://mariadb.com/kb/en/library/mysqldump/ for details. + sqlite_databases: + type: array + items: + type: object + required: ['path','name'] + additionalProperties: false + properties: + path: + type: string + description: | + Path to the SQLite database file to dump. If + relative, it is relative to the current working + directory. If absolute, it is relative to the + root of the filesystem. Note that using this + database hook implicitly enables both + read_special and one_file_system (see above) to + support dump and restore streaming. + example: /var/lib/sqlite/users.db + name: + type: string + description: | + This is used to tag the database dump file with + a name. It is not used to identify the database + file itself. + example: users mongodb_databases: type: array items: diff --git a/borgmatic/hooks/dispatch.py b/borgmatic/hooks/dispatch.py index 41dcee04..88a99eb1 100644 --- a/borgmatic/hooks/dispatch.py +++ b/borgmatic/hooks/dispatch.py @@ -9,6 +9,7 @@ from borgmatic.hooks import ( ntfy, pagerduty, postgresql, + sqlite, ) logger = logging.getLogger(__name__) @@ -22,6 +23,7 @@ HOOK_NAME_TO_MODULE = { 'ntfy': ntfy, 'pagerduty': pagerduty, 'postgresql_databases': postgresql, + 'sqlite_databases': sqlite, } diff --git a/borgmatic/hooks/dump.py b/borgmatic/hooks/dump.py index d4c8e3fd..43686d36 100644 --- a/borgmatic/hooks/dump.py +++ b/borgmatic/hooks/dump.py @@ -6,7 +6,12 @@ from borgmatic.borg.state import DEFAULT_BORGMATIC_SOURCE_DIRECTORY logger = logging.getLogger(__name__) -DATABASE_HOOK_NAMES = ('postgresql_databases', 'mysql_databases', 'mongodb_databases') +DATABASE_HOOK_NAMES = ( + 'postgresql_databases', + 'mysql_databases', + 'mongodb_databases', + 'sqlite_databases', +) def make_database_dump_path(borgmatic_source_directory, database_hook_name): diff --git a/borgmatic/hooks/sqlite.py b/borgmatic/hooks/sqlite.py new file mode 100644 index 00000000..f9b83aea --- /dev/null +++ b/borgmatic/hooks/sqlite.py @@ -0,0 +1,122 @@ +import logging +import os +from subprocess import CalledProcessError + +from borgmatic.execute import execute_command, execute_command_with_processes +from borgmatic.hooks import dump + +logger = logging.getLogger(__name__) + + +def make_dump_path(location_config): # pragma: no cover + ''' + Make the dump path from the given location configuration and the name of this hook. + ''' + return dump.make_database_dump_path( + location_config.get('borgmatic_source_directory'), 'sqlite_databases' + ) + + +def dump_databases(databases, log_prefix, location_config, dry_run): # pragma: no cover + ''' + Dump the given SQLite3 databases to a file. The databases are supplied as a sequence of + configuration dicts, as per the configuration schema. Use the given log prefix in any log + entries. Use the given location configuration dict to construct the destination path. If this + is a dry run, then don't actually dump anything. + ''' + dry_run_label = ' (dry run; not actually dumping anything)' if dry_run else '' + processes = [] + + logger.info('{}: Dumping SQLite databases{}'.format(log_prefix, dry_run_label)) + + for database in databases: + database_path = database['path'] + database_filename = database['name'] + dump_path = make_dump_path(location_config) + dump_filename = dump.make_database_dump_filename(dump_path, database_filename) + if os.path.exists(dump_filename): + logger.warning( + f'{log_prefix}: Skipping duplicate dump of SQLite database at {database_path} to {dump_filename}' + ) + continue + + command = ( + 'sqlite3', + database_path, + '.dump', + '>', + dump_filename, + ) + logger.debug( + f'{log_prefix}: Dumping SQLite database at {database_path} to {dump_filename}{dry_run_label}' + ) + if dry_run: + continue + + dump.create_parent_directory_for_dump(dump_filename) + processes.append(execute_command(command, shell=True, run_to_completion=False)) + + return processes + + +def remove_database_dumps(databases, log_prefix, location_config, dry_run): # pragma: no cover + ''' + Remove the given SQLite3 database dumps from the filesystem. The databases are supplied as a + sequence of configuration dicts, as per the configuration schema. Use the given log prefix in + any log entries. Use the given location configuration dict to construct the destination path. + If this is a dry run, then don't actually remove anything. + ''' + dump.remove_database_dumps(make_dump_path(location_config), 'SQLite', log_prefix, dry_run) + + +def make_database_dump_pattern( + databases, log_prefix, location_config, name=None +): # pragma: no cover + ''' + Make a pattern that matches the given SQLite3 databases. The databases are supplied as a + sequence of configuration dicts, as per the configuration schema. + ''' + return dump.make_database_dump_filename(make_dump_path(location_config), name) + + +def restore_database_dump(database_config, log_prefix, location_config, dry_run, extract_process): + ''' + Restore the given SQLite3 database from an extract stream. The database is supplied as a + one-element sequence containing a dict describing the database, as per the configuration schema. + Use the given log prefix in any log entries. If this is a dry run, then don't actually restore + anything. Trigger the given active extract process (an instance of subprocess.Popen) to produce + output to consume. + ''' + dry_run_label = ' (dry run; not actually restoring anything)' if dry_run else '' + + if len(database_config) != 1: + raise ValueError('The database configuration value is invalid') + + database_path = database_config[0]['path'] + + logger.debug(f'{log_prefix}: Restoring SQLite database at {database_path}{dry_run_label}') + if dry_run: + return + + remove_command = ( + 'rm', + database_path, + ) + try: + execute_command(remove_command, shell=True) + except CalledProcessError: + logger.info(f'{log_prefix}: Database does not exist at {database_path}, skipping removal') + + restore_command = ( + 'sqlite3', + database_path, + ) + + # Don't give Borg local path so as to error on warnings, as "borg extract" only gives a warning + # if the restore paths don't exist in the archive. + execute_command_with_processes( + restore_command, + [extract_process], + output_log_level=logging.DEBUG, + input_file=extract_process.stdout, + ) diff --git a/docs/how-to/backup-your-databases.md b/docs/how-to/backup-your-databases.md index 1c7397be..5bf2002a 100644 --- a/docs/how-to/backup-your-databases.md +++ b/docs/how-to/backup-your-databases.md @@ -15,8 +15,8 @@ consistent snapshot that is more suited for backups. Fortunately, borgmatic includes built-in support for creating database dumps prior to running backups. For example, here is everything you need to dump and -backup a couple of local PostgreSQL databases, a MySQL/MariaDB database, and a -MongoDB database: +backup a couple of local PostgreSQL databases, a MySQL/MariaDB database, a +MongoDB database and a SQLite database: ```yaml hooks: @@ -27,6 +27,9 @@ hooks: - name: posts mongodb_databases: - name: messages + sqlite_databases: + - path: /var/lib/sqlite3/mydb.sqlite + - name: mydb ``` As part of each backup, borgmatic streams a database dump for each configured @@ -74,6 +77,9 @@ hooks: password: trustsome1 authentication_database: mongousers options: "--ssl" + sqlite_databases: + - path: /var/lib/sqlite3/mydb.sqlite + - name: mydb ``` See your [borgmatic configuration @@ -97,7 +103,8 @@ hooks: ``` Note that you may need to use a `username` of the `postgres` superuser for -this to work with PostgreSQL. +this to work with PostgreSQL. Also, the `all` database name is not supported +for SQLite databases. New in version 1.7.6 With PostgreSQL and MySQL, you can optionally dump "all" databases to separate @@ -154,7 +161,7 @@ bring back any missing configuration files in order to restore a database. ## Supported databases -As of now, borgmatic supports PostgreSQL, MySQL/MariaDB, and MongoDB databases +As of now, borgmatic supports PostgreSQL, MySQL/MariaDB, MongoDB databases and SQLite databases directly. But see below about general-purpose preparation and cleanup hooks as a work-around with other database systems. Also, please [file a ticket](https://torsion.org/borgmatic/#issues) for additional database systems diff --git a/tests/end-to-end/test_database.py b/tests/end-to-end/test_database.py index f981b402..3dcb82af 100644 --- a/tests/end-to-end/test_database.py +++ b/tests/end-to-end/test_database.py @@ -73,6 +73,9 @@ hooks: hostname: mongodb username: root password: test + sqlite_databases: + - path: /tmp/sqlite_test.db + - name: sqlite_test ''' with open(config_path, 'w') as config_file: diff --git a/tests/unit/hooks/test_sqlite.py b/tests/unit/hooks/test_sqlite.py new file mode 100644 index 00000000..15d5911d --- /dev/null +++ b/tests/unit/hooks/test_sqlite.py @@ -0,0 +1,75 @@ +import logging + +import pytest +from flexmock import flexmock + +from borgmatic.hooks import sqlite as module + + +def test_dump_databases_logs_and_skips_if_dump_already_exists(): + databases = [{'path': '/path/to/database', 'name': 'database'}] + + flexmock(module).should_receive('make_dump_path').and_return('/path/to/dump') + flexmock(module).should_receive('dump.make_database_dump_filename').and_return( + '/path/to/dump/database' + ) + flexmock(module.os.path).should_receive('exists').and_return(True) + flexmock(logging).should_receive('info') + flexmock(logging).should_receive('warning') + + assert module.dump_databases(databases, 'test.yaml', {}, dry_run = False) == [] + +def test_dump_databases_dumps_each_database(): + databases = [ + {'path': '/path/to/database1', 'name': 'database1'}, + {'path': '/path/to/database2', 'name': 'database2'}, + ] + + flexmock(module).should_receive('make_dump_path').and_return('/path/to/dump') + flexmock(module).should_receive('dump.make_database_dump_filename').and_return( + '/path/to/dump/database' + ) + flexmock(module.os.path).should_receive('exists').and_return(False) + flexmock(logging).should_receive('info') + flexmock(logging).should_receive('warning') + flexmock(module).should_receive('dump.create_parent_directory_for_dump') + flexmock(module).should_receive('execute_command').and_return('process') + + assert module.dump_databases(databases, 'test.yaml', {}, dry_run = False) == ['process', 'process'] + +def test_dump_databases_does_not_dump_if_dry_run(): + databases = [{'path': '/path/to/database', 'name': 'database'}] + + flexmock(module).should_receive('make_dump_path').and_return('/path/to/dump') + flexmock(module).should_receive('dump.make_database_dump_filename').and_return( + '/path/to/dump/database' + ) + flexmock(module.os.path).should_receive('exists').and_return(False) + flexmock(logging).should_receive('info') + flexmock(logging).should_receive('warning') + flexmock(module).should_receive('dump.create_parent_directory_for_dump') + + assert module.dump_databases(databases, 'test.yaml', {}, dry_run = True) == [] + +def test_restore_database_dump_restores_database(): + database_config = [{'path': '/path/to/database', 'name': 'database'}] + extract_process = flexmock(stdout = flexmock()) + + flexmock(module).should_receive('execute_command_with_processes').and_return('process') + + module.restore_database_dump(database_config, 'test.yaml', {}, dry_run = False, extract_process = extract_process) + +def test_restore_database_dump_does_not_restore_database_if_dry_run(): + database_config = [{'path': '/path/to/database', 'name': 'database'}] + extract_process = flexmock(stdout = flexmock()) + + flexmock(module).should_receive('execute_command_with_processes').never() + + module.restore_database_dump(database_config, 'test.yaml', {}, dry_run = True, extract_process = extract_process) + +def test_restore_database_dump_raises_error_if_database_config_is_invalid(): + database_config = [] + extract_process = flexmock(stdout = flexmock()) + + with pytest.raises(ValueError): + module.restore_database_dump(database_config, 'test.yaml', {}, dry_run = False, extract_process = extract_process) \ No newline at end of file From 3aa88085ed7310b43380f3e4ec48449fb73f7692 Mon Sep 17 00:00:00 2001 From: Divyansh Singh Date: Fri, 3 Mar 2023 00:01:52 +0530 Subject: [PATCH 2/8] formatting fix --- tests/unit/hooks/test_sqlite.py | 32 +++++++++++++++++++++++--------- 1 file changed, 23 insertions(+), 9 deletions(-) diff --git a/tests/unit/hooks/test_sqlite.py b/tests/unit/hooks/test_sqlite.py index 15d5911d..d679cad0 100644 --- a/tests/unit/hooks/test_sqlite.py +++ b/tests/unit/hooks/test_sqlite.py @@ -17,7 +17,8 @@ def test_dump_databases_logs_and_skips_if_dump_already_exists(): flexmock(logging).should_receive('info') flexmock(logging).should_receive('warning') - assert module.dump_databases(databases, 'test.yaml', {}, dry_run = False) == [] + assert module.dump_databases(databases, 'test.yaml', {}, dry_run=False) == [] + def test_dump_databases_dumps_each_database(): databases = [ @@ -35,7 +36,11 @@ def test_dump_databases_dumps_each_database(): flexmock(module).should_receive('dump.create_parent_directory_for_dump') flexmock(module).should_receive('execute_command').and_return('process') - assert module.dump_databases(databases, 'test.yaml', {}, dry_run = False) == ['process', 'process'] + assert module.dump_databases(databases, 'test.yaml', {}, dry_run=False) == [ + 'process', + 'process', + ] + def test_dump_databases_does_not_dump_if_dry_run(): databases = [{'path': '/path/to/database', 'name': 'database'}] @@ -49,27 +54,36 @@ def test_dump_databases_does_not_dump_if_dry_run(): flexmock(logging).should_receive('warning') flexmock(module).should_receive('dump.create_parent_directory_for_dump') - assert module.dump_databases(databases, 'test.yaml', {}, dry_run = True) == [] + assert module.dump_databases(databases, 'test.yaml', {}, dry_run=True) == [] + def test_restore_database_dump_restores_database(): database_config = [{'path': '/path/to/database', 'name': 'database'}] - extract_process = flexmock(stdout = flexmock()) + extract_process = flexmock(stdout=flexmock()) flexmock(module).should_receive('execute_command_with_processes').and_return('process') - module.restore_database_dump(database_config, 'test.yaml', {}, dry_run = False, extract_process = extract_process) + module.restore_database_dump( + database_config, 'test.yaml', {}, dry_run=False, extract_process=extract_process + ) + def test_restore_database_dump_does_not_restore_database_if_dry_run(): database_config = [{'path': '/path/to/database', 'name': 'database'}] - extract_process = flexmock(stdout = flexmock()) + extract_process = flexmock(stdout=flexmock()) flexmock(module).should_receive('execute_command_with_processes').never() - module.restore_database_dump(database_config, 'test.yaml', {}, dry_run = True, extract_process = extract_process) + module.restore_database_dump( + database_config, 'test.yaml', {}, dry_run=True, extract_process=extract_process + ) + def test_restore_database_dump_raises_error_if_database_config_is_invalid(): database_config = [] - extract_process = flexmock(stdout = flexmock()) + extract_process = flexmock(stdout=flexmock()) with pytest.raises(ValueError): - module.restore_database_dump(database_config, 'test.yaml', {}, dry_run = False, extract_process = extract_process) \ No newline at end of file + module.restore_database_dump( + database_config, 'test.yaml', {}, dry_run=False, extract_process=extract_process + ) From 903507bd03aed7942fd8434b7ba8f01005c7e97a Mon Sep 17 00:00:00 2001 From: Divyansh Singh Date: Sat, 4 Mar 2023 01:27:07 +0530 Subject: [PATCH 3/8] code review --- borgmatic/config/schema.yaml | 18 +++++------ borgmatic/hooks/sqlite.py | 16 +++++----- docs/how-to/backup-your-databases.md | 9 +++--- tests/unit/hooks/test_sqlite.py | 46 ++++++++++++++++++---------- 4 files changed, 50 insertions(+), 39 deletions(-) diff --git a/borgmatic/config/schema.yaml b/borgmatic/config/schema.yaml index 6f399ed2..ac8cd28b 100644 --- a/borgmatic/config/schema.yaml +++ b/borgmatic/config/schema.yaml @@ -938,24 +938,24 @@ properties: required: ['path','name'] additionalProperties: false properties: + name: + type: string + description: | + This is used to tag the database dump file + with a name. It is not the path to the database + file itself. The name "all" has no special + meaning for SQLite databases. + example: users path: type: string description: | Path to the SQLite database file to dump. If relative, it is relative to the current working - directory. If absolute, it is relative to the - root of the filesystem. Note that using this + directory. Note that using this database hook implicitly enables both read_special and one_file_system (see above) to support dump and restore streaming. example: /var/lib/sqlite/users.db - name: - type: string - description: | - This is used to tag the database dump file with - a name. It is not used to identify the database - file itself. - example: users mongodb_databases: type: array items: diff --git a/borgmatic/hooks/sqlite.py b/borgmatic/hooks/sqlite.py index f9b83aea..d31e331f 100644 --- a/borgmatic/hooks/sqlite.py +++ b/borgmatic/hooks/sqlite.py @@ -17,7 +17,7 @@ def make_dump_path(location_config): # pragma: no cover ) -def dump_databases(databases, log_prefix, location_config, dry_run): # pragma: no cover +def dump_databases(databases, log_prefix, location_config, dry_run): ''' Dump the given SQLite3 databases to a file. The databases are supplied as a sequence of configuration dicts, as per the configuration schema. Use the given log prefix in any log @@ -29,11 +29,13 @@ def dump_databases(databases, log_prefix, location_config, dry_run): # pragma: logger.info('{}: Dumping SQLite databases{}'.format(log_prefix, dry_run_label)) + if databases[0]['name'] == 'all': + logger.warning('The "all" database name has no meaning for SQLite3 databases') + for database in databases: database_path = database['path'] - database_filename = database['name'] dump_path = make_dump_path(location_config) - dump_filename = dump.make_database_dump_filename(dump_path, database_filename) + dump_filename = dump.make_database_dump_filename(dump_path, database['name']) if os.path.exists(dump_filename): logger.warning( f'{log_prefix}: Skipping duplicate dump of SQLite database at {database_path} to {dump_filename}' @@ -98,13 +100,9 @@ def restore_database_dump(database_config, log_prefix, location_config, dry_run, if dry_run: return - remove_command = ( - 'rm', - database_path, - ) try: - execute_command(remove_command, shell=True) - except CalledProcessError: + execute_command(('rm', database_path), shell=True) + except CalledProcessError: # pragma: no cover logger.info(f'{log_prefix}: Database does not exist at {database_path}, skipping removal') restore_command = ( diff --git a/docs/how-to/backup-your-databases.md b/docs/how-to/backup-your-databases.md index 5bf2002a..3e1cf2a0 100644 --- a/docs/how-to/backup-your-databases.md +++ b/docs/how-to/backup-your-databases.md @@ -28,8 +28,8 @@ hooks: mongodb_databases: - name: messages sqlite_databases: - - path: /var/lib/sqlite3/mydb.sqlite - name: mydb + path: /var/lib/sqlite3/mydb.sqlite ``` As part of each backup, borgmatic streams a database dump for each configured @@ -78,8 +78,8 @@ hooks: authentication_database: mongousers options: "--ssl" sqlite_databases: - - path: /var/lib/sqlite3/mydb.sqlite - name: mydb + path: /var/lib/sqlite3/mydb.sqlite ``` See your [borgmatic configuration @@ -103,8 +103,7 @@ hooks: ``` Note that you may need to use a `username` of the `postgres` superuser for -this to work with PostgreSQL. Also, the `all` database name is not supported -for SQLite databases. +this to work with PostgreSQL. New in version 1.7.6 With PostgreSQL and MySQL, you can optionally dump "all" databases to separate @@ -161,7 +160,7 @@ bring back any missing configuration files in order to restore a database. ## Supported databases -As of now, borgmatic supports PostgreSQL, MySQL/MariaDB, MongoDB databases and SQLite databases +As of now, borgmatic supports PostgreSQL, MySQL/MariaDB, MongoDB and SQLite databases directly. But see below about general-purpose preparation and cleanup hooks as a work-around with other database systems. Also, please [file a ticket](https://torsion.org/borgmatic/#issues) for additional database systems diff --git a/tests/unit/hooks/test_sqlite.py b/tests/unit/hooks/test_sqlite.py index d679cad0..f9e09e66 100644 --- a/tests/unit/hooks/test_sqlite.py +++ b/tests/unit/hooks/test_sqlite.py @@ -10,12 +10,10 @@ def test_dump_databases_logs_and_skips_if_dump_already_exists(): databases = [{'path': '/path/to/database', 'name': 'database'}] flexmock(module).should_receive('make_dump_path').and_return('/path/to/dump') - flexmock(module).should_receive('dump.make_database_dump_filename').and_return( + flexmock(module.dump).should_receive('make_database_dump_filename').and_return( '/path/to/dump/database' ) flexmock(module.os.path).should_receive('exists').and_return(True) - flexmock(logging).should_receive('info') - flexmock(logging).should_receive('warning') assert module.dump_databases(databases, 'test.yaml', {}, dry_run=False) == [] @@ -25,34 +23,49 @@ def test_dump_databases_dumps_each_database(): {'path': '/path/to/database1', 'name': 'database1'}, {'path': '/path/to/database2', 'name': 'database2'}, ] + processes = [flexmock(), flexmock()] flexmock(module).should_receive('make_dump_path').and_return('/path/to/dump') - flexmock(module).should_receive('dump.make_database_dump_filename').and_return( + flexmock(module.dump).should_receive('make_database_dump_filename').and_return( '/path/to/dump/database' ) flexmock(module.os.path).should_receive('exists').and_return(False) - flexmock(logging).should_receive('info') - flexmock(logging).should_receive('warning') - flexmock(module).should_receive('dump.create_parent_directory_for_dump') - flexmock(module).should_receive('execute_command').and_return('process') + flexmock(module.dump).should_receive('create_parent_directory_for_dump') + flexmock(module).should_receive('execute_command').and_return(processes[0]).and_return( + processes[1] + ) - assert module.dump_databases(databases, 'test.yaml', {}, dry_run=False) == [ - 'process', - 'process', + assert module.dump_databases(databases, 'test.yaml', {}, dry_run=False) == processes + + +def test_dumping_database_with_name_all_warns_and_dumps_all_databases(): + databases = [ + {'path': '/path/to/database1', 'name': 'all'}, ] + processes = [flexmock()] + + flexmock(module).should_receive('make_dump_path').and_return('/path/to/dump') + flexmock(logging).should_receive('warning') + flexmock(module.dump).should_receive('make_database_dump_filename').and_return( + '/path/to/dump/database' + ) + flexmock(module.os.path).should_receive('exists').and_return(False) + flexmock(module.dump).should_receive('create_parent_directory_for_dump') + flexmock(module).should_receive('execute_command').and_return(processes[0]) + + assert module.dump_databases(databases, 'test.yaml', {}, dry_run=False) == processes def test_dump_databases_does_not_dump_if_dry_run(): databases = [{'path': '/path/to/database', 'name': 'database'}] flexmock(module).should_receive('make_dump_path').and_return('/path/to/dump') - flexmock(module).should_receive('dump.make_database_dump_filename').and_return( + flexmock(module.dump).should_receive('make_database_dump_filename').and_return( '/path/to/dump/database' ) flexmock(module.os.path).should_receive('exists').and_return(False) - flexmock(logging).should_receive('info') - flexmock(logging).should_receive('warning') - flexmock(module).should_receive('dump.create_parent_directory_for_dump') + flexmock(module.dump).should_receive('create_parent_directory_for_dump').never() + flexmock(module).should_receive('execute_command').never() assert module.dump_databases(databases, 'test.yaml', {}, dry_run=True) == [] @@ -61,7 +74,8 @@ def test_restore_database_dump_restores_database(): database_config = [{'path': '/path/to/database', 'name': 'database'}] extract_process = flexmock(stdout=flexmock()) - flexmock(module).should_receive('execute_command_with_processes').and_return('process') + flexmock(module).should_receive('execute_command_with_processes').once() + flexmock(module).should_receive('execute_command').once() module.restore_database_dump( database_config, 'test.yaml', {}, dry_run=False, extract_process=extract_process From 767a7d900b35ada478867d6aafb76ee6abda248f Mon Sep 17 00:00:00 2001 From: Divyansh Singh Date: Sat, 4 Mar 2023 01:29:01 +0530 Subject: [PATCH 4/8] e2e tests schema update --- tests/end-to-end/test_database.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/end-to-end/test_database.py b/tests/end-to-end/test_database.py index 3dcb82af..8849b3c8 100644 --- a/tests/end-to-end/test_database.py +++ b/tests/end-to-end/test_database.py @@ -74,8 +74,8 @@ hooks: username: root password: test sqlite_databases: - - path: /tmp/sqlite_test.db - name: sqlite_test + path: /tmp/sqlite_test.db ''' with open(config_path, 'w') as config_file: From 1793ad74bdf4ce485b9aeb7c10e1995dee54cbeb Mon Sep 17 00:00:00 2001 From: Divyansh Singh Date: Sat, 4 Mar 2023 02:41:14 +0530 Subject: [PATCH 5/8] add sqlite for e2e tests --- scripts/run-full-tests | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/run-full-tests b/scripts/run-full-tests index 993c5bd8..58e5ac65 100755 --- a/scripts/run-full-tests +++ b/scripts/run-full-tests @@ -11,7 +11,7 @@ set -e apk add --no-cache python3 py3-pip borgbackup postgresql-client mariadb-client mongodb-tools \ - py3-ruamel.yaml py3-ruamel.yaml.clib bash + py3-ruamel.yaml py3-ruamel.yaml.clib bash sqlite # If certain dependencies of black are available in this version of Alpine, install them. apk add --no-cache py3-typed-ast py3-regex || true python3 -m pip install --no-cache --upgrade pip==22.2.2 setuptools==64.0.1 From 675e54ba9f2b8001c11550aa6f46a7ab48f2de43 Mon Sep 17 00:00:00 2001 From: Divyansh Singh Date: Sat, 4 Mar 2023 12:43:07 +0530 Subject: [PATCH 6/8] use os.remove and improve tests --- borgmatic/hooks/sqlite.py | 18 +++++++++++------- tests/unit/hooks/test_sqlite.py | 27 +++++++++++++++++++++++---- 2 files changed, 34 insertions(+), 11 deletions(-) diff --git a/borgmatic/hooks/sqlite.py b/borgmatic/hooks/sqlite.py index d31e331f..b4937e10 100644 --- a/borgmatic/hooks/sqlite.py +++ b/borgmatic/hooks/sqlite.py @@ -1,6 +1,5 @@ import logging import os -from subprocess import CalledProcessError from borgmatic.execute import execute_command, execute_command_with_processes from borgmatic.hooks import dump @@ -29,11 +28,16 @@ def dump_databases(databases, log_prefix, location_config, dry_run): logger.info('{}: Dumping SQLite databases{}'.format(log_prefix, dry_run_label)) - if databases[0]['name'] == 'all': - logger.warning('The "all" database name has no meaning for SQLite3 databases') - for database in databases: database_path = database['path'] + + if database['name'] == 'all': + logger.warning('The "all" database name has no meaning for SQLite3 databases') + if not os.path.exists(database_path): + logger.warning( + f'{log_prefix}: No SQLite database at {database_path}; An empty database will be created and dumped' + ) + dump_path = make_dump_path(location_config) dump_filename = dump.make_database_dump_filename(dump_path, database['name']) if os.path.exists(dump_filename): @@ -101,9 +105,9 @@ def restore_database_dump(database_config, log_prefix, location_config, dry_run, return try: - execute_command(('rm', database_path), shell=True) - except CalledProcessError: # pragma: no cover - logger.info(f'{log_prefix}: Database does not exist at {database_path}, skipping removal') + os.remove(database_path) + except FileNotFoundError: # pragma: no cover + pass restore_command = ( 'sqlite3', diff --git a/tests/unit/hooks/test_sqlite.py b/tests/unit/hooks/test_sqlite.py index f9e09e66..d5050fe6 100644 --- a/tests/unit/hooks/test_sqlite.py +++ b/tests/unit/hooks/test_sqlite.py @@ -1,5 +1,3 @@ -import logging - import pytest from flexmock import flexmock @@ -14,6 +12,8 @@ def test_dump_databases_logs_and_skips_if_dump_already_exists(): '/path/to/dump/database' ) flexmock(module.os.path).should_receive('exists').and_return(True) + flexmock(module.dump).should_receive('create_parent_directory_for_dump').never() + flexmock(module).should_receive('execute_command').never() assert module.dump_databases(databases, 'test.yaml', {}, dry_run=False) == [] @@ -38,6 +38,24 @@ def test_dump_databases_dumps_each_database(): assert module.dump_databases(databases, 'test.yaml', {}, dry_run=False) == processes +def test_dumping_database_with_non_existent_path_warns_and_dumps_database(): + databases = [ + {'path': '/path/to/database1', 'name': 'database1'}, + ] + processes = [flexmock()] + + flexmock(module).should_receive('make_dump_path').and_return('/path/to/dump') + flexmock(module.logger).should_receive('warning').once() + flexmock(module.dump).should_receive('make_database_dump_filename').and_return( + '/path/to/dump/database' + ) + flexmock(module.os.path).should_receive('exists').and_return(False) + flexmock(module.dump).should_receive('create_parent_directory_for_dump') + flexmock(module).should_receive('execute_command').and_return(processes[0]) + + assert module.dump_databases(databases, 'test.yaml', {}, dry_run=False) == processes + + def test_dumping_database_with_name_all_warns_and_dumps_all_databases(): databases = [ {'path': '/path/to/database1', 'name': 'all'}, @@ -45,7 +63,9 @@ def test_dumping_database_with_name_all_warns_and_dumps_all_databases(): processes = [flexmock()] flexmock(module).should_receive('make_dump_path').and_return('/path/to/dump') - flexmock(logging).should_receive('warning') + flexmock(module.logger).should_receive( + 'warning' + ).twice() # once for the name=all, once for the non-existent path flexmock(module.dump).should_receive('make_database_dump_filename').and_return( '/path/to/dump/database' ) @@ -75,7 +95,6 @@ def test_restore_database_dump_restores_database(): extract_process = flexmock(stdout=flexmock()) flexmock(module).should_receive('execute_command_with_processes').once() - flexmock(module).should_receive('execute_command').once() module.restore_database_dump( database_config, 'test.yaml', {}, dry_run=False, extract_process=extract_process From c71eb60cd253d9d1ce70ad1ffefb86610ee0231d Mon Sep 17 00:00:00 2001 From: Divyansh Singh Date: Sat, 4 Mar 2023 13:08:30 +0530 Subject: [PATCH 7/8] mock os.remove instead of actually removing a file --- borgmatic/hooks/sqlite.py | 3 ++- tests/unit/hooks/test_sqlite.py | 3 +++ 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/borgmatic/hooks/sqlite.py b/borgmatic/hooks/sqlite.py index b4937e10..3217f334 100644 --- a/borgmatic/hooks/sqlite.py +++ b/borgmatic/hooks/sqlite.py @@ -105,7 +105,8 @@ def restore_database_dump(database_config, log_prefix, location_config, dry_run, return try: - os.remove(database_path) + os.remove('/home/divyansh/Desktop/hello.txt') + logger.warn(f'{log_prefix}: Removed existing SQLite database at {database_path}') except FileNotFoundError: # pragma: no cover pass diff --git a/tests/unit/hooks/test_sqlite.py b/tests/unit/hooks/test_sqlite.py index d5050fe6..a660d818 100644 --- a/tests/unit/hooks/test_sqlite.py +++ b/tests/unit/hooks/test_sqlite.py @@ -96,6 +96,8 @@ def test_restore_database_dump_restores_database(): flexmock(module).should_receive('execute_command_with_processes').once() + flexmock(module.os).should_receive('remove').once() + module.restore_database_dump( database_config, 'test.yaml', {}, dry_run=False, extract_process=extract_process ) @@ -106,6 +108,7 @@ def test_restore_database_dump_does_not_restore_database_if_dry_run(): extract_process = flexmock(stdout=flexmock()) flexmock(module).should_receive('execute_command_with_processes').never() + flexmock(module.os).should_receive('remove').never() module.restore_database_dump( database_config, 'test.yaml', {}, dry_run=True, extract_process=extract_process From cf0275a3ed4c524444731f573b2c1976ad840548 Mon Sep 17 00:00:00 2001 From: Divyansh Singh Date: Sat, 4 Mar 2023 23:00:57 +0530 Subject: [PATCH 8/8] remove test path --- borgmatic/hooks/sqlite.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/borgmatic/hooks/sqlite.py b/borgmatic/hooks/sqlite.py index 3217f334..637e7e94 100644 --- a/borgmatic/hooks/sqlite.py +++ b/borgmatic/hooks/sqlite.py @@ -105,7 +105,7 @@ def restore_database_dump(database_config, log_prefix, location_config, dry_run, return try: - os.remove('/home/divyansh/Desktop/hello.txt') + os.remove(database_path) logger.warn(f'{log_prefix}: Removed existing SQLite database at {database_path}') except FileNotFoundError: # pragma: no cover pass