feat: improve performance (#359)

* refactor(appeal): create find active grant query

* feat(store): add indexes for slow queries

* fix(appeal): return grant err not found

* fix(appeal): use order by

* chore: rename var activeGrant

* fix: test

* chore: add index explaination

core/appeal/errors.go

@@ -48,4 +48,5 @@ var (
 	ErrApproverInvalidType      = errors.New("invalid approver type, expected an email string or array of email string")
 	ErrApproverEmail            = errors.New("approver is not a valid email")
 	ErrApproverNotFound         = errors.New("approver not found")
+	ErrGrantNotFound            = errors.New("grant not found")
 )

core/appeal/service.go

@@ -196,10 +196,6 @@ func (s *Service) Create(ctx context.Context, appeals []*domain.Appeal, opts ...
 	if err != nil {
 		return fmt.Errorf("listing pending appeals: %w", err)
 	}
-	activeGrants, err := s.getActiveGrantsMap(ctx)
-	if err != nil {
-		return fmt.Errorf("listing active grants: %w", err)
-	}
 
 	notifications := []domain.Notification{}
 
@@ -227,8 +223,15 @@ func (s *Service) Create(ctx context.Context, appeals []*domain.Appeal, opts ...
 			}
 		}
 
-		if err := s.checkExtensionEligibility(appeal, provider, policy, activeGrants); err != nil {
-			return fmt.Errorf("checking grant extension eligibility: %w", err)
+		activeGrant, err := s.findActiveGrant(ctx, appeal)
+		if err != nil && err != ErrGrantNotFound {
+			return err
+		}
+
+		if activeGrant != nil {
+			if err := s.checkExtensionEligibility(appeal, provider, policy, activeGrant); err != nil {
+				return fmt.Errorf("checking grant extension eligibility: %w", err)
+			}
 		}
 
 		if err := s.providerService.ValidateAppeal(ctx, appeal, provider, policy); err != nil {
@@ -325,6 +328,26 @@ func (s *Service) Create(ctx context.Context, appeals []*domain.Appeal, opts ...
 	return nil
 }
 
+func (s *Service) findActiveGrant(ctx context.Context, a *domain.Appeal) (*domain.Grant, error) {
+	grants, err := s.grantService.List(ctx, domain.ListGrantsFilter{
+		Statuses:    []string{string(domain.GrantStatusActive)},
+		AccountIDs:  []string{a.AccountID},
+		ResourceIDs: []string{a.ResourceID},
+		Roles:       []string{a.Role},
+		OrderBy:     []string{"updated_at:desc"},
+	})
+
+	if err != nil {
+		return nil, fmt.Errorf("listing active grants: %w", err)
+	}
+
+	if len(grants) == 0 {
+		return nil, ErrGrantNotFound
+	}
+
+	return &grants[0], nil
+}
+
 func addOnBehalfApprovedNotification(appeal *domain.Appeal, notifications []domain.Notification) []domain.Notification {
 	if appeal.AccountType == domain.DefaultAppealAccountType && appeal.AccountID != appeal.CreatedBy {
 		notifications = append(notifications, domain.Notification{
@@ -705,28 +728,6 @@ func (s *Service) getPendingAppealsMap(ctx context.Context) (map[string]map[stri
 	return appealsMap, nil
 }
 
-func (s *Service) getActiveGrantsMap(ctx context.Context) (map[string]map[string]map[string]*domain.Grant, error) {
-	grants, err := s.grantService.List(ctx, domain.ListGrantsFilter{
-		Statuses: []string{string(domain.GrantStatusActive)},
-	})
-	if err != nil {
-		return nil, err
-	}
-
-	grantsMap := map[string]map[string]map[string]*domain.Grant{}
-	for i, a := range grants {
-		if grantsMap[a.AccountID] == nil {
-			grantsMap[a.AccountID] = map[string]map[string]*domain.Grant{}
-		}
-		if grantsMap[a.AccountID][a.ResourceID] == nil {
-			grantsMap[a.AccountID][a.ResourceID] = map[string]*domain.Grant{}
-		}
-		grantsMap[a.AccountID][a.ResourceID][a.Role] = &grants[i]
-	}
-
-	return grantsMap, nil
-}
-
 func (s *Service) getResourcesMap(ctx context.Context, ids []string) (map[string]*domain.Resource, error) {
 	filters := domain.ListResourcesFilter{IDs: ids}
 	resources, err := s.resourceService.Find(ctx, filters)
@@ -941,12 +942,7 @@ func (s *Service) GrantAccessToProvider(ctx context.Context, a *domain.Appeal, o
 	return nil
 }
 
-func (s *Service) checkExtensionEligibility(a *domain.Appeal, p *domain.Provider, policy *domain.Policy, activeGrants map[string]map[string]map[string]*domain.Grant) error {
-	grant, exists := activeGrants[a.AccountID][a.ResourceID][a.Role]
-	if !exists || grant == nil {
-		return nil
-	}
-
+func (s *Service) checkExtensionEligibility(a *domain.Appeal, p *domain.Provider, policy *domain.Policy, activeGrant *domain.Grant) error {
 	AllowActiveAccessExtensionIn := ""
 
 	// Default to use provider config if policy config is not set
@@ -970,7 +966,7 @@ func (s *Service) checkExtensionEligibility(a *domain.Appeal, p *domain.Provider
 		return fmt.Errorf("%w: %v: %v", ErrAppealInvalidExtensionDuration, AllowActiveAccessExtensionIn, err)
 	}
 
-	if !grant.IsEligibleForExtension(extensionDurationRule) {
+	if !activeGrant.IsEligibleForExtension(extensionDurationRule) {
 		return ErrGrantNotEligibleForExtension
 	}
 	return nil

core/appeal/service_test.go

@@ -550,6 +550,7 @@ func (s *ServiceTestSuite) TestCreate() {
 
 		for _, tc := range testCases {
 			s.Run(tc.name, func() {
+				s.setup()
 				s.mockResourceService.On("Find", mock.Anything, mock.Anything).Return(tc.resources, nil).Once()
 				s.mockProviderService.On("Find", mock.Anything).Return(tc.providers, nil).Once()
 				s.mockPolicyService.On("Find", mock.Anything).Return(tc.policies, nil).Once()
@@ -558,7 +559,7 @@ func (s *ServiceTestSuite) TestCreate() {
 					Return(tc.existingAppeals, nil).Once()
 				s.mockGrantService.EXPECT().
 					List(mock.AnythingOfType("*context.emptyCtx"), mock.AnythingOfType("domain.ListGrantsFilter")).
-					Return(tc.activeGrants, nil).Once()
+					Return(tc.activeGrants, nil)
 				if tc.callMockValidateAppeal {
 					s.mockProviderService.On("ValidateAppeal", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(tc.expectedAppealValidationError).Once()
 				}
@@ -1176,7 +1177,11 @@ func (s *ServiceTestSuite) TestCreateAppeal__WithExistingAppealAndWithAutoApprov
 		Return(expectedExistingAppeals, nil).Once()
 	s.mockGrantService.EXPECT().
 		List(mock.AnythingOfType("*context.emptyCtx"), domain.ListGrantsFilter{
-			Statuses: []string{string(domain.GrantStatusActive)},
+			Statuses:    []string{string(domain.GrantStatusActive)},
+			AccountIDs:  []string{appeals[0].AccountID},
+			ResourceIDs: []string{appeals[0].ResourceID},
+			Roles:       []string{appeals[0].Role},
+			OrderBy:     []string{"updated_at:desc"},
 		}).
 		Return(expectedExistingGrants, nil).Once()
 	s.mockProviderService.On("ValidateAppeal", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil)

internal/store/postgres/migrations/000014_add_indexes_to_improve_slow_queries.down.sql

@@ -0,0 +1,6 @@
+DROP INDEX IF EXISTS idx_appeals_created_by_status_deleted_at_updated_at;
+DROP INDEX IF EXISTS idx_appeals_id_deleted_at_null;
+DROP INDEX IF EXISTS idx_approvers_approval_id_deleted_at_null;
+DROP INDEX IF EXISTS idx_grants_appeal_id_deleted_at_null;
+DROP INDEX IF EXISTS idx_grants_status_deleted_at_null;
+DROP INDEX IF EXISTS idx_resources_id_deleted_at_null;

internal/store/postgres/migrations/000014_add_indexes_to_improve_slow_queries.up.sql

@@ -0,0 +1,43 @@
+-- This index reducing query cost from 13997.45..13997 to 17.66..17.67
+-- SELECT ...
+-- FROM "appeals"
+--     LEFT JOIN "resources" "Resource" ON "appeals"."resource_id" = "Resource"."id"
+--     AND "Resource"."deleted_at" IS NULL
+--     LEFT JOIN "grants" "Grant" ON "appeals"."id" = "Grant"."appeal_id"
+--     AND "Grant"."deleted_at" IS NULL
+-- WHERE "appeals"."created_by" = '<creator>'
+--     AND "appeals"."status" IN (...)
+--     AND "appeals"."deleted_at" IS NULL
+-- ORDER BY ARRAY_POSITION(
+--         ARRAY [...],
+--         "appeals"."status"
+--     ),
+--     "updated_at" desc;
+
+CREATE INDEX IF NOT EXISTS idx_appeals_created_by_status_deleted_at_updated_at
+ON appeals (created_by, status, deleted_at, updated_at DESC)
+WHERE deleted_at IS NULL;
+
+-- This index reducing query cost from 0.15..8.17 to 0.00..1.02
+-- Since we have a lot of queries like this due to the gorm soft delete:
+-- SELECT * FROM <table> WHERE id = '<uuid>' AND "deleted_at" IS NULL ORDER BY "id" LIMIT 1
+CREATE INDEX IF NOT EXISTS idx_appeals_id_deleted_at_null
+ON appeals (id, deleted_at)
+WHERE deleted_at IS NULL;
+
+CREATE INDEX IF NOT EXISTS idx_approvers_approval_id_deleted_at_null
+ON approvers (approval_id, deleted_at)
+WHERE deleted_at IS NULL;
+
+CREATE INDEX IF NOT EXISTS idx_grants_appeal_id_deleted_at_null
+ON grants (appeal_id, deleted_at)
+WHERE deleted_at IS NULL;
+
+CREATE INDEX IF NOT EXISTS idx_grants_status_deleted_at_null
+ON grants (status, deleted_at)
+WHERE deleted_at IS NULL and status = 'active';
+
+CREATE INDEX IF NOT EXISTS idx_resources_id_deleted_at_null
+ON resources (id, deleted_at)
+WHERE deleted_at IS NULL;
+