Fix bug in search API

Refactor the logic of search API to fix bug mentioned in issue #5156 and #3838

src/common/models/project.go

@@ -105,26 +105,6 @@ func isTrue(value string) bool {
 		strings.ToLower(value) == "1"
 }
 
-// ProjectSorter holds an array of projects
-type ProjectSorter struct {
-	Projects []*Project
-}
-
-// Len returns the length of array in ProjectSorter
-func (ps *ProjectSorter) Len() int {
-	return len(ps.Projects)
-}
-
-// Less defines the comparison rules of project
-func (ps *ProjectSorter) Less(i, j int) bool {
-	return ps.Projects[i].Name < ps.Projects[j].Name
-}
-
-// Swap swaps the position of i and j
-func (ps *ProjectSorter) Swap(i, j int) {
-	ps.Projects[i], ps.Projects[j] = ps.Projects[j], ps.Projects[i]
-}
-
 // ProjectQueryParam can be used to set query parameters when listing projects.
 // The query condition will be set in the query if its corresponding field
 // is not nil. Leave it empty if you don't want to apply this condition.

src/ui/api/search.go

@@ -17,7 +17,6 @@ package api
 import (
 	"fmt"
 	"net/http"
-	"sort"
 	"strings"
 
 	"github.com/vmware/harbor/src/common"
@@ -80,8 +79,6 @@ func (s *SearchAPI) Get() {
 		}
 	}
 
