Richard/docs/images #36

chomosuke · 2021-08-25T11:25:27Z

I've decided to separate images upload from card update, considering that we might want to upload the image while user is still filling out other details. To avoid garbage images that isn't attach to any business card building up on the database and taking space, we can prune the images for a user every time that user uploads an image.

This means I'll also need to:

modify card schema to include the image url instead of the actual buffer
modify user schema so that it includes another array containing images along with their ids (could be objectId)

related to #30

I've realized that we can prune on every new image upload

shangzhel · 2021-08-25T15:10:08Z

The card JSON sent over the network does not need to have a 1-to-1 correspondence to the database Card schema. If we don't want garbage images building up in MongoDB, we can simply embed the image data as a buffer inside each Card. When a Card gets erased in MongoDB, the image buffer goes along with it.

In regards to the consideration for uploading ahead of time, when we are restricting image sizes to be under 1 MB, it will take mere seconds to upload. Now that I have given it more thought, it seems more dangerous to split image and card update requests, because there can be a race condition where either the request to upload the image arrives at the server or the request to create a card does, but not both, and there will be inconsistencies. Requiring all card update information to travel within a single request avoids this: either the entire request fails, or the entire request succeeds.

The image get endpoint is still useful, because GET /api/me will not transmit the card image data so it must still be done separately.

chomosuke · 2021-08-25T21:46:08Z

@shangzhel I don't think that's the case.

First of all, an 1mb image taking mere seconds to upload might not always be a valid assumption, because internet speed such as 50kbps exist in places such as New Zealand.

Secondly, if we were to enforce that each imagine has to be bounded to a card then yes, we will have inconsistency that'll break the requirements. However, this need not be the case. For that if everything is structured as I've proposed, a card update can not succeed without the imagine upload, because it won't have the imagine URL, which can only be obtained in the response by the server after the success of the image upload. And if the imagine upload succeeds without the card update, nothing will happen, the previous imagine, or lack there of will be retained and we'll just have one imagine on the server not bounded to any card, which will be pruned at the next imagine upload.

shangzhel · 2021-08-26T03:17:06Z

But can you ensure that the image upload and card create/update operations are atomic?
Consider concurrent sessions, a successful sequence would look like this:

Time	Session 1	Session 2	Resources on server
0	Upload image, gets `url1`		`url1`
1	Create card with `url1`, succeeds		`card1:url1`
2		Upload image, prune, nothing unused, gets `url2`	`card1:url1` `url2`
3		Create card with `url2`, succeeds	`card1:url1` `card2:url2`

But there can be a race condition, consider the following sequence of requests:

Time	Session 1	Session 2	Resources on server
0	Upload image, gets `url1`		`url1`
1		Upload image, prune, `url1` unused, replaced with `url2`	`url2`
2	Create card with `url1`, fails because `url1` got deleted		`url2`
3		Create card with `url2`, succeeds	`card1:url2`

shangzhel

Some adjustments needed to fit the conventions that the other endpoints use. Refer to the code comments.

shangzhel · 2021-08-26T03:19:55Z

docs/api/Card.ts

