Add Models using TypeORM #287
Conversation
|
Could you order imports by the importance?
|
| @@ -21,8 +21,22 @@ export default { | |||
| }, | |||
|
|
|||
| async create(req: Request, res: Response) { | |||
| const { name, description, category, details } = req.body; | |||
| const chapter = new Chapter({ name, description, category, details }); | |||
| const { | |||
There was a problem hiding this comment.
This unfortunately wont work at property at runtime, beacuse here we implicitly convert location (string) from body into a Location object, change this to location_id and creator_id (for, now later we'll get that from logged in user), and query location and user before inserting a new chapter
There was a problem hiding this comment.
@Zeko369 i think at runtime TypeORM converts an id to an entity object
There was a problem hiding this comment.
Yeah, I was wrong, it actually work, but sending location_id and creator_id doesn't, so do you think we should change it to location and creator in api docs or make 2 more queries to the db here?
There was a problem hiding this comment.
@Zeko369 sorry for the delay. I think we should change it to location and creator. Since, TypeOrm fails if it is not a valid id we can choose to provide helpful error message instead of making 2 more request to the db.
There was a problem hiding this comment.
Alright cool, I am on it.
| import { Chapter } from './Chapter'; | ||
|
|
||
| @Entity({ name: 'user_bans' }) | ||
| export class UserBan extends BaseModel { |
There was a problem hiding this comment.
Could we maybe instead of doing this create a joinTable https://typeorm.io/#/many-to-many-relations? Also the same for EventSponsor and UserChapter
There was a problem hiding this comment.
yeah, we could. I was thinking of a situation where it doesn't extend BaseModel but I didn't test it first.
There was a problem hiding this comment.
https://typeorm.io/#/many-to-many-relations/many-to-many-relations-with-custom-properties 😢 , for them to have createdAt and updatedAt we need custom entities, so maybe we could update BaseModel to only have createdAt and updatedAt and add ID to all models that need it?
There was a problem hiding this comment.
I think what we currently have will also work since UserBan also needs an id.
ankorGH
force-pushed
the
feature/add-db-models
branch
from
f211d16
to
7f9941a
Compare
|
@QuincyLarson I am great. You? Hopefully, I and @Zeko369 will get this PR soon. |
|
|
||
| try { | ||
| await chapter.save(); | ||
| res.status(201).json(chapter); | ||
| } catch (e) { | ||
| if (e.code === '23503' && e.message.includes('foreign key constraint')) { |
There was a problem hiding this comment.
This will be a bitch to setup so maybe we should extract this for later reuse
| return res.status(404).json({ message: 'creator not found' }); | ||
| } | ||
| } | ||
|
|
||
| res.status(500).json({ error: e }); |
There was a problem hiding this comment.
Is it good idea to throw full error stack in api response?
| }); | ||
|
|
||
| try { | ||
| await chapter.save(); | ||
| res.status(201).json(chapter); | ||
| } catch (e) { | ||
| if (e.code === '23503' && e.message.includes('foreign key constraint')) { | ||
| if (e.detail.includes('location')) { |
There was a problem hiding this comment.
Just an edge case, if e.detail is null or undefined than it will throw an error like ".includes of undefined". To prevent this we can use destructing with default value.
Update README.md).masterbranch of Chapter.Add models using typeorm as shown in db diagram.