From 69ce1ac6f94d02f2497de12d9d3086cc0be78a3e Mon Sep 17 00:00:00 2001 From: Konrad Kleine <193408+kwk@users.noreply.github.com> Date: Tue, 21 Aug 2018 12:44:24 +0200 Subject: [PATCH] Default value (#2247) ## Defaults for fields This change introduces the ability for any field type to OPTIONALLY specify a custom default value. See also https://openshift.io/openshiftio/Openshift_io/plan/detail/35 and https://github.com/openshiftio/openshift.io/issues/3832. For lists and simple types, the only restriction is that the default value is of the same kind as the list or simple type (e.g. integer, float, string, user, label, ...). For enum fields the custom default value needs to be in the list of allowed values. ### Example usage of the feature As an example, we've set the default for severity and priority fields to a medium value instead of the first on in the list, which would be a high severity/priority. (See also https://github.com/openshiftio/openshift.io/issues/3832) ## Improvements to tests ### Space template import When a space template is imported it creates or overrides the work item types. With this change I've introduced a cascading validation that checks 1. the work item types, 2. the fields inside each work item type 3. the field type of each field. This ensures that people don't accidentally specify a float default (e.g. `33.45`) for an integer field. Before this change, most of the input and output validation happened at runtime when a value was converted into another representation. Now, the validation happens at import time and also when a default value for a field is calculated. --- .../list/ok_agile.res.payload.golden.json | 2 + .../list/ok_scrum.res.payload.golden.json | 1 + controller/workitemtype.go | 62 ++++- design/workitemtype.go | 2 +- spacetemplate/assets/agile.yaml | 2 + spacetemplate/assets/scrum.yaml | 2 + spacetemplate/importer/import_helper.go | 6 +- .../importer/importer_blackbox_test.go | 12 +- spacetemplate/importer/repository.go | 28 ++- workitem/enum_type.go | 77 +++++- workitem/enum_type_blackbox_test.go | 222 +++++++++++++++++- workitem/enum_type_whitebox_test.go | 70 ------ workitem/field_definition.go | 37 +-- workitem/json_storage.go | 20 +- workitem/list_type.go | 46 +++- workitem/list_type_blackbox_test.go | 141 +++++++++++ workitem/simple_type.go | 38 ++- workitem/simple_type_blackbox_test.go | 122 ++++++++-- workitem/workitem_repository.go | 2 +- workitem/workitemtype.go | 19 +- 20 files changed, 745 insertions(+), 166 deletions(-) diff --git a/controller/test-files/work_item_type/list/ok_agile.res.payload.golden.json b/controller/test-files/work_item_type/list/ok_agile.res.payload.golden.json index 6b38b806c8..fe2e8d95dd 100755 --- a/controller/test-files/work_item_type/list/ok_agile.res.payload.golden.json +++ b/controller/test-files/work_item_type/list/ok_agile.res.payload.golden.json @@ -416,6 +416,7 @@ "required": false, "type": { "baseType": "string", + "defaultValue": "P3 - Medium", "kind": "enum", "values": [ "P1 - Critical", @@ -458,6 +459,7 @@ "required": false, "type": { "baseType": "string", + "defaultValue": "SEV3 - Medium", "kind": "enum", "values": [ "SEV1 - Urgent", diff --git a/controller/test-files/work_item_type/list/ok_scrum.res.payload.golden.json b/controller/test-files/work_item_type/list/ok_scrum.res.payload.golden.json index d0800c6652..7cf3ef2de3 100755 --- a/controller/test-files/work_item_type/list/ok_scrum.res.payload.golden.json +++ b/controller/test-files/work_item_type/list/ok_scrum.res.payload.golden.json @@ -709,6 +709,7 @@ "required": false, "type": { "baseType": "string", + "defaultValue": "Medium", "kind": "enum", "values": [ "Critical", diff --git a/controller/workitemtype.go b/controller/workitemtype.go index 13514c0b2d..1f6bc12de4 100644 --- a/controller/workitemtype.go +++ b/controller/workitemtype.go @@ -101,16 +101,26 @@ func ConvertWorkItemTypeFromModel(request *http.Request, t *workitem.WorkItemTyp return converted } -// converts the field type from modesl to app representation +// converts the field type from model to app representation func ConvertFieldTypeFromModel(t workitem.FieldType) app.FieldType { result := app.FieldType{} result.Kind = string(t.GetKind()) - switch t2 := t.(type) { + switch modelFieldType := t.(type) { case workitem.ListType: - result.ComponentType = ptr.String(string(t2.ComponentType.GetKind())) + result.ComponentType = ptr.String(string(modelFieldType.ComponentType.GetKind())) + if modelFieldType.DefaultValue != nil { + result.DefaultValue = &modelFieldType.DefaultValue + } case workitem.EnumType: - result.BaseType = ptr.String(string(t2.BaseType.GetKind())) - result.Values = t2.Values + result.BaseType = ptr.String(string(modelFieldType.BaseType.GetKind())) + result.Values = modelFieldType.Values + if modelFieldType.DefaultValue != nil { + result.DefaultValue = &modelFieldType.DefaultValue + } + case workitem.SimpleType: + if modelFieldType.DefaultValue != nil { + result.DefaultValue = &modelFieldType.DefaultValue + } } return result @@ -130,7 +140,19 @@ func ConvertFieldTypeToModel(t app.FieldType) (workitem.FieldType, error) { if !componentType.IsSimpleType() { return nil, fmt.Errorf("Component type is not list type: %T", componentType) } - return workitem.ListType{workitem.SimpleType{*kind}, workitem.SimpleType{*componentType}}, nil + listType := workitem.ListType{ + SimpleType: workitem.SimpleType{Kind: *kind}, + ComponentType: workitem.SimpleType{Kind: *componentType}, + } + // convert list default value from app to model + if t.DefaultValue != nil { + fieldType, err := listType.SetDefaultValue(*t.DefaultValue) + if err != nil { + return nil, errs.Wrapf(err, "failed to convert default list value: %+v", *t.DefaultValue) + } + return fieldType, nil + } + return listType, nil case workitem.KindEnum: bt, err := workitem.ConvertAnyToKind(*t.BaseType) if err != nil { @@ -139,8 +161,9 @@ func ConvertFieldTypeToModel(t app.FieldType) (workitem.FieldType, error) { if !bt.IsSimpleType() { return nil, fmt.Errorf("baseType type is not list type: %T", bt) } - baseType := workitem.SimpleType{*bt} + baseType := workitem.SimpleType{Kind: *bt} + // convert enum values from app to model values := t.Values converted, err := workitem.ConvertList(func(ft workitem.FieldType, element interface{}) (interface{}, error) { return ft.ConvertToModel(element) @@ -148,15 +171,34 @@ func ConvertFieldTypeToModel(t app.FieldType) (workitem.FieldType, error) { if err != nil { return nil, errs.WithStack(err) } - return workitem.EnumType{ // TODO(kwk): handle RewritableValues here? + + enumType := workitem.EnumType{ // TODO(kwk): handle RewritableValues here? SimpleType: workitem.SimpleType{ Kind: *kind, }, BaseType: baseType, Values: converted, - }, nil + } + // convert enum default value from app to model + if t.DefaultValue != nil { + fieldType, err := enumType.SetDefaultValue(*t.DefaultValue) + if err != nil { + return nil, errs.Wrapf(err, "failed to convert default enum value: %+v", *t.DefaultValue) + } + return fieldType, nil + } + return enumType, nil default: - return workitem.SimpleType{*kind}, nil + simpleType := workitem.SimpleType{Kind: *kind} + // convert simple type default value from app to model + if t.DefaultValue != nil { + fieldType, err := simpleType.SetDefaultValue(*t.DefaultValue) + if err != nil { + return nil, errs.Wrapf(err, "failed to convert default simple type value: %+v", *t.DefaultValue) + } + return fieldType, nil + } + return simpleType, nil } } diff --git a/design/workitemtype.go b/design/workitemtype.go index 092cb77856..7095dd6aca 100644 --- a/design/workitemtype.go +++ b/design/workitemtype.go @@ -12,7 +12,7 @@ var fieldType = a.Type("fieldType", func() { a.Attribute("componentType", d.String, "The kind of type of the individual elements for a list type. Required for list types. Must be a simple type, not enum or list") a.Attribute("baseType", d.String, "The kind of type of the enumeration values for an enum type. Required for enum types. Must be a simple type, not enum or list") a.Attribute("values", a.ArrayOf(d.Any), "The possible values for an enum type. The values must be of a type convertible to the base type") - + a.Attribute("defaultValue", d.Any, "Optional default value (if any)") a.Required("kind") }) diff --git a/spacetemplate/assets/agile.yaml b/spacetemplate/assets/agile.yaml index 7ba6ace846..9b97fb7208 100644 --- a/spacetemplate/assets/agile.yaml +++ b/spacetemplate/assets/agile.yaml @@ -123,6 +123,7 @@ work_item_types: - SEV2 - High - SEV3 - Medium - SEV4 - Low + default_value: SEV3 - Medium "priority": label: Priority description: The order in which the developer should resolve a defect. @@ -137,6 +138,7 @@ work_item_types: - P2 - High - P3 - Medium - P4 - Low + default_value: P3 - Medium "resolution": label: Resolution description: > diff --git a/spacetemplate/assets/scrum.yaml b/spacetemplate/assets/scrum.yaml index 34880a1fa3..db19e9e236 100644 --- a/spacetemplate/assets/scrum.yaml +++ b/spacetemplate/assets/scrum.yaml @@ -58,6 +58,7 @@ work_item_types: - Minor - Optional - Trivial + default_value: Minor - id: &impedimentID "ec6918d6-f732-4fc0-a902-6571415aa73c" extends: *scrumCommonTypeID @@ -178,6 +179,7 @@ work_item_types: - High - Medium - Low + default_value: Medium "found_in": label: Found in description: > diff --git a/spacetemplate/importer/import_helper.go b/spacetemplate/importer/import_helper.go index 8a671803ee..20e26d10fc 100644 --- a/spacetemplate/importer/import_helper.go +++ b/spacetemplate/importer/import_helper.go @@ -29,11 +29,15 @@ func (s *ImportHelper) Validate() error { return errs.Wrap(err, "failed to validate space template") } - // Ensure all artifacts have the correct space template ID set + // Ensure all artifacts have the correct space template ID set and are + // valid for _, wit := range s.WITs { if wit.SpaceTemplateID != s.Template.ID { return errors.NewBadParameterError("work item types's space template ID", wit.SpaceTemplateID.String()).Expected(s.Template.ID.String()) } + if err := wit.Validate(); err != nil { + return errs.Wrapf(err, `failed to validate work item type "%s" (ID=%s)`, wit.Name, wit.ID) + } } for _, wilt := range s.WILTs { if wilt.SpaceTemplateID != s.Template.ID { diff --git a/spacetemplate/importer/importer_blackbox_test.go b/spacetemplate/importer/importer_blackbox_test.go index 6f912b9084..a8b3253455 100644 --- a/spacetemplate/importer/importer_blackbox_test.go +++ b/spacetemplate/importer/importer_blackbox_test.go @@ -283,6 +283,7 @@ work_item_types: required: yes type: kind: string + default_value: "foobar" state: label: State description: The state of the bug @@ -295,6 +296,7 @@ work_item_types: values: - new - closed + default_value: closed priority: label: Priority description: The priority of the bug @@ -303,7 +305,8 @@ work_item_types: simple_type: kind: list component_type: - kind: integer + kind: float + default_value: 42.0 work_item_link_types: - id: "` + wiltID.String() + `" name: Blocker @@ -375,7 +378,8 @@ func getValidTestTemplateParsed(t *testing.T, spaceTemplateID, witID, wiltID uui Description: "The title of the bug", Required: true, Type: workitem.SimpleType{ - Kind: workitem.KindString, + Kind: workitem.KindString, + DefaultValue: "foobar", }, }, "state": { @@ -390,6 +394,7 @@ func getValidTestTemplateParsed(t *testing.T, spaceTemplateID, witID, wiltID uui "new", "closed", }, + DefaultValue: "closed", }, }, "priority": { @@ -398,7 +403,8 @@ func getValidTestTemplateParsed(t *testing.T, spaceTemplateID, witID, wiltID uui Required: false, Type: workitem.ListType{ SimpleType: workitem.SimpleType{Kind: workitem.KindList}, - ComponentType: workitem.SimpleType{Kind: workitem.KindInteger}, + ComponentType: workitem.SimpleType{Kind: workitem.KindFloat}, + DefaultValue: 42.0, }, }, }, diff --git a/spacetemplate/importer/repository.go b/spacetemplate/importer/repository.go index 384b164cf4..057be04eb0 100644 --- a/spacetemplate/importer/repository.go +++ b/spacetemplate/importer/repository.go @@ -4,6 +4,7 @@ import ( "context" "fmt" + "github.com/davecgh/go-spew/spew" "github.com/fabric8-services/fabric8-wit/errors" "github.com/fabric8-services/fabric8-wit/id" "github.com/fabric8-services/fabric8-wit/log" @@ -145,9 +146,10 @@ func (r *GormRepository) createOrUpdateWITs(ctx context.Context, s *ImportHelper loadedWIT.Icon = wit.Icon loadedWIT.CanConstruct = wit.CanConstruct - //-------------------------------------------------------------------------------- - // Double check all existing fields are still present in new fields with same type - //-------------------------------------------------------------------------------- + //------------------------------------------------------------------ + // Double check all fields from the old work item type are still + // present in new work item type and still have the same field type. + //------------------------------------------------------------------ // verify that FieldTypes are same as loadedWIT toBeFoundFields := map[string]workitem.FieldType{} for k, fd := range loadedWIT.Fields { @@ -156,16 +158,28 @@ func (r *GormRepository) createOrUpdateWITs(ctx context.Context, s *ImportHelper // Remove fields directly defined in WIT for fieldName, fd := range wit.Fields { // verify FieldType with original value - if originalType, ok := toBeFoundFields[fieldName]; ok { - if equal := fd.Type.Equal(originalType); !equal { + if oldFieldType, ok := toBeFoundFields[fieldName]; ok { + + // When comparing the new and old field types we don't want + // to compare the default value. That is why we always + // overwrite the default value of the old type with the + // default value of the new type. + + defVal := fd.Type.GetDefaultValue() + oldFieldType, err = oldFieldType.SetDefaultValue(defVal) + if err != nil { + return errs.Wrapf(err, "failed to overwrite default of old field type with %+v (%[1]T)", defVal) + } + + if equal := fd.Type.Equal(oldFieldType); !equal { // Special treatment for EnumType - origEnum, ok1 := originalType.(workitem.EnumType) + origEnum, ok1 := oldFieldType.(workitem.EnumType) newEnum, ok2 := fd.Type.(workitem.EnumType) if ok1 && ok2 { equal = newEnum.EqualEnclosing(origEnum) } if !equal { - return errs.Errorf("type of the field %s changed from %s to %s", fieldName, originalType, fd.Type) + return errs.Errorf("type of the field %s changed from %+v to %+v", fieldName, spew.Sdump(oldFieldType), spew.Sdump(fd.Type)) } } } diff --git a/workitem/enum_type.go b/workitem/enum_type.go index fbc7a1eb46..d0d6eac9f5 100644 --- a/workitem/enum_type.go +++ b/workitem/enum_type.go @@ -8,31 +8,81 @@ import ( errs "github.com/pkg/errors" ) +// The EnumType defines the members that make up an enum field type definition. +// The SimpleType is set to KindEnum and the BaseType is set to whatever type of +// enum you want to have (e.g. an enum of strings or integers). The Values array +// specifies what the allowed values in this enum are. If RewritableValues is +// set to true, this type can be overwritten by a work item type that also +// defines a field of the same name with the same type, except with different +// allowed values inside. A classic example for this is the state field that can +// be overwritten by every work item type to fit its needs. type EnumType struct { SimpleType `json:"simple_type"` BaseType SimpleType `json:"base_type"` Values []interface{} `json:"values"` RewritableValues bool `json:"rewritable_values"` + DefaultValue interface{} `json:"default_value,omitempty"` } // Ensure EnumType implements the FieldType interface var _ FieldType = EnumType{} var _ FieldType = (*EnumType)(nil) -// DefaultValue implements FieldType -func (t EnumType) DefaultValue(value interface{}) (interface{}, error) { - if value != nil { - return value, nil +// Validate checks that the type of the enum is "enum", that the base type +// itself a simple type (e.g. not a list or an enum), that the default value +// matches the Kind of the BaseType, that the default value is in the list of +// allowed values and that the Values are all of the base type. +func (t EnumType) Validate() error { + if t.Kind != KindEnum { + return errs.Errorf(`enum has a base type "%s" but needs "%s"`, t.Kind, KindEnum) } + if !t.BaseType.Kind.IsSimpleType() { + return errs.Errorf(`enum type must have a simple component type and not "%s"`, t.Kind) + } + _, err := t.SetDefaultValue(t.DefaultValue) + if err != nil { + return errs.Wrapf(err, "failed to validate default value for kind %s: %+v (%[1]T)", t.Kind, t.DefaultValue) + } + // verify that we have a set of permitted values if t.Values == nil || len(t.Values) <= 0 { - return nil, errs.Errorf("enum has no values") + return errs.Errorf("enum type has no values: %+v", t) + } + for i, v := range t.Values { + _, err := t.ConvertToModel(v) + if err != nil { + return errs.Wrapf(err, `failed to convert value at position %d to kind "%s": %+v`, i, t.BaseType, v) + } } - return t.Values[0], nil + return nil } -// Ensure EnumType implements the FieldType interface -var _ FieldType = EnumType{} -var _ FieldType = (*EnumType)(nil) +// SetDefaultValue implements FieldType +func (t EnumType) SetDefaultValue(v interface{}) (FieldType, error) { + if v == nil { + t.DefaultValue = nil + return t, nil + } + defVal, err := t.ConvertToModel(v) + if err != nil { + return nil, errs.Wrapf(err, "failed to set default value of enum type to %+v (%[1]T)", v) + } + t.DefaultValue = defVal + return t, nil +} + +// GetDefaultValue implements FieldType +func (t EnumType) GetDefaultValue() interface{} { + // manual default value has precedence over first value in list of allowed + // values + if t.DefaultValue != nil { + return t.DefaultValue + } + // fallback to first permitted element + if len(t.Values) > 0 { + return t.Values[0] + } + return nil +} // Ensure EnumType implements the Equaler interface var _ convert.Equaler = EnumType{} @@ -51,7 +101,12 @@ func (t EnumType) Equal(u convert.Equaler) bool { return false } if !t.RewritableValues { - return reflect.DeepEqual(t.Values, other.Values) + if !reflect.DeepEqual(t.Values, other.Values) { + return false + } + } + if !reflect.DeepEqual(t.DefaultValue, other.DefaultValue) { + return false } return true } @@ -76,7 +131,7 @@ func (t EnumType) EqualEnclosing(other EnumType) bool { func (t EnumType) ConvertToModel(value interface{}) (interface{}, error) { converted, err := t.BaseType.ConvertToModel(value) if err != nil { - return nil, fmt.Errorf("error converting enum value: %s", err.Error()) + return nil, errs.Errorf("error converting enum value: %s", err.Error()) } if !contains(t.Values, converted) { diff --git a/workitem/enum_type_blackbox_test.go b/workitem/enum_type_blackbox_test.go index 7bb7a4f65a..ba2aa17acf 100644 --- a/workitem/enum_type_blackbox_test.go +++ b/workitem/enum_type_blackbox_test.go @@ -5,7 +5,7 @@ import ( "github.com/fabric8-services/fabric8-wit/convert" "github.com/fabric8-services/fabric8-wit/resource" - "github.com/fabric8-services/fabric8-wit/workitem" + w "github.com/fabric8-services/fabric8-wit/workitem" "github.com/stretchr/testify/require" ) @@ -13,26 +13,32 @@ func TestEnumType_Equal(t *testing.T) { t.Parallel() resource.Require(t, resource.UnitTest) - a := workitem.EnumType{ - SimpleType: workitem.SimpleType{Kind: workitem.KindEnum}, - BaseType: workitem.SimpleType{Kind: workitem.KindString}, + a := w.EnumType{ + SimpleType: w.SimpleType{Kind: w.KindEnum}, + BaseType: w.SimpleType{Kind: w.KindString}, Values: []interface{}{"foo", "bar"}, RewritableValues: false, + DefaultValue: "fooooooobar", } - t.Run("type inequality", func(t *testing.T) { require.False(t, a.Equal(convert.DummyEqualer{})) }) t.Run("simple type difference", func(t *testing.T) { b := a - b.SimpleType = workitem.SimpleType{Kind: workitem.KindArea} + b.SimpleType = w.SimpleType{Kind: w.KindArea} require.False(t, a.Equal(b)) }) t.Run("base type difference", func(t *testing.T) { b := a - b.BaseType = workitem.SimpleType{Kind: workitem.KindInteger} + b.BaseType = w.SimpleType{Kind: w.KindInteger} + require.False(t, a.Equal(b)) + }) + + t.Run("default value difference", func(t *testing.T) { + b := a + b.DefaultValue = "foo" require.False(t, a.Equal(b)) }) @@ -68,26 +74,218 @@ func TestEnumType_Equal(t *testing.T) { }) } +func TestEnumType_GetDefaultValue(t *testing.T) { + t.Parallel() + resource.Require(t, resource.UnitTest) + + tests := []struct { + name string + enum w.EnumType + expectedOutput interface{} + }{ + {"return first value of enum when default is nil", w.EnumType{ + SimpleType: w.SimpleType{Kind: w.KindEnum}, + BaseType: w.SimpleType{Kind: w.KindString}, + Values: []interface{}{"first", "second", "third"}, + }, "first"}, + {"return custom default when a default is set", w.EnumType{ + SimpleType: w.SimpleType{Kind: w.KindEnum}, + BaseType: w.SimpleType{Kind: w.KindInteger}, + Values: []interface{}{111, 222, 333}, + DefaultValue: 222, + }, 222}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + require.Equal(t, tt.expectedOutput, tt.enum.GetDefaultValue()) + }) + } +} + +func TestEnumType_SetDefaultValue(t *testing.T) { + t.Parallel() + resource.Require(t, resource.UnitTest) + + tests := []struct { + name string + enum w.EnumType + defVal interface{} + expectedOutput w.FieldType + wantErr bool + }{ + {"set default to allowed value", + w.EnumType{ + SimpleType: w.SimpleType{Kind: w.KindEnum}, + BaseType: w.SimpleType{Kind: w.KindString}, + Values: []interface{}{"first", "second", "third"}, + }, + "second", + &w.EnumType{ + SimpleType: w.SimpleType{Kind: w.KindEnum}, + BaseType: w.SimpleType{Kind: w.KindString}, + Values: []interface{}{"first", "second", "third"}, + DefaultValue: "second", + }, + false}, + {"set default to nil value", + w.EnumType{ + SimpleType: w.SimpleType{Kind: w.KindEnum}, + BaseType: w.SimpleType{Kind: w.KindString}, + Values: []interface{}{"first", "second", "third"}, + }, + nil, + &w.EnumType{ + SimpleType: w.SimpleType{Kind: w.KindEnum}, + BaseType: w.SimpleType{Kind: w.KindString}, + Values: []interface{}{"first", "second", "third"}, + DefaultValue: nil, + }, + false}, + {"set default to not-allowed value (wrong base type)", + w.EnumType{ + SimpleType: w.SimpleType{Kind: w.KindEnum}, + BaseType: w.SimpleType{Kind: w.KindString}, + Values: []interface{}{"first", "second", "third"}, + }, + 123, + nil, + true}, + {"set default to not-allowed value (not in list)", + w.EnumType{ + SimpleType: w.SimpleType{Kind: w.KindEnum}, + BaseType: w.SimpleType{Kind: w.KindString}, + Values: []interface{}{"first", "second", "third"}, + }, + "foobar", + nil, + true}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + output, err := tt.enum.SetDefaultValue(tt.defVal) + if tt.wantErr { + require.Error(t, err) + } else { + require.NoError(t, err) + require.Equal(t, tt.expectedOutput, output) + } + }) + } +} + +func TestEnumType_Validate(t *testing.T) { + t.Parallel() + tests := []struct { + name string + obj w.EnumType + wantErr bool + }{ + {"ok", w.EnumType{ + SimpleType: w.SimpleType{Kind: w.KindEnum}, + BaseType: w.SimpleType{Kind: w.KindString}, + Values: []interface{}{"who", "let", "the", "dogs", "out"}, + RewritableValues: false, + DefaultValue: "the", + }, false}, + {"error - empty values", w.EnumType{ + SimpleType: w.SimpleType{Kind: w.KindEnum}, + BaseType: w.SimpleType{Kind: w.KindString}, + Values: []interface{}{}, + RewritableValues: false, + DefaultValue: "the", + }, true}, + {"error - nil values", w.EnumType{ + SimpleType: w.SimpleType{Kind: w.KindEnum}, + BaseType: w.SimpleType{Kind: w.KindString}, + Values: nil, + RewritableValues: false, + DefaultValue: "the", + }, true}, + {"invalid type", w.EnumType{ + SimpleType: w.SimpleType{Kind: w.KindString}, + BaseType: w.SimpleType{Kind: w.KindString}, + Values: []interface{}{"who", "let", "the", "dogs", "out"}, + RewritableValues: false, + DefaultValue: "the", + }, true}, + {"invalid base type (list)", w.EnumType{ + SimpleType: w.SimpleType{Kind: w.KindEnum}, + BaseType: w.SimpleType{Kind: w.KindList}, + Values: []interface{}{"who", "let", "the", "dogs", "out"}, + RewritableValues: false, + DefaultValue: "the", + }, true}, + {"invalid base type (enum)", w.EnumType{ + SimpleType: w.SimpleType{Kind: w.KindEnum}, + BaseType: w.SimpleType{Kind: w.KindEnum}, + Values: []interface{}{"who", "let", "the", "dogs", "out"}, + RewritableValues: false, + DefaultValue: "the", + }, true}, + {"invalid string values", w.EnumType{ + SimpleType: w.SimpleType{Kind: w.KindEnum}, + BaseType: w.SimpleType{Kind: w.KindString}, + Values: []interface{}{"who", 1, "the", "dogs", "out"}, + RewritableValues: false, + DefaultValue: "the", + }, true}, + {"invalid integer values", w.EnumType{ + SimpleType: w.SimpleType{Kind: w.KindEnum}, + BaseType: w.SimpleType{Kind: w.KindInteger}, + Values: []interface{}{1, 2, "the", 4, 5}, + RewritableValues: false, + DefaultValue: "the", + }, true}, + {"invalid default value (wrong type)", w.EnumType{ + SimpleType: w.SimpleType{Kind: w.KindEnum}, + BaseType: w.SimpleType{Kind: w.KindInteger}, + Values: []interface{}{1, 2, 3, 4, 5}, + RewritableValues: false, + DefaultValue: "the", + }, true}, + {"invalid default value (not in allowed values)", w.EnumType{ + SimpleType: w.SimpleType{Kind: w.KindEnum}, + BaseType: w.SimpleType{Kind: w.KindInteger}, + Values: []interface{}{1, 2, 3, 4, 5}, + RewritableValues: false, + DefaultValue: 42, + }, true}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + err := tt.obj.Validate() + if tt.wantErr { + require.Error(t, err) + } else { + require.NoError(t, err) + } + }) + } +} + func TestEnumType_EqualEnclosing(t *testing.T) { t.Parallel() resource.Require(t, resource.UnitTest) - a := workitem.EnumType{ - SimpleType: workitem.SimpleType{Kind: workitem.KindEnum}, - BaseType: workitem.SimpleType{Kind: workitem.KindString}, + a := w.EnumType{ + SimpleType: w.SimpleType{Kind: w.KindEnum}, + BaseType: w.SimpleType{Kind: w.KindString}, Values: []interface{}{"foo", "bar", "baz"}, RewritableValues: false, } t.Run("simple type difference", func(t *testing.T) { b := a - b.SimpleType = workitem.SimpleType{Kind: workitem.KindArea} + b.SimpleType = w.SimpleType{Kind: w.KindArea} require.False(t, a.EqualEnclosing(b)) }) t.Run("base type difference", func(t *testing.T) { b := a - b.BaseType = workitem.SimpleType{Kind: workitem.KindInteger} + b.BaseType = w.SimpleType{Kind: w.KindInteger} require.False(t, a.EqualEnclosing(b)) }) diff --git a/workitem/enum_type_whitebox_test.go b/workitem/enum_type_whitebox_test.go index 644cd7ba96..d0769575ec 100644 --- a/workitem/enum_type_whitebox_test.go +++ b/workitem/enum_type_whitebox_test.go @@ -5,7 +5,6 @@ import ( "github.com/fabric8-services/fabric8-wit/resource" "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" ) func TestEnumTypeContains(t *testing.T) { @@ -103,72 +102,3 @@ func TestEnumTypeContainsAll(t *testing.T) { assert.False(t, containsAll(haystack, needles)) }) } - -func TestEnumTypeDefaultValue(t *testing.T) { - t.Parallel() - resource.Require(t, resource.UnitTest) - - // given - vals := []interface{}{"first", "second", "third"} - e := EnumType{ - SimpleType: SimpleType{Kind: KindEnum}, - BaseType: SimpleType{Kind: KindString}, - Values: vals, - } - - t.Run("default to first value of enum", func(t *testing.T) { - t.Parallel() - // when - def, err := e.DefaultValue(nil) - // then - require.NoError(t, err) - require.Equal(t, def, vals[0]) - }) - - t.Run("return value as is if not nil", func(t *testing.T) { - t.Parallel() - // when - def, err := e.DefaultValue("second") - // then - require.NoError(t, err) - require.Equal(t, def, vals[1]) - }) - - t.Run("return value as is (even if it is not one of the permissable values)", func(t *testing.T) { - t.Parallel() - // when - def, err := e.DefaultValue("not existing value") - // then - require.NoError(t, err) - require.Equal(t, def, "not existing value") - }) - - t.Run("return error when values are nil", func(t *testing.T) { - t.Parallel() - // given - a := EnumType{ - SimpleType: SimpleType{Kind: KindEnum}, - BaseType: SimpleType{Kind: KindString}, - } - // when - def, err := a.DefaultValue(nil) - // then - require.Error(t, err) - require.Nil(t, def) - }) - - t.Run("return error when values are empty", func(t *testing.T) { - t.Parallel() - // given - a := EnumType{ - SimpleType: SimpleType{Kind: KindEnum}, - BaseType: SimpleType{Kind: KindString}, - Values: []interface{}{}, - } - // when - def, err := a.DefaultValue(nil) - // then - require.Error(t, err) - require.Nil(t, def) - }) -} diff --git a/workitem/field_definition.go b/workitem/field_definition.go index 413e2cc0b6..2762cac354 100644 --- a/workitem/field_definition.go +++ b/workitem/field_definition.go @@ -72,9 +72,15 @@ type FieldType interface { ConvertFromModel(value interface{}) (interface{}, error) // Implement the Equaler interface Equal(u convert.Equaler) bool - // DefaultValue is called if a field is not specified. In it's simplest form - // the DefaultValue returns the given input value without any conversion. - DefaultValue(value interface{}) (interface{}, error) + // GetDefaultValue is called if a field's value is nil. + GetDefaultValue() interface{} + // SetDefaultValue returns a copy of the FieldType object at hand if there + // was no error setting the default value of that field type. + SetDefaultValue(v interface{}) (FieldType, error) + // Validate checks that the type definition of a field is correct. Take a + // look at the implementation of this function to find out what's actually + // been checked for each individual type. + Validate() error } // FieldDefinition describes type & other restrictions of a field @@ -93,8 +99,13 @@ var _ convert.Equaler = (*FieldDefinition)(nil) // Ensure FieldDefinition implements the json.Unmarshaler interface var _ json.Unmarshaler = (*FieldDefinition)(nil) -// // Ensure FieldDefinition implements the yaml.Unmarshaler interface -// var _ yaml.Unmarshaler = (*FieldDefinition)(nil) +// Validate checks that a field has a proper setup +func (f FieldDefinition) Validate() error { + if strings.TrimSpace(f.Label) == "" { + return errs.Errorf(`field label is empty "%s" when trimmed`, f.Label) + } + return f.Type.Validate() +} // Equal returns true if two FieldDefinition objects are equal; otherwise false is returned. func (f FieldDefinition) Equal(u convert.Equaler) bool { @@ -121,11 +132,7 @@ func (f FieldDefinition) Equal(u convert.Equaler) bool { func (f FieldDefinition) ConvertToModel(name string, value interface{}) (interface{}, error) { // Overwrite value if default value if none was provided if value == nil { - defValue, err := f.Type.DefaultValue(value) - if err != nil { - return nil, errs.Wrapf(err, "failed to get default value for field \"%s\"", name) - } - value = defValue + value = f.Type.GetDefaultValue() } if f.Required { @@ -183,19 +190,15 @@ func (f rawFieldDef) Equal(u convert.Equaler) bool { if f.Description != other.Description { return false } - if f.Type == nil && other.Type == nil { - return true - } - if f.Type != nil && other.Type != nil { - return reflect.DeepEqual(f.Type, other.Type) + if !reflect.DeepEqual(f.Type, other.Type) { + return false } - return false + return true } // UnmarshalJSON implements encoding/json.Unmarshaler func (f *FieldDefinition) UnmarshalJSON(bytes []byte) error { temp := rawFieldDef{} - err := json.Unmarshal(bytes, &temp) if err != nil { return errs.Wrapf(err, "failed to unmarshall field definition into rawFieldDef") diff --git a/workitem/json_storage.go b/workitem/json_storage.go index 680e5f6e7d..f8257a29c8 100644 --- a/workitem/json_storage.go +++ b/workitem/json_storage.go @@ -10,13 +10,15 @@ import ( errs "github.com/pkg/errors" ) +// Fields is a helper map that later gets replaced with the FieldDefinitions +// type and only exists for parsing in content from JSON. type Fields map[string]interface{} // Ensure Fields implements the Equaler interface var _ convert.Equaler = Fields{} var _ convert.Equaler = (*Fields)(nil) -// Ensure Fields implements the Scanner and Valuer interfaces +// Ensure Fields implements the sql.Scanner and driver.Valuer interfaces var _ sql.Scanner = (*Fields)(nil) var _ driver.Valuer = (*Fields)(nil) @@ -30,6 +32,7 @@ func (f Fields) Equal(u convert.Equaler) bool { return reflect.DeepEqual(f, other) } +// Value implements the driver.Valuer interface func (f Fields) Value() (driver.Value, error) { return toBytes(f) } @@ -41,6 +44,8 @@ func (f *Fields) Scan(src interface{}) error { return fromBytes(src, f) } +// FieldDefinitions define a map of field names pointing to their field +// definition. type FieldDefinitions map[string]FieldDefinition // Ensure FieldDefinitions implements the Scanner and Valuer interfaces @@ -59,6 +64,19 @@ func (j *FieldDefinitions) Scan(src interface{}) error { return fromBytes(src, j) } +// Validate checks that each field definition is valid +func (j *FieldDefinitions) Validate() error { + if j == nil { + return errs.New("fields not defined") + } + for name, field := range *j { + if err := field.Validate(); err != nil { + return errs.Wrapf(err, "failed to validate field %s", name) + } + } + return nil +} + func toBytes(j interface{}) (driver.Value, error) { if j == nil { // log.Trace("returning null") diff --git a/workitem/list_type.go b/workitem/list_type.go index ec32a29bde..fc6faf7892 100644 --- a/workitem/list_type.go +++ b/workitem/list_type.go @@ -5,12 +5,14 @@ import ( "reflect" "github.com/fabric8-services/fabric8-wit/convert" + errs "github.com/pkg/errors" ) -//ListType describes a list of SimpleType values +// ListType describes a list of SimpleType values type ListType struct { SimpleType `json:"simple_type"` - ComponentType SimpleType `json:"component_type"` + ComponentType SimpleType `json:"component_type"` + DefaultValue interface{} `json:"default_value,omitempty"` } // Ensure ListType implements the FieldType interface @@ -21,9 +23,40 @@ var _ FieldType = (*ListType)(nil) var _ convert.Equaler = ListType{} var _ convert.Equaler = (*ListType)(nil) -// DefaultValue implements FieldType -func (t ListType) DefaultValue(value interface{}) (interface{}, error) { - return value, nil +// Validate checks that the type of the list is "list", that the component type +// iteself a simple tpye (e.g. not a list or an enum) and that the default value +// matches the Kind of the ComponentType. +func (t ListType) Validate() error { + if t.Kind != KindList { + return errs.Errorf(`list type cannot have a base type "%s" but needs "%s"`, t.Kind, KindList) + } + if !t.ComponentType.Kind.IsSimpleType() { + return errs.Errorf(`list type must have a simple component type and not "%s"`, t.Kind) + } + _, err := t.SetDefaultValue(t.DefaultValue) + if err != nil { + return errs.Wrapf(err, "failed to validate default value for kind %s: %+v (%[1]T)", t.Kind, t.DefaultValue) + } + return nil +} + +// SetDefaultValue implements FieldType +func (t ListType) SetDefaultValue(v interface{}) (FieldType, error) { + if v == nil { + t.DefaultValue = nil + return t, nil + } + defVal, err := t.ComponentType.ConvertToModel(v) + if err != nil { + return nil, errs.Wrapf(err, "failed to set default value of list type to %+v (%[1]T)", v) + } + t.DefaultValue = defVal + return t, nil +} + +// GetDefaultValue implements FieldType +func (t ListType) GetDefaultValue() interface{} { + return t.DefaultValue } // Equal returns true if two ListType objects are equal; otherwise false is returned. @@ -35,6 +68,9 @@ func (t ListType) Equal(u convert.Equaler) bool { if !t.SimpleType.Equal(other.SimpleType) { return false } + if !reflect.DeepEqual(t.DefaultValue, other.DefaultValue) { + return false + } return t.ComponentType.Equal(other.ComponentType) } diff --git a/workitem/list_type_blackbox_test.go b/workitem/list_type_blackbox_test.go index 6cf53ed3ec..44ba116848 100644 --- a/workitem/list_type_blackbox_test.go +++ b/workitem/list_type_blackbox_test.go @@ -7,6 +7,7 @@ import ( "github.com/fabric8-services/fabric8-wit/resource" . "github.com/fabric8-services/fabric8-wit/workitem" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestListType_Equal(t *testing.T) { @@ -43,3 +44,143 @@ func TestListType_Equal(t *testing.T) { assert.True(t, d.Equal(a)) assert.True(t, a.Equal(d)) // test the inverse } + +func TestListType_Validate(t *testing.T) { + t.Parallel() + tests := []struct { + name string + obj ListType + wantErr bool + }{ + {"ok", ListType{ + SimpleType: SimpleType{Kind: KindList}, + ComponentType: SimpleType{Kind: KindString}, + DefaultValue: "the", + }, false}, + {"invalid type", ListType{ + SimpleType: SimpleType{Kind: KindInteger}, + ComponentType: SimpleType{Kind: KindString}, + DefaultValue: "the", + }, true}, + {"invalid component type (enum)", ListType{ + SimpleType: SimpleType{Kind: KindList}, + ComponentType: SimpleType{Kind: KindEnum}, + DefaultValue: "the", + }, true}, + {"invalid component type (list)", ListType{ + SimpleType: SimpleType{Kind: KindList}, + ComponentType: SimpleType{Kind: KindList}, + DefaultValue: "the", + }, true}, + {"invalid default value (string expect, int provided)", ListType{ + SimpleType: SimpleType{Kind: KindList}, + ComponentType: SimpleType{Kind: KindString}, + DefaultValue: 42, + }, true}, + {"invalid default value (int expect, string provided)", ListType{ + SimpleType: SimpleType{Kind: KindList}, + ComponentType: SimpleType{Kind: KindInteger}, + DefaultValue: "foo", + }, true}, + {"invalid default value (int expect, array of int provided)", ListType{ + SimpleType: SimpleType{Kind: KindList}, + ComponentType: SimpleType{Kind: KindInteger}, + DefaultValue: []int{1, 2, 3}, + }, true}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + err := tt.obj.Validate() + if tt.wantErr { + require.Error(t, err) + } else { + require.NoError(t, err) + } + }) + } +} + +func TestListType_GetDefaultValue(t *testing.T) { + t.Parallel() + tests := []struct { + name string + listType ListType + output interface{} + }{ + {"ok - string list", ListType{ + SimpleType: SimpleType{Kind: KindList}, + ComponentType: SimpleType{Kind: KindString}, + DefaultValue: "the", + }, "the"}, + {"ok - default is nil", ListType{ + SimpleType: SimpleType{Kind: KindList}, + ComponentType: SimpleType{Kind: KindString}, + }, nil}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + require.Equal(t, tt.output, tt.listType.GetDefaultValue()) + + }) + } +} + +func TestListType_SetDefaultValue(t *testing.T) { + t.Parallel() + resource.Require(t, resource.UnitTest) + + tests := []struct { + name string + enum ListType + defVal interface{} + expectedOutput FieldType + wantErr bool + }{ + {"set default to allowed value", + ListType{ + SimpleType: SimpleType{Kind: KindList}, + ComponentType: SimpleType{Kind: KindString}, + }, + []interface{}{"second"}, + &ListType{ + SimpleType: SimpleType{Kind: KindList}, + ComponentType: SimpleType{Kind: KindString}, + DefaultValue: []interface{}{"second"}, + }, + false}, + {"set default to nil", + ListType{ + SimpleType: SimpleType{Kind: KindList}, + ComponentType: SimpleType{Kind: KindString}, + }, + nil, + &ListType{ + SimpleType: SimpleType{Kind: KindList}, + ComponentType: SimpleType{Kind: KindString}, + DefaultValue: nil, + }, + false}, + {"set default to not-allowed (wrong component type)", + ListType{ + SimpleType: SimpleType{Kind: KindList}, + ComponentType: SimpleType{Kind: KindString}, + }, + 123, + nil, + true}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + output, err := tt.enum.SetDefaultValue(tt.defVal) + if tt.wantErr { + require.Error(t, err) + } else { + require.NoError(t, err) + require.Equal(t, tt.expectedOutput, output) + } + }) + } +} diff --git a/workitem/simple_type.go b/workitem/simple_type.go index 60e95fa9a4..1c7359fbe8 100644 --- a/workitem/simple_type.go +++ b/workitem/simple_type.go @@ -14,7 +14,8 @@ import ( // SimpleType is an unstructured FieldType type SimpleType struct { - Kind Kind `json:"kind"` + Kind Kind `json:"kind"` + DefaultValue interface{} `json:"default_value,omitempty"` } // Ensure SimpleType implements the FieldType interface @@ -25,9 +26,35 @@ var _ FieldType = (*SimpleType)(nil) var _ convert.Equaler = SimpleType{} var _ convert.Equaler = (*SimpleType)(nil) -// DefaultValue implements FieldType -func (t SimpleType) DefaultValue(value interface{}) (interface{}, error) { - return value, nil +// Validate checks that the default value matches the Kind +func (t SimpleType) Validate() error { + if !t.Kind.IsSimpleType() { + return errs.New("a simple type can only have a simple type (e.g. no list or enum)") + } + _, err := t.SetDefaultValue(t.DefaultValue) + if err != nil { + return errs.Wrapf(err, "failed to validate default value for kind %s: %+v (%[1]T)", t.Kind, t.DefaultValue) + } + return nil +} + +// SetDefaultValue implements FieldType +func (t SimpleType) SetDefaultValue(v interface{}) (FieldType, error) { + if v == nil { + t.DefaultValue = nil + return t, nil + } + defVal, err := t.ConvertToModel(v) + if err != nil { + return nil, errs.Wrapf(err, "failed to set default value of simple type to %+v (%[1]T)", v) + } + t.DefaultValue = defVal + return t, nil +} + +// GetDefaultValue implements FieldType +func (t SimpleType) GetDefaultValue() interface{} { + return t.DefaultValue } // Equal returns true if two SimpleType objects are equal; otherwise false is returned. @@ -36,6 +63,9 @@ func (t SimpleType) Equal(u convert.Equaler) bool { if !ok { return false } + if t.DefaultValue != other.DefaultValue { + return false + } return t.Kind == other.Kind } diff --git a/workitem/simple_type_blackbox_test.go b/workitem/simple_type_blackbox_test.go index 9000acf59e..cefb19f4cb 100644 --- a/workitem/simple_type_blackbox_test.go +++ b/workitem/simple_type_blackbox_test.go @@ -6,36 +6,118 @@ import ( "github.com/fabric8-services/fabric8-wit/convert" "github.com/fabric8-services/fabric8-wit/resource" . "github.com/fabric8-services/fabric8-wit/workitem" - "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) -func TestSimpleTypeEqual(t *testing.T) { +func TestSimpleType_Equal(t *testing.T) { t.Parallel() resource.Require(t, resource.UnitTest) - // Test type difference - a := SimpleType{Kind: KindString} - assert.False(t, a.Equal(convert.DummyEqualer{})) + t.Run("type difference", func(t *testing.T) { + t.Parallel() + a := SimpleType{Kind: KindString} + require.False(t, a.Equal(convert.DummyEqualer{})) + }) - // Test kind difference - b := SimpleType{Kind: KindInteger} - assert.False(t, a.Equal(b)) + t.Run("kind difference", func(t *testing.T) { + t.Parallel() + a := SimpleType{Kind: KindString} + b := SimpleType{Kind: KindInteger} + require.False(t, a.Equal(b)) + }) + + t.Run("default difference", func(t *testing.T) { + t.Parallel() + a := SimpleType{Kind: KindInteger, DefaultValue: 1} + b := SimpleType{Kind: KindInteger} + require.False(t, a.Equal(b)) + }) +} + +func TestSimpleType_Validate(t *testing.T) { + t.Parallel() + tests := []struct { + name string + obj SimpleType + wantErr bool + }{ + {"ok int field", SimpleType{Kind: KindInteger, DefaultValue: 333}, false}, + {"ok string field", SimpleType{Kind: KindString, DefaultValue: "foo"}, false}, + {"invalid default (int given, string expected)", SimpleType{Kind: KindString, DefaultValue: 333}, true}, + {"ok string field", SimpleType{Kind: KindInteger, DefaultValue: "foo"}, true}, + {"invalud kind (enum)", SimpleType{Kind: KindEnum, DefaultValue: "foo"}, true}, + {"invalid kind (list)", SimpleType{Kind: KindList, DefaultValue: "foo"}, true}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + err := tt.obj.Validate() + if tt.wantErr { + require.Error(t, err) + } else { + require.NoError(t, err) + } + }) + } +} + +func TestSimpleType_GetDefault(t *testing.T) { + t.Parallel() + tests := []struct { + name string + obj SimpleType + output interface{} + }{ + {"ok - int field: output 333", SimpleType{Kind: KindInteger, DefaultValue: 333}, 333}, + {"ok - float field: output 33.3", SimpleType{Kind: KindFloat, DefaultValue: 33.3}, 33.3}, + {"ok - string field: output \"foo\"", SimpleType{Kind: KindString, DefaultValue: "foo"}, "foo"}, + {"ok - string field nil default", SimpleType{Kind: KindString}, nil}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + require.Equal(t, tt.output, tt.obj.GetDefaultValue()) + }) + } } -func TestConvertToModel(t *testing.T) { +func TestSimpleType_SetDefaultValue(t *testing.T) { t.Parallel() resource.Require(t, resource.UnitTest) - // Test nil value - a := SimpleType{Kind: KindString} - res, err := a.ConvertToModel(nil) - assert.Nil(t, res) - require.NoError(t, err) - - // Test default case in swtich statement - b := 42 - res, err = a.ConvertToModel(&b) - assert.NotNil(t, err) - assert.Nil(t, res) + tests := []struct { + name string + enum SimpleType + defVal interface{} + expectedOutput FieldType + wantErr bool + }{ + {"set default to allowed value", + SimpleType{Kind: KindString}, + "foo", + &SimpleType{Kind: KindString, DefaultValue: "foo"}, + false}, + {"set default to nil", + SimpleType{Kind: KindString}, + nil, + &SimpleType{Kind: KindString, DefaultValue: nil}, + false}, + {"set default to not-allowed value", + SimpleType{Kind: KindString}, + 123, + nil, + true}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + output, err := tt.enum.SetDefaultValue(tt.defVal) + if tt.wantErr { + require.Error(t, err) + } else { + require.NoError(t, err) + require.Equal(t, tt.expectedOutput, output) + } + }) + } } diff --git a/workitem/workitem_repository.go b/workitem/workitem_repository.go index a375297d0d..4c2218a8f8 100644 --- a/workitem/workitem_repository.go +++ b/workitem/workitem_repository.go @@ -728,7 +728,7 @@ func (r *GormWorkItemRepository) Create(ctx context.Context, spaceID uuid.UUID, var err error wi.Fields[fieldName], err = fieldDef.ConvertToModel(fieldName, fieldValue) if err != nil { - return nil, errors.NewBadParameterError(fieldName, fieldValue) + return nil, errors.NewBadParameterError(fieldName, fieldValue) // TODO(kwk): Change errors pkg to consume the original error as well } if (fieldName == SystemAssignees || fieldName == SystemLabels || fieldName == SystemBoardcolumns) && fieldValue == nil { delete(wi.Fields, fieldName) diff --git a/workitem/workitemtype.go b/workitem/workitemtype.go index 3635b51669..2541be987c 100644 --- a/workitem/workitemtype.go +++ b/workitem/workitemtype.go @@ -1,13 +1,14 @@ package workitem import ( + "reflect" "strings" "time" "github.com/fabric8-services/fabric8-wit/convert" "github.com/fabric8-services/fabric8-wit/gormsupport" - "github.com/pkg/errors" + errs "github.com/pkg/errors" uuid "github.com/satori/go.uuid" ) @@ -112,6 +113,18 @@ type WorkItemType struct { ChildTypeIDs []uuid.UUID `gorm:"-" json:"child_types,omitempty"` } +// Validate runs some checks on the work item type to ensure the field +// definitions make sense. +func (wit WorkItemType) Validate() error { + if strings.TrimSpace(wit.Name) == "" { + return errs.Errorf(`work item type name "%s" when trimmed has a zero-length`, wit.Name) + } + if err := wit.Fields.Validate(); err != nil { + return errs.Wrapf(err, "failed to validate work item type's fields") + } + return nil +} + // GetTypePathSeparator returns the work item type's path separator "." func GetTypePathSeparator() string { return pathSep @@ -178,7 +191,7 @@ func (wit WorkItemType) Equal(u convert.Equaler) bool { if wit.CanConstruct != other.CanConstruct { return false } - if !strPtrIsNilOrContentIsEqual(wit.Description, other.Description) { + if !reflect.DeepEqual(wit.Description, other.Description) { return false } if wit.Icon != other.Icon { @@ -232,7 +245,7 @@ func (wit WorkItemType) ConvertWorkItemStorageToModel(workItem WorkItemStorage) } result.Fields[name], err = field.ConvertFromModel(name, workItem.Fields[name]) if err != nil { - return nil, errors.WithStack(err) + return nil, errs.WithStack(err) } result.Fields[SystemOrder] = workItem.ExecutionOrder }