From b203ff586825b625d6277fdbd5cad81239a620ae Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Fri, 3 Apr 2026 12:16:10 -0400 Subject: [PATCH] fix: resolve additional Meilisearch plugin bugs found in review Address review findings from PR #12542: - Fix deleteObjectFromMeili using MongoDB _id instead of the Meilisearch primary key (conversationId/messageId), causing post-remove cleanup to silently no-op and leave orphaned documents in the index. - Pass options.primaryKey explicitly to createMeiliMongooseModel factory instead of deriving it from attributesToIndex[0] (schema field order), eliminating a fragile implicit contract. - Fix updateObjectToMeili skipping preprocessObjectForIndex, which meant updates bypassed content array-to-text conversion and conversationId pipe character escaping. - Change collection.updateMany to collection.updateOne in addObjectToMeili since _id is unique (semantic correctness). - Add primaryKey to validateOptions required keys. - Strengthen test assertions to verify { primaryKey } argument is passed to addDocuments, addDocumentsInBatches, and updateDocuments. Add tests for the update path including preprocessObjectForIndex pipe escaping. --- .../src/models/plugins/mongoMeili.spec.ts | 57 +++++++++++++++++-- .../src/models/plugins/mongoMeili.ts | 18 +++--- 2 files changed, 61 insertions(+), 14 deletions(-) diff --git a/packages/data-schemas/src/models/plugins/mongoMeili.spec.ts b/packages/data-schemas/src/models/plugins/mongoMeili.spec.ts index b1d3e24129..81b9dfbac8 100644 --- a/packages/data-schemas/src/models/plugins/mongoMeili.spec.ts +++ b/packages/data-schemas/src/models/plugins/mongoMeili.spec.ts @@ -73,7 +73,10 @@ describe('Meilisearch Mongoose plugin', () => { title: 'Test Conversation', endpoint: EModelEndpoint.openAI, }); - expect(mockAddDocuments).toHaveBeenCalled(); + expect(mockAddDocuments).toHaveBeenCalledWith( + [expect.objectContaining({ conversationId: expect.anything() })], + { primaryKey: 'conversationId' }, + ); }); test('saving conversation indexes with expiredAt=null w/ meilisearch', async () => { @@ -105,7 +108,10 @@ describe('Meilisearch Mongoose plugin', () => { user: new mongoose.Types.ObjectId(), isCreatedByUser: true, }); - expect(mockAddDocuments).toHaveBeenCalled(); + expect(mockAddDocuments).toHaveBeenCalledWith( + [expect.objectContaining({ messageId: expect.anything() })], + { primaryKey: 'messageId' }, + ); }); test('saving messages with expiredAt=null indexes w/ meilisearch', async () => { @@ -130,6 +136,47 @@ describe('Meilisearch Mongoose plugin', () => { expect(mockAddDocuments).not.toHaveBeenCalled(); }); + test('updating an indexed conversation calls updateDocuments with primaryKey', async () => { + const conversationModel = createConversationModel(mongoose); + const convo = await conversationModel.create({ + conversationId: new mongoose.Types.ObjectId().toString(), + user: new mongoose.Types.ObjectId(), + title: 'Original Title', + endpoint: EModelEndpoint.openAI, + }); + mockUpdateDocuments.mockClear(); + + convo._meiliIndex = true; + convo.title = 'Updated Title'; + await convo.save(); + + expect(mockUpdateDocuments).toHaveBeenCalledWith( + [expect.objectContaining({ conversationId: expect.anything() })], + { primaryKey: 'conversationId' }, + ); + }); + + test('updateDocuments receives preprocessed data with primaryKey', async () => { + const conversationModel = createConversationModel(mongoose); + const conversationId = 'abc|def|ghi'; + const convo = await conversationModel.create({ + conversationId, + user: new mongoose.Types.ObjectId(), + title: 'Pipe Test', + endpoint: EModelEndpoint.openAI, + }); + mockUpdateDocuments.mockClear(); + + convo._meiliIndex = true; + convo.title = 'Updated Pipe Test'; + await convo.save(); + + expect(mockUpdateDocuments).toHaveBeenCalledWith( + [expect.objectContaining({ conversationId: 'abc--def--ghi' })], + { primaryKey: 'conversationId' }, + ); + }); + test('sync w/ meili does not include TTL documents', async () => { const conversationModel = createConversationModel(mongoose) as SchemaWithMeiliMethods; await conversationModel.create({ @@ -299,8 +346,10 @@ describe('Meilisearch Mongoose plugin', () => { // Run sync which should call processSyncBatch internally await conversationModel.syncWithMeili(); - // Verify addDocumentsInBatches was called (new batch method) - expect(mockAddDocumentsInBatches).toHaveBeenCalled(); + // Verify addDocumentsInBatches was called with explicit primaryKey + expect(mockAddDocumentsInBatches).toHaveBeenCalledWith(expect.any(Array), undefined, { + primaryKey: 'conversationId', + }); }); test('addObjectToMeili retries on failure', async () => { diff --git a/packages/data-schemas/src/models/plugins/mongoMeili.ts b/packages/data-schemas/src/models/plugins/mongoMeili.ts index 559f633af1..4692e7d4c7 100644 --- a/packages/data-schemas/src/models/plugins/mongoMeili.ts +++ b/packages/data-schemas/src/models/plugins/mongoMeili.ts @@ -94,7 +94,7 @@ const getSyncConfig = () => ({ * Validates the required options for configuring the mongoMeili plugin. */ const validateOptions = (options: Partial): void => { - const requiredKeys: (keyof MongoMeiliOptions)[] = ['host', 'apiKey', 'indexName']; + const requiredKeys: (keyof MongoMeiliOptions)[] = ['host', 'apiKey', 'indexName', 'primaryKey']; requiredKeys.forEach((key) => { if (!options[key]) { throw new Error(`Missing mongoMeili Option: ${key}`); @@ -136,13 +136,14 @@ const processBatch = async ( const createMeiliMongooseModel = ({ index, attributesToIndex, + primaryKey, syncOptions, }: { index: Index; attributesToIndex: string[]; + primaryKey: string; syncOptions: { batchSize: number; delayMs: number }; }) => { - const primaryKey = attributesToIndex[0]; const syncConfig = { ...getSyncConfig(), ...syncOptions }; class MeiliMongooseModel { @@ -436,11 +437,10 @@ const createMeiliMongooseModel = ({ } try { - const Model = this.constructor as Model; - await Model.updateMany( + // eslint-disable-next-line no-restricted-syntax -- _meiliIndex is an internal bookkeeping flag, not tenant-scoped data + await this.collection.updateOne( { _id: this._id as Types.ObjectId }, { $set: { _meiliIndex: true } }, - { timestamps: false }, ); } catch (error) { logger.error('[addObjectToMeili] Error updating _meiliIndex field:', error); @@ -458,9 +458,7 @@ const createMeiliMongooseModel = ({ next: CallbackWithoutResultAndOptionalError, ): Promise { try { - const object = _.omitBy(_.pick(this.toJSON(), attributesToIndex), (v, k) => - k.startsWith('$'), - ); + const object = this.preprocessObjectForIndex!(); await index.updateDocuments([object], { primaryKey }); next(); } catch (error) { @@ -479,7 +477,7 @@ const createMeiliMongooseModel = ({ next: CallbackWithoutResultAndOptionalError, ): Promise { try { - await index.deleteDocument(this._id as string); + await index.deleteDocument(String(this[primaryKey as keyof DocumentWithMeiliIndex])); next(); } catch (error) { logger.error('[deleteObjectFromMeili] Error deleting document from Meili:', error); @@ -645,7 +643,7 @@ export default function mongoMeili(schema: Schema, options: MongoMeiliOptions): logger.debug(`[mongoMeili] Added 'user' field to ${indexName} index attributes`); } - schema.loadClass(createMeiliMongooseModel({ index, attributesToIndex, syncOptions })); + schema.loadClass(createMeiliMongooseModel({ index, attributesToIndex, primaryKey, syncOptions })); // Register Mongoose hooks schema.post('save', function (doc: DocumentWithMeiliIndex, next) {