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

[NEW] Global search #7628

Closed
wants to merge 7 commits into from
Closed

[NEW] Global search #7628

wants to merge 7 commits into from

Conversation

savikko
Copy link
Contributor

@savikko savikko commented Aug 1, 2017

Continuing @cyberhck work:

  • added checkbox for user to enable global search (default: enabled)
  • goes to correct message when clicked
  • shows channel name on search result

@RocketChat/core

Closes #1615

@eloo
Copy link

eloo commented Aug 18, 2017

i've tested your feature quickly and it looks really promising.
looking forward to get this merged ;)

@JSzaszvari
Copy link
Contributor

@geekgonecrazy any chance someone might be able to review this one? This is a super awesome feature.

@savikko
Copy link
Contributor Author

savikko commented Sep 13, 2017

One thing which i think would be nice: now when you get the results you get action menu (cog icon) on every message. Still, only action is "go to message". I'd like to see that only possible action to be clickable directly from message without opening the action menu.

Still, I have no clue what would be the right way to implement that.

}
} else {
query.rid = {
$in : RocketChat.models.Rooms.findByContainingUsername(currentUserName)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This rely's on the username on the room object. I'm not sure if we plan to keep here.

I do worry about how heavy this could be...

Our community server for example we have 145k users. I can only imagine this plus then querying across all rooms in... could be very heavy.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about if global search would be admin-enableable feature?
I see that on large installations this needs highly more optimized search function but on smaller environments current implementation could be acceptable.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@RocketChat/core what do you guys think? Should we have this as something that can be turned on?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not in a position to test this feature, nor do I understand what this bit of code does. Is the proposed new search always doing a global search? Perhaps defaulting to searching the current channel, and requiring the user to select a global search option would result in very few global searches being done, ie only when necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, in large environments global search could be used as DoS-attack by normal users.
Therefore allowing user to enable global search while searching should be only secondary, admin should decide this primarily.

On smaller environments (depends on multiple factors I think) your point still stands.

If time permits, I'll add option to admin.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That works fine for us, our environment is small, and we will enable the option. I would hate to be in a big environment and denied access to global search just because it might be abused.

Compromises could include allowing users to specify up to, say, 5 channels to include in their search, or a search string syntax that can specify up to 5 channels to search. Eg "channel:general,random".

Or, as I suggested earlier, an option to repeat the current search on the next channel in the list.

@geekgonecrazy
Copy link
Member

@RocketChat/core Thoughts on this PR?

@johnyb0y
Copy link

Would also Love to See this Feature merged. Possible Solution to DoS-Attacks would
Be to introduce a global maximum number of queries per timeframe or simultanious queries, which can be edited by admin.

@mrsimpson
Copy link
Collaborator

@geekgonecrazy we're going to implement an asynchronously executed search on Rocket.Chat content based on Solr. Apache licensed, of course. Interested?

@geekgonecrazy
Copy link
Member

@mrsimpson i'd love to chat about it.

I think the biggest concern for all of us is that this can be too heavy. We have enough in our core that is CPU intensive. We add this that will hammer both server and mongo.. We then introduce a lot more performance problems we have to deal with.

External is ideal for larger multi-instance install. I'm being a bit presumptuous speaking for the team 😁 but I think ideally there would be a balance. Small installs nothing extra needed. Big installs, a little extra setup I think is to be expected.

So i'd love to talk about why solr and how that would work.

@cyberhck
Copy link
Contributor

cc @mrsimpson @geekgonecrazy I agree that it might be too heavy and might be a maintenance problem for a lot of people.

I do have a suggestion though, create the search separately on a completely different micro-service. Now to enable the Solr search can be up to user, if they want, well and good, else they won't be able to search. Now to make it less painful for users, you can always publish docker images and all they would do on production server would be to run the image with correct volume mapping for data backup

thoughts?

@geekgonecrazy
Copy link
Member

Please see RocketChat/feature-requests#745 trying to make it so additional search providers can be implemented.

@robguinness
Copy link

This discussion raises an important issue: As Rocket.Chat grows and the needs diversify, there should be a set of core features, as well as a set of advanced features implemented by packages/plug-ins. This is the route that many open-source big projects have taken (e.g. Redmine). I would argue that any of the existing features (some of which are buggy) should actually moved out of the core and into optional packages.

In the case of global search, for large installations, an implementation with Solr or something similar would make sense but for smaller installations, it is better to have this implemented via a "one-click" plug-in to the main application.

@JSzaszvari
Copy link
Contributor

@robguinness Work in progress - #6890

@robguinness
Copy link

@JSzaszvari great to see that!

@MarcoRudolph
Copy link

In our case this does not work. If checked or unchecked the result
is bound to the actual channel.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Global search across channels