From bf953628bb629d0c287ecb6c93407b29d0239e27 Mon Sep 17 00:00:00 2001 From: Simon Willison Date: Wed, 14 Aug 2024 14:28:48 -0700 Subject: [PATCH] Fix bug where -s could reset settings to defaults, closes #2389 --- datasette/cli.py | 5 ++++- datasette/utils/__init__.py | 9 +++++++++ tests/test_cli.py | 25 +++++++++++++++++++++++++ tests/test_utils.py | 32 ++++++++++++++++++++++++++++++++ 4 files changed, 70 insertions(+), 1 deletion(-) diff --git a/datasette/cli.py b/datasette/cli.py index 0c8a8541..0f7ce712 100644 --- a/datasette/cli.py +++ b/datasette/cli.py @@ -25,6 +25,7 @@ from .utils import ( LoadExtension, StartupError, check_connection, + deep_dict_update, find_spatialite, parse_metadata, ConnectionProblem, @@ -552,7 +553,9 @@ def serve( # Merge in settings from -s/--setting if settings: settings_updates = pairs_to_nested_config(settings) - config_data.update(settings_updates) + # Merge recursively, to avoid over-writing nested values + # https://github.com/simonw/datasette/issues/2389 + deep_dict_update(config_data, settings_updates) kwargs = dict( immutables=immutable, diff --git a/datasette/utils/__init__.py b/datasette/utils/__init__.py index c87de5f0..073d6e86 100644 --- a/datasette/utils/__init__.py +++ b/datasette/utils/__init__.py @@ -1451,3 +1451,12 @@ async def calculate_etag(filepath, chunk_size=4096): _etag_cache[filepath] = etag return etag + + +def deep_dict_update(dict1, dict2): + for key, value in dict2.items(): + if isinstance(value, dict): + dict1[key] = deep_dict_update(dict1.get(key, type(value)()), value) + else: + dict1[key] = value + return dict1 diff --git a/tests/test_cli.py b/tests/test_cli.py index d53e0a5f..bfb34c57 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -240,6 +240,31 @@ def test_setting(args): assert settings["default_page_size"] == 5 +def test_setting_compatible_with_config(tmp_path): + # https://github.com/simonw/datasette/issues/2389 + runner = CliRunner() + config_path = tmp_path / "config.json" + config_path.write_text( + '{"settings": {"default_page_size": 5, "sql_time_limit_ms": 50}}', "utf-8" + ) + result = runner.invoke( + cli, + [ + "--get", + "/-/settings.json", + "--config", + str(config_path), + "--setting", + "default_page_size", + "10", + ], + ) + assert result.exit_code == 0, result.output + settings = json.loads(result.output) + assert settings["default_page_size"] == 10 + assert settings["sql_time_limit_ms"] == 50 + + def test_plugin_s_overwrite(): runner = CliRunner() plugins_dir = str(pathlib.Path(__file__).parent / "plugins") diff --git a/tests/test_utils.py b/tests/test_utils.py index 88a4532a..b8d047e9 100644 --- a/tests/test_utils.py +++ b/tests/test_utils.py @@ -722,3 +722,35 @@ async def test_calculate_etag(tmp_path): utils._etag_cache[path] = "hash" assert "hash" == await utils.calculate_etag(path) utils._etag_cache.clear() + + +@pytest.mark.parametrize( + "dict1,dict2,expected", + [ + # Basic update + ({"a": 1, "b": 2}, {"b": 3, "c": 4}, {"a": 1, "b": 3, "c": 4}), + # Nested dictionary update + ( + {"a": 1, "b": {"x": 10, "y": 20}}, + {"b": {"y": 30, "z": 40}}, + {"a": 1, "b": {"x": 10, "y": 30, "z": 40}}, + ), + # Deep nested update + ( + {"a": {"b": {"c": 1}}}, + {"a": {"b": {"d": 2}}}, + {"a": {"b": {"c": 1, "d": 2}}}, + ), + # Update with mixed types + ( + {"a": 1, "b": {"x": 10}}, + {"b": {"y": 20}, "c": [1, 2, 3]}, + {"a": 1, "b": {"x": 10, "y": 20}, "c": [1, 2, 3]}, + ), + ], +) +def test_deep_dict_update(dict1, dict2, expected): + result = utils.deep_dict_update(dict1, dict2) + assert result == expected + # Check that the original dict1 was modified + assert dict1 == expected