[2.x] Expand edge case handling in ConfigManager (#1321)

* add test catching error with empty touched config.json

* handle empty touched config.json, passes test

* ensure schema file doesn't have duplicated types

* pre-commit

* add inline comment explaining st_size check

Author: dlqqq

packages/jupyter-ai/jupyter_ai/config_manager.py

@@ -1,12 +1,11 @@
 import json
 import logging
 import os
-import shutil
 import time
 from copy import deepcopy
 from typing import List, Optional, Type, Union
 
-from deepmerge import always_merger as Merger
+from deepmerge import Merger, always_merger
 from jsonschema import Draft202012Validator as Validator
 from jupyter_ai.models import DescribeConfigResponse, GlobalConfig, UpdateConfigRequest
 from jupyter_ai_magics import JupyternautPersona, Persona
@@ -178,11 +177,23 @@ def _init_config_schema(self):
         with open(OUR_SCHEMA_PATH, encoding="utf-8") as f:
             default_schema = json.load(f)
 
+        # Create a custom `deepmerge.Merger` object to merge lists using the
+        # 'append_unique' strategy.
+        #
+        # This stops type union declarations like `["string", "null"]` from
+        # growing into `["string", "null", "string", "null"]` on restart.
+        # This fixes issue #1320.
+        merger = Merger(
+            [(list, ["append_unique"]), (dict, ["merge"]), (set, ["union"])],
+            ["override"],
+            ["override"],
+        )
+
         # merge existing_schema into default_schema
         # specifying existing_schema as the second argument ensures that
         # existing_schema always overrides existing keys in default_schema, i.e.
         # this call only adds new keys in default_schema.
-        schema = Merger.merge(default_schema, existing_schema)
+        schema = merger.merge(default_schema, existing_schema)
         with open(self.schema_path, encoding="utf-8", mode="w") as f:
             json.dump(schema, f, indent=self.indentation_depth)
 
@@ -194,15 +205,19 @@ def _init_validator(self) -> None:
 
     def _init_config(self):
         default_config = self._init_defaults()
-        if os.path.exists(self.config_path):
+        # if the config file exists, read from it and use our defaults to fill
+        # out any missing fields. otherwise, create a new config file from our
+        # defaults.
+        # the `st_size` check treats empty 0-byte config files as non-existent.
+        if os.path.exists(self.config_path) and os.stat(self.config_path).st_size != 0:
             self._process_existing_config(default_config)
         else:
             self._create_default_config(default_config)
 
     def _process_existing_config(self, default_config):
         with open(self.config_path, encoding="utf-8") as f:
             existing_config = json.loads(f.read())
-            merged_config = Merger.merge(
+            merged_config = always_merger.merge(
                 default_config,
                 {k: v for k, v in existing_config.items() if v is not None},
             )
@@ -481,7 +496,7 @@ def update_config(self, config_update: UpdateConfigRequest):  # type:ignore
                     raise KeyEmptyError("API key value cannot be empty.")
 
         config_dict = self._read_config().model_dump()
-        Merger.merge(config_dict, config_update.model_dump(exclude_unset=True))
+        always_merger.merge(config_dict, config_update.model_dump(exclude_unset=True))
         self._write_config(GlobalConfig(**config_dict))
 
     # this cannot be a property, as the parent Configurable already defines the

packages/jupyter-ai/jupyter_ai/tests/test_config_manager.py

@@ -1,6 +1,7 @@
 import json
 import logging
 import os
+from pathlib import Path
 from unittest.mock import mock_open, patch
 
 import pytest
@@ -513,8 +514,8 @@ def test_config_manager_does_not_write_to_defaults(
 def test_config_manager_updates_schema(jp_data_dir, common_cm_kwargs):
     """
     Asserts that the ConfigManager adds new keys to the user's config schema
-    which are present in Jupyter AI's schema on init. Asserts that #1291 does
-    not occur again in the future.
+    which are present in Jupyter AI's schema on init. Asserts that the main
+    issue reported in #1291 does not occur again in the future.
     """
     schema_path = str(jp_data_dir / "config_schema.json")
     with open(schema_path, "w") as file:
@@ -547,3 +548,17 @@ def test_config_manager_updates_schema(jp_data_dir, common_cm_kwargs):
         assert "fields" in new_schema["properties"]
         assert "embeddings_fields" in new_schema["properties"]
         assert "completions_fields" in new_schema["properties"]
+
+
+def test_config_manager_handles_empty_touched_file(common_cm_kwargs):
+    """
+    Asserts that ConfigManager does not fail at runtime if `config.json` is a
+    "touched file", a completely empty file with 0 bytes. This may happen if a
+    user / build system runs `touch config.json` by accident.
+
+    Asserts that the second issue reported in #1291 does not occur again in the
+    future.
+    """
+    config_path = common_cm_kwargs["config_path"]
+    Path(config_path).touch()
+    ConfigManager(**common_cm_kwargs)