Workitem search #395

sbose78 · 2016-10-25T07:04:26Z

compliance* Add search capabilities to workitems title,description and ID field(s

Used postresql full text search capabilities.
Supported search mechanisms:

For each search string
"ID:12345" --> Search by ID 12345 only
"12345" ---> Search by ID,title,description
"The cost of the device is 12345" --> Search by ID,title,description

URLs can be searched as -
"http://demo.almighty.io/#/detail/212" ----> Understand that it's a URL , search full text by ID as startsWith(212) and the string startsWith("http://demo.almighty.io/#/detail/212")
So effectively the sql parameter for searching this is :
"212* & http://demo.almighty.io/#/detail/212*"

Eventually extend support for scenarios for work item linking where the workitem content has
"Hi this has been done in http://demo.almighty.io/#/detail/212"
The code should parse out the URL and link the workitem with workitem 212.

Pagination (Baiju)
REST API endpoint (Pranav)
search string parser (Pranav)

codecov-io · 2016-10-25T08:44:55Z

Current coverage is 65.42% (diff: 71.62%)

Merging #395 into master will increase coverage by 0.56%

@@             master       #395   diff @@
==========================================
  Files            44         46     +2   
  Lines          1986       2195   +209   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           1288       1436   +148   
- Misses          569        609    +40   
- Partials        129        150    +21

Powered by Codecov. Last update 4c753cb...df47c64

aslakknutsen · 2016-10-25T10:59:02Z

search/search_repository.go

+	return q, nil
+}
+
+func convertFromModel(wiType models.WorkItemType, workItem models.WorkItem) (*app.WorkItem, error) {


Some reuse of function between WorkItemRepository and SearchReposiotry would be useful

Yeah, I think so too but the method is unexported in models. That's why I copied it over temporarily but unsure about the permanent solution.

Could you please suggest a way to re-use the same method instead of copying it over?

Maybe just export them? Possible move them out of the workitem repository

Thanks @aslakknutsen
I will start with exporting it - opening a separate PR for that.

Opened PR for the same: https://github.com/almighty/almighty-core/pull/406/files

aslakknutsen · 2016-10-25T10:59:28Z

search/search_repository.go

+	return &result, nil
+}
+
+func (r *GormSearchRepository) loadTypeFromDB(ctx context.Context, name string) (*models.WorkItemType, error) {


Some reuse of function between WorkItemRepository and SearchReposiotry would be useful

Why are we not adding this code to the WorkItemRepository? It is tightly coupled to the structure of the work items.

We plan to support search for other objects as well, eventually, Users, etc.
But for now we shall support only workitems.

Update: Misunderstood your comment, will do what you suggsted in a separate PR now.

@tsmaeder Did you mean that we add loadTypeFromDb to work item repository as an exported method - I wasn't quite able to figure out the best approach, could you please help?

My comment was aimed at why we're not adding search functionality to the work item repository instead of implementing it here. That is a different discussion.
Since we are accessing the database directly in this package anyway, it doesn't really make a difference: there is not much smarts in the loadTypes... function anyway.

aslakknutsen · 2016-10-25T11:02:49Z

search/search_repository.go

+}
+
+// Search returns work items for the given query
+func (r *GormSearchRepository) SearchFullText(ctx context.Context, sqlQueryString string, sqlQuerytParameter string) ([]*app.WorkItem, error) {


A bit stuttering on the naming here:

search.GormSearchRepository.SearchFullText

aslakknutsen · 2016-10-25T11:03:53Z

search/search_repository.go

+)
+
+const (
+	fulltextPsqlQuery = `select * from work_items WHERE to_tsvector('english', id::text || ' ' || fields::text) @@ to_tsquery($1)`


the to_tsvector should be indexed?

Yes.
Something along these lines:
https://www.postgresql.org/docs/9.6/static/textsearch-tables.html#TEXTSEARCH-TABLES-INDEX

aslakknutsen · 2016-10-26T11:31:16Z

search/search_repository.go

+	// This SQL query is used when search is performed across workitem fields and workitem ID
+	testText = `select * from work_items WHERE
+		setweight(to_tsvector('english', id::text), 'A') ||
+		setweight(to_tsvector('english', fields::text), 'B') @@ to_tsquery('english',$1) and deleted_at is NULL`


Using fields::text allows me to search for 'field names' in the JSON as well. e.g. "system.title". Not sure if that is desirable.

'2':1 'id':15 'item':14 'new':3 'null':10,12,16 'system.assignee':9 'system.creator':7 'system.description':11 'system.remote':13 'system.state':2 'system.title':4 'test':5 'tmaeder':8 'wi':6

Yes @aslakknutsen I will be updating the query to search in "system.description" and "system.title" only.

Fixed in :
7a9501a

sbose78 · 2016-10-26T11:34:05Z

As per discussion,

update all current capabilities with "Starts-with" search.
URLs can be searched as :
"http://demo.almighty.io/#/detail/212" ----> Understand that it's a URL , search full text by ID as startsWith(212) and the string startsWith("http://demo.almighty.io/#/detail/212")
So effectively the sql parameter for searching this is :
"212* & http://demo.almighty.io/#/detail/212*"
"i am looking for ID:42515" --->
The sql query parameter is "I am looking for 42515" ---> this looks at both text and ID for 42515, instead of ID only. That's a known issue for now, to be addressed later.

pranavgore09 · 2016-10-27T05:15:38Z

@sbose78 thanks for writing it up here. I am taking up URL task.

DhritiShikhar · 2016-10-27T06:54:28Z

design/resources.go

+	Action("show", func() {
+		Routing(
+			GET("/:q"),
+		)


Description of the "show" action is missing.

thanks @DhritiShikhar for pointing out. done in 6ed119d

sbose78 · 2016-10-27T14:48:52Z

search/search_repository.go

+}
+
+// RegexWorkItemDetailClientURL tells us how URL for WI details on front end looks like
+const RegexWorkItemDetailClientURL = `^(?P<protocol>http[s]?)://(?P<domain>demo.almighty.io)(?P<path>/#/detail/)(?P<id>\d*)`


@pranavgore09
Could you please move the const to the top of the file?

@sbose78 sure, I am making changes: This and other pieces of code are still in progress, and in upcoming commit I will make these changes.
thanks for pointing out things.

sbose78 · 2016-10-27T14:52:01Z

search/search_repository.go

+
+//mapURLGroupWithValues accepts slice of group names and slice of values.
+//If both slices have different lenghts, empty value will be put for group name.
+func mapURLGroupWithValues(groupNames []string, stringToMatch string) map[string]string {


It is very unclear what this method is doing @pranavgore09 .
Could you please add some code comments with an example what we are trying to accomplish here?
URL groups , values, etc need some explanation :)

sbose78 · 2016-10-27T14:52:59Z

search/search_repository.go

+var compiledWIURL = regexp.MustCompile(RegexWorkItemDetailClientURL)
+
+// First index is ignored because of behaviour of "SubexpNames"
+var groupsWIURL = compiledWIURL.SubexpNames()[:1]


rename to groupsWorkItemUrl.
also add an example what is happening here.

sbose78 · 2016-10-27T14:53:23Z

search/search_repository.go

+const RegexWorkItemDetailClientURL = `^(?P<protocol>http[s]?)://(?P<domain>demo.almighty.io)(?P<path>/#/detail/)(?P<id>\d*)`
+
+// Use following compiled form in future use
+var compiledWIURL = regexp.MustCompile(RegexWorkItemDetailClientURL)


rename to compiledWorkItemURL.
also add an example what is happening here.

pranavgore09 · 2016-10-28T03:06:06Z

@aslakknutsen @sbose78
When user input is http://demo.almighty.io/#/detail/212 for search, we convert it into following search string 212:* & demo.almighty.io/#/detail/212:*

Now, the problem with this behaviour is, as a user when I enter that URL, I expect at least that single object with id 212. But considering our query string, it performs & operation and does not pick up expected records.

This is the case we need OR in the query string instead of AND.

sbose78 · 2016-10-28T03:42:25Z

Hi PranaV
Yes you are right, this is an OR operation.

Convert the URL into
( Url | id ) and then append it to the search query parameter string.

Psql recognizes OR operations written in the form ( url | id )

Shoubhik

On Oct 28, 2016 8:36 AM, "Pranav Gore" notifications@github.com wrote:

@aslakknutsen https://github.com/aslakknutsen @sbose78
https://github.com/sbose78
When user input is http://demo.almighty.io/#/detail/212 for search, we
convert it into following search string 212:* &
demo.almighty.io/#/detail/212:*

Now, the problem with this behaviour is, as a user when I enter that URL,
I expect at least that single object with id 212. But considering our query
string, it performs & operation and does not pick up expected records.

This is the case we need OR in the query string instead of AND.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#395 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAhSAIzddvlUcvDNt5frY1ELC4EgTLkNks5q4WaegaJpZM4KfoQG
.

tsmaeder · 2016-10-26T11:36:22Z

design/resources.go

@@ -355,3 +355,25 @@ var _ = Resource("trackerquery", func() {
 	})

 })
