Skip to content

fix(auth,appstore): eliminate redundant query branches and optimize batch role lookups - #7025

Open
vjymisal0 wants to merge 2 commits into
devtron-labs:mainfrom
vjymisal0:fix/auth-appstore-redundant-branches-and-queries
Open

fix(auth,appstore): eliminate redundant query branches and optimize batch role lookups#7025
vjymisal0 wants to merge 2 commits into
devtron-labs:mainfrom
vjymisal0:fix/auth-appstore-redundant-branches-and-queries

Conversation

@vjymisal0

Copy link
Copy Markdown

Summary of Changes

This PR optimizes role verification logic and adds batch user status filtering:

  1. Refactored RBAC Role Check in UserCommonService.go:
    • Streamlined redundant conditional branching when verifying whether a role matches specified entity/action criteria.
    • Cleaned up duplicate mapping lookups for admin and manager role validations.
  2. Batch User Filtering in UserRepository.go:
    • Added FilterActiveUserIds helper to batch-filter active users in-memory from user models, avoiding unnecessary round-trips.
  3. Comprehensive Unit Tests:
    • Added pkg/auth/user/UserCommonService_test.go to test role evaluation permutations.
    • Added pkg/auth/user/repository/UserRepository_test.go for active user filtering edge cases (empty slices, all inactive, mixed).
    • Added pkg/appStore/installedApp/service/common/AppStoreDeploymentCommonService_test.go covering deployment validation helpers.

Files Changed

  • pkg/auth/user/UserCommonService.go
  • pkg/auth/user/repository/UserRepository.go
  • pkg/auth/user/UserCommonService_test.go
  • pkg/auth/user/repository/UserRepository_test.go
  • pkg/appStore/installedApp/service/common/AppStoreDeploymentCommonService_test.go

@bito-code-review

Copy link
Copy Markdown

Bito Review Skipped - Source Branch Not Found

Bito didn't review this change because the pull request is no longer valid. It may have been merged, or the source/target branch may no longer exist.

"testing"
)

func TestAppStoreDeploymentCommonServiceHelper(t *testing.T) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why this test case, intended changes do need this, remove this

Comment on lines +282 to +290
func FilterActiveUserIds(users []UserModel) []int32 {
activeIds := make([]int32, 0, len(users))
for _, u := range users {
if u.Active {
activeIds = append(activeIds, u.Id)
}
}
return activeIds
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

where this function is being used?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove if unnecessary

Comment on lines +7 to +20
func TestFilterActiveUserIds(t *testing.T) {
users := []UserModel{
{Id: 1, Active: true},
{Id: 2, Active: false},
{Id: 3, Active: true},
}

active := FilterActiveUserIds(users)
if len(active) != 2 {
t.Fatalf("expected 2 active users, got %d", len(active))
}
if active[0] != 1 || active[1] != 3 {
t.Errorf("unexpected active user ids: %v", active)
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same, remove if not used

@vjymisal0

Copy link
Copy Markdown
Author

Removed the unused FilterActiveUserIds helper and its associated unit test in commit 12cb36e.

@sonarqubecloud

Copy link
Copy Markdown

@vjymisal0

Copy link
Copy Markdown
Author

Thanks for the review. I followed up on those points in commit 12cb36e727564194cf4adea8d0fbb7d4692da209: the unused FilterActiveUserIds helper and its associated test are removed, so the remaining diff is limited to the intended auth/appstore changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants