Address GitHub review feedback: add URL validation, helper functions, and edge case tests

- Add _validate_url() helper function with proper URL validation using urllib.parse - Add _construct_embeddings_url() helper function to refactor URL construction logic - Add comprehensive error handling with ValueError for invalid URLs - Fix test mocking to use correct chromadb import path - Add edge case tests for invalid URLs with pytest markers - Organize tests with @pytest.mark.url_configuration and @pytest.mark.error_handling - Remove unused imports (pytest, MagicMock) to fix lint issues This addresses all suggestions from joaomdmoura's AI review while maintaining backward compatibility. Co-Authored-By: João <joao@crewai.com>
2026-01-09 08:08:32 +00:00 · 2025-07-01 22:07:13 +00:00
parent d422439b7a
commit abd1d341da
2 changed files with 89 additions and 16 deletions
--- a/src/crewai/utilities/embedding_configurator.py
+++ b/src/crewai/utilities/embedding_configurator.py
@@ -91,6 +91,29 @@ class EmbeddingConfigurator:
            organization_id=config.get("organization_id"),
        )

+    @staticmethod
+    def _validate_url(url):
+        """Validate that a URL is properly formatted."""
+        if not url:
+            return False
+        
+        from urllib.parse import urlparse
+        
+        try:
+            result = urlparse(url)
+            return all([result.scheme, result.netloc])
+        except ValueError:
+            return False
+
+    @staticmethod
+    def _construct_embeddings_url(base_url):
+        """Construct the full embeddings URL from a base URL."""
+        if not base_url:
+            return "http://localhost:11434/api/embeddings"
+        
+        base_url = base_url.rstrip('/')
+        return f"{base_url}/api/embeddings" if not base_url.endswith('/api/embeddings') else base_url
+
    @staticmethod
    def _configure_ollama(config, model_name):
        from chromadb.utils.embedding_functions.ollama_embedding_function import (
@@ -103,13 +126,10 @@ class EmbeddingConfigurator:
            or os.getenv("API_BASE")
        )
        
-        if url and not url.endswith("/api/embeddings"):
-            if not url.endswith("/"):
-                url += "/"
-            url += "api/embeddings"
+        if url and not EmbeddingConfigurator._validate_url(url):
+            raise ValueError(f"Invalid Ollama API URL: {url}")
        
-        if not url:
-            url = "http://localhost:11434/api/embeddings"
+        url = EmbeddingConfigurator._construct_embeddings_url(url)

        return OllamaEmbeddingFunction(
            url=url,
--- a/tests/utilities/test_ollama_embedding_configurator.py
+++ b/tests/utilities/test_ollama_embedding_configurator.py
@@ -1,9 +1,10 @@
 import os
 import pytest
-from unittest.mock import patch, MagicMock
+from unittest.mock import patch
 from crewai.utilities.embedding_configurator import EmbeddingConfigurator


+@pytest.mark.url_configuration
 class TestOllamaEmbeddingConfigurator:
    def setup_method(self):
        self.configurator = EmbeddingConfigurator()
@@ -12,7 +13,7 @@ class TestOllamaEmbeddingConfigurator:
    def test_ollama_default_url(self):
        config = {"provider": "ollama", "config": {"model": "llama2"}}
        
-        with patch("crewai.utilities.embedding_configurator.OllamaEmbeddingFunction") as mock_ollama:
+        with patch("chromadb.utils.embedding_functions.ollama_embedding_function.OllamaEmbeddingFunction") as mock_ollama:
            self.configurator.configure_embedder(config)
            mock_ollama.assert_called_once_with(
                url="http://localhost:11434/api/embeddings",
@@ -23,7 +24,7 @@ class TestOllamaEmbeddingConfigurator:
    def test_ollama_respects_api_base_env_var(self):
        config = {"provider": "ollama", "config": {"model": "llama2"}}
        
-        with patch("crewai.utilities.embedding_configurator.OllamaEmbeddingFunction") as mock_ollama:
+        with patch("chromadb.utils.embedding_functions.ollama_embedding_function.OllamaEmbeddingFunction") as mock_ollama:
            self.configurator.configure_embedder(config)
            mock_ollama.assert_called_once_with(
                url="http://custom-ollama:8080/api/embeddings",
@@ -40,7 +41,7 @@ class TestOllamaEmbeddingConfigurator:
            }
        }
        
-        with patch("crewai.utilities.embedding_configurator.OllamaEmbeddingFunction") as mock_ollama:
+        with patch("chromadb.utils.embedding_functions.ollama_embedding_function.OllamaEmbeddingFunction") as mock_ollama:
            self.configurator.configure_embedder(config)
            mock_ollama.assert_called_once_with(
                url="http://config-ollama:9090/api/embeddings",
@@ -57,7 +58,7 @@ class TestOllamaEmbeddingConfigurator:
            }
        }
        
