From beb899d6fbf0caa64f8548075351e544c8697217 Mon Sep 17 00:00:00 2001
From: Dan Helfman <witten@torsion.org>
Date: Sat, 10 Jun 2023 15:50:11 -0700
Subject: [PATCH] Make user-facing manifest loading error messages a little
 friendlier (#697).

---
 borgmatic/actions/config/bootstrap.py       | 24 ++++++-
 borgmatic/commands/borgmatic.py             |  4 +-
 tests/unit/actions/config/test_bootstrap.py | 71 +++++++++++++++++++++
 3 files changed, 94 insertions(+), 5 deletions(-)

diff --git a/borgmatic/actions/config/bootstrap.py b/borgmatic/actions/config/bootstrap.py
index 9254da6d..3e4eb150 100644
--- a/borgmatic/actions/config/bootstrap.py
+++ b/borgmatic/actions/config/bootstrap.py
@@ -21,6 +21,9 @@ def get_config_paths(bootstrap_arguments, global_arguments, local_borg_version):
     Return:
     The config paths from the manifest.json file in the borgmatic source directory after extracting it from the
     repository.
+
+    Raise ValueError if the manifest JSON is missing, can't be decoded, or doesn't contain the
+    expected configuration path data.
     '''
     borgmatic_source_directory = (
         bootstrap_arguments.borgmatic_source_directory or DEFAULT_BORGMATIC_SOURCE_DIRECTORY
@@ -46,14 +49,31 @@ def get_config_paths(bootstrap_arguments, global_arguments, local_borg_version):
         extract_to_stdout=True,
     )
 
-    manifest_data = json.loads(extract_process.stdout.read())
+    manifest_json = extract_process.stdout.read()
+    if not manifest_json:
+        raise ValueError(
+            'Cannot read configuration paths from archive due to missing bootstrap manifest'
+        )
 
-    return manifest_data['config_paths']
+    try:
+        manifest_data = json.loads(manifest_json)
+    except json.JSONDecodeError as error:
+        raise ValueError(
+            f'Cannot read configuration paths from archive due to invalid bootstrap manifest JSON: {error}'
+        )
+
+    try:
+        return manifest_data['config_paths']
+    except KeyError:
+        raise ValueError('Cannot read configuration paths from archive due to invalid bootstrap manifest')
 
 
 def run_bootstrap(bootstrap_arguments, global_arguments, local_borg_version):
     '''
     Run the "bootstrap" action for the given repository.
+
+    Raise ValueError if the bootstrap configuration could not be loaded.
+    Raise CalledProcessError or OSError if Borg could not be run.
     '''
     manifest_config_paths = get_config_paths(
         bootstrap_arguments, global_arguments, local_borg_version
diff --git a/borgmatic/commands/borgmatic.py b/borgmatic/commands/borgmatic.py
index 69b7e8dd..6523ebbc 100644
--- a/borgmatic/commands/borgmatic.py
+++ b/borgmatic/commands/borgmatic.py
@@ -647,10 +647,8 @@ def collect_configuration_run_summary_logs(configs, arguments):
             CalledProcessError,
             ValueError,
             OSError,
-            json.JSONDecodeError,
-            KeyError,
         ) as error:
-            yield from log_error_records('Error running bootstrap', error)
+            yield from log_error_records(error)
 
         return
 
diff --git a/tests/unit/actions/config/test_bootstrap.py b/tests/unit/actions/config/test_bootstrap.py
index d1b21511..8c2063a5 100644
--- a/tests/unit/actions/config/test_bootstrap.py
+++ b/tests/unit/actions/config/test_bootstrap.py
@@ -1,3 +1,4 @@
+import pytest
 from flexmock import flexmock
 
 from borgmatic.actions.config import bootstrap as module
@@ -29,6 +30,76 @@ def test_get_config_paths_returns_list_of_config_paths():
     ]
 
 
+def test_get_config_paths_with_missing_manifest_raises_value_error():
+    bootstrap_arguments = flexmock(
+        borgmatic_source_directory=None,
+        repository='repo',
+        archive='archive',
+    )
+    global_arguments = flexmock(
+        dry_run=False,
+    )
+    local_borg_version = flexmock()
+    extract_process = flexmock(stdout=flexmock(read=lambda: ''))
+    flexmock(module.borgmatic.borg.extract).should_receive('extract_archive').and_return(
+        extract_process
+    )
+    flexmock(module.borgmatic.borg.rlist).should_receive('resolve_archive_name').and_return(
+        'archive'
+    )
+
+    with pytest.raises(ValueError):
+        module.get_config_paths(bootstrap_arguments, global_arguments, local_borg_version)
+
+
+def test_get_config_paths_with_broken_json_raises_value_error():
+    bootstrap_arguments = flexmock(
+        borgmatic_source_directory=None,
+        repository='repo',
+        archive='archive',
+    )
+    global_arguments = flexmock(
+        dry_run=False,
+    )
+    local_borg_version = flexmock()
+    extract_process = flexmock(
+        stdout=flexmock(read=lambda: '{"config_paths": ["/oops'),
+    )
+    flexmock(module.borgmatic.borg.extract).should_receive('extract_archive').and_return(
+        extract_process
+    )
+    flexmock(module.borgmatic.borg.rlist).should_receive('resolve_archive_name').and_return(
+        'archive'
+    )
+
+    with pytest.raises(ValueError):
+        module.get_config_paths(bootstrap_arguments, global_arguments, local_borg_version)
+
+
+def test_get_config_paths_with_json_missing_key_raises_value_error():
+    bootstrap_arguments = flexmock(
+        borgmatic_source_directory=None,
+        repository='repo',
+        archive='archive',
+    )
+    global_arguments = flexmock(
+        dry_run=False,
+    )
+    local_borg_version = flexmock()
+    extract_process = flexmock(
+        stdout=flexmock(read=lambda: '{}'),
+    )
+    flexmock(module.borgmatic.borg.extract).should_receive('extract_archive').and_return(
+        extract_process
+    )
+    flexmock(module.borgmatic.borg.rlist).should_receive('resolve_archive_name').and_return(
+        'archive'
+    )
+
+    with pytest.raises(ValueError):
+        module.get_config_paths(bootstrap_arguments, global_arguments, local_borg_version)
+
+
 def test_run_bootstrap_does_not_raise():
     bootstrap_arguments = flexmock(
         repository='repo',