Merge pull request #189 from awslabs/fix/sdk_init_config_handlers

fix(config): config_dict path now applies full normalisation pipeline

Author: fgogolli

src/orb/config/loader.py

@@ -151,6 +151,40 @@ def load(
 
         return config
 
+    @classmethod
+    def _build_raw_config_from_dict(
+        cls,
+        config_dict: dict[str, Any],
+        config_manager: Optional["ConfigurationManager"] = None,
+    ) -> dict[str, Any]:
+        """
+        Apply the full normalisation pipeline to an in-memory dict.
+        Mirrors ConfigurationLoader.load() but starts from a dict instead of a file.
+        Pipeline: package defaults -> strategy defaults -> user dict -> env vars -> env expansion.
+        """
+        from orb.config.utils.env_expansion import expand_config_env_vars
+
+        base = cls._load_default_config()
+
+        strategy_defaults = cls._load_strategy_defaults(config_manager)
+        if strategy_defaults:
+            cls._merge_config(base, strategy_defaults)
+
+        cls._merge_config(base, config_dict)
+
+        cls._load_from_env(base, config_manager)
+        result = expand_config_env_vars(base)
+
+        # Hoist provider.provider_defaults to the top level so callers can
+        # inspect strategy defaults without navigating the nested provider key.
+        # Only hoist if not already present at the top level (e.g. from a mock or explicit dict).
+        if "provider_defaults" not in result:
+            provider_section = result.get("provider", {})
+            if "provider_defaults" in provider_section:
+                result["provider_defaults"] = provider_section["provider_defaults"]
+
+        return result
+
     @classmethod
     def _load_strategy_defaults(cls, config_manager=None) -> dict[str, Any]:
         merged: dict[str, Any] = {}

src/orb/config/managers/configuration_manager.py

@@ -96,9 +96,7 @@ def app_config(self) -> AppConfig:
     def _load_app_config(self) -> AppConfig:
         """Load application configuration from loader or in-memory dict."""
         try:
-            if self._config_dict is not None:
-                return self.loader.create_app_config(self._config_dict)
-            raw_config = self.loader.load(self._config_file, config_manager=self)
+            raw_config = self._ensure_raw_config()
             return self.loader.create_app_config(raw_config)
         except Exception as e:
             logger.error("Failed to load app config: %s", e, exc_info=True)
@@ -108,13 +106,11 @@ def _ensure_raw_config(self) -> dict[str, Any]:
         """Ensure raw configuration is loaded."""
         if self._raw_config is None:
             if self._config_dict is not None:
-                # Merge provided dict on top of package defaults so that
-                # provider_defaults (supports_spot etc.) are always present.
                 from orb.config.loader import ConfigurationLoader
 
-                base = ConfigurationLoader._load_default_config()
-                ConfigurationLoader._merge_config(base, self._config_dict)
-                self._raw_config = base
+                self._raw_config = ConfigurationLoader._build_raw_config_from_dict(
+                    self._config_dict, config_manager=self
+                )
             else:
                 self._raw_config = self.loader.load(self._config_file, config_manager=self)
         return self._raw_config
@@ -169,10 +165,8 @@ def get_typed_with_defaults(self, config_type: type[T]) -> T:
     def reload(self) -> None:
         """Reload configuration from sources."""
         try:
-            # Re-derive config file path in case ORB_CONFIG_DIR changed between tests
-            from orb.config.platform_dirs import get_config_location
-
-            self._config_file = str(get_config_location() / "config.json")
+            # Do NOT re-derive _config_file — preserve construction parameters (_config_dict, _config_file)
+            # Only reset cached derived state
 
             # Clear all caches
             self._cache_manager.clear_cache()

tests/unit/sdk/test_sdk_init_config_handlers.py

@@ -0,0 +1,162 @@
+"""Tests proving bug #185: config_dict path skips _load_strategy_defaults().
+
+Tests 1-2 are baseline/resilience checks (expected to PASS).
+Tests 3-6 prove the bug (expected to FAIL against unmodified code).
+"""
+
+from __future__ import annotations
+
+from unittest.mock import MagicMock, patch
+
+from orb.config.loader import ConfigurationLoader
+from orb.config.managers.configuration_manager import ConfigurationManager
+
+_MINIMAL_AWS_DICT = {"provider": {"type": "aws"}}
+
+
+# ---------------------------------------------------------------------------
+# Test 1 — baseline: _load_strategy_defaults works in isolation (PASS)
+# ---------------------------------------------------------------------------
+
+
+class TestLoadStrategyDefaultsBaseline:
+    def test_load_strategy_defaults_returns_non_empty_dict(self):
+        """_load_strategy_defaults() must return a non-empty dict when the
+        provider registry is available and the aws provider is registered."""
+        result = ConfigurationLoader._load_strategy_defaults()
+        assert isinstance(result, dict)
+        assert len(result) > 0, (
+            "_load_strategy_defaults() returned an empty dict — "
+            "expected provider handler defaults to be present"
+        )
+
+
+# ---------------------------------------------------------------------------
+# Test 2 — resilience: registry failure must not propagate (PASS)
+# ---------------------------------------------------------------------------
+
+
+class TestLoadStrategyDefaultsResilience:
+    def test_load_strategy_defaults_survives_registry_failure(self):
+        """If get_provider_registry raises, _load_strategy_defaults must
+        swallow the error and return a dict (possibly empty).
+
+        The import is local inside the method body, so we patch the symbol
+        on the orb.providers.registry module directly.
+        """
+        with patch(
+            "orb.providers.registry.get_provider_registry",
+            side_effect=RuntimeError("registry unavailable"),
+        ):
+            result = ConfigurationLoader._load_strategy_defaults()
+        assert isinstance(result, dict)
+
+
+# ---------------------------------------------------------------------------
+# Test 3 — bug: dict path never calls _load_strategy_defaults (FAIL)
+# ---------------------------------------------------------------------------
+
+
+class TestEnsureRawConfigCallsLoadStrategyDefaults:
+    def test_ensure_raw_config_with_config_dict_calls_load_strategy_defaults(self):
+        """_ensure_raw_config() must call _load_strategy_defaults() when
+        config_dict= is supplied, just as the file path does.
+
+        FAILS against current code because the dict branch only calls
+        _load_default_config() and skips _load_strategy_defaults().
+        """
+        mock_load_strategy = MagicMock(return_value={})
+
+        with patch.object(ConfigurationLoader, "_load_strategy_defaults", mock_load_strategy):
+            cm = ConfigurationManager(config_dict=_MINIMAL_AWS_DICT)
+            cm._ensure_raw_config()
+
+        mock_load_strategy.assert_called()
+
+
+# ---------------------------------------------------------------------------
+# Test 4 — bug: strategy defaults not merged into result (FAIL)
+# ---------------------------------------------------------------------------
+
+
+class TestEnsureRawConfigMergesStrategyDefaults:
+    def test_ensure_raw_config_with_config_dict_merges_strategy_defaults(self):
+        """Strategy defaults returned by _load_strategy_defaults() must be
+        present in the dict produced by _ensure_raw_config().
+
+        FAILS against current code because the dict branch never merges them.
+        """
+        fake_defaults = {
+            "provider_defaults": {"aws": {"handlers": {"RunInstances": {"enabled": True}}}}
+        }
+
+        with patch.object(
+            ConfigurationLoader, "_load_strategy_defaults", return_value=fake_defaults
+        ):
+            cm = ConfigurationManager(config_dict=_MINIMAL_AWS_DICT)
+            result = cm._ensure_raw_config()
+
+        assert "provider_defaults" in result, (
+            "provider_defaults key missing — strategy defaults were not merged"
+        )
+        assert result["provider_defaults"]["aws"]["handlers"]["RunInstances"]["enabled"] is True
+
+
+# ---------------------------------------------------------------------------
+# Test 5 — bug: real defaults absent from config_dict path (FAIL)
+# ---------------------------------------------------------------------------
+
+
+class TestConfigurationManagerConfigDictHasAwsHandlerDefaults:
+    def test_configuration_manager_config_dict_has_aws_handler_defaults(self):
+        """Without any mocking, constructing ConfigurationManager(config_dict=...)
+        and calling _ensure_raw_config() must produce a result that contains
+        evidence of strategy defaults (e.g. a 'provider_defaults' key or any
+        handler-related key contributed by _load_strategy_defaults()).
+
+        FAILS against current code because the dict branch skips
+        _load_strategy_defaults() entirely.
+        """
+        cm = ConfigurationManager(config_dict=_MINIMAL_AWS_DICT)
+        result = cm._ensure_raw_config()
+
+        # _load_strategy_defaults merges provider_defaults from the AWS strategy.
+        # If the dict path called it, this key would be present.
+        assert "provider_defaults" in result, (
+            "provider_defaults missing from _ensure_raw_config() result when "
+            "config_dict= is used — _load_strategy_defaults() was not called"
+        )
+
+
+# ---------------------------------------------------------------------------
+# Test 6 — bug: call-count confirms dict path skips the method (FAIL)
+# ---------------------------------------------------------------------------
+
+
+class TestFileAndDictPathsProduceEquivalentStrategyDefaults:
+    def test_file_and_dict_paths_produce_equivalent_strategy_defaults(self):
+        """_load_strategy_defaults must be called at least once when the
+        config_dict= path is taken, matching the behaviour of the file path.
+
+        FAILS against current code because the dict branch in
+        _ensure_raw_config() never invokes _load_strategy_defaults().
+        """
+        call_count: list[int] = [0]
+        original = ConfigurationLoader._load_strategy_defaults
+
+        def counting_load_strategy_defaults(*args, **kwargs):
+            call_count[0] += 1
+            return original(*args, **kwargs)
+
+        with patch.object(
+            ConfigurationLoader,
+            "_load_strategy_defaults",
+            side_effect=counting_load_strategy_defaults,
+        ):
+            cm = ConfigurationManager(config_dict=_MINIMAL_AWS_DICT)
+            cm._ensure_raw_config()
+
+        assert call_count[0] >= 1, (
+            f"_load_strategy_defaults was called {call_count[0]} time(s) via the "
+            "config_dict path — expected at least 1 call"
+        )