-        with patch("crewai.utilities.embedding_configurator.OllamaEmbeddingFunction") as mock_ollama:
+        with patch("chromadb.utils.embedding_functions.ollama_embedding_function.OllamaEmbeddingFunction") as mock_ollama:
            self.configurator.configure_embedder(config)
            mock_ollama.assert_called_once_with(
                url="http://config-ollama:9090/api/embeddings",
@@ -75,7 +76,7 @@ class TestOllamaEmbeddingConfigurator:
            }
        }
        
-        with patch("crewai.utilities.embedding_configurator.OllamaEmbeddingFunction") as mock_ollama:
+        with patch("chromadb.utils.embedding_functions.ollama_embedding_function.OllamaEmbeddingFunction") as mock_ollama:
            self.configurator.configure_embedder(config)
            mock_ollama.assert_called_once_with(
                url="http://url-config:1111/api/embeddings",
@@ -86,7 +87,7 @@ class TestOllamaEmbeddingConfigurator:
    def test_ollama_handles_trailing_slash_in_api_base(self):
        config = {"provider": "ollama", "config": {"model": "llama2"}}
        
-        with patch("crewai.utilities.embedding_configurator.OllamaEmbeddingFunction") as mock_ollama:
+        with patch("chromadb.utils.embedding_functions.ollama_embedding_function.OllamaEmbeddingFunction") as mock_ollama:
            self.configurator.configure_embedder(config)
            mock_ollama.assert_called_once_with(
                url="http://localhost:11434/api/embeddings",
@@ -97,7 +98,7 @@ class TestOllamaEmbeddingConfigurator:
    def test_ollama_handles_full_url_in_api_base(self):
        config = {"provider": "ollama", "config": {"model": "llama2"}}
        
-        with patch("crewai.utilities.embedding_configurator.OllamaEmbeddingFunction") as mock_ollama:
+        with patch("chromadb.utils.embedding_functions.ollama_embedding_function.OllamaEmbeddingFunction") as mock_ollama:
            self.configurator.configure_embedder(config)
            mock_ollama.assert_called_once_with(
                url="http://localhost:11434/api/embeddings",
@@ -108,7 +109,7 @@ class TestOllamaEmbeddingConfigurator:
    def test_ollama_api_base_without_trailing_slash(self):
        config = {"provider": "ollama", "config": {"model": "llama2"}}
        
-        with patch("crewai.utilities.embedding_configurator.OllamaEmbeddingFunction") as mock_ollama:
+        with patch("chromadb.utils.embedding_functions.ollama_embedding_function.OllamaEmbeddingFunction") as mock_ollama:
            self.configurator.configure_embedder(config)
            mock_ollama.assert_called_once_with(
                url="http://localhost:11434/api/embeddings",
@@ -125,9 +126,61 @@ class TestOllamaEmbeddingConfigurator:
            }
        }
        
-        with patch("crewai.utilities.embedding_configurator.OllamaEmbeddingFunction") as mock_ollama:
+        with patch("chromadb.utils.embedding_functions.ollama_embedding_function.OllamaEmbeddingFunction") as mock_ollama:
            self.configurator.configure_embedder(config)
            mock_ollama.assert_called_once_with(
                url="http://config-ollama:9090/api/embeddings",
                model_name="llama2"
            )
+
+@pytest.mark.error_handling
+class TestOllamaErrorHandling:
+    def setup_method(self):
+        self.configurator = EmbeddingConfigurator()
+
+    @pytest.mark.parametrize("invalid_url", [
+        "not-a-url",
+        "ftp://invalid-scheme",
+        "http://",
+        "://missing-scheme",
+        "http:///missing-netloc",
+    ])
+    def test_invalid_url_raises_error(self, invalid_url):
+        """Test that invalid URLs raise ValueError with clear error message."""
+        config = {
+            "provider": "ollama",
+            "config": {
+                "model": "llama2",
+                "url": invalid_url
+            }
+        }
+
+        with pytest.raises(ValueError, match="Invalid Ollama API URL"):
+            self.configurator.configure_embedder(config)
+
+    @pytest.mark.parametrize("invalid_api_base", [
+        "not-a-url",
+        "ftp://invalid-scheme", 
+        "http://",
+        "://missing-scheme",
+    ])
+    def test_invalid_api_base_raises_error(self, invalid_api_base):
+        """Test that invalid api_base URLs raise ValueError with clear error message."""
+        config = {
+            "provider": "ollama",
+            "config": {
+                "model": "llama2",
+                "api_base": invalid_api_base
+            }
+        }
+
+        with pytest.raises(ValueError, match="Invalid Ollama API URL"):
+            self.configurator.configure_embedder(config)
+
+    @patch.dict(os.environ, {"API_BASE": "not-a-valid-url"}, clear=True)
+    def test_invalid_env_var_raises_error(self):
+        """Test that invalid API_BASE environment variable raises ValueError."""
+        config = {"provider": "ollama", "config": {"model": "llama2"}}
+
+        with pytest.raises(ValueError, match="Invalid Ollama API URL"):
+            self.configurator.configure_embedder(config)