New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(mongo): move $fulltext from $and to top level #4066
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks!
packages/mongodb/src/MongoDriver.ts
Outdated
const and = copiedData.$and[i]; | ||
if ('$fulltext' in and) { | ||
if ('$fulltext' in copiedData) { | ||
throw new Error('Cannot merge multiple full text searches to top level of the query object.'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lets mention the $fulltext
operator directly here instead of "full text searches"
packages/mongodb/src/MongoDriver.ts
Outdated
if (copiedData.$and) { | ||
for (let i = 0; i < copiedData.$and.length; i++) { | ||
const and = copiedData.$and[i]; | ||
if ('$fulltext' in and) { | ||
if ('$fulltext' in copiedData) { | ||
throw new Error('Cannot merge multiple full text searches to top level of the query object.'); | ||
} | ||
copiedData.$fulltext = and.$fulltext!; | ||
delete and.$fulltext; | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can't say i like the solution, but i don't know any better right now. note that you introduced 4 levels of branching, you will need ~4 tests to cover what this does, not just one.
also lets move this into a private method (we should do the same with the rest of the fulltext code, this method is crazy long and complex)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I see you fixed those things by yourself. I was going to, but then started thinking about that limitation "Full text search is only supported on the top level of the query object", why is it so?
And it looks like mongodb (I checked 4.2.6 and latest) allows $text
to appear in nested conditions. You can check it with these tests
test('positive $text in nested conditions', async () => {
const booksRepository = orm.em.getRepository(Book);
const books = await booksRepository.find({ $or: [
{$text: {$search: 'Bible'} },
{title: {$regex: 'Something'} },
]} as unknown as FilterQuery<Book>);
expect(books.length).toBe(1);
})
test('negative $text in nested conditions', async () => {
const booksRepository = orm.em.getRepository(Book);
const books = await booksRepository.find({ $or: [
{$text: {$search: 'Something'} },
{title: {$regex: 'Bible'} },
]} as unknown as FilterQuery<Book>);
expect(books.length).toBe(1);
})
So I thought is it really necessary to forbid $fulltext
in nested objects?
Of course there are some limitations to use of $text
, for example in $or
mongo won't allow non-indexed fields together with $text
and this kind of query will fail
{$or: [{$text:{$search: 'some full text request'}}, {'someNonIndexedField': 42}]}
If you think it's a good idea, I could work on that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This came in as a community PR, feel free to improve it, the limitation indeed sounds quite artificial.
Closes #4065