Skip to content

Commit 3a69982

Browse files
authored
Extract _resolve_profile to simplify config file loading (#1333)
## Summary Follow-up to #1325 (default_profile support). Applies the same refactoring we did in the Go SDK to improve readability and error messages. **Before:** `_known_file_config_loader` used two booleans (`has_explicit_profile`, `has_default_profile_setting`) to track where the profile came from. The interaction between them encoded a three-way state (explicit, from settings, legacy fallback) that required careful reading to verify correctness. `__settings__` handling was scattered across three locations in the method. **Now:** A `_resolve_profile` static method handles all resolution logic and returns `(profile, is_fallback)`. The loader no longer needs to know about `__settings__` internals. The two booleans collapse into a single `is_fallback` flag. ### Changes - Extract `_resolve_profile(requested_profile, ini_file)` as a static method that resolves the profile name through three sources: explicit, `[__settings__].default_profile`, or `[DEFAULT]` fallback. - Simplify `_known_file_config_loader` to use the resolved result. No more `has_explicit_profile` / `has_default_profile_setting` booleans. - Improve error messages: using `__settings__` as a profile name (either explicitly or via `default_profile`) now returns `"__settings__ is a reserved section name and cannot be used as a profile"` instead of the generic `"has no __settings__ profile configured"`. - Fix flaky SCIM pagination integration test: the raw `GET /scim/v2/Groups` was fetching the full result set just to read `totalResults`, causing read timeouts on large workspaces. Now passes `count=1` since SCIM always returns `totalResults` regardless of page size. - Fix pre-existing formatting issues in 3 test files (`test_auth.py`, `test_config.py`, `test_core.py`) so the CI fmt check passes. ### Why this matters The three sources for a profile name (explicit, settings-based, legacy fallback) produce two distinct behaviors: 1. **Explicit and settings-based** both require the profile to exist (error if missing) and record the active profile on `cfg.profile`. 2. **Legacy fallback** silently returns if `[DEFAULT]` doesn't exist (unconfigured machine) and doesn't set `cfg.profile`. A single `is_fallback` boolean captures this distinction. The previous two-boolean approach worked correctly but required tracing through their interactions to confirm that. ## Test plan - All 8 existing `test_default_profile.py` tests pass (updated 2 tests to match the improved error messages). - No behavioral changes except the error message text for reserved section name usage. This pull request was AI-assisted by Isaac.
1 parent c141f1c commit 3a69982

4 files changed

Lines changed: 49 additions & 24 deletions

File tree

NEXT_CHANGELOG.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,5 +12,6 @@
1212

1313
### Internal Changes
1414
* Replace the async-disabling mechanism on token refresh failure with a 1-minute retry backoff. Previously, a single failed async refresh would disable proactive token renewal until the token expired. Now, the SDK waits a short cooldown period and retries, improving resilience to transient errors.
15+
* Extract `_resolve_profile` to simplify config file loading and improve `__settings__` error messages.
1516

1617
### API Changes

databricks/sdk/config.py

Lines changed: 39 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -716,6 +716,37 @@ def _load_from_env(self):
716716
if found:
717717
logger.debug("Loaded from environment")
718718

719+
@staticmethod
720+
def _resolve_profile(requested_profile, ini_file):
721+
"""Resolve which profile to load from the config file.
722+
723+
Returns (profile_name, is_fallback):
724+
- If requested_profile is set explicitly, returns it with is_fallback=False.
725+
- If [__settings__].default_profile is set, returns it with is_fallback=False.
726+
- If [DEFAULT] has keys, returns "DEFAULT" with is_fallback=True.
727+
- Otherwise returns (None, True) to signal no config is available.
728+
729+
Raises ValueError if the resolved profile is the reserved __settings__ section.
730+
"""
731+
_SETTINGS_SECTION = "__settings__"
732+
733+
if requested_profile is not None:
734+
if requested_profile == _SETTINGS_SECTION:
735+
raise ValueError(f"{_SETTINGS_SECTION} is a reserved section name" " and cannot be used as a profile")
736+
return requested_profile, False
737+
738+
settings = ini_file._sections.get(_SETTINGS_SECTION, {})
739+
default_profile = settings.get("default_profile", "").strip()
740+
if default_profile:
741+
if default_profile == _SETTINGS_SECTION:
742+
raise ValueError(f"{_SETTINGS_SECTION} is a reserved section name" " and cannot be used as a profile")
743+
return default_profile, False
744+
745+
if ini_file.defaults():
746+
return "DEFAULT", True
747+
748+
return None, True
749+
719750
def _known_file_config_loader(self):
720751
if not self.profile and (self.is_any_auth_configured or self.host or self.azure_workspace_resource_id):
721752
# skip loading configuration file if there's any auth configured
@@ -730,31 +761,22 @@ def _known_file_config_loader(self):
730761
return
731762
ini_file = configparser.ConfigParser()
732763
ini_file.read(config_path)
733-
profile = self.profile
734-
has_explicit_profile = self.profile is not None
735-
has_default_profile_setting = False
736764
# In Go SDK, we skip merging the profile with DEFAULT section, though Python's ConfigParser.items()
737765
# is returning profile key-value pairs _including those from DEFAULT_. This is not what we expect
738766
# from Unified Auth test suite at the moment. Hence, the private variable access.
739767
# See: https://docs.python.org/3/library/configparser.html#mapping-protocol-access
740-
if not has_explicit_profile:
741-
# Check [__settings__].default_profile before falling back to [DEFAULT].
742-
settings = ini_file._sections.get("__settings__", {})
743-
default_profile = settings.get("default_profile", "").strip()
744-
if default_profile:
745-
profile = default_profile
746-
has_default_profile_setting = True
747-
elif ini_file.defaults():
748-
profile = "DEFAULT"
749-
else:
750-
logger.debug(f"{config_path} has no DEFAULT profile configured")
751-
return
768+
profile, is_fallback = self._resolve_profile(self.profile, ini_file)
769+
if profile is None:
770+
logger.debug(f"{config_path} has no DEFAULT profile configured")
771+
return
752772
profiles = {name: values for name, values in ini_file._sections.items()}
753-
# [__settings__] is not a profile; exclude it from the profile map.
754773
profiles.pop("__settings__", None)
755774
if ini_file.defaults():
756775
profiles["DEFAULT"] = ini_file.defaults()
757776
if profile not in profiles:
777+
if is_fallback:
778+
logger.debug(f"{config_path} has no {profile} profile configured")
779+
return
758780
raise ValueError(f"resolve: {config_path} has no {profile} profile configured")
759781
raw_config = profiles[profile]
760782
logger.info(f"loading {profile} profile from {config_file}: {', '.join(raw_config.keys())}")
@@ -763,7 +785,7 @@ def _known_file_config_loader(self):
763785
# don't overwrite a value previously set
764786
continue
765787
self.__setattr__(k, v)
766-
if has_default_profile_setting:
788+
if not is_fallback:
767789
self.profile = profile
768790

769791
def _validate(self):

tests/integration/test_iam.py

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,8 @@ def test_scim_get_user_as_dict(w):
3636
],
3737
)
3838
def test_workspace_users_list_pagination(w, path, call):
39-
raw = w.api_client.do("GET", path)
39+
# Use count=1 to avoid serializing the full result set just to read totalResults.
40+
raw = w.api_client.do("GET", path, query={"count": 1})
4041
total = raw["totalResults"]
4142
all = call(w)
4243
found = len(list(all))
@@ -61,7 +62,8 @@ def test_workspace_users_list_pagination(w, path, call):
6162
],
6263
)
6364
def test_account_users_list_pagination(a, path, call):
64-
raw = a.api_client.do("GET", path.replace("%s", a.config.account_id))
65+
# Use count=1 to avoid serializing the full result set just to read totalResults.
66+
raw = a.api_client.do("GET", path.replace("%s", a.config.account_id), query={"count": 1})
6567
total = raw["totalResults"]
6668
all = call(a)
6769
found = len(list(all))

