Skip to content

Commit 61fe629

Browse files
refactor(adms): drop broad except in factories, simplify _BindingData
M1 — ``create_client`` and ``create_async_client`` wrapped any ``Exception`` in ``ClientCreationError``. Because ``load_from_env_or_mount`` already maps real config failures to ``ConfigError`` and explicit ``ValueError`` paths re-raise unchanged, the broad except only ever caught internal SDK bugs (``KeyError``, ``AttributeError``, ``TypeError``) and reported them to callers as "client creation failed", masking the real fault. Drop the catch-all so genuine bugs surface as themselves. The ``ClientCreationError`` symbol remains exported from ``sap_cloud_sdk.adms.exceptions`` for any caller still catching it, but the factories no longer raise it. Update the test that asserted the wrapping behaviour to assert pass-through instead. M2 — ``_BindingData.validate`` iterated a hard-coded list of field names via ``getattr(self, f)``. Since the list is static and matches the dataclass shape, switch to direct attribute access (a list of ``(name, value)`` tuples) — both clearer and friendlier to type checkers.
1 parent 7a2ed1d commit 61fe629

3 files changed

Lines changed: 41 additions & 46 deletions

File tree

src/sap_cloud_sdk/adms/client.py

Lines changed: 24 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -59,11 +59,7 @@
5959
_SERVICE_PATH,
6060
load_from_env_or_mount,
6161
)
62-
from sap_cloud_sdk.adms.exceptions import (
63-
ClientCreationError,
64-
ConfigError,
65-
ScanNotCleanError,
66-
)
62+
from sap_cloud_sdk.adms.exceptions import ScanNotCleanError
6763
from sap_cloud_sdk.core.auth import TokenCache
6864
from sap_cloud_sdk.core.telemetry import Module, Operation, record_metrics
6965

@@ -1581,23 +1577,16 @@ def create_client(
15811577
15821578
Raises:
15831579
ConfigError: If the binding configuration is missing or incomplete.
1584-
ClientCreationError: If client instantiation fails.
1580+
ValueError: If ``instance`` is an empty string.
15851581
"""
1586-
try:
1587-
if instance is not None and instance == "":
1588-
raise ValueError(
1589-
"instance must not be an empty string; omit it to use 'default'"
1590-
)
1591-
binding = config or load_from_env_or_mount(instance)
1592-
token_fetcher = IasTokenFetcher(config=binding, cache=token_cache)
1593-
http = AdmsHttp(config=binding, token_fetcher=token_fetcher, user_jwt=user_jwt)
1594-
return AdmsClient(http)
1595-
except (ConfigError, ValueError):
1596-
raise
1597-
except Exception as exc:
1598-
raise ClientCreationError(
1599-
f"Failed to create ADMS client for instance '{instance or 'default'}': {exc}"
1600-
) from exc
1582+
if instance is not None and instance == "":
1583+
raise ValueError(
1584+
"instance must not be an empty string; omit it to use 'default'"
1585+
)
1586+
binding = config or load_from_env_or_mount(instance)
1587+
token_fetcher = IasTokenFetcher(config=binding, cache=token_cache)
1588+
http = AdmsHttp(config=binding, token_fetcher=token_fetcher, user_jwt=user_jwt)
1589+
return AdmsClient(http)
16011590

16021591

16031592
def create_async_client(
@@ -1623,25 +1612,18 @@ def create_async_client(
16231612
16241613
Raises:
16251614
ConfigError: If binding configuration is missing or incomplete.
1626-
ClientCreationError: If client instantiation fails.
1615+
ValueError: If ``instance`` is an empty string.
16271616
"""
1628-
try:
1629-
if instance is not None and instance == "":
1630-
raise ValueError(
1631-
"instance must not be an empty string; omit it to use 'default'"
1632-
)
1633-
binding = config or load_from_env_or_mount(instance)
1634-
token_fetcher = IasTokenFetcher(config=binding, cache=token_cache)
1635-
http = AsyncAdmsHttp(
1636-
config=binding,
1637-
token_fetcher=token_fetcher,
1638-
client=http_client,
1639-
user_jwt=user_jwt,
1640-
)
1641-
return AsyncAdmsClient(http)
1642-
except (ConfigError, ValueError):
1643-
raise
1644-
except Exception as exc:
1645-
raise ClientCreationError(
1646-
f"Failed to create async ADMS client for instance '{instance or 'default'}': {exc}"
1647-
) from exc
1617+
if instance is not None and instance == "":
1618+
raise ValueError(
1619+
"instance must not be an empty string; omit it to use 'default'"
1620+
)
1621+
binding = config or load_from_env_or_mount(instance)
1622+
token_fetcher = IasTokenFetcher(config=binding, cache=token_cache)
1623+
http = AsyncAdmsHttp(
1624+
config=binding,
1625+
token_fetcher=token_fetcher,
1626+
client=http_client,
1627+
user_jwt=user_jwt,
1628+
)
1629+
return AsyncAdmsClient(http)

src/sap_cloud_sdk/adms/config.py

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -73,8 +73,16 @@ class _BindingData:
7373
resource: str = "" # Optional IAS resource URI (app provider name)
7474

7575
def validate(self) -> None:
76-
required = ["clientid", "clientsecret", "url", "uri"]
77-
missing = [f for f in required if not getattr(self, f)]
76+
missing = [
77+
name
78+
for name, value in (
79+
("clientid", self.clientid),
80+
("clientsecret", self.clientsecret),
81+
("url", self.url),
82+
("uri", self.uri),
83+
)
84+
if not value
85+
]
7886
if missing:
7987
raise ConfigError(
8088
f"ADMS binding is missing required fields: {', '.join(missing)}"

tests/adms/unit/test_client.py

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -143,12 +143,17 @@ def test_raises_config_error_on_missing_binding(self):
143143
with pytest.raises(ConfigError, match="missing fields"):
144144
create_client(instance="nonexistent-instance")
145145

146-
def test_wraps_unexpected_exception_in_client_creation_error(self):
146+
def test_unexpected_exception_propagates_as_is(self):
147+
"""Real bugs (e.g. ``RuntimeError`` from internal logic) must surface
148+
as themselves rather than being silently wrapped — wrapping makes
149+
debugging harder and previously masked SDK programming errors as
150+
"client creation failed".
151+
"""
147152
with patch(
148153
"sap_cloud_sdk.adms.client.load_from_env_or_mount",
149154
side_effect=RuntimeError("unexpected"),
150155
):
151-
with pytest.raises(ClientCreationError, match="Failed to create ADMS client"):
156+
with pytest.raises(RuntimeError, match="unexpected"):
152157
create_client(instance="bad-instance")
153158

154159
def test_returns_adms_client_on_success(self):

0 commit comments

Comments
 (0)