From 33ee7dea1e48c8fa8787914fc5deff737d1c82c5 Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Fri, 3 Apr 2026 18:01:06 -0400 Subject: [PATCH] =?UTF-8?q?=F0=9F=94=8E=20fix:=20Specify=20Explicit=20Prim?= =?UTF-8?q?ary=20Key=20for=20Meilisearch=20Document=20Operations=20(#12542?= =?UTF-8?q?)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix: pass explicit primaryKey to Meilisearch addDocuments/updateDocuments calls Meilisearch v1.0+ refuses to auto-infer the primary key when a document contains multiple fields ending with 'id'. The messages index has both conversationId and messageId, causing addDocuments to silently fail with index_primary_key_multiple_candidates_found, leaving message search empty. Pass { primaryKey } to addDocumentsInBatches, addDocuments, and updateDocuments — the variable was already in scope. Also replace raw this.collection.updateMany with Mongoose Model.updateMany to satisfy the no-restricted-syntax ESLint rule (tenant isolation guard). Closes #12538 * 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. * fix: add regression tests for delete and message update paths Address follow-up review findings: - Add test for deleteObjectFromMeili verifying it uses messageId (not MongoDB _id) when calling index.deleteDocument, guarding against regression of the silent orphaned-document bug. - Add test for message model update path asserting { primaryKey: 'messageId' } is passed to updateDocuments (previously only the conversation model update path was tested). - Add @param config.primaryKey to createMeiliMongooseModel JSDoc. --- .../src/models/plugins/mongoMeili.spec.ts | 97 ++++++++++++++++++- .../src/models/plugins/mongoMeili.ts | 23 ++--- 2 files changed, 105 insertions(+), 15 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..1d341a7939 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,87 @@ 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('updating an indexed message calls updateDocuments with primaryKey: messageId', async () => { + const messageModel = createMessageModel(mongoose); + const msg = await messageModel.create({ + messageId: new mongoose.Types.ObjectId().toString(), + conversationId: new mongoose.Types.ObjectId(), + user: new mongoose.Types.ObjectId(), + isCreatedByUser: true, + }); + mockUpdateDocuments.mockClear(); + + msg._meiliIndex = true; + msg.text = 'Updated text'; + await msg.save(); + + expect(mockUpdateDocuments).toHaveBeenCalledWith( + [expect.objectContaining({ messageId: expect.anything() })], + { primaryKey: 'messageId' }, + ); + }); + + test('deleteObjectFromMeili calls deleteDocument with messageId, not _id', async () => { + const messageModel = createMessageModel(mongoose); + const msgId = new mongoose.Types.ObjectId().toString(); + const msg = await messageModel.create({ + messageId: msgId, + conversationId: new mongoose.Types.ObjectId(), + user: new mongoose.Types.ObjectId(), + isCreatedByUser: true, + }); + mockDeleteDocument.mockClear(); + + const typedMsg = msg as unknown as import('./mongoMeili').DocumentWithMeiliIndex; + await new Promise((resolve, reject) => { + typedMsg.deleteObjectFromMeili!((err) => (err ? reject(err) : resolve())); + }); + + expect(mockDeleteDocument).toHaveBeenCalledWith(msgId); + expect(mockDeleteDocument).not.toHaveBeenCalledWith(String(msg._id)); + }); + + 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 +386,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 cc01dbb6c7..125e7bab71 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}`); @@ -130,19 +130,21 @@ const processBatch = async ( * @param config - Configuration object. * @param config.index - The MeiliSearch index object. * @param config.attributesToIndex - List of attributes to index. + * @param config.primaryKey - The primary key field for MeiliSearch document operations. * @param config.syncOptions - Sync configuration options. * @returns A class definition that will be loaded into the Mongoose schema. */ 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 { @@ -255,7 +257,7 @@ const createMeiliMongooseModel = ({ try { // Add documents to MeiliSearch - await index.addDocumentsInBatches(formattedDocs); + await index.addDocumentsInBatches(formattedDocs, undefined, { primaryKey }); // Update MongoDB to mark documents as indexed. // { timestamps: false } prevents Mongoose from touching updatedAt, preserving @@ -422,7 +424,7 @@ const createMeiliMongooseModel = ({ while (retryCount < maxRetries) { try { - await index.addDocuments([object]); + await index.addDocuments([object], { primaryKey }); break; } catch (error) { retryCount++; @@ -436,7 +438,8 @@ const createMeiliMongooseModel = ({ } try { - await this.collection.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 } }, ); @@ -456,10 +459,8 @@ const createMeiliMongooseModel = ({ next: CallbackWithoutResultAndOptionalError, ): Promise { try { - const object = _.omitBy(_.pick(this.toJSON(), attributesToIndex), (v, k) => - k.startsWith('$'), - ); - await index.updateDocuments([object]); + const object = this.preprocessObjectForIndex!(); + await index.updateDocuments([object], { primaryKey }); next(); } catch (error) { logger.error('[updateObjectToMeili] Error updating document in Meili:', error); @@ -477,7 +478,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); @@ -643,7 +644,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) {