tests/test_default_profile.py

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -102,9 +102,9 @@ def test_settings_section_is_not_a_profile(tmp_path):
102102
token = dapiXYZ
103103
""",
104104
)
105-
# Explicitly requesting __settings__ as a profile should fail,
106-
# proving the SDK excludes it from the profile map.
107-
with pytest.raises(ValueError, match="has no __settings__ profile configured"):
105+
# Explicitly requesting __settings__ as a profile should fail with a
106+
# clear "reserved section name" error, not a generic "no profile" error.
107+
with pytest.raises(ValueError, match="__settings__ is a reserved section name"):
108108
Config(
109109
config_file=cfg_file,
110110
profile="__settings__",
@@ -113,7 +113,7 @@ def test_settings_section_is_not_a_profile(tmp_path):
113113

114114

115115
def test_default_profile_pointing_to_settings_section_is_rejected(tmp_path):
116-
"""default_profile = __settings__ should fail like any other non-profile target."""
116+
"""default_profile = __settings__ should fail with a reserved section name error."""
117117
cfg_file = _write_cfg(
118118
tmp_path,
119119
"""\
@@ -125,7 +125,7 @@ def test_default_profile_pointing_to_settings_section_is_rejected(tmp_path):
125125
token = dapiXYZ
126126
""",
127127
)
128-
with pytest.raises(ValueError, match="has no __settings__ profile configured"):
128+
with pytest.raises(ValueError, match="__settings__ is a reserved section name"):
129129
Config(config_file=cfg_file, credentials_strategy=noop_credentials)
130130

131131

0 commit comments

Comments
 (0)