Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
71 changes: 71 additions & 0 deletions src/controllers/message.controller.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
import { EventEmitter } from 'events';
import { messageService } from '../services/message.service.js';
import { User } from '../models/user.model.js';

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Import path is incorrect; there is no ../models/user.model.js file. Import the actual User model (e.g. ../models/user.js) so User.findByPk/Sequelize won't fail at runtime.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Import path is incorrect — there is no ../models/user.model.js. The User model is defined at ../models/user.js; update this import to avoid a module-not-found error at runtime.


export const messageEmitter = new EventEmitter();

const create = async (req, res) => {
const { roomId } = req.params;
const { text, userId } = req.body;

if (!roomId || !userId || !text || typeof text !== 'string') {
return res.status(400).json({ message: 'Missing or invalid parameters' });
}

try {
const newMessage = await messageService.createMessageInRoom(
text,
userId,
roomId,
);
Comment on lines +16 to +20

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

You call the service to create a message — make sure you handle the exact shape the service returns. If the service returns a mapped object ({ author, time, text }), you must not treat newMessage as a Sequelize instance. Either have the service return the instance with included User, or consume the service's mapped shape here.


const user = await User.findByPk(userId);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This User.findByPk(userId) is redundant if createMessageInRoom already returns the author (name). Extra DB calls are unnecessary and can be removed — instead rely on the service to provide author/time/text, or make the service return the created instance with included User so you can map once.


if (!user) {
return res.status(404).json({ message: 'User not found' });
}
Comment on lines +15 to +26

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

You call messageService.createMessageInRoom(...) then immediately do a User.findByPk(userId) and later use newMessage.createdAt. The service already returns a normalized object (author, time, text, roomId). This mismatch will produce undefined fields and an unnecessary DB lookup. Either have the service return a Sequelize instance including User or update the controller to use newMessage.author / newMessage.time / newMessage.text and remove the extra User.findByPk call.


const formattedMessage = {
author: user.name,
time: newMessage.createdAt,
text: newMessage.text,
Comment on lines +28 to +31

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

You're building formattedMessage using newMessage.createdAt and newMessage.text, but the service returns time/text/author (not createdAt) — createdAt will be undefined. Use the service's returned time/author/text fields (or change the service to return a raw instance and include User).

};
Comment on lines +28 to +32

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

formattedMessage uses user.name and newMessage.createdAt/newMessage.text but the service returns author/time/text. Also the returned/emitted object is missing roomId — the WS broadcaster expects roomId to route messages to rooms. Include roomId and use the service's fields (or change the service to return matching names).


messageEmitter.emit('message', formattedMessage);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

When emitting the message you must include roomId in the payload (e.g. { author, time, text, roomId }). index.js expects message.roomId to route the event to the correct room; without it the WS broadcast will be ignored.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

You emit formattedMessage without roomId. The listener in src/index.js reads message.roomId to decide the target room; without it messages will not be delivered to clients in the correct room. Add roomId to the emitted payload.


res.status(201).json(formattedMessage);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The response res.status(201).json(formattedMessage) returns a payload without roomId. The client expects messages to include roomId for correct handling; include roomId in the response as well.

} catch (err) {
// eslint-disable-next-line no-console
console.error('Error creating message:', err);
res.status(500).json({ message: 'Internal server error' });
}
};

const getMessages = async (req, res) => {
const { roomId } = req.params;

if (!roomId) {
return res.status(400).json({ message: 'Missing roomId' });
}

try {
const messages = await messageService.getAllMessagesInRoom(roomId);

const formattedMessages = messages.map((m) => ({
author: m.User?.name || 'Unknown',
time: m.createdAt,
text: m.text,
Comment on lines +54 to +57

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

In getMessages you map messages expecting Sequelize fields (m.User?.name, m.createdAt, m.text), but messageService.getAllMessagesInRoom returns normalized objects (author, time, text, roomId). Use m.author and m.time (and include roomId if needed), or change the service to return Sequelize instances with User included so the shapes match.

}));
Comment on lines +52 to +58

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

messageService.getAllMessagesInRoom already maps messages to { author, time, text }. Here you re-map expecting m.User and m.createdAt which will produce incorrect/empty values. Return/send the messages array directly (or align service/controller to a single consistent shape).


res.status(200).json(formattedMessages);
} catch (err) {
// eslint-disable-next-line no-console
console.error('Error getting messages:', err);
res.status(500).json({ message: 'Internal server error' });
}
};

