nextapps-de / flexsearch

Next-Generation full text search library for Browser and Node.js
Apache License 2.0
12.53k stars 491 forks source link

Multi-fields search in documents is not working #264

Closed dfanchon closed 2 years ago

dfanchon commented 3 years ago

Hi,

Following the doc this sample code should work but it yields an empty array. Any clues about what is done wrong?

var { Document } = require(`flexsearch`);

var index = new Document({
  document: {
    id: "id",
    index: ["title"]
  }
});

index.add({
  id: 0,
  title: "I am a test"
});
index.add({
  id: 1,
  title: "I am also a test"
});

var result = index.search([
  {
    field: "title",
    query: "test"
  },
  {
    field: "title",
    query: "also"
  }
]);
console.log(`result`, JSON.stringify(result));
zuramai commented 3 years ago

same here

theguy147 commented 3 years ago

same issue here.

I had a quick look at the sources and I think the issue is: https://github.com/nextapps-de/flexsearch/blob/65b027ca316ceefd8ea89f472561a0f91179b2f3/src/index.js#L444-L447

when doing a multi-field search on a document flexsearch appears to call search on each index without setting query. if query were set then length would be assigned in this block of code: https://github.com/nextapps-de/flexsearch/blob/master/src/index.js#L405-L442

But because that block is not being executed length is not set and therefore search returns early (without results).

(I haven't checked my findings yet because I don't currently have a device nearby that I can use for a testing environment)

If I'm right either Document.search needs to retrieve query from opt and pass it along or this has to be done inside Index.search if no query was provided.

theguy147 commented 3 years ago

Ok, finally got around to testing it and this fixes it for me:

https://github.com/nextapps-de/flexsearch/blob/65b027ca316ceefd8ea89f472561a0f91179b2f3/src/index.js#L397-L403

This block could to be changed to include e.g.:

query = query || options["query"];
theguy147 commented 3 years ago

The source of the issue is L481 in document.js.query remains undefined in the example used by @dfanchon because options["query"] is not defined in that case.

https://github.com/nextapps-de/flexsearch/blob/65b027ca316ceefd8ea89f472561a0f91179b2f3/src/document.js#L481

Fix

This would be a fix targeting document.js directly instead of the fix i suggested above in index.js:

diff --git a/src/document.js b/src/document.js
index 25da47c..9df6471 100644
--- a/src/document.js
+++ b/src/document.js
@@ -558,7 +558,8 @@ Document.prototype.search = function(query, limit, options, _resolve){
         if(!is_string(key)){

             opt = key;
-            key = key["field"];
+            key = opt["field"];
+            query = query || opt["query"];
         }

         if(promises){
theguy147 commented 3 years ago

ups, nevermind. my second fix doesn't work as intended because in the second iteration of the loop query has already been set and remains the same for all remaining iterations.

I guess it could be solved by either introducing a new variable or something like this:

diff --git a/src/document.js b/src/document.js
index 25da47c..5fd44c5 100644
--- a/src/document.js
+++ b/src/document.js
@@ -563,7 +563,7 @@ Document.prototype.search = function(query, limit, options, _resolve){

         if(promises){

-            promises[i] = this.index[key].searchAsync(query, limit, opt || options);
+            promises[i] = this.index[key].searchAsync((opt && opt["query"]) || query, limit, opt || options);

             // just collect and continue

@@ -577,7 +577,7 @@ Document.prototype.search = function(query, limit, options, _resolve){

             // inherit options also when search? it is just for laziness, Object.assign() has a cost

-            res = this.index[key].search(query, limit, opt || options);
+            res = this.index[key].search((opt && opt["query"]) || query, limit, opt || options);
         }

         len = res && res.length;

but performance-wise both of these solutions might not be optimal. so for now I will stick to my first suggestion (to modify index.js) but changing the order around to give precedence to opt["query"] over query:

diff --git a/src/index.js b/src/index.js
index 782ee0e..797cda8 100644
--- a/src/index.js
+++ b/src/index.js
@@ -400,6 +400,7 @@ Index.prototype.search = function(query, limit, options){
         offset = options["offset"] || 0;
         context = options["context"];
         suggest = SUPPORT_SUGGESTION && options["suggest"];
+        query = options["query"] || query;
     }

     if(query){
micheljung commented 3 years ago

@theguy147 have you considered making a PR?

GanserITService commented 3 years ago

@theguy147 when is the next release planned with the fix?

micheljung commented 3 years ago

@GanserITService he won't be able to tell you that since he's not a contributor, but a user.

theguy147 commented 3 years ago

@micheljung I had thought about opening a PR but because I didn't know if my fix is good enough I thought explaining what the problem is and what could be changed might be just as good. I did anyway open that PR now and referenced this issue for the details.

@GanserITService Yes, as @micheljung said, I am not a contributor in this project and therefore have no say in this ;) Hopefully soon :)

theguy147 commented 3 years ago

Update: @ts-thomas just merged the fix into master, so it should be fixed if you're on current master now

micheljung commented 3 years ago

@theguy147 do you have any idea how this works with {enrich: true}? I feel like there's another bug :)

a-tonchev commented 2 years ago

I also face this bug, what would be the workaround?

mmm8955405 commented 2 years ago

I also face this bug, what would be the workaround?

mmm8955405 commented 2 years ago

I think the stable production version should not make such low-level mistakes

ts-thomas commented 2 years ago

Automated tests will be back in the next version. Sorry for the circumstances. This issue gets the top priority.

ts-thomas commented 2 years ago

Additional fix was added in v0.7.23