flexsearch icon indicating copy to clipboard operation
flexsearch copied to clipboard

Multi-fields search in documents is not working

Open dfanchon opened this issue 2 years ago • 14 comments

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));

dfanchon avatar Sep 02 '21 15:09 dfanchon

same here

zuramai avatar Sep 03 '21 09:09 zuramai

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 avatar Sep 03 '21 16:09 theguy147

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 avatar Sep 03 '21 17:09 theguy147

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 avatar Sep 10 '21 10:09 theguy147

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){

theguy147 avatar Sep 10 '21 20:09 theguy147

@theguy147 have you considered making a PR?

micheljung avatar Oct 17 '21 07:10 micheljung

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

GanserITService avatar Oct 18 '21 05:10 GanserITService

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

micheljung avatar Oct 18 '21 08:10 micheljung

@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 avatar Oct 18 '21 10:10 theguy147

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

theguy147 avatar Oct 18 '21 16:10 theguy147

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

micheljung avatar Oct 20 '21 17:10 micheljung

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

a-tonchev avatar Nov 30 '21 14:11 a-tonchev

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

mmm8955405 avatar Apr 07 '22 12:04 mmm8955405

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

mmm8955405 avatar Apr 07 '22 12:04 mmm8955405

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

ts-thomas avatar Oct 02 '22 16:10 ts-thomas

Additional fix was added in v0.7.23

ts-thomas avatar Oct 03 '22 09:10 ts-thomas