Skip to content

Commit

Permalink
Allow schema types with the same name from different packages
Browse files Browse the repository at this point in the history 
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 <Mark.S.Lewis@outlook.com>
  • Loading branch information
@bestbeforetoday
bestbeforetoday committed Nov 13, 2023
1 parent fda7a56 commit 64fb8df
Show file tree
Hide file tree
Showing 5 changed files with 111 additions and 87 deletions.
2 changes: 1 addition & 1 deletion internal/types/types_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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")
}

Expand Down
57 changes: 29 additions & 28 deletions metadata/schema.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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
Expand All @@ -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
}
Expand Down Expand Up @@ -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()
}
96 changes: 51 additions & 45 deletions metadata/schema_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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,
Expand All @@ -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,
Expand Down Expand Up @@ -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)
Expand All @@ -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")
}

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

Expand Down Expand Up @@ -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)
Expand All @@ -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)
Expand All @@ -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)
Expand All @@ -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)
Expand All @@ -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)
Expand All @@ -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)
Expand Down
8 changes: 8 additions & 0 deletions serializer/internal/test_data.go
View file
Original file line number Diff line number Diff line change
@@ -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"`
}
Loading

0 comments on commit 64fb8df

Please sign in to comment.