From 669ac6bda2fec49c667974e4787aa3d33ddb02b6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tim=20M=C3=B6hlmann?= Date: Thu, 20 Jun 2024 16:31:58 +0300 Subject: [PATCH] perf(import): do not check for existing grant ID (#8164) # Which Problems Are Solved Improve the performance of the `admin/v1/import` API endpoint. Specifaclly the import of large amount of project grants. # How the Problems Are Solved `AddProjectGrantWithID` and `AddProjectGrantMember` methods of `Commands` used to get the current state of the Writemodel to check if the current GrantID or the combination of GrantID & UserID wasn't already used. However, the Added events already have protection against duplication by the `UniqueConstaint` methods. The queries become very slow when there is a great amount of project grants. Because all the events are pushed to the aggregate ID of the project, we had to obtain all related project events, including events of grantIDs we do not care about. This O(n) duration for bached import jobs adding many organization granted to a single project. This change removes the unnecesary state query to improve performance. # Additional Changes - Add integration tests for import # Additional Context - reported internally --- go.mod | 2 +- .../api/grpc/admin/import_integration_test.go | 492 ++++++++++++++++++ internal/command/project_grant.go | 8 - internal/command/project_grant_member.go | 7 - internal/command/project_grant_member_test.go | 54 -- internal/integration/assert.go | 36 ++ 6 files changed, 529 insertions(+), 70 deletions(-) create mode 100644 internal/api/grpc/admin/import_integration_test.go diff --git a/go.mod b/go.mod index 1f5bcc264e..cff93d4c84 100644 --- a/go.mod +++ b/go.mod @@ -179,7 +179,7 @@ require ( github.com/modern-go/reflect2 v1.0.2 // indirect github.com/muesli/clusters v0.0.0-20200529215643-2700303c1762 // indirect github.com/muesli/kmeans v0.3.1 // indirect - github.com/pmezard/go-difflib v1.0.1-0.20181226105442-5d4384ee4fb2 // indirect + github.com/pmezard/go-difflib v1.0.1-0.20181226105442-5d4384ee4fb2 github.com/prometheus/client_golang v1.19.1 github.com/prometheus/client_model v0.6.1 // indirect github.com/prometheus/common v0.54.0 // indirect diff --git a/internal/api/grpc/admin/import_integration_test.go b/internal/api/grpc/admin/import_integration_test.go new file mode 100644 index 0000000000..1ee7d7d88e --- /dev/null +++ b/internal/api/grpc/admin/import_integration_test.go @@ -0,0 +1,492 @@ +//go:build integration + +package admin_test + +import ( + "testing" + "time" + + "github.com/brianvoe/gofakeit/v6" + "github.com/google/uuid" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/zitadel/zitadel/internal/integration" + "github.com/zitadel/zitadel/pkg/grpc/admin" + "github.com/zitadel/zitadel/pkg/grpc/management" + v1 "github.com/zitadel/zitadel/pkg/grpc/v1" +) + +func TestServer_ImportData(t *testing.T) { + orgIDs := generateIDs(10) + projectIDs := generateIDs(10) + userIDs := generateIDs(10) + grantIDs := generateIDs(10) + + tests := []struct { + name string + req *admin.ImportDataRequest + want *admin.ImportDataResponse + wantErr bool + }{ + { + name: "success", + req: &admin.ImportDataRequest{ + Data: &admin.ImportDataRequest_DataOrgs{ + DataOrgs: &admin.ImportDataOrg{ + Orgs: []*admin.DataOrg{ + { + OrgId: orgIDs[0], + Org: &management.AddOrgRequest{ + Name: gofakeit.ProductName(), + }, + Projects: []*v1.DataProject{ + { + ProjectId: projectIDs[0], + Project: &management.AddProjectRequest{ + Name: gofakeit.AppName(), + ProjectRoleAssertion: true, + }, + }, + { + ProjectId: projectIDs[1], + Project: &management.AddProjectRequest{ + Name: gofakeit.AppName(), + ProjectRoleAssertion: false, + }, + }, + }, + ProjectRoles: []*management.AddProjectRoleRequest{ + { + ProjectId: projectIDs[0], + RoleKey: "role1", + DisplayName: "role1", + }, + { + ProjectId: projectIDs[0], + RoleKey: "role2", + DisplayName: "role2", + }, + { + ProjectId: projectIDs[1], + RoleKey: "role3", + DisplayName: "role3", + }, + { + ProjectId: projectIDs[1], + RoleKey: "role4", + DisplayName: "role4", + }, + }, + HumanUsers: []*v1.DataHumanUser{ + { + UserId: userIDs[0], + User: &management.ImportHumanUserRequest{ + UserName: gofakeit.Username(), + Profile: &management.ImportHumanUserRequest_Profile{ + FirstName: gofakeit.FirstName(), + LastName: gofakeit.LastName(), + DisplayName: gofakeit.Username(), + PreferredLanguage: gofakeit.LanguageBCP(), + }, + Email: &management.ImportHumanUserRequest_Email{ + Email: gofakeit.Email(), + IsEmailVerified: true, + }, + }, + }, + { + UserId: userIDs[1], + User: &management.ImportHumanUserRequest{ + UserName: gofakeit.Username(), + Profile: &management.ImportHumanUserRequest_Profile{ + FirstName: gofakeit.FirstName(), + LastName: gofakeit.LastName(), + DisplayName: gofakeit.Username(), + PreferredLanguage: gofakeit.LanguageBCP(), + }, + Email: &management.ImportHumanUserRequest_Email{ + Email: gofakeit.Email(), + IsEmailVerified: true, + }, + }, + }, + }, + ProjectGrants: []*v1.DataProjectGrant{ + { + GrantId: grantIDs[0], + ProjectGrant: &management.AddProjectGrantRequest{ + ProjectId: projectIDs[0], + GrantedOrgId: orgIDs[1], + RoleKeys: []string{"role1", "role2"}, + }, + }, + { + GrantId: grantIDs[1], + ProjectGrant: &management.AddProjectGrantRequest{ + ProjectId: projectIDs[1], + GrantedOrgId: orgIDs[1], + RoleKeys: []string{"role3", "role4"}, + }, + }, + { + GrantId: grantIDs[2], + ProjectGrant: &management.AddProjectGrantRequest{ + ProjectId: projectIDs[0], + GrantedOrgId: orgIDs[2], + RoleKeys: []string{"role1", "role2"}, + }, + }, + { + GrantId: grantIDs[3], + ProjectGrant: &management.AddProjectGrantRequest{ + ProjectId: projectIDs[1], + GrantedOrgId: orgIDs[2], + RoleKeys: []string{"role3", "role4"}, + }, + }, + }, + }, + { + OrgId: orgIDs[1], + Org: &management.AddOrgRequest{ + Name: gofakeit.ProductName(), + }, + UserGrants: []*management.AddUserGrantRequest{ + { + UserId: userIDs[0], + ProjectId: projectIDs[0], + ProjectGrantId: grantIDs[0], + }, + { + UserId: userIDs[0], + ProjectId: projectIDs[1], + ProjectGrantId: grantIDs[1], + }, + }, + }, + { + OrgId: orgIDs[2], + Org: &management.AddOrgRequest{ + Name: gofakeit.ProductName(), + }, + UserGrants: []*management.AddUserGrantRequest{ + { + UserId: userIDs[1], + ProjectId: projectIDs[0], + ProjectGrantId: grantIDs[2], + }, + { + UserId: userIDs[1], + ProjectId: projectIDs[1], + ProjectGrantId: grantIDs[3], + }, + }, + }, + }, + }, + }, + Timeout: time.Minute.String(), + }, + want: &admin.ImportDataResponse{ + Success: &admin.ImportDataSuccess{ + Orgs: []*admin.ImportDataSuccessOrg{ + { + OrgId: orgIDs[0], + ProjectIds: projectIDs[0:2], + ProjectRoles: []string{ + projectIDs[0] + "_role1", + projectIDs[0] + "_role2", + projectIDs[1] + "_role3", + projectIDs[1] + "_role4", + }, + HumanUserIds: userIDs[0:2], + ProjectGrants: []*admin.ImportDataSuccessProjectGrant{ + { + GrantId: grantIDs[0], + ProjectId: projectIDs[0], + OrgId: orgIDs[1], + }, + { + GrantId: grantIDs[1], + ProjectId: projectIDs[1], + OrgId: orgIDs[1], + }, + { + GrantId: grantIDs[2], + ProjectId: projectIDs[0], + OrgId: orgIDs[2], + }, + { + GrantId: grantIDs[3], + ProjectId: projectIDs[1], + OrgId: orgIDs[2], + }, + }, + }, + { + OrgId: orgIDs[1], + UserGrants: []*admin.ImportDataSuccessUserGrant{ + { + ProjectId: projectIDs[0], + UserId: userIDs[0], + }, + { + UserId: userIDs[0], + ProjectId: projectIDs[1], + }, + }, + }, + { + OrgId: orgIDs[2], + UserGrants: []*admin.ImportDataSuccessUserGrant{ + { + ProjectId: projectIDs[0], + UserId: userIDs[1], + }, + { + UserId: userIDs[1], + ProjectId: projectIDs[1], + }, + }, + }, + }, + }, + }, + }, + { + name: "duplicate project grant error", + req: &admin.ImportDataRequest{ + Data: &admin.ImportDataRequest_DataOrgs{ + DataOrgs: &admin.ImportDataOrg{ + Orgs: []*admin.DataOrg{ + { + OrgId: orgIDs[3], + Org: &management.AddOrgRequest{ + Name: gofakeit.ProductName(), + }, + Projects: []*v1.DataProject{ + { + ProjectId: projectIDs[2], + Project: &management.AddProjectRequest{ + Name: gofakeit.AppName(), + ProjectRoleAssertion: true, + }, + }, + { + ProjectId: projectIDs[3], + Project: &management.AddProjectRequest{ + Name: gofakeit.AppName(), + ProjectRoleAssertion: false, + }, + }, + }, + ProjectRoles: []*management.AddProjectRoleRequest{ + { + ProjectId: projectIDs[2], + RoleKey: "role1", + DisplayName: "role1", + }, + { + ProjectId: projectIDs[2], + RoleKey: "role2", + DisplayName: "role2", + }, + { + ProjectId: projectIDs[3], + RoleKey: "role3", + DisplayName: "role3", + }, + { + ProjectId: projectIDs[3], + RoleKey: "role4", + DisplayName: "role4", + }, + }, + ProjectGrants: []*v1.DataProjectGrant{ + { + GrantId: grantIDs[4], + ProjectGrant: &management.AddProjectGrantRequest{ + ProjectId: projectIDs[2], + GrantedOrgId: orgIDs[4], + RoleKeys: []string{"role1", "role2"}, + }, + }, + { + GrantId: grantIDs[4], + ProjectGrant: &management.AddProjectGrantRequest{ + ProjectId: projectIDs[2], + GrantedOrgId: orgIDs[4], + RoleKeys: []string{"role1", "role2"}, + }, + }, + }, + }, + }, + }, + }, + Timeout: time.Minute.String(), + }, + want: &admin.ImportDataResponse{ + Errors: []*admin.ImportDataError{ + { + Type: "project_grant", + Id: orgIDs[3] + "_" + projectIDs[2] + "_" + orgIDs[4], + Message: "ID=V3-DKcYh Message=Errors.Project.Grant.AlreadyExists Parent=(ERROR: duplicate key value violates unique constraint \"unique_constraints_pkey\" (SQLSTATE 23505))", + }, + }, + Success: &admin.ImportDataSuccess{ + Orgs: []*admin.ImportDataSuccessOrg{ + { + OrgId: orgIDs[3], + ProjectIds: projectIDs[2:4], + ProjectRoles: []string{ + projectIDs[2] + "_role1", + projectIDs[2] + "_role2", + projectIDs[3] + "_role3", + projectIDs[3] + "_role4", + }, + ProjectGrants: []*admin.ImportDataSuccessProjectGrant{ + { + GrantId: grantIDs[4], + ProjectId: projectIDs[2], + OrgId: orgIDs[4], + }, + }, + }, + }, + }, + }, + }, + { + name: "duplicate project grant member error", + req: &admin.ImportDataRequest{ + Data: &admin.ImportDataRequest_DataOrgs{ + DataOrgs: &admin.ImportDataOrg{ + Orgs: []*admin.DataOrg{ + { + OrgId: orgIDs[5], + Org: &management.AddOrgRequest{ + Name: gofakeit.ProductName(), + }, + Projects: []*v1.DataProject{ + { + ProjectId: projectIDs[4], + Project: &management.AddProjectRequest{ + Name: gofakeit.AppName(), + ProjectRoleAssertion: true, + }, + }, + }, + ProjectRoles: []*management.AddProjectRoleRequest{ + { + ProjectId: projectIDs[4], + RoleKey: "role1", + DisplayName: "role1", + }, + }, + HumanUsers: []*v1.DataHumanUser{ + { + UserId: userIDs[2], + User: &management.ImportHumanUserRequest{ + UserName: gofakeit.Username(), + Profile: &management.ImportHumanUserRequest_Profile{ + FirstName: gofakeit.FirstName(), + LastName: gofakeit.LastName(), + DisplayName: gofakeit.Username(), + PreferredLanguage: gofakeit.LanguageBCP(), + }, + Email: &management.ImportHumanUserRequest_Email{ + Email: gofakeit.Email(), + IsEmailVerified: true, + }, + }, + }, + }, + ProjectGrants: []*v1.DataProjectGrant{ + { + GrantId: grantIDs[5], + ProjectGrant: &management.AddProjectGrantRequest{ + ProjectId: projectIDs[4], + GrantedOrgId: orgIDs[6], + RoleKeys: []string{"role1", "role2"}, + }, + }, + }, + ProjectGrantMembers: []*management.AddProjectGrantMemberRequest{ + { + ProjectId: projectIDs[4], + GrantId: grantIDs[5], + UserId: userIDs[2], + Roles: []string{"PROJECT_GRANT_OWNER"}, + }, + { + ProjectId: projectIDs[4], + GrantId: grantIDs[5], + UserId: userIDs[2], + Roles: []string{"PROJECT_GRANT_OWNER"}, + }, + }, + }, + }, + }, + }, + Timeout: time.Minute.String(), + }, + want: &admin.ImportDataResponse{ + Errors: []*admin.ImportDataError{ + { + Type: "project_grant_member", + Id: orgIDs[5] + "_" + projectIDs[4] + "_" + grantIDs[5] + "_" + userIDs[2], + Message: "ID=V3-DKcYh Message=Errors.Project.Member.AlreadyExists Parent=(ERROR: duplicate key value violates unique constraint \"unique_constraints_pkey\" (SQLSTATE 23505))", + }, + }, + Success: &admin.ImportDataSuccess{ + Orgs: []*admin.ImportDataSuccessOrg{ + { + OrgId: orgIDs[5], + ProjectIds: projectIDs[4:5], + ProjectRoles: []string{ + projectIDs[4] + "_role1", + }, + HumanUserIds: userIDs[2:3], + ProjectGrants: []*admin.ImportDataSuccessProjectGrant{ + { + GrantId: grantIDs[5], + ProjectId: projectIDs[4], + OrgId: orgIDs[6], + }, + }, + ProjectGrantMembers: []*admin.ImportDataSuccessProjectGrantMember{ + { + ProjectId: projectIDs[4], + GrantId: grantIDs[5], + UserId: userIDs[2], + }, + }, + }, + }, + }, + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := Client.ImportData(AdminCTX, tt.req) + if tt.wantErr { + assert.Error(t, err) + return + } + require.NoError(t, err) + integration.EqualProto(t, tt.want, got) + }) + } +} + +func generateIDs(n int) []string { + ids := make([]string, n) + for i := range ids { + ids[i] = uuid.NewString() + } + return ids +} diff --git a/internal/command/project_grant.go b/internal/command/project_grant.go index 6a5ab96fee..d0f2d210fb 100644 --- a/internal/command/project_grant.go +++ b/internal/command/project_grant.go @@ -17,14 +17,6 @@ func (c *Commands) AddProjectGrantWithID(ctx context.Context, grant *domain.Proj ctx, span := tracing.NewSpan(ctx) defer func() { span.EndWithError(err) }() - existingMember, err := c.projectGrantWriteModelByID(ctx, grantID, grant.AggregateID, resourceOwner) - if err != nil && !zerrors.IsNotFound(err) { - return nil, err - } - if existingMember != nil && existingMember.State != domain.ProjectGrantStateUnspecified { - return nil, zerrors.ThrowInvalidArgument(nil, "PROJECT-2b8fs", "Errors.Project.Grant.AlreadyExisting") - } - return c.addProjectGrantWithID(ctx, grant, grantID, resourceOwner) } diff --git a/internal/command/project_grant_member.go b/internal/command/project_grant_member.go index 9208ebf011..f7ea887475 100644 --- a/internal/command/project_grant_member.go +++ b/internal/command/project_grant_member.go @@ -26,13 +26,6 @@ func (c *Commands) AddProjectGrantMember(ctx context.Context, member *domain.Pro return nil, err } addedMember := NewProjectGrantMemberWriteModel(member.AggregateID, member.UserID, member.GrantID) - err = c.eventstore.FilterToQueryReducer(ctx, addedMember) - if err != nil { - return nil, err - } - if addedMember.State == domain.MemberStateActive { - return nil, zerrors.ThrowAlreadyExists(nil, "PROJECT-16dVN", "Errors.Project.Member.AlreadyExists") - } projectAgg := ProjectAggregateFromWriteModel(&addedMember.WriteModel) pushedEvents, err := c.eventstore.Push( ctx, diff --git a/internal/command/project_grant_member_test.go b/internal/command/project_grant_member_test.go index 2c52eace28..5535cc9367 100644 --- a/internal/command/project_grant_member_test.go +++ b/internal/command/project_grant_member_test.go @@ -104,58 +104,6 @@ func TestCommandSide_AddProjectGrantMember(t *testing.T) { err: zerrors.IsPreconditionFailed, }, }, - { - name: "member already exists, precondition error", - fields: fields{ - eventstore: eventstoreExpect( - t, - expectFilter( - eventFromEventPusher( - user.NewHumanAddedEvent(context.Background(), - &user.NewAggregate("user1", "org1").Aggregate, - "username1", - "firstname1", - "lastname1", - "nickname1", - "displayname1", - language.German, - domain.GenderMale, - "email1", - true, - ), - ), - ), - expectFilter( - eventFromEventPusher( - project.NewProjectGrantMemberAddedEvent(context.Background(), - &project.NewAggregate("project1", "org1").Aggregate, - "user1", - "projectgrant1", - ), - ), - ), - ), - zitadelRoles: []authz.RoleMapping{ - { - Role: "PROJECT_GRANT_OWNER", - }, - }, - }, - args: args{ - ctx: context.Background(), - member: &domain.ProjectGrantMember{ - ObjectRoot: models.ObjectRoot{ - AggregateID: "project1", - }, - GrantID: "projectgrant1", - UserID: "user1", - Roles: []string{"PROJECT_GRANT_OWNER"}, - }, - }, - res: res{ - err: zerrors.IsErrorAlreadyExists, - }, - }, { name: "member add uniqueconstraint err, already exists", fields: fields{ @@ -177,7 +125,6 @@ func TestCommandSide_AddProjectGrantMember(t *testing.T) { ), ), ), - expectFilter(), expectPushFailed(zerrors.ThrowAlreadyExists(nil, "ERROR", "internal"), project.NewProjectGrantMemberAddedEvent(context.Background(), &project.NewAggregate("project1", "").Aggregate, @@ -229,7 +176,6 @@ func TestCommandSide_AddProjectGrantMember(t *testing.T) { ), ), ), - expectFilter(), expectPush( project.NewProjectGrantMemberAddedEvent(context.Background(), &project.NewAggregate("project1", "").Aggregate, diff --git a/internal/integration/assert.go b/internal/integration/assert.go index 225b3399b4..610c48cf31 100644 --- a/internal/integration/assert.go +++ b/internal/integration/assert.go @@ -4,7 +4,10 @@ import ( "testing" "time" + "github.com/pmezard/go-difflib/difflib" "github.com/stretchr/testify/assert" + "google.golang.org/protobuf/encoding/protojson" + "google.golang.org/protobuf/proto" "google.golang.org/protobuf/types/known/timestamppb" object "github.com/zitadel/zitadel/pkg/grpc/object/v2beta" @@ -71,3 +74,36 @@ func AssertListDetails[D ListDetailsMsg](t testing.TB, expected, actual D) { assert.WithinRange(t, gotCD, wantCD.Add(-time.Minute), wantCD.Add(time.Minute)) } } + +// EqualProto is inspired by [assert.Equal], only that it tests equality of a proto message. +// A message diff is printed on the error test log if the messages are not equal. +// +// As [assert.Equal] is based on reflection, comparing 2 proto messages sometimes fails, +// due to their internal state. +// Expected messages are usually with a vanilla state, eg only exported fields contain data. +// Actual messages obtained from the gRPC client had unexported fields with data. +// This makes them hard to compare. +func EqualProto(t testing.TB, expected, actual proto.Message) bool { + t.Helper() + if proto.Equal(expected, actual) { + return true + } + t.Errorf("Proto messages not equal: %s", diffProto(expected, actual)) + return false +} + +func diffProto(expected, actual proto.Message) string { + diff, err := difflib.GetUnifiedDiffString(difflib.UnifiedDiff{ + A: difflib.SplitLines(protojson.Format(expected)), + B: difflib.SplitLines(protojson.Format(actual)), + FromFile: "Expected", + FromDate: "", + ToFile: "Actual", + ToDate: "", + Context: 1, + }) + if err != nil { + panic(err) + } + return "\n\nDiff:\n" + diff +}