From 5627daf57180ae034b5c316a3ecf2fbc95042756 Mon Sep 17 00:00:00 2001 From: Miguel Martinez Trivino Date: Sun, 18 Feb 2024 21:46:22 +0100 Subject: [PATCH 1/7] feat(org): generate random name Signed-off-by: Miguel Martinez Trivino --- app/controlplane/internal/biz/biz.go | 26 ++++++++++- app/controlplane/internal/biz/errors.go | 2 + app/controlplane/internal/biz/organization.go | 45 +++++++++++++++---- .../internal/data/organization.go | 4 +- app/controlplane/internal/service/auth.go | 2 +- 5 files changed, 67 insertions(+), 12 deletions(-) diff --git a/app/controlplane/internal/biz/biz.go b/app/controlplane/internal/biz/biz.go index 4b397b6a4..0b0266d13 100644 --- a/app/controlplane/internal/biz/biz.go +++ b/app/controlplane/internal/biz/biz.go @@ -15,7 +15,15 @@ package biz -import "github.com/google/wire" +import ( + "crypto/rand" + "fmt" + "math/big" + "strings" + + "github.com/google/wire" + "github.com/moby/moby/pkg/namesgenerator" +) // ProviderSet is biz providers. var ProviderSet = wire.NewSet( @@ -42,3 +50,19 @@ var ProviderSet = wire.NewSet( wire.Struct(new(NewIntegrationUseCaseOpts), "*"), wire.Struct(new(NewUserUseCaseParams), "*"), ) + +// generate a DNS1123-valid random name using moby's namesgenerator +// plus an additional random number +func generateRandomName() (string, error) { + // Create a random name + name := namesgenerator.GetRandomName(0) + // and append a random number to it + randomNumber, err := rand.Int(rand.Reader, big.NewInt(1000)) + if err != nil { + return "", fmt.Errorf("failed to generate random number: %w", err) + } + + // Replace underscores with dashes to make it compatible with DNS1123 + name = strings.ReplaceAll(fmt.Sprintf("%s-%d", name, randomNumber), "_", "-") + return name, nil +} diff --git a/app/controlplane/internal/biz/errors.go b/app/controlplane/internal/biz/errors.go index b4e850104..b6e0ab071 100644 --- a/app/controlplane/internal/biz/errors.go +++ b/app/controlplane/internal/biz/errors.go @@ -20,6 +20,8 @@ import ( "fmt" ) +var ErrAlreadyExists = errors.New("duplicate") + type ErrNotFound struct { entity string } diff --git a/app/controlplane/internal/biz/organization.go b/app/controlplane/internal/biz/organization.go index 17924ace0..faab4102f 100644 --- a/app/controlplane/internal/biz/organization.go +++ b/app/controlplane/internal/biz/organization.go @@ -19,12 +19,10 @@ import ( "context" "errors" "fmt" - "strings" "time" "github.com/go-kratos/kratos/v2/log" "github.com/google/uuid" - "github.com/moby/moby/pkg/namesgenerator" "k8s.io/apimachinery/pkg/util/validation" ) @@ -57,19 +55,48 @@ func NewOrganizationUseCase(repo OrganizationRepo, repoUC *CASBackendUseCase, iU } } -func (uc *OrganizationUseCase) Create(ctx context.Context, name string) (*Organization, error) { - // Create a random name if none is provided - if name == "" { - name = namesgenerator.GetRandomName(0) - // Replace underscores with dashes to make it compatible with DNS1123 - name = strings.ReplaceAll(name, "_", "-") +func (uc *OrganizationUseCase) CreateWithRandomName(ctx context.Context) (*Organization, error) { + // Try 5 times to create a random name + for i := 0; i < 5; i++ { + // Create a random name + name, err := generateRandomName() + if err != nil { + return nil, fmt.Errorf("failed to generate random name: %w", err) + } + + org, err := uc.Create(ctx, name) + if err != nil { + // We retry if the organization already exists + if errors.Is(err, ErrAlreadyExists) { + uc.logger.Debugw("msg", "Org exists!", "name", name) + continue + } + return nil, err + } + + return org, nil } + return nil, errors.New("failed to create a random organization name") +} + +func (uc *OrganizationUseCase) Create(ctx context.Context, name string) (*Organization, error) { + uc.logger.Infow("msg", "Creating organization", "name", name) + if err := validateOrgName(name); err != nil { return nil, NewErrValidation(fmt.Errorf("invalid organization name: %w", err)) } - return uc.orgRepo.Create(ctx, name) + org, err := uc.orgRepo.Create(ctx, name) + if err != nil { + if errors.Is(err, ErrAlreadyExists) { + return nil, NewErrValidationStr("organization already exists") + } + + return nil, fmt.Errorf("failed to create organization: %w", err) + } + + return org, nil } func validateOrgName(name string) error { diff --git a/app/controlplane/internal/data/organization.go b/app/controlplane/internal/data/organization.go index f941d63bd..9f5606798 100644 --- a/app/controlplane/internal/data/organization.go +++ b/app/controlplane/internal/data/organization.go @@ -41,7 +41,9 @@ func (r *OrganizationRepo) Create(ctx context.Context, name string) (*biz.Organi org, err := r.data.db.Organization.Create(). SetName(name). Save(ctx) - if err != nil { + if err != nil && ent.IsConstraintError(err) { + return nil, biz.ErrAlreadyExists + } else if err != nil { return nil, err } diff --git a/app/controlplane/internal/service/auth.go b/app/controlplane/internal/service/auth.go index d1680e2b2..b6805f802 100644 --- a/app/controlplane/internal/service/auth.go +++ b/app/controlplane/internal/service/auth.go @@ -214,7 +214,7 @@ func callbackHandler(svc *AuthService, w http.ResponseWriter, r *http.Request) ( // If there is not, we create it and associate the user to it if currentOrg == nil { // Create an org - currentOrg, err = svc.orgUseCase.Create(ctx, "") + currentOrg, err = svc.orgUseCase.CreateWithRandomName(ctx) if err != nil { return http.StatusInternalServerError, sl.LogAndMaskErr(err, svc.log) } From 493ef1c3a18314d2a3d482bb8e53c133ec5f2bed Mon Sep 17 00:00:00 2001 From: Miguel Martinez Trivino Date: Sun, 18 Feb 2024 16:07:06 +0100 Subject: [PATCH 2/7] feat(controlplane): unique name Signed-off-by: Miguel Martinez Trivino --- .../ent/migrate/migrations/20240218095416.sql | 28 +++++++++++++++++++ .../data/ent/migrate/migrations/atlas.sum | 3 +- .../internal/data/ent/migrate/schema.go | 2 +- .../data/ent/organization/organization.go | 2 -- .../internal/data/ent/organization_create.go | 12 -------- .../internal/data/ent/organization_update.go | 16 ----------- app/controlplane/internal/data/ent/runtime.go | 4 --- .../internal/data/ent/schema/organization.go | 2 +- 8 files changed, 32 insertions(+), 37 deletions(-) create mode 100644 app/controlplane/internal/data/ent/migrate/migrations/20240218095416.sql diff --git a/app/controlplane/internal/data/ent/migrate/migrations/20240218095416.sql b/app/controlplane/internal/data/ent/migrate/migrations/20240218095416.sql new file mode 100644 index 000000000..a3c5194da --- /dev/null +++ b/app/controlplane/internal/data/ent/migrate/migrations/20240218095416.sql @@ -0,0 +1,28 @@ +-- Update existing data in "organizations" table +-- Make the content RFC 1123 compliant +UPDATE organizations +SET name = regexp_replace( + lower(name), + '[^a-z0-9-]', + '-', + 'g' + ); + +-- Append suffixes to duplicates +WITH numbered_names AS ( + SELECT + id, + name, + ROW_NUMBER() OVER (PARTITION BY name ORDER BY id) AS rn + FROM organizations +) +UPDATE organizations AS o +SET name = CONCAT(o.name, '-', nn.rn - 1) +FROM numbered_names AS nn +WHERE o.id = nn.id AND nn.rn > 1; + +-- Modify "organizations" table +ALTER TABLE "organizations" ALTER COLUMN "name" DROP DEFAULT; + +-- Create index "organizations_name_key" to table: "organizations" +CREATE UNIQUE INDEX "organizations_name_key" ON "organizations" ("name"); diff --git a/app/controlplane/internal/data/ent/migrate/migrations/atlas.sum b/app/controlplane/internal/data/ent/migrate/migrations/atlas.sum index b59711912..974d75cc2 100644 --- a/app/controlplane/internal/data/ent/migrate/migrations/atlas.sum +++ b/app/controlplane/internal/data/ent/migrate/migrations/atlas.sum @@ -1,4 +1,4 @@ -h1:s+xNGo/bDICYTLPzV2msNFN0lBHjvNEVuKKP1zNVKd0= +h1:vTsHuB9AeHVCvbjVI+WEKhyysLD5ZDyw2NyYkcV/4Xw= 20230706165452_init-schema.sql h1:VvqbNFEQnCvUVyj2iDYVQQxDM0+sSXqocpt/5H64k8M= 20230710111950-cas-backend.sql h1:A8iBuSzZIEbdsv9ipBtscZQuaBp3V5/VMw7eZH6GX+g= 20230712094107-cas-backends-workflow-runs.sql h1:a5rzxpVGyd56nLRSsKrmCFc9sebg65RWzLghKHh5xvI= @@ -18,3 +18,4 @@ h1:s+xNGo/bDICYTLPzV2msNFN0lBHjvNEVuKKP1zNVKd0= 20231204210217.sql h1:7sZmEr3PAJ5jmuNRk0vxbhZ/eLKIm95r5hNJOQ4bRps= 20231217154320.sql h1:D+JCkv64OJRWsdaBz2enuVE3DcF3omhjTaInIWAYrtw= 20240209150351.sql h1:QQxdcJ67KEYtQkABI4qJZwC8LsTG+r2n8kUmZjMT5+E= +20240218095416.sql h1:Q+LNC5+jdGRMVlDlZAHe1UWsXMAQC6QtI/R3U7Gc0NA= diff --git a/app/controlplane/internal/data/ent/migrate/schema.go b/app/controlplane/internal/data/ent/migrate/schema.go index 5227567af..a94945039 100644 --- a/app/controlplane/internal/data/ent/migrate/schema.go +++ b/app/controlplane/internal/data/ent/migrate/schema.go @@ -225,7 +225,7 @@ var ( // OrganizationsColumns holds the columns for the "organizations" table. OrganizationsColumns = []*schema.Column{ {Name: "id", Type: field.TypeUUID, Unique: true}, - {Name: "name", Type: field.TypeString, Default: "default"}, + {Name: "name", Type: field.TypeString, Unique: true}, {Name: "created_at", Type: field.TypeTime, Default: "CURRENT_TIMESTAMP"}, } // OrganizationsTable holds the schema information for the "organizations" table. diff --git a/app/controlplane/internal/data/ent/organization/organization.go b/app/controlplane/internal/data/ent/organization/organization.go index 5c1ccaf97..e9e6e4bbc 100644 --- a/app/controlplane/internal/data/ent/organization/organization.go +++ b/app/controlplane/internal/data/ent/organization/organization.go @@ -86,8 +86,6 @@ func ValidColumn(column string) bool { } var ( - // DefaultName holds the default value on creation for the "name" field. - DefaultName string // DefaultCreatedAt holds the default value on creation for the "created_at" field. DefaultCreatedAt func() time.Time // DefaultID holds the default value on creation for the "id" field. diff --git a/app/controlplane/internal/data/ent/organization_create.go b/app/controlplane/internal/data/ent/organization_create.go index 74c3634c3..51a30af79 100644 --- a/app/controlplane/internal/data/ent/organization_create.go +++ b/app/controlplane/internal/data/ent/organization_create.go @@ -32,14 +32,6 @@ func (oc *OrganizationCreate) SetName(s string) *OrganizationCreate { return oc } -// SetNillableName sets the "name" field if the given value is not nil. -func (oc *OrganizationCreate) SetNillableName(s *string) *OrganizationCreate { - if s != nil { - oc.SetName(*s) - } - return oc -} - // SetCreatedAt sets the "created_at" field. func (oc *OrganizationCreate) SetCreatedAt(t time.Time) *OrganizationCreate { oc.mutation.SetCreatedAt(t) @@ -178,10 +170,6 @@ func (oc *OrganizationCreate) ExecX(ctx context.Context) { // defaults sets the default values of the builder before save. func (oc *OrganizationCreate) defaults() { - if _, ok := oc.mutation.Name(); !ok { - v := organization.DefaultName - oc.mutation.SetName(v) - } if _, ok := oc.mutation.CreatedAt(); !ok { v := organization.DefaultCreatedAt() oc.mutation.SetCreatedAt(v) diff --git a/app/controlplane/internal/data/ent/organization_update.go b/app/controlplane/internal/data/ent/organization_update.go index a2ab8e2e3..f8d14f356 100644 --- a/app/controlplane/internal/data/ent/organization_update.go +++ b/app/controlplane/internal/data/ent/organization_update.go @@ -39,14 +39,6 @@ func (ou *OrganizationUpdate) SetName(s string) *OrganizationUpdate { return ou } -// SetNillableName sets the "name" field if the given value is not nil. -func (ou *OrganizationUpdate) SetNillableName(s *string) *OrganizationUpdate { - if s != nil { - ou.SetName(*s) - } - return ou -} - // AddMembershipIDs adds the "memberships" edge to the Membership entity by IDs. func (ou *OrganizationUpdate) AddMembershipIDs(ids ...uuid.UUID) *OrganizationUpdate { ou.mutation.AddMembershipIDs(ids...) @@ -522,14 +514,6 @@ func (ouo *OrganizationUpdateOne) SetName(s string) *OrganizationUpdateOne { return ouo } -// SetNillableName sets the "name" field if the given value is not nil. -func (ouo *OrganizationUpdateOne) SetNillableName(s *string) *OrganizationUpdateOne { - if s != nil { - ouo.SetName(*s) - } - return ouo -} - // AddMembershipIDs adds the "memberships" edge to the Membership entity by IDs. func (ouo *OrganizationUpdateOne) AddMembershipIDs(ids ...uuid.UUID) *OrganizationUpdateOne { ouo.mutation.AddMembershipIDs(ids...) diff --git a/app/controlplane/internal/data/ent/runtime.go b/app/controlplane/internal/data/ent/runtime.go index 4fc7cc3b7..72c974eb3 100644 --- a/app/controlplane/internal/data/ent/runtime.go +++ b/app/controlplane/internal/data/ent/runtime.go @@ -120,10 +120,6 @@ func init() { orginvitation.DefaultID = orginvitationDescID.Default.(func() uuid.UUID) organizationFields := schema.Organization{}.Fields() _ = organizationFields - // organizationDescName is the schema descriptor for name field. - organizationDescName := organizationFields[1].Descriptor() - // organization.DefaultName holds the default value on creation for the name field. - organization.DefaultName = organizationDescName.Default.(string) // organizationDescCreatedAt is the schema descriptor for created_at field. organizationDescCreatedAt := organizationFields[2].Descriptor() // organization.DefaultCreatedAt holds the default value on creation for the created_at field. diff --git a/app/controlplane/internal/data/ent/schema/organization.go b/app/controlplane/internal/data/ent/schema/organization.go index 54cacebec..a382495a0 100644 --- a/app/controlplane/internal/data/ent/schema/organization.go +++ b/app/controlplane/internal/data/ent/schema/organization.go @@ -34,7 +34,7 @@ type Organization struct { func (Organization) Fields() []ent.Field { return []ent.Field{ field.UUID("id", uuid.UUID{}).Default(uuid.New).Unique(), - field.String("name").Default("default"), + field.String("name").Unique(), field.Time("created_at"). Default(time.Now). Immutable(). From 1977d99ceab6c9a2519c33d420f7e68878c85666 Mon Sep 17 00:00:00 2001 From: Miguel Martinez Trivino Date: Mon, 19 Feb 2024 08:49:02 +0100 Subject: [PATCH 3/7] fix tests Signed-off-by: Miguel Martinez Trivino --- .../internal/biz/membership_integration_test.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/app/controlplane/internal/biz/membership_integration_test.go b/app/controlplane/internal/biz/membership_integration_test.go index 31456f575..ac466c3b8 100644 --- a/app/controlplane/internal/biz/membership_integration_test.go +++ b/app/controlplane/internal/biz/membership_integration_test.go @@ -100,7 +100,7 @@ func (s *membershipIntegrationTestSuite) TestCreateMembership() { assert.NoError(err) s.T().Run("Create default", func(t *testing.T) { - org, err := s.Organization.Create(ctx, "foo") + org, err := s.Organization.CreateWithRandomName(ctx) assert.NoError(err) m, err := s.Membership.Create(ctx, org.ID, user.ID, true) @@ -119,7 +119,7 @@ func (s *membershipIntegrationTestSuite) TestCreateMembership() { }) s.T().Run("Non current", func(t *testing.T) { - org, err := s.Organization.Create(ctx, "foo") + org, err := s.Organization.CreateWithRandomName(ctx) assert.NoError(err) m, err := s.Membership.Create(ctx, org.ID, user.ID, false) @@ -134,7 +134,7 @@ func (s *membershipIntegrationTestSuite) TestCreateMembership() { }) s.T().Run("Invalid User", func(t *testing.T) { - org, err := s.Organization.Create(ctx, "foo") + org, err := s.Organization.CreateWithRandomName(ctx) assert.NoError(err) m, err := s.Membership.Create(ctx, org.ID, uuid.NewString(), false) assert.Error(err) From ed56c5bd0eb181803a39ce90e9d2a398120a3327 Mon Sep 17 00:00:00 2001 From: Miguel Martinez Trivino Date: Mon, 19 Feb 2024 12:04:11 +0100 Subject: [PATCH 4/7] fix tests Signed-off-by: Miguel Martinez Trivino --- .../internal/biz/organization_integration_test.go | 4 +--- app/controlplane/internal/biz/workflow_integration_test.go | 2 +- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/app/controlplane/internal/biz/organization_integration_test.go b/app/controlplane/internal/biz/organization_integration_test.go index d5de2772e..4840704c9 100644 --- a/app/controlplane/internal/biz/organization_integration_test.go +++ b/app/controlplane/internal/biz/organization_integration_test.go @@ -43,8 +43,6 @@ func (s *OrgIntegrationTestSuite) TestCreate() { name string expectedError bool }{ - // autogenerated name - {"", false}, {"a", false}, {"aa-aa", false}, {"-aaa", true}, @@ -96,7 +94,7 @@ func (s *OrgIntegrationTestSuite) TestUpdate() { }) s.T().Run("org not accessible to user", func(t *testing.T) { - org2, err := s.Organization.Create(ctx, "testing-org") + org2, err := s.Organization.CreateWithRandomName(ctx) require.NoError(s.T(), err) _, err = s.Organization.Update(ctx, s.user.ID, org2.ID, nil) s.Error(err) diff --git a/app/controlplane/internal/biz/workflow_integration_test.go b/app/controlplane/internal/biz/workflow_integration_test.go index bbb1d8563..7c6825644 100644 --- a/app/controlplane/internal/biz/workflow_integration_test.go +++ b/app/controlplane/internal/biz/workflow_integration_test.go @@ -38,7 +38,7 @@ func (s *workflowIntegrationTestSuite) TestUpdate() { project = "test project" ) - org2, err := s.Organization.Create(context.Background(), "testing-org") + org2, err := s.Organization.CreateWithRandomName(context.Background()) require.NoError(s.T(), err) workflow, err := s.Workflow.Create(ctx, &biz.WorkflowCreateOpts{Name: name, OrgID: s.org.ID}) require.NoError(s.T(), err) From fc0be92973e7a4950b1d3d971a78cdd66ed901ce Mon Sep 17 00:00:00 2001 From: Miguel Martinez Trivino Date: Mon, 19 Feb 2024 12:10:13 +0100 Subject: [PATCH 5/7] fix tests Signed-off-by: Miguel Martinez Trivino --- app/controlplane/internal/biz/biz.go | 2 +- app/controlplane/internal/biz/organization.go | 4 ++-- .../internal/biz/organization_integration_test.go | 10 +++++++++- 3 files changed, 12 insertions(+), 4 deletions(-) diff --git a/app/controlplane/internal/biz/biz.go b/app/controlplane/internal/biz/biz.go index 0b0266d13..53b05334f 100644 --- a/app/controlplane/internal/biz/biz.go +++ b/app/controlplane/internal/biz/biz.go @@ -57,7 +57,7 @@ func generateRandomName() (string, error) { // Create a random name name := namesgenerator.GetRandomName(0) // and append a random number to it - randomNumber, err := rand.Int(rand.Reader, big.NewInt(1000)) + randomNumber, err := rand.Int(rand.Reader, big.NewInt(10000)) if err != nil { return "", fmt.Errorf("failed to generate random number: %w", err) } diff --git a/app/controlplane/internal/biz/organization.go b/app/controlplane/internal/biz/organization.go index faab4102f..4cf20b693 100644 --- a/app/controlplane/internal/biz/organization.go +++ b/app/controlplane/internal/biz/organization.go @@ -56,8 +56,8 @@ func NewOrganizationUseCase(repo OrganizationRepo, repoUC *CASBackendUseCase, iU } func (uc *OrganizationUseCase) CreateWithRandomName(ctx context.Context) (*Organization, error) { - // Try 5 times to create a random name - for i := 0; i < 5; i++ { + // Try 10 times to create a random name + for i := 0; i < 10; i++ { // Create a random name name, err := generateRandomName() if err != nil { diff --git a/app/controlplane/internal/biz/organization_integration_test.go b/app/controlplane/internal/biz/organization_integration_test.go index 4840704c9..e5f4ae48e 100644 --- a/app/controlplane/internal/biz/organization_integration_test.go +++ b/app/controlplane/internal/biz/organization_integration_test.go @@ -35,7 +35,15 @@ import ( "github.com/stretchr/testify/suite" ) -// and delete cascades that we want to validate that they work too +func (s *OrgIntegrationTestSuite) TestCreateWithRandomName() { + // It can create thousands of orgs without any problem + for i := 0; i < 1000; i++ { + org, err := s.Organization.CreateWithRandomName(context.Background()) + s.NoError(err) + s.NotNil(org) + } +} + func (s *OrgIntegrationTestSuite) TestCreate() { ctx := context.Background() From e3762416775dea0340d533e3265139c7d26f59a2 Mon Sep 17 00:00:00 2001 From: Miguel Martinez Trivino Date: Mon, 19 Feb 2024 12:14:55 +0100 Subject: [PATCH 6/7] fix tests Signed-off-by: Miguel Martinez Trivino --- app/controlplane/internal/biz/apitoken_integration_test.go | 6 +++--- .../internal/biz/membership_integration_test.go | 4 ++-- app/controlplane/internal/biz/referrer_integration_test.go | 4 ++-- app/controlplane/internal/biz/user_integration_test.go | 4 ++-- app/controlplane/internal/biz/workflow_integration_test.go | 2 +- app/controlplane/internal/dispatcher/dispatcher_test.go | 4 ++-- 6 files changed, 12 insertions(+), 12 deletions(-) diff --git a/app/controlplane/internal/biz/apitoken_integration_test.go b/app/controlplane/internal/biz/apitoken_integration_test.go index 1e3cabbba..bc2e33125 100644 --- a/app/controlplane/internal/biz/apitoken_integration_test.go +++ b/app/controlplane/internal/biz/apitoken_integration_test.go @@ -161,7 +161,7 @@ func (s *apiTokenTestSuite) TestList() { }) s.T().Run("returns empty list", func(t *testing.T) { - emptyOrg, err := s.Organization.Create(ctx, "org1") + emptyOrg, err := s.Organization.CreateWithRandomName(ctx) require.NoError(s.T(), err) tokens, err := s.APIToken.List(ctx, emptyOrg.ID, false) s.NoError(err) @@ -240,9 +240,9 @@ func (s *apiTokenTestSuite) SetupTest() { ctx := context.Background() s.TestingUseCases = testhelpers.NewTestingUseCases(t) - s.org, err = s.Organization.Create(ctx, "org1") + s.org, err = s.Organization.CreateWithRandomName(ctx) assert.NoError(err) - s.org2, err = s.Organization.Create(ctx, "org2") + s.org2, err = s.Organization.CreateWithRandomName(ctx) assert.NoError(err) // Create 2 tokens for org 1 diff --git a/app/controlplane/internal/biz/membership_integration_test.go b/app/controlplane/internal/biz/membership_integration_test.go index ac466c3b8..8f67cff08 100644 --- a/app/controlplane/internal/biz/membership_integration_test.go +++ b/app/controlplane/internal/biz/membership_integration_test.go @@ -34,9 +34,9 @@ func (s *membershipIntegrationTestSuite) TestDeleteWithOrg() { s.NoError(err) user2, err := s.User.FindOrCreateByEmail(ctx, "foo-2@test.com") s.NoError(err) - userOrg, err := s.Organization.Create(ctx, "foo") + userOrg, err := s.Organization.CreateWithRandomName(ctx) s.NoError(err) - sharedOrg, err := s.Organization.Create(ctx, "shared-org") + sharedOrg, err := s.Organization.CreateWithRandomName(ctx) s.NoError(err) mUser, err := s.Membership.Create(ctx, userOrg.ID, user.ID, true) diff --git a/app/controlplane/internal/biz/referrer_integration_test.go b/app/controlplane/internal/biz/referrer_integration_test.go index 8fa7798b5..ed8b33821 100644 --- a/app/controlplane/internal/biz/referrer_integration_test.go +++ b/app/controlplane/internal/biz/referrer_integration_test.go @@ -359,9 +359,9 @@ func (s *referrerIntegrationTestSuite) SetupTest() { ctx := context.Background() var err error - s.org1, err = s.Organization.Create(ctx, "testing-org") + s.org1, err = s.Organization.CreateWithRandomName(ctx) require.NoError(s.T(), err) - s.org2, err = s.Organization.Create(ctx, "testing-org-2") + s.org2, err = s.Organization.CreateWithRandomName(ctx) require.NoError(s.T(), err) s.org1UUID, err = uuid.Parse(s.org1.ID) diff --git a/app/controlplane/internal/biz/user_integration_test.go b/app/controlplane/internal/biz/user_integration_test.go index fcd2ddaab..fcba90ed3 100644 --- a/app/controlplane/internal/biz/user_integration_test.go +++ b/app/controlplane/internal/biz/user_integration_test.go @@ -78,9 +78,9 @@ func (s *userIntegrationTestSuite) SetupTest() { s.TestingUseCases = testhelpers.NewTestingUseCases(t) - s.userOneOrg, err = s.Organization.Create(ctx, "user-1-org") + s.userOneOrg, err = s.Organization.CreateWithRandomName(ctx) assert.NoError(err) - s.sharedOrg, err = s.Organization.Create(ctx, "shared-org") + s.sharedOrg, err = s.Organization.CreateWithRandomName(ctx) assert.NoError(err) // Create User 1 diff --git a/app/controlplane/internal/biz/workflow_integration_test.go b/app/controlplane/internal/biz/workflow_integration_test.go index 7c6825644..ae2145e41 100644 --- a/app/controlplane/internal/biz/workflow_integration_test.go +++ b/app/controlplane/internal/biz/workflow_integration_test.go @@ -148,7 +148,7 @@ func (s *workflowIntegrationTestSuite) SetupTest() { s.TestingUseCases = testhelpers.NewTestingUseCases(s.T()) ctx := context.Background() - s.org, err = s.Organization.Create(ctx, "testing-org") + s.org, err = s.Organization.CreateWithRandomName(ctx) assert.NoError(err) } diff --git a/app/controlplane/internal/dispatcher/dispatcher_test.go b/app/controlplane/internal/dispatcher/dispatcher_test.go index 292a8b7f0..47b7ee145 100644 --- a/app/controlplane/internal/dispatcher/dispatcher_test.go +++ b/app/controlplane/internal/dispatcher/dispatcher_test.go @@ -213,11 +213,11 @@ func (s *dispatcherTestSuite) SetupTest() { s.TestingUseCases = testhelpers.NewTestingUseCases(s.T()) // Create org, integration and oci repository - s.org, err = s.Organization.Create(ctx, "testing-org") + s.org, err = s.Organization.CreateWithRandomName(ctx) assert.NoError(s.T(), err) // Create org, integration and oci repository - s.emptyOrg, err = s.Organization.Create(ctx, "empty-org") + s.emptyOrg, err = s.Organization.CreateWithRandomName(ctx) assert.NoError(s.T(), err) // Workflow From 0de237f34e624f080cca04e8a47d6199a61649c4 Mon Sep 17 00:00:00 2001 From: Miguel Martinez Trivino Date: Mon, 19 Feb 2024 12:52:52 +0100 Subject: [PATCH 7/7] fix tests Signed-off-by: Miguel Martinez Trivino --- app/controlplane/internal/biz/organization.go | 31 +++++++++++----- .../internal/biz/organization_test.go | 35 +++++++++++++++++-- 2 files changed, 55 insertions(+), 11 deletions(-) diff --git a/app/controlplane/internal/biz/organization.go b/app/controlplane/internal/biz/organization.go index 4cf20b693..13e3dbe22 100644 --- a/app/controlplane/internal/biz/organization.go +++ b/app/controlplane/internal/biz/organization.go @@ -55,22 +55,25 @@ func NewOrganizationUseCase(repo OrganizationRepo, repoUC *CASBackendUseCase, iU } } +const OrganizationRandomNameMaxTries = 10 + func (uc *OrganizationUseCase) CreateWithRandomName(ctx context.Context) (*Organization, error) { // Try 10 times to create a random name - for i := 0; i < 10; i++ { + for i := 0; i < OrganizationRandomNameMaxTries; i++ { // Create a random name name, err := generateRandomName() if err != nil { return nil, fmt.Errorf("failed to generate random name: %w", err) } - org, err := uc.Create(ctx, name) + org, err := uc.doCreate(ctx, name) if err != nil { // We retry if the organization already exists if errors.Is(err, ErrAlreadyExists) { uc.logger.Debugw("msg", "Org exists!", "name", name) continue } + uc.logger.Debugw("msg", "BOOM", "name", name, "err", err) return nil, err } @@ -80,26 +83,36 @@ func (uc *OrganizationUseCase) CreateWithRandomName(ctx context.Context) (*Organ return nil, errors.New("failed to create a random organization name") } +// Create an organization with the given name func (uc *OrganizationUseCase) Create(ctx context.Context, name string) (*Organization, error) { + org, err := uc.doCreate(ctx, name) + if err != nil { + if errors.Is(err, ErrAlreadyExists) { + return nil, NewErrValidationStr("organization already exists") + } + + return nil, fmt.Errorf("failed to create organization: %w", err) + } + + return org, nil +} + +func (uc *OrganizationUseCase) doCreate(ctx context.Context, name string) (*Organization, error) { uc.logger.Infow("msg", "Creating organization", "name", name) - if err := validateOrgName(name); err != nil { + if err := ValidateOrgName(name); err != nil { return nil, NewErrValidation(fmt.Errorf("invalid organization name: %w", err)) } org, err := uc.orgRepo.Create(ctx, name) if err != nil { - if errors.Is(err, ErrAlreadyExists) { - return nil, NewErrValidationStr("organization already exists") - } - return nil, fmt.Errorf("failed to create organization: %w", err) } return org, nil } -func validateOrgName(name string) error { +func ValidateOrgName(name string) error { // The same validation done by Kubernetes for their namespace name // https://github.com/kubernetes/apimachinery/blob/fa98d6eaedb4caccd69fc07d90bbb6a1e551f00f/pkg/api/validation/generic.go#L63 err := validation.IsDNS1123Label(name) @@ -128,7 +141,7 @@ func (uc *OrganizationUseCase) Update(ctx context.Context, userID, orgID string, // We validate the name to get ready for the name to become identifiers if name != nil { - if err := validateOrgName(*name); err != nil { + if err := ValidateOrgName(*name); err != nil { return nil, NewErrValidation(fmt.Errorf("invalid organization name: %w", err)) } } diff --git a/app/controlplane/internal/biz/organization_test.go b/app/controlplane/internal/biz/organization_test.go index 6312579b6..3b2b7c9b4 100644 --- a/app/controlplane/internal/biz/organization_test.go +++ b/app/controlplane/internal/biz/organization_test.go @@ -13,11 +13,17 @@ // See the License for the specific language governing permissions and // limitations under the License. -package biz +package biz_test import ( + "context" + "io" "testing" + "github.com/chainloop-dev/chainloop/app/controlplane/internal/biz" + repoM "github.com/chainloop-dev/chainloop/app/controlplane/internal/biz/mocks" + "github.com/go-kratos/kratos/v2/log" + "github.com/stretchr/testify/mock" "github.com/stretchr/testify/suite" ) @@ -25,6 +31,31 @@ type organizationTestSuite struct { suite.Suite } +func (s *organizationTestSuite) TestCreateWithRandomName() { + repo := repoM.NewOrganizationRepo(s.T()) + uc := biz.NewOrganizationUseCase(repo, nil, nil, nil, log.NewStdLogger(io.Discard)) + + s.Run("the org exists, we retry", func() { + ctx := context.Background() + // the first one fails because it already exists + repo.On("Create", ctx, mock.Anything).Once().Return(nil, biz.ErrAlreadyExists) + // but the second call creates the org + repo.On("Create", ctx, mock.Anything).Once().Return(&biz.Organization{Name: "foobar"}, nil) + got, err := uc.CreateWithRandomName(ctx) + s.NoError(err) + s.Equal("foobar", got.Name) + }) + + s.Run("if it runs out of tries, it fails", func() { + ctx := context.Background() + // the first one fails because it already exists + repo.On("Create", ctx, mock.Anything).Times(biz.OrganizationRandomNameMaxTries).Return(nil, biz.ErrAlreadyExists) + got, err := uc.CreateWithRandomName(ctx) + s.Error(err) + s.Nil(got) + }) +} + func (s *organizationTestSuite) TestValidateOrgName() { testCases := []struct { name string @@ -47,7 +78,7 @@ func (s *organizationTestSuite) TestValidateOrgName() { for _, tc := range testCases { s.T().Run(tc.name, func(t *testing.T) { - err := validateOrgName(tc.name) + err := biz.ValidateOrgName(tc.name) if tc.expectedError { s.Error(err) } else {