Add Stronger Types for Enums and Improve Set Encoding #151

dave-tucker · 2021-06-09T22:00:20Z

This PR:

Adds stronger typing for Enums
Encodes sets with min 0, max 1 as pointers
Encodes sets where max is not limited an array
Updates Modelgen

These changes are together, as it was an optional (min 0, max 1) enum that prompted this discussion in the first place.

In the generated code, enum constants can no lionger be constant, but must instead be var because we can't take a const string by address for use in structs with *string fields (or * AnEnumType fields).

Closes: #154
Closes: #191

coveralls · 2021-06-10T14:40:33Z

Pull Request Test Coverage Report for Build 1034832956

94 of 167 (56.29%) changed or added relevant lines in 4 files are covered.
6 unchanged lines in 3 files lost coverage.
Overall coverage decreased (-1.05%) to 76.157%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
ovsdb/set.go	12	13	92.31%
modelgen/table.go	8	18	44.44%
mapper/info.go	25	38	65.79%
ovsdb/bindings.go	49	98	50.0%

Files with Coverage Reduction	New Missed Lines	%
ovsdb/bindings.go	1	72.55%
ovsdb/set.go	1	86.76%
modelgen/table.go	4	88.29%

Totals
Change from base Build 1033039223:	-1.05%
Covered Lines:	3242
Relevant Lines:	4257

💛 - Coveralls

dave-tucker · 2021-07-14T18:48:42Z

@amorenoz can you please take a look when you have time

This commit: 1. Adjusts the mapper to handle stronger enum types 2. Adjusts modelgen to produce these types Fixes: ovn-org#154 Signed-off-by: Dave Tucker <[email protected]>

Sets with min 1 and max 1 are a type Sets with min 0 and max 1 are a pointer to a type Sets with a max > 1 (non-default) are an array Otherwise a set is represented by a slice Signed-off-by: Dave Tucker <[email protected]>

This handles mapping from OvsToNative - min 0, max > 1 && !unlimted to an array - min 0, max 1 to a pointer - min 0, max unlimited to a slice And in NativeToOvs, we convert all cases back in to an OvsSet Signed-off-by: Dave Tucker <[email protected]>

amorenoz · 2021-07-16T10:36:22Z

mapper/info.go

@@ -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 {


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

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

type Priority int type ACL struct { Priority Priority }

amorenoz · 2021-07-16T10:54:02Z

mapper/info.go

-		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


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

amorenoz · 2021-07-16T10:54:12Z

mapper/info.go

 	}
-	fieldValue.Set(reflect.ValueOf(value))
+	fieldValue.Set(v)


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?

amorenoz · 2021-07-16T11:06:12Z

ovsdb/bindings.go

@@ -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) {


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?

amorenoz · 2021-07-16T12:45:45Z

mapper/info.go

+				v = v.Elem()
+			} else {
+				schema := i.table.Column(column)
+				native, err := ovsdb.OvsToNative(schema, value)


Info.SetField is expected to receive Native values. Why do we need this?
Besides, we are not making this optional are we? I mean, we are enforcing that the native type of a set with max = 1 is a pointer. OvsToNative should create a pointer and SetField should fail if the Field is not a pointer. Why do we need extra conversion here?

amorenoz · 2021-07-16T12:47:58Z

ovsdb/bindings.go

+			switch ovsSet := ovsElem.(type) {
+			case OvsSet:
+				if len(ovsSet.GoSet) > 1 {
+					return nil, fmt.Errorf("expected a slice of len < 1, but got a slice with %d elements", len(ovsSet.GoSet))


s/</<=/?

Besides, I think we should refactor slice length checking to a common place and verify len(ovsSet) <= columnSchema.Max.

dave-tucker · 2021-07-19T12:59:54Z

@amorenoz after thinking about it some more, I'm going to split this change up.
The bits dealing with #191 should be more merge-able as-is. The stronger enum typing needs more though since it broke the separation between mapper and bindings.

dave-tucker mentioned this pull request Jun 9, 2021

add enum support for modelgen #147

Merged

dave-tucker force-pushed the enum branch from e290247 to d1ba0b0 Compare June 10, 2021 14:29

dave-tucker changed the title ~~WIP: Adjust Mapper/Bindings for Enums~~ WIP: Adjust Mapper/Bindings for Stronger Typed Enums Jun 10, 2021

dave-tucker force-pushed the enum branch from d1ba0b0 to ad4f0bb Compare June 10, 2021 14:32

dave-tucker force-pushed the enum branch from ad4f0bb to 00358b3 Compare July 14, 2021 17:39

dave-tucker changed the title ~~WIP: Adjust Mapper/Bindings for Stronger Typed Enums~~ Add Stronger Types for Enums and Improve Set Encoding Jul 14, 2021

dave-tucker marked this pull request as ready for review July 14, 2021 17:47

dave-tucker force-pushed the enum branch from 00358b3 to 96029bc Compare July 14, 2021 18:14

dave-tucker added the feature label Jul 14, 2021

dave-tucker added this to the 0.7.0 milestone Jul 14, 2021

dave-tucker added 3 commits July 15, 2021 18:38

Add stronger typing for Enums

5dc9643

This commit: 1. Adjusts the mapper to handle stronger enum types 2. Adjusts modelgen to produce these types Fixes: ovn-org#154 Signed-off-by: Dave Tucker <[email protected]>

modelgen: Add constraints to sets

27341d5

Sets with min 1 and max 1 are a type Sets with min 0 and max 1 are a pointer to a type Sets with a max > 1 (non-default) are an array Otherwise a set is represented by a slice Signed-off-by: Dave Tucker <[email protected]>

dave-tucker force-pushed the enum branch from 96029bc to ccd9a6f Compare July 15, 2021 17:38

amorenoz reviewed Jul 16, 2021

View reviewed changes

dave-tucker closed this Jul 19, 2021

dave-tucker mentioned this pull request Jul 19, 2021

Use pointers for optional datatypes. Use arrays for fixed size sets #204

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Stronger Types for Enums and Improve Set Encoding #151

Add Stronger Types for Enums and Improve Set Encoding #151

dave-tucker commented Jun 9, 2021 •

edited

Loading

coveralls commented Jun 10, 2021 •

edited

Loading

dave-tucker commented Jul 14, 2021

amorenoz Jul 16, 2021

amorenoz Jul 16, 2021

amorenoz Jul 16, 2021

amorenoz Jul 16, 2021

amorenoz Jul 16, 2021

amorenoz Jul 16, 2021

amorenoz Jul 16, 2021

dave-tucker commented Jul 19, 2021

		@@ -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) {

Add Stronger Types for Enums and Improve Set Encoding #151

Add Stronger Types for Enums and Improve Set Encoding #151

Conversation

dave-tucker commented Jun 9, 2021 • edited Loading

coveralls commented Jun 10, 2021 • edited Loading

Pull Request Test Coverage Report for Build 1034832956

💛 - Coveralls

dave-tucker commented Jul 14, 2021

amorenoz Jul 16, 2021

Choose a reason for hiding this comment

amorenoz Jul 16, 2021

Choose a reason for hiding this comment

amorenoz Jul 16, 2021

Choose a reason for hiding this comment

amorenoz Jul 16, 2021

Choose a reason for hiding this comment

amorenoz Jul 16, 2021

Choose a reason for hiding this comment

amorenoz Jul 16, 2021

Choose a reason for hiding this comment

amorenoz Jul 16, 2021

Choose a reason for hiding this comment

dave-tucker commented Jul 19, 2021

dave-tucker commented Jun 9, 2021 •

edited

Loading

coveralls commented Jun 10, 2021 •

edited

Loading