From da321e180d9e7cab15a6756e78a311c2341d158a Mon Sep 17 00:00:00 2001 From: Dan Helfman Date: Sun, 26 Feb 2023 22:15:12 -0800 Subject: [PATCH] Fix the "create" action with the "--dry-run" flag querying for databases when a PostgreSQL/MySQL "all" database is configured. --- NEWS | 2 ++ borgmatic/hooks/mysql.py | 14 ++++++---- borgmatic/hooks/postgresql.py | 13 ++++++---- tests/unit/hooks/test_mysql.py | 32 ++++++++++++++++++----- tests/unit/hooks/test_postgresql.py | 40 +++++++++++++++++++++++------ 5 files changed, 77 insertions(+), 24 deletions(-) diff --git a/NEWS b/NEWS index e197767..46042ab 100644 --- a/NEWS +++ b/NEWS @@ -4,6 +4,8 @@ This lines up with the new behavior in Borg 2.0.0b5. * Internally support new Borg 2.0.0b5 "--filter" status characters / item flags for the "create" action. + * Fix the "create" action with the "--dry-run" flag querying for databases when a PostgreSQL/MySQL + "all" database is configured. Now, these queries are skipped due to the dry run. 1.7.7 * #642: Add MySQL database hook "add_drop_database" configuration option to control whether dumped diff --git a/borgmatic/hooks/mysql.py b/borgmatic/hooks/mysql.py index ef2c336..e53b896 100644 --- a/borgmatic/hooks/mysql.py +++ b/borgmatic/hooks/mysql.py @@ -24,7 +24,7 @@ def make_dump_path(location_config): # pragma: no cover SYSTEM_DATABASE_NAMES = ('information_schema', 'mysql', 'performance_schema', 'sys') -def database_names_to_dump(database, extra_environment, log_prefix, dry_run_label): +def database_names_to_dump(database, extra_environment, log_prefix, dry_run): ''' Given a requested database config, return the corresponding sequence of database names to dump. In the case of "all", query for the names of databases on the configured host and return them, @@ -32,6 +32,8 @@ def database_names_to_dump(database, extra_environment, log_prefix, dry_run_labe ''' if database['name'] != 'all': return (database['name'],) + if dry_run: + return () show_command = ( ('mysql',) @@ -43,9 +45,7 @@ def database_names_to_dump(database, extra_environment, log_prefix, dry_run_labe + ('--skip-column-names', '--batch') + ('--execute', 'show schemas') ) - logger.debug( - '{}: Querying for "all" MySQL databases to dump{}'.format(log_prefix, dry_run_label) - ) + logger.debug(f'{log_prefix}: Querying for "all" MySQL databases to dump') show_output = execute_command_and_capture_output( show_command, extra_environment=extra_environment ) @@ -125,9 +125,13 @@ def dump_databases(databases, log_prefix, location_config, dry_run): dump_path = make_dump_path(location_config) extra_environment = {'MYSQL_PWD': database['password']} if 'password' in database else None dump_database_names = database_names_to_dump( - database, extra_environment, log_prefix, dry_run_label + database, extra_environment, log_prefix, dry_run ) + if not dump_database_names: + if dry_run: + continue + raise ValueError('Cannot find any MySQL databases to dump.') if database['name'] == 'all' and database.get('format'): diff --git a/borgmatic/hooks/postgresql.py b/borgmatic/hooks/postgresql.py index 98c1108..3d3676f 100644 --- a/borgmatic/hooks/postgresql.py +++ b/borgmatic/hooks/postgresql.py @@ -43,7 +43,7 @@ def make_extra_environment(database): EXCLUDED_DATABASE_NAMES = ('template0', 'template1') -def database_names_to_dump(database, extra_environment, log_prefix, dry_run_label): +def database_names_to_dump(database, extra_environment, log_prefix, dry_run): ''' Given a requested database config, return the corresponding sequence of database names to dump. In the case of "all" when a database format is given, query for the names of databases on the @@ -56,6 +56,8 @@ def database_names_to_dump(database, extra_environment, log_prefix, dry_run_labe return (requested_name,) if not database.get('format'): return ('all',) + if dry_run: + return () list_command = ( ('psql', '--list', '--no-password', '--csv', '--tuples-only') @@ -64,9 +66,7 @@ def database_names_to_dump(database, extra_environment, log_prefix, dry_run_labe + (('--username', database['username']) if 'username' in database else ()) + (tuple(database['list_options'].split(' ')) if 'list_options' in database else ()) ) - logger.debug( - '{}: Querying for "all" PostgreSQL databases to dump{}'.format(log_prefix, dry_run_label) - ) + logger.debug(f'{log_prefix}: Querying for "all" PostgreSQL databases to dump') list_output = execute_command_and_capture_output( list_command, extra_environment=extra_environment ) @@ -99,10 +99,13 @@ def dump_databases(databases, log_prefix, location_config, dry_run): extra_environment = make_extra_environment(database) dump_path = make_dump_path(location_config) dump_database_names = database_names_to_dump( - database, extra_environment, log_prefix, dry_run_label + database, extra_environment, log_prefix, dry_run ) if not dump_database_names: + if dry_run: + continue + raise ValueError('Cannot find any PostgreSQL databases to dump.') for database_name in dump_database_names: diff --git a/tests/unit/hooks/test_mysql.py b/tests/unit/hooks/test_mysql.py index 3befab7..1e8df69 100644 --- a/tests/unit/hooks/test_mysql.py +++ b/tests/unit/hooks/test_mysql.py @@ -9,26 +9,36 @@ from borgmatic.hooks import mysql as module def test_database_names_to_dump_passes_through_name(): extra_environment = flexmock() log_prefix = '' - dry_run_label = '' names = module.database_names_to_dump( - {'name': 'foo'}, extra_environment, log_prefix, dry_run_label + {'name': 'foo'}, extra_environment, log_prefix, dry_run=False ) assert names == ('foo',) +def test_database_names_to_dump_bails_for_dry_run(): + extra_environment = flexmock() + log_prefix = '' + flexmock(module).should_receive('execute_command_and_capture_output').never() + + names = module.database_names_to_dump( + {'name': 'all'}, extra_environment, log_prefix, dry_run=True + ) + + assert names == () + + def test_database_names_to_dump_queries_mysql_for_database_names(): extra_environment = flexmock() log_prefix = '' - dry_run_label = '' flexmock(module).should_receive('execute_command_and_capture_output').with_args( ('mysql', '--skip-column-names', '--batch', '--execute', 'show schemas'), extra_environment=extra_environment, ).and_return('foo\nbar\nmysql\n').once() names = module.database_names_to_dump( - {'name': 'all'}, extra_environment, log_prefix, dry_run_label + {'name': 'all'}, extra_environment, log_prefix, dry_run=False ) assert names == ('foo', 'bar') @@ -323,7 +333,6 @@ def test_execute_dump_command_with_dry_run_skips_mysqldump(): def test_dump_databases_errors_for_missing_all_databases(): databases = [{'name': 'all'}] - process = flexmock() flexmock(module).should_receive('make_dump_path').and_return('') flexmock(module.dump).should_receive('make_database_dump_filename').and_return( 'databases/localhost/all' @@ -331,7 +340,18 @@ def test_dump_databases_errors_for_missing_all_databases(): flexmock(module).should_receive('database_names_to_dump').and_return(()) with pytest.raises(ValueError): - assert module.dump_databases(databases, 'test.yaml', {}, dry_run=False) == [process] + assert module.dump_databases(databases, 'test.yaml', {}, dry_run=False) + + +def test_dump_databases_does_not_error_for_missing_all_databases_with_dry_run(): + databases = [{'name': 'all'}] + flexmock(module).should_receive('make_dump_path').and_return('') + flexmock(module.dump).should_receive('make_database_dump_filename').and_return( + 'databases/localhost/all' + ) + flexmock(module).should_receive('database_names_to_dump').and_return(()) + + assert module.dump_databases(databases, 'test.yaml', {}, dry_run=True) == [] def test_restore_database_dump_runs_mysql_to_restore(): diff --git a/tests/unit/hooks/test_postgresql.py b/tests/unit/hooks/test_postgresql.py index 628e8c0..9cb4c0f 100644 --- a/tests/unit/hooks/test_postgresql.py +++ b/tests/unit/hooks/test_postgresql.py @@ -9,19 +9,32 @@ from borgmatic.hooks import postgresql as module def test_database_names_to_dump_passes_through_individual_database_name(): database = {'name': 'foo'} - assert module.database_names_to_dump(database, flexmock(), flexmock(), flexmock()) == ('foo',) + assert module.database_names_to_dump(database, flexmock(), flexmock(), dry_run=False) == ( + 'foo', + ) def test_database_names_to_dump_passes_through_individual_database_name_with_format(): database = {'name': 'foo', 'format': 'custom'} - assert module.database_names_to_dump(database, flexmock(), flexmock(), flexmock()) == ('foo',) + assert module.database_names_to_dump(database, flexmock(), flexmock(), dry_run=False) == ( + 'foo', + ) def test_database_names_to_dump_passes_through_all_without_format(): database = {'name': 'all'} - assert module.database_names_to_dump(database, flexmock(), flexmock(), flexmock()) == ('all',) + assert module.database_names_to_dump(database, flexmock(), flexmock(), dry_run=False) == ( + 'all', + ) + + +def test_database_names_to_dump_with_all_and_format_and_dry_run_bails(): + database = {'name': 'all', 'format': 'custom'} + flexmock(module).should_receive('execute_command_and_capture_output').never() + + assert module.database_names_to_dump(database, flexmock(), flexmock(), dry_run=True) == () def test_database_names_to_dump_with_all_and_format_lists_databases(): @@ -30,7 +43,7 @@ def test_database_names_to_dump_with_all_and_format_lists_databases(): 'foo,test,\nbar,test,"stuff and such"' ) - assert module.database_names_to_dump(database, flexmock(), flexmock(), flexmock()) == ( + assert module.database_names_to_dump(database, flexmock(), flexmock(), dry_run=False) == ( 'foo', 'bar', ) @@ -53,7 +66,7 @@ def test_database_names_to_dump_with_all_and_format_lists_databases_with_hostnam extra_environment=object, ).and_return('foo,test,\nbar,test,"stuff and such"') - assert module.database_names_to_dump(database, flexmock(), flexmock(), flexmock()) == ( + assert module.database_names_to_dump(database, flexmock(), flexmock(), dry_run=False) == ( 'foo', 'bar', ) @@ -66,7 +79,7 @@ def test_database_names_to_dump_with_all_and_format_lists_databases_with_usernam extra_environment=object, ).and_return('foo,test,\nbar,test,"stuff and such"') - assert module.database_names_to_dump(database, flexmock(), flexmock(), flexmock()) == ( + assert module.database_names_to_dump(database, flexmock(), flexmock(), dry_run=False) == ( 'foo', 'bar', ) @@ -79,7 +92,7 @@ def test_database_names_to_dump_with_all_and_format_lists_databases_with_options extra_environment=object, ).and_return('foo,test,\nbar,test,"stuff and such"') - assert module.database_names_to_dump(database, flexmock(), flexmock(), flexmock()) == ( + assert module.database_names_to_dump(database, flexmock(), flexmock(), dry_run=False) == ( 'foo', 'bar', ) @@ -91,7 +104,9 @@ def test_database_names_to_dump_with_all_and_format_excludes_particular_database 'foo,test,\ntemplate0,test,blah' ) - assert module.database_names_to_dump(database, flexmock(), flexmock(), flexmock()) == ('foo',) + assert module.database_names_to_dump(database, flexmock(), flexmock(), dry_run=False) == ( + 'foo', + ) def test_dump_databases_runs_pg_dump_for_each_database(): @@ -139,6 +154,15 @@ def test_dump_databases_raises_when_no_database_names_to_dump(): module.dump_databases(databases, 'test.yaml', {}, dry_run=False) +def test_dump_databases_does_not_raise_when_no_database_names_to_dump(): + databases = [{'name': 'foo'}, {'name': 'bar'}] + flexmock(module).should_receive('make_extra_environment').and_return({'PGSSLMODE': 'disable'}) + flexmock(module).should_receive('make_dump_path').and_return('') + flexmock(module).should_receive('database_names_to_dump').and_return(()) + + module.dump_databases(databases, 'test.yaml', {}, dry_run=True) == [] + + def test_dump_databases_with_duplicate_dump_skips_pg_dump(): databases = [{'name': 'foo'}, {'name': 'bar'}] flexmock(module).should_receive('make_extra_environment').and_return({'PGSSLMODE': 'disable'})