NW6 | Rabia Avci | Full-Stack-Project | Week-2 | Post new video endpoint #24
Conversation
✅ Deploy Preview for watch-next-cyf ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
There was a problem hiding this comment.
Overall, this works just fine, but there are some important issues that are worth reviewing. You'll find that as your applications get more complex, there are more of these issues. Embrace them all learn the why, they're intresting and will help you grow your knowledge
| @@ -3,8 +3,10 @@ | |||
| DROP TABLE IF EXISTS videos CASCADE; | |||
|
|
|||
| CREATE TABLE videos ( | |||
| id SERIAL PRIMARY KEY, | |||
| return res.status(422).json({ message: "Title field is required" }); | ||
| } | ||
| if (!req.body.src) { | ||
| return res.status(422).json({ message: "src field is required" }); |
There was a problem hiding this comment.
It is good to see you using HTTP status codes other than 200 and thinking carefully about the right one to use. However, it's worth being aware that there are conventions - 400 bad request is usually used for this situation. It makes it very clear that the problem is on the users side.
The message is a nice touch and would be much appreciated when debugging!
| return res.status(422).json({ message: "src field is required" }); | ||
| } | ||
| const result = await db.query( | ||
| `INSERT INTO videos (title,src) VALUES ('${req.body.title}','${req.body.src}') RETURNING id` |
There was a problem hiding this comment.
This line works, but sadly it's horribly insecure thanks to SQL injection. Anyone could delete your videos table just by sending a src with a value like this:
Fakesrc');DROP TABLE videos;
I don't know if Kieth mentioned this, and even if he did, it's easy not to make the connection so there’s no shame in having made the mistake. However, you need to get into the habit of doing the right thing have a look at https://stackoverflow.com/questions/58174695/prevent-sql-injection-with-nodejs-and-postgres
| `INSERT INTO videos (title,src) VALUES ('${req.body.title}','${req.body.src}') RETURNING id` | ||
| ); | ||
| const newVideoId = result.rows[0].id; | ||
| res.status(200).json({ success: true, data: { id: newVideoId } }); |
There was a problem hiding this comment.
It's good that you're sending back the video ID that's extremely useful 👍
| app.use((err, req, res, next) => { | ||
| res.status(500).json({ message: "An unexpected error occurred" }); | ||
| next(err); | ||
| }); |
There was a problem hiding this comment.
So good to see error handling and the health endpoint but there's a major issue with doing it just in this file. None of the stuff in this file ever runs in Netifly (the starting point for Netlifly is functions/app.mjs), so this code only works on your local machine. If this is intentional, that's fine, but please add a comment to clarify :)
Completed these tasks for this ticket: