mirror of
https://github.com/jamiepine/voicebox.git
synced 2026-09-15 04:40:40 -07:00
Fix #134: Add validation for duplicate profile names
- Add validation in create_profile() to check for existing names before insert - Add validation in update_profile() to prevent renaming to duplicate names - Improve error handling in API endpoints with user-friendly messages - Add comprehensive test suite for duplicate name validation - Update CHANGELOG.md with fix details This fix prevents database constraint violations and provides clear error messages when users attempt to create or update profiles with names that already exist in the database.
This commit is contained in:
@@ -5,6 +5,14 @@ All notable changes to Voicebox will be documented in this file.
|
||||
The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
|
||||
and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
|
||||
|
||||
## [Unreleased]
|
||||
|
||||
### Fixed
|
||||
- **Profile Name Validation** - Added proper validation to prevent duplicate profile names ([#134](https://github.com/jamiepine/voicebox/issues/134))
|
||||
- Users now receive clear error messages when attempting to create or update profiles with duplicate names
|
||||
- Improved error handling in create and update profile API endpoints
|
||||
- Added comprehensive test suite for duplicate name validation
|
||||
|
||||
## [0.1.0] - 2026-01-25
|
||||
|
||||
### Added
|
||||
|
||||
+10
-4
@@ -221,7 +221,10 @@ async def create_profile(
|
||||
"""Create a new voice profile."""
|
||||
try:
|
||||
return await profiles.create_profile(data, db)
|
||||
except ValueError as e:
|
||||
raise HTTPException(status_code=400, detail=str(e))
|
||||
except Exception as e:
|
||||
# Fallback for unexpected errors
|
||||
raise HTTPException(status_code=400, detail=str(e))
|
||||
|
||||
|
||||
@@ -277,10 +280,13 @@ async def update_profile(
|
||||
db: Session = Depends(get_db),
|
||||
):
|
||||
"""Update a voice profile."""
|
||||
profile = await profiles.update_profile(profile_id, data, db)
|
||||
if not profile:
|
||||
raise HTTPException(status_code=404, detail="Profile not found")
|
||||
return profile
|
||||
try:
|
||||
profile = await profiles.update_profile(profile_id, data, db)
|
||||
if not profile:
|
||||
raise HTTPException(status_code=404, detail="Profile not found")
|
||||
return profile
|
||||
except ValueError as e:
|
||||
raise HTTPException(status_code=400, detail=str(e))
|
||||
|
||||
|
||||
@app.delete("/profiles/{profile_id}")
|
||||
|
||||
+27
-10
@@ -38,14 +38,22 @@ async def create_profile(
|
||||
) -> VoiceProfileResponse:
|
||||
"""
|
||||
Create a new voice profile.
|
||||
|
||||
|
||||
Args:
|
||||
data: Profile creation data
|
||||
db: Database session
|
||||
|
||||
|
||||
Returns:
|
||||
Created profile
|
||||
|
||||
Raises:
|
||||
ValueError: If a profile with the same name already exists
|
||||
"""
|
||||
# Check if profile name already exists
|
||||
existing_profile = db.query(DBVoiceProfile).filter_by(name=data.name).first()
|
||||
if existing_profile:
|
||||
raise ValueError(f"A profile with the name '{data.name}' already exists. Please choose a different name.")
|
||||
|
||||
# Create profile in database
|
||||
db_profile = DBVoiceProfile(
|
||||
id=str(uuid.uuid4()),
|
||||
@@ -55,15 +63,15 @@ async def create_profile(
|
||||
created_at=datetime.utcnow(),
|
||||
updated_at=datetime.utcnow(),
|
||||
)
|
||||
|
||||
|
||||
db.add(db_profile)
|
||||
db.commit()
|
||||
db.refresh(db_profile)
|
||||
|
||||
|
||||
# Create profile directory
|
||||
profile_dir = _get_profiles_dir() / db_profile.id
|
||||
profile_dir.mkdir(parents=True, exist_ok=True)
|
||||
|
||||
|
||||
return VoiceProfileResponse.model_validate(db_profile)
|
||||
|
||||
|
||||
@@ -191,28 +199,37 @@ async def update_profile(
|
||||
) -> Optional[VoiceProfileResponse]:
|
||||
"""
|
||||
Update a voice profile.
|
||||
|
||||
|
||||
Args:
|
||||
profile_id: Profile ID
|
||||
data: Updated profile data
|
||||
db: Database session
|
||||
|
||||
|
||||
Returns:
|
||||
Updated profile or None if not found
|
||||
|
||||
Raises:
|
||||
ValueError: If a profile with the same name already exists (different profile)
|
||||
"""
|
||||
profile = db.query(DBVoiceProfile).filter_by(id=profile_id).first()
|
||||
if not profile:
|
||||
return None
|
||||
|
||||
|
||||
# Check if the new name conflicts with another profile
|
||||
if profile.name != data.name:
|
||||
existing_profile = db.query(DBVoiceProfile).filter_by(name=data.name).first()
|
||||
if existing_profile:
|
||||
raise ValueError(f"A profile with the name '{data.name}' already exists. Please choose a different name.")
|
||||
|
||||
# Update fields
|
||||
profile.name = data.name
|
||||
profile.description = data.description
|
||||
profile.language = data.language
|
||||
profile.updated_at = datetime.utcnow()
|
||||
|
||||
|
||||
db.commit()
|
||||
db.refresh(profile)
|
||||
|
||||
|
||||
return VoiceProfileResponse.model_validate(profile)
|
||||
|
||||
|
||||
|
||||
@@ -0,0 +1,217 @@
|
||||
"""
|
||||
Tests for profile duplicate name validation.
|
||||
|
||||
This test suite verifies that the application correctly handles
|
||||
duplicate profile names and provides user-friendly error messages.
|
||||
"""
|
||||
|
||||
import pytest
|
||||
import tempfile
|
||||
import shutil
|
||||
from pathlib import Path
|
||||
from sqlalchemy import create_engine
|
||||
from sqlalchemy.orm import sessionmaker
|
||||
|
||||
# Add parent directory to path to import backend modules
|
||||
import sys
|
||||
sys.path.insert(0, str(Path(__file__).parent.parent))
|
||||
|
||||
from database import Base, VoiceProfile as DBVoiceProfile
|
||||
from models import VoiceProfileCreate
|
||||
from profiles import create_profile, update_profile
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def test_db():
|
||||
"""Create a temporary test database."""
|
||||
# Create temporary directory for test database
|
||||
temp_dir = tempfile.mkdtemp()
|
||||
db_path = Path(temp_dir) / "test.db"
|
||||
|
||||
# Create engine and session
|
||||
engine = create_engine(f"sqlite:///{db_path}")
|
||||
Base.metadata.create_all(bind=engine)
|
||||
SessionLocal = sessionmaker(autocommit=False, autoflush=False, bind=engine)
|
||||
|
||||
db = SessionLocal()
|
||||
|
||||
yield db
|
||||
|
||||
# Cleanup
|
||||
db.close()
|
||||
shutil.rmtree(temp_dir)
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def mock_profiles_dir(monkeypatch, tmp_path):
|
||||
"""Mock the profiles directory to use a temporary path."""
|
||||
import profiles
|
||||
monkeypatch.setattr(profiles, '_get_profiles_dir', lambda: tmp_path)
|
||||
return tmp_path
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_create_profile_duplicate_name_raises_error(test_db, mock_profiles_dir):
|
||||
"""Test that creating a profile with a duplicate name raises a ValueError."""
|
||||
# Create first profile
|
||||
profile_data_1 = VoiceProfileCreate(
|
||||
name="Test Profile",
|
||||
description="First profile",
|
||||
language="en"
|
||||
)
|
||||
|
||||
profile_1 = await create_profile(profile_data_1, test_db)
|
||||
assert profile_1.name == "Test Profile"
|
||||
|
||||
# Try to create second profile with same name
|
||||
profile_data_2 = VoiceProfileCreate(
|
||||
name="Test Profile",
|
||||
description="Second profile",
|
||||
language="en"
|
||||
)
|
||||
|
||||
with pytest.raises(ValueError) as exc_info:
|
||||
await create_profile(profile_data_2, test_db)
|
||||
|
||||
# Verify error message is user-friendly
|
||||
assert "already exists" in str(exc_info.value)
|
||||
assert "Test Profile" in str(exc_info.value)
|
||||
assert "choose a different name" in str(exc_info.value).lower()
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_create_profile_different_names_succeeds(test_db, mock_profiles_dir):
|
||||
"""Test that creating profiles with different names succeeds."""
|
||||
# Create first profile
|
||||
profile_data_1 = VoiceProfileCreate(
|
||||
name="Profile One",
|
||||
description="First profile",
|
||||
language="en"
|
||||
)
|
||||
|
||||
profile_1 = await create_profile(profile_data_1, test_db)
|
||||
assert profile_1.name == "Profile One"
|
||||
|
||||
# Create second profile with different name
|
||||
profile_data_2 = VoiceProfileCreate(
|
||||
name="Profile Two",
|
||||
description="Second profile",
|
||||
language="en"
|
||||
)
|
||||
|
||||
profile_2 = await create_profile(profile_data_2, test_db)
|
||||
assert profile_2.name == "Profile Two"
|
||||
|
||||
# Verify both profiles exist
|
||||
assert profile_1.id != profile_2.id
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_update_profile_to_duplicate_name_raises_error(test_db, mock_profiles_dir):
|
||||
"""Test that updating a profile to a duplicate name raises a ValueError."""
|
||||
# Create two profiles with different names
|
||||
profile_data_1 = VoiceProfileCreate(
|
||||
name="Profile A",
|
||||
description="First profile",
|
||||
language="en"
|
||||
)
|
||||
profile_1 = await create_profile(profile_data_1, test_db)
|
||||
|
||||
profile_data_2 = VoiceProfileCreate(
|
||||
name="Profile B",
|
||||
description="Second profile",
|
||||
language="en"
|
||||
)
|
||||
profile_2 = await create_profile(profile_data_2, test_db)
|
||||
|
||||
# Try to update profile_2 to use profile_1's name
|
||||
update_data = VoiceProfileCreate(
|
||||
name="Profile A", # Duplicate name
|
||||
description="Updated description",
|
||||
language="en"
|
||||
)
|
||||
|
||||
with pytest.raises(ValueError) as exc_info:
|
||||
await update_profile(profile_2.id, update_data, test_db)
|
||||
|
||||
# Verify error message is user-friendly
|
||||
assert "already exists" in str(exc_info.value)
|
||||
assert "Profile A" in str(exc_info.value)
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_update_profile_keep_same_name_succeeds(test_db, mock_profiles_dir):
|
||||
"""Test that updating a profile while keeping the same name succeeds."""
|
||||
# Create profile
|
||||
profile_data = VoiceProfileCreate(
|
||||
name="My Profile",
|
||||
description="Original description",
|
||||
language="en"
|
||||
)
|
||||
profile = await create_profile(profile_data, test_db)
|
||||
|
||||
# Update profile with same name but different description
|
||||
update_data = VoiceProfileCreate(
|
||||
name="My Profile", # Same name
|
||||
description="Updated description",
|
||||
language="en"
|
||||
)
|
||||
|
||||
updated_profile = await update_profile(profile.id, update_data, test_db)
|
||||
|
||||
# Verify update succeeded
|
||||
assert updated_profile is not None
|
||||
assert updated_profile.id == profile.id
|
||||
assert updated_profile.name == "My Profile"
|
||||
assert updated_profile.description == "Updated description"
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_update_profile_to_new_unique_name_succeeds(test_db, mock_profiles_dir):
|
||||
"""Test that updating a profile to a new unique name succeeds."""
|
||||
# Create profile
|
||||
profile_data = VoiceProfileCreate(
|
||||
name="Original Name",
|
||||
description="Profile description",
|
||||
language="en"
|
||||
)
|
||||
profile = await create_profile(profile_data, test_db)
|
||||
|
||||
# Update profile with new unique name
|
||||
update_data = VoiceProfileCreate(
|
||||
name="New Unique Name",
|
||||
description="Updated description",
|
||||
language="en"
|
||||
)
|
||||
|
||||
updated_profile = await update_profile(profile.id, update_data, test_db)
|
||||
|
||||
# Verify update succeeded
|
||||
assert updated_profile is not None
|
||||
assert updated_profile.id == profile.id
|
||||
assert updated_profile.name == "New Unique Name"
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_case_sensitive_names_allowed(test_db, mock_profiles_dir):
|
||||
"""Test that profile names are case-sensitive (e.g., 'Test' and 'test' are different)."""
|
||||
# Create profile with lowercase name
|
||||
profile_data_1 = VoiceProfileCreate(
|
||||
name="test profile",
|
||||
description="Lowercase",
|
||||
language="en"
|
||||
)
|
||||
profile_1 = await create_profile(profile_data_1, test_db)
|
||||
|
||||
# Create profile with different case
|
||||
profile_data_2 = VoiceProfileCreate(
|
||||
name="Test Profile",
|
||||
description="Title case",
|
||||
language="en"
|
||||
)
|
||||
profile_2 = await create_profile(profile_data_2, test_db)
|
||||
|
||||
# Both should succeed since SQLite unique constraint is case-sensitive by default
|
||||
assert profile_1.name == "test profile"
|
||||
assert profile_2.name == "Test Profile"
|
||||
assert profile_1.id != profile_2.id
|
||||
Reference in New Issue
Block a user