Introduce file upload api #5977

flvndvd · 2024-07-01T12:27:06Z

Description

This PR goes one step further than the original attempt at introducing a file API in #5953.

Summary

While working on vision, we identified a race condition in the process of uploading content fragment files. Currently, we create the conversation or post the message with the content fragment before uploading the associated file, which causes issues. As we plan to use URLs for vision and need to resize images (increasing upload time), the existing logic requires refactoring.

Key Changes

This PR establishes a new set of endpoints aimed at facilitating file uploads, laying the groundwork for future expansion to support various file types and use cases. The key additions include:

/files Endpoint:
- Creates a file entry in the database and generates a file ID.
- Ensures the file adheres to size and content type requirements.
- Enforces an agressive rate limit.
- Returns a file ID and an upload URL valid for 60 seconds to prevent incomplete uploads.
/files/:fileId Endpoint:
- Supports both file uploads and retrievals.
- Allows a 60-second window for uploading files.
- Streams the file directly to cloud storage, applies pre-processing if necessary, and stores the processed file in the cloud.
- Currently, the endpoint resizes images and plans to incorporate PDF text extraction in a future update.
- Returns a download URL after the file is uploaded and pre-processed.

Pre-processing is currently handled within this endpoint due to the following reasons:

Only small files (a few megabytes) are supported, making quick pre-processing feasible.
Simplifies the client experience by providing ready-to-use files upon API response.

In the future, this approach may need to be revised for larger files (a few gigabytes).

The concept of use cases is introduced, although not yet implemented. Future updates may apply different pre-processing based on the use case. The only supported use case at present is conversation, aligning with existing content fragment logic.

Memory and Performance Considerations

The service minimizes memory usage by avoiding loading entire files into memory or writing them to disk, leveraging cloud storage instead. The exception is image files, which require full memory loading for resizing using Sharp.

Known Issues and Updates

The current version of google-cloud/storage has a known issue with streams on Node.js > 21.x. Consequently, this PR includes a version bump. The update primarily changes a few types without affecting behavior.

Risk

This endpoint is not yet used.

Deploy Plan

flvndvd · 2024-07-02T07:56:36Z

front/lib/resources/base_resource.ts

@@ -45,5 +47,8 @@ export abstract class BaseResource<M extends Model> {
    return new this(this.model, blob.get());
  }

-  abstract delete(transaction?: Transaction): Promise<Result<undefined, Error>>;
+  abstract delete(


Refactored so delete always takes an Authenticator instance.

spolu

Looks great, left mostly aesthetic comments

spolu · 2024-07-02T08:23:43Z

front/lib/resources/file_resource.ts

+  // Cloud storage logic.
+
+  getPublicUrl(auth: Authenticator): string {
+    // TODO(2024-07-01 flav) Remove once we introduce AuthenticatorWithWorkspace.


this todo really is not needed :)

I'll keep around since this check really bothers me and clutters a lot the code.

spolu · 2024-07-02T08:24:20Z

front/lib/resources/base_resource.ts

@@ -28,7 +30,7 @@ export abstract class BaseResource<M extends Model> {
    this.id = blob.id;
  }

-  static async fetchById<T extends BaseResource<M>, M extends Model>(
+  static async fetchByInternalId<T extends BaseResource<M>, M extends Model>(


If we rename it I would call it fetchByModelId to be even more explicit

Fair point.

spolu · 2024-07-02T08:24:52Z

front/lib/resources/file_resource.ts

+
+  static async fetchById(
+    auth: Authenticator,
+    id: FileId


we generaly use fileId for sId arguments

spolu · 2024-07-02T08:25:13Z

front/lib/resources/file_resource.ts

+  static async makeNew(
+    blob: Omit<CreationAttributes<FileModel>, "status" | "sId">
+  ) {
+    const fileId = makeDustFileId();


let's not introduce new terminology, this is an sId right?

spolu · 2024-07-02T08:25:36Z

front/lib/resources/file_resource.ts

+  }
+}
+
+function makeDustFileId(): FileId {


Let's modify generateModelSId to take an optional prefix instead?

Wanted to do it at first, but was not sure we were okay with it 🔥 !

spolu · 2024-07-02T08:33:25Z

front/pages/api/w/[wId]/files/[fileId].ts

+
+        res.status(200).json({ file: fileRes.toJSON(auth) });
+        return;
+      } catch (error) {


catch should be around external functions only. Let's narrow the catch to the relevant call. If that can come from maybeApplyPreProcessing then this catch is not valid

spolu · 2024-07-02T08:34:47Z

front/pages/api/w/[wId]/files/[fileId].ts

+          });
+        }
+
+        const preProcessingRes = await maybeApplyPreProcessing(auth, fileRes);


Not a big fine of "...Res" for resources. Should be just "file" no since ...Res is already used everywhere for results

Agree but file was taken by formiddable, will update.

spolu · 2024-07-02T08:35:06Z

front/pages/api/w/[wId]/files/[fileId].ts

+    });
+  }
+
+  const fileRes = await FileResource.fetchById(auth, fileId);


