Merge pull request #1301 from laz2/fix-783-filter-top-repos

Fix filtering top repositories (#783, #1281) (recreated for dev branch)

src/common/dao/dao_test.go

@@ -980,13 +980,6 @@ func TestGetRecentLogs(t *testing.T) {
 	}
 }
 
-func TestGetTopRepos(t *testing.T) {
-	_, err := GetTopRepos(10)
-	if err != nil {
-		t.Fatalf("error occured in getting top repos, error: %v", err)
-	}
-}
-
 var targetID, policyID, policyID2, policyID3, jobID, jobID2, jobID3 int64
 
 func TestAddRepTarget(t *testing.T) {

src/common/dao/repository.go

@@ -102,12 +102,22 @@ func GetRepositoryByProjectName(name string) ([]*models.RepoRecord, error) {
 }
 
 //GetTopRepos returns the most popular repositories
-func GetTopRepos(count int) ([]models.TopRepo, error) {
+func GetTopRepos(userID int, count int) ([]models.TopRepo, error) {
 	topRepos := []models.TopRepo{}
 
+	sql := `select r.name, r.pull_count from repository r
+		inner join project p on r.project_id = p.project_id
+		where (
+			p.deleted = 0 and (
+				p.public = 1 or (
+					? <> ? and exists (
+						select 1 from project_member pm
+						where pm.project_id = p.project_id and pm.user_id = ?
+		))))
+		order by r.pull_count desc, r.name limit ?`
 	repositories := []*models.RepoRecord{}
-	if _, err := GetOrmer().QueryTable(&models.RepoRecord{}).
-		OrderBy("-PullCount", "Name").Limit(count).All(&repositories); err != nil {
+	_, err := GetOrmer().Raw(sql, userID, NonExistUserID, userID, count).QueryRows(&repositories)
+	if err != nil {
 		return topRepos, err
 	}
 

src/common/dao/repository_test.go

@@ -16,8 +16,11 @@
 package dao
 
 import (
+	"fmt"
 	"testing"
+	"time"
 
+	"github.com/stretchr/testify/require"
 	"github.com/vmware/harbor/src/common/models"
 )
 
@@ -160,6 +163,161 @@ func TestGetTotalOfUserRelevantRepositories(t *testing.T) {
 	}
 }
 
+func TestGetTopRepos(t *testing.T) {
+	var err error
+	require := require.New(t)
+
+	require.NoError(GetOrmer().Begin())
+	defer func() {
+		require.NoError(GetOrmer().Rollback())
+	}()
+
+	admin, err := GetUser(models.User{Username: "admin"})
+	require.NoError(err)
+
+	user := models.User{
+		Username: "user",
+		Password: "user",
+		Email:    "user@test.com",
+	}
+	userID, err := Register(user)
+	require.NoError(err)
+	user.UserID = int(userID)
+
+	//
+	// public project with 1 repository
+	// non-public project with 2 repositories visible by admin
+	// non-public project with 1 repository visible by admin and user
+	// deleted public project with 1 repository
+	//
+
+	project1 := models.Project{
+		OwnerID:      admin.UserID,
+		Name:         "project1",
+		CreationTime: time.Now(),
+		OwnerName:    admin.Username,
+		Public:       0,
+	}
+	project1.ProjectID, err = AddProject(project1)
+	require.NoError(err)
+
+	project2 := models.Project{
+		OwnerID:      admin.UserID,
+		Name:         "project2",
+		CreationTime: time.Now(),
+		OwnerName:    admin.Username,
+		Public:       0,
+	}
+	project2.ProjectID, err = AddProject(project2)
+	require.NoError(err)
+
+	require.NoError(AddProjectMember(project2.ProjectID, user.UserID, models.PROJECTADMIN))
+
+	err = AddRepository(*repository)
+	require.NoError(err)
+
+	repository1 := &models.RepoRecord{
+		Name:        fmt.Sprintf("%v/repository1", project1.Name),
+		OwnerName:   admin.Username,
+		ProjectName: project1.Name,
+	}
+	err = AddRepository(*repository1)
+	require.NoError(err)
+	require.NoError(IncreasePullCount(repository1.Name))
+	repository1, err = GetRepositoryByName(repository1.Name)
+	require.NoError(err)
+
+	repository2 := &models.RepoRecord{
+		Name:        fmt.Sprintf("%v/repository2", project1.Name),
+		OwnerName:   admin.Username,
+		ProjectName: project1.Name,
+	}
+	err = AddRepository(*repository2)
+	require.NoError(err)
+	require.NoError(IncreasePullCount(repository2.Name))
+	require.NoError(IncreasePullCount(repository2.Name))
+	repository2, err = GetRepositoryByName(repository2.Name)
+	require.NoError(err)
+
+	repository3 := &models.RepoRecord{
+		Name:        fmt.Sprintf("%v/repository3", project2.Name),
+		OwnerName:   admin.Username,
+		ProjectName: project2.Name,
+	}
+	err = AddRepository(*repository3)
+	require.NoError(err)
+	require.NoError(IncreasePullCount(repository3.Name))
+	require.NoError(IncreasePullCount(repository3.Name))
+	require.NoError(IncreasePullCount(repository3.Name))
+	repository3, err = GetRepositoryByName(repository3.Name)
+	require.NoError(err)
+
+	deletedPublicProject := models.Project{
+		OwnerID:      admin.UserID,
+		Name:         "public-deleted",
+		CreationTime: time.Now(),
+		OwnerName:    admin.Username,
+		Public:       1,
+	}
+	deletedPublicProject.ProjectID, err = AddProject(deletedPublicProject)
+	require.NoError(err)
+	deletedPublicRepository1 := &models.RepoRecord{
+		Name:        fmt.Sprintf("%v/repository1", deletedPublicProject.Name),
+		OwnerName:   admin.Username,
+		ProjectName: deletedPublicProject.Name,
+	}
+	err = AddRepository(*deletedPublicRepository1)
+	require.NoError(err)
+	DeleteProject(deletedPublicProject.ProjectID)
+
+	var topRepos []models.TopRepo
+
+	// anonymous should retrieve public non-deleted repositories
+	topRepos, err = GetTopRepos(NonExistUserID, 3)
+	require.NoError(err)
+	require.Len(topRepos, 1)
+	require.Equal(topRepos, []models.TopRepo{
+		models.TopRepo{
+			RepoName:    repository.Name,
+			AccessCount: repository.PullCount,
+		},
+	})
+
+	// admin should retrieve all visible repositories limited by count
+	topRepos, err = GetTopRepos(admin.UserID, 3)
+	require.NoError(err)
+	require.Len(topRepos, 3)
+	require.Equal(topRepos, []models.TopRepo{
+		models.TopRepo{
+			RepoName:    repository3.Name,
+			AccessCount: repository3.PullCount,
+		},
+		models.TopRepo{
+			RepoName:    repository2.Name,
+			AccessCount: repository2.PullCount,
+		},
+		models.TopRepo{
+			RepoName:    repository1.Name,
+			AccessCount: repository1.PullCount,
+		},
+	})
+
+	// user should retrieve all visible repositories
+	topRepos, err = GetTopRepos(user.UserID, 3)
+	require.NoError(err)
+	require.Len(topRepos, 2)
+	require.Equal(topRepos, []models.TopRepo{
+		models.TopRepo{
+			RepoName:    repository3.Name,
+			AccessCount: repository3.PullCount,
+		},
+		models.TopRepo{
+			RepoName:    repository.Name,
+			AccessCount: repository.PullCount,
+		},
+	})
+}
+
 func addRepository(repository *models.RepoRecord) error {
 	return AddRepository(*repository)
 }

src/ui/api/repository.go

@@ -416,7 +416,12 @@ func (ra *RepositoryAPI) GetTopRepos() {
 		ra.CustomAbort(http.StatusBadRequest, "invalid count")
 	}
 
-	repos, err := dao.GetTopRepos(count)
+	userID, _, ok := ra.GetUserIDForRequest()
+	if !ok {
+		userID = dao.NonExistUserID
+	}
+
+	repos, err := dao.GetTopRepos(userID, count)
 	if err != nil {
 		log.Errorf("failed to get top repos: %v", err)
 		ra.CustomAbort(http.StatusInternalServerError, "internal server error")