From 7833b33b00c04e68ef148d484324e47992cf9931 Mon Sep 17 00:00:00 2001 From: chon3806 <93464148+chon3806@users.noreply.github.com> Date: Sat, 25 Apr 2026 14:00:25 -0400 Subject: [PATCH] fix(knowledge_base): correct Ollama embedding URL and validate storage type Two related bugs surfaced when configuring a knowledge base with an Ollama embedding model (gh-11910): 1. The OpenAI SDK appends route paths directly to base_url, so an Ollama base_url of `http://localhost:11434` resolves to `http://localhost:11434/embeddings` and fails with 404. Ollama only serves its OpenAI-compatible API under `/v1`. Provide a sensible default (`http://localhost:11434/v1`) and transparently append `/v1` to user-supplied URLs that lack it. 2. Pointing `storage` at a non-vector integration (e.g. `files`) crashed deep inside `vector_store_handler.create_table` with an opaque `AttributeError`. Validate the resolved handler is a `VectorStoreHandler` up front and raise a clear, actionable `ValueError` naming the bad integration and pointing at valid alternatives. --- .../interfaces/knowledge_base/controller.py | 11 ++ .../interfaces/knowledge_base/llm_client.py | 26 ++++ .../knowledge_base/test_ollama_base_url.py | 56 ++++++++ .../test_storage_handler_validation.py | 132 ++++++++++++++++++ 4 files changed, 225 insertions(+) create mode 100644 tests/unit/interfaces/knowledge_base/test_ollama_base_url.py create mode 100644 tests/unit/interfaces/knowledge_base/test_storage_handler_validation.py diff --git a/mindsdb/interfaces/knowledge_base/controller.py b/mindsdb/interfaces/knowledge_base/controller.py index dd7d7397ca8..9d42fa550f3 100644 --- a/mindsdb/interfaces/knowledge_base/controller.py +++ b/mindsdb/interfaces/knowledge_base/controller.py @@ -1094,6 +1094,17 @@ def add( raise ValueError( f"Unable to find database named {vector_db_name}, please make sure {vector_db_name} is defined" ) + # The storage backend for a knowledge base must be a vector database. If the user + # points `storage` at a non-vector integration (e.g. `files`, a SQL database, etc.) + # we previously crashed deep inside `create_table` with an opaque AttributeError. + # Detect that case here and raise a clear, actionable message. Fixes gh-11910. + if not isinstance(vector_store_handler, VectorStoreHandler): + handler_type = type(vector_store_handler).__name__ + raise ValueError( + f"Storage '{vector_db_name}' is not a vector database " + f"(handler type: {handler_type}). Knowledge base storage must be a " + f"vector database integration such as pgvector, chromadb, or faiss." + ) # create table in vectordb before creating KB if "default_vector_storage" in params: # if vector db is a default - drop previous table, if exists diff --git a/mindsdb/interfaces/knowledge_base/llm_client.py b/mindsdb/interfaces/knowledge_base/llm_client.py index 61591b62c17..d4b506625de 100644 --- a/mindsdb/interfaces/knowledge_base/llm_client.py +++ b/mindsdb/interfaces/knowledge_base/llm_client.py @@ -2,6 +2,7 @@ import time import asyncio from typing import List +from urllib.parse import urlparse from openai import OpenAI, AzureOpenAI @@ -98,6 +99,13 @@ def __init__(self, params: dict = None, session=None): elif self.provider == "ollama": if params.get("api_key") is None: params["api_key"] = "n/a" + # Ollama's OpenAI-compatible API is served under the `/v1` path. The OpenAI SDK + # appends route paths (e.g. `/embeddings`) directly to `base_url`, so a value + # like `http://localhost:11434` resolves to `http://localhost:11434/embeddings` + # which Ollama does not serve and returns 404. Provide a sensible default and + # transparently append `/v1` when the user-supplied URL is missing it. + # Fixes gh-11910. + params["base_url"] = self._normalize_ollama_base_url(params.get("base_url")) self.client = OpenAI(**params) elif self.provider == "bedrock": if "aws_region" in params: @@ -110,6 +118,24 @@ def __init__(self, params: dict = None, session=None): else: raise NotImplementedError(f'Provider "{self.provider}" is not supported') + @staticmethod + def _normalize_ollama_base_url(base_url: str | None) -> str: + """Return an Ollama base_url that points at the OpenAI-compatible `/v1` endpoint. + + - When ``base_url`` is missing, default to ``http://localhost:11434/v1``. + - When the user-supplied URL has no ``v1`` path segment, append one. + - Otherwise return the URL unchanged so explicit user configuration wins. + """ + default_base_url = "http://localhost:11434/v1" + if not base_url: + return default_base_url + + normalized = base_url.rstrip("/") + path_segments = [seg for seg in urlparse(normalized).path.split("/") if seg] + if "v1" in path_segments: + return normalized + return f"{normalized}/v1" + @run_in_batches(1000) @retry_with_exponential_backoff def embeddings(self, messages: List[str]): diff --git a/tests/unit/interfaces/knowledge_base/test_ollama_base_url.py b/tests/unit/interfaces/knowledge_base/test_ollama_base_url.py new file mode 100644 index 00000000000..0c5fe7d823f --- /dev/null +++ b/tests/unit/interfaces/knowledge_base/test_ollama_base_url.py @@ -0,0 +1,56 @@ +"""Tests for ``LLMClient._normalize_ollama_base_url`` (gh-11910). + +When a knowledge base is configured with an Ollama embedding model, the +OpenAI Python SDK appends route paths (``/embeddings``, ``/chat/completions``) +directly onto whatever ``base_url`` the client was constructed with. Ollama +serves its OpenAI-compatible API only under the ``/v1`` prefix, so a value +like ``http://localhost:11434`` resolves to ``http://localhost:11434/embeddings`` +which Ollama does not expose and answers with ``404 Not Found``. + +These tests pin the normalization rules so future refactors cannot silently +re-introduce the original bug. +""" + +import pytest + +from mindsdb.interfaces.knowledge_base.llm_client import LLMClient + + +OLLAMA_DEFAULT = "http://localhost:11434/v1" + + +class TestNormalizeOllamaBaseUrl: + @pytest.mark.parametrize("missing", [None, ""]) + def test_missing_base_url_falls_back_to_local_default(self, missing): + assert LLMClient._normalize_ollama_base_url(missing) == OLLAMA_DEFAULT + + def test_user_url_without_v1_gets_v1_appended(self): + # The exact case reported in gh-11910 — first user attempt. + assert LLMClient._normalize_ollama_base_url("http://localhost:11434") == "http://localhost:11434/v1" + + def test_trailing_slash_is_collapsed_before_appending(self): + assert LLMClient._normalize_ollama_base_url("http://localhost:11434/") == "http://localhost:11434/v1" + + def test_explicit_v1_is_left_unchanged(self): + assert LLMClient._normalize_ollama_base_url("http://localhost:11434/v1") == "http://localhost:11434/v1" + + def test_explicit_v1_with_trailing_slash_is_normalized(self): + # rstrip("/") collapses trailing slashes; presence of /v1 still detected. + assert LLMClient._normalize_ollama_base_url("http://localhost:11434/v1/") == "http://localhost:11434/v1" + + def test_remote_host_without_v1_gets_v1_appended(self): + assert ( + LLMClient._normalize_ollama_base_url("https://ollama.example.com:8080") + == "https://ollama.example.com:8080/v1" + ) + + def test_url_with_v1_in_subpath_is_preserved(self): + # Some users front Ollama with a reverse proxy under a sub-path. + assert ( + LLMClient._normalize_ollama_base_url("http://gateway.local/ollama/v1") == "http://gateway.local/ollama/v1" + ) + + def test_v10_is_not_mistaken_for_v1(self): + # Path-segment match must be exact: "v10" must not satisfy the "v1" check, + # otherwise we would silently drop the append and break custom deployments. + assert LLMClient._normalize_ollama_base_url("http://host/v10") == "http://host/v10/v1" diff --git a/tests/unit/interfaces/knowledge_base/test_storage_handler_validation.py b/tests/unit/interfaces/knowledge_base/test_storage_handler_validation.py new file mode 100644 index 00000000000..96f62efcce2 --- /dev/null +++ b/tests/unit/interfaces/knowledge_base/test_storage_handler_validation.py @@ -0,0 +1,132 @@ +"""Tests for KB storage handler validation (gh-11910). + +Before this fix, ``KnowledgeBaseController.add`` happily accepted any +integration as ``storage`` and only blew up later inside +``vector_store_handler.create_table(...)`` with an opaque +``AttributeError: 'FileHandler' object has no attribute 'create_table'``. +The user has no way to tell from that message that they need to point +``storage`` at a vector database. + +This test pins the friendly, up-front ``ValueError`` so the bad message +cannot regress. +""" + +from types import SimpleNamespace +from unittest.mock import MagicMock, patch + +import pytest + +from mindsdb_sql_parser.ast import Identifier + +from mindsdb.integrations.libs.vectordatabase_handler import VectorStoreHandler +from mindsdb.interfaces.knowledge_base.controller import KnowledgeBaseController + + +class _NotAVectorStore: + """Stand-in for an integration handler that is not a vector database + (e.g. a FileHandler or a SQL handler).""" + + +class _FakeVectorStore(VectorStoreHandler): + """Concrete subclass so isinstance(_, VectorStoreHandler) is True.""" + + def __init__(self): # pragma: no cover - trivial + # Intentionally bypass BaseHandler.__init__ — we only need the type. + pass + + +def _build_controller_with_storage(handler): + """Wire up just enough of the controller to reach the storage validation.""" + project = SimpleNamespace(id=1, name="mindsdb") + database_controller = MagicMock() + database_controller.get_project.return_value = project + + data_node = SimpleNamespace(integration_handler=handler) + datahub = MagicMock() + datahub.get.return_value = data_node + + integration_controller = MagicMock() + integration_controller.get.return_value = {"id": 42} + + session = SimpleNamespace( + database_controller=database_controller, + datahub=datahub, + integration_controller=integration_controller, + model_controller=MagicMock(), + ) + return KnowledgeBaseController(session) + + +def _embedding_params(): + return { + "embedding_model": { + "provider": "openai", + "model_name": "text-embedding-3-small", + "api_key": "sk-test", + } + } + + +def test_non_vector_storage_raises_clear_error_before_create_table(): + """A non-vector handler must be rejected with an actionable message.""" + controller = _build_controller_with_storage(_NotAVectorStore()) + + with ( + patch.object( + KnowledgeBaseController, + "_check_embedding_model", + return_value={"dimension": 1536}, + ), + patch.object(KnowledgeBaseController, "get", return_value=None), + ): + with pytest.raises(ValueError) as exc_info: + controller.add( + name="kb_bad_storage", + project_name="mindsdb", + storage=Identifier(parts=["files", "test_knowledge1"]), + params=_embedding_params(), + ) + + message = str(exc_info.value) + assert "files" in message + assert "vector database" in message + assert "pgvector" in message # message must point user at valid alternatives + # The original AttributeError leak must not be what the user sees. + assert "create_table" not in message + assert "_NotAVectorStore" in message + + +def test_vector_storage_passes_validation(): + """A real VectorStoreHandler subclass is accepted (proves the guard + is not over-eager).""" + handler = _FakeVectorStore() + handler.create_table = MagicMock() + handler.drop_table = MagicMock() + handler.add_full_text_index = MagicMock() + controller = _build_controller_with_storage(handler) + + with ( + patch.object( + KnowledgeBaseController, + "_check_embedding_model", + return_value={"dimension": 1536}, + ), + patch.object(KnowledgeBaseController, "_check_vector_table", return_value=None), + patch.object(KnowledgeBaseController, "get", return_value=None), + patch("mindsdb.interfaces.knowledge_base.controller.db.session"), + ): + # We don't care about the return value here; we only care that the + # storage-type guard does not reject a real vector handler. Any + # downstream failure should NOT be the ValueError we are protecting + # against in the negative test above. + try: + controller.add( + name="kb_good_storage", + project_name="mindsdb", + storage=Identifier(parts=["pgvector_db", "kb_table"]), + params=_embedding_params(), + ) + except ValueError as e: + assert "is not a vector database" not in str(e) + + handler.create_table.assert_called_once_with("kb_table")