Skip to content
This repository has been archived by the owner on Aug 30, 2022. It is now read-only.

Allow conversation-independent search (#62) #98

Merged
merged 3 commits into from
Oct 2, 2017

Conversation

ja-fra
Copy link
Member

@ja-fra ja-fra commented Sep 26, 2017

Conversation-independent search is available under ${base.url}/rocket/${client.name}/search

It accepts a text query parameter for full-text search, but also allows passing in arbitrary SolrRequestParams.

connected to #62

@ghost ghost assigned ja-fra Sep 26, 2017
@ghost ghost added the in review label Sep 26, 2017
Copy link
Collaborator

@ruKurz ruKurz left a comment

Choose a reason for hiding this comment

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

I've added my comments. In general I would say: Straight forward, nothing special here.

*/
@ApiOperation(value = "search for a conversation", response = SearchResult.class,
notes = "besides simple text-queries, you can pass in arbitrary solr query parameter.")
@RequestMapping(value = "{clientId}/search", method = RequestMethod.GET)
Copy link
Collaborator

Choose a reason for hiding this comment

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

As far as I understand: You're extending the rocket-chat service endpoint for a {clientId}/search function/call. So it is ensured that I will only get results from that {clientId}? Is that right?

Copy link
Member Author

Choose a reason for hiding this comment

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

👍


@Service
public class ConversationSearchService {

Copy link
Collaborator

Choose a reason for hiding this comment

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

TOP! Having a conversation search service makes absolutely sense to me, since searching conversations within the knowledge base is a core feature of Smarti itself, so this should be part of the API.

public ResponseEntity<?> search(
@PathVariable(value = "clientId") String clientName,
@ApiParam("fulltext search") @RequestParam(value = "text", required = false) String text,
@ApiParam(hidden = true) @RequestParam MultiValueMap<String, String> queryParams) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

so I can use any of the supported search query parameters of Solr by simply appending these to the HTTP query string?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, but:

  • the required filters for access-controll (fq=owner:<clientId>) and type (fq=type:conversation) are always appended.
  • if a text-parameter is provided, any q-parameter passed in is replaced by q={!parent which="type:conversation"}text:<text>


public SearchResult<Conversation> search(Client client, MultiValueMap<String, String> queryParams) throws IOException {

final ModifiableSolrParams solrParams = new ModifiableSolrParams(toListOfStringArrays(queryParams, "text"));
Copy link
Collaborator

@ruKurz ruKurz Sep 26, 2017

Choose a reason for hiding this comment

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

There is not much semantic within the queryParams. I could imagine that at this point it would be great to have some more meaningful "ConversationQuery" object. Or in other words: If I should make a suggestion at wich point of code I would expect most changes in future, than I would point on this part, where the Solr query is being constructed.

My question: Should we spend some efforts on how to create the query? Or should we create an improvement issue for this? Or do you think, that just using HTTP query Strings and ModifiableSolrParams is sufficient?

https://github.com/redlink-gmbh/smarti/tree/develop/lib/query-conversation/src/main/java/io/redlink/smarti/query/conversation

Copy link
Member Author

Choose a reason for hiding this comment

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

I think we currently have good trade-off between simplicity and flexibility:

  • the text parameter allows simple full-text search on conversations
  • advanced queries can be executed by using the Solr-parameters.

If you identify additional features that should be exposed through explicit parameters (e.g. date-filters, participants, etc - just guessing) on the web-service level we can add those.

On the service level, I'd stay with the current api for now - we can add a method accepting a ConvesationSearchQuery (and a matching ConversationSearchQueryBuilder) when we need it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

@tkurz
Copy link
Member

tkurz commented Sep 27, 2017

When setting the rows parameter, the numFounds behave cumbersome. Actually the numFounds is always lower than or equals the row parameter. It seems like this is not the num founds but the cardinality of documents returned. @ja-fra should be an easy fix.

@ja-fra
Copy link
Member Author

ja-fra commented Sep 28, 2017

@tkurz: true, there was a bug with the responses, but in the implementation for #39. Here, everything should be fine.

@ja-fra ja-fra merged commit 49f08d7 into develop Oct 2, 2017
@ghost ghost removed the in review label Oct 2, 2017
@ja-fra ja-fra deleted the issue-62_global-search branch October 3, 2017 09:40
@ruKurz
Copy link
Collaborator

ruKurz commented Nov 3, 2017

After installing 0.6.0 do we have to trigger an indexing process to get the search work.
When executing a search like: http://localhost:8080/rocket/team/search?q=*:*
I only get 3 results (Smarti has stored > 100 conversations for the client team).
Note the first result is null

bildschirmfoto 2017-11-03 um 20 54 34

@ja-fra
Copy link
Member Author

ja-fra commented Nov 3, 2017

No triggering is required for the global search to work. Please note that only completed conversations are considered in search.

The null-result must not happen, we'll have a look at this (#133).

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

Successfully merging this pull request may close these issues.

4 participants