From 64fb8dfbb8b8e8514078bdf581c1c6803c09e262 Mon Sep 17 00:00:00 2001 From: "Mark S. Lewis" Date: Mon, 13 Nov 2023 11:56:33 +0000 Subject: [PATCH] Allow schema types with the same name from different packages Previously the short type name was used as a schema key. This meant that types of the same name from different packages would overwrite each other in the map of schema type definitions, causing unexpected serialization errors. Now the fully qualified (package + type) name is used so that entries for types with the same name from different packages are unique. Signed-off-by: Mark S. Lewis --- internal/types/types_test.go | 2 +- metadata/schema.go | 57 +++++------ metadata/schema_test.go | 96 ++++++++++--------- serializer/internal/test_data.go | 8 ++ .../json_transaction_serializer_test.go | 35 ++++--- 5 files changed, 111 insertions(+), 87 deletions(-) create mode 100644 serializer/internal/test_data.go diff --git a/internal/types/types_test.go b/internal/types/types_test.go index 9e1d5f0..879db46 100644 --- a/internal/types/types_test.go +++ b/internal/types/types_test.go @@ -74,7 +74,7 @@ func TestBoolType(t *testing.T) { assert.False(t, val.Interface().(bool), "should have returned the boolean false for blank value") val, err = boolTypeVar.Convert("non bool") - assert.Error(t, fmt.Errorf(convertError, "non bool"), "should return error for invalid bool value") + assert.Error(t, err, fmt.Errorf(convertError, "non bool"), "should return error for invalid bool value") assert.Equal(t, reflect.Value{}, val, "should have returned the blank value for non bool") } diff --git a/metadata/schema.go b/metadata/schema.go index ce69dc9..1bde852 100644 --- a/metadata/schema.go +++ b/metadata/schema.go @@ -24,32 +24,26 @@ func GetSchema(field reflect.Type, components *ComponentMetadata) (*spec.Schema, } func getSchema(field reflect.Type, components *ComponentMetadata, nested bool) (*spec.Schema, error) { - var schema *spec.Schema - var err error - - if bt, ok := types.BasicTypes[field.Kind()]; !ok { - if field == types.TimeType { - schema = spec.DateTimeProperty() - } else if field.Kind() == reflect.Array { - schema, err = buildArraySchema(reflect.New(field).Elem(), components, nested) - } else if field.Kind() == reflect.Slice { - schema, err = buildSliceSchema(reflect.MakeSlice(field, 1, 1), components, nested) - } else if field.Kind() == reflect.Map { - schema, err = buildMapSchema(reflect.MakeMap(field), components, nested) - } else if field.Kind() == reflect.Struct || (field.Kind() == reflect.Ptr && field.Elem().Kind() == reflect.Struct) { - schema, err = buildStructSchema(field, components, nested) - } else { - return nil, fmt.Errorf("%s was not a valid type", field.String()) - } - } else { + if bt, ok := types.BasicTypes[field.Kind()]; ok { return bt.GetSchema(), nil } - - if err != nil { - return nil, err + if field == types.TimeType { + return spec.DateTimeProperty(), nil + } + if field.Kind() == reflect.Array { + return buildArraySchema(reflect.New(field).Elem(), components, nested) + } + if field.Kind() == reflect.Slice { + return buildSliceSchema(reflect.MakeSlice(field, 1, 1), components, nested) + } + if field.Kind() == reflect.Map { + return buildMapSchema(reflect.MakeMap(field), components, nested) + } + if field.Kind() == reflect.Struct || (field.Kind() == reflect.Ptr && field.Elem().Kind() == reflect.Struct) { + return buildStructSchema(field, components, nested) } - return schema, nil + return nil, fmt.Errorf("%s was not a valid type", field.String()) } func buildArraySchema(array reflect.Value, components *ComponentMetadata, nested bool) (*spec.Schema, error) { @@ -95,12 +89,14 @@ func addComponentIfNotExists(obj reflect.Type, components *ComponentMetadata) er obj = obj.Elem() } - if _, ok := components.Schemas[obj.Name()]; ok { + key := schemaKey(obj) + + if _, ok := components.Schemas[key]; ok { return nil } schema := ObjectMetadata{} - schema.ID = obj.Name() + schema.ID = key schema.Required = []string{} schema.Properties = make(map[string]spec.Schema) schema.AdditionalProperties = false @@ -109,18 +105,18 @@ func addComponentIfNotExists(obj reflect.Type, components *ComponentMetadata) er components.Schemas = make(map[string]ObjectMetadata) } - components.Schemas[obj.Name()] = schema // lock up slot for cyclic + components.Schemas[key] = schema // lock up slot for cyclic for i := 0; i < obj.NumField(); i++ { err := getField(obj.Field(i), &schema, components) if err != nil { - delete(components.Schemas, obj.Name()) + delete(components.Schemas, key) return err } } - components.Schemas[obj.Name()] = schema // include changes + components.Schemas[key] = schema // include changes return nil } @@ -205,5 +201,10 @@ func buildStructSchema(obj reflect.Type, components *ComponentMetadata, nested b refPath = "" } - return spec.RefSchema(refPath + obj.Name()), nil + return spec.RefSchema(refPath + schemaKey(obj)), nil +} + +func schemaKey(obj reflect.Type) string { + return strings.ReplaceAll(obj.PkgPath(), "/", ".") + "." + obj.Name() + // return obj.Name() } diff --git a/metadata/schema_test.go b/metadata/schema_test.go index 4590c88..993f2bb 100644 --- a/metadata/schema_test.go +++ b/metadata/schema_test.go @@ -45,8 +45,10 @@ var simpleStructPropertiesMap = map[string]spec.Schema{ "jsonname2": *spec.StringProperty(), } +var simpleStructKey = schemaKey(reflect.TypeOf(simpleStruct{})) + var simpleStructMetadata = ObjectMetadata{ - ID: "simpleStruct", + ID: simpleStructKey, Properties: simpleStructPropertiesMap, Required: []string{"Prop1", "propname", "Prop5", "jsonname2"}, AdditionalProperties: false, @@ -59,15 +61,17 @@ type complexStruct struct { Prop3 *complexStruct `metadata:",optional"` } +var complexStructKey = schemaKey(reflect.TypeOf(complexStruct{})) + var complexStructPropertiesMap = map[string]spec.Schema{ "Prop0": *spec.StringProperty(), "Prop1": *spec.StringProperty(), - "Prop2": *spec.RefSchema("simpleStruct"), - "Prop3": *spec.RefSchema("complexStruct"), + "Prop2": *spec.RefSchema(simpleStructKey), + "Prop3": *spec.RefSchema(complexStructKey), } var complexStructMetadata = ObjectMetadata{ - ID: "complexStruct", + ID: complexStructKey, Properties: complexStructPropertiesMap, Required: []string{"Prop0", "Prop1", "Prop2"}, AdditionalProperties: false, @@ -84,16 +88,18 @@ type superComplexStruct struct { var superComplexStructPropertiesMap = map[string]spec.Schema{ "Prop0": *spec.StringProperty(), "Prop1": *spec.StringProperty(), - "Prop2": *spec.RefSchema("simpleStruct"), - "Prop3": *spec.RefSchema("complexStruct"), - "Prop4": *spec.ArrayProperty(spec.RefSchema("complexStruct")), - "Prop5": *spec.ArrayProperty(spec.RefSchema("simpleStruct")), - "Prop6": *spec.MapProperty(spec.RefSchema("complexStruct")), - "Prop7": *spec.MapProperty(spec.ArrayProperty(spec.RefSchema("simpleStruct"))), + "Prop2": *spec.RefSchema(simpleStructKey), + "Prop3": *spec.RefSchema(complexStructKey), + "Prop4": *spec.ArrayProperty(spec.RefSchema(complexStructKey)), + "Prop5": *spec.ArrayProperty(spec.RefSchema(simpleStructKey)), + "Prop6": *spec.MapProperty(spec.RefSchema(complexStructKey)), + "Prop7": *spec.MapProperty(spec.ArrayProperty(spec.RefSchema(simpleStructKey))), } +var superComplexStructKey = schemaKey(reflect.TypeOf(superComplexStruct{})) + var superComplexStructMetadata = ObjectMetadata{ - ID: "superComplexStruct", + ID: superComplexStructKey, Properties: superComplexStructPropertiesMap, Required: append(complexStructMetadata.Required, "Prop4", "Prop5", "Prop6", "Prop7"), AdditionalProperties: false, @@ -201,18 +207,18 @@ func TestAddComponentIfNotExists(t *testing.T) { components = new(ComponentMetadata) components.Schemas = make(map[string]ObjectMetadata) - components.Schemas["simpleStruct"] = someObject + components.Schemas[simpleStructKey] = someObject err = addComponentIfNotExists(reflect.TypeOf(simpleStruct{}), components) - assert.Nil(t, err, "should return nil for error when component of name already exists") + assert.NoError(t, err, "should return nil for error when component of name already exists") assert.Equal(t, len(components.Schemas), 1, "should not have added a new component when one already exists") - _, ok = components.Schemas["simpleStruct"].Properties["some property"] + _, ok = components.Schemas[simpleStructKey].Properties["some property"] assert.True(t, ok, "should not overwrite existing component") err = addComponentIfNotExists(reflect.TypeOf(new(simpleStruct)), components) assert.Nil(t, err, "should return nil for error when component of name already exists for pointer") assert.Equal(t, len(components.Schemas), 1, "should not have added a new component when one already exists for pointer") - _, ok = components.Schemas["simpleStruct"].Properties["some property"] + _, ok = components.Schemas[simpleStructKey].Properties["some property"] assert.True(t, ok, "should not overwrite existing component when already exists and pointer passed") err = addComponentIfNotExists(reflect.TypeOf(badStruct{}), components) @@ -222,13 +228,13 @@ func TestAddComponentIfNotExists(t *testing.T) { components.Schemas = nil err = addComponentIfNotExists(reflect.TypeOf(simpleStruct{}), components) assert.Nil(t, err, "should not error when adding new component when schemas not initialised") - assert.Equal(t, components.Schemas["simpleStruct"], simpleStructMetadata, "should set correct metadata for new component when schemas not initialised") + assert.Equal(t, components.Schemas[simpleStructKey], simpleStructMetadata, "should set correct metadata for new component when schemas not initialised") - delete(components.Schemas, "simpleStruct") + delete(components.Schemas, simpleStructKey) components.Schemas["otherStruct"] = someObject err = addComponentIfNotExists(reflect.TypeOf(simpleStruct{}), components) assert.Nil(t, err, "should not error when adding new component") - assert.Equal(t, components.Schemas["simpleStruct"], simpleStructMetadata, "should set correct metadata for new component") + assert.Equal(t, components.Schemas[simpleStructKey], simpleStructMetadata, "should set correct metadata for new component") assert.Equal(t, components.Schemas["otherStruct"], someObject, "should not affect existing components") } @@ -247,21 +253,21 @@ func TestBuildStructSchema(t *testing.T) { schema, err = buildStructSchema(reflect.TypeOf(simpleStruct{}), components, false) assert.Nil(t, err, "should not return error when struct is good") - assert.Equal(t, schema, spec.RefSchema("#/components/schemas/simpleStruct"), "should make schema ref to component") - _, ok := components.Schemas["simpleStruct"] + assert.Equal(t, spec.RefSchema("#/components/schemas/"+simpleStructKey), schema, "should make schema ref to component") + _, ok := components.Schemas[simpleStructKey] assert.True(t, ok, "should have added component") schema, err = buildStructSchema(reflect.TypeOf(simpleStruct{}), components, true) assert.Nil(t, err, "should not return error when struct is good") - assert.Equal(t, schema, spec.RefSchema("simpleStruct"), "should make schema ref to component for nested ref") - _, ok = components.Schemas["simpleStruct"] + assert.Equal(t, spec.RefSchema(simpleStructKey), schema, "should make schema ref to component for nested ref") + _, ok = components.Schemas[simpleStructKey] assert.True(t, ok, "should have added component for nested ref") schema, err = buildStructSchema(reflect.TypeOf(new(simpleStruct)), components, false) assert.Nil(t, err, "should not return error when pointer to struct is good") - assert.Equal(t, schema, spec.RefSchema("#/components/schemas/simpleStruct"), "should make schema ref to component") + assert.Equal(t, spec.RefSchema("#/components/schemas/"+simpleStructKey), schema, "should make schema ref to component") - _, ok = components.Schemas["simpleStruct"] + _, ok = components.Schemas[simpleStructKey] assert.True(t, ok, "should have use already added component") } @@ -444,9 +450,9 @@ func TestGetSchema(t *testing.T) { schema, err = GetSchema(reflect.TypeOf(simpleStruct{}), components) assert.Nil(t, err, "should return nil when valid object") - assert.Equal(t, len(components.Schemas), 1, "should have added a new component") - assert.Equal(t, components.Schemas["simpleStruct"], simpleStructMetadata, "should have added correct metadata to components") - assert.Equal(t, schema, spec.RefSchema("#/components/schemas/simpleStruct")) + assert.Equal(t, 1, len(components.Schemas), "should have added a new component") + assert.Equal(t, simpleStructMetadata, components.Schemas[simpleStructKey], "should have added correct metadata to components") + assert.Equal(t, spec.RefSchema("#/components/schemas/"+simpleStructKey), schema) // should handle pointer to struct components = new(ComponentMetadata) @@ -455,9 +461,9 @@ func TestGetSchema(t *testing.T) { schema, err = GetSchema(reflect.TypeOf(new(simpleStruct)), components) assert.Nil(t, err, "should return nil when valid object") - assert.Equal(t, len(components.Schemas), 1, "should have added a new component") - assert.Equal(t, components.Schemas["simpleStruct"], simpleStructMetadata, "should have added correct metadata to components") - assert.Equal(t, schema, spec.RefSchema("#/components/schemas/simpleStruct")) + assert.Equal(t, 1, len(components.Schemas), "should have added a new component") + assert.Equal(t, simpleStructMetadata, components.Schemas[simpleStructKey], "should have added correct metadata to components") + assert.Equal(t, spec.RefSchema("#/components/schemas/"+simpleStructKey), schema) // Should handle an array of structs components = new(ComponentMetadata) @@ -466,9 +472,9 @@ func TestGetSchema(t *testing.T) { schema, err = GetSchema(reflect.TypeOf([1]simpleStruct{}), components) assert.Nil(t, err, "should return nil when valid object") - assert.Equal(t, len(components.Schemas), 1, "should have added a new component") - assert.Equal(t, components.Schemas["simpleStruct"], simpleStructMetadata, "should have added correct metadata to components") - assert.Equal(t, schema, spec.ArrayProperty(spec.RefSchema("#/components/schemas/simpleStruct"))) + assert.Equal(t, 1, len(components.Schemas), "should have added a new component") + assert.Equal(t, simpleStructMetadata, components.Schemas[simpleStructKey], "should have added correct metadata to components") + assert.Equal(t, spec.ArrayProperty(spec.RefSchema("#/components/schemas/"+simpleStructKey)), schema) // Should handle a slice of structs components = new(ComponentMetadata) @@ -477,9 +483,9 @@ func TestGetSchema(t *testing.T) { schema, err = GetSchema(reflect.TypeOf([]simpleStruct{}), components) assert.Nil(t, err, "should return nil when valid object") - assert.Equal(t, len(components.Schemas), 1, "should have added a new component") - assert.Equal(t, components.Schemas["simpleStruct"], simpleStructMetadata, "should have added correct metadata to components") - assert.Equal(t, schema, spec.ArrayProperty(spec.RefSchema("#/components/schemas/simpleStruct"))) + assert.Equal(t, 1, len(components.Schemas), "should have added a new component") + assert.Equal(t, simpleStructMetadata, components.Schemas[simpleStructKey], "should have added correct metadata to components") + assert.Equal(t, spec.ArrayProperty(spec.RefSchema("#/components/schemas/"+simpleStructKey)), schema) // Should handle a valid struct with struct property and add to components components = new(ComponentMetadata) @@ -488,10 +494,10 @@ func TestGetSchema(t *testing.T) { schema, err = GetSchema(reflect.TypeOf(new(complexStruct)), components) assert.Nil(t, err, "should return nil when valid object") - assert.Equal(t, len(components.Schemas), 2, "should have added two new components") - assert.Equal(t, components.Schemas["simpleStruct"], simpleStructMetadata, "should have added correct metadata to components for sub struct") - assert.Equal(t, components.Schemas["complexStruct"], complexStructMetadata, "should have added correct metadata to components for main struct") - assert.Equal(t, schema, spec.RefSchema("#/components/schemas/complexStruct")) + assert.Equal(t, 2, len(components.Schemas), "should have added two new components") + assert.Equal(t, simpleStructMetadata, components.Schemas[simpleStructKey], "should have added correct metadata to components for sub struct") + assert.Equal(t, complexStructMetadata, components.Schemas[complexStructKey], "should have added correct metadata to components for main struct") + assert.Equal(t, spec.RefSchema("#/components/schemas/"+complexStructKey), schema) // Should handle a valid struct with struct properties of array, slice and map types components = new(ComponentMetadata) @@ -500,11 +506,11 @@ func TestGetSchema(t *testing.T) { schema, err = GetSchema(reflect.TypeOf(new(superComplexStruct)), components) assert.Nil(t, err, "should return nil when valid object") - assert.Equal(t, len(components.Schemas), 3, "should have added two new components") - assert.Equal(t, components.Schemas["simpleStruct"], simpleStructMetadata, "should have added correct metadata to components for sub struct") - assert.Equal(t, components.Schemas["complexStruct"], complexStructMetadata, "should have added correct metadata to components for sub struct") - assert.Equal(t, components.Schemas["superComplexStruct"], superComplexStructMetadata, "should have added correct metadata to components for main struct") - assert.Equal(t, schema, spec.RefSchema("#/components/schemas/superComplexStruct")) + assert.Equal(t, 3, len(components.Schemas), "should have added two new components") + assert.Equal(t, simpleStructMetadata, components.Schemas[simpleStructKey], "should have added correct metadata to components for sub struct") + assert.Equal(t, complexStructMetadata, components.Schemas[complexStructKey], "should have added correct metadata to components for sub struct") + assert.Equal(t, superComplexStructMetadata, components.Schemas[superComplexStructKey], "should have added correct metadata to components for main struct") + assert.Equal(t, spec.RefSchema("#/components/schemas/"+superComplexStructKey), schema) // Should return an error for a bad struct components = new(ComponentMetadata) diff --git a/serializer/internal/test_data.go b/serializer/internal/test_data.go new file mode 100644 index 0000000..35050f0 --- /dev/null +++ b/serializer/internal/test_data.go @@ -0,0 +1,8 @@ +// Copyright the Hyperledger Fabric contributors. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +package internal + +type SimpleStruct struct { + Prop1 string `json:"Prop1"` +} diff --git a/serializer/json_transaction_serializer_test.go b/serializer/json_transaction_serializer_test.go index 13b020f..3788ebe 100644 --- a/serializer/json_transaction_serializer_test.go +++ b/serializer/json_transaction_serializer_test.go @@ -13,6 +13,7 @@ import ( "github.com/go-openapi/spec" "github.com/hyperledger/fabric-contract-api-go/internal/types" "github.com/hyperledger/fabric-contract-api-go/metadata" + "github.com/hyperledger/fabric-contract-api-go/serializer/internal" "github.com/stretchr/testify/assert" "github.com/xeipuuv/gojsonschema" ) @@ -21,7 +22,7 @@ import ( // HELPERS // ================================ -type simpleStruct struct { +type SimpleStruct struct { Prop1 string `json:"prop1"` prop2 string } @@ -36,8 +37,8 @@ type usefulStruct struct { channel chan string basic string array [1]string - strct simpleStruct - strctPtr *simpleStruct + strct SimpleStruct + strctPtr *SimpleStruct } func (us usefulStruct) DoNothing() string { @@ -146,9 +147,9 @@ func TestCreateArraySliceMapOrStruct(t *testing.T) { assert.Nil(t, err, "should not error for valid slice JSON") assert.Equal(t, []string{"slice", "slice", "baby"}, val.Interface().([]string), "should produce populated slice") - val, err = createArraySliceMapOrStruct("{\"Prop1\": \"value\"}", reflect.TypeOf(simpleStruct{})) + val, err = createArraySliceMapOrStruct("{\"Prop1\": \"value\"}", reflect.TypeOf(SimpleStruct{})) assert.Nil(t, err, "should not error for valid struct json") - assert.Equal(t, simpleStruct{"value", ""}, val.Interface().(simpleStruct), "should produce populated struct") + assert.Equal(t, SimpleStruct{"value", ""}, val.Interface().(SimpleStruct), "should produce populated struct") val, err = createArraySliceMapOrStruct("{\"key\": 1}", reflect.TypeOf(make(map[string]int))) assert.Nil(t, err, "should not error for valid map JSON") @@ -188,8 +189,8 @@ func TestConvertArg(t *testing.T) { testConvertArgsComplexType(t, [1]int{}, "[1,2,3]") testConvertArgsComplexType(t, []string{}, "[\"a\",\"string\",\"array\"]") testConvertArgsComplexType(t, make(map[string]bool), "{\"a\": true, \"b\": false}") - testConvertArgsComplexType(t, simpleStruct{}, "{\"Prop1\": \"hello\"}") - testConvertArgsComplexType(t, &simpleStruct{}, "{\"Prop1\": \"hello\"}") + testConvertArgsComplexType(t, SimpleStruct{}, "{\"Prop1\": \"hello\"}") + testConvertArgsComplexType(t, &SimpleStruct{}, "{\"Prop1\": \"hello\"}") // should handle time actualValue, actualErr = convertArg(types.TimeType, "2002-10-02T15:00:00Z") @@ -217,13 +218,21 @@ func TestValidateAgainstSchema(t *testing.T) { err = validateAgainstSchema("prop", reflect.TypeOf(10), "10", 10, comparisonSchema) assert.Nil(t, err, "should not error when matches schema") - expectedStruct := new(simpleStruct) + expectedStruct := new(SimpleStruct) expectedStruct.Prop1 = "hello" bytes, _ := json.Marshal(expectedStruct) schema, _ := metadata.GetSchema(reflect.TypeOf(expectedStruct), components) comparisonSchema = createGoJSONSchemaSchema("prop", schema, components) err = validateAgainstSchema("prop", reflect.TypeOf(expectedStruct), string(bytes), expectedStruct, comparisonSchema) assert.Nil(t, err, "should handle struct") + + expectedStruct2 := new(internal.SimpleStruct) + expectedStruct2.Prop1 = "world" + bytes, _ = json.Marshal(expectedStruct2) + schema, _ = metadata.GetSchema(reflect.TypeOf(expectedStruct2), components) + comparisonSchema = createGoJSONSchemaSchema("prop", schema, components) + err = validateAgainstSchema("prop", reflect.TypeOf(expectedStruct2), string(bytes), expectedStruct2, comparisonSchema) + assert.NoError(t, err, "should handle struct with same name in different package") } func TestFromString(t *testing.T) { @@ -255,7 +264,7 @@ func TestFromString(t *testing.T) { assert.Nil(t, err, "should not error when convert args passes and no schema") assert.Equal(t, reflect.ValueOf(1234).Interface(), value.Interface(), "should reflect value for converted arg") - expectedStruct := new(simpleStruct) + expectedStruct := new(SimpleStruct) expectedStruct.Prop1 = "hello" components := new(metadata.ComponentMetadata) schema, _ = metadata.GetSchema(reflect.TypeOf(expectedStruct), components) @@ -276,12 +285,12 @@ func TestToString(t *testing.T) { serializer := new(JSONSerializer) - var nilResult *simpleStruct - value, err = serializer.ToString(reflect.ValueOf(nilResult), reflect.TypeOf(new(simpleStruct)), nil, nil) + var nilResult *SimpleStruct + value, err = serializer.ToString(reflect.ValueOf(nilResult), reflect.TypeOf(new(SimpleStruct)), nil, nil) assert.Nil(t, err, "should not error when receives nil") assert.Equal(t, "", value, "should return blank string for nil value") - result := new(simpleStruct) + result := new(SimpleStruct) result.Prop1 = "property 1" value, err = serializer.ToString(reflect.ValueOf(result), reflect.TypeOf(result), nil, nil) assert.Nil(t, err, "should not error when receives non nil nillable type") @@ -306,7 +315,7 @@ func TestToString(t *testing.T) { assert.EqualError(t, err, expectedErr.Error(), "should error when validateAgainstSchema errors") assert.Equal(t, "", value, "should return an empty string value when it errors due to validateAgainstSchema") - expectedStruct := new(simpleStruct) + expectedStruct := new(SimpleStruct) expectedStruct.Prop1 = "hello" components := new(metadata.ComponentMetadata) schema, _ = metadata.GetSchema(reflect.TypeOf(expectedStruct), components)