From 8d662105d3049a5cd67a7bf42ab70d479d4348e2 Mon Sep 17 00:00:00 2001 From: George Goldberg Date: Wed, 27 Sep 2017 18:44:22 +0100 Subject: PLT-7207: Change from fulltext to LIKE search for user filtering (#7343) * PLT-7202: Switch user search to LIKE queries to avoid fulltext pitfalls. * Add 2 char name unit test. * Escape underscores properly. * Add more tests and fix * handling. * Make search/indexes case insensitive for postgres. --- store/sqlstore/channel_store.go | 2 +- store/sqlstore/user_store.go | 95 ++++++++++++++------------------------- store/sqlstore/user_store_test.go | 84 ++++++++++++++++++++++++++++++++++ 3 files changed, 119 insertions(+), 62 deletions(-) (limited to 'store') diff --git a/store/sqlstore/channel_store.go b/store/sqlstore/channel_store.go index 06cca9e88..ed001f2d2 100644 --- a/store/sqlstore/channel_store.go +++ b/store/sqlstore/channel_store.go @@ -1583,7 +1583,7 @@ func (s SqlChannelStore) performSearch(searchQuery string, term string, paramete result := store.StoreResult{} // these chars have special meaning and can be treated as spaces - for _, c := range specialUserSearchChar { + for _, c := range ignoreUserSearchChar { term = strings.Replace(term, c, " ", -1) } diff --git a/store/sqlstore/user_store.go b/store/sqlstore/user_store.go index 4f4380d79..abc2d2d28 100644 --- a/store/sqlstore/user_store.go +++ b/store/sqlstore/user_store.go @@ -78,6 +78,14 @@ func (us SqlUserStore) CreateIndexesIfNotExists() { us.CreateIndexIfNotExists("idx_users_create_at", "Users", "CreateAt") us.CreateIndexIfNotExists("idx_users_delete_at", "Users", "DeleteAt") + if *utils.Cfg.SqlSettings.DriverName == model.DATABASE_DRIVER_POSTGRES { + us.CreateIndexIfNotExists("idx_users_email_lower", "Users", "lower(Email)") + us.CreateIndexIfNotExists("idx_users_username_lower", "Users", "lower(Username)") + us.CreateIndexIfNotExists("idx_users_nickname_lower", "Users", "lower(Nickname)") + us.CreateIndexIfNotExists("idx_users_firstname_lower", "Users", "lower(FirstName)") + us.CreateIndexIfNotExists("idx_users_lastname_lower", "Users", "lower(LastName)") + } + us.CreateFullTextIndexIfNotExists("idx_users_all_txt", "Users", USER_SEARCH_TYPE_ALL) us.CreateFullTextIndexIfNotExists("idx_users_all_no_full_name_txt", "Users", USER_SEARCH_TYPE_ALL_NO_FULL_NAME) us.CreateFullTextIndexIfNotExists("idx_users_names_txt", "Users", USER_SEARCH_TYPE_NAMES) @@ -1398,46 +1406,26 @@ func (us SqlUserStore) SearchInChannel(channelId string, term string, options ma return storeChannel } -var specialUserSearchChar = []string{ - "<", - ">", - "+", - "-", - "(", - ")", - "~", - ":", - "*", - "\"", - "!", - "@", +var escapeUserSearchChar = []string{ + "%", + "_", } -var postgresSearchChar = []string{ - "(", - ")", - ":", - "!", +var ignoreUserSearchChar = []string{ + "*", } func (us SqlUserStore) performSearch(searchQuery string, term string, options map[string]bool, parameters map[string]interface{}) store.StoreResult { result := store.StoreResult{} - // Special handling for emails - originalTerm := term - postgresUseOriginalTerm := false - if strings.Contains(term, "@") && strings.Contains(term, ".") { - if *utils.Cfg.SqlSettings.DriverName == model.DATABASE_DRIVER_POSTGRES { - postgresUseOriginalTerm = true - } else if *utils.Cfg.SqlSettings.DriverName == model.DATABASE_DRIVER_MYSQL { - lastIndex := strings.LastIndex(term, ".") - term = term[0:lastIndex] - } + // These chars must be removed from the like query. + for _, c := range ignoreUserSearchChar { + term = strings.Replace(term, c, "", -1) } - // these chars have special meaning and can be treated as spaces - for _, c := range specialUserSearchChar { - term = strings.Replace(term, c, " ", -1) + // These chars must be escaped in the like query. + for _, c := range escapeUserSearchChar { + term = strings.Replace(term, c, "*"+c, -1) } searchType := USER_SEARCH_TYPE_ALL @@ -1457,45 +1445,30 @@ func (us SqlUserStore) performSearch(searchQuery string, term string, options ma if term == "" { searchQuery = strings.Replace(searchQuery, "SEARCH_CLAUSE", "", 1) - } else if *utils.Cfg.SqlSettings.DriverName == model.DATABASE_DRIVER_POSTGRES { - if postgresUseOriginalTerm { - term = originalTerm - // these chars will break the query and must be removed - for _, c := range postgresSearchChar { - term = strings.Replace(term, c, "", -1) - } - } else { - splitTerm := strings.Fields(term) - for i, t := range strings.Fields(term) { - if i == len(splitTerm)-1 { - splitTerm[i] = t + ":*" + } else { + splitTerms := strings.Fields(term) + splitFields := strings.Split(searchType, ", ") + + terms := []string{} + for i, term := range splitTerms { + fields := []string{} + for _, field := range splitFields { + if *utils.Cfg.SqlSettings.DriverName == model.DATABASE_DRIVER_POSTGRES { + fields = append(fields, fmt.Sprintf("lower(%s) LIKE lower(%s) escape '*' ", field, fmt.Sprintf(":Term%d", i))) } else { - splitTerm[i] = t + ":* &" + fields = append(fields, fmt.Sprintf("%s LIKE %s escape '*' ", field, fmt.Sprintf(":Term%d", i))) } } - - term = strings.Join(splitTerm, " ") - } - - searchType = convertMySQLFullTextColumnsToPostgres(searchType) - searchClause := fmt.Sprintf("AND (%s) @@ to_tsquery('simple', :Term)", searchType) - searchQuery = strings.Replace(searchQuery, "SEARCH_CLAUSE", searchClause, 1) - } else if *utils.Cfg.SqlSettings.DriverName == model.DATABASE_DRIVER_MYSQL { - splitTerm := strings.Fields(term) - for i, t := range strings.Fields(term) { - splitTerm[i] = "+" + t + "*" + terms = append(terms, fmt.Sprintf("(%s)", strings.Join(fields, " OR "))) + parameters[fmt.Sprintf("Term%d", i)] = fmt.Sprintf("%s%%", term) } - term = strings.Join(splitTerm, " ") - - searchClause := fmt.Sprintf("AND MATCH(%s) AGAINST (:Term IN BOOLEAN MODE)", searchType) - searchQuery = strings.Replace(searchQuery, "SEARCH_CLAUSE", searchClause, 1) + term := strings.Join(terms, " AND ") + searchQuery = strings.Replace(searchQuery, "SEARCH_CLAUSE", fmt.Sprintf(" AND %s ", term), 1) } var users []*model.User - parameters["Term"] = term - if _, err := us.GetReplica().Select(&users, searchQuery, parameters); err != nil { result.Err = model.NewAppError("SqlUserStore.Search", "store.sql_user.search.app_error", nil, "term="+term+", "+"search_type="+searchType+", "+err.Error(), http.StatusInternalServerError) } else { diff --git a/store/sqlstore/user_store_test.go b/store/sqlstore/user_store_test.go index d46513ccc..646d785f7 100644 --- a/store/sqlstore/user_store_test.go +++ b/store/sqlstore/user_store_test.go @@ -1334,10 +1334,28 @@ func TestUserStoreSearch(t *testing.T) { u3.DeleteAt = 1 store.Must(ss.User().Save(u3)) + u5 := &model.User{} + u5.Username = "yu" + model.NewId() + u5.FirstName = "En" + u5.LastName = "Yu" + u5.Nickname = "enyu" + u5.Email = model.NewId() + "@simulator.amazonses.com" + store.Must(ss.User().Save(u5)) + + u6 := &model.User{} + u6.Username = "underscore" + model.NewId() + u6.FirstName = "Du_" + u6.LastName = "_DE" + u6.Nickname = "lodash" + u6.Email = model.NewId() + "@simulator.amazonses.com" + store.Must(ss.User().Save(u6)) + tid := model.NewId() store.Must(ss.Team().SaveMember(&model.TeamMember{TeamId: tid, UserId: u1.Id})) store.Must(ss.Team().SaveMember(&model.TeamMember{TeamId: tid, UserId: u2.Id})) store.Must(ss.Team().SaveMember(&model.TeamMember{TeamId: tid, UserId: u3.Id})) + store.Must(ss.Team().SaveMember(&model.TeamMember{TeamId: tid, UserId: u5.Id})) + store.Must(ss.Team().SaveMember(&model.TeamMember{TeamId: tid, UserId: u6.Id})) searchOptions := map[string]bool{} searchOptions[store.USER_SEARCH_OPTION_NAMES_ONLY] = true @@ -1367,6 +1385,22 @@ func TestUserStoreSearch(t *testing.T) { } } + if r1 := <-ss.User().Search(tid, "en", searchOptions); r1.Err != nil { + t.Fatal(r1.Err) + } else { + profiles := r1.Data.([]*model.User) + found1 := false + for _, profile := range profiles { + if profile.Id == u5.Id { + found1 = true + } + } + + if !found1 { + t.Fatal("should have found user") + } + } + searchOptions[store.USER_SEARCH_OPTION_NAMES_ONLY] = false if r1 := <-ss.User().Search(tid, u1.Email, searchOptions); r1.Err != nil { @@ -1429,6 +1463,56 @@ func TestUserStoreSearch(t *testing.T) { } } + // % should be escaped and searched for. + if r1 := <-ss.User().Search(tid, "h%", searchOptions); r1.Err != nil { + t.Fatal(r1.Err) + } else { + profiles := r1.Data.([]*model.User) + if len(profiles) != 0 { + t.Fatal("shouldn't have found anything") + } + } + + // "_" should be properly escaped and searched for. + if r1 := <-ss.User().Search(tid, "h_", searchOptions); r1.Err != nil { + t.Fatal(r1.Err) + } else { + profiles := r1.Data.([]*model.User) + if len(profiles) != 0 { + t.Fatal("shouldn't have found anything") + } + } + if r1 := <-ss.User().Search(tid, "Du_", searchOptions); r1.Err != nil { + t.Fatal(r1.Err) + } else { + profiles := r1.Data.([]*model.User) + found6 := false + for _, profile := range profiles { + if profile.Id == u6.Id { + found6 = true + } + } + + if !found6 { + t.Fatal("should have found user") + } + } + if r1 := <-ss.User().Search(tid, "_dE", searchOptions); r1.Err != nil { + t.Fatal(r1.Err) + } else { + profiles := r1.Data.([]*model.User) + found6 := false + for _, profile := range profiles { + if profile.Id == u6.Id { + found6 = true + } + } + + if !found6 { + t.Fatal("should have found user") + } + } + searchOptions[store.USER_SEARCH_OPTION_ALLOW_INACTIVE] = true if r1 := <-ss.User().Search(tid, "jimb", searchOptions); r1.Err != nil { -- cgit v1.2.3-1-g7c22