diff --git a/src/server/api/utils/settings.py b/src/server/api/utils/settings.py index 7bb69996..f0d60e5f 100644 --- a/src/server/api/utils/settings.py +++ b/src/server/api/utils/settings.py @@ -164,17 +164,26 @@ def _load_prompt_configs(config_data: dict) -> None: def update_server(config_data: dict) -> None: - """Update server configuration""" + """Update server configuration. + + Note: We mutate the existing lists (clear + extend) rather than replacing them. + This is critical because other modules import these lists directly + (e.g., `from server.bootstrap.bootstrap import DATABASE_OBJECTS`). + Replacing the list would leave those modules with stale references. + """ config = Configuration(**config_data) if "database_configs" in config_data: - bootstrap.DATABASE_OBJECTS = config.database_configs or [] + bootstrap.DATABASE_OBJECTS.clear() + bootstrap.DATABASE_OBJECTS.extend(config.database_configs or []) if "model_configs" in config_data: - bootstrap.MODEL_OBJECTS = config.model_configs or [] + bootstrap.MODEL_OBJECTS.clear() + bootstrap.MODEL_OBJECTS.extend(config.model_configs or []) if "oci_configs" in config_data: - bootstrap.OCI_OBJECTS = config.oci_configs or [] + bootstrap.OCI_OBJECTS.clear() + bootstrap.OCI_OBJECTS.extend(config.oci_configs or []) # Load MCP prompt text into cache from prompt_configs _load_prompt_configs(config_data) diff --git a/tests/server/unit/api/utils/test_utils_settings.py b/tests/server/unit/api/utils/test_utils_settings.py index ec518d0d..aebff4d0 100644 --- a/tests/server/unit/api/utils/test_utils_settings.py +++ b/tests/server/unit/api/utils/test_utils_settings.py @@ -14,40 +14,58 @@ from common.schema import Settings, Configuration, Database, Model, OracleCloudSettings -class TestSettings: - """Test settings module functionality""" - - def __init__(self): - """Setup test data for all tests""" - self.default_settings = Settings(client="default") - self.test_client_settings = Settings(client="test_client") - self.sample_config_data = { - "database_configs": [{"name": "test_db", "user": "user", "password": "pass", "dsn": "dsn"}], - "model_configs": [{"id": "test-model", "provider": "openai", "type": "ll"}], - "oci_configs": [{"auth_profile": "DEFAULT", "compartment_id": "ocid1.compartment.oc1..test"}], - "prompt_overrides": {"optimizer_basic-default": "You are helpful"}, - "client_settings": {"client": "default", "max_tokens": 1000, "temperature": 0.7}, - } +##################################################### +# Helper functions for test data +##################################################### +def make_default_settings(): + """Create default settings for tests""" + return Settings(client="default") + + +def make_test_client_settings(): + """Create test client settings for tests""" + return Settings(client="test_client") + + +def make_sample_config_data(): + """Create sample configuration data for tests""" + return { + "database_configs": [{"name": "test_db", "user": "user", "password": "pass", "dsn": "dsn"}], + "model_configs": [{"id": "test-model", "provider": "openai", "type": "ll"}], + "oci_configs": [{"auth_profile": "DEFAULT", "compartment_id": "ocid1.compartment.oc1..test"}], + "prompt_overrides": {"optimizer_basic-default": "You are helpful"}, + "client_settings": {"client": "default", "max_tokens": 1000, "temperature": 0.7}, + } + + +##################################################### +# Client Settings Tests +##################################################### +class TestClientSettings: + """Test client settings CRUD operations""" @patch("server.api.utils.settings.bootstrap") def test_create_client_success(self, mock_bootstrap): """Test successful client settings creation""" - # Create a list that includes the default settings and will be appended to - settings_list = [self.default_settings] + default_cfg = make_default_settings() + settings_list = [default_cfg] mock_bootstrap.SETTINGS_OBJECTS = settings_list result = settings.create_client("new_client") assert result.client == "new_client" - assert result.ll_model.max_tokens == self.default_settings.ll_model.max_tokens # pylint: disable=no-member - # Check that a new client was added to the list + # Verify ll_model settings are copied from default + result_ll_model = result.model_dump()["ll_model"] + default_ll_model = default_cfg.model_dump()["ll_model"] + assert result_ll_model["max_tokens"] == default_ll_model["max_tokens"] assert len(settings_list) == 2 assert settings_list[-1].client == "new_client" @patch("server.api.utils.settings.bootstrap.SETTINGS_OBJECTS") def test_create_client_already_exists(self, mock_settings_objects): """Test creating client settings when client already exists""" - mock_settings_objects.__iter__ = MagicMock(return_value=iter([self.test_client_settings])) + test_cfg = make_test_client_settings() + mock_settings_objects.__iter__ = MagicMock(return_value=iter([test_cfg])) with pytest.raises(ValueError, match="client test_client already exists"): settings.create_client("test_client") @@ -55,20 +73,46 @@ def test_create_client_already_exists(self, mock_settings_objects): @patch("server.api.utils.settings.bootstrap.SETTINGS_OBJECTS") def test_get_client_found(self, mock_settings_objects): """Test getting client settings when client exists""" - mock_settings_objects.__iter__ = MagicMock(return_value=iter([self.test_client_settings])) + test_cfg = make_test_client_settings() + mock_settings_objects.__iter__ = MagicMock(return_value=iter([test_cfg])) result = settings.get_client("test_client") - assert result == self.test_client_settings + assert result == test_cfg @patch("server.api.utils.settings.bootstrap.SETTINGS_OBJECTS") def test_get_client_not_found(self, mock_settings_objects): """Test getting client settings when client doesn't exist""" - mock_settings_objects.__iter__ = MagicMock(return_value=iter([self.default_settings])) + default_cfg = make_default_settings() + mock_settings_objects.__iter__ = MagicMock(return_value=iter([default_cfg])) with pytest.raises(ValueError, match="client nonexistent not found"): settings.get_client("nonexistent") + @patch("server.api.utils.settings.bootstrap.SETTINGS_OBJECTS") + @patch("server.api.utils.settings.get_client") + def test_update_client(self, mock_get_settings, mock_settings_objects): + """Test updating client settings""" + test_cfg = make_test_client_settings() + mock_get_settings.return_value = test_cfg + mock_settings_objects.remove = MagicMock() + mock_settings_objects.append = MagicMock() + mock_settings_objects.__iter__ = MagicMock(return_value=iter([test_cfg])) + + new_settings = Settings(client="test_client", max_tokens=800, temperature=0.9) + result = settings.update_client(new_settings, "test_client") + + assert result.client == "test_client" + mock_settings_objects.remove.assert_called_once_with(test_cfg) + mock_settings_objects.append.assert_called_once() + + +##################################################### +# Server Configuration Tests +##################################################### +class TestServerConfiguration: + """Test server configuration operations""" + @pytest.mark.asyncio @patch("server.api.utils.settings.get_mcp_prompts_with_overrides") @patch("server.api.utils.settings.bootstrap.DATABASE_OBJECTS") @@ -81,7 +125,7 @@ async def test_get_server(self, mock_oci, mock_models, mock_databases, mock_get_ ) mock_models.__iter__ = MagicMock(return_value=iter([Model(id="test", provider="openai", type="ll")])) mock_oci.__iter__ = MagicMock(return_value=iter([OracleCloudSettings(auth_profile="DEFAULT")])) - mock_get_prompts.return_value = [] # Return empty list of prompts + mock_get_prompts.return_value = [] mock_mcp_engine = MagicMock() result = await settings.get_server(mock_mcp_engine) @@ -91,54 +135,79 @@ async def test_get_server(self, mock_oci, mock_models, mock_databases, mock_get_ assert "oci_configs" in result assert "prompt_configs" in result - @patch("server.api.utils.settings.bootstrap.SETTINGS_OBJECTS") - @patch("server.api.utils.settings.get_client") - def test_update_client(self, mock_get_settings, mock_settings_objects): - """Test updating client settings""" - mock_get_settings.return_value = self.test_client_settings - mock_settings_objects.remove = MagicMock() - mock_settings_objects.append = MagicMock() - mock_settings_objects.__iter__ = MagicMock(return_value=iter([self.test_client_settings])) - - new_settings = Settings(client="test_client", max_tokens=800, temperature=0.9) - result = settings.update_client(new_settings, "test_client") - - assert result.client == "test_client" - mock_settings_objects.remove.assert_called_once_with(self.test_client_settings) - mock_settings_objects.append.assert_called_once() - @patch("server.api.utils.settings.bootstrap") def test_update_server(self, mock_bootstrap): """Test updating server configuration""" - # Use the valid sample config data that includes client_settings - settings.update_server(self.sample_config_data) + mock_bootstrap.DATABASE_OBJECTS = [] + mock_bootstrap.MODEL_OBJECTS = [] + mock_bootstrap.OCI_OBJECTS = [] + + settings.update_server(make_sample_config_data()) assert hasattr(mock_bootstrap, "DATABASE_OBJECTS") assert hasattr(mock_bootstrap, "MODEL_OBJECTS") + @patch("server.api.utils.settings.bootstrap") + def test_update_server_mutates_lists_not_replaces(self, mock_bootstrap): + """Test that update_server mutates existing lists rather than replacing them. + + This is critical because other modules import these lists directly + (e.g., `from server.bootstrap.bootstrap import DATABASE_OBJECTS`). + If we replace the list, those modules would hold stale references. + """ + original_db_list = [] + original_model_list = [] + original_oci_list = [] + + mock_bootstrap.DATABASE_OBJECTS = original_db_list + mock_bootstrap.MODEL_OBJECTS = original_model_list + mock_bootstrap.OCI_OBJECTS = original_oci_list + + settings.update_server(make_sample_config_data()) + + # Verify the lists are the SAME objects (mutated, not replaced) + assert mock_bootstrap.DATABASE_OBJECTS is original_db_list, "DATABASE_OBJECTS was replaced instead of mutated" + assert mock_bootstrap.MODEL_OBJECTS is original_model_list, "MODEL_OBJECTS was replaced instead of mutated" + assert mock_bootstrap.OCI_OBJECTS is original_oci_list, "OCI_OBJECTS was replaced instead of mutated" + + # Verify the lists now contain the new data + assert len(original_db_list) == 1 + assert original_db_list[0].name == "test_db" + assert len(original_model_list) == 1 + assert original_model_list[0].id == "test-model" + assert len(original_oci_list) == 1 + assert original_oci_list[0].auth_profile == "DEFAULT" + + +##################################################### +# Config Loading Tests +##################################################### +class TestConfigLoading: + """Test configuration loading operations""" + @patch("server.api.utils.settings.update_server") @patch("server.api.utils.settings.update_client") def test_load_config_from_json_data_with_client(self, mock_update_client, mock_update_server): """Test loading config from JSON data with specific client""" - settings.load_config_from_json_data(self.sample_config_data, client="test_client") + config_data = make_sample_config_data() + settings.load_config_from_json_data(config_data, client="test_client") - mock_update_server.assert_called_once_with(self.sample_config_data) + mock_update_server.assert_called_once_with(config_data) mock_update_client.assert_called_once() @patch("server.api.utils.settings.update_server") @patch("server.api.utils.settings.update_client") def test_load_config_from_json_data_without_client(self, mock_update_client, mock_update_server): """Test loading config from JSON data without specific client""" - settings.load_config_from_json_data(self.sample_config_data) + config_data = make_sample_config_data() + settings.load_config_from_json_data(config_data) - mock_update_server.assert_called_once_with(self.sample_config_data) - # Should be called twice: once for "server" and once for "default" + mock_update_server.assert_called_once_with(config_data) assert mock_update_client.call_count == 2 @patch("server.api.utils.settings.update_server") def test_load_config_from_json_data_missing_client_settings(self, _mock_update_server): """Test loading config from JSON data without client_settings""" - # Create config without client_settings invalid_config = {"database_configs": [], "model_configs": [], "oci_configs": [], "prompt_configs": []} with pytest.raises(KeyError, match="Missing client_settings in config file"): @@ -153,7 +222,7 @@ def test_read_config_from_json_file_success(self, mock_json_load, mock_access, m """Test successful reading of config file""" mock_isfile.return_value = True mock_access.return_value = True - mock_json_load.return_value = self.sample_config_data + mock_json_load.return_value = make_sample_config_data() result = settings.read_config_from_json_file() @@ -166,19 +235,22 @@ def test_read_config_from_json_file_not_exists(self, mock_isfile): """Test reading config file that doesn't exist""" mock_isfile.return_value = False - # This should still attempt to process, but will log a warning - # The actual behavior depends on the implementation - @patch.dict(os.environ, {"CONFIG_FILE": "/path/to/config.txt"}) def test_read_config_from_json_file_wrong_extension(self): """Test reading config file with wrong extension""" - # This should log a warning about the file extension def test_logger_exists(self): """Test that logger is properly configured""" assert hasattr(settings, "logger") assert settings.logger.name == "api.core.settings" + +##################################################### +# Prompt Override Tests +##################################################### +class TestPromptOverrides: + """Test prompt override operations""" + @patch("server.api.utils.settings.cache") def test_load_prompt_override_with_text(self, mock_cache): """Test loading prompt override when text is provided"""