From 1acd47a0f628b6a956bb0fa9acde9d71a1d3c981 Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Thu, 18 Jul 2024 10:07:10 -0400 Subject: [PATCH] =?UTF-8?q?=F0=9F=8C=BF=20fix:=20Message=20Route=20Improve?= =?UTF-8?q?ments=20(#3378)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix: do not throw errors for invalid convo id, simply return undefined * feat: Add error handling and logging to message route definitions * feat: Refactor Message.js to improve code organization and readability * fix: Return undefined instead of throwing error for invalid conversation ID in Message.spec.js --- api/models/Message.js | 53 +++++++++++++------------- api/models/Message.spec.js | 2 +- api/server/routes/messages.js | 72 +++++++++++++++++++++++++---------- 3 files changed, 79 insertions(+), 48 deletions(-) diff --git a/api/models/Message.js b/api/models/Message.js index 460c693439..76a566d9f9 100644 --- a/api/models/Message.js +++ b/api/models/Message.js @@ -1,6 +1,6 @@ const { z } = require('zod'); const Message = require('./schema/messageSchema'); -const logger = require('~/config/winston'); +const { logger } = require('~/config'); const idSchema = z.string().uuid(); @@ -27,42 +27,43 @@ const idSchema = z.string().uuid(); * @param {string} [params.finish_reason] - Reason for finishing the message. * @param {number} [params.tokenCount] - The number of tokens in the message. * @param {string} [params.plugin] - Plugin associated with the message. - * @param {Object[]} [params.plugins] - An array of plugins associated with the message. + * @param {string[]} [params.plugins] - An array of plugins associated with the message. * @param {string} [params.model] - The model used to generate the message. * @returns {Promise} The updated or newly inserted message document. * @throws {Error} If there is an error in saving the message. */ -async function saveMessage( - req, - { - endpoint, - iconURL, - messageId, - newMessageId, - conversationId, - parentMessageId, - sender, - text, - isCreatedByUser, - error, - unfinished, - files, - isEdited, - finish_reason, - tokenCount, - plugin, - plugins, - model, - }, -) { +async function saveMessage(req, params) { try { if (!req || !req.user || !req.user.id) { throw new Error('User not authenticated'); } + const { + text, + error, + model, + files, + plugin, + sender, + plugins, + iconURL, + endpoint, + isEdited, + messageId, + unfinished, + tokenCount, + newMessageId, + finish_reason, + conversationId, + parentMessageId, + isCreatedByUser, + } = params; + const validConvoId = idSchema.safeParse(conversationId); if (!validConvoId.success) { - throw new Error('Invalid conversation ID'); + logger.warn(`Invalid conversation ID: ${conversationId}`); + logger.info(params); + return; } const update = { diff --git a/api/models/Message.spec.js b/api/models/Message.spec.js index d5fbba9ed8..a38458a68b 100644 --- a/api/models/Message.spec.js +++ b/api/models/Message.spec.js @@ -78,7 +78,7 @@ describe('Message Operations', () => { it('should throw an error for invalid conversation ID', async () => { mockMessage.conversationId = 'invalid-id'; - await expect(saveMessage(mockReq, mockMessage)).rejects.toThrow('Invalid conversation ID'); + await expect(saveMessage(mockReq, mockMessage)).resolves.toBeUndefined(); }); }); diff --git a/api/server/routes/messages.js b/api/server/routes/messages.js index f06a19b483..99c098e9ed 100644 --- a/api/server/routes/messages.js +++ b/api/server/routes/messages.js @@ -1,45 +1,75 @@ const express = require('express'); -const router = express.Router(); const { saveConvo, saveMessage, getMessages, updateMessage, deleteMessages } = require('~/models'); const { requireJwtAuth, validateMessageReq } = require('~/server/middleware'); const { countTokens } = require('~/server/utils'); +const { logger } = require('~/config'); +const router = express.Router(); router.use(requireJwtAuth); /* Note: It's necessary to add `validateMessageReq` within route definition for correct params */ router.get('/:conversationId', validateMessageReq, async (req, res) => { - const { conversationId } = req.params; - res.status(200).send(await getMessages({ conversationId }, '-_id -__v -user')); + try { + const { conversationId } = req.params; + const messages = await getMessages({ conversationId }, '-_id -__v -user'); + res.status(200).json(messages); + } catch (error) { + logger.error('Error fetching messages:', error); + res.status(500).json({ error: 'Internal server error' }); + } }); -// CREATE router.post('/:conversationId', validateMessageReq, async (req, res) => { - const message = req.body; - const savedMessage = await saveMessage(req, { ...message, user: req.user.id }); - await saveConvo(req.user.id, savedMessage); - res.status(201).send(savedMessage); + try { + const message = req.body; + const savedMessage = await saveMessage(req, { ...message, user: req.user.id }); + if (!savedMessage) { + return res.status(400).json({ error: 'Message not saved' }); + } + await saveConvo(req.user.id, savedMessage); + res.status(201).json(savedMessage); + } catch (error) { + logger.error('Error saving message:', error); + res.status(500).json({ error: 'Internal server error' }); + } }); -// READ router.get('/:conversationId/:messageId', validateMessageReq, async (req, res) => { - const { conversationId, messageId } = req.params; - res.status(200).send(await getMessages({ conversationId, messageId }, '-_id -__v -user')); + try { + const { conversationId, messageId } = req.params; + const message = await getMessages({ conversationId, messageId }, '-_id -__v -user'); + if (!message) { + return res.status(404).json({ error: 'Message not found' }); + } + res.status(200).json(message); + } catch (error) { + logger.error('Error fetching message:', error); + res.status(500).json({ error: 'Internal server error' }); + } }); -// UPDATE router.put('/:conversationId/:messageId', validateMessageReq, async (req, res) => { - const { messageId, model } = req.params; - const { text } = req.body; - const tokenCount = await countTokens(text, model); - const result = await updateMessage(req, { messageId, text, tokenCount }); - res.status(201).json(result); + try { + const { messageId, model } = req.params; + const { text } = req.body; + const tokenCount = await countTokens(text, model); + const result = await updateMessage(req, { messageId, text, tokenCount }); + res.status(200).json(result); + } catch (error) { + logger.error('Error updating message:', error); + res.status(500).json({ error: 'Internal server error' }); + } }); -// DELETE router.delete('/:conversationId/:messageId', validateMessageReq, async (req, res) => { - const { messageId } = req.params; - await deleteMessages({ messageId }); - res.status(204).send(); + try { + const { messageId } = req.params; + await deleteMessages({ messageId }); + res.status(204).send(); + } catch (error) { + logger.error('Error deleting message:', error); + res.status(500).json({ error: 'Internal server error' }); + } }); module.exports = router;