fix(qa): address qa-tester findings — mock target_column, FetchModelsRequest schema, error handling
- Fix MagicMock poisoning: add target_column=None to _make_mock_job/mock_job fixtures (3 files) - Add FetchModelsRequest Pydantic model for type-safe fetch-models endpoint - logger.warning→logger.error for fetch_models 502 path - Early validation: require api_key or provider_id in fetch-models - 50 tests pass, 0 failures (test_clickhouse_insert_integration, test_orchestrator, test_preview)
This commit is contained in:
@@ -8,6 +8,7 @@
|
||||
|
||||
from fastapi import APIRouter, Depends, HTTPException, status
|
||||
from typing import List, Optional
|
||||
from pydantic import BaseModel, Field
|
||||
from ...core.logger import logger
|
||||
from ...schemas.auth import User
|
||||
from ...dependencies import get_current_user as get_current_active_user
|
||||
@@ -16,6 +17,18 @@ from ...services.llm_provider import LLMProviderService
|
||||
from ...core.database import get_db
|
||||
from sqlalchemy.orm import Session
|
||||
|
||||
|
||||
# #region FetchModelsRequest [C:1] [TYPE Class]
|
||||
# @BRIEF Pydantic request model for the fetch-models endpoint.
|
||||
class FetchModelsRequest(BaseModel):
|
||||
base_url: Optional[str] = Field(None, description="LLM provider base URL")
|
||||
provider_type: Optional[str] = Field(None, description="Provider type (openai, anthropic, etc)")
|
||||
api_key: Optional[str] = Field(None, description="Direct API key (takes precedence over provider_id)")
|
||||
provider_id: Optional[str] = Field(None, description="Saved provider ID to look up stored API key")
|
||||
|
||||
|
||||
# #endregion FetchModelsRequest
|
||||
|
||||
# #region router [TYPE Global]
|
||||
# @BRIEF APIRouter instance for LLM routes.
|
||||
router = APIRouter(tags=["LLM"])
|
||||
@@ -83,17 +96,24 @@ async def get_providers(
|
||||
# @RELATION CALLS -> [LLMClient]
|
||||
@router.post("/providers/fetch-models")
|
||||
async def fetch_models(
|
||||
payload: dict,
|
||||
payload: FetchModelsRequest,
|
||||
current_user: User = Depends(get_current_active_user),
|
||||
db: Session = Depends(get_db),
|
||||
):
|
||||
from ...plugins.llm_analysis.service import LLMClient
|
||||
from ...plugins.llm_analysis.models import LLMProviderType
|
||||
|
||||
base_url = (payload.get("base_url") or "").strip()
|
||||
provider_type_str = (payload.get("provider_type") or "").strip()
|
||||
api_key = payload.get("api_key") or ""
|
||||
provider_id = payload.get("provider_id") or ""
|
||||
base_url = (payload.base_url or "").strip()
|
||||
provider_type_str = (payload.provider_type or "").strip()
|
||||
api_key = payload.api_key or ""
|
||||
provider_id = payload.provider_id or ""
|
||||
|
||||
# Early validation: at least one of api_key or provider_id is required
|
||||
if not api_key and not provider_id:
|
||||
raise HTTPException(
|
||||
status_code=400,
|
||||
detail="api_key or provider_id is required",
|
||||
)
|
||||
|
||||
# Resolve provider_id to stored credentials if no direct api_key given
|
||||
if not api_key and provider_id:
|
||||
@@ -128,7 +148,7 @@ async def fetch_models(
|
||||
models = await client.fetch_models()
|
||||
return {"models": models}
|
||||
except Exception as e:
|
||||
logger.warning(
|
||||
logger.error(
|
||||
f"[llm_routes.fetch_models] Failed to fetch models: {e}",
|
||||
extra={"src": "llm_routes.fetch_models"},
|
||||
)
|
||||
|
||||
@@ -35,6 +35,7 @@ def _make_mock_job(**overrides):
|
||||
job.target_database_id = "1"
|
||||
job.environment_id = "test-env"
|
||||
job.context_columns = []
|
||||
job.target_column = None
|
||||
for k, v in overrides.items():
|
||||
setattr(job, k, v)
|
||||
return job
|
||||
|
||||
@@ -48,6 +48,7 @@ def mock_job() -> MagicMock:
|
||||
job.provider_id = "provider-1"
|
||||
job.batch_size = 50
|
||||
job.upsert_strategy = "MERGE"
|
||||
job.target_column = None
|
||||
return job
|
||||
|
||||
|
||||
|
||||
@@ -35,6 +35,7 @@ def _make_mock_job(**overrides):
|
||||
job.batch_size = overrides.get("batch_size", 50)
|
||||
job.upsert_strategy = overrides.get("upsert_strategy", "MERGE")
|
||||
job.status = overrides.get("status", "DRAFT")
|
||||
job.target_column = None
|
||||
return job
|
||||
# endregion _make_mock_job
|
||||
|
||||
|
||||
Reference in New Issue
Block a user