let's call it "file"

spolu · 2024-07-02T08:35:21Z

front/lib/api/files/preprocessing.ts

+
+export async function maybeApplyPreProcessing(
+  auth: Authenticator,
+  fileRes: FileResource


spolu · 2024-07-02T08:35:55Z

front/lib/api/files/preprocessing.ts

+  auth: Authenticator,
+  fileRes: FileResource
+): Promise<Result<undefined, Error>> {
+  const processing = processingPerContentType[fileRes.contentType];


Can we introduce the useCase logic here even if there is only one possible value for useCase? 🙏

spolu

LGTM \o/

* Checkout file upload endpoint related code * Limit file upload to 1. * Tmp * Implement FileResource * ✨ * Fix google-cloud/storage issue * Add unique sId index * Fix migration * 👕 * Make file upload rate limit more aggressive * Enforce upload window * Ensure the uploaded file matches the requirements * ✂️ * Use fromidable to adhere to file requirements * s/fetchByInternalId/fetchByModelId * Move try/catch + move markAsFailed for preprocessing * s/fileRes/file * Remove types for FileId * 🙈 * Implement use cases for pre-processing

flvndvd added 14 commits July 1, 2024 14:11

Checkout file upload endpoint related code

8dbf8d2

Limit file upload to 1.

a64f26f

Tmp

b3c041c

Implement FileResource

e261d09

✨

702bae3

Fix google-cloud/storage issue

c5c634f

Add unique sId index

95f9003

Fix migration

02065d7

👕

5600005

Make file upload rate limit more aggressive

3e88bc3

Enforce upload window

db82768

Ensure the uploaded file matches the requirements

6980606

✂️

2d0fead

Use fromidable to adhere to file requirements

acae12e

flvndvd commented Jul 2, 2024

View reviewed changes

flvndvd marked this pull request as ready for review July 2, 2024 08:17

spolu reviewed Jul 2, 2024

View reviewed changes

flvndvd added 6 commits July 2, 2024 11:05

s/fetchByInternalId/fetchByModelId

1df3779

Move try/catch + move markAsFailed for preprocessing

b3ac7ec

s/fileRes/file

0751f7c

Remove types for FileId

cb9277d

🙈

caa6f15

Implement use cases for pre-processing

d62ad30

spolu approved these changes Jul 2, 2024

View reviewed changes

flvndvd merged commit 9c592c4 into main Jul 2, 2024
4 checks passed

flvndvd deleted the flav/file-upload-api branch July 2, 2024 11:00

flvndvd mentioned this pull request Jul 2, 2024

Flav/input bar uses file api #5997

Merged

flvndvd mentioned this pull request Jul 8, 2024

File upload API #5976

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce file upload api #5977

Introduce file upload api #5977

flvndvd commented Jul 1, 2024 •

edited

Loading

flvndvd Jul 2, 2024

spolu left a comment

spolu Jul 2, 2024

flvndvd Jul 2, 2024

spolu Jul 2, 2024

flvndvd Jul 2, 2024

spolu Jul 2, 2024

spolu Jul 2, 2024

flvndvd Jul 2, 2024

spolu Jul 2, 2024

flvndvd Jul 2, 2024

spolu Jul 2, 2024

spolu Jul 2, 2024

flvndvd Jul 2, 2024

spolu Jul 2, 2024

spolu Jul 2, 2024

spolu Jul 2, 2024

flvndvd Jul 2, 2024

spolu left a comment

Introduce file upload api #5977

Introduce file upload api #5977

Conversation

flvndvd commented Jul 1, 2024 • edited Loading

Description

Summary

Key Changes

Memory and Performance Considerations

Known Issues and Updates

Risk

Deploy Plan

Choose a reason for hiding this comment

spolu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spolu left a comment

Choose a reason for hiding this comment

flvndvd commented Jul 1, 2024 •

edited

Loading