-	projectSorter := &models.ProjectSorter{Projects: projects}
-	sort.Sort(projectSorter)
 	projectResult := []*models.Project{}
 	for _, p := range projects {
 		if len(keyword) > 0 && !strings.Contains(p.Name, keyword) {
@@ -125,43 +122,46 @@ func (s *SearchAPI) Get() {
 
 func filterRepositories(projects []*models.Project, keyword string) (
 	[]map[string]interface{}, error) {
+	result := []map[string]interface{}{}
+	if len(projects) == 0 {
+		return result, nil
+	}
 
-	repositories, err := dao.GetRepositories()
+	repositories, err := dao.GetRepositories(&models.RepositoryQuery{
+		Name: keyword,
+	})
 	if err != nil {
 		return nil, err
 	}
+	if len(repositories) == 0 {
+		return result, nil
+	}
 
-	i, j := 0, 0
+	projectMap := map[string]*models.Project{}
-	result := []map[string]interface{}{}
+	for _, project := range projects {
-	for i < len(repositories) && j < len(projects) {
+		projectMap[project.Name] = project
-		r := repositories[i]
+	}
-		p, _ := utils.ParseRepository(r.Name)
+
-		d := strings.Compare(p, projects[j].Name)
+	for _, repository := range repositories {
-		if d < 0 {
+		projectName, _ := utils.ParseRepository(repository.Name)
-			i++
+		project, exist := projectMap[projectName]
+		if !exist {
 			continue
-		} else if d == 0 {
-			i++
-			if len(keyword) != 0 && !strings.Contains(r.Name, keyword) {
-				continue
-			}
-			entry := make(map[string]interface{})
-			entry["repository_name"] = r.Name
-			entry["project_name"] = projects[j].Name
-			entry["project_id"] = projects[j].ProjectID
-			entry["project_public"] = projects[j].IsPublic()
-			entry["pull_count"] = r.PullCount
-
-			tags, err := getTags(r.Name)
-			if err != nil {
-				return nil, err
-			}
-			entry["tags_count"] = len(tags)
-
-			result = append(result, entry)
-		} else {
-			j++
 		}
+		entry := make(map[string]interface{})
+		entry["repository_name"] = repository.Name
+		entry["project_name"] = project.Name
+		entry["project_id"] = project.ProjectID
+		entry["project_public"] = project.IsPublic()
+		entry["pull_count"] = repository.PullCount
+
+		tags, err := getTags(repository.Name)
+		if err != nil {
+			return nil, err
+		}
+		entry["tags_count"] = len(tags)
+
+		result = append(result, entry)
 	}
 	return result, nil
 }

src/ui/api/search_test.go

@@ -15,53 +15,153 @@ package api
 
 import (
 	"fmt"
+	"net/http"
 	"testing"
 
+	"github.com/vmware/harbor/src/common"
+	"github.com/vmware/harbor/src/common/models"
+
 	"github.com/stretchr/testify/assert"
-	"github.com/vmware/harbor/tests/apitests/apilib"
+	"github.com/stretchr/testify/require"
+	"github.com/vmware/harbor/src/common/dao"
+	member "github.com/vmware/harbor/src/common/dao/project"
 )
 
 func TestSearch(t *testing.T) {
 	fmt.Println("Testing Search(SearchGet) API")
-	assert := assert.New(t)
+	// create a public project named "search"
+	projectID1, err := dao.AddProject(models.Project{
+		Name:    "search",
+		OwnerID: int(nonSysAdminID),
+	})
+	require.Nil(t, err)
+	defer dao.DeleteProject(projectID1)
 
-	apiTest := newHarborAPI()
+	err = dao.AddProjectMetadata(&models.ProjectMetadata{
-	var result apilib.Search
+		ProjectID: projectID1,
+		Name:      "public",
+		Value:     "true",
+	})
+	require.Nil(t, err)
 
-	//-------------case 1 : Response Code  = 200, Not sysAdmin --------------//
+	memberID1, err := member.AddProjectMember(models.Member{
-	httpStatusCode, result, err := apiTest.SearchGet("library")
+		ProjectID:  projectID1,
-	if err != nil {
+		EntityID:   int(nonSysAdminID),
-		t.Error("Error while search project or repository", err.Error())
+		EntityType: common.UserMember,
-		t.Log(err)
+		Role:       models.GUEST,
-	} else {
+	})
-		assert.Equal(int(200), httpStatusCode, "httpStatusCode should be 200")
+	require.Nil(t, err)
-		assert.Equal(int64(1), result.Projects[0].ProjectID, "Project id should be equal")
+	defer member.DeleteProjectMemberByID(memberID1)
-		assert.Equal("library", result.Projects[0].Name, "Project name should be library")
+
-		assert.True(result.Projects[0].IsPublic(), "Project public status should be 1 (true)")
+	// create a private project named "search-2", the "-" is necessary
+	// in the project name to test some corner cases
+	projectID2, err := dao.AddProject(models.Project{
+		Name:    "search-2",
+		OwnerID: int(nonSysAdminID),
+	})
+	require.Nil(t, err)
+	defer dao.DeleteProject(projectID2)
+
+	memberID2, err := member.AddProjectMember(models.Member{
+		ProjectID:  projectID2,
+		EntityID:   int(nonSysAdminID),
+		EntityType: common.UserMember,
+		Role:       models.GUEST,
+	})
+	require.Nil(t, err)
+	defer member.DeleteProjectMemberByID(memberID2)
+
+	// add a repository in project "search"
+	err = dao.AddRepository(models.RepoRecord{
+		ProjectID: projectID1,
+		Name:      "search/hello-world",
+	})
+	require.Nil(t, err)
+
+	// add a repository in project "search-2"
+	err = dao.AddRepository(models.RepoRecord{
+		ProjectID: projectID2,
+		Name:      "search-2/hello-world",
+	})
+	require.Nil(t, err)
+
+	// search without login
+	result := &searchResult{}
+	err = handleAndParse(&testingRequest{
+		method: http.MethodGet,
+		url:    "/api/search",
+		queryStruct: struct {
+			Keyword string `url:"q"`
+		}{
+			Keyword: "search",
+		},
+	}, result)
+	require.Nil(t, err)
+	require.Equal(t, 1, len(result.Project))
+	require.Equal(t, 1, len(result.Repository))
+	assert.Equal(t, "search", result.Project[0].Name)
+	assert.Equal(t, "search/hello-world", result.Repository[0]["repository_name"].(string))
+
+	// search with user who is the member of the project
+	err = handleAndParse(&testingRequest{
+		method: http.MethodGet,
+		url:    "/api/search",
+		queryStruct: struct {
+			Keyword string `url:"q"`
+		}{
+			Keyword: "search",
+		},
+		credential: nonSysAdmin,
+	}, result)
+	require.Nil(t, err)
+	require.Equal(t, 2, len(result.Project))
+	require.Equal(t, 2, len(result.Repository))
+	projects := map[string]struct{}{}
+	repositories := map[string]struct{}{}
+	for _, project := range result.Project {
+		projects[project.Name] = struct{}{}
+	}
+	for _, repository := range result.Repository {
+		repositories[repository["repository_name"].(string)] = struct{}{}
 	}
 
-	//--------case 2 : Response Code  = 200, sysAdmin and search repo--------//
+	_, exist := projects["search"]
-	httpStatusCode, result, err = apiTest.SearchGet("library", *admin)
+	assert.True(t, exist)
-	if err != nil {
+	_, exist = projects["search-2"]
-		t.Error("Error while search project or repository", err.Error())
+	assert.True(t, exist)
-		t.Log(err)
+	_, exist = repositories["search/hello-world"]
-	} else {
+	assert.True(t, exist)
-		assert.Equal(int(200), httpStatusCode, "httpStatusCode should be 200")
+	_, exist = repositories["search-2/hello-world"]
-		assert.Equal("library", result.Repositories[0].ProjectName, "Project name should be library")
+	assert.True(t, exist)
-		assert.Equal("library/busybox", result.Repositories[0].RepositoryName, "Repository  name should be library/busybox")
-		assert.True(result.Repositories[0].ProjectPublic, "Project public status should be 1 (true)")
-	}
 
-	//--------case 3 : Response Code  = 200, normal user and search repo--------//
+	// search with system admin
-	httpStatusCode, result, err = apiTest.SearchGet("library", *testUser)
+	err = handleAndParse(&testingRequest{
-	if err != nil {
+		method: http.MethodGet,
-		t.Error("Error while search project or repository", err.Error())
+		url:    "/api/search",
-		t.Log(err)
+		queryStruct: struct {
-	} else {
+			Keyword string `url:"q"`
-		assert.Equal(int(200), httpStatusCode, "httpStatusCode should be 200")
+		}{
-		assert.Equal("library", result.Repositories[0].ProjectName, "Project name should be library")
+			Keyword: "search",
-		assert.Equal("library/busybox", result.Repositories[0].RepositoryName, "Repository  name should be library/busybox")
+		},
-		assert.True(result.Repositories[0].ProjectPublic, "Project public status should be 1 (true)")
+		credential: sysAdmin,
+	}, result)
+	require.Nil(t, err)
+	require.Equal(t, 2, len(result.Project))
+	require.Equal(t, 2, len(result.Repository))
+	projects = map[string]struct{}{}
+	repositories = map[string]struct{}{}
+	for _, project := range result.Project {
+		projects[project.Name] = struct{}{}
 	}
-
+	for _, repository := range result.Repository {
+		repositories[repository["repository_name"].(string)] = struct{}{}
+	}
+	_, exist = projects["search"]
+	assert.True(t, exist)
+	_, exist = projects["search-2"]
+	assert.True(t, exist)
+	_, exist = repositories["search/hello-world"]
+	assert.True(t, exist)
+	_, exist = repositories["search-2/hello-world"]
+	assert.True(t, exist)
 }