Skip to content

Commit

Permalink
Support explicitly setting proto field tags.
Browse files Browse the repository at this point in the history 
Credits to @jasonewang for posting the first implementation at #339. I've
modified this a bit to read the extension values from the new
openconfig-codegen-extensions.yang model that's now in
openconfig/public, and also to address some of the corner cases that
were discussed in the PR.

Spec:
https://github.com/openconfig/ygot/blob/master/docs/yang-to-protobuf-transformations-spec.md#field-numbering

Corner cases not mentioned in the spec:
1. multiple `field-number` extensions.
  - Error out.
1. multiple `field-number-offset` extensions.
  - Add them up.
1. `field-number` or `field-number-offset` extensions on non-leaves.
  - Silently ignore.
1. `field-number-offset` on non-uses statements, including leafs.
  - Allow.
1. Conflicts.
  - Allow. The proto compilation will of course fail. Automated checking can be added here if we'd prefer.

I made some simple choices for the above items. Erroring out for 3 and 4 would
likely require changing `goyang` to specifically support checking these cases,
which I'm not sure is worth it here.
  • Loading branch information
@wenovus
wenovus committed Jun 21, 2021
1 parent 4795ccf commit 3aa2434
Show file tree
Hide file tree
Showing 10 changed files with 323 additions and 43 deletions.
2 changes: 1 addition & 1 deletion go.mod
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ require (
github.com/google/go-cmp v0.5.5
github.com/kylelemons/godebug v1.1.0
github.com/openconfig/gnmi v0.0.0-20200508230933-d19cebf5e7be
github.com/openconfig/goyang v0.2.5
github.com/openconfig/goyang v0.2.6
github.com/openconfig/gribi v0.1.1-0.20210423184541-ce37eb4ba92f
github.com/pmezard/go-difflib v1.0.0
google.golang.org/genproto v0.0.0-20201214200347-8c77b98c765d
Expand Down
4 changes: 2 additions & 2 deletions go.sum
View file
Original file line number Diff line number Diff line change
Expand Up @@ -44,8 +44,8 @@ github.com/openconfig/gnmi v0.0.0-20200508230933-d19cebf5e7be h1:VEK8utxoyZu/hkp
github.com/openconfig/gnmi v0.0.0-20200508230933-d19cebf5e7be/go.mod h1:M/EcuapNQgvzxo1DDXHK4tx3QpYM/uG4l591v33jG2A=
github.com/openconfig/goyang v0.0.0-20200115183954-d0a48929f0ea/go.mod h1:dhXaV0JgHJzdrHi2l+w0fZrwArtXL7jEFoiqLEdmkvU=
github.com/openconfig/goyang v0.2.2/go.mod h1:vX61x01Q46AzbZUzG617vWqh/cB+aisc+RrNkXRd3W8=
github.com/openconfig/goyang v0.2.5 h1:ZvV+5cF5thPFun1H6/Itt/Jbwb/bKmjS0o8imN+7ddw=
github.com/openconfig/goyang v0.2.5/go.mod h1:vX61x01Q46AzbZUzG617vWqh/cB+aisc+RrNkXRd3W8=
github.com/openconfig/goyang v0.2.6 h1:uJlr2bOUe/9mTtnIXgWG9VBqZd/BeCKur4+wAU2jJPc=
github.com/openconfig/goyang v0.2.6/go.mod h1:vX61x01Q46AzbZUzG617vWqh/cB+aisc+RrNkXRd3W8=
github.com/openconfig/gribi v0.1.1-0.20210423184541-ce37eb4ba92f h1:8vRtC+y0xh9BYPrEGf/jG/paYXiDUJ6P8iYt5rCVols=
github.com/openconfig/gribi v0.1.1-0.20210423184541-ce37eb4ba92f/go.mod h1:OoH46A2kV42cIXGyviYmAlGmn6cHjGduyC2+I9d/iVs=
github.com/openconfig/ygot v0.6.0/go.mod h1:o30svNf7O0xK+R35tlx95odkDmZWS9JyWWQSmIhqwAs=
Expand Down
7 changes: 5 additions & 2 deletions ygen/codegen_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1967,8 +1967,11 @@ func TestGenerateProto3(t *testing.T) {
wantOutputFiles map[string]string
wantErr bool
}{{
name: "simple protobuf test with compression",
inFiles: []string{filepath.Join(TestRoot, "testdata", "proto", "proto-test-a.yang")},
name: "simple protobuf test with compression",
inFiles: []string{
filepath.Join(TestRoot, "testdata", "proto", "proto-test-a.yang"),
filepath.Join(TestRoot, "testdata", "proto", "openconfig-codegen-extensions.yang"),
},
inConfig: GeneratorConfig{
TransformationOptions: TransformationOpts{
CompressBehaviour: genutil.PreferIntendedConfig,
Expand Down
67 changes: 39 additions & 28 deletions ygen/protogen.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,13 @@ const (
protoMatchingListNameKeySuffix = "key"
)

// Names from https://github.com/openconfig/public/blob/master/release/models/extensions/openconfig-codegen-extensions.yang
const (
codegenExtModuleName = "openconfig-codegen-extensions"
fieldNumExtName = "field-number"
offsetExtName = "field-number-offset"
)

// protoMsgField describes a field of a protobuf message.
// Note, throughout this package private structs that have public fields are used
// in text/template which cannot refer to unexported fields.
Expand Down Expand Up @@ -1048,40 +1055,44 @@ func safeProtoIdentifierName(name string) string {
}

// protoTagForEntry returns a protobuf tag value for the entry e.
// If the entry has user-specified field numberings, then use that instead:
// https://github.com/openconfig/ygot/blob/master/docs/yang-to-protobuf-transformations-spec.md#field-numbering
func protoTagForEntry(e *yang.Entry) (uint32, error) {
var offset uint32
for _, ext := range e.Exts {
if ext.Keyword == "occodegenext:field-number-offset" {
off, err := strconv.ParseUint(ext.Argument, 10, 32)
if err != nil {
return 0, fmt.Errorf("could not parse %s %s: %s", ext.Keyword, ext.Argument, err)
}
offset = uint32(off)
if offset == 0 {
return 0, fmt.Errorf("%s cannot be 0", ext.Keyword)
}
break
var fieldNum uint32
exts, err := yang.MatchingEntryExtensions(e, codegenExtModuleName, fieldNumExtName)
if err != nil {
return 0, err
}
switch len(exts) {
case 0:
return fieldTag(e.Path())
case 1:
num, err := strconv.ParseUint(exts[0].Argument, 10, 32)
if err != nil || num == 0 {
return 0, fmt.Errorf("could not parse %s:%s %q as a non-zero integer: %s", codegenExtModuleName, fieldNumExtName, exts[0].Argument, err)
}
fieldNum = uint32(num)
default:
return 0, fmt.Errorf("more than one %s:%s defined", codegenExtModuleName, fieldNumExtName)
}
for _, ext := range e.Exts {
if ext.Keyword == "occodegenext:field-number" {
fn, err := strconv.ParseUint(ext.Argument, 10, 32)
if err != nil {
return 0, fmt.Errorf("could not parse %s %s: %s", ext.Keyword, ext.Argument, err)
}
if fn == 0 {
return 0, fmt.Errorf("%s cannot be 0", ext.Keyword)
}
annotatedFieldNumber := uint32(fn) + offset
if annotatedFieldNumber > 1000 || annotatedFieldNumber < 1 {
return 0, fmt.Errorf("%s field number %d not in annotation reserved range of 1-1000",
e.Name, annotatedFieldNumber)
}
return annotatedFieldNumber, nil

exts, err = yang.MatchingEntryExtensions(e, codegenExtModuleName, offsetExtName)
if err != nil {
return 0, err
}
for _, ext := range exts {
num, err := strconv.ParseUint(ext.Argument, 10, 32)
if err != nil {
return 0, fmt.Errorf("could not parse %s:%s %q as an uint: %s", codegenExtModuleName, offsetExtName, ext.Argument, err)
}
// Allow for multiple offsets to accumulate.
fieldNum += uint32(num)
}

return fieldTag(e.Path())
if fieldNum > 1000 || fieldNum < 1 {
return 0, fmt.Errorf("%s: %d not in reserved range of 1-1000 for %s:%s", e.Path(), fieldNum, codegenExtModuleName, offsetExtName)
}
return fieldNum, nil
}

// fieldTag takes an input string and calculates a FNV hash for the value. If the
Expand Down
194 changes: 187 additions & 7 deletions ygen/protogen_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import (

"github.com/google/go-cmp/cmp"
"github.com/kylelemons/godebug/pretty"
"github.com/openconfig/gnmi/errdiff"
"github.com/openconfig/goyang/pkg/yang"
"github.com/openconfig/ygot/testutil"
)
Expand Down Expand Up @@ -63,7 +64,7 @@ func TestGenProto3Msg(t *testing.T) {
inParentPackage string
inChildMsgs []*generatedProto3Message
wantMsgs map[string]*protoMsg
wantErr bool
wantErrSubstring string
}{{
name: "simple message with only scalar fields",
inMsg: &Directory{
Expand Down Expand Up @@ -539,7 +540,7 @@ func TestGenProto3Msg(t *testing.T) {
},
},
},
wantErr: true,
wantErrSubstring: "could not resolve",
}, {
name: "message with an unimplemented mapping",
inMsg: &Directory{
Expand All @@ -565,7 +566,7 @@ func TestGenProto3Msg(t *testing.T) {
},
Path: []string{"", "messasge-with-invalid-contents", "unimplemented"},
},
wantErr: true,
wantErrSubstring: "unimplemented type",
}, {
name: "message with any anydata field",
inMsg: &Directory{
Expand Down Expand Up @@ -673,6 +674,26 @@ func TestGenProto3Msg(t *testing.T) {
Argument: "100",
},
},
Node: &yang.Leaf{
Name: "field-one",
Parent: &yang.Container{
Name: "message-name",
Parent: &yang.Module{
Name: "module",
Prefix: &yang.Value{
Name: "foo",
},
Import: []*yang.Import{{
Prefix: &yang.Value{
Name: "occodegenext",
},
Module: &yang.Module{
Name: codegenExtModuleName,
},
}},
},
},
},
},
"field-two": {
Name: "field-two",
Expand All @@ -683,6 +704,26 @@ func TestGenProto3Msg(t *testing.T) {
Argument: "1",
},
},
Node: &yang.Leaf{
Name: "field-two",
Parent: &yang.Container{
Name: "message-name",
Parent: &yang.Module{
Name: "module",
Prefix: &yang.Value{
Name: "foo",
},
Import: []*yang.Import{{
Prefix: &yang.Value{
Name: "occodegenext",
},
Module: &yang.Module{
Name: codegenExtModuleName,
},
}},
},
},
},
},
"field-three": {
Name: "field-three",
Expand All @@ -697,6 +738,64 @@ func TestGenProto3Msg(t *testing.T) {
Argument: "100",
},
},
Node: &yang.Leaf{
Name: "field-three",
Parent: &yang.Container{
Name: "message-name",
Parent: &yang.Module{
Name: "module",
Prefix: &yang.Value{
Name: "foo",
},
Import: []*yang.Import{{
Prefix: &yang.Value{
Name: "occodegenext",
},
Module: &yang.Module{
Name: codegenExtModuleName,
},
}},
},
},
},
},
"field-four": {
Name: "field-four",
Type: &yang.YangType{Kind: yang.Yint8},
Exts: []*yang.Statement{
{
Keyword: "occodegenext:field-number",
Argument: "1",
},
{
Keyword: "occodegenext:field-number-offset",
Argument: "100",
},
{
Keyword: "occodegenext:field-number-offset",
Argument: "200",
},
},
Node: &yang.Leaf{
Name: "field-four",
Parent: &yang.Container{
Name: "message-name",
Parent: &yang.Module{
Name: "module",
Prefix: &yang.Value{
Name: "foo",
},
Import: []*yang.Import{{
Prefix: &yang.Value{
Name: "occodegenext",
},
Module: &yang.Module{
Name: codegenExtModuleName,
},
}},
},
},
},
},
},
Path: []string{"", "root", "message-name"},
Expand All @@ -719,6 +818,10 @@ func TestGenProto3Msg(t *testing.T) {
Tag: 101,
Name: "field_three",
Type: "ywrapper.IntValue",
}, {
Tag: 301,
Name: "field_four",
Type: "ywrapper.IntValue",
}},
},
},
Expand All @@ -745,11 +848,79 @@ func TestGenProto3Msg(t *testing.T) {
Argument: "1000",
},
},
Node: &yang.Leaf{
Name: "field-one",
Parent: &yang.Container{
Name: "message-name",
Parent: &yang.Module{
Name: "module",
Prefix: &yang.Value{
Name: "foo",
},
Import: []*yang.Import{{
Prefix: &yang.Value{
Name: "occodegenext",
},
Module: &yang.Module{
Name: codegenExtModuleName,
},
}},
},
},
},
},
},
Path: []string{"", "root", "message-name"},
},
wantErr: true,
wantErrSubstring: "not in reserved range",
}, {
name: "message with duplicate field numbers",
inMsg: &Directory{
Name: "MessageName",
Entry: &yang.Entry{
Name: "message-name",
Dir: map[string]*yang.Entry{},
Kind: yang.DirectoryEntry,
},
Fields: map[string]*yang.Entry{
"field-one": {
Name: "field-one",
Type: &yang.YangType{Kind: yang.Ystring},
Exts: []*yang.Statement{
{
Keyword: "occodegenext:field-number",
Argument: "1",
},
{
Keyword: "occodegenext:field-number",
Argument: "1000",
},
},
Node: &yang.Leaf{
Name: "field-one",
Parent: &yang.Container{
Name: "message-name",
Parent: &yang.Module{
Name: "module",
Prefix: &yang.Value{
Name: "foo",
},
Import: []*yang.Import{{
Prefix: &yang.Value{
Name: "occodegenext",
},
Module: &yang.Module{
Name: codegenExtModuleName,
},
}},
},
},
},
},
},
Path: []string{"", "root", "message-name"},
},
wantErrSubstring: "more than one",
}}

