-
Notifications
You must be signed in to change notification settings - Fork 10.7k
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] Search Provider Framework #10110
[NEW] Search Provider Framework #10110
Conversation
# Conflicts: # .meteor/packages # package-lock.json
Hi all! |
I am so much looking forward to using this on open.rocket.chat - imho the most important contribution for months! |
…dlink-gmbh/Rocket.Chat into redlink-local/#8381-search_provider_concept
I just did a minor extension of the suggestion functionality to overcome an issue with specific filters (e.g. for messages only) within search providers. @karlprieb this should not influence the positive review from your side right? |
@tkurz can you add a recent screenshot - we need more teasers. |
Yes, if you don't mind adding screenshots that we can use for our release blog post that would be awesome! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
found a few more things..
everything else looks good.
if you can fix them we can still merge tomorrow.
SearchLogger.debug(`user ${ uid } can access ${ msg.rid } ( ${ subscription.t === 'd' ? subscription.username : subscription.name } )`); | ||
} else { | ||
SearchLogger.debug(`user ${ uid } can NOT access ${ msg.rid }`); | ||
}console.log(msg); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oops.. found a console.log
@@ -226,7 +226,7 @@ Meteor.methods({ | |||
}; | |||
} | |||
|
|||
result.messages = RocketChat.models.Messages.find(query, options).fetch(); | |||
result.message.docs = RocketChat.models.Messages.find(query, options).fetch(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any reason to change the result schema? I'm afraid this could break some clients. if there is no strong reason for this change, I would ask to stick with the old schema.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The result schema has been changed because we wanted to allow some metadata to the search results (e.g. number of matching documents, scoring, etc) In order to keep one schema (necessary e.g. for result validation) for all providers, we changed it also here. Nevertheless, I changed this in the code in api, which uses his function, too . If you want to change there we can have the solutions: add a parameter format to the function, that is default basic but can be set to extended
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tkurz what about adding another property to the result which is exclusively dedicated to the metadata?
result: {
messages: [...],
meta: {
messages: {
totalCount: 17177882,
pagesize: 50,
currentPage: 37
},
user: {
language: 'prBR'
}
}
I don't think it's a good style to change the schema of a response, but having optional parts for which a projection might be specified by the consumer is fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, would be a valid solution, but enforces some major changes in the current Chatpal implementation. What about this: Keep the "old" schema in the messageSearch Service and map it in the search provider. so we do not change the current api. IMHO the new schema is more adequate (and common) on search index systems like solr/elastic. Would be easy and would not contain api changes (and this be downwards compatible to 3rd party)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As you said, the new Schema us common for search providers. Since it’s the implementation of the „search“ service, I believe it’s valid to go for .docs - as you also changed the old global search implementation and the api to match, I don’t see a constraint to use this new schema. It was a beta in the current version explicitly anyway. But I think you should discuss this with @diegosampaio (probably on open.rocket.chat) during the course of today. 🇧🇷 ist close to waking up now anyway ;-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rafaelks do the mobile apps utilize the real time method or the endpoint for searching? Or do they not search messages yet?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@graywolf336 We're implementing using this API: https://rocket.chat/docs/developer-guides/rest-api/chat/search/.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I will take a look!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@graywolf336 the rest endpoint uses still current search service (mongo based). And it returns the same result format as before. So schema change should not influence the app right? In a next version we can change the method (and maybe the api) so it supports both, basic and extended schema version. Then it would also be possible to use the current provider for API as well. But for now it should be fine right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yup.. seems we're good with mobile apps.. I'll still add a [BREAK]
flag for third parties who still rely on DDP method.
looking forward to the next version with metadata on REST endpoints 😃
@@ -25,7 +25,7 @@ Meteor.methods({ | |||
if (!Meteor.call('canAccessRoom', rid, currentUserId)) { | |||
return result; | |||
} | |||
} else if (RocketChat.settings.get('Message_GlobalSearch') !== true) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks like we don't need the old setting any more right? if so, I would ask to remove Message_GlobalSearch
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have to check this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mean that is should be removed from the config?
this.add('Message_GlobalSearch', false, { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes. a migration will also be needed to reflect the current value of Message_GlobalSearch
into the new Search.defaultProvider.GlobalSearchEnabled
setting.
additionally just found that old templates are also not being used anymore, so we would need to also delete the files:
and remove its references at:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fully agree: @sampaiodiego Is there any standard way for setting migration or do we need to do it programatically (in our code).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the migration is simple.. take a look at migration 112 (you'll need to create the version number 113) .. you just need to find
the old setting, grab its value and run an upsert
(the settings might have been created yet, so an upsert
is more recommended for this case) on the new setting with the old setting's value. after that you can remove
the old setting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool, i will commit this as soon it is ready.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sampaiodiego I added the migration. Can you please check? Should do the trick right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you forgot to remove the setting :) I just did it
👍
…dlink-gmbh/Rocket.Chat into redlink-local/#8381-search_provider_concept
My team has been complaining that this feature isn't enabled in our self hosted rocket chat, making us impossible to fully ditch slack. I know its 6 years old, but is there anyone out there who still uses global search - can anyone recommend how to set it up, at this point we'd even contact the task. |
Actually it was quite easy, see https://github.com/chatpal and especially https://github.com/chatpal/chatpal-search-standalone to run a search instance. But as the project is retired I cannot guarantee it works. It is Open Source so feel free to make PRs if necessary!! |
@RocketChat/core , @mrsimpson , @geekgonecrazy , @engelgabriel, @rodrigok
Relates RocketChat/feature-requests#745, #9651, #8714, #1615
Rocket.Chat Search Providers
In this pull request I introdude a provider concept for chatpal search. Beside the providerService
itself it contains 2 reference implementations:
In addition to the 2 packages it adds some i18n variables and does smaller adaptions to the result model of the messageSearch function.
Providers
A new Provider just extends the provider class and registers itself in the SearchProviderService.
Settings
In order to enable Settings within the admin UI for your own provider, you can add it (e.g. in the constructor).
The setting values are loaded, when you use your provider. The values can be easily accessed.
The settings can be set in Search section in the admin ui:
Search UI
Search provider can have their own result template. The template is loaded with data.
The default search ui looks like the existing ones:
The chatpal UI supports also rooms and users and looks like this:
Search Result
In order to provide a proper validation of the results the search function of the provider must follow at the following (extendable) format:
I am happily looking forward to your comments and hope you like my approach!