Error out for Go code generation if list key contains binary (#459)

Error out for Go code generation if list key contains binary

.github/workflows/go.yml

@@ -13,7 +13,7 @@ jobs:
     strategy:
       fail-fast: false
       matrix:
-        go: ['1.12', '1.13', '1.14']
+        go: ['1.13', '1.14', '1.15']
 
     steps:
       - name: Install protobuf
@@ -73,7 +73,7 @@ jobs:
       - name: Install Go
         uses: actions/setup-go@v2
         with:
-          go-version: '1.14'
+          go-version: '1.15'
         id: go
 
       - name: Install required static analysis tools

README.md

@@ -1,6 +1,6 @@
 ![Go](https://github.com/openconfig/ygot/workflows/Go/badge.svg?branch=master)
 [![Coverage Status](https://coveralls.io/repos/github/openconfig/ygot/badge.svg?branch=master)](https://coveralls.io/github/openconfig/ygot?branch=master)
-[![Go releases supported](https://img.shields.io/badge/Go-1.12%2B-blue)](https://golang.org/project/#release)
+[![Go releases supported](https://img.shields.io/badge/Go-1.13%2B-blue)](https://golang.org/project/#release)
 
 ![#ygot](docs/img/ygot.png)
 

docs/design.md

@@ -287,6 +287,12 @@ Such that the `Foo` field is a map, keyed on the type of the key leaf (`fookey`)
 
 Each YANG list that exists within a container has a helper-method generated for it. For a list named `foo`, the parent container (`C`) has a `NewFoo(fookey string)` method generated, taking a key value as an argument, and returning a new member of the map within the `foo` list.
 
+##### Note on using binary as a list key type
+Because `Binary`'s underlying `[]byte` type is not hashable, YANG models
+containing lists with `binary` as a key value, or a `union` type containing a
+`binary` type is not supported. An error is returned by the Go code generation
+process for such cases, this is a known limitation.
+
 ### YANG Union Leaves
 
 In order to preserve strict type validation at compile time, `union` leaves within the YANG schema are mapped to an Go `interface` which is subsequently implemented for each type that is defined within the YANG union.
@@ -298,37 +304,78 @@ container foo {
 	container bar {
 		leaf union-leaf {
 			type union {
-				type string;
 				type int8;
+				type enumeration {
+					enum ONE;
+					enum TWO;
+				}
 			}
 		}
 	}
 }
 ```
 
-the `bar` container is mapped to:
+The `bar` container can be translated to Go code according to one of the
+following strategies:
+
+#### Simplified Union Leaves (Recommended)
+In this representation, generated defined types are used to represent all concrete union types.
+```go
+type Binary []byte
+type YANGEmpty bool
+type Int8 int8
+type Int16 int16
+// ... etc.
+type String string
+type Bool bool
+```
 
 ```go
 type Bar struct {
 	UnionLeaf		Foo_Bar_UnionLeaf_Union		`path:"union-leaf"`
 }
 
 type Foo_Bar_UnionLeaf_Union interface {
-	Is_Foo_Bar_UnionLeaf_Union()
+	// Union type can be one of [Int8, E_Foo_Bar_UnionLeaf]
+	Documentation_for_Foo_Bar_UnionLeaf_Union()
 }
 
-type Foo_Bar_UnionLeaf_Union_String struct {
-	String string
+func (Int8) Documentation_for_Foo_Bar_UnionLeaf_Union() {}
+
+func (E_Foo_Bar_UnionLeaf) Documentation_for_Foo_Bar_UnionLeaf_Union() {}
+```
+
+The `UnionLeaf` field can be set to any defined type (including enumeration
+typedefs) that implements the `Foo_Bar_UnionLeaf_Union` interface. These
+typedefs are re-used for different union types; so, it's possible to assign an
+`Int8` value to any union which has `int8` in its definition.
+
+#### Wrapper Union Leaves
+
+```go
+type Bar struct {
+	UnionLeaf		Foo_Bar_UnionLeaf_Union		`path:"union-leaf"`
 }
 
-func (Foo_Bar_UnionLeaf_Union_String) Is_Foo_Bar_UnionLeaf_Union() {}
+type Foo_Bar_UnionLeaf_Union interface {
+	Is_Foo_Bar_UnionLeaf_Union()
+}
 
 type Foo_Bar_UnionLeaf_Union_Int8 struct {
 	Int8 int8
 }
 
 func (Foo_Bar_UnionLeaf_Union_Int8) Is_Foo_Bar_UnionLeaf_Union() {}
-```
 
-The `UnionLeaf` field can be set to any of the structs that implement the `Foo_Bar_UnionLeaf_Union` interface. Since these structs are single-field entities, a struct initialiser that does not specify the field name can be used (e.g., `Foo_Bar_UnionLeaf_Union_String{"baz"}`), similarly to the generate Go code for a Protobuf `oneof`.
+type Foo_Bar_UnionLeaf_Union_E_Foo_Bar_UnionLeaf struct {
+	E_Foo_Bar_UnionLeaf E_Foo_Bar_UnionLeaf
+}
+
+func (Foo_Bar_UnionLeaf_Union_E_Foo_Bar_UnionLeaf) Is_Foo_Bar_UnionLeaf_Union() {}
+```
 
+The `UnionLeaf` field can be set to any of the structs that implement the
+`Foo_Bar_UnionLeaf_Union` interface. Since these structs are single-field
+entities, a struct initialiser that does not specify the field name can be used
+(e.g., `Foo_Bar_UnionLeaf_Union_Int8{42}`), similarly to the generate Go code
+for a Protobuf `oneof`.

testdata/modules/openconfig-binary-list.yang

@@ -0,0 +1,40 @@
+module openconfig-binary-list {
+  namespace "urn:ocblist";
+  prefix "oc";
+
+  description
+    "A simple test module that is used to verify code generation for a
+    schema that contains a list with a binary key";
+
+  grouping single-key-config {
+    leaf key { type binary; }
+  }
+
+  grouping lists-top {
+    container model {
+      container a {
+        list single-key {
+          key "key";
+
+          leaf key {
+            type leafref {
+              path "../config/key";
+            }
+          }
+
+          container config {
+            uses single-key-config;
+          }
+
+          container state {
+            config false;
+            uses single-key-config;
+          }
+        }
+      }
+    }
+  }
+
+  uses lists-top;
+
+}

testdata/modules/openconfig-binary-multi-list.yang

@@ -0,0 +1,47 @@
+module openconfig-binary-multi-list {
+  namespace "urn:ocbmlist";
+  prefix "oc";
+
+  description
+    "A simple test module that is used to verify code generation for a
+    schema that contains a multi-keyed list with a binary key";
+
+  grouping multi-key-config {
+    leaf key1 { type uint32; }
+    leaf key2 { type binary; }
+  }
+
+  grouping lists-top {
+    container model {
+      container b {
+        list multi-key {
+          key "key1 key2";
+
+          leaf key1 {
+            type leafref {
+              path "../config/key1";
+            }
+          }
+
+          leaf key2 {
+            type leafref {
+              path "../config/key2";
+            }
+          }
+
+          container config {
+            uses multi-key-config;
+          }
+
+          container state {
+            config false;
+            uses multi-key-config;
+          }
+        }
+      }
+    }
+  }
+
+  uses lists-top;
+
+}

testdata/modules/openconfig-union-binary-list.yang

@@ -0,0 +1,45 @@
+module openconfig-union-binary-list {
+  namespace "urn:ocublist";
+  prefix "oc";
+
+  description
+    "A simple test module that is used to verify code generation for a
+    schema that contains a list with a union key containing a binary";
+
+  grouping single-key-config {
+    leaf key {
+      type union {
+        type string;
+        type binary;
+      }
+    }
+  }
+
+  grouping lists-top {
+    container model {
+      container a {
+        list single-key {
+          key "key";
+
+          leaf key {
+            type leafref {
+              path "../config/key";
+            }
+          }
+
+          container config {
+            uses single-key-config;
+          }
+
+          container state {
+            config false;
+            uses single-key-config;
+          }
+        }
+      }
+    }
+  }
+
+  uses lists-top;
+
+}

ygen/codegen.go

@@ -341,6 +341,27 @@ func IsFakeRoot(e *yang.Entry) bool {
 	return e != nil && e.Node != nil && e.Node.NName() == rootElementNodeName
 }
 
+// checkForBinaryKeys returns a non-empty list of errors if the input directory
+// has one or more binary types (including union types containing binary types)
+// as a list key.
+func checkForBinaryKeys(dir *Directory) []error {
+	var errs []error
+	if dir.ListAttr != nil {
+		for _, t := range dir.ListAttr.Keys {
+			if t.NativeType == ygot.BinaryTypeName {
+				errs = append(errs, fmt.Errorf("list %s has a binary key -- this is unsupported", strings.Join(dir.Path, "/")))
+				continue
+			}
+			for typeName := range t.UnionTypes {
+				if typeName == ygot.BinaryTypeName {
+					errs = append(errs, fmt.Errorf("list %s has a union key containing a binary -- this is unsupported", strings.Join(dir.Path, "/")))
+				}
+			}
+		}
+	}
+	return errs
+}
+
 // GenerateGoCode takes a slice of strings containing the path to a set of YANG
 // files which contain YANG modules, and a second slice of strings which
 // specifies the set of paths that are to be searched for associated models (e.g.,
@@ -401,6 +422,10 @@ func (cg *YANGCodeGenerator) GenerateGoCode(yangFiles, includePaths []string) (*
 	enumTypeMap := map[string][]string{}
 	var structSnippets []GoStructCodeSnippet
 	for _, directoryName := range orderedDirNames {
+		if errs := checkForBinaryKeys(dirNameMap[directoryName]); len(errs) != 0 {
+			codegenErr = util.AppendErrs(codegenErr, errs)
+			continue
+		}
 		structOut, errs := writeGoStruct(dirNameMap[directoryName], directoryMap, gogen,
 			cg.Config.TransformationOptions.CompressBehaviour.CompressEnabled(), cg.Config.GenerateJSONSchema, cg.Config.ParseOptions.SkipEnumDeduplication, cg.Config.TransformationOptions.ShortenEnumLeafNames, cg.Config.TransformationOptions.UseDefiningModuleForTypedefEnumNames, cg.Config.GoOptions)
 		if errs != nil {