Skip to content

Commit ea1b2fd

Browse files
committed
fix: address CodeRabbit review findings on PR langflow-ai#2063
- Clear providers.openai.base_url (not just api_key/configured) when removing the OpenAI config, so a later re-enable falls back to api.openai.com instead of silently reusing a stale gateway URL. - Reject whitespace-only openai_base_url in SettingsUpdateBody and OnboardingBody via a field_validator, instead of silently accepting it and stripping it down to an empty (no-op) value downstream. - Move test_normalize_model_name_still_distinguishes_cohere_variants out of the base_url-focused config test file into test_embedding_fields.py, where the rest of that module's coverage lives.
1 parent a818b30 commit ea1b2fd

5 files changed

Lines changed: 79 additions & 20 deletions

File tree

src/api/settings/endpoints.py

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -677,6 +677,7 @@ async def update_settings(
677677
if affected:
678678
return _embedding_conflict_response("OpenAI", "openai", affected)
679679
current_config.providers.openai.api_key = ""
680+
current_config.providers.openai.base_url = ""
680681
current_config.providers.openai.configured = False
681682
if current_config.agent.llm_provider == "openai":
682683
fb = _first_configured_llm_provider(current_config, "openai")

src/api/settings/models.py

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,13 @@ class SettingsUpdateBody(BaseModel):
4141
# the backend returns 409 and the frontend prompts the user.
4242
force_remove: bool | None = False
4343

44+
@field_validator("openai_base_url")
45+
@classmethod
46+
def reject_whitespace_only_base_url(cls, value: str | None) -> str | None:
47+
if value is not None and not value.strip():
48+
raise ValueError("openai_base_url must not be blank")
49+
return value
50+
4451

4552
class OnboardingBody(BaseModel):
4653
llm_provider: str | None = Field(None, pattern="^(openai|anthropic|watsonx|ollama)$")
@@ -55,6 +62,13 @@ class OnboardingBody(BaseModel):
5562
watsonx_project_id: str | None = Field(None, min_length=1)
5663
ollama_endpoint: str | None = Field(None, min_length=1)
5764

65+
@field_validator("openai_base_url")
66+
@classmethod
67+
def reject_whitespace_only_base_url(cls, value: str | None) -> str | None:
68+
if value is not None and not value.strip():
69+
raise ValueError("openai_base_url must not be blank")
70+
return value
71+
5872

5973
class CitationDisplayData(BaseModel):
6074
file_path: str | None = None

tests/unit/config/test_openai_base_url_config.py

Lines changed: 0 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -128,14 +128,3 @@ def test_env_override_does_not_clobber_other_openai_fields(self, monkeypatch):
128128

129129
assert config.providers.openai.api_key == "sk-from-env"
130130
assert config.providers.openai.base_url == "http://localhost:4444/v1"
131-
132-
133-
def test_normalize_model_name_still_distinguishes_cohere_variants():
134-
"""Not directly about base_url, but the task calls out locking this in:
135-
normalize_model_name() must keep these two Cohere model ids distinct.
136-
"""
137-
from utils.embedding_fields import normalize_model_name
138-
139-
a = normalize_model_name("cohere.embed-multilingual-v3")
140-
b = normalize_model_name("cohere.embed-multilingual-v3.0")
141-
assert a != b

tests/unit/test_embedding_fields.py

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,11 @@
1414

1515
import pytest
1616

17-
from utils.embedding_fields import build_knn_vector_field, get_embedding_field_name
17+
from utils.embedding_fields import (
18+
build_knn_vector_field,
19+
get_embedding_field_name,
20+
normalize_model_name,
21+
)
1822

1923

2024
class TestBuildKnnVectorFieldStructure:
@@ -138,3 +142,12 @@ async def test_create_index_body_uses_configured_shards_and_replicas(
138142

139143
assert body["settings"]["number_of_shards"] == 3
140144
assert body["settings"]["number_of_replicas"] == 2
145+
146+
147+
class TestNormalizeModelName:
148+
"""normalize_model_name() must produce distinct, safe field-name suffixes."""
149+
150+
def test_still_distinguishes_cohere_variants(self) -> None:
151+
a = normalize_model_name("cohere.embed-multilingual-v3")
152+
b = normalize_model_name("cohere.embed-multilingual-v3.0")
153+
assert a != b

tests/unit/test_settings_openai_base_url.py

Lines changed: 50 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99
from unittest.mock import AsyncMock
1010

1111
import pytest
12+
from pydantic import ValidationError
1213

1314
import api.settings.endpoints as settings_api
1415
from config.config_manager import (
@@ -32,10 +33,10 @@ def add_done_callback(self, cb):
3233
self.done_callback = cb
3334

3435

35-
def _make_config(*, edited: bool = True) -> OpenRAGConfig:
36+
def _make_config(*, edited: bool = True, openai: OpenAIConfig | None = None) -> OpenRAGConfig:
3637
return OpenRAGConfig(
3738
providers=ProvidersConfig(
38-
openai=OpenAIConfig(),
39+
openai=openai if openai is not None else OpenAIConfig(),
3940
anthropic=AnthropicConfig(),
4041
watsonx=WatsonXConfig(),
4142
ollama=OllamaConfig(),
@@ -67,9 +68,7 @@ def _fake_create_task(coro):
6768
lambda updated_config: True,
6869
raising=True,
6970
)
70-
monkeypatch.setattr(
71-
settings_api.clients, "refresh_patched_client", _noop_refresh, raising=True
72-
)
71+
monkeypatch.setattr(settings_api.clients, "refresh_patched_client", _noop_refresh, raising=True)
7372
monkeypatch.setattr(settings_api.TelemetryClient, "send_event", AsyncMock(), raising=True)
7473
monkeypatch.setattr(settings_api.asyncio, "create_task", _fake_create_task, raising=True)
7574
return fake_task
@@ -109,6 +108,35 @@ async def test_update_settings_strips_openai_base_url_whitespace(monkeypatch):
109108
assert config.providers.openai.base_url == "https://gateway.example.com/v1"
110109

111110

111+
@pytest.mark.asyncio
112+
async def test_remove_openai_config_clears_base_url(monkeypatch):
113+
"""Regression: removing the OpenAI config used to clear api_key/configured
114+
but leave a stale base_url behind. A later re-enable of OpenAI would then
115+
silently keep routing through the old (possibly now-invalid) gateway
116+
instead of falling back to api.openai.com.
117+
"""
118+
settings_api._background_tasks.clear()
119+
config = _make_config(
120+
openai=OpenAIConfig(
121+
api_key="sk-test", base_url="https://gateway.example.com/v1", configured=True
122+
)
123+
)
124+
# remove_openai_config requires another provider to be configured.
125+
config.providers.anthropic.configured = True
126+
_patch_update_settings_deps(monkeypatch, config)
127+
128+
response = await settings_api.update_settings(
129+
settings_api.SettingsUpdateBody(remove_openai_config=True, force_remove=True),
130+
session_manager=object(),
131+
user=None,
132+
)
133+
134+
assert isinstance(response, settings_api.SettingsUpdateResponse)
135+
assert config.providers.openai.api_key == ""
136+
assert config.providers.openai.configured is False
137+
assert config.providers.openai.base_url == ""
138+
139+
112140
@pytest.mark.asyncio
113141
async def test_onboarding_persists_openai_base_url(monkeypatch):
114142
config = _make_config(edited=False)
@@ -123,9 +151,7 @@ async def _noop_refresh():
123151
lambda updated_config: True,
124152
raising=True,
125153
)
126-
monkeypatch.setattr(
127-
settings_api.clients, "refresh_patched_client", _noop_refresh, raising=True
128-
)
154+
monkeypatch.setattr(settings_api.clients, "refresh_patched_client", _noop_refresh, raising=True)
129155
monkeypatch.setattr(settings_api.TelemetryClient, "send_event", AsyncMock(), raising=True)
130156
monkeypatch.setattr(settings_api, "wait_for_langflow", AsyncMock(), raising=True)
131157

@@ -144,3 +170,19 @@ async def _noop_refresh():
144170
assert isinstance(response, settings_api.OnboardingResponse)
145171
assert config.providers.openai.base_url == "https://gateway.example.com/v1"
146172
assert config.providers.openai.configured is False
173+
174+
175+
class TestWhitespaceOnlyBaseUrlRejected:
176+
"""A whitespace-only openai_base_url must fail validation instead of being
177+
silently accepted and later stripped down to an empty string by the
178+
endpoint handlers (which would look identical to "not configured" with
179+
no error surfaced to the caller).
180+
"""
181+
182+
def test_settings_update_body_rejects_whitespace_only_base_url(self):
183+
with pytest.raises(ValidationError):
184+
settings_api.SettingsUpdateBody(openai_base_url=" ")
185+
186+
def test_onboarding_body_rejects_whitespace_only_base_url(self):
187+
with pytest.raises(ValidationError):
188+
settings_api.OnboardingBody(openai_base_url=" ")

0 commit comments

Comments
 (0)