-
Notifications
You must be signed in to change notification settings - Fork 275
SOlution 1.0 #131
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
base: master
Are you sure you want to change the base?
SOlution 1.0 #131
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,153 @@ | ||
| import { messageService } from '../services/message.service.js'; | ||
| import { roomService } from '../services/room.service.js'; | ||
| import { Message } from '../data/message.js'; | ||
| import { userService } from '../services/user.service.js'; | ||
|
|
||
| const getRoomInfoByRoomId = async (req, res) => { | ||
| const { roomId } = req.params; | ||
|
|
||
| const id = parseInt(roomId, 10); | ||
|
|
||
| if (isNaN(id)) { | ||
| return res.status(400).json({ message: 'Invalid roomId' }); | ||
| } | ||
|
|
||
| const room = await roomService.findRoomById(id); | ||
|
|
||
| if (!room) { | ||
| return res.status(404).json({ message: 'Room not Found' }); | ||
| } | ||
|
|
||
| const messages = await messageService.findRoomsMessages(id); | ||
|
|
||
| return res.status(200).json({ | ||
| room: { | ||
| id: room.id, | ||
| name: room.name, | ||
| }, | ||
| messages, | ||
| }); | ||
| }; | ||
|
|
||
| const createRoom = async (req, res) => { | ||
| const { name } = req.body; | ||
| const user = req.user; | ||
|
|
||
| if (!name) { | ||
| return res.status(400).json({ message: 'Room name is required' }); | ||
| } | ||
|
|
||
| const newRoom = await roomService.createRoom(name); | ||
|
|
||
| if (!newRoom) { | ||
| return res.status(400).json({ message: 'Cannot create the room' }); | ||
| } | ||
|
|
||
| await newRoom.addUser(user); | ||
|
Comment on lines
+34
to
+46
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. createRoom assumes req.user exists and then does newRoom.addUser(user). Ensure req.user is present (return 401 if not authenticated) and confirm whether addUser expects the user instance or an id. If it expects an id, use newRoom.addUser(user.id). Add an explicit check before calling addUser to avoid runtime errors when req.user is absent. |
||
|
|
||
| const roomUsers = await newRoom.getUsers(); | ||
| const roomInfo = { | ||
| room: { id: newRoom.id, name: newRoom.name }, | ||
| members: roomUsers, | ||
| }; | ||
|
|
||
| return res.status(201).json(roomInfo); | ||
| }; | ||
|
|
||
| const deleteRoom = async (req, res) => { | ||
| const { roomId } = req.params; | ||
| const room = await roomService.findRoomById(roomId); | ||
|
|
||
| if (!roomId || !room) { | ||
| return res.status(404).json({ message: 'Room not Found' }); | ||
| } | ||
|
Comment on lines
+57
to
+63
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In deleteRoom you call roomService.findRoomById(roomId) without normalizing the id. Also the existence check uses |
||
|
|
||
| await room.setUsers([]); | ||
| await Message.destroy({ where: { roomId: room.id } }); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Line performing Message.destroy will remove message rows directly from the model. Prefer using a messageService.deleteMessagesByRoom(room.id) if available so message-related logic stays in one place and any hooks/side-effects are handled consistently. If a service helper doesn't exist, add one or document why direct model access is used. |
||
| await room.destroy(); | ||
|
|
||
| res.status(200).json({ message: 'Room deleted successfully' }); | ||
| }; | ||
|
|
||
| const renameRoom = async (req, res) => { | ||
| const room = req.room; | ||
| const { newName } = req.body; | ||
|
|
||
| if (!room) { | ||
| return res.status(404).json({ message: 'Room not Found' }); | ||
| } | ||
|
|
||
| if (!newName || typeof newName !== 'string' || !newName.trim()) { | ||
| return res.status(400).json({ message: 'Invalid room name provided' }); | ||
| } | ||
|
|
||
| room.name = newName.trim(); | ||
| await room.save(); | ||
|
|
||
| return res.status(200).json(room); | ||
| }; | ||
|
|
||
| const mergeRooms = async (req, res) => { | ||
| const currentRoom = req.room; | ||
| const user = req.user; | ||
| const { targetRoomId } = req.body; | ||
|
|
||
| if (!user) { | ||
| return res.status(401).json({ error: 'User not authenticated' }); | ||
| } | ||
|
|
||
| if (!currentRoom) { | ||
| return res.status(404).json({ error: 'Current room not found' }); | ||
| } | ||
|
|
||
| if (!targetRoomId) { | ||
| return res.status(400).json({ error: 'No targetRoomId provided' }); | ||
| } | ||
|
|
||
| const targetRoomIdNum = parseInt(targetRoomId, 10); | ||
|
|
||
| if (Number.isNaN(targetRoomIdNum)) { | ||
| return res.status(400).json({ error: 'Invalid targetRoomId' }); | ||
| } | ||
|
|
||
| const usersRooms = await roomService.findRoomsByUserId(user.id); | ||
| const targetRoom = usersRooms.find((r) => r.id === targetRoomIdNum); | ||
|
|
||
| if (!targetRoom) { | ||
| return res | ||
| .status(403) | ||
| .json({ error: 'The user doesn’t have access to the target room' }); | ||
| } | ||
|
|
||
| await userService.mergeUsers(currentRoom.id, targetRoomIdNum); | ||
| await messageService.mergeMessages(currentRoom.id, targetRoomIdNum); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Potential blocking issue in mergeRooms: this handler calls |
||
|
|
||
| await currentRoom.setUsers([]); | ||
| await currentRoom.destroy(); | ||
|
Comment on lines
+121
to
+126
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. mergeRooms calls userService.mergeUsers and messageService.mergeMessages and then immediately clears users and destroys the current room. These multi-step changes should be executed atomically (DB transaction) so failures won't leave the system in an inconsistent state. Move merge logic into a transactional service method or start a transaction here and only destroy the source room after merge success. This addresses requirements 3.3 and 3.4 (preserve history and consistent identity updates). |
||
|
|
||
| return res.status(200).json(targetRoom); | ||
| }; | ||
|
|
||
| const joinRoom = async (req, res) => { | ||
| const { roomId } = req.params; | ||
| const userId = req.user.id; | ||
|
|
||
| const room = await roomService.findRoomById(roomId); | ||
|
Comment on lines
+134
to
+135
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You're passing req.params.roomId directly into findRoomById. Normalize and validate it first (parseInt + Number.isNaN check) to avoid edge cases when the service expects a number. Also validate the result before proceeding. Example: const id = parseInt(roomId, 10); if (Number.isNaN(id)) return 400... then use id in service calls. |
||
|
|
||
| if (!room) { | ||
| return res.status(404).json({ message: 'Room not found' }); | ||
| } | ||
|
|
||
| await roomService.addUserToRoom(userId, roomId); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Small positive note: the joinRoom call to roomService.addUserToRoom uses (userId, roomId) which resolves the previously reported argument-order bug. Good work fixing that earlier issue — keep up that attention to service signatures. |
||
|
|
||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The router currently mounts the join route without the |
||
| return res.status(200).json({ message: 'Joined room successfully' }); | ||
|
Comment on lines
+140
to
+143
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Requirement 1.6 (message history retrieval on join) is not fulfilled here. After adding the user to the room you must return the room's previous messages so the client can display history. Consider calling messageService.findRoomsMessages(...) and include the messages in the JSON response (and/or include members). Example flow:
|
||
| }; | ||
|
|
||
| export const roomController = { | ||
| getRoomInfoByRoomId, | ||
| createRoom, | ||
| deleteRoom, | ||
| renameRoom, | ||
| mergeRooms, | ||
| joinRoom, | ||
| }; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,76 @@ | ||
| import { roomService } from '../services/room.service.js'; | ||
| import { userService } from '../services/user.service.js'; | ||
|
|
||
| const createUser = async (req, res) => { | ||
| const { name } = req.body; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Input validation is too weak: you read |
||
|
|
||
| if (!name) { | ||
| return res.status(400).json({ message: 'Name is required' }); | ||
| } | ||
|
|
||
| const newUser = await userService.createUser(name); | ||
|
Comment on lines
+5
to
+11
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. createUser currently trusts There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. After creating a user you call |
||
|
|
||
| res.status(201).json(newUser); | ||
| }; | ||
|
|
||
| const getUserInfo = async (req, res) => { | ||
| const { userId } = req.params; | ||
|
Comment on lines
+16
to
+17
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. getUserInfo reads userId from req.params but does not validate or normalize it. Consider parsing to an integer and validating the input before passing it to the service (e.g. parseInt and check Number.isInteger). Also consider whether you need to enforce authorization here — the route is protected by isAuth (which sets req.user) but this function allows fetching any user by id without verifying the requester’s rights. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
|
|
||
| if (!userId) { | ||
| return res.status(400).json({ message: 'Id is required' }); | ||
| } | ||
|
|
||
| const user = await userService.getUserById(userId); | ||
|
|
||
| if (!user) { | ||
| return res.status(404).json({ message: 'User not found' }); | ||
| } | ||
|
|
||
| const rooms = await roomService.findRoomsByUserId(user.id); | ||
|
Comment on lines
+23
to
+29
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You call userService.getUserById(userId) and then fetch rooms via roomService.findRoomsByUserId(user.id). Two issues: 1) If you intend that only the authenticated user can fetch their info, you should compare req.user.id to req.params.userId or otherwise use req.user directly. 2) If you expect room lists or their ordering to be deterministic, ensure the roomService returns ordered results (currently it returns User.Rooms without an explicit order).
Comment on lines
+16
to
+29
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. getUserInfo reads
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You fetch rooms with |
||
| const userInfo = { | ||
| user: { id: user.id, name: user.name, createdAt: user.createdAt }, | ||
| rooms, | ||
| }; | ||
|
|
||
| res.status(200).json(userInfo); | ||
| }; | ||
|
|
||
| const changeUserName = async (req, res) => { | ||
| const user = req.user; | ||
| const { newName } = req.body; | ||
|
|
||
| if (!user) { | ||
| return res.status(404).json({ message: 'User not Found' }); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Minor: error message strings are inconsistent in capitalization, e.g. 'User not Found' vs 'User not found'. This is cosmetic but makes client-side handling/error matching harder. Consider standardizing messages/keys returned in error responses. |
||
| } | ||
|
Comment on lines
+38
to
+44
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. changeUserName uses req.user (from isAuth) but the route includes a :userId parameter. This mismatch can be confusing or unsafe: either remove the :userId param from the route if operations should always act on the authenticated user, or fetch the target user by req.params.userId and explicitly verify that the authenticated user is allowed to change that target. Make the expected authorization semantics explicit and return 403 when appropriate. |
||
|
|
||
| if (!newName) { | ||
| return res.status(400).json({ message: 'No new name provided' }); | ||
| } | ||
|
|
||
| user.name = newName; | ||
|
|
||
| await user.save(); | ||
|
|
||
| res.status(200).json({ id: user.id, userName: user.name }); | ||
|
Comment on lines
+46
to
+54
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The newName validation exists (good), but after deleting/renaming the controller updates the Sequelize model and calls user.save(). This is fine, but consider returning the canonical fields (id, name, createdAt) instead of a model if you want to keep API responses consistent and avoid exposing internal metadata.
Comment on lines
+38
to
+54
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. changeUserName uses
Also validate
Comment on lines
+39
to
+54
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| }; | ||
|
|
||
| const deleteUser = async (req, res) => { | ||
| const user = req.user; | ||
|
|
||
| if (!user) { | ||
| return res.status(404).json({ message: 'User not Found' }); | ||
| } | ||
|
|
||
| await user.destroy(); | ||
|
|
||
| req.user = null; | ||
|
Comment on lines
+57
to
+66
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. deleteUser similarly ignores req.params.userId and uses req.user. Also, setting req.user = null after await user.destroy() is unnecessary in a stateless HTTP handler — it only mutates the current request object and doesn’t affect client-side state. If you need to revoke authentication, do so via the proper session/logout mechanism.
Comment on lines
+64
to
+66
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. After deleting the user you call |
||
|
|
||
| res.status(200).json({ message: 'User deleted successfully' }); | ||
|
Comment on lines
+57
to
+68
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. deleteUser similarly relies on Also: after |
||
| }; | ||
|
|
||
| export const userController = { | ||
| createUser, | ||
| getUserInfo, | ||
| changeUserName, | ||
| deleteUser, | ||
| }; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,31 @@ | ||
| import express from 'express'; | ||
| import cors from 'cors'; | ||
|
|
||
| import { userRouter } from './routes/user.router.js'; | ||
| import { roomRouter } from './routes/room.router.js'; | ||
|
|
||
| export function createServer() { | ||
| const server = express(); | ||
|
|
||
| server.use(express.json()); | ||
|
|
||
| server.use( | ||
| cors({ | ||
| origin: process.env.CLIENT_ORIGIN || 'http://localhost:3000', | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You're setting CORS origin from |
||
| credentials: true, | ||
| }), | ||
|
Comment on lines
+12
to
+16
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. CORS alignment suggestion: this file sets CORS origin with |
||
| ); | ||
|
Comment on lines
+12
to
+17
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You're enabling credentials in CORS — when you set credentials: true the origin must be an explicit origin (not ''). The code already reads CLIENT_ORIGIN from env, which is good; just ensure that variable is set in production. Also note: src/index.js configures Socket.IO with origin: '' and credentials: true which is invalid for browser credentialed requests — align the Socket.IO CORS to use the same explicit origin. See index.js where Socket.IO is configured. |
||
|
|
||
| server.get('/', (req, res) => { | ||
| res.status(200).json({ message: 'Server is running' }); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good: the root health route now uses |
||
| }); | ||
|
|
||
| server.use('/user', userRouter); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Optional: it may be helpful to document (or add a comment) that authentication is header-based in this app — the isAuth middleware expects the username in |
||
| server.use('/room', roomRouter); | ||
|
Comment on lines
+23
to
+24
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You mount the user and room routers here which is correct. I verified that |
||
|
|
||
| server.use((req, res, next) => { | ||
| res.status(404).json({ message: 'Route not found' }); | ||
|
Comment on lines
+26
to
+27
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The app registers a 404 fallback but does not register a centralized error-handling middleware (i.e. server.use((err, req, res, next) => {
// eslint-disable-next-line no-console
console.error(err);
res.status(err.status || 500).json({ message: err.message || 'Internal Server Error' });
});This will ensure consistent JSON errors for clients and tests. |
||
| }); | ||
|
Comment on lines
+26
to
+28
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Improve error handling: there is a 404 handler, but no centralized error-handling middleware to catch thrown errors and return a proper 500 response. Add an Express error handler after the 404 handler, for example:
Placing this after the 404 ensures unexpected exceptions are returned consistently to clients.
Comment on lines
+26
to
+28
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There is no global error-handling middleware in this file. The project uses catchError to forward async errors with next(error) (see utils/catchError). Without an Express error handler the client won't get consistent JSON errors. Add an error handler after the routes/404, for example: server.use((err, req, res, next) => {
// log
console.error(err);
res.status(err.status || 500).json({ error: err.message || 'Internal Server Error' });
});Place it after the 404 handler so all forwarded errors are caught. |
||
|
|
||
| return server; | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,12 @@ | ||
| import { User } from './user.js'; | ||
| import { Message } from './message.js'; | ||
| import { Room } from './room.js'; | ||
|
|
||
| User.hasMany(Message, { foreignKey: 'userId' }); | ||
| Message.belongsTo(User, { foreignKey: 'userId' }); | ||
|
Comment on lines
+5
to
+6
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The User <-> Message association is correct. Consider adding referential action options like |
||
|
|
||
| Room.hasMany(Message, { foreignKey: 'roomId' }); | ||
| Message.belongsTo(Room, { foreignKey: 'roomId' }); | ||
|
Comment on lines
+8
to
+9
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The Room <-> Message association is correct. As with users, you may want to add
Comment on lines
+5
to
+9
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The associations here correctly express the relationships needed by the app (User ⇄ Message, Room ⇄ Message, User ⇄ Room). Good — these are necessary for features like per-room message history and membership checks.
Comment on lines
+8
to
+9
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider adding DB-level cascade for Room -> Message so messages are removed when a room is deleted. That makes deletion semantics more robust and directly supports the requirement that deleting a room removes its message history. Example suggestion: |
||
|
|
||
| User.belongsToMany(Room, { through: 'user_room', foreignKey: 'userId' }); | ||
| Room.belongsToMany(User, { through: 'user_room', foreignKey: 'roomId' }); | ||
|
Comment on lines
+11
to
+12
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The many-to-many User <-> Room mapping via
Comment on lines
+11
to
+12
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For clarity, you may want to make the join-table keys explicit by adding |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,29 @@ | ||
| import { DataTypes } from 'sequelize'; | ||
| import { client } from '../db/db.js'; | ||
|
|
||
| export const Message = client.define( | ||
| 'Message', | ||
| { | ||
| id: { | ||
| type: DataTypes.INTEGER, | ||
| primaryKey: true, | ||
| autoIncrement: true, | ||
| }, | ||
| roomId: { | ||
| type: DataTypes.INTEGER, | ||
| allowNull: false, | ||
|
Comment on lines
+12
to
+14
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The model correctly requires |
||
| }, | ||
|
Comment on lines
+12
to
+15
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The |
||
| userId: { | ||
| type: DataTypes.INTEGER, | ||
| allowNull: false, | ||
|
Comment on lines
+16
to
+18
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| }, | ||
|
Comment on lines
+16
to
+19
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| text: { | ||
| type: DataTypes.STRING, | ||
| allowNull: false, | ||
|
Comment on lines
+20
to
+22
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Comment on lines
+20
to
+22
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Message |
||
| }, | ||
|
Comment on lines
+20
to
+23
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| }, | ||
| { | ||
| timestamps: true, | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Timestamps are enabled (createdAt will be recorded). Ensure when you return message history to clients you order by There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You enabled |
||
| tableName: 'messages', | ||
|
Comment on lines
+25
to
+27
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Timestamps are enabled which provides |
||
| }, | ||
| ); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,22 @@ | ||
| import { DataTypes } from 'sequelize'; | ||
| import { client } from '../db/db.js'; | ||
|
|
||
| export const Room = client.define( | ||
| 'Room', | ||
| { | ||
| id: { | ||
| type: DataTypes.INTEGER, | ||
| primaryKey: true, | ||
| autoIncrement: true, | ||
| }, | ||
| name: { | ||
| type: DataTypes.STRING, | ||
| allowNull: false, | ||
| unique: true, | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You have |
||
| }, | ||
|
Comment on lines
+12
to
+16
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider enforcing non-empty room names at the model level to avoid whitespace-only or empty names making it into the DB. For example add |
||
| }, | ||
| { | ||
| timestamps: true, | ||
| tableName: 'rooms', | ||
| }, | ||
| ); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,22 @@ | ||
| import { DataTypes } from 'sequelize'; | ||
| import { client } from '../db/db.js'; | ||
|
|
||
| export const User = client.define( | ||
| 'User', | ||
| { | ||
| id: { | ||
| type: DataTypes.INTEGER, | ||
| primaryKey: true, | ||
| autoIncrement: true, | ||
| }, | ||
| name: { | ||
| type: DataTypes.STRING, | ||
| allowNull: false, | ||
| unique: true, | ||
|
Comment on lines
+12
to
+15
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Because
Comment on lines
+12
to
+15
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The |
||
| }, | ||
|
Comment on lines
+12
to
+16
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Model correctly defines
Comment on lines
+12
to
+16
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add a
Comment on lines
+12
to
+16
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You currently use |
||
| }, | ||
| { | ||
| timestamps: true, | ||
|
Comment on lines
+18
to
+19
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| tableName: 'users', | ||
|
Comment on lines
+18
to
+20
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| }, | ||
| ); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,17 @@ | ||
| import { client } from './db.js'; | ||
| import '../data/associations.js'; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Importing associations before syncing is correct — it ensures model relationships are registered before client.sync runs. Keep this import so associations are applied prior to schema sync (associations are defined in src/data/associations.js) . There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Importing associations here is correct because associations must be registered before calling There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Importing associations before calling sync is correct and necessary so Sequelize knows about relationships when syncing. Good placement. See associations file where relationships are defined. |
||
|
|
||
| export const dbInit = async () => { | ||
| await client.authenticate(); | ||
|
Comment on lines
+4
to
+5
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider adding a local try/catch inside dbInit to produce DB-specific diagnostics (for example to log more context about DB authentication failures) even though index.js wraps dbInit in a top-level try/catch. This can make startup errors easier to debug. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Calling client.authenticate() is appropriate. If you want more granular startup errors, you can add a try/catch here, but index.js already wraps dbInit() in try/catch so this is acceptable. |
||
| // eslint-disable-next-line no-console | ||
| console.log('✅ Database connected'); | ||
|
|
||
| await client.sync({ alter: true }); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. sync({ alter: true }) is convenient for development but can be unsafe in production because it can silently alter schema. Consider using Sequelize migrations for production environments or gate this behavior behind an environment flag (e.g., run alter only for NODE_ENV !== 'production'). |
||
| // eslint-disable-next-line no-console | ||
| console.log('✅ Tables synced'); | ||
|
|
||
| const tables = await client.getQueryInterface().showAllTables(); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You log There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. showAllTables and printing the result is useful for debugging but may be noisy or leak schema info in non-dev runs. Consider gating this log with a debug flag or removing it for production. |
||
|
|
||
| // eslint-disable-next-line no-console | ||
| console.log(tables); | ||
|
Comment on lines
+13
to
+16
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Logging |
||
| }; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,10 @@ | ||
| import { Sequelize } from 'sequelize'; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Importing Sequelize here is correct and aligns with the rest of the project where There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Import is correct and Sequelize is available. This client is used by db.init and models to connect and sync the DB. Good. |
||
|
|
||
| export const client = new Sequelize({ | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You create and export the Sequelize client here. For better portability and to avoid hard-coding credentials in source, consider using environment variables for database, username, password, host and port (e.g. process.env.DB_NAME, process.env.DB_USER, etc.). This will make local, CI and production setups easier to manage. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The Sequelize client is created correctly and exported as |
||
| database: 'postgres', | ||
| username: 'postgres', | ||
| host: 'localhost', | ||
| dialect: 'postgres', | ||
| port: 5432, | ||
| password: 'Rerlol100', | ||
|
Comment on lines
+4
to
+9
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Recommendation: avoid hard-coding DB credentials in source. Use environment variables (e.g., |
||
| }); | ||
|
Comment on lines
+3
to
+10
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The connection options are minimal and valid, but you may want to add optional configuration such as
Comment on lines
+3
to
+10
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Make sure the database server and credentials you provide here actually exist in the environment where the app runs. The DB initialization (
Comment on lines
+3
to
+10
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Optional improvement: consider providing
Comment on lines
+3
to
+10
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The Sequelize client is created with hardcoded connection parameters (database, username, host, port, password). It’s better to read these from environment variables (e.g.
Comment on lines
+3
to
+10
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Optional: consider adding |
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When you return messages from getRoomInfoByRoomId (and when you start returning messages in joinRoom), make sure each message object contains
author,time, andtextfields and thattimeuses a consistent, verifiable format (e.g. ISO string). The task requires that all messages have these fields and a consistent timestamp format. Confirm messageService.findRoomsMessages returns messages in that shape.