+
+var _ = Resource("search", func() {
+	BasePath("/search")


Since we are only searching for work items here, should we reflect that in the path? like /search/workitems or /workitems/search?

Technically search should be able to return 'all type' of objects. I think there are 2 different ways to filter the type, by ?type=workitem|user|project|code and as part of the query it self.

In the UI for the specific use case of linking, the UI would know it can only link workitems(?) so it can auto add that filter.

But in a more general search bar, the user can filter it via e.g. "type:workitem search term a b"

yes @aslakknutsen , type will be another query param (not mandatory).
In current case if not provided we will consider type=workitem.

This is helpful for searching user while assigning tasks to users as well.

tsmaeder · 2016-10-26T11:36:48Z

design/resources.go

+			Param("q", String, "Search Query")
+		})
+		Response(OK, func() {
+			Media(CollectionOf(workItem))


What about paging? I'm fine to say "add it later" if the task gets too big.

Yes @tsmaeder , @baijum is working on adding paging.

tsmaeder · 2016-10-26T11:40:21Z

search.go

+	return transaction.Do(c.ts, func() error {
+		t, err := c.sRepository.SearchFullText(ctx.Context, ctx.Q)
+		if err != nil {
+			return err


Generally, we should handle errors by calling stuff like ctx.BadParameter() instead of just returning the error.

tsmaeder · 2016-10-26T11:45:20Z

search/search_repository.go

+	return &result, nil
+}
+
+func (r *GormSearchRepository) loadTypeFromDB(ctx context.Context, name string) (*models.WorkItemType, error) {


Why are we not adding this code to the WorkItemRepository? It is tightly coupled to the structure of the work items.

tsmaeder · 2016-10-26T11:52:56Z

search/search_repository.go

+		// Is "id:45453 id:43234" be valid ? NO, because the no row can have 2 IDs.
+		searchQuery = testID
+	}
+	return searchQuery, idStr + wordStr


Shoudn't idStr and wordStr be separated by something here?

An example string that would be passed to the SQL query for search would be :
"2342 & issue & with & network"
The search for all the tokens above will be done on

ID ,

fields->>system.description ,

fields->>system.title.

So if 2342 is an ID as well, then that specific row would be considered as well.

But you're doing "idStr + wordStr". I doubt that is correct.

Thanks @tsmaeder I get what you mean, let me check this.

I made a commit to address this @tsmaeder .
@pranavgore09 : could you please review

Commit : f86839f

@tsmaeder thanks for pointing out. ! I missed that.
@sbose78 checked, that was needed thanks.

tsmaeder · 2016-10-26T11:57:45Z

search/search_repository.go

+	parts := strings.Split(rawSearchString, " ")
+	var res searchKeyword
+	for _, part := range parts {
+		if strings.HasPrefix(part, "id:") {


How does the front end know when to put "id:" in front of a word the user enters in the "context assist"? Can't we make this decision here in the back end?

How does the front end know when to put "id:" in front of a word the user enters in the "context assist"? Can't we make this decision here in the back end?

Good question, @tsmaeder ,
The UX for using this is yet to be defined, but one way in which the UI might be putting an "id:" explicitly is when the user types something like #SOME_NUMBER ( like we do here in GH ). In that case, the UI would make an Ajax call with "ID:SOME_NUMBER " asking the API to do a search-by-id explicitly. If the "ID" was not specified by the UI during the API call, the API would have searched by both ID and text containing SOME_NUMBER.

Why are we not adding this code to the WorkItemRepository? It is tightly coupled to the structure of the work items.

Hi @tsmaeder , I didn't find the 'comment' box under your comment and therefore I'm replying here for your comment on #395 (diff)

Are you suggesting I make the loadTypeFromDB an exported method of WorkItemRepository ? I wish we could do something similar..

baijum · 2016-10-29T13:47:43Z

migration/sql-files/005-add-search-index.sql

@@ -0,0 +1,2 @@
+CREATE INDEX IF NOT EXISTS fields_title ON work_items USING GIN (to_tsvector('english', fields->>'system.title'));


Do we need condition checking as we are using a migration tool?

I prefer writing re-runnable sql queries - just doing so as a good practice :)

I have created #422 for adding general indexing for fields

baijum · 2016-11-02T11:04:22Z

[test]

tsmaeder · 2016-11-02T14:59:08Z

design/media_types.go

-	Attribute("first", String)
-	Attribute("last", String)
+var searchPagingLinks = a.Type("searchPagingLinks", func() {
+	a.Attribute("prev", d.String)


As a follow-up on the PR @kwk did: I think we should standardize on how to do the import renames: I would propose 3-letter lowercase, like "dsl" or "api"

@tsmaeder yes I would agree with you on this. dsl and api much better than a and d. Still I am not very ok with the name api as it is too generic (I don't have any suggestions either). dsl sounds perfect to use.

pranavgore09 · 2016-11-03T04:51:50Z

search/search_repository.go

+	*/
+
+	// This SQL query is used when search is performed across workitem fields and workitem ID
+	WhereClauseForSearchByText = `setweight(to_tsvector('english',fields->>'system.title'),'B')||


@sbose78 : IMO, we shold somehow mention that this where clause is only for workitem relation ?
We can maintain a constants in a table like -

search by ID full text search query

work_item WHERE_CLAUSE_WI_BY_ID WHERE_CLAUSE_WI_BY_FULL_TEXT

users WHERE_CLAUSE_USER_BY_ID WHERE_CLAUSE_USER_BY_FULL_TEXT

Because soon we will have type in query param that will decide which table to query on.
We can take union of multiple query results in the DB itself.

Very good observation @pranavgore09 ,
I agree with you that we need to have something like this, but not sure if we should be doing so right now because we are unsure about the implementation at a sql-query level about how to search multiple types. We need to figure out if we could go the single-query path , or multiple-query path.

Tracking it as part of #430

tsmaeder · 2016-11-03T09:40:15Z

search.go

+		}
+
+		// prev link
+		if offset > 0 && count > 0 {


Now that pagination for work items is in master, let's use the same code in both places.

@tsmaeder is there any function that I can call from here? or copy the same code to here?

No, we would have to factor that code out first.

Hi @baijum , @tsmaeder ,
It would be nice to have it factored out and modularize the code for the same instead of repetitive code. Could we fix it as part of a separate PR , merged to master , and then changes taken into this ( and other PRs ) ?

tracking this here #431

tsmaeder · 2016-11-03T09:52:45Z

search/search_repository.go

+}
+
+// Validate ensures that the search string is valid and also ensures its not an injection attack.
+func (r *GormSearchRepository) Validate(ctx context.Context, rawSearchString string) error {


How is this going to be used? Our protection against sql injection should not rely on detecting certain classes of strings. We're going to miss one eventually.

Hi @tsmaeder ,
I'm still thinking about how to validate the search string. But the thought process behind this is that there should be some validation of the string before we pass it on to be searched. As you mentioned, injection needn't be only of a specific grammar. Could you help with some ideas/implementation around this?

Generally, injection is not a problem if you always treat user input as values, not part of the query string. In general, this means don't manipulate the user input, pass it as parameters to the query. I'm not sure what we're doing when constructing the query parameter is safe. If we used multiple parameters instead of one, i would be confident that we are injection-safe.
I really don't think we should validate the input string. For a malicious client, it won't make a difference (it simply will not call validate()) and for a normal client, what is the use case? Does Google validate your search?

Thanks @tsmaeder ,
Your argument sounds valid to me. I'll remove the Validate(..).
db := r.db.Model(&models.WorkItem{}).Where(sqlSearchQuery, sqlSearchQueryParameter)

Should be taking care of query injection issues.

tsmaeder · 2016-11-03T10:22:42Z

I don't like that fact that we have a service that takes a very unstructured, highly application-specific search string and directly uses it to query the database. I think it would be better if we had a layer where we have clearly defined search operators ("full text search for $1 on field X", "date < 1.1. 2017") and a separate service that turns the "unstructured" queries into structured ones. This will allow us to reuse the search facilities we are building now.
I can live with what we have here at the moment, but I think we should refactor it sooner rather than later.

Updated respective tests. Added t.Parallel() to all unit tests.

test-all working fine on local system but seems to be breaking on jenkins. Trying to figure out the difference.

Did not find simple and better way in goadesign for adding usage example to GET request with query params, hence added as a multiline string in parameter value. Using multiple comments instead of single line comment multiple times.

Added test for empty q value result.

Test cases updated. tested with http://localhost:8080/api/search?q=%22http://demo.almighty.io/detail/1%22&page[limit]=2 query on local DB.

Usefull while testing locally. Added tests for the same.

Update wir.create() with creator string value.

* Use ts_vector column on work_items updated via triggers * Rank results

aslakknutsen · 2016-11-08T01:25:52Z

ugh, damn it! Forgot to add "Contributed by xxx" to the merge commit. Sorry!

God job, all! 👍

aslakknutsen reviewed Oct 25, 2016

View reviewed changes

pranavgore09 mentioned this pull request Oct 25, 2016

Support search on ID #381

Closed

sbose78 force-pushed the workitem_search branch 2 times, most recently from ca14d3d to 5985c65 Compare October 26, 2016 06:27

aslakknutsen reviewed Oct 26, 2016

View reviewed changes

DhritiShikhar reviewed Oct 27, 2016

View reviewed changes

sbose78 commented Oct 27, 2016

View reviewed changes

sbose78 force-pushed the workitem_search branch from 6ed119d to 643aeac Compare October 27, 2016 15:01

tsmaeder suggested changes Oct 28, 2016

View reviewed changes

baijum force-pushed the workitem_search branch 2 times, most recently from 3fac5fe to 5765868 Compare October 29, 2016 10:28

baijum reviewed Oct 29, 2016

View reviewed changes

sbose78 force-pushed the workitem_search branch 2 times, most recently from 477527e to d1dbca4 Compare November 2, 2016 10:54

baijum changed the title ~~WIP : Workitem search~~ Workitem search Nov 2, 2016

tsmaeder reviewed Nov 2, 2016

View reviewed changes

pranavgore09 reviewed Nov 3, 2016

View reviewed changes

sbose78 force-pushed the workitem_search branch from ee2c5ab to 13fb7f1 Compare November 3, 2016 08:08

tsmaeder reviewed Nov 3, 2016

View reviewed changes

sbose78 force-pushed the workitem_search branch from 50e7dbe to ed97d6c Compare November 3, 2016 10:50

This was referenced Nov 3, 2016

Expose ConversionError type using exported method #423

Merged

Design SQL for expanding support for searching mulitple types ( users, Workitems ) #430

Open

pranavgore09 and others added 25 commits November 8, 2016 01:33

Priority to OR operation in the search query, added paranthesis.

43e0edf

Updated respective tests. Added t.Parallel() to all unit tests.

Put KnownURLs map behing the lok.

099a5b3

basic API test

26ce929

Trying to fix tests with t.Parallel()

ab3e151

test-all working fine on local system but seems to be breaking on jenkins. Trying to figure out the difference.

Add support for handling null fields ( coalesce )

ff6f250

pagination test

02f5973

add test case for id only

cfdb99c

Remove golint warnings.

cad81ac

Fix variable name of sql where clause for ID search

62dce5c

Added example for search query string.

074a1df

Did not find simple and better way in goadesign for adding usage example to GET request with query params, hence added as a multiline string in parameter value. Using multiple comments instead of single line comment multiple times.

use the common media types

cbdf45d

use & as separator

969187b

use int type

01898e4

use absolute URLs in links

5611696

reuse WIT load function

2498367

Make Q query param a Required param for searh API.

274dfd9

Added test for empty q value result.

use integer for count

f2dd70c

update index for multiple column lookup

8ab2aab

use string for offset

9b0ff06

Value of q should be trimmed before parsing.

34afcc8

Test cases updated. tested with http://localhost:8080/api/search?q=%22http://demo.almighty.io/detail/1%22&page[limit]=2 query on local DB.

Removed unused 'Validate'

1d65ca4

Added facility to add localhost:8080~ urls.

dc3782e

Usefull while testing locally. Added tests for the same.

update tests as using ServiceAsUser.

963cafc

Update wir.create() with creator string value.

remove copy of errors.go and use exported method

a209b8f

Rework Full Text Search

e65d544

* Use ts_vector column on work_items updated via triggers * Rank results

aslakknutsen force-pushed the workitem_search branch from df47c64 to e65d544 Compare November 8, 2016 00:56

aslakknutsen approved these changes Nov 8, 2016

View reviewed changes

aslakknutsen merged commit 22be2a3 into master Nov 8, 2016

baijum deleted the workitem_search branch November 9, 2016 10:12

		@@ -0,0 +1,2 @@
		CREATE INDEX IF NOT EXISTS fields_title ON work_items USING GIN (to_tsvector('english', fields->>'system.title'));

	search by ID	full text search query
work_item	WHERE_CLAUSE_WI_BY_ID	WHERE_CLAUSE_WI_BY_FULL_TEXT
users	WHERE_CLAUSE_USER_BY_ID	WHERE_CLAUSE_USER_BY_FULL_TEXT

Workitem search #395

Workitem search #395

Conversation

sbose78 commented Oct 25, 2016 • edited by baijum

codecov-io commented Oct 25, 2016 • edited

Current coverage is 65.42% (diff: 71.62%)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbose78 Oct 28, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbose78 commented Oct 26, 2016

pranavgore09 commented Oct 27, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pranavgore09 commented Oct 28, 2016

sbose78 commented Oct 28, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

baijum commented Nov 2, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbose78 Nov 3, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tsmaeder Nov 3, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tsmaeder commented Nov 3, 2016

aslakknutsen commented Nov 8, 2016

sbose78 commented Oct 25, 2016 •

edited by baijum

codecov-io commented Oct 25, 2016 •

edited

sbose78 Oct 28, 2016 •

edited

sbose78 Nov 3, 2016 •

edited

tsmaeder Nov 3, 2016 •

edited