Skip to content

Commit 01ad13a

Browse files
migmartrimigmartri
authored
feat(controlplane): unique workflow name and formatted project (#605)
Signed-off-by: Miguel Martinez Trivino <[email protected]>
1 parent d2b9803 commit 01ad13a

File tree

10 files changed

+244
-30
lines changed

10 files changed

+244
-30
lines changed

‎app/controlplane/internal/biz/biz.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -78,7 +78,7 @@ func ValidateIsDNS1123(name string) error {
7878
if len(err) > 0 {
7979
errMsg := ""
8080
for _, e := range err {
81-
errMsg += e + "\n"
81+
errMsg += fmt.Sprintf("%q: %s\n", name, e)
8282
}
8383

8484
return errors.New(errMsg)

‎app/controlplane/internal/biz/casmapping_integration_test.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -355,7 +355,7 @@ func (s *casMappingIntegrationSuite) SetupTest() {
355355
workflow, err := s.Workflow.Create(ctx, &biz.WorkflowCreateOpts{Name: "test-workflow", OrgID: s.org1.ID})
356356
assert.NoError(err)
357357

358-
publicWorkflow, err := s.Workflow.Create(ctx, &biz.WorkflowCreateOpts{Name: "test-workflow", OrgID: s.org1.ID, Public: true})
358+
publicWorkflow, err := s.Workflow.Create(ctx, &biz.WorkflowCreateOpts{Name: "test-workflow-public", OrgID: s.org1.ID, Public: true})
359359
assert.NoError(err)
360360

361361
// Robot account

‎app/controlplane/internal/biz/workflow.go‎

Lines changed: 44 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -77,17 +77,43 @@ func (uc *WorkflowUseCase) Create(ctx context.Context, opts *WorkflowCreateOpts)
7777
return nil, errors.New("workflow name is required")
7878
}
7979

80+
// validate format of the name and the project
81+
if err := ValidateIsDNS1123(opts.Name); err != nil {
82+
return nil, NewErrValidation(err)
83+
}
84+
85+
if opts.Project != "" {
86+
if err := ValidateIsDNS1123(opts.Project); err != nil {
87+
return nil, NewErrValidation(err)
88+
}
89+
}
90+
8091
contract, err := uc.findOrCreateContract(ctx, opts.OrgID, opts.ContractID, opts.Project, opts.Name)
8192
if err != nil {
8293
return nil, err
94+
} else if contract == nil {
95+
return nil, NewErrNotFound("contract")
8396
}
8497

8598
// Set the potential new schemaID
8699
opts.ContractID = contract.ID.String()
87-
return uc.wfRepo.Create(ctx, opts)
100+
wf, err := uc.wfRepo.Create(ctx, opts)
101+
if err != nil {
102+
if errors.Is(err, ErrAlreadyExists) {
103+
return nil, NewErrValidationStr("name already taken")
104+
}
105+
106+
return nil, fmt.Errorf("failed to create workflow: %w", err)
107+
}
108+
109+
return wf, nil
88110
}
89111

90112
func (uc *WorkflowUseCase) Update(ctx context.Context, orgID, workflowID string, opts *WorkflowUpdateOpts) (*Workflow, error) {
113+
if opts == nil {
114+
return nil, NewErrValidationStr("no updates provided")
115+
}
116+
91117
orgUUID, err := uuid.Parse(orgID)
92118
if err != nil {
93119
return nil, NewErrInvalidUUID(err)
@@ -98,6 +124,19 @@ func (uc *WorkflowUseCase) Update(ctx context.Context, orgID, workflowID string,
98124
return nil, NewErrInvalidUUID(err)
99125
}
100126

127+
if opts.Name != nil {
128+
// validate format of the name and the project
129+
if err := ValidateIsDNS1123(*opts.Name); err != nil {
130+
return nil, NewErrValidation(err)
131+
}
132+
}
133+
134+
if opts.Project != nil && *opts.Project != "" {
135+
if err := ValidateIsDNS1123(*opts.Project); err != nil {
136+
return nil, NewErrValidation(err)
137+
}
138+
}
139+
101140
// make sure that the workflow is for the provided org
102141
if wf, err := uc.wfRepo.GetOrgScoped(ctx, orgUUID, workflowUUID); err != nil {
103142
return nil, err
@@ -107,6 +146,10 @@ func (uc *WorkflowUseCase) Update(ctx context.Context, orgID, workflowID string,
107146

108147
wf, err := uc.wfRepo.Update(ctx, workflowUUID, opts)
109148
if err != nil {
149+
if errors.Is(err, ErrAlreadyExists) {
150+
return nil, NewErrValidationStr("name already taken")
151+
}
152+
110153
return nil, fmt.Errorf("failed to update workflow: %w", err)
111154
} else if wf == nil {
112155
return nil, NewErrNotFound("workflow")

‎app/controlplane/internal/biz/workflow_integration_test.go‎

Lines changed: 120 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
//
2-
// Copyright 2023 The Chainloop Authors.
2+
// Copyright 2024 The Chainloop Authors.
33
//
44
// Licensed under the Apache License, Version 2.0 (the "License");
55
// you may not use this file except in compliance with the License.
@@ -17,6 +17,7 @@ package biz_test
1717

1818
import (
1919
"context"
20+
"fmt"
2021
"testing"
2122
"time"
2223

@@ -54,6 +55,72 @@ func (s *workflowIntegrationTestSuite) TestContractLatestAvailable() {
5455
})
5556
}
5657

58+
func (s *workflowIntegrationTestSuite) TestCreate() {
59+
ctx := context.Background()
60+
testCases := []struct {
61+
name string
62+
opts *biz.WorkflowCreateOpts
63+
wantErrMsg string
64+
}{
65+
{
66+
name: "org missing",
67+
opts: &biz.WorkflowCreateOpts{Name: "name"},
68+
wantErrMsg: "required",
69+
},
70+
{
71+
name: "name missing",
72+
opts: &biz.WorkflowCreateOpts{OrgID: s.org.ID},
73+
wantErrMsg: "required",
74+
},
75+
{
76+
name: "invalid name",
77+
opts: &biz.WorkflowCreateOpts{OrgID: s.org.ID, Name: "this/not/valid"},
78+
wantErrMsg: "RFC 1123",
79+
},
80+
{
81+
name: "another invalid name",
82+
opts: &biz.WorkflowCreateOpts{OrgID: s.org.ID, Name: "this-not Valid"},
83+
wantErrMsg: "RFC 1123",
84+
},
85+
{
86+
name: " invalid project name",
87+
opts: &biz.WorkflowCreateOpts{OrgID: s.org.ID, Name: "valid", Project: "this-not Valid"},
88+
wantErrMsg: "RFC 1123",
89+
},
90+
{
91+
name: "non-existing contract",
92+
opts: &biz.WorkflowCreateOpts{OrgID: s.org.ID, Name: "name", ContractID: uuid.Generate().String()},
93+
wantErrMsg: "not found",
94+
},
95+
{
96+
name: "can create it with just the name and the org",
97+
opts: &biz.WorkflowCreateOpts{OrgID: s.org.ID, Name: "name"},
98+
},
99+
{
100+
name: "with all items",
101+
opts: &biz.WorkflowCreateOpts{OrgID: s.org.ID, Name: "another-name", Project: "project", Team: "team", Description: "description"},
102+
},
103+
}
104+
105+
for _, tc := range testCases {
106+
s.Run(tc.name, func() {
107+
got, err := s.Workflow.Create(ctx, tc.opts)
108+
if tc.wantErrMsg != "" {
109+
s.ErrorContains(err, tc.wantErrMsg)
110+
return
111+
}
112+
113+
require.NoError(s.T(), err)
114+
s.NotEmpty(got.ID)
115+
s.NotEmpty(got.CreatedAt)
116+
s.Equal(tc.opts.Name, got.Name)
117+
s.Equal(tc.opts.Description, got.Description)
118+
s.Equal(tc.opts.Team, got.Team)
119+
s.Equal(tc.opts.Project, got.Project)
120+
})
121+
}
122+
}
123+
57124
func (s *workflowIntegrationTestSuite) TestUpdate() {
58125
ctx := context.Background()
59126
const (
@@ -72,8 +139,15 @@ func (s *workflowIntegrationTestSuite) TestUpdate() {
72139
s.False(workflow.Public)
73140
})
74141

75-
s.Run("can't update a workflow in another org", func() {
142+
s.Run("can't update if no changes are provided", func() {
76143
got, err := s.Workflow.Update(ctx, org2.ID, workflow.ID.String(), nil)
144+
s.True(biz.IsErrValidation(err))
145+
s.Error(err)
146+
s.Nil(got)
147+
})
148+
149+
s.Run("can't update a workflow in another org", func() {
150+
got, err := s.Workflow.Update(ctx, org2.ID, workflow.ID.String(), &biz.WorkflowUpdateOpts{Name: toPtrS("new-name")})
77151
s.True(biz.IsNotFound(err))
78152
s.Error(err)
79153
s.Nil(got)
@@ -82,62 +156,77 @@ func (s *workflowIntegrationTestSuite) TestUpdate() {
82156
testCases := []struct {
83157
name string
84158
// if not set, it will use the workflow we create on each run
85-
id string
86-
updates *biz.WorkflowUpdateOpts
87-
want *biz.Workflow
88-
wantErr bool
159+
id string
160+
updates *biz.WorkflowUpdateOpts
161+
want *biz.Workflow
162+
wantErr bool
163+
wantErrMsg string
89164
}{
90165
{
91166
name: "non existing workflow",
92167
id: uuid.Generate().String(),
93-
updates: &biz.WorkflowUpdateOpts{Name: toPtrS("new name")},
168+
updates: &biz.WorkflowUpdateOpts{Name: toPtrS("new-name")},
94169
wantErr: true,
95170
},
96171
{
97172
name: "invalid uuid",
98173
id: "deadbeef",
99-
updates: &biz.WorkflowUpdateOpts{Name: toPtrS("new name")},
174+
updates: &biz.WorkflowUpdateOpts{Name: toPtrS("new-name")},
100175
wantErr: true,
101176
},
102177
{
103-
name: "no updates",
104-
want: &biz.Workflow{Name: name, Team: team, Project: project, Public: false, Description: description},
178+
name: "no updates",
179+
wantErr: true,
180+
wantErrMsg: "no updates provided",
181+
},
182+
{
183+
name: "invalid name",
184+
wantErr: true,
185+
wantErrMsg: "RFC 1123",
186+
updates: &biz.WorkflowUpdateOpts{Name: toPtrS(" no no ")},
187+
},
188+
{
189+
name: "invalid Project",
190+
wantErr: true,
191+
wantErrMsg: "RFC 1123",
192+
updates: &biz.WorkflowUpdateOpts{Project: toPtrS(" no no ")},
105193
},
106194
{
107195
name: "update name",
108-
updates: &biz.WorkflowUpdateOpts{Name: toPtrS("new name")},
109-
want: &biz.Workflow{Name: "new name", Description: description, Team: team, Project: project, Public: false},
196+
updates: &biz.WorkflowUpdateOpts{Name: toPtrS("new-name")},
197+
want: &biz.Workflow{Name: "new-name", Description: description, Team: team, Project: project, Public: false},
110198
},
111199
{
112200
name: "update description",
113201
updates: &biz.WorkflowUpdateOpts{Description: toPtrS("new description")},
114-
want: &biz.Workflow{Name: name, Description: "new description", Team: team, Project: project, Public: false},
202+
want: &biz.Workflow{Description: "new description", Team: team, Project: project, Public: false},
115203
},
116204
{
117205
name: "update visibility",
118206
updates: &biz.WorkflowUpdateOpts{Public: toPtrBool(true)},
119-
want: &biz.Workflow{Name: name, Description: description, Team: team, Project: project, Public: true},
207+
want: &biz.Workflow{Description: description, Team: team, Project: project, Public: true},
120208
},
121209
{
122210
name: "update all options",
123-
updates: &biz.WorkflowUpdateOpts{Name: toPtrS("new name"), Project: toPtrS("new project"), Team: toPtrS("new team"), Public: toPtrBool(true)},
124-
want: &biz.Workflow{Name: "new name", Description: description, Team: "new team", Project: "new project", Public: true},
211+
updates: &biz.WorkflowUpdateOpts{Name: toPtrS("new-name-2"), Project: toPtrS("new-project"), Team: toPtrS("new team"), Public: toPtrBool(true)},
212+
want: &biz.Workflow{Name: "new-name-2", Description: description, Team: "new team", Project: "new-project", Public: true},
125213
},
126214
{
127215
name: "name can't be emptied",
128216
updates: &biz.WorkflowUpdateOpts{Name: toPtrS("")},
129-
want: &biz.Workflow{Name: name, Team: team, Project: project, Description: description},
217+
wantErr: true,
130218
},
131219
{
132220
name: "but other opts can",
133221
updates: &biz.WorkflowUpdateOpts{Team: toPtrS(""), Project: toPtrS(""), Description: toPtrS("")},
134-
want: &biz.Workflow{Name: name, Team: "", Project: "", Description: ""},
222+
want: &biz.Workflow{Team: "", Project: "", Description: ""},
135223
},
136224
}
137225

138-
for _, tc := range testCases {
226+
for i, tc := range testCases {
139227
s.Run(tc.name, func() {
140-
workflow, err := s.Workflow.Create(ctx, &biz.WorkflowCreateOpts{Description: description, Name: name, Team: team, Project: project, OrgID: s.org.ID})
228+
wfName := fmt.Sprintf("%s-%d", name, i)
229+
workflow, err := s.Workflow.Create(ctx, &biz.WorkflowCreateOpts{Description: description, Name: wfName, Team: team, Project: project, OrgID: s.org.ID})
141230
require.NoError(s.T(), err)
142231

143232
workflowID := tc.id
@@ -148,15 +237,25 @@ func (s *workflowIntegrationTestSuite) TestUpdate() {
148237
got, err := s.Workflow.Update(ctx, s.org.ID, workflowID, tc.updates)
149238
if tc.wantErr {
150239
s.Error(err)
240+
if tc.wantErrMsg != "" {
241+
s.Contains(err.Error(), tc.wantErrMsg)
242+
}
243+
151244
return
152245
}
153246
s.NoError(err)
154247

155248
if diff := cmp.Diff(tc.want, got,
156-
cmpopts.IgnoreFields(biz.Workflow{}, "CreatedAt", "ID", "OrgID", "ContractID", "ContractRevisionLatest"),
249+
cmpopts.IgnoreFields(biz.Workflow{}, "Name", "CreatedAt", "ID", "OrgID", "ContractID", "ContractRevisionLatest"),
157250
); diff != "" {
158251
s.Failf("mismatch (-want +got):\n%s", diff)
159252
}
253+
254+
if tc.want.Name != "" {
255+
s.Equal(tc.want.Name, got.Name)
256+
} else {
257+
s.Equal(wfName, got.Name)
258+
}
160259
})
161260
}
162261
}

‎app/controlplane/internal/biz/workflowcontract.go‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -155,7 +155,7 @@ func (uc *WorkflowContractUseCase) Create(ctx context.Context, opts *WorkflowCon
155155

156156
if err != nil {
157157
if errors.Is(err, ErrAlreadyExists) {
158-
return nil, NewErrValidationStr("that name is already taken")
158+
return nil, NewErrValidationStr("name already taken")
159159
}
160160

161161
return nil, fmt.Errorf("failed to create contract: %w", err)
@@ -189,7 +189,7 @@ func (uc *WorkflowContractUseCase) createWithUniqueName(ctx context.Context, opt
189189
return c, nil
190190
}
191191

192-
return nil, NewErrValidationStr("that name is already taken")
192+
return nil, NewErrValidationStr("name already taken")
193193
}
194194

195195
func (uc *WorkflowContractUseCase) Describe(ctx context.Context, orgID, contractID string, revision int) (*WorkflowContractWithVersion, error) {
@@ -247,7 +247,7 @@ func (uc *WorkflowContractUseCase) Update(ctx context.Context, orgID, contractID
247247
c, err := uc.repo.Update(ctx, opts)
248248
if err != nil {
249249
if errors.Is(err, ErrAlreadyExists) {
250-
return nil, NewErrValidationStr("that name is already taken")
250+
return nil, NewErrValidationStr("name already taken")
251251
}
252252

253253
return nil, fmt.Errorf("failed to update contract: %w", err)

‎app/controlplane/internal/data/ent/migrate/migrations/20240312141340.sql‎

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,47 @@
1+
-- Update existing data in "workflows" table
2+
-- Make the name and project RFC 1123 compliant
3+
UPDATE workflows
4+
SET name = regexp_replace(
5+
lower(name),
6+
'[^a-z0-9-]',
7+
'-',
8+
'g'
9+
);
10+
11+
-- and project
12+
UPDATE workflows
13+
SET project = regexp_replace(
14+
lower(name),
15+
'[^a-z0-9-]',
16+
'-',
17+
'g'
18+
);
19+
20+
-- Append suffixes to duplicates
21+
WITH numbered_names AS (
22+
SELECT
23+
id,
24+
name,
25+
ROW_NUMBER() OVER (PARTITION BY name ORDER BY id) AS rn
26+
FROM workflows
27+
)
28+
UPDATE workflows AS o
29+
SET name = CONCAT(o.name, '-', nn.rn - 1)
30+
FROM numbered_names AS nn
31+
WHERE o.id = nn.id AND nn.rn > 1;
32+
33+
34+
WITH numbered_projects AS (
35+
SELECT
36+
id,
37+
project,
38+
ROW_NUMBER() OVER (PARTITION BY name ORDER BY id) AS rn
39+
FROM workflows
40+
)
41+
UPDATE workflows AS o
42+
SET name = CONCAT(o.project, '-', np.rn - 1)
43+
FROM numbered_projects AS np
44+
WHERE o.id = np.id AND np.rn > 1;
45+
46+
-- Create index "workflow_name_organization_id" to table: "workflows"
47+
CREATE UNIQUE INDEX "workflow_name_organization_id" ON "workflows" ("name", "organization_id");

0 commit comments

Comments
 (0)