Skip to content

Commit

Permalink
Fixed the race condition issue on form fields
Browse files Browse the repository at this point in the history
  • Loading branch information
@kyasbal
kyasbal committed Feb 14, 2025
1 parent e12bbca commit 9cbe4ab
Show file tree
Hide file tree
Showing 7 changed files with 65 additions and 64 deletions.
14 changes: 7 additions & 7 deletions pkg/inspection/form/textform.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)
Expand All @@ -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...)
}
26 changes: 10 additions & 16 deletions pkg/inspection/form/textform_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
}{
Expand All @@ -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,
},
Expand All @@ -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,
Expand All @@ -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,
Expand All @@ -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,
},
Expand All @@ -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,
Expand All @@ -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,
Expand All @@ -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,
Expand All @@ -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",
Expand All @@ -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)
Expand All @@ -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)
}

Expand All @@ -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)
}

Expand Down
2 changes: 1 addition & 1 deletion pkg/inspection/metadata/form/conformance_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ import (

func newFormFieldsForConformanceTest() metadata.Metadata {
forms := (&FormFieldSetMetadataFactory{}).Instanciate().(*FormFieldSet)
forms.SetField(&FormField{})
forms.SetField(FormField{})
return forms
}

Expand Down
18 changes: 12 additions & 6 deletions pkg/inspection/metadata/form/form.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ package form
import (
"fmt"
"slices"
"sync"

"github.com/GoogleCloudPlatform/khi/pkg/inspection/metadata"
"github.com/GoogleCloudPlatform/khi/pkg/task"
Expand Down Expand Up @@ -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)
Expand All @@ -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")
}
Expand All @@ -71,29 +75,31 @@ 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
}

// 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{}

// Instanciate implements metadata.MetadataFactory.
func (f *FormFieldSetMetadataFactory) Instanciate() metadata.Metadata {
return &FormFieldSet{
fields: make([]*FormField, 0),
fields: make([]FormField, 0),
}
}

Expand Down
9 changes: 5 additions & 4 deletions pkg/inspection/metadata/form/form_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
}
Expand All @@ -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)
}
}
Loading

0 comments on commit 9cbe4ab

Please sign in to comment.