diff --git a/.github/workflows/go.yml b/.github/workflows/go.yml index b529ff819..5e8c2ea34 100644 --- a/.github/workflows/go.yml +++ b/.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 diff --git a/README.md b/README.md index 64ba96537..262794e0e 100644 --- a/README.md +++ b/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) diff --git a/docs/design.md b/docs/design.md index e810f1a8a..4bcc9b4bf 100644 --- a/docs/design.md +++ b/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,15 +304,31 @@ 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 { @@ -314,21 +336,46 @@ type Bar struct { } 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`. diff --git a/testdata/modules/openconfig-binary-list.yang b/testdata/modules/openconfig-binary-list.yang new file mode 100644 index 000000000..87dd7ddf6 --- /dev/null +++ b/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; + +} diff --git a/testdata/modules/openconfig-binary-multi-list.yang b/testdata/modules/openconfig-binary-multi-list.yang new file mode 100644 index 000000000..865678874 --- /dev/null +++ b/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; + +} diff --git a/testdata/modules/openconfig-union-binary-list.yang b/testdata/modules/openconfig-union-binary-list.yang new file mode 100644 index 000000000..398c6123e --- /dev/null +++ b/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; + +} diff --git a/ygen/codegen.go b/ygen/codegen.go index f97687b4f..a5f2ceea4 100644 --- a/ygen/codegen.go +++ b/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 { diff --git a/ygen/codegen_test.go b/ygen/codegen_test.go index 5791dac92..496f9450f 100644 --- a/ygen/codegen_test.go +++ b/ygen/codegen_test.go @@ -413,7 +413,7 @@ type yangTestCase struct { inExcludeModules []string // inExcludeModules is the set of modules that should be excluded from code generation. inConfig GeneratorConfig // inConfig specifies the configuration that should be used for the generator test case. wantStructsCodeFile string // wantsStructsCodeFile is the path of the generated Go code that the output of the test should be compared to. - wantErr bool // wantErr specifies whether the test should expect an error. + wantErrSubstring string // wantErrSubstring specifies whether the test should expect an error. wantSchemaFile string // wantSchemaFile is the path to the schema JSON that the output of the test should be compared to. } @@ -956,11 +956,56 @@ func TestSimpleStructs(t *testing.T) { }, }, wantStructsCodeFile: filepath.Join(TestRoot, "testdata", "structs", "enum-duplication-dup.formatted-txt"), + }, { + name: "OpenConfig schema test - list with binary key", + inFiles: []string{filepath.Join(datapath, "openconfig-binary-list.yang")}, + inConfig: GeneratorConfig{ + TransformationOptions: TransformationOpts{ + CompressBehaviour: genutil.PreferIntendedConfig, + ShortenEnumLeafNames: true, + UseDefiningModuleForTypedefEnumNames: true, + }, + GoOptions: GoOpts{ + GenerateRenameMethod: true, + GenerateSimpleUnions: true, + }, + }, + wantErrSubstring: "has a binary key", + }, { + name: "OpenConfig schema test - multi-keyed list with binary key", + inFiles: []string{filepath.Join(datapath, "openconfig-binary-multi-list.yang")}, + inConfig: GeneratorConfig{ + TransformationOptions: TransformationOpts{ + CompressBehaviour: genutil.PreferIntendedConfig, + ShortenEnumLeafNames: true, + UseDefiningModuleForTypedefEnumNames: true, + }, + GoOptions: GoOpts{ + GenerateRenameMethod: true, + GenerateSimpleUnions: true, + }, + }, + wantErrSubstring: "has a binary key", + }, { + name: "OpenConfig schema test - list with union key containing binary", + inFiles: []string{filepath.Join(datapath, "openconfig-union-binary-list.yang")}, + inConfig: GeneratorConfig{ + TransformationOptions: TransformationOpts{ + CompressBehaviour: genutil.PreferIntendedConfig, + ShortenEnumLeafNames: true, + UseDefiningModuleForTypedefEnumNames: true, + }, + GoOptions: GoOpts{ + GenerateRenameMethod: true, + GenerateSimpleUnions: true, + }, + }, + wantErrSubstring: "has a union key containing a binary", }} for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - genCode := func() (*GeneratedGoCode, string, map[string]interface{}) { + genCode := func() (*GeneratedGoCode, string, map[string]interface{}, error) { // Set defaults within the supplied configuration for these tests. if tt.inConfig.Caller == "" { // Set the name of the caller explicitly to avoid issues when @@ -972,9 +1017,16 @@ func TestSimpleStructs(t *testing.T) { cg := NewYANGCodeGenerator(&tt.inConfig) - gotGeneratedCode, err := cg.GenerateGoCode(tt.inFiles, tt.inIncludePaths) - if err != nil && !tt.wantErr { - t.Fatalf("%s: cg.GenerateCode(%v, %v): Config: %+v, got unexpected error: %v, want: nil", tt.name, tt.inFiles, tt.inIncludePaths, tt.inConfig, err) + gotGeneratedCode, errs := cg.GenerateGoCode(tt.inFiles, tt.inIncludePaths) + var err error + if len(errs) > 0 { + err = fmt.Errorf("%w", errs) + } + if diff := errdiff.Substring(err, tt.wantErrSubstring); diff != "" { + t.Fatalf("%s: cg.GenerateCode(%v, %v): Config: %+v, Did not get expected error: %s", tt.name, tt.inFiles, tt.inIncludePaths, tt.inConfig, diff) + } + if err != nil { + return nil, "", nil, err } // Write all the received structs into a single file such that @@ -1003,10 +1055,13 @@ func TestSimpleStructs(t *testing.T) { t.Fatalf("%s: json.Unmarshal(..., %v), could not unmarshal received JSON: %v", tt.name, gotGeneratedCode.RawJSONSchema, err) } } - return gotGeneratedCode, gotCode.String(), gotJSON + return gotGeneratedCode, gotCode.String(), gotJSON, nil } - gotGeneratedCode, gotCode, gotJSON := genCode() + gotGeneratedCode, gotCode, gotJSON, err := genCode() + if err != nil { + return + } if tt.wantSchemaFile != "" { wantSchema, rferr := ioutil.ReadFile(tt.wantSchemaFile) @@ -1042,7 +1097,7 @@ func TestSimpleStructs(t *testing.T) { } for i := 0; i < deflakeRuns; i++ { - _, gotAttempt, _ := genCode() + _, gotAttempt, _, _ := genCode() if gotAttempt != gotCode { diff, _ := testutil.GenerateUnifiedDiff(gotAttempt, gotCode) t.Fatalf("flaky code generation, diff:\n%s", diff) diff --git a/ytypes/schema_tests/validate_test.go b/ytypes/schema_tests/validate_test.go index 5b6f26d2a..2106b4236 100644 --- a/ytypes/schema_tests/validate_test.go +++ b/ytypes/schema_tests/validate_test.go @@ -145,12 +145,11 @@ func TestValidateInterface(t *testing.T) { VlanId: oc.UnionUint16(4095), } // Validate the vlan. - if err := vlan0.Validate(); err == nil { - t.Errorf("bad vlan-id value: got nil, want error") - } else { - if diff := errdiff.Substring(err, "/device/interfaces/interface/subinterfaces/subinterface/vlan/config/vlan-id: unsigned integer value 4095 is outside specified ranges"); diff != "" { - t.Errorf("did not get expected vlan-id error, %s", diff) - } + err = vlan0.Validate() + if diff := errdiff.Substring(err, "/device/interfaces/interface/subinterfaces/subinterface/vlan/config/vlan-id: unsigned integer value 4095 is outside specified ranges"); diff != "" { + t.Errorf("did not get expected vlan-id error, %s", diff) + } + if err != nil { testErrLog(t, "bad vlan-id value", err) } @@ -168,7 +167,7 @@ func TestValidateInterfaceWrapperUnion(t *testing.T) { dev := &woc.Device{} eth0, err := dev.NewInterface("eth0") if err != nil { - t.Errorf("eth0.NewInterface(): got %v, want nil", err) + t.Errorf("dev.NewInterface(): got %v, want nil", err) } eth0.Description = ygot.String("eth0 description") @@ -185,12 +184,11 @@ func TestValidateInterfaceWrapperUnion(t *testing.T) { // Key in map != key field value in element. Key should be "eth0" here. dev.Interface["bad_key"] = eth0 - if err := dev.Validate(); err == nil { - t.Errorf("bad key: got nil, want error") - } else { - if diff := errdiff.Substring(err, "/device/interfaces/interface: key field Name: element key eth0 != map key bad_key"); diff != "" { - t.Errorf("did not get expected vlan-id error, %s", diff) - } + err = dev.Validate() + if diff := errdiff.Substring(err, "/device/interfaces/interface: key field Name: element key eth0 != map key bad_key"); diff != "" { + t.Errorf("did not get expected vlan-id error, %s", diff) + } + if err != nil { testErrLog(t, "bad key", err) }