@@ -9,6 +9,7 @@ export interface Card {
  email: string;
  jobTitle: string;
  company: string;
+  imageUrl?: string;


Instead of keeping an entire URL, it can keep just an ObjectId to the image in the other array.

May I know which other array you are referring to?
@shangzhel

The array no longer exist as now there's a 1 to 1 relationship between the image and the cards, I've modify the API accordingly

shangzhel · 2021-08-26T03:20:23Z

docs/api/images.md

+
+Response body:
+```ts
+import type { ObjectId } from './ObjectId';


ObjectId is unused here, no need to import it.

shangzhel · 2021-08-26T03:20:46Z

docs/api/images.md

+import type { ObjectId } from './ObjectId';
+
+interface ImagePostResponse {
+    imageUrl: string;


Can be an Id instead of a URL.

shangzhel · 2021-08-26T03:21:32Z

docs/api/images.md

+}
+```
+
+# /api/image/imageId


Use the route parameter syntax.

chomosuke · 2021-08-26T03:41:39Z

@shangzhel This is a very valid scenario that I did not consider before.

There are three way that I can think of to address this.

First is to make image and card update a single request, as you've proposed.

Second is to set a time stamp on images to say, prevent the image from getting pruned within a day.

Third is that, if when attempting to update card, the server complain image not found. Default to another end point which upload the image and update the card on a single POST.

Tell me what you think is best (maybe justify) and I'll just do that.

Also regarding to your requested changes about /api/image/imageId, I thought we wanted to keep the route a get request so that we can simply feed the url into the src property in whatever image element on the frontend we'll use.

waltervan00 · 2021-08-26T04:04:06Z

Also regarding to your requested changes about /api/image/imageId, I thought we wanted to keep the route a get request so that we can simply feed the url into the src property in whatever image element on the frontend we'll use.

When we were discussing image upload, I remembered that the binary data of the image would be stored on the database. Is it possible to generate a url for you to open the image with an external browser? What if we could turn binary data sent from the backend into a url to display on in the img element?

If we let the image be transmitted as a binary buffer over the api, will our image validation library still be able to validate it? If so we can create an image validation middleware for endpoints #23 and #25. Patching a cards details would be an overwrite of its contents in this situation.

Alternatively I've been reading on the transactions feature offered by Mongoose: https://mongoosejs.com/docs/transactions.html
You could store in back-end cache each user's editing transaction. It may provide the atomicity we need:

User starts a transaction
User edits details
User saves details, patch request is sent.
Server saves new card details under transaction.
Server closes the transaction

1a. User already has a previous transaction when starting a new one

Abort previous transaction and start a new one

2a. User uploads image on front end

POST image request is stored in database under transaction

4a. Image has been uploaded from 2a.

Card on database is linked to the saved image under the transaction

*a. User closes page

Transaction remains in server

*b. User cancels transaction

Transaction is aborted.

I'm slightly worried about Heroku's environment between the two api calls. How often does it clean out its deployment environment? If we have to consider more permanent transaction management solutions this may not be valid.

waltervan00 · 2021-08-26T04:08:44Z

We could defer this issue to Sprint 2 once we have at least the basic functions down. Then we can have a more focused attempt at it. When we have some front-end developed, we also have to identify how to insert the user's image files into the front end interface (allowing for binary encoding).

chomosuke · 2021-08-26T04:14:10Z

@waltervan00

Yes, and yes, that's exactly what I'm saying for /api/image/imageId, that would be the url we feed into the img element, or used to open the image in a new window (though have to be the same browser as we need to cookie for authentication).

Yes, and very likely we will.

One problem with the transaction is that, overtime we'll have unaborted transaction garbages on the server, not to mention that managing those transaction between all the different endpoint would be difficult to say the least as far as I can imagine.

shangzhel · 2021-08-26T04:19:18Z

@chomosuke

set a time stamp on images to say, prevent the image from getting pruned within a day.

Default to another end point which upload the image and update the card on a single POST.

The more complicated or split APIs get the more difficult it is to get right. This is because REST APIs by design are stateless, which means we are not allowed to assume that any sequence of more than one requests actually completes. This is why I propose each API completes exactly one action.

Also regarding to your requested changes about /api/image/imageId, I thought we wanted to keep the route a get request so that we can simply feed the url into the src property in whatever image element on the frontend we'll use.

I had forgotten about this, but it is not the best either way so I will adjust my code comment to propose a different change.

@waltervan00

You could store in back-end cache each user's editing transaction.

That's not how transactions work. MongoDB transactions are database-wide. This means once a transaction starts, no other transaction can begin, regardless of whether it is the same user or otherwise. If our server begins a transaction and the client disconnects, it becomes a denial-of-service attack while the server waits for the next message from the client. There is also no way to "store a transaction". They are not documents, transactions is a MongoDB function, not data.

chomosuke · 2021-08-26T04:50:32Z

@shangzhel

Fair enough, I will move forward with the first way.

waltervan00 · 2021-08-26T05:24:30Z

Fair enough, I will move forward with the first way.

@shangzhel
Will this affect the processes #23 and #25 and /api/card/ POST, PATCH? We can implement a middleware for validating / processing the image binary -- a common feature between these api calls. And each endpoint can work as if overwriting or creating a new card.

chomosuke · 2021-08-26T06:08:06Z

@waltervan00 now I'm thinking that I might have to take over #23 and #25.

Or alternatively, I'll wait for you to finish writing, and then I'll start working on it, which might be a bit inefficient.

shangzhel

Just minor changes left, then it should be good to go.

shangzhel · 2021-08-26T06:23:05Z

docs/api/Image.ts

@@ -0,0 +1 @@
+type Image = string;


Needs to export the type, and add a newline at the end of the file.

shangzhel · 2021-08-26T06:26:30Z

docs/api/card.md

@@ -1,12 +1,16 @@
 # /api/card
 ## PUT
 - Authenticated
+- 413 Payload Too Large if image string is larger than 1 MB


Verifying only the image string will require first processing the entire request body which can itself DoS the server. Instead we should limit the total request size for all requests. The server can do this with a middleware. Instead of noting this here, state this restriction in docs/api/README.md.

shangzhel · 2021-08-26T06:44:31Z

docs/api/card.md


 Request body:
 ```ts
 import type { Card } from './Card';

-type CardPatchRequest = Pick<Card, 'id'> & Partial<Omit<Card, 'id'>>;
+interface CardPatchRequest extends Pick<Card, 'id'> & Partial<Omit<Card, 'id' | 'hasImage'>> {
+  image?: Image;


There needs to be a way to indicate the intention to "remove image". I think it will be best to assign null to image, so the type needs to be Image | null instead of just Image.

shangzhel · 2021-08-26T06:46:17Z

docs/api/images.md

@@ -0,0 +1,8 @@
+# /api/image/:cardId


Either change the endpoint path to /api/images/:cardId to match the file name or rename the file to image.md.

shangzhel · 2021-08-26T06:47:50Z

docs/api/card.md

@@ -34,12 +38,15 @@ Response body: None
 ## PATCH
 - Authenticated
 - 410 Gone if card does not exist.
+- 413 Payload Too Large if image string is larger than 1 MB


Same as above, instead of noting this here, state this restriction for all requests in docs/api/README.md.

waltervan00 · 2021-08-26T07:05:34Z

@waltervan00 now I'm thinking that I might have to take over #23 and #25.

Or alternatively, I'll wait for you to finish writing, and then I'll start working on it, which might be a bit inefficient.

@chomosuke
I can first write up the Mongoose code first, take this time to research how the user is able to get the images from their computer into the UI.

shangzhel · 2021-08-26T07:20:59Z

With window.showOpenFilePicker

… image

shangzhel

Looks good to me.

waltervan00

lgtm

chomosuke added 2 commits August 25, 2021 21:05

modified API for images

a884e17

removed prune route

2943a4f

I've realized that we can prune on every new image upload

chomosuke requested review from shangzhel, waltervan00, oldbugo and pep1032314 August 25, 2021 11:25

chomosuke added the documentation Improvements or additions to documentation label Aug 25, 2021

chomosuke added this to the Sprint 1 milestone Aug 25, 2021

made image URL optional in Card.ts

1219f8a

chomosuke removed request for oldbugo and pep1032314 August 25, 2021 12:38

chomosuke self-assigned this Aug 25, 2021

shangzhel requested changes Aug 26, 2021

View reviewed changes

converted API to support atomic card update alongside with image

6106375

chomosuke requested a review from shangzhel August 26, 2021 05:41

shangzhel requested changes Aug 26, 2021

View reviewed changes

chomosuke added 4 commits August 26, 2021 17:36

modified doc to restrict size for all requests

c10916e

modified doc to allow card PATCH update to advise intention to remove…

fc4ff1d

… image

rename images.md to image.md for consistency

9e4f4dc

exported image type in Image.ts

24b5d27

chomosuke requested a review from shangzhel August 26, 2021 07:38

Fix grammar and punctuation.

24b56e3

shangzhel approved these changes Aug 26, 2021

View reviewed changes

waltervan00 approved these changes Aug 27, 2021

View reviewed changes

chomosuke merged commit b73e47d into docs Aug 27, 2021

chomosuke deleted the richard/docs/images branch August 27, 2021 05:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Richard/docs/images #36

Richard/docs/images #36

chomosuke commented Aug 25, 2021 •

edited

Loading

shangzhel commented Aug 25, 2021 •

edited

Loading

chomosuke commented Aug 25, 2021 •

edited

Loading

shangzhel commented Aug 26, 2021

shangzhel left a comment

shangzhel Aug 26, 2021

waltervan00 Aug 26, 2021

chomosuke Aug 26, 2021

shangzhel Aug 26, 2021

shangzhel Aug 26, 2021

shangzhel Aug 26, 2021 •

edited

Loading

chomosuke commented Aug 26, 2021 •

edited

Loading

waltervan00 commented Aug 26, 2021

waltervan00 commented Aug 26, 2021 •

edited

Loading

chomosuke commented Aug 26, 2021

shangzhel commented Aug 26, 2021

chomosuke commented Aug 26, 2021

waltervan00 commented Aug 26, 2021 •

edited

Loading

chomosuke commented Aug 26, 2021

shangzhel left a comment

shangzhel Aug 26, 2021

shangzhel Aug 26, 2021

shangzhel Aug 26, 2021

shangzhel Aug 26, 2021

shangzhel Aug 26, 2021

waltervan00 commented Aug 26, 2021

shangzhel commented Aug 26, 2021

shangzhel left a comment

waltervan00 left a comment

Richard/docs/images #36

Richard/docs/images #36

Conversation

chomosuke commented Aug 25, 2021 • edited Loading

shangzhel commented Aug 25, 2021 • edited Loading

chomosuke commented Aug 25, 2021 • edited Loading

shangzhel commented Aug 26, 2021

shangzhel 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

shangzhel Aug 26, 2021 • edited Loading

Choose a reason for hiding this comment

chomosuke commented Aug 26, 2021 • edited Loading

waltervan00 commented Aug 26, 2021

waltervan00 commented Aug 26, 2021 • edited Loading

chomosuke commented Aug 26, 2021

shangzhel commented Aug 26, 2021

chomosuke commented Aug 26, 2021

waltervan00 commented Aug 26, 2021 • edited Loading

chomosuke commented Aug 26, 2021

shangzhel 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

waltervan00 commented Aug 26, 2021

shangzhel commented Aug 26, 2021

shangzhel left a comment

Choose a reason for hiding this comment

waltervan00 left a comment

Choose a reason for hiding this comment

chomosuke commented Aug 25, 2021 •

edited

Loading

shangzhel commented Aug 25, 2021 •

edited

Loading

chomosuke commented Aug 25, 2021 •

edited

Loading

shangzhel Aug 26, 2021 •

edited

Loading

chomosuke commented Aug 26, 2021 •

edited

Loading

waltervan00 commented Aug 26, 2021 •

edited

Loading

waltervan00 commented Aug 26, 2021 •

edited

Loading