From f8597b345346295594b2ded17a008131796b49c9 Mon Sep 17 00:00:00 2001 From: Alex Castillo Date: Sat, 3 Oct 2026 00:35:18 -0400 Subject: [PATCH] fix(sap): address Greptile review on #44314 - Strip a trailing slash from any deployment_url override before appending /v2/completion, so an override ending in / does not produce a double slash. - Include each ignored deployment's URL alongside its name in the multiple-deployments warning, so same-named deployments stay distinguishable. - Add full type annotations (Final, pytest.MonkeyPatch, pytest.LogCaptureFixture, return types) to the new test class, per AGENTS.md. --- litellm/llms/sap/chat/transformation.py | 4 +- .../llms/sap/chat/test_sap_transformation.py | 64 ++++++++++++++----- 2 files changed, 49 insertions(+), 19 deletions(-) diff --git a/litellm/llms/sap/chat/transformation.py b/litellm/llms/sap/chat/transformation.py index 620ff8d69f2..65028a2b042 100755 --- a/litellm/llms/sap/chat/transformation.py +++ b/litellm/llms/sap/chat/transformation.py @@ -195,7 +195,7 @@ class GenAIHubOrchestrationConfig(OpenAIGPTConfig): ranked: Final = sorted(valid, key=lambda dep: dep[1], reverse=True) if len(ranked) > 1: chosen_url, _, chosen_name = ranked[0] - others: Final = [name for _, _, name in ranked[1:]] + others: Final = [(name, url) for url, _, name in ranked[1:]] litellm.verbose_logger.warning( "SAP: %d orchestration deployments found; using newest (name=%r, url=%r). Others ignored: %r.", len(ranked), @@ -275,7 +275,7 @@ class GenAIHubOrchestrationConfig(OpenAIGPTConfig): or os.environ.get("AICORE_ORCHESTRATION_DEPLOYMENT_URL") or self.deployment_url ) - return f"{deployment_url}/v2/completion" + return f"{deployment_url.rstrip('/')}/v2/completion" def _build_prompt_module( self, diff --git a/tests/unit/llms/sap/chat/test_sap_transformation.py b/tests/unit/llms/sap/chat/test_sap_transformation.py index 79c1210f787..12b47b84306 100644 --- a/tests/unit/llms/sap/chat/test_sap_transformation.py +++ b/tests/unit/llms/sap/chat/test_sap_transformation.py @@ -1,7 +1,15 @@ +from __future__ import annotations + import warnings +from typing import TYPE_CHECKING, Final +from unittest.mock import MagicMock + import pytest from pydantic import ValidationError +if TYPE_CHECKING: + from litellm.llms.sap.chat.transformation import GenAIHubOrchestrationConfig + class TestSAPTransformationIntegration: """Integration tests for SAP transformation.""" @@ -641,13 +649,15 @@ class TestSAPTransformationIntegration: ) + + class TestDeploymentUrlResolution: """get_complete_url must skip discovery when an override is given, and deployment_url must fail gracefully, not with a bare IndexError, when discovery finds nothing.""" @pytest.fixture - def mock_config(self): + def mock_config(self) -> GenAIHubOrchestrationConfig: from litellm.llms.sap.chat.transformation import GenAIHubOrchestrationConfig config = GenAIHubOrchestrationConfig() @@ -658,10 +668,8 @@ class TestDeploymentUrlResolution: return config @staticmethod - def _mock_client(*names: str): - from unittest.mock import MagicMock - - resources = [ + def _mock_client(*names: str) -> MagicMock: + resources: list[dict[str, str]] = [ { "scenarioId": "orchestration", "configurationId": f"cfg-{n}", @@ -670,9 +678,11 @@ class TestDeploymentUrlResolution: } for i, n in enumerate(names) ] - configs = {f"cfg-{n}": {"executableId": "orchestration", "name": n} for n in names} + configs: dict[str, dict[str, str]] = { + f"cfg-{n}": {"executableId": "orchestration", "name": n} for n in names + } - def fake_get(url, headers=None): + def fake_get(url: str, headers: dict[str, str] | None = None) -> MagicMock: resp = MagicMock() if "/lm/deployments" in url: resp.json.return_value = {"resources": resources} @@ -685,10 +695,10 @@ class TestDeploymentUrlResolution: client.get.side_effect = fake_get return client - def test_optional_param_skips_discovery(self, mock_config, monkeypatch): - from unittest.mock import MagicMock - - explicit = "https://custom.sap.com/deployments/abc" + def test_optional_param_skips_discovery( + self, mock_config: GenAIHubOrchestrationConfig, monkeypatch: pytest.MonkeyPatch + ) -> None: + explicit: Final[str] = "https://custom.sap.com/deployments/abc" mock_client = MagicMock() monkeypatch.setattr("litellm.module_level_client", mock_client) @@ -697,10 +707,21 @@ class TestDeploymentUrlResolution: assert url == f"{explicit}/v2/completion" assert not mock_client.get.called - def test_env_var_skips_discovery(self, mock_config, monkeypatch): - from unittest.mock import MagicMock + def test_optional_param_strips_trailing_slash( + self, mock_config: GenAIHubOrchestrationConfig, monkeypatch: pytest.MonkeyPatch + ) -> None: + monkeypatch.setattr("litellm.module_level_client", MagicMock()) - env_url = "https://env.sap.com/deployments/env" + url = mock_config.get_complete_url( + None, None, "gpt-4o", {"deployment_url": "https://custom.sap.com/deployments/abc/"}, {} + ) + + assert url == "https://custom.sap.com/deployments/abc/v2/completion" + + def test_env_var_skips_discovery( + self, mock_config: GenAIHubOrchestrationConfig, monkeypatch: pytest.MonkeyPatch + ) -> None: + env_url: Final[str] = "https://env.sap.com/deployments/env" mock_client = MagicMock() monkeypatch.setattr("litellm.module_level_client", mock_client) monkeypatch.setenv("AICORE_ORCHESTRATION_DEPLOYMENT_URL", env_url) @@ -710,7 +731,9 @@ class TestDeploymentUrlResolution: assert url == f"{env_url}/v2/completion" assert not mock_client.get.called - def test_no_deployments_raises_orchestration_error(self, mock_config, monkeypatch): + def test_no_deployments_raises_orchestration_error( + self, mock_config: GenAIHubOrchestrationConfig, monkeypatch: pytest.MonkeyPatch + ) -> None: from litellm.llms.sap.chat.handler import GenAIHubOrchestrationError monkeypatch.setattr("litellm.module_level_client", self._mock_client()) @@ -719,7 +742,12 @@ class TestDeploymentUrlResolution: with pytest.raises(GenAIHubOrchestrationError, match="No orchestration deployment found"): mock_config.get_complete_url(None, None, "gpt-4o", {}, {}) - def test_discovery_picks_newest_of_multiple_and_warns(self, mock_config, monkeypatch, caplog): + def test_discovery_picks_newest_of_multiple_and_warns( + self, + mock_config: GenAIHubOrchestrationConfig, + monkeypatch: pytest.MonkeyPatch, + caplog: pytest.LogCaptureFixture, + ) -> None: monkeypatch.setattr("litellm.module_level_client", self._mock_client("older", "newer")) monkeypatch.setenv("AICORE_ORCHESTRATION_DEPLOYMENT_URL", "") @@ -728,6 +756,8 @@ class TestDeploymentUrlResolution: assert url == "https://deploy-newer.sap.com/v2/completion" assert any( - "2 orchestration deployments found" in record.getMessage() and "'older'" in record.getMessage() + "2 orchestration deployments found" in record.getMessage() + and "'older'" in record.getMessage() + and "deploy-older.sap.com" in record.getMessage() for record in caplog.records )