Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Server side implementation of file uploading feature #96

Merged
merged 37 commits into from
Jun 1, 2020

Conversation

yong-jie
Copy link
Member

@yong-jie yong-jie commented May 19, 2020

Problem

Under the premise that url objects cannot be deleted, the creation of a link + upload file to s3 step cannot be atomic when it is done client side.

Solution

This PR modifies the shortUrl creation endpoint to accept files as an optional parameter that enables the file-uploading process to S3.

Additional Changes

This PR also introduces Joi validation to protect the endpoint handlers.

Environment Variables

These new env vars will be introduced in docker-compose.yml to allow aws-sdk to interface with S3. They are necessary for development only when working with file links. These env vars will become obsolete once #117 is complete.

  • AWS_ACCESS_KEY_ID
  • AWS_SECRET_ACCESS_KEY

src/server/index.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@liangyuanruo liangyuanruo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

overall approach lgtm! apart from the questions raised, please help to document the API for the benefit of the next developer, as well as the lessons learnt on transactions, 2-phase commit etc. for posterity.

@yong-jie yong-jie marked this pull request as ready for review May 21, 2020 16:55
Copy link
Contributor

@liangyuanruo liangyuanruo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes suggested as per comments.

liangyuanruo
liangyuanruo previously approved these changes May 26, 2020
@yong-jie yong-jie requested a review from liangyuanruo May 29, 2020 04:40
* Move otp-related endpoints in login to Joi

* Add types to the shortUrl destructured from params in redirect.

The reason I opted not to move any of the validation in this function
into Joi was because it has its own set of behaviours depending on
each validation failure, such as rendering 404 pages etc. This is not
something that can be done by a Joi validator.

* Remove obsolete tests

* Minor code cleanup for Joi validation (#132)

* Name a new VerifyOtpRequest type after the API

* Move types into /types folder

* Rename transitionPage.ts from camelCase to kebab-case

* Remove duplicate type file

Co-authored-by: Yuan Ruo <liangyuanruo@gmail.com>
JasonChong96
JasonChong96 previously approved these changes Jun 1, 2020
Copy link
Contributor

@JasonChong96 JasonChong96 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Copy link
Contributor

@kylerwsm kylerwsm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, added some comments

src/server/api/redirect.ts Show resolved Hide resolved
src/server/api/login/validators.ts Show resolved Hide resolved
docker-compose.yml Show resolved Hide resolved
@yong-jie yong-jie merged commit db58007 into develop Jun 1, 2020
@yong-jie yong-jie deleted the file-upload-server-side branch June 1, 2020 10:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants