mirror of
https://github.com/danny-avila/LibreChat.git
synced 2026-04-07 00:15:23 +02:00
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.
This commit is contained in:
parent
dc7889770a
commit
b203ff5868
2 changed files with 61 additions and 14 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,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 () => {
|
||||
|
|
|
|||
|
|
@ -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}`);
|
||||
|
|
@ -136,13 +136,14 @@ const processBatch = async <T>(
|
|||
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 {
|
||||
|
|
@ -436,11 +437,10 @@ const createMeiliMongooseModel = ({
|
|||
}
|
||||
|
||||
try {
|
||||
const Model = this.constructor as Model<DocumentWithMeiliIndex>;
|
||||
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<void> {
|
||||
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<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);
|
||||
|
|
@ -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) {
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue