fix(utils): narrow bare except Exception in merge_json_files (#4189)
merge_json_files's read of the existing JSON file caught bare
`Exception` around `json5.load`, so a real bug there (e.g. a
`TypeError`/`AttributeError`) was silently treated the same as a normal
parse failure -- `None` returned, existing settings preserved untouched,
nothing surfaced unless `verbose`. Only `OSError` (inaccessible file) and
`ValueError` (malformed JSON5 -- json5's decode error is a `ValueError`
subclass) are expected outcomes here; anything else should propagate.
Same bug, same fix shape, as the caller `handle_vscode_settings`, whose
own bare `except Exception` was just narrowed to `(OSError, ValueError,
KeyError)` in commit 16f4577 (PR #3844) with the same rationale
("let programming errors like TypeError or AttributeError propagate").
That PR's own regression test monkeypatched `merge_json_files` to prove
the caller's narrowing works; this fixes and tests the callee itself,
which still had the original bare-except bug.
Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
This commit is contained in:
@@ -249,7 +249,7 @@ def merge_json_files(existing_path: Path, new_content: Any, verbose: bool = Fals
|
||||
except FileNotFoundError:
|
||||
# Handle race condition where file is deleted after exists() check
|
||||
exists = False
|
||||
except Exception as e:
|
||||
except (OSError, ValueError) as e:
|
||||
if verbose:
|
||||
console.print(f"[yellow]Warning: Could not read or parse existing JSON in {existing_path.name} ({e}).[/yellow]")
|
||||
# Skip merge to preserve existing file if unparseable or inaccessible (e.g. PermissionError)
|
||||
|
||||
@@ -212,3 +212,29 @@ def test_handle_vscode_settings_propagates_programming_errors(tmp_path):
|
||||
)
|
||||
finally:
|
||||
utils_mod.merge_json_files = original_merge
|
||||
|
||||
|
||||
def test_merge_json_files_propagates_programming_errors(tmp_path, monkeypatch):
|
||||
"""Unexpected programming errors reading the existing file must propagate.
|
||||
|
||||
``merge_json_files``'s own read of the existing JSON file caught bare
|
||||
``Exception`` around ``json5.load``, so a real bug there (e.g. a
|
||||
``TypeError``) was silently treated the same as a normal parse failure --
|
||||
``None`` returned, existing settings preserved, nothing logged unless
|
||||
``verbose``. Only ``OSError`` (inaccessible file) and ``ValueError``
|
||||
(malformed JSON5 -- json5's decode error is a ``ValueError`` subclass)
|
||||
are expected outcomes here; anything else must propagate, matching the
|
||||
narrowing already applied to the caller, ``handle_vscode_settings``.
|
||||
"""
|
||||
existing_file = tmp_path / "settings.json"
|
||||
existing_file.write_text('{"a": 1}\n', encoding="utf-8")
|
||||
|
||||
import specify_cli._utils as utils_mod
|
||||
|
||||
def _boom(*_a, **_kw):
|
||||
raise TypeError("boom")
|
||||
|
||||
monkeypatch.setattr(utils_mod.json5, "load", _boom)
|
||||
|
||||
with pytest.raises(TypeError):
|
||||
merge_json_files(existing_file, {"b": 2})
|
||||
|
||||
Reference in New Issue
Block a user