Perf: Fix slow dashboards ACL query (#17427)

Fix slow ACL query for dashboards that was used as subquery on multiple places slowing down search and login in instances with many dashboards.
7 years ago · 1c3ad78672
parent 8a602e86f3
commit 1c3ad78672
2 changed files with 382 additions and 22 deletions
--- a/pkg/services/sqlstore/sqlbuilder.go
+++ b/pkg/services/sqlstore/sqlbuilder.go
@ -45,31 +45,48 @@ func (sb *SqlBuilder) writeDashboardPermissionFilter(user *m.SignedInUser, permi
 	sb.sql.WriteString(` AND
 	(
 		dashboard.id IN (
-			SELECT distinct d.id AS DashboardId
-			FROM dashboard AS d
-			 	LEFT JOIN dashboard folder on folder.id = d.folder_id
-			    LEFT JOIN dashboard_acl AS da ON
-	 			da.dashboard_id = d.id OR
-	 			da.dashboard_id = d.folder_id OR
-	 			(
-	 				-- include default permissions -->
-					da.org_id = -1 AND (
-					  (folder.id IS NOT NULL AND folder.has_acl = ` + falseStr + `) OR
-					  (folder.id IS NULL AND d.has_acl = ` + falseStr + `)
-					)
-	 			)
-				LEFT JOIN team_member as ugm on ugm.team_id = da.team_id
-			WHERE
-				d.org_id = ? AND
-				da.permission >= ? AND
-				(
-					da.user_id = ? OR
-					ugm.user_id = ? OR
-					da.role IN (?` + strings.Repeat(",?", len(okRoles)-1) + `)
-				)
+			SELECT distinct DashboardId from (
+				SELECT d.id AS DashboardId
+					FROM dashboard AS d
+					LEFT JOIN dashboard AS folder on folder.id = d.folder_id
+					LEFT JOIN dashboard_acl AS da ON
+						da.dashboard_id = d.id OR
+						da.dashboard_id = d.folder_id
+					LEFT JOIN team_member as ugm on ugm.team_id = da.team_id
+					WHERE
+						d.org_id = ? AND
+						da.permission >= ? AND
+						(
+							da.user_id = ? OR
+							ugm.user_id = ? OR
+							da.role IN (?` + strings.Repeat(",?", len(okRoles)-1) + `)
+						)
+				UNION
+				SELECT d.id AS DashboardId
+					FROM dashboard AS d
+					LEFT JOIN dashboard AS folder on folder.id = d.folder_id
+					LEFT JOIN dashboard_acl AS da ON
+						(
+							-- include default permissions -->
+							da.org_id = -1 AND (
+							  (folder.id IS NOT NULL AND folder.has_acl = ` + falseStr + `) OR
+							  (folder.id IS NULL AND d.has_acl = ` + falseStr + `)
+							)
+						)
+					WHERE
+						d.org_id = ? AND
+						da.permission >= ? AND
+						(
+							da.user_id = ? OR
+							da.role IN (?` + strings.Repeat(",?", len(okRoles)-1) + `)
+						)
+			) AS a
 		)
 	)`)

 	sb.params = append(sb.params, user.OrgId, permission, user.UserId, user.UserId)
 	sb.params = append(sb.params, okRoles...)
+
+	sb.params = append(sb.params, user.OrgId, permission, user.UserId)
+	sb.params = append(sb.params, okRoles...)
 }
--- a/pkg/services/sqlstore/sqlbuilder_test.go
+++ b/pkg/services/sqlstore/sqlbuilder_test.go
@ -0,0 +1,343 @@
+package sqlstore
+
+import (
+	"context"
+	"math/rand"
+	"testing"
+	"time"
+
+	"github.com/grafana/grafana/pkg/components/simplejson"
+	"github.com/grafana/grafana/pkg/models"
+	"github.com/stretchr/testify/assert"
+)
+
+func TestSqlBuilder(t *testing.T) {
+	t.Run("writeDashboardPermissionFilter", func(t *testing.T) {
+		t.Run("user ACL", func(t *testing.T) {
+			test(t,
+				DashboardProps{},
+				&DashboardPermission{User: true, Permission: models.PERMISSION_VIEW},
+				Search{UserFromACL: true, RequiredPermission: models.PERMISSION_VIEW},
+				shouldFind,
+			)
+
+			test(t,
+				DashboardProps{},
+				&DashboardPermission{User: true, Permission: models.PERMISSION_VIEW},
+				Search{UserFromACL: true, RequiredPermission: models.PERMISSION_EDIT},
+				shouldNotFind,
+			)
+
+			test(t,
+				DashboardProps{},
+				&DashboardPermission{User: true, Permission: models.PERMISSION_EDIT},
+				Search{UserFromACL: true, RequiredPermission: models.PERMISSION_EDIT},
+				shouldFind,
+			)
+
+			test(t,
+				DashboardProps{},
+				&DashboardPermission{User: true, Permission: models.PERMISSION_VIEW},
+				Search{RequiredPermission: models.PERMISSION_VIEW},
+				shouldNotFind,
+			)
+		})
+
+		t.Run("role ACL", func(t *testing.T) {
+			test(t,
+				DashboardProps{},
+				&DashboardPermission{Role: models.ROLE_VIEWER, Permission: models.PERMISSION_VIEW},
+				Search{UsersOrgRole: models.ROLE_VIEWER, RequiredPermission: models.PERMISSION_VIEW},
+				shouldFind,
+			)
+
+			test(t,
+				DashboardProps{},
+				&DashboardPermission{Role: models.ROLE_VIEWER, Permission: models.PERMISSION_VIEW},
+				Search{UsersOrgRole: models.ROLE_VIEWER, RequiredPermission: models.PERMISSION_EDIT},
+				shouldNotFind,
+			)
+
+			test(t,
+				DashboardProps{},
+				&DashboardPermission{Role: models.ROLE_EDITOR, Permission: models.PERMISSION_VIEW},
+				Search{UsersOrgRole: models.ROLE_VIEWER, RequiredPermission: models.PERMISSION_VIEW},
+				shouldNotFind,
+			)
+
+			test(t,
+				DashboardProps{},
+				&DashboardPermission{Role: models.ROLE_EDITOR, Permission: models.PERMISSION_VIEW},
+				Search{UsersOrgRole: models.ROLE_VIEWER, RequiredPermission: models.PERMISSION_VIEW},
+				shouldNotFind,
+			)
+		})
+
+		t.Run("team ACL", func(t *testing.T) {
+			test(t,
+				DashboardProps{},
+				&DashboardPermission{Team: true, Permission: models.PERMISSION_VIEW},
+				Search{UserFromACL: true, RequiredPermission: models.PERMISSION_VIEW},
+				shouldFind,
+			)
+
+			test(t,
+				DashboardProps{},
+				&DashboardPermission{Team: true, Permission: models.PERMISSION_VIEW},
+				Search{UserFromACL: true, RequiredPermission: models.PERMISSION_EDIT},
+				shouldNotFind,
+			)
+
+			test(t,
+				DashboardProps{},
+				&DashboardPermission{Team: true, Permission: models.PERMISSION_EDIT},
+				Search{UserFromACL: true, RequiredPermission: models.PERMISSION_EDIT},
+				shouldFind,
+			)
+
+			test(t,
+				DashboardProps{},
+				&DashboardPermission{Team: true, Permission: models.PERMISSION_EDIT},
+				Search{UserFromACL: false, RequiredPermission: models.PERMISSION_EDIT},
+				shouldNotFind,
+			)
+		})
+
+		t.Run("defaults for user ACL", func(t *testing.T) {
+			test(t,
+				DashboardProps{},
+				nil,
+				Search{OrgId: -1, UsersOrgRole: models.ROLE_VIEWER, RequiredPermission: models.PERMISSION_VIEW},
+				shouldNotFind,
+			)
+
+			test(t,
+				DashboardProps{OrgId: -1},
+				nil,
+				Search{OrgId: -1, UsersOrgRole: models.ROLE_VIEWER, RequiredPermission: models.PERMISSION_VIEW},
+				shouldFind,
+			)
+
+			test(t,
+				DashboardProps{OrgId: -1},
+				nil,
+				Search{OrgId: -1, UsersOrgRole: models.ROLE_EDITOR, RequiredPermission: models.PERMISSION_EDIT},
+				shouldFind,
+			)
+
+			test(t,
+				DashboardProps{OrgId: -1},
+				nil,
+				Search{OrgId: -1, UsersOrgRole: models.ROLE_VIEWER, RequiredPermission: models.PERMISSION_EDIT},
+				shouldNotFind,
+			)
+		})
+	})
+}
+
+var shouldFind = true
+var shouldNotFind = false
+
+type DashboardProps struct {
+	OrgId int64
+}
+
+type DashboardPermission struct {
+	User       bool
+	Team       bool
+	Role       models.RoleType
+	Permission models.PermissionType
+}
+
+type Search struct {
+	UsersOrgRole       models.RoleType
+	UserFromACL        bool
+	RequiredPermission models.PermissionType
+	OrgId              int64
+}
+
+type dashboardResponse struct {
+	Id int64
+}
+
+func test(t *testing.T, dashboardProps DashboardProps, dashboardPermission *DashboardPermission, search Search, shouldFind bool) {
+	// Will also cleanup the db
+	sqlStore := InitTestDB(t)
+
+	dashboard, err := createDummyDashboard(dashboardProps)
+	if !assert.Equal(t, nil, err) {
+		return
+	}
+
+	var aclUserId int64
+	if dashboardPermission != nil {
+		aclUserId, err = createDummyAcl(dashboardPermission, search, dashboard.Id)
+		if !assert.Equal(t, nil, err) {
+			return
+		}
+	}
+	dashboards, err := getDashboards(sqlStore, search, aclUserId)
+	if !assert.Equal(t, nil, err) {
+		return
+	}
+
+	if shouldFind {
+		if assert.Equal(t, 1, len(dashboards), "Should return one dashboard") {
+			assert.Equal(t, dashboards[0].Id, dashboard.Id, "Should return created dashboard")
+		}
+	} else {
+		assert.Equal(t, 0, len(dashboards), "Should node return any dashboard")
+	}
+}
+
+func createDummyUser() (*models.User, error) {
+	uid := rand.Intn(9999999)
+	createUserCmd := &models.CreateUserCommand{
+		Email:          string(uid) + "@example.com",
+		Login:          string(uid),
+		Name:           string(uid),
+		Company:        "",
+		OrgName:        "",
+		Password:       string(uid),
+		EmailVerified:  true,
+		IsAdmin:        false,
+		SkipOrgSetup:   false,
+		DefaultOrgRole: string(models.ROLE_VIEWER),
+	}
+	err := CreateUser(context.Background(), createUserCmd)
+	if err != nil {
+		return nil, err
+	}
+
+	return &createUserCmd.Result, nil
+}
+
+func createDummyTeam() (*models.Team, error) {
+	cmd := &models.CreateTeamCommand{
+		// Does not matter in this tests actually
+		OrgId: 1,
+		Name:  "test",
+		Email: "test@example.com",
+	}
+	err := CreateTeam(cmd)
+	if err != nil {
+		return nil, err
+	}
+
+	return &cmd.Result, nil
+}
+
+func createDummyDashboard(dashboardProps DashboardProps) (*models.Dashboard, error) {
+	json, _ := simplejson.NewJson([]byte(`{"schemaVersion":17,"title":"gdev dashboards","uid":"","version":1}`))
+
+	saveDashboardCmd := &models.SaveDashboardCommand{
+		Dashboard:    json,
+		UserId:       0,
+		Overwrite:    false,
+		Message:      "",
+		RestoredFrom: 0,
+		PluginId:     "",
+		FolderId:     0,
+		IsFolder:     false,
+		UpdatedAt:    time.Time{},
+	}
+	if dashboardProps.OrgId != 0 {
+		saveDashboardCmd.OrgId = dashboardProps.OrgId
+	} else {
+		saveDashboardCmd.OrgId = 1
+	}
+
+	err := SaveDashboard(saveDashboardCmd)
+	if err != nil {
+		return nil, err
+	}
+
+	return saveDashboardCmd.Result, nil
+}
+
+func createDummyAcl(dashboardPermission *DashboardPermission, search Search, dashboardId int64) (int64, error) {
+	acl := &models.DashboardAcl{
+		OrgId:       1,
+		Created:     time.Now(),
+		Updated:     time.Now(),
+		Permission:  dashboardPermission.Permission,
+		DashboardId: dashboardId,
+	}
+
+	var user *models.User
+	var err error
+	if dashboardPermission.User {
+		user, err = createDummyUser()
+		if err != nil {
+			return 0, err
+		}
+
+		acl.UserId = user.Id
+	}
+
+	if dashboardPermission.Team {
+		team, err := createDummyTeam()
+		if err != nil {
+			return 0, err
+		}
+		if search.UserFromACL {
+			user, err = createDummyUser()
+			if err != nil {
+				return 0, err
+			}
+			addTeamMemberCmd := &models.AddTeamMemberCommand{
+				UserId: user.Id,
+				OrgId:  1,
+				TeamId: team.Id,
+			}
+			err = AddTeamMember(addTeamMemberCmd)
+			if err != nil {
+				return 0, err
+			}
+		}
+
+		acl.TeamId = team.Id
+	}
+
+	if len(string(dashboardPermission.Role)) > 0 {
+		acl.Role = &dashboardPermission.Role
+	}
+
+	updateAclCmd := &models.UpdateDashboardAclCommand{
+		DashboardId: dashboardId,
+		Items:       []*models.DashboardAcl{acl},
+	}
+	err = UpdateDashboardAcl(updateAclCmd)
+	if user != nil {
+		return user.Id, err
+	}
+	return 0, err
+}
+
+func getDashboards(sqlStore *SqlStore, search Search, aclUserId int64) ([]*dashboardResponse, error) {
+	builder := &SqlBuilder{}
+	signedInUser := &models.SignedInUser{
+		UserId: 9999999999,
+	}
+
+	if search.OrgId == 0 {
+		signedInUser.OrgId = 1
+	} else {
+		signedInUser.OrgId = search.OrgId
+	}
+
+	if len(string(search.UsersOrgRole)) > 0 {
+		signedInUser.OrgRole = search.UsersOrgRole
+	} else {
+		signedInUser.OrgRole = models.ROLE_VIEWER
+	}
+	if search.UserFromACL {
+		signedInUser.UserId = aclUserId
+	}
+
+	var res []*dashboardResponse
+	builder.Write("SELECT * FROM dashboard WHERE true")
+	builder.writeDashboardPermissionFilter(signedInUser, search.RequiredPermission)
+	err := sqlStore.engine.SQL(builder.GetSqlString(), builder.params...).Find(&res)
+	return res, err
+}