for _, tt := range tests {
Expand All @@ -771,11 +942,20 @@ func TestGenProto3Msg(t *testing.T) {
annotateSchemaPaths: tt.inAnnotateSchemaPaths,
}, tt.inParentPackage, tt.inChildMsgs, true, true)

if (errs != nil) != tt.wantErr {
t.Errorf("s: genProtoMsg(%#v, %#v, *genState, %v, %v, %s, %s): did not get expected error status, got: %v, wanted err: %v", tt.name, tt.inMsg, tt.inMsgs, tt.inCompressPaths, tt.inBasePackage, tt.inEnumPackage, errs, tt.wantErr)
var err error
switch len(errs) {
case 1:
err = errs[0]
fallthrough
case 0:
if diff := errdiff.Substring(err, tt.wantErrSubstring); diff != "" {
t.Fatalf("s: genProtoMsg(%#v, %#v, *genState, %v, %v, %s, %s): did not get expected error: %s", tt.name, tt.inMsg, tt.inMsgs, tt.inCompressPaths, tt.inBasePackage, tt.inEnumPackage, diff)
}
default:
t.Fatalf("got more than 1 error: %v", errs)
}

if tt.wantErr {
if tt.wantErrSubstring != "" {
return
}

Expand Down
Loading

0 comments on commit 3aa2434

Please sign in to comment.