Unify Logic in Image APIs #647

patriciaahuang · 2024-10-01T22:12:58Z

Summary

Unified the logic in the backend image APIs and the frontend image API, since there are starting to be more use cases for images but the logic is all the same.

Notion/Figma Link

https://www.notion.so/Unify-Logic-in-Image-APIs-1180ad723ce18078b289c240371aea60?pvs=4

Test Plan

I checked that team event proof images and member profile images still works properly, using the same method as in PR #642.

dti-github-bot · 2024-10-01T22:13:11Z

[diff-counting] Significant lines: 144.

andrew032011 · 2024-10-02T01:50:43Z

backend/src/api.ts

@@ -315,6 +321,20 @@ loginCheckedPut('/coffee-chat', async (req, user) => ({
  coffeeChats: await updateCoffeeChat(req.body, user)
 }));

+// Coffee Chat Proof Image


What if we unified this on the backend as well? So just have a generic /image/:name(*) (get), /image-signed-url/:name(*) (get), /image/:name(*) (delete) endpoints? I think the logic is the same across all the different use-cases anyways.

Instead of the utils, we'd have just three endpoints total (as opposed to 9)

You should keep the image names the same though, so I would still make sure the images, when they're created, are prefixed correctly (i.e. eventProofs/, coffeeChatProofs/, member-image/)

Oops, I missed unifying this file as well. Will do that!

Even better if it's just prefixed with /files to be super generic :D

henry-li-06 · 2024-10-02T15:27:14Z

backend/src/utils/image-utils.ts

+export const setImage = async (name: string): Promise<string> => {
+  const file = bucket.file(`${name}.jpg`);
+  const signedURL = await file.getSignedUrl({
+    action: 'write',
+    version: 'v4',
+    expires: Date.now() + 15 * 60000 // 15 min
+  });
+  return signedURL[0];
+};
+
+/**
+ * Gets image for member
+ * @param name - the name of the image
+ * @throws NotFoundError if the requested image does not exist
+ * @returns a Promise to the signed URL to the image file
+ */
+export const getImage = async (name: string): Promise<string> => {
+  const file = bucket.file(`${name}.jpg`);
+  const fileExists = await file.exists().then((result) => result[0]);
+  if (!fileExists) {
+    throw new NotFoundError(`The requested image (${name}) does not exist`);
+  }
+  const signedUrl = await file.getSignedUrl({
+    action: 'read',
+    expires: Date.now() + 15 * 60000
+  });
+  return signedUrl[0];
+};


ok so let's rename these while you're at it (ik i'm the one that gave them shitty names in the first place)

getWriteSignedURL

getReadSignedURL

or something a long those lines

henry-li-06 · 2024-10-02T15:28:39Z

backend/src/utils/image-utils.ts

+ * @returns a Promise to the signed URL to the image file
+ */
+export const setImage = async (name: string): Promise<string> => {
+  const file = bucket.file(`${name}.jpg`);


on a side note - probably better off including the file extension in the actual file path itself instead of assuming they're all .jpg

andrew032011 · 2024-10-05T02:01:44Z

backend/src/API/imageAPI.ts

+    })
+  );
+
+  images


I think this value looks like it's floating and nothing is assigned to it?

andrew032011 · 2024-10-05T02:25:46Z

I'm unable to fetch existing images anymore here. Did we change how we construct the image paths? The image paths should still be the same (to maintain backwards compatibility with the existing images we're storing), even if we're changing the endpoints.

patriciaahuang · 2024-10-05T02:58:01Z

I'm unable to fetch existing images anymore here. Did we change how we construct the image paths? The image paths should still be the same (to maintain backwards compatibility with the existing images we're storing), even if we're changing the endpoints.

I included the file extension in the file path to avoid assuming all images are .jpg. Before, .jpg was manually added to the file path name when getting an image in imageAPI.ts, so removing that will cause existing images to be fetched incorrectly (instead of fetching pathname.jpg it'll try fetching just pathname). Should I revert this change to maintain backwards compatibility?

andrew032011 · 2024-10-05T03:03:22Z

I'm unable to fetch existing images anymore here. Did we change how we construct the image paths? The image paths should still be the same (to maintain backwards compatibility with the existing images we're storing), even if we're changing the endpoints.

