fix(viz_routes): address PR #767 review — param parity + always-on page_number
- Replace bare int() casts for start/end/context_chars in chunk_context_endpoint with _parse_int_param, matching visualization.py bounds (0–10M for offsets, 0–10K for context_chars), and add the missing end > start guard. - Initialize page_number from chunk_context.page_number so non-file doc_types surface it; include page_number, chunk_index, and total_chunks unconditionally in the response. Only highlighted_page_image stays gated on its own truthiness. - Add a chunk_index forwarding regression test that asserts the new kwargs reach get_chunk_with_context and appear in the response payload. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 4.7
parent
a33a365a69
commit
53e6dba5a2
@@ -538,7 +538,6 @@ async def chunk_context_endpoint(request: Request) -> JSONResponse:
|
|||||||
end_str = request.query_params.get("end")
|
end_str = request.query_params.get("end")
|
||||||
chunk_index_str = request.query_params.get("chunk_index")
|
chunk_index_str = request.query_params.get("chunk_index")
|
||||||
total_chunks_str = request.query_params.get("total_chunks")
|
total_chunks_str = request.query_params.get("total_chunks")
|
||||||
context_chars = int(request.query_params.get("context", "500"))
|
|
||||||
|
|
||||||
# Validate required parameters
|
# Validate required parameters
|
||||||
if not all([doc_type, doc_id, start_str, end_str]):
|
if not all([doc_type, doc_id, start_str, end_str]):
|
||||||
@@ -556,8 +555,17 @@ async def chunk_context_endpoint(request: Request) -> JSONResponse:
|
|||||||
assert start_str is not None
|
assert start_str is not None
|
||||||
assert end_str is not None
|
assert end_str is not None
|
||||||
|
|
||||||
start = int(start_str)
|
context_chars = _parse_int_param(
|
||||||
end = int(end_str)
|
request.query_params.get("context"),
|
||||||
|
500,
|
||||||
|
0,
|
||||||
|
10000,
|
||||||
|
"context_chars",
|
||||||
|
)
|
||||||
|
start = _parse_int_param(start_str, 0, 0, 10000000, "start")
|
||||||
|
end = _parse_int_param(end_str, 0, 0, 10000000, "end")
|
||||||
|
if end <= start:
|
||||||
|
raise ValueError("end must be greater than start")
|
||||||
chunk_index: int | None = None
|
chunk_index: int | None = None
|
||||||
if chunk_index_str is not None:
|
if chunk_index_str is not None:
|
||||||
chunk_index = _parse_int_param(
|
chunk_index = _parse_int_param(
|
||||||
@@ -617,7 +625,7 @@ async def chunk_context_endpoint(request: Request) -> JSONResponse:
|
|||||||
|
|
||||||
# For PDF files, also fetch the highlighted page image from Qdrant
|
# For PDF files, also fetch the highlighted page image from Qdrant
|
||||||
highlighted_page_image = None
|
highlighted_page_image = None
|
||||||
page_number = None
|
page_number = chunk_context.page_number
|
||||||
if doc_type == "file":
|
if doc_type == "file":
|
||||||
try:
|
try:
|
||||||
settings = get_settings()
|
settings = get_settings()
|
||||||
@@ -697,12 +705,13 @@ async def chunk_context_endpoint(request: Request) -> JSONResponse:
|
|||||||
"after_context": chunk_context.after_context,
|
"after_context": chunk_context.after_context,
|
||||||
"has_more_before": chunk_context.has_before_truncation,
|
"has_more_before": chunk_context.has_before_truncation,
|
||||||
"has_more_after": chunk_context.has_after_truncation,
|
"has_more_after": chunk_context.has_after_truncation,
|
||||||
|
"page_number": page_number,
|
||||||
|
"chunk_index": chunk_context.chunk_index,
|
||||||
|
"total_chunks": chunk_context.total_chunks,
|
||||||
}
|
}
|
||||||
|
|
||||||
# Add image data if available
|
|
||||||
if highlighted_page_image:
|
if highlighted_page_image:
|
||||||
response_data["highlighted_page_image"] = highlighted_page_image
|
response_data["highlighted_page_image"] = highlighted_page_image
|
||||||
response_data["page_number"] = page_number
|
|
||||||
|
|
||||||
return JSONResponse(response_data)
|
return JSONResponse(response_data)
|
||||||
|
|
||||||
|
|||||||
@@ -259,6 +259,58 @@ class TestChunkContextCredentialPath:
|
|||||||
assert "failed to fetch chunk context" in data["error"].lower()
|
assert "failed to fetch chunk context" in data["error"].lower()
|
||||||
|
|
||||||
|
|
||||||
|
class TestChunkContextParameterForwarding:
|
||||||
|
"""Verify new chunk_index / total_chunks query params reach the lookup.
|
||||||
|
|
||||||
|
Regression guard for PR #767: the whole point of the fix is that callers
|
||||||
|
pass chunk_index, and it must arrive at get_chunk_with_context as the
|
||||||
|
primary Qdrant lookup key.
|
||||||
|
"""
|
||||||
|
|
||||||
|
def test_chunk_index_and_total_chunks_forwarded(self):
|
||||||
|
mock_nc_client = _make_mock_nc_client()
|
||||||
|
mock_ctx = _make_mock_chunk_context()
|
||||||
|
mock_ctx.chunk_index = 7
|
||||||
|
mock_ctx.total_chunks = 10
|
||||||
|
|
||||||
|
with (
|
||||||
|
patch(
|
||||||
|
"nextcloud_mcp_server.api.visualization.validate_token_and_get_user",
|
||||||
|
new_callable=AsyncMock,
|
||||||
|
return_value=("testuser", True),
|
||||||
|
),
|
||||||
|
patch(
|
||||||
|
"nextcloud_mcp_server.api.visualization.get_user_client_basic_auth",
|
||||||
|
new_callable=AsyncMock,
|
||||||
|
return_value=mock_nc_client,
|
||||||
|
),
|
||||||
|
patch(
|
||||||
|
"nextcloud_mcp_server.api.visualization.get_chunk_with_context",
|
||||||
|
new_callable=AsyncMock,
|
||||||
|
return_value=mock_ctx,
|
||||||
|
) as mock_get_chunk,
|
||||||
|
):
|
||||||
|
app = create_test_app()
|
||||||
|
client = TestClient(app)
|
||||||
|
response = client.get(
|
||||||
|
"/api/v1/chunk-context?doc_type=note&doc_id=42"
|
||||||
|
"&start=0&end=10&chunk_index=7&total_chunks=10",
|
||||||
|
headers={"Authorization": "Bearer test-token"},
|
||||||
|
)
|
||||||
|
|
||||||
|
assert response.status_code == 200
|
||||||
|
kwargs = mock_get_chunk.await_args.kwargs
|
||||||
|
assert kwargs["chunk_index"] == 7
|
||||||
|
assert kwargs["total_chunks"] == 10
|
||||||
|
|
||||||
|
data = response.json()
|
||||||
|
assert data["chunk_index"] == 7
|
||||||
|
assert data["total_chunks"] == 10
|
||||||
|
# page_number must be present even when None (frontend may scroll
|
||||||
|
# by it for non-file doc types)
|
||||||
|
assert "page_number" in data
|
||||||
|
|
||||||
|
|
||||||
class TestChunkContextConfigErrors:
|
class TestChunkContextConfigErrors:
|
||||||
"""Tests for configuration failure paths."""
|
"""Tests for configuration failure paths."""
|
||||||
|
|
||||||
|
|||||||
Reference in New Issue
Block a user