export const messageController = {
create,
getMessages,
};
51 changes: 51 additions & 0 deletions src/controllers/rooms.controller.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
import { roomsService } from '../services/rooms.service.js';

const getAllRooms = async (req, res) => {
const rooms = await roomsService.getAllRooms();

res.status(200).send(rooms);
Comment on lines +3 to +6

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Wrap service calls in try/catch to handle unexpected errors (DB failures, etc.) and return 500 on server errors. Right now getAllRooms calls the service directly without error handling which may cause unhandled promise rejections.

Comment on lines +4 to +6

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

getAllRooms has no error handling. Consider surrounding the service call with try/catch and return 500 on unexpected errors to avoid unhandled rejections impacting the server process.

};

const createRoom = async (req, res) => {
const { title, userId, description } = req.body;

if (!title || !userId) {
return res.sendStatus(404);
Comment on lines +12 to +13

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

For missing required parameters you should return 400 (Bad Request), not 404. Change this to res.status(400).json(...) or res.sendStatus(400) so clients get the correct semantics when title or userId are missing.

Comment on lines +12 to +13

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Returning 404 for missing title/userId is incorrect — this is a client validation failure. Use 400 (Bad Request), return a helpful JSON message, and consider trimming title before validating.

}

await roomsService.createRoom(title, userId, description);

res.sendStatus(201);
Comment on lines +16 to +18

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Validate the title (non-empty string) and userId (expected type/format) before calling roomsService.createRoom. Also consider returning the created room (or Location header) instead of just 201 with no body so the client can get the new room's id.

Comment on lines +16 to +18

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Call to roomsService.createRoom is not guarded. Wrap in try/catch to handle service errors (e.g. unique constraint or DB errors). Also consider returning the created room object (201 with body) rather than an empty 201 to help clients.

};

const updateRoom = async (req, res) => {
const { roomId } = req.params;
const { title, description } = req.body;

if ((!title && !description) || !roomId) {
return res.sendStatus(404);
Comment on lines +25 to +26

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Same issue: updateRoom returns 404 for missing roomId or when neither title nor description is provided. Return 400 for invalid/missing input instead of 404 (404 means "not found").

Comment on lines +25 to +26

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Validation here uses 404 for missing title/description or roomId. Missing request fields should be 400. Also validate/trim title/description and ensure roomId is a non-empty UUID string (or return 400).

}

await roomsService.updateRoom(roomId, title, description);

res.sendStatus(204);
Comment on lines +29 to +31

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

roomsService.updateRoom may throw 'Room not found' — currently the controller always responds 204. Wrap the service call in try/catch and return 404 when the service indicates the room doesn't exist, otherwise 500 for unexpected errors.

};

const deleteRoom = async (req, res) => {
const { roomId } = req.params;

if (!roomId) {
return res.sendStatus(400);
}

await roomsService.deleteRoom(roomId);

res.sendStatus(204);
Comment on lines +41 to +43

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

In deleteRoom, you validate roomId (good) but don't handle service errors. roomsService.deleteRoom throws when the room doesn't exist — wrap the call in try/catch and return 404 for not-found, 500 for other errors. Keeping the 400 for missing roomId is appropriate.

};

export const roomController = {
getAllRooms,
createRoom,
updateRoom,
deleteRoom,
};
28 changes: 28 additions & 0 deletions src/controllers/user.controller.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
import { User } from '../models/user.js';
import { userService } from '../services/user.service.js';

const create = async (req, res) => {
try {
const { name } = req.body;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

You destructure name from the body but don't validate its type or trim whitespace. A value like ' ' will pass the current if (!name) check. Consider ensuring name is a string and doing const cleanName = typeof name === 'string' ? name.trim() : '' and validate cleanName instead.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Trim incoming name and validate the trimmed value. Currently if (!name) will allow strings containing only whitespace (e.g. " ") — trim and check if (!name || !name.trim()) so empty/whitespace-only names are rejected.


if (!name) {
return res.status(400).json({ message: 'Name is required' });
Comment on lines +8 to +9

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The presence check if (!name) is too weak for user input because whitespace-only strings are truthy. Validate after trimming (e.g., if (!cleanName) return 400). This ensures you don't create users with empty names and satisfies the requirement to validate input before DB calls.

}

const isNameExist = await User.findOne({ where: { name } });

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

You check User.findOne to return 409 when a name exists — good. However this doesn't prevent a race condition where User.create could still fail with a unique constraint error if two requests create the same name concurrently. Handle SequelizeUniqueConstraintError in the catch and return 409 to cover that case.


if (isNameExist) {
return res.status(409).json({ message: 'User already exists' });
}

await userService.createUser(name);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

You're proactively checking for an existing user which is good, but there's still a race condition between this check and createUser. Consider handling the unique constraint error from the DB during creation instead of relying only on the pre-check; that makes the endpoint safe for concurrent requests.


return res.status(201).json({ message: 'User was created!' });
} catch {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The catch block has no error parameter and returns 500 for all failures. Change to catch (err) and log the error (e.g., console.error(err)). Also detect unique constraint errors (by err.name === 'SequelizeUniqueConstraintError' or using UniqueConstraintError from Sequelize) and return 409 in that case. This maps DB errors to appropriate HTTP statuses as required.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The catch block doesn't capture the thrown error object. Use catch (err) and handle SequelizeUniqueConstraintError (return 409) or at least log err before returning 500 so one can diagnose failures. This also lets you return a meaningful status when DB uniqueness is violated.

return res.status(500).json({ message: 'Internal server error' });
}
};

export const userController = {
create,
};
10 changes: 10 additions & 0 deletions src/db.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
import 'dotenv/config';

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Importing dotenv here is fine to load environment variables. Ensure you have a .env in development and that CI/production provide these env vars; otherwise Sequelize will be constructed with undefined values.

import { Sequelize } from 'sequelize';

export const client = new Sequelize({

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Sequelize constructor uses several environment variables. Consider validating these values (or providing sensible defaults) before constructing Sequelize so the app fails fast with a clear error when DB config is missing.

host: process.env.DB_HOST,

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

If your database uses a non-default port, include port: process.env.DB_PORT (or Number(process.env.DB_PORT) || 5432) in this config. Without it, connections to nonstandard ports will fail.

username: process.env.DB_USERNAME,
password: process.env.DB_PASS,
database: process.env.DB_DATABASE,
dialect: 'postgres',

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

For production (e.g. Heroku, managed Postgres) you may need to pass dialectOptions (SSL) or disable verbose logging: e.g. { dialect: 'postgres', logging: false, dialectOptions: { ssl: { rejectUnauthorized: false } } }. Add those only if required by your deployment.

});
82 changes: 82 additions & 0 deletions src/index.js
Original file line number Diff line number Diff line change
@@ -1 +1,83 @@
'use strict';
import 'dotenv/config';
import express from 'express';
import { WebSocketServer } from 'ws';
import cors from 'cors';
import { userRoute } from './routes/user.route.js';
import { roomsRoute } from './routes/rooms.route.js';
import { messageRoute } from './routes/message.route.js';
import { messageEmitter } from './controllers/message.controller.js';
import { messageService } from './services/message.service.js';

const PORT = process.env.PORT || 5000;
const app = express();

app.use(cors());
app.use(express.json());

app.use('/user', userRoute);
app.use('/rooms', roomsRoute);
app.use(messageRoute);

const server = app.listen(PORT);

const wss = new WebSocketServer({ server });
const rooms = {};

