(drive) use url_permalink and limit request to drive resource server (#587)

With the latest version of Drive, items have no a url_permalink property which
is more robust than the previous url property we were using as download link
for drive attachments.

Furthermore, we totally revamp the logic to save attachment to drive. Actually
the current implementation triggered n requests for n attachments each time a
user opened a thread... that was great for ux as the user always know if the
attachment exists in its workspace but it triggers too muck request to be a
production ready implementation. So now, the user is no more able to know
if an attachment exists in its workspace until it clicks to upload the
attachment and the backend checks if the file already exists in the user
workspace.

Finally, we also replace the DriveIcon which was used to open drive item
preview by an eye icon.

Resolve #567
This commit is contained in:
Jean-Baptiste PENRATH
2026-03-11 18:45:05 +01:00
committed by GitHub
parent 4a18fe2019
commit b1d455022c
10 changed files with 320 additions and 127 deletions

View File

@@ -4671,8 +4671,7 @@
"schema": {
"type": "string"
},
"description": "Search files by title.",
"required": true
"description": "Search files by title."
}
],
"tags": [
@@ -4698,7 +4697,7 @@
},
"post": {
"operationId": "third_party_drive_create",
"description": "Create a new file in the main workspace.",
"description": "Save an attachment to the user's Drive workspace. If the file already exists (matched by title and size), returns the existing item with a 200 status. Otherwise, creates a new file and returns it with a 201 status.",
"tags": [
"third-party/drive"
],
@@ -4723,6 +4722,16 @@
}
],
"responses": {
"200": {
"content": {
"application/json": {
"schema": {
"$ref": "#/components/schemas/PartialDriveItem"
}
}
},
"description": "File already exists in Drive"
},
"201": {
"content": {
"application/json": {

View File

@@ -49,7 +49,7 @@ class DriveAPIView(APIView):
type=OpenApiTypes.STR,
location=OpenApiParameter.QUERY,
description="Search files by title.",
required=True,
required=False,
),
],
responses={
@@ -82,21 +82,34 @@ class DriveAPIView(APIView):
filters.update({"title": title})
# Search for files at the root of the user's workspace
response = requests.get(
f"{self.drive_external_api}/items/",
params=filters,
headers={
"Authorization": f"Bearer {access_token}",
"Content-Type": "application/json",
},
timeout=5,
)
try:
response = requests.get(
f"{self.drive_external_api}/items/",
params=filters,
headers={
"Authorization": f"Bearer {access_token}",
"Content-Type": "application/json",
},
timeout=5,
)
response.raise_for_status()
except requests.exceptions.RequestException:
logger.exception("Failed to search files in Drive")
return Response(
status=status.HTTP_502_BAD_GATEWAY,
data={"error": "Failed to search files in Drive"},
)
return Response(response.json())
@extend_schema(
tags=["third-party/drive"],
description="Create a new file in the main workspace.",
description=(
"Save an attachment to the user's Drive workspace. "
"If the file already exists (matched by title and size), "
"returns the existing item with a 200 status. "
"Otherwise, creates a new file and returns it with a 201 status."
),
request=inline_serializer(
name="DriveUploadAttachment",
fields={
@@ -107,18 +120,21 @@ class DriveAPIView(APIView):
},
),
responses={
200: OpenApiResponse(
description="File already exists in Drive",
response=PartialDriveItemSerializer,
),
201: OpenApiResponse(
description="File created successfully",
response=PartialDriveItemSerializer,
)
),
},
)
@method_decorator(refresh_oidc_access_token)
def post(self, request):
"""
Create a new file in the main workspace.
Save an attachment to the user's Drive workspace (get or create).
"""
# Get the access token from the session
access_token = request.session.get("oidc_access_token")
blob_id = request.data.get("blob_id")
if not blob_id:
@@ -134,26 +150,75 @@ class DriveAPIView(APIView):
status=status.HTTP_400_BAD_REQUEST, data={"error": str(exc)}
)
# Create a new file in the main workspace
auth_headers = {
"Authorization": f"Bearer {access_token}",
"Content-Type": "application/json",
}
# Check if file already exists in Drive (get_or_create pattern)
try:
existing_item = self._find_existing_drive_item(attachment, auth_headers)
except requests.exceptions.RequestException:
logger.exception("Failed to search Drive for existing file")
return Response(
status=status.HTTP_502_BAD_GATEWAY,
data={"error": "Failed to search Drive for existing file"},
)
if existing_item:
return Response(status=status.HTTP_200_OK, data=existing_item)
# File doesn't exist, create it
try:
return self._create_drive_item(attachment, auth_headers)
except requests.exceptions.RequestException:
logger.exception("Failed to create file in Drive")
return Response(
status=status.HTTP_502_BAD_GATEWAY,
data={"error": "Failed to create file in Drive"},
)
def _find_existing_drive_item(self, attachment, headers):
"""Search for an existing file in Drive matching the attachment name and size.
Raises RequestException on network/server errors so callers don't
silently fall through to creation when the lookup is inconclusive.
"""
search_response = requests.get(
f"{self.drive_external_api}/items/",
params={
"is_creator_me": True,
"type": "file",
"title": attachment["name"],
},
headers=headers,
timeout=5,
)
search_response.raise_for_status()
for item in search_response.json().get("results", []):
if item.get("size") == attachment["size"]:
return item
return None
def _create_drive_item(self, attachment, headers):
"""Create a new file in Drive and upload its content."""
response = requests.post(
f"{self.drive_external_api}/items/",
json={
"type": "file",
"filename": attachment["name"],
},
headers={
"Authorization": f"Bearer {access_token}",
"Content-Type": "application/json",
},
headers=headers,
timeout=5,
)
response.raise_for_status()
item = response.json()
policy = item["policy"]
# Upload file content using the presigned URL
upload_response = requests.put(
policy,
item["policy"],
data=attachment["content"],
headers={"Content-Type": attachment["type"], "x-amz-acl": "private"},
timeout=180,
@@ -163,10 +228,7 @@ class DriveAPIView(APIView):
# Tell the Drive API that the upload is ended
response = requests.post(
f"{self.drive_external_api}/items/{item['id']}/upload-ended/",
headers={
"Authorization": f"Bearer {access_token}",
"Content-Type": "application/json",
},
headers=headers,
timeout=5,
)
response.raise_for_status()

View File

@@ -10,7 +10,6 @@ from unittest.mock import patch
from django.urls import reverse
import pytest
import requests
import responses
from rest_framework import status
from rest_framework.test import APIClient
@@ -347,7 +346,7 @@ Test file content for Drive upload without access.
def test_api_third_party_drive_post_success(
self, _mock, api_client_with_user, mailbox_with_message
):
"""Test successfully uploading a file to Drive."""
"""Test successfully uploading a file to Drive when it doesn't exist yet."""
client, _ = api_client_with_user
_, message = mailbox_with_message
@@ -356,6 +355,19 @@ Test file content for Drive upload without access.
file_id = str(uuid.uuid4())
presigned_url = "http://s3.test/presigned-upload-url"
# Mock the search for existing file (not found)
responses.add(
responses.GET,
"http://drive.test/external_api/v1.0/items/",
json={
"count": 0,
"next": None,
"previous": None,
"results": [],
},
status=status.HTTP_200_OK,
)
# Mock the file creation response
responses.add(
responses.POST,
@@ -403,28 +415,144 @@ Test file content for Drive upload without access.
assert data["id"] == file_id
assert data["title"] == "test_file.txt"
# Verify the 3 expected API calls (create → S3 upload → upload-ended)
assert len(responses.calls) == 3
# Verify the 4 expected API calls (search → create → S3 upload → upload-ended)
assert len(responses.calls) == 4
# Verify search request
assert "is_creator_me=True" in responses.calls[0].request.url
assert "title=test_file.txt" in responses.calls[0].request.url
# Verify file creation request
assert (
responses.calls[0].request.url
== "http://drive.test/external_api/v1.0/items/"
)
assert (
responses.calls[0].request.headers["Authorization"]
== "Bearer test-access-token"
)
file_creation_body = json.loads(responses.calls[0].request.body)
assert responses.calls[1].request.method == "POST"
file_creation_body = json.loads(responses.calls[1].request.body)
assert file_creation_body["type"] == "file"
assert file_creation_body["filename"] == "test_file.txt"
# Verify presigned URL upload
assert responses.calls[1].request.url == presigned_url
assert responses.calls[1].request.headers["x-amz-acl"] == "private"
assert responses.calls[2].request.url == presigned_url
assert responses.calls[2].request.headers["x-amz-acl"] == "private"
# Verify upload-ended confirmation
assert f"items/{file_id}/upload-ended/" in responses.calls[2].request.url
assert f"items/{file_id}/upload-ended/" in responses.calls[3].request.url
@responses.activate
@patch(
"lasuite.oidc_login.middleware.RefreshOIDCAccessToken.is_expired",
return_value=False,
)
def test_api_third_party_drive_post_file_already_exists(
self, _mock, api_client_with_user, mailbox_with_message
):
"""Test that POST returns 200 when the file already exists in Drive."""
client, _ = api_client_with_user
_, message = mailbox_with_message
blob_id = f"msg_{message.id}_0"
existing_file_id = str(uuid.uuid4())
# The attachment "test_file.txt" has content "Test file content for Drive upload."
# which is 35 bytes
attachment_size = len(b"Test file content for Drive upload.")
# Mock the search returning an existing file with matching size
responses.add(
responses.GET,
"http://drive.test/external_api/v1.0/items/",
json={
"count": 1,
"next": None,
"previous": None,
"results": [
{
"id": existing_file_id,
"filename": "test_file.txt",
"mimetype": "text/plain",
"size": attachment_size,
}
],
},
status=status.HTTP_200_OK,
)
response = client.post(
reverse("drive"),
{"blob_id": blob_id},
format="json",
)
assert response.status_code == status.HTTP_200_OK
data = response.json()
assert data["id"] == existing_file_id
# Only the search request should have been made (no create/upload)
assert len(responses.calls) == 1
assert "title=test_file.txt" in responses.calls[0].request.url
@responses.activate
@patch(
"lasuite.oidc_login.middleware.RefreshOIDCAccessToken.is_expired",
return_value=False,
)
def test_api_third_party_drive_post_same_name_different_size(
self, _mock, api_client_with_user, mailbox_with_message
):
"""Test that POST creates a new file when existing file has same name but different size."""
client, _ = api_client_with_user
_, message = mailbox_with_message
blob_id = f"msg_{message.id}_0"
file_id = str(uuid.uuid4())
presigned_url = "http://s3.test/presigned-upload-url"
# Mock the search returning a file with same name but different size
responses.add(
responses.GET,
"http://drive.test/external_api/v1.0/items/",
json={
"count": 1,
"next": None,
"previous": None,
"results": [
{
"id": str(uuid.uuid4()),
"filename": "test_file.txt",
"mimetype": "text/plain",
"size": 999999,
}
],
},
status=status.HTTP_200_OK,
)
# Mock the file creation (since size didn't match)
responses.add(
responses.POST,
"http://drive.test/external_api/v1.0/items/",
json={
"id": file_id,
"title": "test_file.txt",
"type": "file",
"policy": presigned_url,
},
status=status.HTTP_200_OK,
)
responses.add(responses.PUT, presigned_url, status=status.HTTP_200_OK)
responses.add(
responses.POST,
f"http://drive.test/external_api/v1.0/items/{file_id}/upload-ended/",
json={"id": file_id, "title": "test_file.txt", "type": "file"},
status=status.HTTP_200_OK,
)
response = client.post(
reverse("drive"),
{"blob_id": blob_id},
format="json",
)
assert response.status_code == status.HTTP_201_CREATED
# search + create + S3 upload + upload-ended
assert len(responses.calls) == 4
@responses.activate
@patch(
@@ -488,6 +616,19 @@ Test file content for Drive upload without access.
file_id = str(uuid.uuid4())
presigned_url = "http://s3.test/presigned-upload-url"
# Mock the search for existing file (not found)
responses.add(
responses.GET,
"http://drive.test/external_api/v1.0/items/",
json={
"count": 0,
"next": None,
"previous": None,
"results": [],
},
status=status.HTTP_200_OK,
)
# Mock the file creation response
responses.add(
responses.POST,
@@ -510,13 +651,11 @@ Test file content for Drive upload without access.
status=status.HTTP_500_INTERNAL_SERVER_ERROR,
)
with pytest.raises(requests.exceptions.HTTPError) as excinfo:
client.post(
reverse("drive"),
{"blob_id": blob_id},
format="json",
)
assert (
excinfo.value.response.status_code == status.HTTP_500_INTERNAL_SERVER_ERROR
response = client.post(
reverse("drive"),
{"blob_id": blob_id},
format="json",
)
assert response.status_code == status.HTTP_502_BAD_GATEWAY
assert response.json()["error"] == "Failed to create file in Drive"

View File

@@ -10,5 +10,5 @@ export type ThirdPartyDriveRetrieveParams = {
/**
* Search files by title.
*/
title: string;
title?: string;
};

View File

@@ -48,7 +48,7 @@ export type thirdPartyDriveRetrieveResponse =
thirdPartyDriveRetrieveResponseSuccess;
export const getThirdPartyDriveRetrieveUrl = (
params: ThirdPartyDriveRetrieveParams,
params?: ThirdPartyDriveRetrieveParams,
) => {
const normalizedParams = new URLSearchParams();
@@ -66,7 +66,7 @@ export const getThirdPartyDriveRetrieveUrl = (
};
export const thirdPartyDriveRetrieve = async (
params: ThirdPartyDriveRetrieveParams,
params?: ThirdPartyDriveRetrieveParams,
options?: RequestInit,
): Promise<thirdPartyDriveRetrieveResponse> => {
return fetchAPI<thirdPartyDriveRetrieveResponse>(
@@ -88,7 +88,7 @@ export const getThirdPartyDriveRetrieveQueryOptions = <
TData = Awaited<ReturnType<typeof thirdPartyDriveRetrieve>>,
TError = unknown,
>(
params: ThirdPartyDriveRetrieveParams,
params?: ThirdPartyDriveRetrieveParams,
options?: {
query?: Partial<
UseQueryOptions<
@@ -126,7 +126,7 @@ export function useThirdPartyDriveRetrieve<
TData = Awaited<ReturnType<typeof thirdPartyDriveRetrieve>>,
TError = unknown,
>(
params: ThirdPartyDriveRetrieveParams,
params: undefined | ThirdPartyDriveRetrieveParams,
options: {
query: Partial<
UseQueryOptions<
@@ -153,7 +153,7 @@ export function useThirdPartyDriveRetrieve<
TData = Awaited<ReturnType<typeof thirdPartyDriveRetrieve>>,
TError = unknown,
>(
params: ThirdPartyDriveRetrieveParams,
params?: ThirdPartyDriveRetrieveParams,
options?: {
query?: Partial<
UseQueryOptions<
@@ -180,7 +180,7 @@ export function useThirdPartyDriveRetrieve<
TData = Awaited<ReturnType<typeof thirdPartyDriveRetrieve>>,
TError = unknown,
>(
params: ThirdPartyDriveRetrieveParams,
params?: ThirdPartyDriveRetrieveParams,
options?: {
query?: Partial<
UseQueryOptions<
@@ -200,7 +200,7 @@ export function useThirdPartyDriveRetrieve<
TData = Awaited<ReturnType<typeof thirdPartyDriveRetrieve>>,
TError = unknown,
>(
params: ThirdPartyDriveRetrieveParams,
params?: ThirdPartyDriveRetrieveParams,
options?: {
query?: Partial<
UseQueryOptions<
@@ -228,17 +228,24 @@ export function useThirdPartyDriveRetrieve<
}
/**
* Create a new file in the main workspace.
* Save an attachment to the user's Drive workspace. If the file already exists (matched by title and size), returns the existing item with a 200 status. Otherwise, creates a new file and returns it with a 201 status.
*/
export type thirdPartyDriveCreateResponse200 = {
data: PartialDriveItem;
status: 200;
};
export type thirdPartyDriveCreateResponse201 = {
data: PartialDriveItem;
status: 201;
};
export type thirdPartyDriveCreateResponseSuccess =
thirdPartyDriveCreateResponse201 & {
headers: Headers;
};
export type thirdPartyDriveCreateResponseSuccess = (
| thirdPartyDriveCreateResponse200
| thirdPartyDriveCreateResponse201
) & {
headers: Headers;
};
export type thirdPartyDriveCreateResponse =
thirdPartyDriveCreateResponseSuccess;

View File

@@ -15,6 +15,9 @@ type DriveAttachmentPickerProps = ButtonProps & {
onPick: (attachments: DriveFile[]) => void;
}
// TODO: Remove this type once the Drive SDK is updated to include the url_permalink field
type PatchedItem = Item & { url_permalink: string };
/**
* DriveAttachmentPicker is a component that allows the user to pick files
* from a Drive instance if one is configured otherwise it will return null.
@@ -29,10 +32,10 @@ export const DriveAttachmentPicker = ({ onPick, ...buttonProps }: DriveAttachmen
const [isLoading, setIsLoading] = useState(false);
const config = useConfig();
const isDriveDisabled = !useFeatureFlag(FEATURE_KEYS.DRIVE);
const serializeToDriveFile = (item: Item): DriveFile => ({
const serializeToDriveFile = (item: PatchedItem): DriveFile => ({
id: item.id,
name: item.title,
url: item.url,
url: item.url_permalink ?? item.url,
type: item.type,
size: item.size,
created_at: new Date().toISOString(),
@@ -55,7 +58,7 @@ export const DriveAttachmentPicker = ({ onPick, ...buttonProps }: DriveAttachmen
}
if (result?.type === "picked" && result.items) {
onPick(result.items.map(serializeToDriveFile));
onPick((result.items as PatchedItem[]).map(serializeToDriveFile));
}
}, [isDriveDisabled]);

View File

@@ -42,7 +42,7 @@
.attachment-item-actions {
display: flex;
flex-direction: row;
gap: var(--c--globals--spacings--2xs);
gap: var(--c--globals--spacings--4xs);
flex-wrap: nowrap;
flex: 0 0 auto;
}

View File

@@ -1,7 +1,7 @@
import { DriveIcon } from "@/features/forms/components/message-form/drive-icon";
import { useConfig } from "@/features/providers/config";
import { FEATURE_KEYS, useFeatureFlag } from "@/hooks/use-feature";
import { Button } from "@gouvfr-lasuite/cunningham-react";
import { Icon } from "@gouvfr-lasuite/ui-kit";
import { useTranslation } from "react-i18next";
type DrivePreviewLinkProps = {
@@ -28,7 +28,7 @@ export const DrivePreviewLink = ({ fileId }: DrivePreviewLinkProps) => {
target="_blank"
size="medium"
variant="tertiary"
icon={<DriveIcon />}
icon={<Icon name="remove_red_eye" />}
/>
)
}

View File

@@ -1,7 +1,6 @@
import { useState, useMemo, useEffect } from "react";
import { useTranslation } from "react-i18next";
import { useQueryClient } from "@tanstack/react-query";
import { useThirdPartyDriveRetrieve, useThirdPartyDriveCreate } from "@/features/api/gen";
import { useThirdPartyDriveCreate } from "@/features/api/gen";
import { Attachment } from "@/features/api/gen/models";
import usePrevious from "@/hooks/use-previous";
import { Spinner, Icon } from "@gouvfr-lasuite/ui-kit";
@@ -11,6 +10,7 @@ import { DrivePreviewLink } from "./drive-preview-link";
import { FEATURE_KEYS, useFeatureFlag } from "@/hooks/use-feature";
import { useConfig } from "@/features/providers/config";
import { handle } from "@/features/utils/errors";
import { driveUploadStore } from "./drive-upload-store";
type DriveUploadButtonProps = {
@@ -18,34 +18,20 @@ type DriveUploadButtonProps = {
}
/**
* UploadDriveButton
* Button to upload an attachment to the Drive personal workspace of the user.
* At mount, it checks if the file is already in the Drive personal workspace and display the preview link if it is.
* If the file is not in the Drive personal workspace, it displays the upload button.
*
* To prevent to check if the file already exists too often, the query is cached for 10 minutes.
* DriveUploadButton
* Button to save an attachment to the user's Drive workspace.
* Uses a get_or_create pattern: the backend checks if the file already exists
* before uploading, returning 200 (existing) or 201 (created).
*/
export const DriveUploadButton = ({ attachment }: DriveUploadButtonProps) => {
const { t } = useTranslation();
const { DRIVE } = useConfig();
const isDriveDisabled = !useFeatureFlag(FEATURE_KEYS.DRIVE);
const [state, setState] = useState<'idle' | 'uploading' | 'error' | 'success'>('idle');
const [driveFileId, setDriveFileId] = useState<string | null>(null);
const [driveFileId, setDriveFileId] = useState<string | undefined>(
() => driveUploadStore.get(attachment.blobId),
);
const prevState = usePrevious(state);
const queryClient = useQueryClient();
const driveFilesQuery = useThirdPartyDriveRetrieve({
title: attachment.name,
}, {
query: {
enabled: !isDriveDisabled && !driveFileId,
// Keep data fresh for 10 minutes to prevent requests each time the component is rendered.
staleTime: 600000,
refetchOnReconnect: false,
},
request: {
logoutOn401: false,
},
});
const uploadToDrive = useThirdPartyDriveCreate({
request: {
logoutOn401: false,
@@ -53,26 +39,14 @@ export const DriveUploadButton = ({ attachment }: DriveUploadButtonProps) => {
mutation: {
onSuccess: (data) => {
setDriveFileId(data.data.id);
// Update the drive files query data to include the new file and prevent a new request.
queryClient.setQueryData(driveFilesQuery.queryKey, (cachedData) => {
const previousData = cachedData ?? { status: 200, headers: new Headers(), data: { next: null, previous: null, count: 0, results: [] } };
return {
...previousData,
data: {
...previousData.data,
count: previousData.data.count + 1,
results: [...previousData.data.results, data.data!],
}
}
});
driveUploadStore.set(attachment.blobId, data.data.id);
},
},
});
const showUploadTooltip = useMemo(() => ['success', 'error'].includes(state), [state]);
const isBusy = state === 'uploading' || driveFilesQuery.isLoading;
const handleUploadToDrive = async () => {
if (isBusy) return;
if (state === 'uploading') return;
setState('uploading');
try {
await uploadToDrive.mutateAsync({
@@ -88,11 +62,11 @@ export const DriveUploadButton = ({ attachment }: DriveUploadButtonProps) => {
}
const StateIcon = useMemo(() => {
if (isBusy) return <Spinner size="sm" />;
if (state === 'uploading') return <Spinner size="sm" />;
if (state === 'success') return <Icon name="check_circle" />;
if (state === 'error') return <Icon name="error" />;
return <Icon name="drive_folder_upload" />;
}, [state, driveFilesQuery.isLoading]);
}, [state]);
useEffect(() => {
if (['error', 'success'].includes(state)) {
@@ -103,19 +77,6 @@ export const DriveUploadButton = ({ attachment }: DriveUploadButtonProps) => {
}
}, [state]);
useEffect(() => {
const driveFiles = driveFilesQuery.data?.data?.results ?? [];
if (!driveFileId && driveFiles.length > 0) {
const file = driveFiles.find((file) =>
file.filename === attachment.name
&& file.size === attachment.size
);
if (file) {
setDriveFileId(file.id);
}
}
}, [driveFilesQuery.data?.data?.results?.length, driveFileId]);
if (isDriveDisabled) return null;
return (
@@ -126,8 +87,8 @@ export const DriveUploadButton = ({ attachment }: DriveUploadButtonProps) => {
aria-label={t("Save into your {{driveAppName}}'s workspace", { driveAppName: DRIVE.app_name })}
size="medium"
icon={StateIcon}
disabled={isBusy || state !== 'idle'}
aria-busy={isBusy}
disabled={state === 'uploading' || state !== 'idle'}
aria-busy={state === 'uploading'}
color={state === 'error' ? 'error' : 'brand'}
variant="tertiary"
onClick={handleUploadToDrive}
@@ -152,4 +113,3 @@ export const DriveUploadButton = ({ attachment }: DriveUploadButtonProps) => {
</div>
)
}

View File

@@ -0,0 +1,13 @@
/**
* In-memory store that maps attachment blobIds to their Drive file IDs.
* Persists across thread navigations (component mount/unmount cycles)
* so the user sees the Drive preview link without re-uploading.
*/
const driveFileIds = new Map<string, string>();
export const driveUploadStore = {
get: (blobId: string) => driveFileIds.get(blobId),
set: (blobId: string, driveFileId: string) => {
driveFileIds.set(blobId, driveFileId);
},
};