From 12ab381c1e9bd7f84cd5be747da1fdf9778065de Mon Sep 17 00:00:00 2001 From: Shubham Minglani Date: Mon, 10 Jul 2017 20:25:02 +0530 Subject: [PATCH] auto detect conflicting JSON tags in spec.go Before this commit, we had no way to know if the embedded structs had any conflicting fields between each other or with the struct embedding the structs. This led to inconsistency since in case of conflicting fields, the data would not be unmarshalled and due to presence of a lot of fields, this would go unnoticed. This commit handles this in a 4 step process. Step 1: Get all the target structs. Look at tags in the fields of the input struct as well as the embedded structs, so both of them become our target structs. Step 2: Get blacklisted tags. Get the fields which have already been handled and were previously conflicting. This needs to be done only for the structs in spec.go, and with fields marked as "conflicting" using a JSON tag Step 3: Get JSON tags from target structs. Get the JSON tags from all of the target structs which we got in Step 1 except for the blacklisted tags, along with the names of the structs which hold the field with that tag. Step 4: Check if more than one struct holds a tag. Delete all the tags in Step 3 which are only help by one struct, because it means that the JSON tag is not conficting, and finally return the remaining tags which are held by more than one struct, and are not handled, hence conflicting tags The code has been heavily documented, and tests have been added for this behavior. The entire code is not a part of the main program and is not run every time a user executes any kedge command, instead it has been put as a test, since it's more of a developer facing problem than a user facing one. Fixes #111 --- pkg/spec/spec.go | 6 +- pkg/spec/spec_test.go | 44 +++++++++ pkg/spec/util.go | 214 ++++++++++++++++++++++++++++++++++++++++++ pkg/spec/util_test.go | 161 +++++++++++++++++++++++++++++++ 4 files changed, 422 insertions(+), 3 deletions(-) create mode 100644 pkg/spec/spec_test.go create mode 100644 pkg/spec/util.go create mode 100644 pkg/spec/util_test.go diff --git a/pkg/spec/spec.go b/pkg/spec/spec.go index 66c300197..026f3d1a4 100644 --- a/pkg/spec/spec.go +++ b/pkg/spec/spec.go @@ -37,7 +37,7 @@ type ServicePortMod struct { type ServiceSpecMod struct { api_v1.ServiceSpec `json:",inline"` Name string `json:"name,omitempty"` - Ports []ServicePortMod `json:"ports"` + Ports []ServicePortMod `json:"ports,conflicting"` } type IngressSpecMod struct { @@ -71,13 +71,13 @@ type ConfigMapMod struct { } type PodSpecMod struct { - Containers []Container `json:"containers,omitempty"` + Containers []Container `json:"containers,conflicting,omitempty"` api_v1.PodSpec `json:",inline"` } type App struct { Name string `json:"name"` - Replicas *int32 `json:"replicas,omitempty"` + Replicas *int32 `json:"replicas,conflicting,omitempty"` Labels map[string]string `json:"labels,omitempty"` PersistentVolumes []PersistentVolume `json:"persistentVolumes,omitempty"` ConfigMaps []ConfigMapMod `json:"configMaps,omitempty"` diff --git a/pkg/spec/spec_test.go b/pkg/spec/spec_test.go new file mode 100644 index 000000000..8b5d16cf7 --- /dev/null +++ b/pkg/spec/spec_test.go @@ -0,0 +1,44 @@ +package spec + +import ( + "testing" + + "github.com/pkg/errors" + "reflect" +) + +func TestConflictingFields(t *testing.T) { + // This array should ideally go away and every struct in spec.go should + // get tested automatically, but for now, let's pass in all the structs + // https://stackoverflow.com/questions/20803758/how-to-get-all-defined-struct-in-golang + structsToTest := []interface{}{ + &PersistentVolume{}, + &ServicePortMod{}, + &ServiceSpecMod{}, + &IngressSpecMod{}, + &EnvFromSource{}, + &ConfigMapEnvSource{}, + &Container{}, + &ConfigMapMod{}, + &PodSpecMod{}, + &App{}, + } + + for _, inputStruct := range structsToTest { + t.Run("Testing conflicting fields", func(t *testing.T) { + + // Checking if input is pointer to struct + if err := isPointerToStruct(inputStruct); err != nil { + t.Error(errors.Wrap(err, "Input parameter type mismatch")) + } + + conflictingTags, err := findConflictingYAMLTags(inputStruct) + if err != nil { + t.Error(errors.Wrap(err, "Unable to find conflicting tags for spec.App")) + } + if len(conflictingTags) != 0 { + t.Fatalf("The struct %v has unhandled conflicting JSON tags which exist in other structs.\n%v", reflect.Indirect(reflect.ValueOf(inputStruct)).Type().String(), conflictingTags) + } + }) + } +} diff --git a/pkg/spec/util.go b/pkg/spec/util.go new file mode 100644 index 000000000..a1c70c442 --- /dev/null +++ b/pkg/spec/util.go @@ -0,0 +1,214 @@ +package spec + +import ( + "fmt" + "reflect" + "strings" + + "github.com/pkg/errors" +) + +// This function takes an map as an input which stores the JSON tag as key and +// the names of the structs holding that tag as the value. The passed in +// blaclisted tags are ignored and are not populated at all. +func getUnmarshalJSONTagsMap(inputMap map[string][]string, inputStruct interface{}, blacklistedTags []string) error { + + // Checking if input is pointer to struct + if err := isPointerToStruct(inputStruct); err != nil { + return errors.Wrap(err, "Input parameter type mismatch") + } + + val := reflect.ValueOf(inputStruct).Elem() + + for i := 0; i < val.NumField(); i++ { + field := val.Type().Field(i) + + // We do not need to store JSON tags like "inline" or "omitempty", + // instead we only need JSON tag which is used for unmarshalling the + // YAML. It's safe to assume that, that JSON tag is the one that + // is specified first. + // Let's split the JSON tag on ",", and store the first element of the + // resulting array. + + unmarshalJsonTag := strings.SplitN(field.Tag.Get("json"), ",", 2)[0] + + skipTag := false + // Populating only the non blacklisted tags + for _, blacklistedTag := range blacklistedTags { + if unmarshalJsonTag == blacklistedTag { + skipTag = true + } + } + if unmarshalJsonTag == "" { + // It does not make any sense to store anything for JSON tags with no + // first element, e.g. `json:",inline"` + skipTag = true + } + + if skipTag { + continue + } + inputMap[unmarshalJsonTag] = append(inputMap[unmarshalJsonTag], val.Type().String()) + } + return nil +} + +// This function returns an array of pointers to all of the embedded structs +// in the input struct. +// The embedded structs in an embedded struct are also taken into considertation +// in a recursive manner. +func getEmbeddedStructs(inputStruct interface{}) ([]interface{}, error) { + + // Checking if input is pointer to struct + if err := isPointerToStruct(inputStruct); err != nil { + return nil, errors.Wrap(err, "Input parameter type mismatch") + } + + var embeddedStructs []interface{} + + val := reflect.ValueOf(inputStruct).Elem() + + for i := 0; i < val.NumField(); i++ { + // Checking for embedded structs + if val.Type().Field(i).Anonymous { + + // Appending pointer to the embedded/anonymous struct to + // embeddedStructs + embeddedStructs = append(embeddedStructs, val.Field(i).Addr().Interface()) + + // Since the current field is an anonymous struct, we call the + // function recursively to get embedded structs under it + recursiveEmbeddedStructs, err := getEmbeddedStructs(val.Field(i).Addr().Interface()) + if err != nil { + return nil, errors.Wrapf(err, "Unable to get embedded structs recursively from %v", val.Field(i).String()) + } + + // Adding the returned recursive structs to embeddedStructs + embeddedStructs = append(embeddedStructs, recursiveEmbeddedStructs...) + } + } + return embeddedStructs, nil +} + +// This function returns an array of all the JSON tags used for unmarshalling +// (which are the first ones specified) which have a JSON tag "conflicting", in +// all of the input structs. +// It's made sure that only the tags in "package spec" are checked and all other +// packages are ignored. This is done because we are expecting the structs to +// be handled, i.e. marked as "conflicting" only in spec.go, and we do not want +// to populate the list with JSON tags from some other package which had the +// "conflicting" JSON tag for some other reason +func getMarkedAsConflictingJSONUnmarshalTags(inputStructs []interface{}) ([]string, error) { + var blacklistedTags []string + for _, inputStruct := range inputStructs { + + // Checking if input is pointer to struct + if err := isPointerToStruct(inputStruct); err != nil { + return nil, errors.Wrap(err, "Input parameter type mismatch") + } + + val := reflect.ValueOf(inputStruct).Elem() + + // Proceeding only if the struct belongs to the kedge's spec package + if val.Type().PkgPath() == "github.com/kedgeproject/kedge/pkg/spec" { + for i := 0; i < val.NumField(); i++ { + field := val.Type().Field(i) + + fieldJSONTags := strings.Split(field.Tag.Get("json"), ",") + + // Adding fields that have "conflicting" as a JSON tag to the + // array. We need not check the first tag that is used for + // unmarshalling. + for _, tag := range fieldJSONTags[1:] { + if tag == "conflicting" { + blacklistedTags = append(blacklistedTags, fieldJSONTags[0]) + } + } + } + } + } + return blacklistedTags, nil +} + +// This function checks if the input parameter has the pointer to a struct as +// its underlying type. +// Returns "nil" if true, or an error if false +func isPointerToStruct(input interface{}) error { + if reflect.Indirect(reflect.ValueOf(input)).Kind().String() != "struct" { + return errors.New(fmt.Sprintf("Expected pointer to struct, got %v", reflect.ValueOf(input).Kind().String())) + } + return nil +} + +// This function takes in pointer to a struct struct as input, and returns a map +// of conflicting JSON tags used for unmarshaling. +// All of the embedded structs are taken into account, and the fields with a +// JSON tag "conflicting" are assumed to be handled. +// The returned map contains the JSON tags as the keys, and the values are an +// array of struct names which contain those fields, without being handled. +func findConflictingYAMLTags(inputStruct interface{}) (map[string][]string, error) { + + // We need to check that no JSON tag is specified more than once at the + // top level of the struct. + // This means that the struct fields, and the JSON tags of all the + // fields of all the embedded structs need to be unique. + // We accomplish this over a 4 step process. + + // Checking if input is pointer to struct + if err := isPointerToStruct(inputStruct); err != nil { + return nil, errors.Wrap(err, "Input parameter type mismatch") + } + + // Step 1: Get all the target structs. + // Look at tags in the fields of the input struct as well as the embedded + // structs, so both of them become our target structs. + + var targetStructs []interface{} + // The first target struct is the input struct to the function + targetStructs = append(targetStructs, inputStruct) + + // Getting all embedded structs for the input struct and appending to the + // list of target structs. + embeddedStructs, err := getEmbeddedStructs(inputStruct) + if err != nil { + return nil, errors.Wrapf(err, "Error getting embedded structs for struct: %v", inputStruct) + } + targetStructs = append(targetStructs, embeddedStructs...) + + // Step 2: Get blacklisted tags. + // Get the fields which have already been handled and were previously + // conflicting. This needs to be done only for the structs in spec.go, + // and with fields marked as "conflicting" using a JSON tag + + blacklistedTags, err := getMarkedAsConflictingJSONUnmarshalTags(targetStructs) + if err != nil { + return nil, errors.Wrap(err, "Failed to get blacklisted tags for target structs") + } + + // Step 3: Get JSON tags from target structs. + // Get the JSON tags from all of the target structs which we got in Step 1 + // except for the blacklisted tags, along with the names of the structs + // which hold the field with that tag. + + tagStructMap := make(map[string][]string) + for _, targetStruct := range targetStructs { + err := getUnmarshalJSONTagsMap(tagStructMap, targetStruct, blacklistedTags) + if err != nil { + return nil, errors.Wrapf(err, "Failed to get JSON tags from the struct: %v", inputStruct) + } + } + + // Step 4: Check if more than one struct holds a tag. + // Delete all the tags in Step 3 which are only help by one struct, because + // it means that the JSON tag is not conficting, and finally return the + // remaining tags which are held by more than one struct, and are not + // handled, hence conflicting tags + + for tag, containingStructs := range tagStructMap { + if len(containingStructs) == 1 { + delete(tagStructMap, tag) + } + } + + return tagStructMap, nil +} diff --git a/pkg/spec/util_test.go b/pkg/spec/util_test.go new file mode 100644 index 000000000..eb7a335d3 --- /dev/null +++ b/pkg/spec/util_test.go @@ -0,0 +1,161 @@ +package spec + +import ( + "reflect" + "testing" +) + +// No conflicting field +type MasterStruct1 struct { + Ms1 string `json:"ms1"` + Ms2 int `json:"ms2"` + Ms3 float64 `json:"ms3"` + EmbeddedStruct1 + EmbeddedStruct2 +} + +// Conflicting field with EmbeddedStruct1 +type MasterStruct2 struct { + Ms1 string `json:"ms1"` + Ms2 int `json:"ms2"` + Ms3 float64 `json:"ms3"` + Es11 int `json:"es11"` + EmbeddedStruct1 + EmbeddedStruct2 +} + +// No conflicting field with embedded structs, but conflicting fields in embedded structs +type MasterStruct3 struct { + Ms1 string `json:"ms1"` + Ms2 int `json:"ms2"` + Ms3 float64 `json:"ms3"` + EmbeddedStruct3 + EmbeddedStruct4 +} + +type EmbeddedStruct1 struct { + Es11 string `json:"es11"` + Es12 int `json:"es12"` + Es13 float64 `json:"es13"` +} + +type EmbeddedStruct2 struct { + Es21 string `json:"es21"` + Es22 int `json:"es22"` + Es23 float64 `json:"es23"` +} + +type EmbeddedStruct3 struct { + Es31 string `json:"es31"` + Es32 int `json:"es32"` + Es33 float64 `json:"es33"` +} + +type EmbeddedStruct4 struct { + Es31 string `json:"es31"` // conflicting field + Es41 string `json:"es41"` + Es42 int `json:"es42"` + Es43 float64 `json:"es43"` +} + +// Conflicting fields in this struct and both the embedded structs +type MasterStruct4 struct { + Ms1 string `json:"ms1"` // conflicting field + Ms2 int `json:"ms2"` + Ms3 float64 `json:"ms3"` + EmbeddedStruct5 + EmbeddedStruct6 +} + +// Conflicting fields in this struct and both the embedded structs, with +// "conflicting" JSON tag +type MasterStruct5 struct { + Ms1 string `json:"ms1,conflicting"` // conflicting field + Ms2 int `json:"ms2"` + Ms3 float64 `json:"ms3"` + EmbeddedStruct5 + EmbeddedStruct6 +} + +type EmbeddedStruct5 struct { + MasterField1 int `json:"ms1"` // conflicting field + Es51 string `json:"es51"` + Es52 int `json:"es52"` + Es53 float64 `json:"es53"` +} + +type EmbeddedStruct6 struct { + MasterFoo1 float64 `json:"ms1"` // conflicting field + Es63 float64 `json:"es63"` +} + +func TestFindConflictingYAMLTags(t *testing.T) { + tests := []struct { + Name string + InputStuct interface{} + ConflictingTags map[string][]string + IsError bool + }{ + { + Name: "Pointer to struct not passed", + InputStuct: []int{42}, + ConflictingTags: nil, + IsError: true, + }, + { + Name: "No conflicting field", + InputStuct: &MasterStruct1{}, + ConflictingTags: map[string][]string{}, + IsError: false, + }, + { + Name: "Conflicting field in top level struct and embedded struct", + InputStuct: &MasterStruct2{}, + ConflictingTags: map[string][]string{ + "es11": {"spec.MasterStruct2", "spec.EmbeddedStruct1"}, + }, + IsError: false, + }, + { + Name: "Conflicting field in embedded structs but not in top level struct", + InputStuct: &MasterStruct3{}, + ConflictingTags: map[string][]string{ + "es31": {"spec.EmbeddedStruct3", "spec.EmbeddedStruct4"}, + }, + IsError: false, + }, + { + Name: "Conflicting field in top level and all embedded structs", + InputStuct: &MasterStruct4{}, + ConflictingTags: map[string][]string{ + "ms1": {"spec.MasterStruct4", "spec.EmbeddedStruct5", "spec.EmbeddedStruct6"}, + }, + IsError: false, + }, + { + Name: "Conflicting tags should be empty when 'conflicting' JSON tag present", + InputStuct: &MasterStruct5{}, + ConflictingTags: map[string][]string{}, + IsError: false, + }, + + // TODO: Conflicting tags should not be empty when 'conflicting' JSON tag present in embedded struct not in "spec" package + } + for _, test := range tests { + t.Run(test.Name, func(t *testing.T) { + conflictingTags, err := findConflictingYAMLTags(test.InputStuct) + + // Testing errors + if test.IsError && err == nil { + t.Fatal("Expected function to return an error, but no error returned") + } else if !test.IsError && err != nil { + t.Fatalf("No error expected, but got %v", err) + } + + // Testing conflicting tags + if !reflect.DeepEqual(conflictingTags, test.ConflictingTags) { + t.Fatalf("Expected conflicting tags to be:\n%v\nBut got:\n%v\n", test.ConflictingTags, conflictingTags) + } + }) + } +}