Skip to content
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

Multi-fields search in documents is not working #264

Closed
dfanchon opened this issue Sep 2, 2021 · 16 comments
Closed

Multi-fields search in documents is not working #264

dfanchon opened this issue Sep 2, 2021 · 16 comments
Assignees
Labels
bug Something isn't working

Comments

@dfanchon
Copy link

dfanchon commented Sep 2, 2021

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
Copy link

zuramai commented Sep 3, 2021

same here

@theguy147
Copy link
Contributor

same issue here.

I had a quick look at the sources and I think the issue is:

flexsearch/src/index.js

Lines 444 to 447 in 65b027c

if(!length){
return result;
}

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
Copy link
Contributor

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

flexsearch/src/index.js

Lines 397 to 403 in 65b027c

if(options){
limit = options["limit"];
offset = options["offset"] || 0;
context = options["context"];
suggest = SUPPORT_SUGGESTION && options["suggest"];
}

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

query = query || options["query"];

@theguy147
Copy link
Contributor

theguy147 commented Sep 10, 2021

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.

query = options["query"];

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
Copy link
Contributor

theguy147 commented Sep 10, 2021

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
Copy link

@theguy147 have you considered making a PR?

@GanserITService
Copy link

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

@micheljung
Copy link

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

@theguy147
Copy link
Contributor

theguy147 commented Oct 18, 2021

@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
Copy link
Contributor

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

@micheljung
Copy link

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

@a-tonchev
Copy link

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

1 similar comment
@mmm8955405
Copy link

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

@mmm8955405
Copy link

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

@ts-thomas ts-thomas self-assigned this Oct 2, 2022
@ts-thomas ts-thomas added the bug Something isn't working label Oct 2, 2022
@ts-thomas
Copy link
Contributor

ts-thomas commented Oct 2, 2022

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

@ts-thomas
Copy link
Contributor

Additional fix was added in v0.7.23

ts-thomas added a commit that referenced this issue Oct 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

8 participants