Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add Stronger Types for Enums and Improve Set Encoding #151

Closed
wants to merge 3 commits into from
Closed

Add Stronger Types for Enums and Improve Set Encoding #151

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5dc9643
Add stronger typing for Enums
dave-tucker Jun 9, 2021
27341d5
modelgen: Add constraints to sets
dave-tucker Jul 13, 2021
ccd9a6f
Update mapper/bindings to support new constraints
dave-tucker Jul 14, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 26 additions & 5 deletions mapper/info.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,12 +38,28 @@ func (i *Info) SetField(column string, value interface{}) error {
return fmt.Errorf("column %s not found in orm info", column)
}
fieldValue := reflect.ValueOf(i.obj).Elem().FieldByName(fieldName)

v := reflect.ValueOf(value)
if !fieldValue.Type().AssignableTo(reflect.TypeOf(value)) {
return fmt.Errorf("column %s: native value %v (%s) is not assignable to field %s (%s)",
column, value, reflect.TypeOf(value), fieldName, fieldValue.Type())
if v.Type().ConvertibleTo(fieldValue.Type()) {
// handle enum
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would explicitly check the tableSchema to make sure it's a Enum

v = v.Convert(fieldValue.Type())
} else if fieldValue.Kind() == reflect.Slice {
// handle set of enums
if !v.Type().Elem().ConvertibleTo(fieldValue.Type().Elem()) {
return fmt.Errorf("column %s: element %v (%s) is not convertible to field %s element (%s)",
column, value, reflect.TypeOf(value), fieldName, fieldValue.Type())
}
nv := reflect.Zero(fieldValue.Type())
for i := 0; i < v.Len(); i++ {
nv = reflect.Append(nv, v.Index(i).Convert(fieldValue.Type().Elem()))
}
v = nv
} else {
return fmt.Errorf("column %s: native value %v (%s) is not assignable or convertible to field %s (%s)",
column, value, reflect.TypeOf(value), fieldName, fieldValue.Type())
}
}
fieldValue.Set(reflect.ValueOf(value))
fieldValue.Set(v)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a shame that we have to split the conversion of a type between Ovs2Native and this function.
I don't see a clear way out of it.
Maybe add a comment to the function explaining that some fields (despite being already "native") need an extra translation step?

return nil
}

Expand Down Expand Up @@ -133,7 +149,12 @@ func NewInfo(table *ovsdb.TableSchema, obj interface{}) (*Info, error) {

// Perform schema-based type checking
expType := ovsdb.NativeType(column)
if expType != field.Type {
// check for slice of enums
if expType.Kind() == reflect.Slice && expType.Elem().Kind() == reflect.String {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should distinguish between slice of strings and slice of enums. We should check the ColumnSchema here

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alternatively, do we want to allow all kind of type aliases? e.g:

type Priority int
type ACL struct {
     Priority Priority
}

// it's a slice of enums
} else if expType.Kind() == reflect.String && field.Type.Kind() == reflect.String {
// it' an enum
} else if expType != field.Type {
return nil, &ErrMapper{
objType: objType.String(),
field: field.Name,
Expand Down
90 changes: 73 additions & 17 deletions mapper/info_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,25 +10,57 @@ import (
)

var sampleTable = []byte(`{
"columns": {
"columns": {
"aString": {
"type": "string"
"type": "string"
},
"aInteger": {
"type": "integer"
"type": "integer"
},
"aSet": {
"type": {
"key": "string",
"max": "unlimited",
"min": 0
}
"type": {
"key": "string",
"max": "unlimited",
"min": 0
}
},
"aMap": {
"type": {
"key": "string",
"value": "string"
}
"type": {
"key": "string",
"value": "string"
}
},
"aEnum": {
"type": {
"key": {
"enum": [
"set",
[
"enum1",
"enum2",
"enum3"
]
],
"type": "string"
}
}
},
"aEnumSet": {
"type": {
"key": {
"enum": [
"set",
[
"enum1",
"enum2",
"enum3"
]
],
"type": "string"
},
"max": "unlimited",
"min": 0
}
}
}
}`)
Expand Down Expand Up @@ -73,11 +105,19 @@ func TestNewMapperInfo(t *testing.T) {
}

func TestMapperInfoSet(t *testing.T) {
type enum string
const (
enum1 enum = "one"
enum2 enum = "two"
enum3 enum = "three"
)
type obj struct {
Ostring string `ovsdb:"aString"`
Oint int `ovsdb:"aInteger"`
Oset []string `ovsdb:"aSet"`
Omap map[string]string `ovsdb:"aMap"`
Ostring string `ovsdb:"aString"`
Oint int `ovsdb:"aInteger"`
Oset []string `ovsdb:"aSet"`
Omap map[string]string `ovsdb:"aMap"`
Oenum enum `ovsdb:"aEnum"`
OenumSet []enum `ovsdb:"aEnumSet"`
}

type test struct {
Expand Down Expand Up @@ -127,13 +167,29 @@ func TestMapperInfoSet(t *testing.T) {
err: false,
},
{
name: "un-assignable",
name: "unassignable",
table: sampleTable,
obj: &obj{},
field: []string{"foo"},
column: "aMap",
err: true,
},
{
name: "enum",
table: sampleTable,
obj: &obj{},
field: enum1,
column: "aEnum",
err: false,
},
{
name: "enumSet",
table: sampleTable,
obj: &obj{},
field: []enum{enum2, enum3},
column: "aEnumSet",
err: false,
},
}
for _, tt := range tests {
t.Run(fmt.Sprintf("SetField_%s", tt.name), func(t *testing.T) {
Expand Down
2 changes: 1 addition & 1 deletion modelgen/table.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

12 changes: 6 additions & 6 deletions modelgen/table_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,8 +54,8 @@ func TestNewTableTemplate(t *testing.T) {
package test

type (
AtomicTableEventType = string
AtomicTableProtocol = string
AtomicTableEventType string
AtomicTableProtocol string
)

const (
Expand Down Expand Up @@ -114,8 +114,8 @@ type AtomicTable struct {
package test

type (
AtomicTableEventType = string
AtomicTableProtocol = string
AtomicTableEventType string
AtomicTableProtocol string
)

const (
Expand Down Expand Up @@ -163,8 +163,8 @@ func {{ index . "TestName" }} () string {
package test

type (
AtomicTableEventType = string
AtomicTableProtocol = string
AtomicTableEventType string
AtomicTableProtocol string
)

const (
Expand Down
23 changes: 20 additions & 3 deletions ovsdb/bindings.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -182,11 +182,28 @@ func NativeToOvsAtomic(basicType string, nativeElem interface{}) (interface{}, e
// NativeToOvs transforms an native type to a ovs type based on the column type information
func NativeToOvs(column *ColumnSchema, rawElem interface{}) (interface{}, error) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In order to keep things more or less tidy and symetric, how about moving this logic to mapper.info?
That way we would have a clear logical separation:

bindings.NativeToOvs & bindings.OvsToNative: Convert from/to OVS types to a neutral, common, generic, native type that can hold the column

info.SetField & info.FieldByPtr & info.FieldByColumn : May perform an extra conversion step to adjust the neutral, common, generic native type to whatever type the Field actually has

WDYT?

naType := NativeType(column)

if t := reflect.TypeOf(rawElem); t != naType {
rawElemType := reflect.TypeOf(rawElem)
if column.Type == TypeEnum && rawElemType != strType && rawElemType.Kind() == reflect.String {
// cast type alias back to string
rawElem = rawElem.(string)
rawElemType = reflect.TypeOf(rawElem)
}
if column.Type == TypeSet && len(column.TypeObj.Key.Enum) > 0 && rawElemType.Elem() != strType && rawElemType.Elem().Kind() == reflect.String {
// convert a set of enums where the type is an alias, to a set of strings
tmp := []string{}
v := reflect.ValueOf(rawElem)
for i := 0; i < v.Len(); i++ {
// convert value to string first, because the String method won't panic
// but will instead return a string of <T value> which isn't what we want
vi := v.Index(i).Convert(strType)
tmp = append(tmp, vi.String())
}
rawElem = tmp
rawElemType = reflect.TypeOf(rawElem)
}
if t := rawElemType; t != naType {
return nil, NewErrWrongType("NativeToOvs", naType.String(), rawElem)
}

switch column.Type {
case TypeInteger, TypeReal, TypeString, TypeBoolean, TypeEnum:
return rawElem, nil
Expand Down