From 9cbe4abf050bf905d5d83e97f13df80fa137fbef Mon Sep 17 00:00:00 2001 From: Kakeru Ishii Date: Fri, 14 Feb 2025 01:44:48 +0000 Subject: [PATCH] Fixed the race condition issue on form fields --- pkg/inspection/form/textform.go | 14 ++--- pkg/inspection/form/textform_test.go | 26 ++++----- .../metadata/form/conformance_test.go | 2 +- pkg/inspection/metadata/form/form.go | 18 ++++-- pkg/inspection/metadata/form/form_test.go | 9 +-- pkg/source/gcp/task/form_test.go | 58 +++++++++---------- pkg/testutil/form/util.go | 2 +- 7 files changed, 65 insertions(+), 64 deletions(-) diff --git a/pkg/inspection/form/textform.go b/pkg/inspection/form/textform.go index 126c7d9..e0ae537 100644 --- a/pkg/inspection/form/textform.go +++ b/pkg/inspection/form/textform.go @@ -175,7 +175,7 @@ func (b *TextFormDefinitionBuilder) Build(labelOpts ...common_task.LabelOpt) com if err != nil { return nil, fmt.Errorf("allowEdit provider for task `%s` returned an error\n%v", b.id, err) } - field := &form_metadata.FormField{} + field := form_metadata.FormField{} field.AllowEdit = allowEdit // Compute the default value of the form @@ -222,12 +222,6 @@ func (b *TextFormDefinitionBuilder) Build(labelOpts ...common_task.LabelOpt) com return nil, fmt.Errorf("validator for task `%s` returned a validation error. But this task was executed as a Run mode not in DryRun. All validations must be resolved before running.\n%v", b.id, field.ValidationError) } - formFields := m.LoadOrStore(form_metadata.FormFieldSetMetadataKey, &form_metadata.FormFieldSetMetadataFactory{}).(*form_metadata.FormFieldSet) - err = formFields.SetField(field) - if err != nil { - return nil, fmt.Errorf("failed to configure the form metadata in task `%s`\n%v", b.id, err) - } - convertedValue, err := b.converter(ctx, currentValue, v) if err != nil { return nil, fmt.Errorf("failed to convert the value `%s` to the dedicated value in task %s\n%v", currentValue, b.id, err) @@ -244,6 +238,12 @@ func (b *TextFormDefinitionBuilder) Build(labelOpts ...common_task.LabelOpt) com cacheStore.Store(previousValueStoreKey, newValueHistory) } } + + formFields := m.LoadOrStore(form_metadata.FormFieldSetMetadataKey, &form_metadata.FormFieldSetMetadataFactory{}).(*form_metadata.FormFieldSet) + err = formFields.SetField(field) + if err != nil { + return nil, fmt.Errorf("failed to configure the form metadata in task `%s`\n%v", b.id, err) + } return convertedValue, nil }, labelOpts...) } diff --git a/pkg/inspection/form/textform_test.go b/pkg/inspection/form/textform_test.go index 288f51a..39401b5 100644 --- a/pkg/inspection/form/textform_test.go +++ b/pkg/inspection/form/textform_test.go @@ -50,7 +50,7 @@ func TestTextFormDefinitionBuilder(t *testing.T) { Name string FormConfigurator testFormConfigurator RequestValue string - ExpectedFormField *form_metadata.FormField + ExpectedFormField form_metadata.FormField ExpectedValue any ExpectedError string }{ @@ -60,7 +60,7 @@ func TestTextFormDefinitionBuilder(t *testing.T) { RequestValue: "bar", ExpectedValue: "bar", ExpectedError: "", - ExpectedFormField: &form_metadata.FormField{ + ExpectedFormField: form_metadata.FormField{ AllowEdit: true, HintType: form_metadata.HintTypeInfo, }, @@ -73,7 +73,7 @@ func TestTextFormDefinitionBuilder(t *testing.T) { RequestValue: "", ExpectedValue: "foo-default", ExpectedError: "", - ExpectedFormField: &form_metadata.FormField{ + ExpectedFormField: form_metadata.FormField{ AllowEdit: true, Default: "foo-default", HintType: form_metadata.HintTypeInfo, @@ -89,7 +89,7 @@ func TestTextFormDefinitionBuilder(t *testing.T) { RequestValue: "", ExpectedValue: "foo-default", ExpectedError: "", - ExpectedFormField: &form_metadata.FormField{ + ExpectedFormField: form_metadata.FormField{ AllowEdit: true, ValidationError: "foo validation error", HintType: form_metadata.HintTypeInfo, @@ -105,7 +105,7 @@ func TestTextFormDefinitionBuilder(t *testing.T) { RequestValue: "", ExpectedValue: "", ExpectedError: "", - ExpectedFormField: &form_metadata.FormField{ + ExpectedFormField: form_metadata.FormField{ AllowEdit: false, HintType: form_metadata.HintTypeInfo, }, @@ -120,7 +120,7 @@ func TestTextFormDefinitionBuilder(t *testing.T) { RequestValue: "bar-from-request", ExpectedValue: "foo-from-default", ExpectedError: "", - ExpectedFormField: &form_metadata.FormField{ + ExpectedFormField: form_metadata.FormField{ AllowEdit: false, Default: "foo-from-default", HintType: form_metadata.HintTypeInfo, @@ -136,7 +136,7 @@ func TestTextFormDefinitionBuilder(t *testing.T) { RequestValue: "bar-from-request", ExpectedValue: "bar-from-request", ExpectedError: "", - ExpectedFormField: &form_metadata.FormField{ + ExpectedFormField: form_metadata.FormField{ AllowEdit: true, Hint: "foo-hint", HintType: form_metadata.HintTypeInfo, @@ -152,7 +152,7 @@ func TestTextFormDefinitionBuilder(t *testing.T) { RequestValue: "bar-from-request", ExpectedValue: "bar-from-request", ExpectedError: "", - ExpectedFormField: &form_metadata.FormField{ + ExpectedFormField: form_metadata.FormField{ AllowEdit: true, Default: "foo-from-default", HintType: form_metadata.HintTypeInfo, @@ -170,7 +170,7 @@ func TestTextFormDefinitionBuilder(t *testing.T) { RequestValue: "bar-from-request", ExpectedValue: "bar-from-request", ExpectedError: "", - ExpectedFormField: &form_metadata.FormField{ + ExpectedFormField: form_metadata.FormField{ AllowEdit: true, Suggestions: []string{ "foo-suggest1", @@ -187,7 +187,7 @@ func TestTextFormDefinitionBuilder(t *testing.T) { originalBuilder := NewInputFormDefinitionBuilder("foo", 1, "foo label") testCase.FormConfigurator(originalBuilder) taskDef := originalBuilder.Build() - formFields := []*form_metadata.FormField{} + formFields := []form_metadata.FormField{} // Execute task as DryRun mode vs := generateFakeVariableSet("foo", testCase.RequestValue) @@ -208,9 +208,6 @@ func TestTextFormDefinitionBuilder(t *testing.T) { t.Errorf("unexpected error while getting metadata\n%v", err) } field := ms.LoadOrStore(form_metadata.FormFieldSetMetadataKey, &form_metadata.FormFieldSetMetadataFactory{}).(*form_metadata.FormFieldSet).DangerouslyGetField("foo") - if field == nil { - t.Errorf("field metadata wasn't added as expected") - } formFields = append(formFields, field) } @@ -234,9 +231,6 @@ func TestTextFormDefinitionBuilder(t *testing.T) { t.Errorf("unexpected error while getting metadata\n%v", err) } field := ms.LoadOrStore(form_metadata.FormFieldSetMetadataKey, &form_metadata.FormFieldSetMetadataFactory{}).(*form_metadata.FormFieldSet).DangerouslyGetField("foo") - if field == nil { - t.Errorf("field metadata wasn't added as expected") - } formFields = append(formFields, field) } diff --git a/pkg/inspection/metadata/form/conformance_test.go b/pkg/inspection/metadata/form/conformance_test.go index bd8d097..ea45d16 100644 --- a/pkg/inspection/metadata/form/conformance_test.go +++ b/pkg/inspection/metadata/form/conformance_test.go @@ -23,7 +23,7 @@ import ( func newFormFieldsForConformanceTest() metadata.Metadata { forms := (&FormFieldSetMetadataFactory{}).Instanciate().(*FormFieldSet) - forms.SetField(&FormField{}) + forms.SetField(FormField{}) return forms } diff --git a/pkg/inspection/metadata/form/form.go b/pkg/inspection/metadata/form/form.go index 307d9a0..9431d10 100644 --- a/pkg/inspection/metadata/form/form.go +++ b/pkg/inspection/metadata/form/form.go @@ -17,6 +17,7 @@ package form import ( "fmt" "slices" + "sync" "github.com/GoogleCloudPlatform/khi/pkg/inspection/metadata" "github.com/GoogleCloudPlatform/khi/pkg/task" @@ -47,7 +48,8 @@ type FormField struct { // FormFieldSet is a metadata type used in frontend to generate the form fields. type FormFieldSet struct { - fields []*FormField + fieldsLock sync.RWMutex + fields []FormField } var _ metadata.Metadata = (*FormFieldSet)(nil) @@ -61,7 +63,9 @@ func (f *FormFieldSet) ToSerializable() interface{} { return f.fields } -func (f *FormFieldSet) SetField(newField *FormField) error { +func (f *FormFieldSet) SetField(newField FormField) error { + f.fieldsLock.Lock() + defer f.fieldsLock.Unlock() if newField.Id == "" { return fmt.Errorf("id must not be empty") } @@ -71,7 +75,7 @@ func (f *FormFieldSet) SetField(newField *FormField) error { } } f.fields = append(f.fields, newField) - slices.SortFunc(f.fields, func(a, b *FormField) int { + slices.SortFunc(f.fields, func(a, b FormField) int { return b.Priority - a.Priority }) return nil @@ -79,13 +83,15 @@ func (f *FormFieldSet) SetField(newField *FormField) error { // DangerouslyGetField shouldn't be used in non testing code. Because a field shouldn't depend on the other field metadata. // This is only for testing purpose. -func (f *FormFieldSet) DangerouslyGetField(id string) *FormField { +func (f *FormFieldSet) DangerouslyGetField(id string) FormField { + f.fieldsLock.RLock() + defer f.fieldsLock.RUnlock() for _, field := range f.fields { if field.Id == id { return field } } - return nil + return FormField{} } type FormFieldSetMetadataFactory struct{} @@ -93,7 +99,7 @@ type FormFieldSetMetadataFactory struct{} // Instanciate implements metadata.MetadataFactory. func (f *FormFieldSetMetadataFactory) Instanciate() metadata.Metadata { return &FormFieldSet{ - fields: make([]*FormField, 0), + fields: make([]FormField, 0), } } diff --git a/pkg/inspection/metadata/form/form_test.go b/pkg/inspection/metadata/form/form_test.go index da4ee46..1073ec1 100644 --- a/pkg/inspection/metadata/form/form_test.go +++ b/pkg/inspection/metadata/form/form_test.go @@ -18,10 +18,11 @@ import ( "testing" "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" ) -func fieldWithIdAndPriorityForTest(id string, priority int) *FormField { - return &FormField{ +func fieldWithIdAndPriorityForTest(id string, priority int) FormField { + return FormField{ Id: id, Priority: priority, } @@ -34,14 +35,14 @@ func TestFormFieldSetShouldSortOnAddingNewField(t *testing.T) { fsActual.SetField(fieldWithIdAndPriorityForTest("qux", 2)) fsExpected := &FormFieldSet{ - fields: []*FormField{ + fields: []FormField{ fieldWithIdAndPriorityForTest("bar", 3), fieldWithIdAndPriorityForTest("qux", 2), fieldWithIdAndPriorityForTest("foo", 1), }, } - if diff := cmp.Diff(fsActual, fsExpected, cmp.AllowUnexported(FormFieldSet{})); diff != "" { + if diff := cmp.Diff(fsActual, fsExpected, cmp.AllowUnexported(FormFieldSet{}),cmpopts.IgnoreFields(FormFieldSet{},"fieldsLock")); diff != "" { t.Errorf("FieldSet has fields in unexpected shape\n%v", diff) } } diff --git a/pkg/source/gcp/task/form_test.go b/pkg/source/gcp/task/form_test.go index 41c6bf9..69e07ec 100644 --- a/pkg/source/gcp/task/form_test.go +++ b/pkg/source/gcp/task/form_test.go @@ -44,7 +44,7 @@ func TestProjectIdInput(t *testing.T) { Dependencies: []task.Definition{ testClusterNamePrefix, }, - ExpectedFormField: &form.FormField{ + ExpectedFormField: form.FormField{ Priority: 1, Id: GCPPrefix + "input/project-id", Type: "Text", @@ -61,7 +61,7 @@ func TestProjectIdInput(t *testing.T) { Dependencies: []task.Definition{ testClusterNamePrefix, }, - ExpectedFormField: &form.FormField{ + ExpectedFormField: form.FormField{ Priority: 1, Id: GCPPrefix + "input/project-id", Type: "Text", @@ -86,7 +86,7 @@ func TestProjectIdInput(t *testing.T) { Dependencies: []task.Definition{ testClusterNamePrefix, }, - ExpectedFormField: &form.FormField{ + ExpectedFormField: form.FormField{ Priority: 1, Id: GCPPrefix + "input/project-id", Type: "Text", @@ -104,7 +104,7 @@ func TestProjectIdInput(t *testing.T) { Dependencies: []task.Definition{ testClusterNamePrefix, }, - ExpectedFormField: &form.FormField{ + ExpectedFormField: form.FormField{ Priority: 1, Id: GCPPrefix + "input/project-id", Type: "Text", @@ -121,7 +121,7 @@ func TestProjectIdInput(t *testing.T) { Dependencies: []task.Definition{ testClusterNamePrefix, }, - ExpectedFormField: &form.FormField{ + ExpectedFormField: form.FormField{ Priority: 1, Id: GCPPrefix + "input/project-id", Type: "Text", @@ -145,7 +145,7 @@ func TestClusterNameInput(t *testing.T) { Input: "foo-cluster", ExpectedValue: "foo-cluster", Dependencies: []task.Definition{mockClusterNamesTask1, testClusterNamePrefix}, - ExpectedFormField: &form.FormField{ + ExpectedFormField: form.FormField{ Priority: 1, Id: GCPPrefix + "input/cluster-name", Type: "Text", @@ -161,7 +161,7 @@ func TestClusterNameInput(t *testing.T) { Input: " foo-cluster ", ExpectedValue: "foo-cluster", Dependencies: []task.Definition{mockClusterNamesTask1, testClusterNamePrefix}, - ExpectedFormField: &form.FormField{ + ExpectedFormField: form.FormField{ Priority: 1, Id: GCPPrefix + "input/cluster-name", Type: "Text", @@ -177,7 +177,7 @@ func TestClusterNameInput(t *testing.T) { Input: "An invalid cluster name", ExpectedValue: "foo-cluster", Dependencies: []task.Definition{mockClusterNamesTask1, testClusterNamePrefix}, - ExpectedFormField: &form.FormField{ + ExpectedFormField: form.FormField{ Priority: 1, Id: GCPPrefix + "input/cluster-name", Type: "Text", @@ -194,7 +194,7 @@ func TestClusterNameInput(t *testing.T) { Input: "nonexisting-cluster", ExpectedValue: "nonexisting-cluster", Dependencies: []task.Definition{mockClusterNamesTask1, testClusterNamePrefix}, - ExpectedFormField: &form.FormField{ + ExpectedFormField: form.FormField{ Priority: 1, Id: GCPPrefix + "input/cluster-name", Type: "Text", @@ -225,7 +225,7 @@ func TestDurationInput(t *testing.T) { Input: "10m", ExpectedValue: time.Duration(time.Minute) * 10, Dependencies: []task.Definition{endTimeTask, currentTimeTask1, timezoneTaskUTC}, - ExpectedFormField: &form.FormField{ + ExpectedFormField: form.FormField{ Label: expectedLabel, Description: expectedDescription, Suggestions: expectedSuggestions, @@ -243,7 +243,7 @@ func TestDurationInput(t *testing.T) { Input: "foo", ExpectedValue: time.Hour, Dependencies: []task.Definition{endTimeTask, currentTimeTask1, timezoneTaskUTC}, - ExpectedFormField: &form.FormField{ + ExpectedFormField: form.FormField{ Label: expectedLabel, Description: expectedDescription, AllowEdit: true, @@ -258,7 +258,7 @@ func TestDurationInput(t *testing.T) { Input: "-10m", ExpectedValue: time.Hour, Dependencies: []task.Definition{endTimeTask, currentTimeTask1, timezoneTaskUTC}, - ExpectedFormField: &form.FormField{ + ExpectedFormField: form.FormField{ Label: expectedLabel, Description: expectedDescription, AllowEdit: true, @@ -273,7 +273,7 @@ func TestDurationInput(t *testing.T) { Input: "672h", // starting time will be 30 days before the inspection time ExpectedValue: time.Hour * 672, Dependencies: []task.Definition{endTimeTask, currentTimeTask1, timezoneTaskUTC}, - ExpectedFormField: &form.FormField{ + ExpectedFormField: form.FormField{ Type: "Text", Label: expectedLabel, Description: expectedDescription, @@ -294,7 +294,7 @@ Query range: Input: "1h", ExpectedValue: time.Hour, Dependencies: []task.Definition{endTimeTask, currentTimeTask1, timezoneTaskJST}, - ExpectedFormField: &form.FormField{ + ExpectedFormField: form.FormField{ Type: "Text", Label: expectedLabel, Description: expectedDescription, @@ -331,7 +331,7 @@ func TestInputEndtime(t *testing.T) { Input: "", ExpectedValue: expectedValue1, Dependencies: []task.Definition{inspection_task.TestInspectionTimeTaskProducer("2020-01-02T03:04:05Z"), timezoneTaskUTC}, - ExpectedFormField: &form.FormField{ + ExpectedFormField: form.FormField{ Label: expectedLabel, Description: expectedDescription, AllowEdit: true, @@ -346,7 +346,7 @@ func TestInputEndtime(t *testing.T) { Input: "2020-01-02T00:00:00Z", ExpectedValue: expectedValue2, Dependencies: []task.Definition{inspection_task.TestInspectionTimeTaskProducer("2020-01-02T03:04:05Z"), timezoneTaskUTC}, - ExpectedFormField: &form.FormField{ + ExpectedFormField: form.FormField{ Label: expectedLabel, Description: expectedDescription, AllowEdit: true, @@ -361,7 +361,7 @@ func TestInputEndtime(t *testing.T) { Input: "2020-01-02T00:00:00Z", ExpectedValue: expectedValue2, Dependencies: []task.Definition{inspection_task.TestInspectionTimeTaskProducer("2020-01-02T03:04:05Z"), timezoneTaskJST}, - ExpectedFormField: &form.FormField{ + ExpectedFormField: form.FormField{ Label: expectedLabel, Description: expectedDescription, AllowEdit: true, @@ -414,7 +414,7 @@ func TestInputKindName(t *testing.T) { ValidationError: "", SubtractMode: false, }, - ExpectedFormField: &form.FormField{ + ExpectedFormField: form.FormField{ Label: expectedLabel, Description: expectedDescription, AllowEdit: true, @@ -431,7 +431,7 @@ func TestInputKindName(t *testing.T) { ValidationError: "", SubtractMode: false, }, - ExpectedFormField: &form.FormField{ + ExpectedFormField: form.FormField{ Label: expectedLabel, Description: expectedDescription, AllowEdit: true, @@ -446,7 +446,7 @@ func TestInputKindName(t *testing.T) { Subtractives: []string{}, ValidationError: "", SubtractMode: false, - }, ExpectedFormField: &form.FormField{ + }, ExpectedFormField: form.FormField{ Label: expectedLabel, Description: expectedDescription, AllowEdit: true, @@ -475,7 +475,7 @@ func TestInputNamespaces(t *testing.T) { ValidationError: "", SubtractMode: false, }, - ExpectedFormField: &form.FormField{ + ExpectedFormField: form.FormField{ Label: expectedLabel, Description: expectedDescription, AllowEdit: true, @@ -492,7 +492,7 @@ func TestInputNamespaces(t *testing.T) { ValidationError: "", SubtractMode: false, }, - ExpectedFormField: &form.FormField{ + ExpectedFormField: form.FormField{ Label: expectedLabel, Description: expectedDescription, AllowEdit: true, @@ -507,7 +507,7 @@ func TestInputNamespaces(t *testing.T) { Subtractives: []string{}, ValidationError: "", SubtractMode: false, - }, ExpectedFormField: &form.FormField{ + }, ExpectedFormField: form.FormField{ Label: expectedLabel, Description: expectedDescription, AllowEdit: true, @@ -530,7 +530,7 @@ func TestNodeNameFiltertask(t *testing.T) { Input: "", ExpectedValue: []string{}, Dependencies: []task.Definition{}, - ExpectedFormField: &form.FormField{ + ExpectedFormField: form.FormField{ Label: wantLabelName, Description: wantDescription, AllowEdit: true, @@ -543,7 +543,7 @@ func TestNodeNameFiltertask(t *testing.T) { Input: "node-name-1", ExpectedValue: []string{"node-name-1"}, Dependencies: []task.Definition{}, - ExpectedFormField: &form.FormField{ + ExpectedFormField: form.FormField{ Label: wantLabelName, Description: wantDescription, AllowEdit: true, @@ -556,7 +556,7 @@ func TestNodeNameFiltertask(t *testing.T) { Input: "node-name-1 node-name-2 node-name-3", ExpectedValue: []string{"node-name-1", "node-name-2", "node-name-3"}, Dependencies: []task.Definition{}, - ExpectedFormField: &form.FormField{ + ExpectedFormField: form.FormField{ Label: wantLabelName, Description: wantDescription, AllowEdit: true, @@ -569,7 +569,7 @@ func TestNodeNameFiltertask(t *testing.T) { Input: "node-name-1 invalid=node=name node-name-3", ExpectedValue: []string{}, Dependencies: []task.Definition{}, - ExpectedFormField: &form.FormField{ + ExpectedFormField: form.FormField{ Label: wantLabelName, Description: wantDescription, AllowEdit: true, @@ -583,7 +583,7 @@ func TestNodeNameFiltertask(t *testing.T) { Input: " node-name-1 node-name-2 ", ExpectedValue: []string{"node-name-1", "node-name-2"}, Dependencies: []task.Definition{}, - ExpectedFormField: &form.FormField{ + ExpectedFormField: form.FormField{ Label: wantLabelName, Description: wantDescription, AllowEdit: true, @@ -601,7 +601,7 @@ func TestLocationInput(t *testing.T) { Input: "asia-northeast1", ExpectedValue: "asia-northeast1", Dependencies: []task.Definition{}, - ExpectedFormField: &form.FormField{ + ExpectedFormField: form.FormField{ Priority: 1, Id: GCPPrefix + "input/location", Type: "Text", diff --git a/pkg/testutil/form/util.go b/pkg/testutil/form/util.go index a2b6a9d..9707312 100644 --- a/pkg/testutil/form/util.go +++ b/pkg/testutil/form/util.go @@ -30,7 +30,7 @@ type FormTestCase struct { Name string Input string ExpectedValue any - ExpectedFormField *form.FormField + ExpectedFormField form.FormField Dependencies []task.Definition Before func() After func()