I included the file extension in the file path to avoid assuming all images are .jpg. Before, .jpg was manually added to the file path name when getting an image in imageAPI.ts, so removing that will cause existing images to be fetched incorrectly (instead of fetching pathname.jpg it'll try fetching just pathname). Should I revert this change to maintain backwards compatibility?

Wow, good find! In an ideal world we would go in and change all images to not have the .jpg suffix and patch this, so that your fix can stay and we no longer suffix our images with .jpg. Of course, there is always a risk with such a change and trying to change all these images might be more work than necessary.

Let's keep the .jpg for now! (rationale: This gets us unblocked the quickest. What might be easier is at the end of this semester, we can remove all images in firestore and start afresh next semester. And if it really bothers us this semester, we can always remove it later by writing a script to change all the images)

patriciaahuang · 2024-10-05T03:05:22Z

I'm unable to fetch existing images anymore here. Did we change how we construct the image paths? The image paths should still be the same (to maintain backwards compatibility with the existing images we're storing), even if we're changing the endpoints.

I included the file extension in the file path to avoid assuming all images are .jpg. Before, .jpg was manually added to the file path name when getting an image in imageAPI.ts, so removing that will cause existing images to be fetched incorrectly (instead of fetching pathname.jpg it'll try fetching just pathname). Should I revert this change to maintain backwards compatibility?

Wow, good find! In an ideal world we would go in and change all images to not have the .jpg suffix and patch this, so that your fix can stay and we no longer suffix our images with .jpg. Of course, there is always a risk with such a change and trying to change all these images might be more work than necessary.

Let's keep the .jpg for now! (rationale: This gets us unblocked the quickest. What might be easier is at the end of this semester, we can remove all images in firestore and start afresh next semester. And if it really bothers us this semester, we can always remove it later by writing a script to change all the images)

Sounds good, thanks for catching this!

andrew032011 · 2024-10-05T04:11:33Z

frontend/src/components/Forms/UserProfile/UserProfileImage.tsx

@@ -31,7 +31,7 @@ const UserProfileImage: React.FC = () => {
        .then((res) => res.blob())
        .then((blob) => {
          imageURL = window.URL.createObjectURL(blob);
-          ImagesAPI.uploadMemberImage(blob);
+          ImagesAPI.uploadImage(blob, `image-signed-url/${userInfo ? userInfo.email : ''}`);


we can move this prefix image-signed-url/ into the ImagesAPI.uploadImage function

andrew032011 · 2024-10-05T04:11:49Z

frontend/src/components/Forms/TeamEventCreditsForm/TeamEventsCreditDashboard.tsx

@@ -63,7 +63,7 @@ const TeamEventCreditDashboard = (props: {
          headerMsg: 'Team Event Attendance Deleted!',
          contentMsg: 'Your team event attendance was successfully deleted!'
        });
-        ImagesAPI.deleteEventProofImage(attendance.image);
+        ImagesAPI.deleteImage(`image/${attendance.image}`);


we can move this prefix image/ into the ImagesAPI.deleteImage function

andrew032011 · 2024-10-05T04:29:55Z

frontend/src/API/ImagesAPI.ts

-    return this.getSignedURL().then((url) => {
-      const headers = { 'content-type': 'image/jpeg' };
+  public static uploadImage(body: Blob, name: string): Promise<void> {
+    return this.getSignedURL(`${name}`).then((url) => {


Suggested change

return this.getSignedURL(`${name}`).then((url) => {

return this.getSignedURL(name).then((url) => {

andrew032011 · 2024-10-05T04:30:06Z

frontend/src/API/ImagesAPI.ts

-      const headers = { 'content-type': 'image/jpeg' };
+  public static uploadImage(body: Blob, name: string): Promise<void> {
+    return this.getSignedURL(`${name}`).then((url) => {
+      const headers = { 'content-type': `${body.type}` };


Suggested change

const headers = { 'content-type': `${body.type}` };

const headers = { 'content-type': body.type };

andrew032011 · 2024-10-05T04:44:36Z

frontend/src/API/ImagesAPI.ts

-      (res) => res.data
-    );
+  private static getSignedURL(name: string): Promise<string> {
+    const responseProm = APIWrapper.get(`${backendURL}/${name}`).then((res) => res.data);


Suggested change

const responseProm = APIWrapper.get(`${backendURL}/${name}`).then((res) => res.data);

const responseProm = APIWrapper.get(`${backendURL}/image-signed-url/${name}`).then((res) => res.data);

andrew032011 · 2024-10-05T04:44:47Z

frontend/src/API/ImagesAPI.ts

-    return this.getSignedURL().then((url) => {
-      const headers = { 'content-type': 'image/jpeg' };
+  public static uploadImage(body: Blob, name: string): Promise<void> {
+    return this.getSignedURL(`image-signed-url/${name}`).then((url) => {


Suggested change

return this.getSignedURL(`image-signed-url/${name}`).then((url) => {

return this.getSignedURL(name).then((url) => {

andrew032011 · 2024-10-05T05:05:54Z

~~Just tested manually and if you upload and then try to get a TEC image, it does not fetch the image properly. I think it has something to do with how the file is named when it is uploaded.~~
Ignore me!

patriciaahuang added 3 commits September 27, 2024 17:25

Implement coffee chat image API

645ef8c

Run formatter

1ac2f21

Unify logic in image apis

75e86d2

patriciaahuang requested a review from a team as a code owner October 1, 2024 22:12

patriciaahuang mentioned this pull request Oct 1, 2024

Implement Coffee Chat Image API #642

Closed

andrew032011 reviewed Oct 2, 2024

View reviewed changes

Merge branch 'main' into image-api

3a45673

henry-li-06 reviewed Oct 2, 2024

View reviewed changes

Unify api.ts,, rename functions, include file extension in path

4972b1c

andrew032011 reviewed Oct 5, 2024

View reviewed changes

Fix floating value

0a6896c

Revert adding file extensionn to pathname

e6fe838

andrew032011 reviewed Oct 5, 2024

View reviewed changes

Fix profile image pathname

9cb2e40

andrew032011 reviewed Oct 5, 2024

View reviewed changes

Fix prefix placement + syntax

7263dca

andrew032011 approved these changes Oct 5, 2024

View reviewed changes

patriciaahuang added 2 commits October 5, 2024 00:58

Remove unused functions + fix nit

3a98726

Run formatter

06a929c

patriciaahuang merged commit f18e6d4 into main Oct 5, 2024
17 checks passed

patriciaahuang deleted the image-api branch October 5, 2024 05:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unify Logic in Image APIs #647

Unify Logic in Image APIs #647

patriciaahuang commented Oct 1, 2024 •

edited by andrew032011

Loading

dti-github-bot commented Oct 1, 2024 •

edited

Loading

andrew032011 Oct 2, 2024

andrew032011 Oct 2, 2024

patriciaahuang Oct 2, 2024

henry-li-06 Oct 2, 2024

henry-li-06 Oct 2, 2024 •

edited

Loading

henry-li-06 Oct 2, 2024

andrew032011 Oct 5, 2024

andrew032011 commented Oct 5, 2024 •

edited

Loading

patriciaahuang commented Oct 5, 2024

andrew032011 commented Oct 5, 2024

patriciaahuang commented Oct 5, 2024

andrew032011 Oct 5, 2024

andrew032011 Oct 5, 2024

andrew032011 Oct 5, 2024

andrew032011 Oct 5, 2024

andrew032011 Oct 5, 2024

andrew032011 Oct 5, 2024

andrew032011 commented Oct 5, 2024 •

edited

Loading

+                  })
+                );
+                images

	return this.getSignedURL(`${name}`).then((url) => {
	return this.getSignedURL(name).then((url) => {

	const headers = { 'content-type': `${body.type}` };
	const headers = { 'content-type': body.type };

	const responseProm = APIWrapper.get(`${backendURL}/${name}`).then((res) => res.data);
	const responseProm = APIWrapper.get(`${backendURL}/image-signed-url/${name}`).then((res) => res.data);

	return this.getSignedURL(`image-signed-url/${name}`).then((url) => {
	return this.getSignedURL(name).then((url) => {

Unify Logic in Image APIs #647

Unify Logic in Image APIs #647

Conversation

patriciaahuang commented Oct 1, 2024 • edited by andrew032011 Loading

Summary

Notion/Figma Link

Test Plan

dti-github-bot commented Oct 1, 2024 • edited Loading

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

henry-li-06 Oct 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrew032011 commented Oct 5, 2024 • edited Loading

patriciaahuang commented Oct 5, 2024

andrew032011 commented Oct 5, 2024

patriciaahuang commented Oct 5, 2024

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

andrew032011 commented Oct 5, 2024 • edited Loading

patriciaahuang commented Oct 1, 2024 •

edited by andrew032011

Loading

dti-github-bot commented Oct 1, 2024 •

edited

Loading

henry-li-06 Oct 2, 2024 •

edited

Loading

andrew032011 commented Oct 5, 2024 •

edited

Loading

andrew032011 commented Oct 5, 2024 •

edited

Loading