diff --git a/CHANGELOG.md b/CHANGELOG.md index 1d62d79b3..86498a8a4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -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 diff --git a/src/backend/core/api/viewsets.py b/src/backend/core/api/viewsets.py index 8a452936e..257a55a00 100644 --- a/src/backend/core/api/viewsets.py +++ b/src/backend/core/api/viewsets.py @@ -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 diff --git a/src/backend/core/services/converter_services.py b/src/backend/core/services/converter_services.py index 26ca3bd57..1d4c5b167 100644 --- a/src/backend/core/services/converter_services.py +++ b/src/backend/core/services/converter_services.py @@ -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( diff --git a/src/backend/core/tests/documents/test_api_documents_create_with_file.py b/src/backend/core/tests/documents/test_api_documents_create_with_file.py index 4bf07a940..034e888f3 100644 --- a/src/backend/core/tests/documents/test_api_documents_create_with_file.py +++ b/src/backend/core/tests/documents/test_api_documents_create_with_file.py @@ -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). diff --git a/src/backend/core/tests/test_services_docspec_converter.py b/src/backend/core/tests/test_services_docspec_converter.py index 3c43f1f78..aefc64a3b 100644 --- a/src/backend/core/tests/test_services_docspec_converter.py +++ b/src/backend/core/tests/test_services_docspec_converter.py @@ -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.""" diff --git a/src/frontend/apps/impress/src/features/docs/doc-management/api/__tests__/useImportDoc.test.tsx b/src/frontend/apps/impress/src/features/docs/doc-management/api/__tests__/useImportDoc.test.tsx new file mode 100644 index 000000000..41624df5b --- /dev/null +++ b/src/frontend/apps/impress/src/features/docs/doc-management/api/__tests__/useImportDoc.test.tsx @@ -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('@gouvfr-lasuite/ui-components'); + return { + ...actual, + useToastProvider: () => ({ toast: mockToast }), + }; +}); + +vi.mock('@/core', async () => { + const actual = await vi.importActual('@/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', + ); + }); +}); diff --git a/src/frontend/apps/impress/src/features/docs/doc-management/api/useImportDoc.tsx b/src/frontend/apps/impress/src/features/docs/doc-management/api/useImportDoc.tsx index 9f4b48cee..a85a2f89d 100644 --- a/src/frontend/apps/impress/src/features/docs/doc-management/api/useImportDoc.tsx +++ b/src/frontend/apps/impress/src/features/docs/doc-management/api/useImportDoc.tsx @@ -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({ 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); },