mirror of
https://github.com/lfnovo/open-notebook.git
synced 2026-05-02 21:30:38 +00:00
refactor: optimize duplicate model validation and improve error handling (#219)
Some checks are pending
Development Build / extract-version (push) Waiting to run
Development Build / test-build-regular (push) Blocked by required conditions
Development Build / test-build-single (push) Blocked by required conditions
Development Build / summary (push) Blocked by required conditions
Some checks are pending
Development Build / extract-version (push) Waiting to run
Development Build / test-build-regular (push) Blocked by required conditions
Development Build / test-build-single (push) Blocked by required conditions
Development Build / summary (push) Blocked by required conditions
* feat: prevent duplicate model names under same provider Implement case-insensitive validation to prevent users from creating duplicate model names under the same provider. This validation is implemented both in the backend API and the frontend UI. Changes: - Backend: Add duplicate check in create_model endpoint (case-insensitive) - Frontend: Add client-side validation in AddModelForm - Frontend: Improve error message display from backend - Tests: Add unit tests for duplicate model validation 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> * refactor: optimize duplicate model validation and improve error handling - Replace O(n) model iteration with efficient SurrealDB query for duplicate check - Improve error message to include model name and provider for better UX - Remove frontend duplicate validation (backend-only enforcement) - Fix test authentication by setting OPEN_NOTEBOOK_PASSWORD before imports - Update test mocking to use repo_query instead of Model.get_all() - Add pytest fixture for TestClient to ensure proper test isolation All 11 tests passing. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> * remove unnecessary package * fix: replace any with unknown type in error handler - Change error type from 'any' to 'unknown' to satisfy ESLint - Add proper type assertion for error object structure - Maintains same runtime behavior with better type safety --------- Co-authored-by: Claude <noreply@anthropic.com>
This commit is contained in:
Luis Novo
GitHub
parent
a0a2282bfa
commit
a287d3b248
7 changed files with 130 additions and 40 deletions
|
|
@ -1,11 +1,84 @@
|
|||
from unittest.mock import patch
|
||||
from unittest.mock import AsyncMock, patch
|
||||
|
||||
import pytest
|
||||
from fastapi.testclient import TestClient
|
||||
|
||||
from api.main import app
|
||||
|
||||
client = TestClient(app)
|
||||
@pytest.fixture
|
||||
def client():
|
||||
"""Create test client after environment variables have been cleared by conftest."""
|
||||
from api.main import app
|
||||
return TestClient(app)
|
||||
|
||||
|
||||
class TestModelCreation:
|
||||
"""Test suite for Model Creation endpoint."""
|
||||
|
||||
@pytest.mark.asyncio
|
||||
@patch("open_notebook.database.repository.repo_query")
|
||||
@patch("api.routers.models.Model.save")
|
||||
async def test_create_duplicate_model_same_case(self, mock_save, mock_repo_query, client):
|
||||
"""Test that creating a duplicate model with same case returns 400."""
|
||||
# Mock repo_query to return a duplicate model
|
||||
mock_repo_query.return_value = [{"id": "model:123", "name": "gpt-4", "provider": "openai", "type": "language"}]
|
||||
|
||||
# Attempt to create duplicate
|
||||
response = client.post(
|
||||
"/api/models",
|
||||
json={
|
||||
"name": "gpt-4",
|
||||
"provider": "openai",
|
||||
"type": "language"
|
||||
}
|
||||
)
|
||||
|
||||
assert response.status_code == 400
|
||||
assert response.json()["detail"] == "Model 'gpt-4' already exists for provider 'openai'"
|
||||
|
||||
@pytest.mark.asyncio
|
||||
@patch("open_notebook.database.repository.repo_query")
|
||||
@patch("api.routers.models.Model.save")
|
||||
async def test_create_duplicate_model_different_case(self, mock_save, mock_repo_query, client):
|
||||
"""Test that creating a duplicate model with different case returns 400."""
|
||||
# Mock repo_query to return a duplicate model (case-insensitive match)
|
||||
mock_repo_query.return_value = [{"id": "model:123", "name": "gpt-4", "provider": "openai", "type": "language"}]
|
||||
|
||||
# Attempt to create duplicate with different case
|
||||
response = client.post(
|
||||
"/api/models",
|
||||
json={
|
||||
"name": "GPT-4",
|
||||
"provider": "OpenAI",
|
||||
"type": "language"
|
||||
}
|
||||
)
|
||||
|
||||
assert response.status_code == 400
|
||||
assert response.json()["detail"] == "Model 'GPT-4' already exists for provider 'OpenAI'"
|
||||
|
||||
@pytest.mark.asyncio
|
||||
@patch("open_notebook.database.repository.repo_query")
|
||||
async def test_create_same_model_name_different_provider(self, mock_repo_query, client):
|
||||
"""Test that creating a model with same name but different provider is allowed."""
|
||||
from open_notebook.domain.models import Model
|
||||
|
||||
# Mock repo_query to return empty (no duplicate found for different provider)
|
||||
mock_repo_query.return_value = []
|
||||
|
||||
# Patch the save method on the Model class
|
||||
with patch.object(Model, 'save', new_callable=AsyncMock) as mock_save:
|
||||
# Attempt to create same model name with different provider (anthropic)
|
||||
response = client.post(
|
||||
"/api/models",
|
||||
json={
|
||||
"name": "gpt-4",
|
||||
"provider": "anthropic",
|
||||
"type": "language"
|
||||
}
|
||||
)
|
||||
|
||||
# Should succeed because provider is different
|
||||
assert response.status_code == 200
|
||||
|
||||
|
||||
class TestModelsProviderAvailability:
|
||||
|
|
@ -13,7 +86,7 @@ class TestModelsProviderAvailability:
|
|||
|
||||
@patch("api.routers.models.os.environ.get")
|
||||
@patch("api.routers.models.AIFactory.get_available_providers")
|
||||
def test_generic_env_var_enables_all_modes(self, mock_esperanto, mock_env):
|
||||
def test_generic_env_var_enables_all_modes(self, mock_esperanto, mock_env, client):
|
||||
"""Test that OPENAI_COMPATIBLE_BASE_URL enables all 4 modes."""
|
||||
|
||||
# Mock environment: only generic var is set
|
||||
|
|
@ -51,7 +124,7 @@ class TestModelsProviderAvailability:
|
|||
|
||||
@patch("api.routers.models.os.environ.get")
|
||||
@patch("api.routers.models.AIFactory.get_available_providers")
|
||||
def test_mode_specific_env_vars_llm_embedding(self, mock_esperanto, mock_env):
|
||||
def test_mode_specific_env_vars_llm_embedding(self, mock_esperanto, mock_env, client):
|
||||
"""Test mode-specific env vars (LLM + EMBEDDING) enable only those 2 modes."""
|
||||
|
||||
# Mock environment: only LLM and EMBEDDING specific vars are set
|
||||
|
|
@ -91,7 +164,7 @@ class TestModelsProviderAvailability:
|
|||
|
||||
@patch("api.routers.models.os.environ.get")
|
||||
@patch("api.routers.models.AIFactory.get_available_providers")
|
||||
def test_no_env_vars_set(self, mock_esperanto, mock_env):
|
||||
def test_no_env_vars_set(self, mock_esperanto, mock_env, client):
|
||||
"""Test that openai-compatible is not available when no env vars are set."""
|
||||
|
||||
# Mock environment: no openai-compatible vars are set
|
||||
|
|
@ -120,7 +193,7 @@ class TestModelsProviderAvailability:
|
|||
|
||||
@patch("api.routers.models.os.environ.get")
|
||||
@patch("api.routers.models.AIFactory.get_available_providers")
|
||||
def test_mixed_config_generic_and_mode_specific(self, mock_esperanto, mock_env):
|
||||
def test_mixed_config_generic_and_mode_specific(self, mock_esperanto, mock_env, client):
|
||||
"""Test mixed config: generic + mode-specific (generic should enable all)."""
|
||||
|
||||
# Mock environment: both generic and mode-specific vars are set
|
||||
|
|
@ -160,7 +233,7 @@ class TestModelsProviderAvailability:
|
|||
|
||||
@patch("api.routers.models.os.environ.get")
|
||||
@patch("api.routers.models.AIFactory.get_available_providers")
|
||||
def test_individual_mode_llm_only(self, mock_esperanto, mock_env):
|
||||
def test_individual_mode_llm_only(self, mock_esperanto, mock_env, client):
|
||||
"""Test individual mode-specific var (LLM only)."""
|
||||
|
||||
# Mock environment: only LLM specific var is set
|
||||
|
|
@ -190,7 +263,7 @@ class TestModelsProviderAvailability:
|
|||
|
||||
@patch("api.routers.models.os.environ.get")
|
||||
@patch("api.routers.models.AIFactory.get_available_providers")
|
||||
def test_individual_mode_embedding_only(self, mock_esperanto, mock_env):
|
||||
def test_individual_mode_embedding_only(self, mock_esperanto, mock_env, client):
|
||||
"""Test individual mode-specific var (EMBEDDING only)."""
|
||||
|
||||
# Mock environment: only EMBEDDING specific var is set
|
||||
|
|
@ -220,7 +293,7 @@ class TestModelsProviderAvailability:
|
|||
|
||||
@patch("api.routers.models.os.environ.get")
|
||||
@patch("api.routers.models.AIFactory.get_available_providers")
|
||||
def test_individual_mode_stt_only(self, mock_esperanto, mock_env):
|
||||
def test_individual_mode_stt_only(self, mock_esperanto, mock_env, client):
|
||||
"""Test individual mode-specific var (STT only)."""
|
||||
|
||||
# Mock environment: only STT specific var is set
|
||||
|
|
@ -250,7 +323,7 @@ class TestModelsProviderAvailability:
|
|||
|
||||
@patch("api.routers.models.os.environ.get")
|
||||
@patch("api.routers.models.AIFactory.get_available_providers")
|
||||
def test_individual_mode_tts_only(self, mock_esperanto, mock_env):
|
||||
def test_individual_mode_tts_only(self, mock_esperanto, mock_env, client):
|
||||
"""Test individual mode-specific var (TTS only)."""
|
||||
|
||||
# Mock environment: only TTS specific var is set
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue