Skip to content
Closed
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
35 changes: 35 additions & 0 deletions src/controllers/message.controller.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
import { EventEmitter } from 'events';
import { messageService } from '../services/message.service.js';

export const messageEmitter = new EventEmitter();

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

if (!roomId || !userId || !text) {
return res.sendStatus(404);
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 fields in the request body or parameters, it's more appropriate to return a 400 Bad Request status code. The 404 Not Found status should be reserved for when the requested resource (i.e., the URL endpoint itself) cannot be found.

}

const newMessage = await messageService.createMessageInRoom(
text,
userId,
roomId,
);

messageEmitter.emit('message', newMessage);

res.status(201).json(newMessage);
};

export const getMessages = async (req, res) => {
const { roomId } = req.params;
const messages = await messageService.getAllMessagesInRoom(roomId);

res.status(200).json(messages);
};

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);
};

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

if (!title || !userId) {
return res.sendStatus(404);
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 required fields like title or userId are missing from the request body, it's more appropriate to return a 400 Bad Request status code. The 404 Not Found status should be reserved for when the requested resource (i.e., the URL endpoint itself) cannot be found.

}

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

res.sendStatus(201);
};

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

if ((!title && !description) || !roomId) {
return res.sendStatus(404);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Similar to the createRoom function, if the request is malformed (e.g., no updateable fields provided or roomId is missing), a 400 Bad Request status is more appropriate than 404 Not Found.

}

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

res.sendStatus(204);
};

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

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

await roomsService.deleteRoom(roomId);

res.sendStatus(204);
};

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

const create = async (req, res) => {
const { name } = req.body;
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.

The logic to check for an existing user is a piece of business logic that should be encapsulated within the userService. Moving this User.findOne call into the service layer will improve the separation of concerns, making the controller leaner and the service layer more comprehensive.


if (!name) {
return res.sendStatus(404);
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 required field like name is missing from the request body, the appropriate HTTP status code is 400 Bad Request. The 404 Not Found status should be reserved for cases where the requested resource (e.g., the URL endpoint) does not exist.

}

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

await userService.createUser(name);

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

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';
import { Sequelize } from 'sequelize';

export const client = new Sequelize({
host: process.env.DB_HOST,
username: process.env.DB_USERNAME,
password: process.env.DB_PASS,
database: process.env.DB_DATABASE,
dialect: 'postgres',
});
Comment on lines +4 to +10
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 a logging option to the Sequelize configuration to control SQL query logging. For instance, you could set logging: false to prevent Sequelize from outputting every executed SQL query to the console, which is a common practice in production environments to reduce noise and overhead.

63 changes: 63 additions & 0 deletions src/index.js
Original file line number Diff line number Diff line change
@@ -1 +1,64 @@
'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 { Message } from './models/message.js';
import { roomsRoute } from './routes/rooms.route.js';
import { messageRoute } from './routes/message.route.js';
import { messageEmitter } from './controllers/message.controller.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, () => {
// eslint-disable-next-line no-console
console.log(`Server was started on ${PORT}`);
});

const wss = new WebSocketServer({ server });

const rooms = {};

wss.on('connection', (ws) => {
ws.on('message', async (message) => {
const { roomId } = JSON.parse(message);

if (!rooms[roomId]) {
rooms[roomId] = [];
}

rooms[roomId].push(ws);

const messages = await Message.findAll({ where: { 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.

This direct database query using the Message model bypasses the service layer. To adhere to the application's architecture and maintain separation of concerns, you should use the existing messageService.getAllMessagesInRoom(roomId) function instead.


messages.forEach((msg) => {
ws.send(JSON.stringify(msg));
});
});
Comment on lines +31 to +45
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 logic in the 'message' handler is flawed. The line rooms[roomId].push(ws) is executed on every message, which adds the same client to the room's array repeatedly. This will cause the client to receive an increasing number of duplicate messages. A client should only be added to a room once, ideally upon a distinct "join" event. Additionally, the JSON.parse(message) call should be wrapped in a try...catch block to prevent malformed messages from crashing the server.


ws.on('close', () => {
for (const roomId in rooms) {
rooms[roomId] = rooms[roomId].filter((client) => client !== ws);
}
});
Comment on lines +47 to +51
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 close event handler iterates through every room to find and remove a disconnected client. This is inefficient and will perform poorly as the number of rooms increases. A more efficient approach would be to store the roomId on the WebSocket connection object (ws) when the client joins a room. This allows for direct and fast removal from the specific room's array on disconnection.

});

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

if (rooms[roomId]) {
rooms[roomId].forEach((client) => {
if (client.readyState === 1) {
client.send(JSON.stringify(message));
}
});
}
});
31 changes: 31 additions & 0 deletions src/models/message.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
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,
allowNull: false,
},
createdAt: {
type: DataTypes.DATE,
defaultValue: DataTypes.NOW,
allowNull: false,
},
},
{ updatedAt: false },
);

Message.belongsTo(User);
User.hasMany(Message);
Message.belongsTo(Room);
Room.hasMany(Message);
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.

The current associations do not specify what should happen to a message if its parent User or Room is deleted. This can lead to orphaned records or failed deletion attempts due to foreign key constraints. Consider adding onDelete: 'CASCADE' to the hasMany associations in the Room and User models to automatically delete all of a room's or user's messages when they are deleted. For example: Room.hasMany(Message, { onDelete: 'CASCADE' });.

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.

The description field is defined as allowNull: false, but the corresponding createRoom service function treats it as optional by providing a default empty string. This creates a disconnect between the data model and the business logic. To align them, the model should be updated to reflect that the description is optional, for instance by setting allowNull: true or defaultValue: ''.

},
});

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.

The association between User and Room does not specify what should happen to a room if its owning user is deleted. This can lead to orphaned room records in the database. It is good practice to define this behavior explicitly. Consider adding an onDelete: 'CASCADE' option to the hasMany association on the User model, which will ensure that all rooms created by a user are automatically deleted when that user is removed.

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,
Comment on lines +11 to +14
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 name field correctly enforces uniqueness and that a value is present. To further improve data integrity and robustness, consider adding a validate block. This would allow you to specify additional constraints, such as a minimum or maximum length for usernames (e.g., validate: { len: [3, 25] }), preventing overly short or long names.

},
});
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);
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 routes are clear and functional. For improved architectural consistency and to better model the resource hierarchy (messages as a sub-resource of rooms), consider nesting this router within rooms.route.js. This would involve changing the paths here to be relative (e.g., /) and then mounting messageRoute on the /:roomId/messages path within the roomsRoute. This change would create a more modular and hierarchical routing structure.

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);
roomsRoute.patch('/:roomId', roomController.updateRoom);
roomsRoute.delete('/:roomId', roomController.deleteRoom);
Comment on lines +6 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.

This router is well-defined. To further improve the application's structure, you could centralize all room-related endpoints here. Since messages belong to a room, you could import messageRoute and mount it on a parameterized path, like roomsRoute.use('/:roomId/messages', messageRoute). This would make the relationship between rooms and messages explicit in the routing layer and improve the overall organization of your routes.

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);
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 route is correctly defined for creating a new user. As the application evolves, you could expand this router to include other user management endpoints, such as GET /:userId to retrieve a specific user or GET / to list all users, each pointing to a corresponding controller function.

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

const getAllMessagesInRoom = (roomId) => {
return Message.findAll({ where: { 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.

To improve efficiency and provide more useful data to the client, consider including the author's information with each message. You can achieve this by using the include option in your Sequelize query, like so: Message.findAll({ where: { roomId }, include: User }). This will eager-load the associated user for each message.

};

const createMessageInRoom = (text, userId, roomId) => {
return Message.create({ text, userId, roomId });
Comment on lines +7 to +8
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Before creating a message, it's crucial to validate that the provided userId and roomId correspond to existing records in the database. Adding checks to ensure the user and room exist will prevent the creation of orphaned messages and improve the overall data integrity of the application. This logic belongs in the service layer.

};

export const messageService = {
getAllMessagesInRoom,
createMessageInRoom,
};
42 changes: 42 additions & 0 deletions src/services/rooms.service.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
import { Room } from '../models/room.js';

const getAllRooms = () => {
return Room.findAll();
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

To provide richer data to the client, consider eager-loading the associated user data. By changing the query to Room.findAll({ include: User }), you can include the details of the user who created each room, reducing the need for follow-up API calls from the client.

};

const createRoom = (title, userId, description = '') => {
return Room.create({ title, userId, description });
Comment on lines +7 to +8
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 currently trusts that the provided userId is valid. To ensure data integrity, you should add a check to verify that a user with the given userId exists before attempting to create the room. This prevents potential foreign key constraint violations and ensures every room has a valid creator.

};

const deleteRoom = (id) => {
return Room.destroy({ where: { id } });
};

const updateRoom = (id, title, description) => {
if (!id) {
throw new Error('Room id is required');
}

const updatedData = {};

if (title) {
updatedData.title = title;
}

if (description) {
updatedData.description = description;
}

if (Object.keys(updatedData).length === 0) {
throw new Error('At least one field must be provided');
}

return Room.update(updatedData, { where: { id } });
Comment on lines +15 to +34
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Before attempting to update a room, it is good practice to first verify that a room with the given id exists. This allows you to provide a more specific error message, such as 'Room not found', which can then be handled appropriately in the controller (e.g., with a 404 status code). The current implementation doesn't distinguish between an update on a non-existent room and a successful update that changes no data.

};

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

const createUser = async (name) => {
return User.create({ name });
};
Comment on lines +3 to +5
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

To improve separation of concerns, the logic for checking if a user already exists should be moved from the controller into this function. The createUser function should first query the database to see if a user with the given name already exists. If so, it should throw an error, which the controller can then catch and translate into a 409 Conflict response. This makes the service layer responsible for all business logic related to user creation.


export const userService = {
createUser,
};