mirror of
https://github.com/suitenumerique/docs.git
synced 2026-10-01 05:55:16 +02:00
🥅(docspec) handle DocSpec 4xx/5xx import errors distinctly
DocSpec now returns 415 when the file type is rejected, 422 when it can't process the file content, and 500 when it crashes, instead of one generic failure. Propagate those statuses through the API and show a matching toast message per case on the frontend. Closes #2552
This commit is contained in:
@@ -73,6 +73,7 @@ and this project adheres to
|
||||
`/external_api/{version}/jwks`
|
||||
- 🔧(collaboration) adapt docker stack for development purpose
|
||||
- 🔧(helm) run a valkey for the backend and one for yhub in dev and feature
|
||||
- ✨(docspec) handle DocSpec 4xx/5xx import errors distinctly #2552
|
||||
|
||||
### Fixed
|
||||
|
||||
|
||||
@@ -57,6 +57,12 @@ from core.services.converter_services import (
|
||||
from core.services.converter_services import (
|
||||
ServiceUnavailableError as YProviderServiceUnavailableError,
|
||||
)
|
||||
from core.services.converter_services import (
|
||||
UnprocessableContentError as YProviderUnprocessableContentError,
|
||||
)
|
||||
from core.services.converter_services import (
|
||||
UnsupportedMediaTypeError as YProviderUnsupportedMediaTypeError,
|
||||
)
|
||||
from core.services.converter_services import (
|
||||
ValidationError as YProviderValidationError,
|
||||
)
|
||||
@@ -730,9 +736,21 @@ class DocumentViewSet(
|
||||
self.request.user,
|
||||
{"content_type": uploaded_file.content_type},
|
||||
)
|
||||
except YProviderUnsupportedMediaTypeError as err:
|
||||
logger.error("could not convert file content with error: %s", err)
|
||||
exc = drf.exceptions.APIException({"file": ["File type is not supported"]})
|
||||
exc.status_code = status.HTTP_415_UNSUPPORTED_MEDIA_TYPE
|
||||
raise exc from err
|
||||
except YProviderUnprocessableContentError as err:
|
||||
logger.error("could not convert file content with error: %s", err)
|
||||
exc = drf.exceptions.APIException(
|
||||
{"file": ["Could not process file content"]}
|
||||
)
|
||||
exc.status_code = status.HTTP_422_UNPROCESSABLE_ENTITY
|
||||
raise exc from err
|
||||
except ConversionError as err:
|
||||
logger.error("could not convert file content with error: %s", err)
|
||||
raise drf.exceptions.ValidationError(
|
||||
raise drf.exceptions.APIException(
|
||||
{"file": ["Could not convert file content"]}
|
||||
) from err
|
||||
|
||||
|
||||
@@ -26,6 +26,14 @@ class ServiceUnavailableError(ConversionError):
|
||||
"""Raised when the conversion service is unavailable."""
|
||||
|
||||
|
||||
class UnsupportedMediaTypeError(ConversionError):
|
||||
"""Raised when the conversion service rejects the input file type (HTTP 415)."""
|
||||
|
||||
|
||||
class UnprocessableContentError(ConversionError):
|
||||
"""Raised when the conversion service cannot process the file content (HTTP 422)."""
|
||||
|
||||
|
||||
class ConverterProtocol(typing.Protocol):
|
||||
"""Protocol for converter classes."""
|
||||
|
||||
@@ -99,6 +107,20 @@ class DocSpecConverter:
|
||||
|
||||
try:
|
||||
return self._request(settings.DOCSPEC_API_URL, data, content_type).content
|
||||
except requests.HTTPError as err:
|
||||
status_code = err.response.status_code if err.response is not None else None
|
||||
if status_code == 415:
|
||||
raise UnsupportedMediaTypeError(
|
||||
"DocSpec rejected the file type",
|
||||
) from err
|
||||
if status_code == 422:
|
||||
raise UnprocessableContentError(
|
||||
"DocSpec could not process the file content",
|
||||
) from err
|
||||
logger.exception("DocSpec service error: url=%s", settings.DOCSPEC_API_URL)
|
||||
raise ServiceUnavailableError(
|
||||
"Failed to connect to DocSpec conversion service",
|
||||
) from err
|
||||
except requests.RequestException as err:
|
||||
logger.exception("DocSpec service error: url=%s", settings.DOCSPEC_API_URL)
|
||||
raise ServiceUnavailableError(
|
||||
|
||||
@@ -14,6 +14,8 @@ from core.services import mime_types
|
||||
from core.services.converter_services import (
|
||||
ConversionError,
|
||||
ServiceUnavailableError,
|
||||
UnprocessableContentError,
|
||||
UnsupportedMediaTypeError,
|
||||
)
|
||||
from core.services.yhub_services import (
|
||||
ServiceUnavailableError as YHubServiceUnavailableError,
|
||||
@@ -290,7 +292,8 @@ def test_api_documents_create_with_empty_file(settings):
|
||||
@patch("core.services.converter_services.Converter.convert")
|
||||
def test_api_documents_create_with_file_conversion_error(mock_convert, settings):
|
||||
"""
|
||||
When conversion fails, the API should return a 400 error with appropriate message.
|
||||
When conversion fails for an unspecified reason, the API should return a 500
|
||||
error with appropriate message.
|
||||
"""
|
||||
user = factories.UserFactory()
|
||||
client = APIClient()
|
||||
@@ -315,7 +318,7 @@ def test_api_documents_create_with_file_conversion_error(mock_convert, settings)
|
||||
format="multipart",
|
||||
)
|
||||
|
||||
assert response.status_code == 400
|
||||
assert response.status_code == 500
|
||||
assert response.json() == {"file": ["Could not convert file content"]}
|
||||
assert not Document.objects.exists()
|
||||
|
||||
@@ -326,7 +329,8 @@ def test_api_documents_create_with_file_conversion_error(mock_convert, settings)
|
||||
@patch("core.services.converter_services.Converter.convert")
|
||||
def test_api_documents_create_with_file_service_unavailable(mock_convert, settings):
|
||||
"""
|
||||
When the conversion service is unavailable, appropriate error should be returned.
|
||||
When the conversion service is unavailable (DocSpec crashed), the API should
|
||||
return a 500 error.
|
||||
"""
|
||||
user = factories.UserFactory()
|
||||
client = APIClient()
|
||||
@@ -353,7 +357,7 @@ def test_api_documents_create_with_file_service_unavailable(mock_convert, settin
|
||||
format="multipart",
|
||||
)
|
||||
|
||||
assert response.status_code == 400
|
||||
assert response.status_code == 500
|
||||
assert response.json() == {"file": ["Could not convert file content"]}
|
||||
assert not Document.objects.exists()
|
||||
|
||||
@@ -361,6 +365,77 @@ def test_api_documents_create_with_file_service_unavailable(mock_convert, settin
|
||||
mock_capture.assert_not_called()
|
||||
|
||||
|
||||
@patch("core.services.converter_services.Converter.convert")
|
||||
def test_api_documents_create_with_file_unsupported_media_type(mock_convert, settings):
|
||||
"""
|
||||
When DocSpec rejects the file type (HTTP 415), the API should return a 415 error.
|
||||
"""
|
||||
user = factories.UserFactory()
|
||||
client = APIClient()
|
||||
client.force_login(user)
|
||||
|
||||
settings.CONVERSION_UPLOAD_ENABLED = True
|
||||
|
||||
mock_convert.side_effect = UnsupportedMediaTypeError(
|
||||
"DocSpec rejected the file type"
|
||||
)
|
||||
|
||||
file_content = b"fake docx content"
|
||||
file = BytesIO(file_content)
|
||||
file.name = "document.docx"
|
||||
|
||||
with patch("core.api.viewsets.posthog_capture") as mock_capture:
|
||||
response = client.post(
|
||||
"/api/v1.0/documents/",
|
||||
{
|
||||
"file": file,
|
||||
},
|
||||
format="multipart",
|
||||
)
|
||||
|
||||
assert response.status_code == 415
|
||||
assert response.json() == {"file": ["File type is not supported"]}
|
||||
assert not Document.objects.exists()
|
||||
|
||||
mock_capture.assert_not_called()
|
||||
|
||||
|
||||
@patch("core.services.converter_services.Converter.convert")
|
||||
def test_api_documents_create_with_file_unprocessable_content(mock_convert, settings):
|
||||
"""
|
||||
When DocSpec cannot process the file content (HTTP 422), the API should return
|
||||
a 422 error.
|
||||
"""
|
||||
user = factories.UserFactory()
|
||||
client = APIClient()
|
||||
client.force_login(user)
|
||||
|
||||
settings.CONVERSION_UPLOAD_ENABLED = True
|
||||
|
||||
mock_convert.side_effect = UnprocessableContentError(
|
||||
"DocSpec could not process the file content"
|
||||
)
|
||||
|
||||
file_content = b"fake docx content"
|
||||
file = BytesIO(file_content)
|
||||
file.name = "document.docx"
|
||||
|
||||
with patch("core.api.viewsets.posthog_capture") as mock_capture:
|
||||
response = client.post(
|
||||
"/api/v1.0/documents/",
|
||||
{
|
||||
"file": file,
|
||||
},
|
||||
format="multipart",
|
||||
)
|
||||
|
||||
assert response.status_code == 422
|
||||
assert response.json() == {"file": ["Could not process file content"]}
|
||||
assert not Document.objects.exists()
|
||||
|
||||
mock_capture.assert_not_called()
|
||||
|
||||
|
||||
def test_api_documents_create_without_file_still_works():
|
||||
"""
|
||||
Creating a document without a file should still work as before (backward compatibility).
|
||||
|
||||
@@ -9,6 +9,8 @@ from core.services import mime_types
|
||||
from core.services.converter_services import (
|
||||
DocSpecConverter,
|
||||
ServiceUnavailableError,
|
||||
UnprocessableContentError,
|
||||
UnsupportedMediaTypeError,
|
||||
ValidationError,
|
||||
)
|
||||
|
||||
@@ -74,6 +76,55 @@ def test_docspec_convert_http_error(mock_post):
|
||||
converter.convert(b"test data", mime_types.DOCX, mime_types.BLOCKNOTE)
|
||||
|
||||
|
||||
@patch("requests.post")
|
||||
def test_docspec_convert_unsupported_media_type(mock_post):
|
||||
"""Should raise UnsupportedMediaTypeError when DocSpec responds with HTTP 415."""
|
||||
converter = DocSpecConverter()
|
||||
mock_response = MagicMock(status_code=415)
|
||||
mock_response.raise_for_status.side_effect = requests.HTTPError(
|
||||
"Unsupported Media Type", response=mock_response
|
||||
)
|
||||
mock_post.return_value = mock_response
|
||||
|
||||
with pytest.raises(
|
||||
UnsupportedMediaTypeError, match="DocSpec rejected the file type"
|
||||
):
|
||||
converter.convert(b"test data", mime_types.DOCX, mime_types.BLOCKNOTE)
|
||||
|
||||
|
||||
@patch("requests.post")
|
||||
def test_docspec_convert_unprocessable_content(mock_post):
|
||||
"""Should raise UnprocessableContentError when DocSpec responds with HTTP 422."""
|
||||
converter = DocSpecConverter()
|
||||
mock_response = MagicMock(status_code=422)
|
||||
mock_response.raise_for_status.side_effect = requests.HTTPError(
|
||||
"Unprocessable Content", response=mock_response
|
||||
)
|
||||
mock_post.return_value = mock_response
|
||||
|
||||
with pytest.raises(
|
||||
UnprocessableContentError, match="DocSpec could not process the file content"
|
||||
):
|
||||
converter.convert(b"test data", mime_types.DOCX, mime_types.BLOCKNOTE)
|
||||
|
||||
|
||||
@patch("requests.post")
|
||||
def test_docspec_convert_server_error(mock_post):
|
||||
"""Should raise ServiceUnavailableError when DocSpec responds with HTTP 500."""
|
||||
converter = DocSpecConverter()
|
||||
mock_response = MagicMock(status_code=500)
|
||||
mock_response.raise_for_status.side_effect = requests.HTTPError(
|
||||
"Internal Server Error", response=mock_response
|
||||
)
|
||||
mock_post.return_value = mock_response
|
||||
|
||||
with pytest.raises(
|
||||
ServiceUnavailableError,
|
||||
match="Failed to connect to DocSpec conversion service",
|
||||
):
|
||||
converter.convert(b"test data", mime_types.DOCX, mime_types.BLOCKNOTE)
|
||||
|
||||
|
||||
@patch("requests.post")
|
||||
def test_docspec_convert_timeout(mock_post):
|
||||
"""Should raise ServiceUnavailableError when request times out."""
|
||||
|
||||
+108
@@ -0,0 +1,108 @@
|
||||
import { renderHook, waitFor } from '@testing-library/react';
|
||||
import fetchMock from 'fetch-mock';
|
||||
import { beforeEach, describe, expect, it, vi } from 'vitest';
|
||||
|
||||
import { AppWrapper } from '@/tests/utils';
|
||||
|
||||
import { useImportDoc } from '../useImportDoc';
|
||||
|
||||
const { mockToast } = vi.hoisted(() => ({ mockToast: vi.fn() }));
|
||||
|
||||
vi.mock('@gouvfr-lasuite/ui-components', async () => {
|
||||
const actual = await vi.importActual<any>('@gouvfr-lasuite/ui-components');
|
||||
return {
|
||||
...actual,
|
||||
useToastProvider: () => ({ toast: mockToast }),
|
||||
};
|
||||
});
|
||||
|
||||
vi.mock('@/core', async () => {
|
||||
const actual = await vi.importActual<any>('@/core');
|
||||
return {
|
||||
...actual,
|
||||
useConfig: () => ({
|
||||
data: { CONVERSION_FILE_EXTENSIONS_ALLOWED: ['.docx', '.md'] },
|
||||
}),
|
||||
};
|
||||
});
|
||||
|
||||
const uploadUrl = 'http://test.jest/api/v1.0/documents/';
|
||||
const createFile = () =>
|
||||
new File(['content'], 'my-document.docx', {
|
||||
type: 'application/vnd.openxmlformats-officedocument.wordprocessingml.document',
|
||||
});
|
||||
|
||||
describe('useImportDoc', () => {
|
||||
beforeEach(() => {
|
||||
vi.clearAllMocks();
|
||||
fetchMock.hardReset();
|
||||
fetchMock.mockGlobal();
|
||||
});
|
||||
|
||||
const renderUseImportDoc = () =>
|
||||
renderHook(() => useImportDoc(), { wrapper: AppWrapper }).result;
|
||||
|
||||
it('shows an unsupported media type message on a 415 response', async () => {
|
||||
fetchMock.post(uploadUrl, { status: 415, body: {} });
|
||||
|
||||
const result = renderUseImportDoc();
|
||||
result.current.mutate([createFile(), 'application/octet-stream']);
|
||||
|
||||
await waitFor(() => {
|
||||
expect(mockToast).toHaveBeenCalled();
|
||||
});
|
||||
|
||||
expect(mockToast).toHaveBeenCalledWith(
|
||||
'The document "my-document.docx" import has failed (only .docx, .md files are allowed)',
|
||||
'error',
|
||||
);
|
||||
});
|
||||
|
||||
it('shows an unprocessable content message on a 422 response', async () => {
|
||||
fetchMock.post(uploadUrl, { status: 422, body: {} });
|
||||
|
||||
const result = renderUseImportDoc();
|
||||
result.current.mutate([createFile(), 'application/octet-stream']);
|
||||
|
||||
await waitFor(() => {
|
||||
expect(mockToast).toHaveBeenCalled();
|
||||
});
|
||||
|
||||
expect(mockToast).toHaveBeenCalledWith(
|
||||
'The import of the document « my-document.docx » failed because something is wrong with the file.',
|
||||
'error',
|
||||
);
|
||||
});
|
||||
|
||||
it('shows a technical issue message on a 500 response', async () => {
|
||||
fetchMock.post(uploadUrl, { status: 500, body: {} });
|
||||
|
||||
const result = renderUseImportDoc();
|
||||
result.current.mutate([createFile(), 'application/octet-stream']);
|
||||
|
||||
await waitFor(() => {
|
||||
expect(mockToast).toHaveBeenCalled();
|
||||
});
|
||||
|
||||
expect(mockToast).toHaveBeenCalledWith(
|
||||
'The import of the document « my-document.docx » failed due to a technical issue',
|
||||
'error',
|
||||
);
|
||||
});
|
||||
|
||||
it('shows the generic message on other error statuses', async () => {
|
||||
fetchMock.post(uploadUrl, { status: 400, body: {} });
|
||||
|
||||
const result = renderUseImportDoc();
|
||||
result.current.mutate([createFile(), 'application/octet-stream']);
|
||||
|
||||
await waitFor(() => {
|
||||
expect(mockToast).toHaveBeenCalled();
|
||||
});
|
||||
|
||||
expect(mockToast).toHaveBeenCalledWith(
|
||||
'The document "my-document.docx" import has failed',
|
||||
'error',
|
||||
);
|
||||
});
|
||||
});
|
||||
@@ -12,6 +12,7 @@ import {
|
||||
errorCauses,
|
||||
fetchAPI,
|
||||
} from '@/api';
|
||||
import { useConfig } from '@/core';
|
||||
import { useToast } from '@/hooks';
|
||||
|
||||
import { Doc } from '../types';
|
||||
@@ -75,6 +76,7 @@ export function useImportDoc(props?: UseImportDocOptions) {
|
||||
const { toast } = useToast();
|
||||
const queryClient = useQueryClient();
|
||||
const { t } = useTranslation();
|
||||
const { data: config } = useConfig();
|
||||
|
||||
return useMutation<Doc, APIError, [File, string]>({
|
||||
mutationFn: importDoc,
|
||||
@@ -123,6 +125,8 @@ export function useImportDoc(props?: UseImportDocOptions) {
|
||||
toast(
|
||||
t('The document "{{documentName}}" has been successfully imported', {
|
||||
documentName: importedDoc.title || '',
|
||||
description:
|
||||
'Toast message when the document has been successfully imported',
|
||||
}),
|
||||
VariantType.SUCCESS,
|
||||
);
|
||||
@@ -130,12 +134,64 @@ export function useImportDoc(props?: UseImportDocOptions) {
|
||||
props?.onSuccess?.(...successProps);
|
||||
},
|
||||
onError: (...errorProps) => {
|
||||
toast(
|
||||
t(`The document "{{documentName}}" import has failed`, {
|
||||
documentName: errorProps?.[1][0].name || '',
|
||||
}),
|
||||
VariantType.ERROR,
|
||||
);
|
||||
const [error, [file]] = errorProps;
|
||||
const documentName = file?.name || '';
|
||||
|
||||
if (error.status === 415) {
|
||||
const allowedExtensions =
|
||||
config?.CONVERSION_FILE_EXTENSIONS_ALLOWED?.join(', ') || '';
|
||||
toast(
|
||||
allowedExtensions
|
||||
? t(
|
||||
`The document "{{documentName}}" import has failed (only {{allowedExtensions}} files are allowed)`,
|
||||
{
|
||||
documentName,
|
||||
allowedExtensions,
|
||||
description:
|
||||
'Toast message when the document import has failed due to unsupported media type',
|
||||
},
|
||||
)
|
||||
: t(`The document "{{documentName}}" import has failed`, {
|
||||
documentName,
|
||||
description:
|
||||
'Toast message when the document import has failed due to unsupported media type',
|
||||
}),
|
||||
VariantType.ERROR,
|
||||
);
|
||||
} else if (error.status === 422) {
|
||||
toast(
|
||||
t(
|
||||
`The import of the document « {{documentName}} » failed because something is wrong with the file.`,
|
||||
{
|
||||
documentName,
|
||||
description:
|
||||
'Toast message when the document import has failed due to unprocessable entity',
|
||||
},
|
||||
),
|
||||
VariantType.ERROR,
|
||||
);
|
||||
} else if (error.status === 500) {
|
||||
toast(
|
||||
t(
|
||||
`The import of the document « {{documentName}} » failed due to a technical issue`,
|
||||
{
|
||||
documentName,
|
||||
description:
|
||||
'Toast message when the document import has failed due to a technical issue',
|
||||
},
|
||||
),
|
||||
VariantType.ERROR,
|
||||
);
|
||||
} else {
|
||||
toast(
|
||||
t(`The document "{{documentName}}" import has failed`, {
|
||||
documentName,
|
||||
description:
|
||||
'Toast message when the document import has failed due to an unknown error',
|
||||
}),
|
||||
VariantType.ERROR,
|
||||
);
|
||||
}
|
||||
|
||||
props?.onError?.(...errorProps);
|
||||
},
|
||||
|
||||
Reference in New Issue
Block a user