Add support for searching in external files (fixing #89)

[R0Wi]

This fixes #89. It is implemented like proposed here: https://github.com/nextcloud/fulltextsearch/issues/546#issuecomment-583791976

[TheR00st3r]

Are there plans when this PR will be considered and released?

[R0Wi]

@4ipalino does this fix the issue for you?

[TheR00st3r]

@R0Wi I tested it again and it worked on my system. As long as the external Drive is not used as Rootdirectory with foldername "/".

It´s important to set an explicit foldername for the external share:

[R0Wi]

Thanks for the feedback. Thats a case i didn't test so i'll have a look at this. Maybe i can implement a solution for that, too.

[R0Wi]

@4ipalino i modified the patch so now a external share mounted as root-directory should also work. If that's the case, the check of the filepath is now omitted because logically seen every "external file" is allowed to appear in the search result.

Maybe you would like to check that out if you've some time left and give me feedback on this :smile:

[trendzetter]

I can confirm the fix is working. The code looks fine but probably my opinion is is not enough as a review since I am not a nextcloud developer.

[R0Wi]

@daita @MorrisJobke or maybe @skjnldsv any chance to get some feedback on this, please?

[skjnldsv]

@daita @MorrisJobke or maybe @skjnldsv any chance to get some feedback on this, please?

Sorry, this is out of my expertise! :/

[trendzetter]

WTF!? This is a fix for a major blocking bug in one of the most important features to make this enterprise ready. Why is no-one of the nextcloud willing to review your code??

[ArtificialOwl]

Hello, main dev on fts here.

I was supposed to work on the app last week but got overwhelmed by other more important thing.

[R0Wi]

Hello, main dev on fts here.

I was supposed to work on the app last week but got overwhelmed by other more important thing.

No problem for me. I already did a few minor improvements for some customers who use the FTS so maybe it would be interesting for you if i can support you somehow. Just let me know :-)

[trendzetter]

Sorry for my emotional reaction. I am looking into the contribution guidelines, see if I can make more of a contribution instead of pointing out issues.

[trendzetter]

I am still figuring out why this happens, not sure if it has anything to do with this fix but I am not seeing all of the results found. I often see only one or 2 or even no results listed. I just know results are found because it displays on top of the page:"de zoekopdracht in Files naar 'bunny' leverde 8 resultaten op in 51 ms" (8 results found in Dutch)

[R0Wi]

I am still figuring out why this happens, not sure if it has anything to do with this fix but I am not seeing all of the results found. I often see only one or 2 or even no results listed. I just know results are found because it displays on top of the page:"de zoekopdracht in Files naar 'bunny' leverde 8 resultaten op in 51 ms" (8 results found in Dutch)

I think this has nothing to do with this change i should rather be a frontend problem (this fix just extends the backend search mechanism). Maybe you could take a look at the dev-console network tab and see the JSON response returned by the server? If the result rows are missing it could be something on the backend otherwise i'd say there's something going on in the frontend javascript code or something. If you like you can create a issue here and i'll see if i can help you somehow (just to keep this discussion clean here)

[Jake-Etherflow]

Works perfectly! Thank you very much. I was able to mount nfs and then map nextcloud local storage to it with no further issues displaying results for the index.

[ArtificialOwl]

Hello,

There is no reason for this app to search/access remote files. Can you try to migrate your work onto files_fulltextsearch ?

[R0Wi]

Well currently the issue is that external files are properly indexed (with the help of files_fulltextsearch). But the results (indexed files) are then filtered by ownership when creating a search request inside this app. Because external files do not have a special owner they're filtered.

My current approach is to get all external mounts accessible to the current user when he uses the search functionallity. This info can then be integrated into the filter part of the ES query.

I know that this somehow mixes up the separation between files_fulltextsearch and fulltextsearch_elasticsearch but i did not find another way to implement the mentioned feature. Maybe you could give a draft on how to provide this inside files_fulltextsearch? I'd then really be looking forward implementing this :-)

[omidmeh]

I'm also very interested in this.

