From ba71375982ac287ae81707329b4e95d27988f393 Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Thu, 6 Nov 2025 11:44:28 -0500 Subject: [PATCH] =?UTF-8?q?=F0=9F=97=91=EF=B8=8F=20fix:=20Delete=20Shared?= =?UTF-8?q?=20Links=20on=20Conversation=20Deletion=20(#10396)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * ✨ feat: Enhance DELETE /all endpoint to remove shared links alongside conversations and tool calls - Added functionality to delete all shared links for a user when clearing conversations. - Introduced comprehensive tests to ensure correct behavior and error handling for the new deletion process. * ✨ feat: Implement deleteConvoSharedLink method and update conversation deletion logic to remove associated shared links --- api/server/routes/__tests__/convos.spec.js | 502 ++++++++++++++++++ api/server/routes/convos.js | 7 +- .../data-schemas/src/methods/share.test.ts | 101 ++++ packages/data-schemas/src/methods/share.ts | 33 +- 4 files changed, 640 insertions(+), 3 deletions(-) create mode 100644 api/server/routes/__tests__/convos.spec.js diff --git a/api/server/routes/__tests__/convos.spec.js b/api/server/routes/__tests__/convos.spec.js new file mode 100644 index 0000000000..e1f9469bef --- /dev/null +++ b/api/server/routes/__tests__/convos.spec.js @@ -0,0 +1,502 @@ +const express = require('express'); +const request = require('supertest'); + +jest.mock('@librechat/agents', () => ({ + sleep: jest.fn(), +})); + +jest.mock('@librechat/api', () => ({ + isEnabled: jest.fn(), + createAxiosInstance: jest.fn(() => ({ + get: jest.fn(), + post: jest.fn(), + put: jest.fn(), + delete: jest.fn(), + })), + logAxiosError: jest.fn(), +})); + +jest.mock('@librechat/data-schemas', () => ({ + logger: { + debug: jest.fn(), + info: jest.fn(), + warn: jest.fn(), + error: jest.fn(), + }, + createModels: jest.fn(() => ({ + User: {}, + Conversation: {}, + Message: {}, + SharedLink: {}, + })), +})); + +jest.mock('~/models/Conversation', () => ({ + getConvosByCursor: jest.fn(), + getConvo: jest.fn(), + deleteConvos: jest.fn(), + saveConvo: jest.fn(), +})); + +jest.mock('~/models/ToolCall', () => ({ + deleteToolCalls: jest.fn(), +})); + +jest.mock('~/models', () => ({ + deleteAllSharedLinks: jest.fn(), + deleteConvoSharedLink: jest.fn(), +})); + +jest.mock('~/server/middleware/requireJwtAuth', () => (req, res, next) => next()); + +jest.mock('~/server/middleware', () => ({ + createImportLimiters: jest.fn(() => ({ + importIpLimiter: (req, res, next) => next(), + importUserLimiter: (req, res, next) => next(), + })), + createForkLimiters: jest.fn(() => ({ + forkIpLimiter: (req, res, next) => next(), + forkUserLimiter: (req, res, next) => next(), + })), + configMiddleware: (req, res, next) => next(), +})); + +jest.mock('~/server/utils/import/fork', () => ({ + forkConversation: jest.fn(), + duplicateConversation: jest.fn(), +})); + +jest.mock('~/server/utils/import', () => ({ + importConversations: jest.fn(), +})); + +jest.mock('~/cache/getLogStores', () => jest.fn()); + +jest.mock('~/server/routes/files/multer', () => ({ + storage: {}, + importFileFilter: jest.fn(), +})); + +jest.mock('multer', () => { + return jest.fn(() => ({ + single: jest.fn(() => (req, res, next) => { + req.file = { path: '/tmp/test-file.json' }; + next(); + }), + })); +}); + +jest.mock('librechat-data-provider', () => ({ + CacheKeys: { + GEN_TITLE: 'GEN_TITLE', + }, + EModelEndpoint: { + azureAssistants: 'azureAssistants', + assistants: 'assistants', + }, +})); + +jest.mock('~/server/services/Endpoints/azureAssistants', () => ({ + initializeClient: jest.fn(), +})); + +jest.mock('~/server/services/Endpoints/assistants', () => ({ + initializeClient: jest.fn(), +})); + +describe('Convos Routes', () => { + let app; + let convosRouter; + const { deleteAllSharedLinks, deleteConvoSharedLink } = require('~/models'); + const { deleteConvos } = require('~/models/Conversation'); + const { deleteToolCalls } = require('~/models/ToolCall'); + + beforeAll(() => { + convosRouter = require('../convos'); + + app = express(); + app.use(express.json()); + + /** Mock authenticated user */ + app.use((req, res, next) => { + req.user = { id: 'test-user-123' }; + next(); + }); + + app.use('/api/convos', convosRouter); + }); + + beforeEach(() => { + jest.clearAllMocks(); + }); + + describe('DELETE /all', () => { + it('should delete all conversations, tool calls, and shared links for a user', async () => { + const mockDbResponse = { + deletedCount: 5, + message: 'All conversations deleted successfully', + }; + + deleteConvos.mockResolvedValue(mockDbResponse); + deleteToolCalls.mockResolvedValue({ deletedCount: 10 }); + deleteAllSharedLinks.mockResolvedValue({ + message: 'All shared links deleted successfully', + deletedCount: 3, + }); + + const response = await request(app).delete('/api/convos/all'); + + expect(response.status).toBe(201); + expect(response.body).toEqual(mockDbResponse); + + /** Verify deleteConvos was called with correct userId */ + expect(deleteConvos).toHaveBeenCalledWith('test-user-123', {}); + expect(deleteConvos).toHaveBeenCalledTimes(1); + + /** Verify deleteToolCalls was called with correct userId */ + expect(deleteToolCalls).toHaveBeenCalledWith('test-user-123'); + expect(deleteToolCalls).toHaveBeenCalledTimes(1); + + /** Verify deleteAllSharedLinks was called with correct userId */ + expect(deleteAllSharedLinks).toHaveBeenCalledWith('test-user-123'); + expect(deleteAllSharedLinks).toHaveBeenCalledTimes(1); + }); + + it('should call deleteAllSharedLinks even when no conversations exist', async () => { + const mockDbResponse = { + deletedCount: 0, + message: 'No conversations to delete', + }; + + deleteConvos.mockResolvedValue(mockDbResponse); + deleteToolCalls.mockResolvedValue({ deletedCount: 0 }); + deleteAllSharedLinks.mockResolvedValue({ + message: 'All shared links deleted successfully', + deletedCount: 0, + }); + + const response = await request(app).delete('/api/convos/all'); + + expect(response.status).toBe(201); + expect(deleteAllSharedLinks).toHaveBeenCalledWith('test-user-123'); + }); + + it('should return 500 if deleteConvos fails', async () => { + const errorMessage = 'Database connection error'; + deleteConvos.mockRejectedValue(new Error(errorMessage)); + + const response = await request(app).delete('/api/convos/all'); + + expect(response.status).toBe(500); + expect(response.text).toBe('Error clearing conversations'); + + /** Verify error was logged */ + const { logger } = require('@librechat/data-schemas'); + expect(logger.error).toHaveBeenCalledWith('Error clearing conversations', expect.any(Error)); + }); + + it('should return 500 if deleteToolCalls fails', async () => { + deleteConvos.mockResolvedValue({ deletedCount: 5 }); + deleteToolCalls.mockRejectedValue(new Error('Tool calls deletion failed')); + + const response = await request(app).delete('/api/convos/all'); + + expect(response.status).toBe(500); + expect(response.text).toBe('Error clearing conversations'); + }); + + it('should return 500 if deleteAllSharedLinks fails', async () => { + deleteConvos.mockResolvedValue({ deletedCount: 5 }); + deleteToolCalls.mockResolvedValue({ deletedCount: 10 }); + deleteAllSharedLinks.mockRejectedValue(new Error('Shared links deletion failed')); + + const response = await request(app).delete('/api/convos/all'); + + expect(response.status).toBe(500); + expect(response.text).toBe('Error clearing conversations'); + }); + + it('should handle multiple users independently', async () => { + /** First user */ + deleteConvos.mockResolvedValue({ deletedCount: 3 }); + deleteToolCalls.mockResolvedValue({ deletedCount: 5 }); + deleteAllSharedLinks.mockResolvedValue({ deletedCount: 2 }); + + let response = await request(app).delete('/api/convos/all'); + + expect(response.status).toBe(201); + expect(deleteAllSharedLinks).toHaveBeenCalledWith('test-user-123'); + + jest.clearAllMocks(); + + /** Second user (simulate different user by modifying middleware) */ + const app2 = express(); + app2.use(express.json()); + app2.use((req, res, next) => { + req.user = { id: 'test-user-456' }; + next(); + }); + app2.use('/api/convos', require('../convos')); + + deleteConvos.mockResolvedValue({ deletedCount: 7 }); + deleteToolCalls.mockResolvedValue({ deletedCount: 12 }); + deleteAllSharedLinks.mockResolvedValue({ deletedCount: 4 }); + + response = await request(app2).delete('/api/convos/all'); + + expect(response.status).toBe(201); + expect(deleteAllSharedLinks).toHaveBeenCalledWith('test-user-456'); + }); + + it('should execute deletions in correct sequence', async () => { + const executionOrder = []; + + deleteConvos.mockImplementation(() => { + executionOrder.push('deleteConvos'); + return Promise.resolve({ deletedCount: 5 }); + }); + + deleteToolCalls.mockImplementation(() => { + executionOrder.push('deleteToolCalls'); + return Promise.resolve({ deletedCount: 10 }); + }); + + deleteAllSharedLinks.mockImplementation(() => { + executionOrder.push('deleteAllSharedLinks'); + return Promise.resolve({ deletedCount: 3 }); + }); + + await request(app).delete('/api/convos/all'); + + /** Verify all three functions were called */ + expect(executionOrder).toEqual(['deleteConvos', 'deleteToolCalls', 'deleteAllSharedLinks']); + }); + + it('should maintain data integrity by cleaning up shared links when conversations are deleted', async () => { + /** This test ensures that orphaned shared links are prevented */ + const mockConvosDeleted = { deletedCount: 10 }; + const mockToolCallsDeleted = { deletedCount: 15 }; + const mockSharedLinksDeleted = { + message: 'All shared links deleted successfully', + deletedCount: 8, + }; + + deleteConvos.mockResolvedValue(mockConvosDeleted); + deleteToolCalls.mockResolvedValue(mockToolCallsDeleted); + deleteAllSharedLinks.mockResolvedValue(mockSharedLinksDeleted); + + const response = await request(app).delete('/api/convos/all'); + + expect(response.status).toBe(201); + + /** Verify that shared links cleanup was called for the same user */ + expect(deleteAllSharedLinks).toHaveBeenCalledWith('test-user-123'); + + /** Verify no shared links remain for deleted conversations */ + expect(deleteAllSharedLinks).toHaveBeenCalledAfter(deleteConvos); + }); + }); + + describe('DELETE /', () => { + it('should delete a single conversation, tool calls, and associated shared links', async () => { + const mockConversationId = 'conv-123'; + const mockDbResponse = { + deletedCount: 1, + message: 'Conversation deleted successfully', + }; + + deleteConvos.mockResolvedValue(mockDbResponse); + deleteToolCalls.mockResolvedValue({ deletedCount: 3 }); + deleteConvoSharedLink.mockResolvedValue({ + message: 'Shared links deleted successfully', + deletedCount: 1, + }); + + const response = await request(app) + .delete('/api/convos') + .send({ + arg: { + conversationId: mockConversationId, + }, + }); + + expect(response.status).toBe(201); + expect(response.body).toEqual(mockDbResponse); + + /** Verify deleteConvos was called with correct parameters */ + expect(deleteConvos).toHaveBeenCalledWith('test-user-123', { + conversationId: mockConversationId, + }); + + /** Verify deleteToolCalls was called */ + expect(deleteToolCalls).toHaveBeenCalledWith('test-user-123', mockConversationId); + + /** Verify deleteConvoSharedLink was called */ + expect(deleteConvoSharedLink).toHaveBeenCalledWith('test-user-123', mockConversationId); + }); + + it('should not call deleteConvoSharedLink when no conversationId provided', async () => { + deleteConvos.mockResolvedValue({ deletedCount: 0 }); + deleteToolCalls.mockResolvedValue({ deletedCount: 0 }); + + const response = await request(app) + .delete('/api/convos') + .send({ + arg: { + source: 'button', + }, + }); + + expect(response.status).toBe(200); + expect(deleteConvoSharedLink).not.toHaveBeenCalled(); + }); + + it('should handle deletion of conversation without shared links', async () => { + const mockConversationId = 'conv-no-shares'; + + deleteConvos.mockResolvedValue({ deletedCount: 1 }); + deleteToolCalls.mockResolvedValue({ deletedCount: 0 }); + deleteConvoSharedLink.mockResolvedValue({ + message: 'Shared links deleted successfully', + deletedCount: 0, + }); + + const response = await request(app) + .delete('/api/convos') + .send({ + arg: { + conversationId: mockConversationId, + }, + }); + + expect(response.status).toBe(201); + expect(deleteConvoSharedLink).toHaveBeenCalledWith('test-user-123', mockConversationId); + }); + + it('should return 400 when no parameters provided', async () => { + const response = await request(app).delete('/api/convos').send({ + arg: {}, + }); + + expect(response.status).toBe(400); + expect(response.body).toEqual({ error: 'no parameters provided' }); + expect(deleteConvos).not.toHaveBeenCalled(); + expect(deleteConvoSharedLink).not.toHaveBeenCalled(); + }); + + it('should return 500 if deleteConvoSharedLink fails', async () => { + const mockConversationId = 'conv-error'; + + deleteConvos.mockResolvedValue({ deletedCount: 1 }); + deleteToolCalls.mockResolvedValue({ deletedCount: 2 }); + deleteConvoSharedLink.mockRejectedValue(new Error('Failed to delete shared links')); + + const response = await request(app) + .delete('/api/convos') + .send({ + arg: { + conversationId: mockConversationId, + }, + }); + + expect(response.status).toBe(500); + expect(response.text).toBe('Error clearing conversations'); + }); + + it('should execute deletions in correct sequence for single conversation', async () => { + const mockConversationId = 'conv-sequence'; + const executionOrder = []; + + deleteConvos.mockImplementation(() => { + executionOrder.push('deleteConvos'); + return Promise.resolve({ deletedCount: 1 }); + }); + + deleteToolCalls.mockImplementation(() => { + executionOrder.push('deleteToolCalls'); + return Promise.resolve({ deletedCount: 2 }); + }); + + deleteConvoSharedLink.mockImplementation(() => { + executionOrder.push('deleteConvoSharedLink'); + return Promise.resolve({ deletedCount: 1 }); + }); + + await request(app) + .delete('/api/convos') + .send({ + arg: { + conversationId: mockConversationId, + }, + }); + + expect(executionOrder).toEqual(['deleteConvos', 'deleteToolCalls', 'deleteConvoSharedLink']); + }); + + it('should prevent orphaned shared links when deleting single conversation', async () => { + const mockConversationId = 'conv-with-shares'; + + deleteConvos.mockResolvedValue({ deletedCount: 1 }); + deleteToolCalls.mockResolvedValue({ deletedCount: 4 }); + deleteConvoSharedLink.mockResolvedValue({ + message: 'Shared links deleted successfully', + deletedCount: 2, + }); + + const response = await request(app) + .delete('/api/convos') + .send({ + arg: { + conversationId: mockConversationId, + }, + }); + + expect(response.status).toBe(201); + + /** Verify shared links were deleted for the specific conversation */ + expect(deleteConvoSharedLink).toHaveBeenCalledWith('test-user-123', mockConversationId); + + /** Verify it was called after the conversation was deleted */ + expect(deleteConvoSharedLink).toHaveBeenCalledAfter(deleteConvos); + }); + }); +}); + +/** + * Custom Jest matcher to verify function call order + */ +expect.extend({ + toHaveBeenCalledAfter(received, other) { + const receivedCalls = received.mock.invocationCallOrder; + const otherCalls = other.mock.invocationCallOrder; + + if (receivedCalls.length === 0) { + return { + pass: false, + message: () => + `Expected ${received.getMockName()} to have been called after ${other.getMockName()}, but ${received.getMockName()} was never called`, + }; + } + + if (otherCalls.length === 0) { + return { + pass: false, + message: () => + `Expected ${received.getMockName()} to have been called after ${other.getMockName()}, but ${other.getMockName()} was never called`, + }; + } + + const lastReceivedCall = receivedCalls[receivedCalls.length - 1]; + const firstOtherCall = otherCalls[0]; + + const pass = lastReceivedCall > firstOtherCall; + + return { + pass, + message: () => + pass + ? `Expected ${received.getMockName()} not to have been called after ${other.getMockName()}` + : `Expected ${received.getMockName()} to have been called after ${other.getMockName()}`, + }; + }, +}); diff --git a/api/server/routes/convos.js b/api/server/routes/convos.js index 704b12751f..766b2a21b0 100644 --- a/api/server/routes/convos.js +++ b/api/server/routes/convos.js @@ -12,6 +12,7 @@ const { const { getConvosByCursor, deleteConvos, getConvo, saveConvo } = require('~/models/Conversation'); const { forkConversation, duplicateConversation } = require('~/server/utils/import/fork'); const { storage, importFileFilter } = require('~/server/routes/files/multer'); +const { deleteAllSharedLinks, deleteConvoSharedLink } = require('~/models'); const requireJwtAuth = require('~/server/middleware/requireJwtAuth'); const { importConversations } = require('~/server/utils/import'); const { deleteToolCalls } = require('~/models/ToolCall'); @@ -124,7 +125,10 @@ router.delete('/', async (req, res) => { try { const dbResponse = await deleteConvos(req.user.id, filter); - await deleteToolCalls(req.user.id, filter.conversationId); + if (filter.conversationId) { + await deleteToolCalls(req.user.id, filter.conversationId); + await deleteConvoSharedLink(req.user.id, filter.conversationId); + } res.status(201).json(dbResponse); } catch (error) { logger.error('Error clearing conversations', error); @@ -136,6 +140,7 @@ router.delete('/all', async (req, res) => { try { const dbResponse = await deleteConvos(req.user.id, {}); await deleteToolCalls(req.user.id); + await deleteAllSharedLinks(req.user.id); res.status(201).json(dbResponse); } catch (error) { logger.error('Error clearing conversations', error); diff --git a/packages/data-schemas/src/methods/share.test.ts b/packages/data-schemas/src/methods/share.test.ts index 1deec967e8..302b16811c 100644 --- a/packages/data-schemas/src/methods/share.test.ts +++ b/packages/data-schemas/src/methods/share.test.ts @@ -936,6 +936,107 @@ describe('Share Methods', () => { }); }); + describe('deleteConvoSharedLink', () => { + test('should delete all shared links for a specific conversation', async () => { + const userId = new mongoose.Types.ObjectId().toString(); + const conversationId1 = 'conv-to-delete'; + const conversationId2 = 'conv-to-keep'; + + await SharedLink.create([ + { shareId: 'share1', conversationId: conversationId1, user: userId, isPublic: true }, + { shareId: 'share2', conversationId: conversationId1, user: userId, isPublic: false }, + { shareId: 'share3', conversationId: conversationId2, user: userId, isPublic: true }, + ]); + + const result = await shareMethods.deleteConvoSharedLink(userId, conversationId1); + + expect(result.deletedCount).toBe(2); + expect(result.message).toContain('successfully'); + + const remainingShares = await SharedLink.find({}); + expect(remainingShares).toHaveLength(1); + expect(remainingShares[0].conversationId).toBe(conversationId2); + }); + + test('should only delete shares for the specified user and conversation', async () => { + const userId1 = new mongoose.Types.ObjectId().toString(); + const userId2 = new mongoose.Types.ObjectId().toString(); + const conversationId = 'shared-conv'; + + await SharedLink.create([ + { shareId: 'share1', conversationId, user: userId1, isPublic: true }, + { shareId: 'share2', conversationId, user: userId2, isPublic: true }, + { shareId: 'share3', conversationId: 'other-conv', user: userId1, isPublic: true }, + ]); + + const result = await shareMethods.deleteConvoSharedLink(userId1, conversationId); + + expect(result.deletedCount).toBe(1); + + const remainingShares = await SharedLink.find({}); + expect(remainingShares).toHaveLength(2); + expect( + remainingShares.some((s) => s.user === userId2 && s.conversationId === conversationId), + ).toBe(true); + expect( + remainingShares.some((s) => s.user === userId1 && s.conversationId === 'other-conv'), + ).toBe(true); + }); + + test('should handle when no shares exist for the conversation', async () => { + const userId = new mongoose.Types.ObjectId().toString(); + const conversationId = 'nonexistent-conv'; + + const result = await shareMethods.deleteConvoSharedLink(userId, conversationId); + + expect(result.deletedCount).toBe(0); + expect(result.message).toContain('successfully'); + }); + + test('should throw error when userId is missing', async () => { + await expect(shareMethods.deleteConvoSharedLink('', 'conv123')).rejects.toThrow( + 'Missing required parameters', + ); + }); + + test('should throw error when conversationId is missing', async () => { + await expect(shareMethods.deleteConvoSharedLink('user123', '')).rejects.toThrow( + 'Missing required parameters', + ); + }); + + test('should delete multiple shared links for same conversation', async () => { + const userId = new mongoose.Types.ObjectId().toString(); + const conversationId = 'conv-with-many-shares'; + + await SharedLink.create([ + { shareId: 'share1', conversationId, user: userId, isPublic: true }, + { + shareId: 'share2', + conversationId, + user: userId, + isPublic: true, + targetMessageId: 'msg1', + }, + { + shareId: 'share3', + conversationId, + user: userId, + isPublic: true, + targetMessageId: 'msg2', + }, + { shareId: 'share4', conversationId, user: userId, isPublic: false }, + ]); + + const result = await shareMethods.deleteConvoSharedLink(userId, conversationId); + + expect(result.deletedCount).toBe(4); + + const remainingShares = await SharedLink.find({ conversationId, user: userId }); + expect(remainingShares).toHaveLength(0); + }); + }); + describe('Edge Cases and Error Handling', () => { test('should handle conversation with special characters in ID', async () => { const userId = new mongoose.Types.ObjectId().toString(); diff --git a/packages/data-schemas/src/methods/share.ts b/packages/data-schemas/src/methods/share.ts index 11e893ff9c..2a0d2bc3bd 100644 --- a/packages/data-schemas/src/methods/share.ts +++ b/packages/data-schemas/src/methods/share.ts @@ -173,8 +173,8 @@ export function createShareMethods(mongoose: typeof import('mongoose')) { return null; } - // Filter messages based on targetMessageId if present (branch-specific sharing) - let messagesToShare = share.messages; + /** Filtered messages based on targetMessageId if present (branch-specific sharing) */ + let messagesToShare: t.IMessage[] = share.messages; if (share.targetMessageId) { messagesToShare = getMessagesUpToTarget(share.messages, share.targetMessageId); } @@ -310,6 +310,34 @@ export function createShareMethods(mongoose: typeof import('mongoose')) { } } + /** + * Delete shared links by conversation ID + */ + async function deleteConvoSharedLink( + user: string, + conversationId: string, + ): Promise { + if (!user || !conversationId) { + throw new ShareServiceError('Missing required parameters', 'INVALID_PARAMS'); + } + + try { + const SharedLink = mongoose.models.SharedLink as Model; + const result = await SharedLink.deleteMany({ user, conversationId }); + return { + message: 'Shared links deleted successfully', + deletedCount: result.deletedCount, + }; + } catch (error) { + logger.error('[deleteConvoSharedLink] Error deleting shared links', { + error: error instanceof Error ? error.message : 'Unknown error', + user, + conversationId, + }); + throw new ShareServiceError('Error deleting shared links', 'SHARE_DELETE_ERROR'); + } + } + /** * Create a new shared link for a conversation */ @@ -528,6 +556,7 @@ export function createShareMethods(mongoose: typeof import('mongoose')) { deleteSharedLink, getSharedMessages, deleteAllSharedLinks, + deleteConvoSharedLink, }; }