mirror of
https://github.com/vectorize-io/hindsight.git
synced 2026-09-14 19:31:49 +08:00
fix(api): declare binary download bodies so generated clients return bytes (#4315)
* fix(api): declare binary download bodies so generated clients return bytes (#4292) GET attachments/{id} and files/download/{key} returned a raw Response with no response_class, so FastAPI added application/json next to the declared binary media type. The generated clients picked JSON and decoded the body as text, corrupting the bytes (UnicodeDecodeError on a PNG's 0x89 magic byte). Set response_class=Response and a string/binary schema on both routes, and regenerate the spec and clients (Python now returns bytearray, TS Blob). Tests: an api-slim spec guard that no 200 response mixes application/json with a binary media type, and a live-server Python client test that retains an inline PNG and fetches it back byte-identical. * chore(docs-skill): regenerate the skill's OpenAPI copy for the binary download schema
This commit is contained in:
@@ -23,7 +23,7 @@ from urllib.parse import quote
|
||||
|
||||
from fastapi import Depends, FastAPI, File, Form, Header, HTTPException, Query, Request, UploadFile
|
||||
from fastapi.middleware.gzip import GZipMiddleware
|
||||
from fastapi.responses import JSONResponse
|
||||
from fastapi.responses import JSONResponse, Response
|
||||
|
||||
from hindsight_api.api import page_markdown
|
||||
from hindsight_api.api.disconnect import ClientDisconnectCancellationMiddleware, get_scope_cancellation_token
|
||||
@@ -886,6 +886,11 @@ def bank_attachment_url(bank_id: str, attachment_id: str) -> str:
|
||||
return f"/v1/default/banks/{quote(bank_id, safe='')}/attachments/{attachment_id}"
|
||||
|
||||
|
||||
# OpenAPI content entry for a raw-bytes response body, so generated clients
|
||||
# return bytes instead of trying to decode the payload.
|
||||
_BINARY_SCHEMA: dict[str, Any] = {"schema": {"type": "string", "format": "binary"}}
|
||||
|
||||
|
||||
def chunk_attachments_of(
|
||||
bank_id: str,
|
||||
text: str,
|
||||
@@ -8269,7 +8274,11 @@ def _register_routes(app: FastAPI):
|
||||
"cannot be used to probe what a bank holds.",
|
||||
operation_id="get_bank_attachment",
|
||||
tags=["Memory"],
|
||||
responses={200: {"content": {"application/octet-stream": {}}, "description": "Attachment bytes"}},
|
||||
# An explicit response_class stops FastAPI adding its default
|
||||
# application/json media type next to the binary one, which made the
|
||||
# generated clients decode the bytes as JSON text (#4292).
|
||||
response_class=Response,
|
||||
responses={200: {"content": {"application/octet-stream": _BINARY_SCHEMA}, "description": "Attachment bytes"}},
|
||||
)
|
||||
async def api_get_bank_attachment(
|
||||
bank_id: str,
|
||||
@@ -8277,7 +8286,6 @@ def _register_routes(app: FastAPI):
|
||||
request_context: RequestContext = Depends(get_request_context),
|
||||
):
|
||||
"""Serve one of a bank's retained inline attachments."""
|
||||
from fastapi.responses import Response
|
||||
|
||||
try:
|
||||
attachment = await app.state.memory.retrieve_bank_attachment(bank_id, attachment_id, request_context)
|
||||
@@ -8316,14 +8324,14 @@ def _register_routes(app: FastAPI):
|
||||
"download_url). Access is authorized against the bank the key belongs to.",
|
||||
operation_id="download_file",
|
||||
tags=["Document Transfer"],
|
||||
responses={200: {"content": {"application/zip": {}}, "description": "Stored file"}},
|
||||
response_class=Response,
|
||||
responses={200: {"content": {"application/zip": _BINARY_SCHEMA}, "description": "Stored file"}},
|
||||
)
|
||||
async def api_download_file(
|
||||
key: str,
|
||||
request_context: RequestContext = Depends(get_request_context),
|
||||
):
|
||||
"""Download a bank-scoped stored file (export archive) by storage key."""
|
||||
from fastapi.responses import Response
|
||||
|
||||
try:
|
||||
if not get_config().enable_document_export_api:
|
||||
|
||||
@@ -0,0 +1,41 @@
|
||||
"""Binary download endpoints must publish only their binary media type (#4292).
|
||||
|
||||
A route that returns a raw ``Response`` without ``response_class`` gets FastAPI's
|
||||
default ``application/json`` media type added next to the declared one. The
|
||||
generated SDKs pick JSON first and decode the body as text, which corrupts the
|
||||
bytes (a PNG attachment fails with ``UnicodeDecodeError`` on its 0x89 magic byte).
|
||||
"""
|
||||
|
||||
from types import SimpleNamespace
|
||||
|
||||
from hindsight_api.api import create_app
|
||||
|
||||
_TEXT_MEDIA_PREFIXES = ("application/json", "text/")
|
||||
|
||||
|
||||
def test_no_success_response_mixes_json_with_a_binary_media_type():
|
||||
spec = create_app(SimpleNamespace(audit_logger=None), initialize_memory=False).openapi()
|
||||
|
||||
offenders = []
|
||||
for path, operations in spec["paths"].items():
|
||||
for method, operation in operations.items():
|
||||
content = operation.get("responses", {}).get("200", {}).get("content", {})
|
||||
binary = [media for media in content if not media.startswith(_TEXT_MEDIA_PREFIXES)]
|
||||
if binary and "application/json" in content:
|
||||
offenders.append(f"{method.upper()} {path}: {sorted(content)}")
|
||||
|
||||
assert not offenders, "binary endpoints also declare application/json:\n" + "\n".join(offenders)
|
||||
|
||||
|
||||
def test_download_endpoints_declare_a_binary_schema():
|
||||
"""``format: binary`` is what makes the generated clients return bytes, not ``object``."""
|
||||
spec = create_app(SimpleNamespace(audit_logger=None), initialize_memory=False).openapi()
|
||||
expected = {
|
||||
"/v1/default/banks/{bank_id}/attachments/{attachment_id}": "application/octet-stream",
|
||||
"/v1/default/files/download/{key}": "application/zip",
|
||||
}
|
||||
|
||||
for path, media_type in expected.items():
|
||||
content = spec["paths"][path]["get"]["responses"]["200"]["content"]
|
||||
assert list(content) == [media_type]
|
||||
assert content[media_type]["schema"] == {"type": "string", "format": "binary"}
|
||||
@@ -3634,9 +3634,10 @@ paths:
|
||||
responses:
|
||||
"200":
|
||||
content:
|
||||
application/json:
|
||||
schema: {}
|
||||
application/octet-stream: {}
|
||||
application/octet-stream:
|
||||
schema:
|
||||
format: binary
|
||||
type: string
|
||||
description: Attachment bytes
|
||||
"422":
|
||||
content:
|
||||
@@ -3674,9 +3675,10 @@ paths:
|
||||
responses:
|
||||
"200":
|
||||
content:
|
||||
application/json:
|
||||
schema: {}
|
||||
application/zip: {}
|
||||
application/zip:
|
||||
schema:
|
||||
format: binary
|
||||
type: string
|
||||
description: Stored file
|
||||
"422":
|
||||
content:
|
||||
|
||||
@@ -17,8 +17,8 @@ import (
|
||||
"net/http"
|
||||
"net/url"
|
||||
"strings"
|
||||
"reflect"
|
||||
"os"
|
||||
"reflect"
|
||||
)
|
||||
|
||||
|
||||
@@ -37,7 +37,7 @@ func (r ApiDownloadFileRequest) Authorization(authorization string) ApiDownloadF
|
||||
return r
|
||||
}
|
||||
|
||||
func (r ApiDownloadFileRequest) Execute() (interface{}, *http.Response, error) {
|
||||
func (r ApiDownloadFileRequest) Execute() (*os.File, *http.Response, error) {
|
||||
return r.ApiService.DownloadFileExecute(r)
|
||||
}
|
||||
|
||||
@@ -59,13 +59,13 @@ func (a *DocumentTransferAPIService) DownloadFile(ctx context.Context, key strin
|
||||
}
|
||||
|
||||
// Execute executes the request
|
||||
// @return interface{}
|
||||
func (a *DocumentTransferAPIService) DownloadFileExecute(r ApiDownloadFileRequest) (interface{}, *http.Response, error) {
|
||||
// @return *os.File
|
||||
func (a *DocumentTransferAPIService) DownloadFileExecute(r ApiDownloadFileRequest) (*os.File, *http.Response, error) {
|
||||
var (
|
||||
localVarHTTPMethod = http.MethodGet
|
||||
localVarPostBody interface{}
|
||||
formFiles []formFile
|
||||
localVarReturnValue interface{}
|
||||
localVarReturnValue *os.File
|
||||
)
|
||||
|
||||
localBasePath, err := a.client.cfg.ServerURLWithContext(r.ctx, "DocumentTransferAPIService.DownloadFile")
|
||||
@@ -90,7 +90,7 @@ func (a *DocumentTransferAPIService) DownloadFileExecute(r ApiDownloadFileReques
|
||||
}
|
||||
|
||||
// to determine the Accept header
|
||||
localVarHTTPHeaderAccepts := []string{"application/json", "application/zip"}
|
||||
localVarHTTPHeaderAccepts := []string{"application/zip", "application/json"}
|
||||
|
||||
// set Accept header
|
||||
localVarHTTPHeaderAccept := selectHeaderAccept(localVarHTTPHeaderAccepts)
|
||||
|
||||
@@ -17,6 +17,7 @@ import (
|
||||
"net/http"
|
||||
"net/url"
|
||||
"strings"
|
||||
"os"
|
||||
"reflect"
|
||||
)
|
||||
|
||||
@@ -428,7 +429,7 @@ func (r ApiGetBankAttachmentRequest) Authorization(authorization string) ApiGetB
|
||||
return r
|
||||
}
|
||||
|
||||
func (r ApiGetBankAttachmentRequest) Execute() (interface{}, *http.Response, error) {
|
||||
func (r ApiGetBankAttachmentRequest) Execute() (*os.File, *http.Response, error) {
|
||||
return r.ApiService.GetBankAttachmentExecute(r)
|
||||
}
|
||||
|
||||
@@ -454,13 +455,13 @@ func (a *MemoryAPIService) GetBankAttachment(ctx context.Context, bankId string,
|
||||
}
|
||||
|
||||
// Execute executes the request
|
||||
// @return interface{}
|
||||
func (a *MemoryAPIService) GetBankAttachmentExecute(r ApiGetBankAttachmentRequest) (interface{}, *http.Response, error) {
|
||||
// @return *os.File
|
||||
func (a *MemoryAPIService) GetBankAttachmentExecute(r ApiGetBankAttachmentRequest) (*os.File, *http.Response, error) {
|
||||
var (
|
||||
localVarHTTPMethod = http.MethodGet
|
||||
localVarPostBody interface{}
|
||||
formFiles []formFile
|
||||
localVarReturnValue interface{}
|
||||
localVarReturnValue *os.File
|
||||
)
|
||||
|
||||
localBasePath, err := a.client.cfg.ServerURLWithContext(r.ctx, "MemoryAPIService.GetBankAttachment")
|
||||
@@ -486,7 +487,7 @@ func (a *MemoryAPIService) GetBankAttachmentExecute(r ApiGetBankAttachmentReques
|
||||
}
|
||||
|
||||
// to determine the Accept header
|
||||
localVarHTTPHeaderAccepts := []string{"application/json", "application/octet-stream"}
|
||||
localVarHTTPHeaderAccepts := []string{"application/octet-stream", "application/json"}
|
||||
|
||||
// set Accept header
|
||||
localVarHTTPHeaderAccept := selectHeaderAccept(localVarHTTPHeaderAccepts)
|
||||
|
||||
@@ -57,7 +57,7 @@ class DocumentTransferApi:
|
||||
_content_type: Optional[StrictStr] = None,
|
||||
_headers: Optional[Dict[StrictStr, Any]] = None,
|
||||
_host_index: Annotated[StrictInt, Field(ge=0, le=0)] = 0,
|
||||
) -> object:
|
||||
) -> bytearray:
|
||||
"""Download a stored file (async export archive)
|
||||
|
||||
Stream a file previously written to file storage — currently the transfer ZIP produced by an async document export. The key comes from the export operation's result_metadata (storage_key / download_url). Access is authorized against the bank the key belongs to.
|
||||
@@ -98,7 +98,7 @@ class DocumentTransferApi:
|
||||
)
|
||||
|
||||
_response_types_map: Dict[str, Optional[str]] = {
|
||||
'200': "object",
|
||||
'200': "bytearray",
|
||||
'422': "HTTPValidationError",
|
||||
}
|
||||
response_data = await self.api_client.call_api(
|
||||
@@ -129,7 +129,7 @@ class DocumentTransferApi:
|
||||
_content_type: Optional[StrictStr] = None,
|
||||
_headers: Optional[Dict[StrictStr, Any]] = None,
|
||||
_host_index: Annotated[StrictInt, Field(ge=0, le=0)] = 0,
|
||||
) -> ApiResponse[object]:
|
||||
) -> ApiResponse[bytearray]:
|
||||
"""Download a stored file (async export archive)
|
||||
|
||||
Stream a file previously written to file storage — currently the transfer ZIP produced by an async document export. The key comes from the export operation's result_metadata (storage_key / download_url). Access is authorized against the bank the key belongs to.
|
||||
@@ -170,7 +170,7 @@ class DocumentTransferApi:
|
||||
)
|
||||
|
||||
_response_types_map: Dict[str, Optional[str]] = {
|
||||
'200': "object",
|
||||
'200': "bytearray",
|
||||
'422': "HTTPValidationError",
|
||||
}
|
||||
response_data = await self.api_client.call_api(
|
||||
@@ -242,7 +242,7 @@ class DocumentTransferApi:
|
||||
)
|
||||
|
||||
_response_types_map: Dict[str, Optional[str]] = {
|
||||
'200': "object",
|
||||
'200': "bytearray",
|
||||
'422': "HTTPValidationError",
|
||||
}
|
||||
response_data = await self.api_client.call_api(
|
||||
@@ -291,8 +291,8 @@ class DocumentTransferApi:
|
||||
if 'Accept' not in _header_params:
|
||||
_header_params['Accept'] = self.api_client.select_header_accept(
|
||||
[
|
||||
'application/json',
|
||||
'application/zip'
|
||||
'application/zip',
|
||||
'application/json'
|
||||
]
|
||||
)
|
||||
|
||||
|
||||
@@ -16,8 +16,8 @@ from pydantic import validate_call, Field, StrictFloat, StrictStr, StrictInt
|
||||
from typing import Any, Dict, List, Optional, Tuple, Union
|
||||
from typing_extensions import Annotated
|
||||
|
||||
from pydantic import Field, StrictStr, field_validator
|
||||
from typing import Any, List, Optional
|
||||
from pydantic import Field, StrictBytes, StrictStr, field_validator
|
||||
from typing import Any, List, Optional, Tuple, Union
|
||||
from typing_extensions import Annotated
|
||||
from hindsight_client_api.models.clear_memory_observations_response import ClearMemoryObservationsResponse
|
||||
from hindsight_client_api.models.delete_response import DeleteResponse
|
||||
@@ -965,7 +965,7 @@ class MemoryApi:
|
||||
_content_type: Optional[StrictStr] = None,
|
||||
_headers: Optional[Dict[StrictStr, Any]] = None,
|
||||
_host_index: Annotated[StrictInt, Field(ge=0, le=0)] = 0,
|
||||
) -> object:
|
||||
) -> bytearray:
|
||||
"""Fetch an attachment retained inline with a document
|
||||
|
||||
Serve the bytes of an attachment retained as inline content. The id is the one inside a placeholder token, and is returned on `attachments[].url` by recall and by the document/chunk/memory reads — so an agent can show or reason over the original behind an attachment-derived fact. Bytes are served with the Content-Type the caller declared at retain. Access is authorized against the bank; a missing attachment and an invisible bank both return 404, so the endpoint cannot be used to probe what a bank holds.
|
||||
@@ -1009,7 +1009,7 @@ class MemoryApi:
|
||||
)
|
||||
|
||||
_response_types_map: Dict[str, Optional[str]] = {
|
||||
'200': "object",
|
||||
'200': "bytearray",
|
||||
'422': "HTTPValidationError",
|
||||
}
|
||||
response_data = await self.api_client.call_api(
|
||||
@@ -1041,7 +1041,7 @@ class MemoryApi:
|
||||
_content_type: Optional[StrictStr] = None,
|
||||
_headers: Optional[Dict[StrictStr, Any]] = None,
|
||||
_host_index: Annotated[StrictInt, Field(ge=0, le=0)] = 0,
|
||||
) -> ApiResponse[object]:
|
||||
) -> ApiResponse[bytearray]:
|
||||
"""Fetch an attachment retained inline with a document
|
||||
|
||||
Serve the bytes of an attachment retained as inline content. The id is the one inside a placeholder token, and is returned on `attachments[].url` by recall and by the document/chunk/memory reads — so an agent can show or reason over the original behind an attachment-derived fact. Bytes are served with the Content-Type the caller declared at retain. Access is authorized against the bank; a missing attachment and an invisible bank both return 404, so the endpoint cannot be used to probe what a bank holds.
|
||||
@@ -1085,7 +1085,7 @@ class MemoryApi:
|
||||
)
|
||||
|
||||
_response_types_map: Dict[str, Optional[str]] = {
|
||||
'200': "object",
|
||||
'200': "bytearray",
|
||||
'422': "HTTPValidationError",
|
||||
}
|
||||
response_data = await self.api_client.call_api(
|
||||
@@ -1161,7 +1161,7 @@ class MemoryApi:
|
||||
)
|
||||
|
||||
_response_types_map: Dict[str, Optional[str]] = {
|
||||
'200': "object",
|
||||
'200': "bytearray",
|
||||
'422': "HTTPValidationError",
|
||||
}
|
||||
response_data = await self.api_client.call_api(
|
||||
@@ -1213,8 +1213,8 @@ class MemoryApi:
|
||||
if 'Accept' not in _header_params:
|
||||
_header_params['Accept'] = self.api_client.select_header_accept(
|
||||
[
|
||||
'application/json',
|
||||
'application/octet-stream'
|
||||
'application/octet-stream',
|
||||
'application/json'
|
||||
]
|
||||
)
|
||||
|
||||
|
||||
@@ -0,0 +1,65 @@
|
||||
"""
|
||||
The generated client returns an inline attachment's bytes unchanged (#4292).
|
||||
|
||||
The spec used to list ``application/json`` next to ``application/octet-stream``
|
||||
for this endpoint, so the generated client decoded the body as UTF-8 text and a
|
||||
PNG failed on its 0x89 magic byte. This goes through the real server and the
|
||||
generated method so a spec regression shows up as a client failure.
|
||||
|
||||
These tests require a running Hindsight API server.
|
||||
"""
|
||||
|
||||
import base64
|
||||
import os
|
||||
import uuid
|
||||
|
||||
import pytest
|
||||
|
||||
from hindsight_client import Hindsight
|
||||
from hindsight_client.hindsight_client import _run_async
|
||||
|
||||
HINDSIGHT_API_URL = os.getenv("HINDSIGHT_API_URL", "http://localhost:8888")
|
||||
|
||||
PNG_BYTES = base64.b64decode(
|
||||
"iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAAAADUlEQVR42mP8z8BQDwAEhQGAhKmMIQAAAABJRU5ErkJggg=="
|
||||
)
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def client():
|
||||
with Hindsight(base_url=HINDSIGHT_API_URL) as client:
|
||||
yield client
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def bank_id(client):
|
||||
bid = f"test_bank_{uuid.uuid4().hex[:12]}"
|
||||
yield bid
|
||||
try:
|
||||
client.delete_bank(bank_id=bid)
|
||||
except Exception:
|
||||
# Best-effort cleanup: must not mask the test result.
|
||||
pass
|
||||
|
||||
|
||||
def test_inline_image_round_trips_byte_identical(client, bank_id):
|
||||
client.retain(
|
||||
bank_id=bank_id,
|
||||
document_id="d1",
|
||||
content=[
|
||||
{"type": "text", "text": "Alice stood in front of the Brandenburg Gate."},
|
||||
{
|
||||
"type": "image",
|
||||
"source": {"type": "base64", "media_type": "image/png", "data": base64.b64encode(PNG_BYTES).decode()},
|
||||
},
|
||||
],
|
||||
)
|
||||
|
||||
document = _run_async(client._documents_api.get_document(bank_id, "d1"))
|
||||
assert document.attachments, "the retained document lists no attachments"
|
||||
attachment = document.attachments[0]
|
||||
assert attachment.media_type == "image/png"
|
||||
|
||||
data = _run_async(client._memory_api.get_bank_attachment(bank_id, attachment.id))
|
||||
|
||||
assert bytes(data) == PNG_BYTES
|
||||
@@ -246,6 +246,7 @@ export type {
|
||||
DownloadFileData,
|
||||
DownloadFileError,
|
||||
DownloadFileErrors,
|
||||
DownloadFileResponse,
|
||||
DownloadFileResponses,
|
||||
DryRunExtractionResult,
|
||||
DryRunExtractMemoriesData,
|
||||
@@ -309,6 +310,7 @@ export type {
|
||||
GetBankAttachmentData,
|
||||
GetBankAttachmentError,
|
||||
GetBankAttachmentErrors,
|
||||
GetBankAttachmentResponse,
|
||||
GetBankAttachmentResponses,
|
||||
GetBankConfigData,
|
||||
GetBankConfigError,
|
||||
|
||||
@@ -9480,9 +9480,12 @@ export type GetBankAttachmentResponses = {
|
||||
/**
|
||||
* Attachment bytes
|
||||
*/
|
||||
200: unknown;
|
||||
200: Blob | File;
|
||||
};
|
||||
|
||||
export type GetBankAttachmentResponse =
|
||||
GetBankAttachmentResponses[keyof GetBankAttachmentResponses];
|
||||
|
||||
export type DownloadFileData = {
|
||||
body?: never;
|
||||
headers?: {
|
||||
@@ -9514,9 +9517,11 @@ export type DownloadFileResponses = {
|
||||
/**
|
||||
* Stored file
|
||||
*/
|
||||
200: unknown;
|
||||
200: Blob | File;
|
||||
};
|
||||
|
||||
export type DownloadFileResponse = DownloadFileResponses[keyof DownloadFileResponses];
|
||||
|
||||
export type GetBankTemplateSchemaData = {
|
||||
body?: never;
|
||||
path?: never;
|
||||
|
||||
@@ -5209,10 +5209,12 @@
|
||||
"200": {
|
||||
"description": "Attachment bytes",
|
||||
"content": {
|
||||
"application/json": {
|
||||
"schema": {}
|
||||
},
|
||||
"application/octet-stream": {}
|
||||
"application/octet-stream": {
|
||||
"schema": {
|
||||
"type": "string",
|
||||
"format": "binary"
|
||||
}
|
||||
}
|
||||
}
|
||||
},
|
||||
"422": {
|
||||
@@ -5267,10 +5269,12 @@
|
||||
"200": {
|
||||
"description": "Stored file",
|
||||
"content": {
|
||||
"application/json": {
|
||||
"schema": {}
|
||||
},
|
||||
"application/zip": {}
|
||||
"application/zip": {
|
||||
"schema": {
|
||||
"type": "string",
|
||||
"format": "binary"
|
||||
}
|
||||
}
|
||||
}
|
||||
},
|
||||
"422": {
|
||||
|
||||
@@ -5209,10 +5209,12 @@
|
||||
"200": {
|
||||
"description": "Attachment bytes",
|
||||
"content": {
|
||||
"application/json": {
|
||||
"schema": {}
|
||||
},
|
||||
"application/octet-stream": {}
|
||||
"application/octet-stream": {
|
||||
"schema": {
|
||||
"type": "string",
|
||||
"format": "binary"
|
||||
}
|
||||
}
|
||||
}
|
||||
},
|
||||
"422": {
|
||||
@@ -5267,10 +5269,12 @@
|
||||
"200": {
|
||||
"description": "Stored file",
|
||||
"content": {
|
||||
"application/json": {
|
||||
"schema": {}
|
||||
},
|
||||
"application/zip": {}
|
||||
"application/zip": {
|
||||
"schema": {
|
||||
"type": "string",
|
||||
"format": "binary"
|
||||
}
|
||||
}
|
||||
}
|
||||
},
|
||||
"422": {
|
||||
|
||||
Reference in New Issue
Block a user