In the meantime, I tried just downloading the files that you've changed and replacing the ones from the original plugin installation with them. However, it looks like when I do that the search no longer works!

Is there any other steps I need to take other than replacing the old files with these in the installation folder?

[Jake-Etherflow]

I don't remember where the other patch was, but I had to combine the two in the same file to get search working and external files as well. If you'd like a copy of it, shoot me a message, and I can send it to you.

EDIT: Pretty sure #107 was it.

[R0Wi]

@omidmeh i think what you could do is cloning this repo, applying the patch from this PR and installing the apps dependencies manually (note you need to install git as well as composer). So basically you could do something like this:

cd <NEXTCLOUD_ROOT>/apps
rm -rf fulltextsearch_elasticsearch
mkdir fulltextsearch_elasticsearch && cd fulltextsearch_elasticsearch
git clone https://github.com/nextcloud/fulltextsearch_elasticsearch.git .
git checkout v1.5.2
curl https://patch-diff.githubusercontent.com/raw/nextcloud/fulltextsearch_elasticsearch/pull/100.patch | git apply -v
composer install
cd .. && chown -R www-data:www-data fulltextsearch_elasticsearch

Let me know if this helps.

EDIT: note that v1.5.2 should already contain the fix regarding ES7.7 support mentioned by @Jake-Etherflow so the above should work

[omidmeh]

This fixed my issue as well. Thank you!!

[omidmeh]

The only thing that I noticed is that when I search for something that belongs to a different user, although I don't actually see the result, it does show "the search in Files for 'search-term' returned 1 results in 56 ms".

That's still a huge improvement since the owner user sees the file but the non-owners don't. Would be nice if we could adjust that number as well though!

[R0Wi]

Could be related to https://github.com/R0Wi/fulltextsearch_elasticsearch/issues/1

[ravermeister]

sigh, I really hope this fix find the way to the master branch (and the next Version). otherwise the whole thing is nearly useless, even worse, It gives the User the wrong feeling about the content of his own data!.

[lhurt]

Why is it not merged to the main branch?

[Apfelwurm]

Hi, can please someone of the staff review this PR? It works perfectly and its a pain to implement this fix for every release manually. Thanks!

[trendzetter]

I think someone is waiting for your good consultancy fees.

[R0Wi]

Like mentioned in https://github.com/nextcloud/fulltextsearch_elasticsearch/pull/100#issuecomment-668095902 this PR does not respect the separation of concerns between the FTS apps (even it works...). Unfortunately currently i don't have much time to port my work onto files_fulltextsearch because i think this would be a bit more heavy lifting.

[tauceti82]

@omidmeh i think what you could do is cloning this repo, applying the patch from this PR and installing the apps dependencies manually (note you need to install git as well as composer). So basically you could do something like this:

cd <NEXTCLOUD_ROOT>/apps
rm -rf fulltextsearch_elasticsearch
mkdir fulltextsearch_elasticsearch && cd fulltextsearch_elasticsearch
git clone https://github.com/nextcloud/fulltextsearch_elasticsearch.git .
git checkout v1.5.2
curl https://patch-diff.githubusercontent.com/raw/nextcloud/fulltextsearch_elasticsearch/pull/100.patch | git apply -v
composer install
cd .. && chown -R www-data:www-data fulltextsearch_elasticsearch

Let me know if this helps.

EDIT: note that v1.5.2 should already contain the fix regarding ES7.7 support mentioned by @Jake-Etherflow so the above should work

the 100.patch was adapted so it couldn't patch the Makefile as the version+=1.5.2 was changed to 20.0.0 so it could not find the correct positions to patch...what helped was to manually change version+=1.5.2 to version+=20.0.0 before applying the patch...

And then do in the end occ upgrade otherwise it will disable the plugin.

it cost me hours to get the search working on external files! This has to be fixed asap!

[R0Wi]

@tauceti82 sorry for the circumstances i rebased this PR onto the current v21.0.0 so you should now be able to apply the patch again by git checkout v21.0.0 and then following the commands from above. Maybe i manage to build a regularly updated custom .tar file via Github Actions and link it here these days.