diff --git a/core/appeal/errors.go b/core/appeal/errors.go index 55413f81b..f67982011 100644 --- a/core/appeal/errors.go +++ b/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") ) diff --git a/core/appeal/service.go b/core/appeal/service.go index a2bae8037..c46c79f0d 100644 --- a/core/appeal/service.go +++ b/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 diff --git a/core/appeal/service_test.go b/core/appeal/service_test.go index 17b1035e7..b1586a7fe 100644 --- a/core/appeal/service_test.go +++ b/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) diff --git a/internal/store/postgres/migrations/000014_add_indexes_to_improve_slow_queries.down.sql b/internal/store/postgres/migrations/000014_add_indexes_to_improve_slow_queries.down.sql new file mode 100644 index 000000000..182e0b6e7 --- /dev/null +++ b/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; \ No newline at end of file diff --git a/internal/store/postgres/migrations/000014_add_indexes_to_improve_slow_queries.up.sql b/internal/store/postgres/migrations/000014_add_indexes_to_improve_slow_queries.up.sql new file mode 100644 index 000000000..33bb7b62d --- /dev/null +++ b/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" = '' +-- 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 WHERE id = '' 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; +