wss.on('connection', (ws) => {
ws.on('message', async (rawMessage) => {
let parsed;

try {
parsed = JSON.parse(rawMessage);
} catch {
return;
}

const { roomId } = parsed;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

You only parse JSON and check roomId, but you should validate the parsed payload structure more robustly (e.g. confirm it's an object and roomId is a non-empty string/UUID). This prevents unexpected payloads from being treated as joins.


if (!roomId || typeof roomId !== 'number') {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The code rejects roomId values that are not numbers. Room IDs are UUID strings in the models, so this validation will block valid join requests. Accept non-empty strings (or validate UUID format) instead of requiring typeof roomId === 'number'.

return;
}
Comment on lines +39 to +41

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The WS join validation requires roomId to be a number. Room IDs are UUID strings in this project — this check will prevent clients from joining. Accept non-empty strings (or validate UUID format) instead of typeof roomId !== 'number' so join requests succeed.


if (!rooms[roomId]) {
rooms[roomId] = [];
Comment on lines +43 to +44

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

You create the room array if missing but never validate roomId before using it; this can create rooms[undefined]. Ensure roomId exists and is a valid identifier before mutating rooms.

}

if (!rooms[roomId].includes(ws)) {
rooms[roomId].push(ws);
}

const messages = await messageService.getAllMessagesInRoom(roomId);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

When a client joins you push the socket then fetch and send previous messages — good. Ensure messageService.getAllMessagesInRoom returns messages already mapped to { author, time, text }; otherwise you must map them before sending. Confirm the service/controller agree on a single message shape to avoid sending incomplete data.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

messageService.getAllMessagesInRoom(roomId) is called without error handling. If the service throws (invalid id, DB error) this will produce an unhandled rejection. Wrap the call in try/catch and return/log an appropriate error instead of letting the handler fail silently.


messages.forEach((msg) => {
ws.send(JSON.stringify(msg));

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Sending Sequelize model instances with JSON.stringify(msg) may not produce the intended payload. Convert to a plain object (msg.toJSON() or msg.get({ plain: true })) and map to { author, time: createdAt, text } before sending so clients always receive the required fields.

});
});

ws.on('close', () => {
for (const roomId in rooms) {
rooms[roomId] = rooms[roomId].filter((client) => client !== ws);
}
});
});

messageEmitter.on('message', async (message) => {
const { roomId } = message;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The emitter handler expects message.roomId to exist; ensure all places that emit messages (controllers/services) include roomId in the emitted payload. Without roomId the broadcast will not be routed to the appropriate room and clients won't see new messages (requirement: joined users must receive prior and live messages for the room).


if (!roomId || !rooms[roomId]) {
return;
Comment on lines +66 to +69

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This handler expects incoming emitted message objects to include roomId. If upstream emitters/controllers don't include roomId, this check (if (!roomId || !rooms[roomId])) will silently skip broadcasting. Ensure all emitters include roomId when emitting events.

}

const formatted = {
author: message.author || message.User?.name || 'Unknown',
time: message.time || message.createdAt,
Comment on lines +72 to +74

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The code mixes shapes when formatting outgoing WS messages: it uses message.author || message.User?.name and message.time || message.createdAt. This is fragile — choose one canonical shape (prefer { author, time, text, roomId }) and broadcast that directly. That ensures every message sent to WS clients contains the required author, time, and text fields.

Comment on lines +72 to +74

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Accessing message.User?.name and message.createdAt assumes a Sequelize instance; the service currently returns normalized fields (author, time). Update these lines to use message.author and message.time (or change the service to return instances) to avoid undefined values at runtime.

text: message.text,
};
Comment on lines +72 to +76

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The formatted object sent to clients omits roomId and mixes field access patterns (message.author vs message.User?.name, message.time vs message.createdAt). Unify the message shape across the app (prefer a normalized object: author, time, text, roomId) and include roomId when sending to clients so payloads are consistent and the client can rely on one schema.


rooms[roomId].forEach((client) => {
if (client.readyState === 1) {
client.send(JSON.stringify(formatted));
}
});
});
35 changes: 35 additions & 0 deletions src/models/message.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
import { DataTypes } from 'sequelize';
import { client } from '../db.js';
import { User } from './user.js';
import { Room } from './room.js';

export const Message = client.define(
'message',
{
id: {
type: DataTypes.UUID,
defaultValue: DataTypes.UUIDV4,
allowNull: false,
primaryKey: true,
},
text: {
type: DataTypes.STRING,
Comment on lines +15 to +16

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Use DataTypes.TEXT for text rather than DataTypes.STRING. Message bodies can be longer than default STRING limits and the task expects robust text storage.

Comment on lines +15 to +16

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Use a longer text type for message bodies. DataTypes.STRING imposes length limits; consider DataTypes.TEXT so long messages won't be truncated or rejected.

allowNull: false,
Comment on lines +15 to +17

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The text attribute uses DataTypes.STRING, which may be too short for chat messages and could lead to truncation. Consider using DataTypes.TEXT to avoid length limits so message text is not lost.

},
createdAt: {
type: DataTypes.DATE,
defaultValue: DataTypes.NOW,
allowNull: false,
},
Comment on lines +19 to +23

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

You define createdAt manually which provides the required timestamp. Alternatively you could enable Sequelize timestamps and keep updatedAt: false to let Sequelize manage createdAt automatically — either approach is fine as long as controllers return this value as the message time.

Comment on lines +19 to +23

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

createdAt is present and non-null with default NOW, which satisfies the requirement to expose message timestamps. Keeping updatedAt: false is fine if edits are not supported, but be explicit in commit message/docs why updates are disabled.

},
Comment on lines +19 to +24

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Define userId and roomId explicitly in the model attributes (e.g. type: DataTypes.UUID, allowNull: false, references: { model: 'users', key: 'id' }). Right now the associations (below) will create these columns implicitly but without explicit NOT NULL constraints — the task recommends non-null FKs so messages always belong to a user and a room.

{
updatedAt: false,
tableName: 'messages',
},
);

Message.belongsTo(User, { foreignKey: 'userId' });
User.hasMany(Message, { foreignKey: 'userId' });

Message.belongsTo(Room, { foreignKey: 'roomId' });
Room.hasMany(Message, { foreignKey: 'roomId' });
Comment on lines +31 to +35

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

userId and roomId columns are not explicitly defined—rely on associations alone means you can't enforce allowNull: false or add DB references/onDelete behavior. Define userId and roomId in the model (e.g. UUID type, allowNull: false, references: { model: 'users', key: 'id' }, and onDelete: 'CASCADE') so every message is guaranteed to belong to a user and a room.

Comment on lines +31 to +35

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Associations are correct, but they rely on implicitly-created FK columns. After adding explicit userId/roomId attributes, keep these belongsTo/hasMany lines so Sequelize sets up relations and foreign keys consistently.

23 changes: 23 additions & 0 deletions src/models/room.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
import { DataTypes } from 'sequelize';
import { client } from '../db.js';
import { User } from './user.js';

export const Room = client.define('room', {
id: {
type: DataTypes.UUID,
defaultValue: DataTypes.UUIDV4,
allowNull: false,
primaryKey: true,
},
title: {
type: DataTypes.STRING,
allowNull: false,
},
description: {
type: DataTypes.STRING,
allowNull: false,
Comment on lines +16 to +18

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

description is allowNull: false but has no defaultValue. The rooms service provides a default empty string when creating rooms, so creation works, but consider adding defaultValue: '' here (or allowNull: true) to keep the model and service aligned and avoid potential errors elsewhere.

Comment on lines +16 to +18

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The model sets description as allowNull: false, but the model does not provide a defaultValue. The service currently uses an empty string default, but make the default explicit in the model (e.g. defaultValue: '') so direct model creations won't fail.

Comment on lines +16 to +18

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

description is declared allowNull: false but has no defaultValue. If any code path creates a Room without providing description the DB will error. Either set defaultValue: '' here or ensure every create call always supplies a non-null description (service currently defaults, but making the model resilient is safer).

},
});

Room.belongsTo(User);
User.hasMany(Room);
Comment on lines +22 to +23

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This model and its belongsTo/hasMany associations satisfy the requirement to associate rooms with users and therefore support room ownership and room-related operations required by the task (create/rename/delete/join) — ensure other modules use these associations consistently when fetching or serializing rooms.

Comment on lines +22 to +23

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Association uses default FK options. To guarantee every room is associated with a user (matching the controller/service which requires userId), set the foreign key to non-null and consider onDelete: 'CASCADE', e.g. Room.belongsTo(User, { foreignKey: { name: 'userId', allowNull: false }, onDelete: 'CASCADE' }) and mirror the foreignKey in User.hasMany.

Comment on lines +22 to +23

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Associations are declared but there is no explicit userId attribute on the Room model. The service createRoom(title, userId, ...) expects a userId — add a userId field (UUID, allowNull: false) to the model and declare the association with foreignKey: 'userId' so the DB enforces the relationship.

Comment on lines +22 to +23

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

When adding the explicit userId field, ensure the association uses the same foreign key, e.g. Room.belongsTo(User, { foreignKey: 'userId' }) and User.hasMany(Room, { foreignKey: 'userId' }), to keep associations and constraints consistent across models and services.

16 changes: 16 additions & 0 deletions src/models/user.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
import { DataTypes } from 'sequelize';
import { client } from '../db.js';

export const User = client.define('user', {
id: {
type: DataTypes.UUID,
defaultValue: DataTypes.UUIDV4,
allowNull: false,
primaryKey: true,
},
name: {
type: DataTypes.STRING,
unique: true,
allowNull: false,
},
});
7 changes: 7 additions & 0 deletions src/routes/message.route.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
import express from 'express';
import { messageController } from '../controllers/message.controller.js';

export const messageRoute = new express.Router();

messageRoute.get('/rooms/:roomId/messages', messageController.getMessages);
messageRoute.post('/rooms/:roomId/messages', messageController.create);
9 changes: 9 additions & 0 deletions src/routes/rooms.route.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
import express from 'express';
import { roomController } from '../controllers/rooms.controller.js';

export const roomsRoute = new express.Router();

roomsRoute.get('/', roomController.getAllRooms);
roomsRoute.post('/', roomController.createRoom);
Comment on lines +6 to +7

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The file registers GET, POST, PATCH and DELETE routes which cover list, create, rename/update and delete room operations. However there is no explicit HTTP "join room" endpoint here. If joining is intentionally handled via WebSocket (src/index.js), please document that for clients or add an HTTP join handler for clients that expect to join via REST.

roomsRoute.patch('/:roomId', roomController.updateRoom);
roomsRoute.delete('/:roomId', roomController.deleteRoom);
Comment on lines +8 to +9

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Consider adding validation for :roomId (e.g. check non-empty / UUID format) as middleware on the PATCH and DELETE routes so malformed IDs are rejected early with a 400. You can use router.param or a small middleware before the controller call.

6 changes: 6 additions & 0 deletions src/routes/user.route.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
import express from 'express';
import { userController } from '../controllers/user.controller.js';

export const userRoute = new express.Router();

userRoute.post('/', userController.create);
45 changes: 45 additions & 0 deletions src/services/message.service.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
import { Message } from '../models/message.js';

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The service imports only Message. To return messages with the author's username you should also import the User model (Message belongsTo User) and include it in queries so you can access the user's name to use as author.

import { User } from '../models/user.js';

const getAllMessagesInRoom = async (roomId) => {
if (!roomId) {
throw new Error('roomId is required');
}

const messages = await Message.findAll({
where: { roomId },
include: [{ model: User, attributes: ['name'] }],
order: [['createdAt', 'ASC']],
});

return messages.map((msg) => ({
author: msg.user?.name || 'Unknown',
Comment on lines +15 to +16

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

When including User via Sequelize, the included model is typically available on the User property (capitalized), not user. msg.user?.name will be undefined; use msg.User?.name (or explicitly alias the include) so author is populated correctly.

time: msg.createdAt,
text: msg.text,
roomId: msg.roomId,
}));
};

const createMessageInRoom = async (text, userId, roomId) => {
if (!text || !userId || !roomId) {
throw new Error('Missing required fields');
}

const newMessage = await Message.create({ text, userId, roomId });

const messageWithUser = await Message.findByPk(newMessage.id, {
include: [{ model: User, attributes: ['name'] }],
});
Comment on lines +30 to +32

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

After Message.findByPk(...) you assume messageWithUser exists. Add a null check and throw or handle the error if it's not found to avoid runtime exceptions when accessing properties on null.


return {
author: messageWithUser.user?.name || 'Unknown',
Comment on lines +34 to +35

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Same issue as above: messageWithUser.user?.name should reference the included model property messageWithUser.User?.name (capital U) or use an explicit alias in the include. Otherwise author will be 'Unknown' even when the user exists.

time: messageWithUser.createdAt,
text: messageWithUser.text,
roomId: messageWithUser.roomId,
};
Comment on lines +34 to +39

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This service returns a normalized plain object ({ author, time, text, roomId }). The controller currently expects Sequelize instances (accessing .createdAt and .User). Align the contract: either return the full instance including User from the service, or update the controller to use the normalized fields (time, author, etc.). Without alignment, message fields will be undefined when controllers/emitters try to access them.

};

export const messageService = {
getAllMessagesInRoom,
createMessageInRoom,
Comment on lines +42 to +44

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Consider normalizing the returned value from both functions to a consistent plain payload (not Sequelize instances). Controllers and WebSocket code assume they can serialize message; returning a plain object avoids surprises and makes broadcasting predictable.

};
Loading