mirror of
https://github.com/lfnovo/open-notebook.git
synced 2026-10-19 09:13:16 +00:00
Unsupported files used to enqueue a background job that failed and then burned the full 15-attempt retry budget (~1h) before showing a generic "Failed" with no actionable detail. - Add a pre-flight `_assert_file_supported()` using content-core 2.x's header-only `check_file_support()` (same routing as real extraction) in the upload branch of `_build_content_state`, before any job is enqueued and before the source record is created. - Guard the source-retry endpoint the same way. Unexpected check errors (e.g. file removed before a retry) fall through to normal extraction. - Map `UnsupportedTypeException` to `415 Unsupported Media Type` via a dedicated global handler (previously fell through to the base OpenNotebookError handler's 500). The message names the detected type. Part of #939.
This commit is contained in:
parent
3a509a05a0
commit
faf0f0e65d
5 changed files with 127 additions and 0 deletions
|
|
@ -17,6 +17,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
|
|||
- The legacy provider/model string fields on podcast profiles (`outline_provider`, `outline_model`, `transcript_provider`, `transcript_model` on episode profiles; `tts_provider`, `tts_model` on speaker profiles) are gone from the database, the API and the UI — the app has ignored them since the Model registry references landed in v1.11. Migration 22 first best-effort maps any profile whose `outline_llm`/`transcript_llm`/`voice_model` reference is still empty to an existing model record (matching provider + name + type; no auto-creation, since a migration must not touch credentials), then drops the six columns; the startup data migration that used to retry this mapping on every boot (`open_notebook/podcasts/migration.py`) was deleted. Accepted trade-off: profiles whose mapping never converged (e.g. the provider credential was never configured) lose the legacy strings and stay unresolved — they were already non-functional and the UI already flags them as needing model selection, so you just re-pick the models in the profile form once (#1107)
|
||||
|
||||
### Fixed
|
||||
- Uploading a file content-core can't extract now fails immediately at ingestion with a clear `415 Unsupported Media Type` error that names the detected MIME type, instead of enqueueing a background job that retried up to 15 times over ~1 hour before surfacing a generic "Failed" with no actionable detail. The pre-flight uses content-core 2.x's header-only `check_file_support()` — the same routing real extraction uses, so the verdict can't disagree with what would happen downstream — and the source-retry endpoint is guarded the same way; unexpected check errors (e.g. a file removed before a retry) fall through to normal extraction rather than becoming a hard rejection (#975)
|
||||
- Podcast episode cards no longer show "— / —" for the outline, transcript and speaker model rows on new episodes: the API now resolves the snapshot's model references (`outline_llm`/`transcript_llm`/`voice_model`) to provider/name display fields at serialization time — batched into a single query per request, so listing episodes never does a per-row model lookup — and the card falls back to the legacy snapshot strings for old episodes and degrades to "—" when a referenced model was deleted (#1114)
|
||||
- Renaming a speaker profile no longer breaks the episode profiles that use it: `episode_profile.speaker_config` now stores a `record<speaker_profile>` reference instead of the profile name (migration 20 converts existing rows; references whose speaker profile no longer exists at migration time become null, and any reference that later stops resolving is treated as "needs setup" — the UI asks you to pick a speaker again). The `POST /api/podcasts/generate` contract is unchanged — it still accepts the speaker profile by name and resolves it at the API boundary (#630)
|
||||
- Clicking a chat/Ask citation that points at a deleted source, insight, or note now shows a shared, friendly "this content no longer exists" state in all three dialogs (instead of a raw error, a blank dialog, or an empty editable note editor), 404 lookups are no longer retried, and non-404 failures show a distinct "unable to load" message (#455)
|
||||
|
|
|
|||
12
api/main.py
12
api/main.py
|
|
@ -48,6 +48,7 @@ from open_notebook.exceptions import (
|
|||
NotFoundError,
|
||||
OpenNotebookError,
|
||||
RateLimitError,
|
||||
UnsupportedTypeException,
|
||||
)
|
||||
from open_notebook.utils.encryption import get_secret_from_env
|
||||
|
||||
|
|
@ -350,6 +351,17 @@ async def external_service_error_handler(request: Request, exc: ExternalServiceE
|
|||
)
|
||||
|
||||
|
||||
@app.exception_handler(UnsupportedTypeException)
|
||||
async def unsupported_type_error_handler(
|
||||
request: Request, exc: UnsupportedTypeException
|
||||
):
|
||||
return JSONResponse(
|
||||
status_code=415,
|
||||
content={"detail": str(exc)},
|
||||
headers=_cors_headers(request),
|
||||
)
|
||||
|
||||
|
||||
@app.exception_handler(OpenNotebookError)
|
||||
async def open_notebook_error_handler(request: Request, exc: OpenNotebookError):
|
||||
return JSONResponse(
|
||||
|
|
|
|||
|
|
@ -3,6 +3,7 @@ import os
|
|||
from pathlib import Path
|
||||
from typing import Any, List, Optional
|
||||
|
||||
from content_core import check_file_support
|
||||
from fastapi import (
|
||||
APIRouter,
|
||||
Depends,
|
||||
|
|
@ -39,10 +40,37 @@ from open_notebook.exceptions import (
|
|||
InvalidInputError,
|
||||
NotFoundError,
|
||||
OpenNotebookError,
|
||||
UnsupportedTypeException,
|
||||
)
|
||||
|
||||
router = APIRouter()
|
||||
|
||||
|
||||
async def _assert_file_supported(file_path: str) -> None:
|
||||
"""Pre-flight check that content-core can actually extract this file.
|
||||
|
||||
Rejects unsupported uploads at ingestion time (HTTP 415) instead of letting
|
||||
them enqueue a background job that fails and then burns the full retry
|
||||
budget before surfacing a generic error (#975). Uses content-core's
|
||||
header-only routing, which is the same logic real extraction uses, so the
|
||||
verdict can't disagree with what would happen downstream.
|
||||
|
||||
Unexpected errors (e.g. the file no longer exists on a retry) are swallowed
|
||||
so this never turns a transient problem into a hard rejection — real
|
||||
extraction will surface those.
|
||||
"""
|
||||
try:
|
||||
support = await check_file_support(file_path)
|
||||
except Exception as e: # pragma: no cover - defensive
|
||||
logger.debug(f"Pre-flight file-support check skipped for {file_path}: {e}")
|
||||
return
|
||||
|
||||
if not support.supported:
|
||||
detail = support.reason or "Unsupported file type"
|
||||
if support.identified_type:
|
||||
detail = f"{detail} (detected type: {support.identified_type})"
|
||||
raise UnsupportedTypeException(detail)
|
||||
|
||||
SOURCE_SORT_FIELDS = {
|
||||
"created": "created",
|
||||
"updated": "updated",
|
||||
|
|
@ -416,6 +444,8 @@ async def _build_content_state(
|
|||
status_code=400,
|
||||
detail="Invalid file path: must be within the uploads directory",
|
||||
)
|
||||
# Reject unsupported files before enqueueing a doomed background job.
|
||||
await _assert_file_supported(final_file_path)
|
||||
content_state["file_path"] = final_file_path
|
||||
content_state["delete_source"] = source_data.delete_source
|
||||
elif source_data.type == "text":
|
||||
|
|
@ -931,6 +961,8 @@ async def retry_source_processing(source_id: str):
|
|||
content_state = {}
|
||||
if source.asset:
|
||||
if source.asset.file_path:
|
||||
# Don't re-queue a retry for a file content-core can't extract.
|
||||
await _assert_file_supported(source.asset.file_path)
|
||||
content_state = {
|
||||
"file_path": source.asset.file_path,
|
||||
"delete_source": False, # Don't delete on retry
|
||||
|
|
|
|||
|
|
@ -91,6 +91,28 @@ class TestNotFoundErrorPropagation:
|
|||
assert response.json()["detail"] == "Credential not found"
|
||||
|
||||
|
||||
class TestUnsupportedTypeErrorPropagation:
|
||||
"""UnsupportedTypeException maps to 415 (Unsupported Media Type) via its
|
||||
dedicated global handler, instead of falling through to the base
|
||||
OpenNotebookError handler's 500 (#975)."""
|
||||
|
||||
def test_unsupported_type_maps_to_415(self, client):
|
||||
from open_notebook.exceptions import UnsupportedTypeException
|
||||
|
||||
with patch(
|
||||
"api.routers.sources.repo_query",
|
||||
new=AsyncMock(
|
||||
side_effect=UnsupportedTypeException(
|
||||
"Unsupported file type: application/zip"
|
||||
)
|
||||
),
|
||||
):
|
||||
response = client.get("/api/sources")
|
||||
|
||||
assert response.status_code == 415
|
||||
assert "application/zip" in response.json()["detail"]
|
||||
|
||||
|
||||
class TestUntypedExceptionsStillSanitized:
|
||||
"""The final `except Exception` arm must keep catching untyped errors and
|
||||
returning a fixed, sanitized 500 (never the raw exception text)."""
|
||||
|
|
|
|||
|
|
@ -92,3 +92,63 @@ class TestContentCoreRejectsUnrecognizedContent:
|
|||
detector = FileDetector()
|
||||
with pytest.raises(UnsupportedTypeException):
|
||||
await detector.detect(str(garbage))
|
||||
|
||||
|
||||
class TestPreflightFileSupport:
|
||||
"""The upload flow rejects unsupported files at ingestion via content-core's
|
||||
check_file_support(), so they never enqueue a doomed background job (#975)."""
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_unsupported_verdict_raises(self):
|
||||
from content_core import FileSupport
|
||||
|
||||
from api.routers.sources import _assert_file_supported
|
||||
from open_notebook.exceptions import UnsupportedTypeException
|
||||
|
||||
verdict = FileSupport(
|
||||
supported=False,
|
||||
file_path="/uploads/archive.zip",
|
||||
identified_type="application/zip",
|
||||
reason="Unsupported file type: application/zip",
|
||||
)
|
||||
with patch(
|
||||
"api.routers.sources.check_file_support",
|
||||
new=AsyncMock(return_value=verdict),
|
||||
):
|
||||
with pytest.raises(UnsupportedTypeException) as exc:
|
||||
await _assert_file_supported("/uploads/archive.zip")
|
||||
|
||||
# message carries both the reason and the detected type for the UI
|
||||
assert "application/zip" in str(exc.value)
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_supported_verdict_passes(self):
|
||||
from content_core import FileSupport
|
||||
|
||||
from api.routers.sources import _assert_file_supported
|
||||
|
||||
verdict = FileSupport(
|
||||
supported=True,
|
||||
file_path="/uploads/doc.pdf",
|
||||
identified_type="application/pdf",
|
||||
processor="pdf",
|
||||
)
|
||||
with patch(
|
||||
"api.routers.sources.check_file_support",
|
||||
new=AsyncMock(return_value=verdict),
|
||||
):
|
||||
# must not raise
|
||||
await _assert_file_supported("/uploads/doc.pdf")
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_check_errors_are_swallowed(self):
|
||||
"""A missing file on retry (or any check error) must not become a hard
|
||||
415 — extraction downstream will surface the real problem."""
|
||||
from api.routers.sources import _assert_file_supported
|
||||
|
||||
with patch(
|
||||
"api.routers.sources.check_file_support",
|
||||
new=AsyncMock(side_effect=FileNotFoundError("gone")),
|
||||
):
|
||||
# must not raise
|
||||
await _assert_file_supported("/uploads/missing.pdf")
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue