mirror of
https://github.com/danny-avila/LibreChat.git
synced 2026-04-05 23:37:19 +02:00
🔎 fix: Specify Explicit Primary Key for Meilisearch Document Operations (#12542)
* 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.
This commit is contained in:
Danny Avila
GitHub
parent
b44ce264a4
commit
33ee7dea1e
2 changed files with 105 additions and 15 deletions
|
|
@ -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<void>((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 () => {
|
||||
|
|
|
|||
|
|
@ -94,7 +94,7 @@ const getSyncConfig = () => ({
|
|||
* Validates the required options for configuring the mongoMeili plugin.
|
||||
*/
|
||||
const validateOptions = (options: Partial<MongoMeiliOptions>): 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 <T>(
|
|||
* @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<MeiliIndexable>;
|
||||
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<void> {
|
||||
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<void> {
|
||||
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) {
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue