Add support for compressed schemas/fix basePath trimming. (#540)

* Handle compressed schemas, fix base path trimming, test coverage.

  * (M) protomap/proto.go
  * (M) protomap/proto_test.go
    - handle the case where there is a schemapath of the form
      {config,state}/field - i.e., a compressed schema.
    - fix trimming of base path and add coverage for it within
      tets.
  * (A) testdata/
    - Update example protobufs for testing.

Author: robshakir

protomap/proto.go

@@ -27,6 +27,7 @@ import (
 	"google.golang.org/protobuf/types/descriptorpb"
 
 	"github.com/openconfig/gnmi/value"
+	"github.com/openconfig/ygot/util"
 	"github.com/openconfig/ygot/ygot"
 
 	gpb "github.com/openconfig/gnmi/proto/gnmi"
@@ -361,20 +362,106 @@ func resolvedPath(basePath, annotatedPath *gpb.Path) *gpb.Path {
 	return np
 }
 
-// ProtoFromPaths takes an input ygot-generated protobuf and unmarshals the values that are specified in the map
-// vals into it, using prefix as the prefix to any paths within the vals map. The message, p, is modified in place.
-// The map, vals, must be keyed by the gNMI path to the field which is annotated in the ygot generated protobuf,
-// the complete path is taken to be prefix + the key found in the map.
-func ProtoFromPaths(p proto.Message, vals map[*gpb.Path]interface{}, prefix *gpb.Path) error {
+// UnmapOpt marks that a particular option can be supplied as an argument
+// to the ProtoFromPaths function.
+type UnmapOpt interface {
+	isUnmapOpt()
+}
+
+// IgnoreExtraPaths indicates that unmapping should ignore any additional
+// paths that are found in the gNMI Notifications that do not have corresponding
+// fields in the protobuf.
+//
+// This option is typically used in conjunction with path compression where there
+// are some leaves that do not have corresponding fields.
+func IgnoreExtraPaths() *ignoreExtraPaths { return &ignoreExtraPaths{} }
+
+type ignoreExtraPaths struct{}
+
+// isUnmapOpt marks ignoreExtraPaths as an unmap option.
+func (*ignoreExtraPaths) isUnmapOpt() {}
+
+// ValuePathPrefix indicates that the values in the supplied map have a prefix which
+// is equal to the supplied path. The prefix plus each path in the vals map must be
+// equal to the absolute path for the supplied values.
+func ValuePathPrefix(path *gpb.Path) *valuePathPrefix {
+	return &valuePathPrefix{p: path}
+}
+
+type valuePathPrefix struct{ p *gpb.Path }
+
+// isUnmapOpt marks valuePathPrefix as an unmap option.
+func (*valuePathPrefix) isUnmapOpt() {}
+
+// ProtobufMessagePrefix specifies the path that the protobuf message supplied to ProtoFromPaths
+// makes up. This is used in cases where the message itself is not the root - and hence unmarshalling
+// should look for paths relative to the specified path in the vals map.
+func ProtobufMessagePrefix(path *gpb.Path) *protoMsgPrefix {
+	return &protoMsgPrefix{p: path}
+}
+
+type protoMsgPrefix struct{ p *gpb.Path }
+
+// isUnmapOpt marks protoMsgPrefix as an unmap option.
+func (*protoMsgPrefix) isUnmapOpt() {}
+
+// ProtoFromPaths takes an input ygot-generated protobuf and unmarshals the values provided in vals into the map.
+// The vals map must be keyed by the gNMI path to the leaf, with the interface{} value being the value that the
+// leaf at the field should be set to.
+//
+// The protobuf p is modified in place to add the values.
+//
+// The set of UnmapOpts that are provided (opt) are used to control the behaviour of unmarshalling the specified data.
+//
+// ProtoFromPaths returns an error if the data cannot be unmarshalled.
+func ProtoFromPaths(p proto.Message, vals map[*gpb.Path]interface{}, opt ...UnmapOpt) error {
 	if p == nil {
 		return errors.New("nil protobuf supplied")
 	}
 
+	valPrefix, err := hasValuePathPrefix(opt)
+	if err != nil {
+		return fmt.Errorf("invalid value prefix supplied, %v", err)
+	}
+
+	protoPrefix, err := hasProtoMsgPrefix(opt)
+	if err != nil {
+		return fmt.Errorf("invalid protobuf message prefix supplied in options, %v", err)
+	}
+
+	schemaPath := func(p *gpb.Path) *gpb.Path {
+		np := proto.Clone(p).(*gpb.Path)
+		for _, e := range np.Elem {
+			e.Key = nil
+		}
+		return np
+	}
+
+	// directCh is a map between the absolute schema path for a particular value, and
+	// the value specified.
 	directCh := map[*gpb.Path]interface{}{}
 	for p, v := range vals {
-		// TODO(robjs): needs fixing for compressed schemas.
-		if len(p.GetElem()) == len(prefix.GetElem())+1 {
-			directCh[p] = v
+		absPath := &gpb.Path{
+			Elem: append(append([]*gpb.PathElem{}, schemaPath(valPrefix).Elem...), p.Elem...),
+		}
+
+		if !util.PathMatchesPathElemPrefix(absPath, protoPrefix) {
+			return fmt.Errorf("invalid path provided, absolute paths must be used, %s does not have prefix %s", absPath, protoPrefix)
+		}
+
+		// make the path absolute, and a schema path.
+		pp := util.TrimGNMIPathElemPrefix(absPath, protoPrefix)
+
+		if len(pp.GetElem()) == 1 {
+			directCh[pp] = v
+		}
+		// TODO(robjs): it'd be good to have something here that tells us whether we are in
+		// a compressed schema. Potentially we should add something to the generated protobuf
+		// as a fileoption that would give us this indication.
+		if len(pp.Elem) == 2 {
+			if pp.Elem[len(pp.Elem)-2].Name == "config" || pp.Elem[len(pp.Elem)-2].Name == "state" {
+				directCh[pp] = v
+			}
 		}
 	}
 
@@ -389,8 +476,13 @@ func ProtoFromPaths(p proto.Message, vals map[*gpb.Path]interface{}, prefix *gpb
 		}
 
 		for _, ap := range annotatedPath {
+			if !util.PathMatchesPathElemPrefix(ap, protoPrefix) {
+				rangeErr = fmt.Errorf("annotation %s does not match the supplied prefix %s", ap, protoPrefix)
+				return false
+			}
+			trimmedAP := util.TrimGNMIPathElemPrefix(ap, protoPrefix)
 			for chp, chv := range directCh {
-				if proto.Equal(ap, chp) {
+				if proto.Equal(trimmedAP, chp) {
 					switch fd.Kind() {
 					case protoreflect.MessageKind:
 						v, isWrap, err := makeWrapper(m, fd, chv)
@@ -428,15 +520,56 @@ func ProtoFromPaths(p proto.Message, vals map[*gpb.Path]interface{}, prefix *gpb
 		return rangeErr
 	}
 
-	for chp := range directCh {
-		if !mapped[chp] {
-			return fmt.Errorf("did not map path %s to a proto field", chp)
+	if !hasIgnoreExtraPaths(opt) {
+		for chp := range directCh {
+			if !mapped[chp] {
+				return fmt.Errorf("did not map path %s to a proto field", chp)
+			}
 		}
 	}
 
 	return nil
 }
 
+// hasIgnoreExtraPaths checks whether the supplied opts slice contains the
+// ignoreExtraPaths option.
+func hasIgnoreExtraPaths(opts []UnmapOpt) bool {
+	for _, o := range opts {
+		if _, ok := o.(*ignoreExtraPaths); ok {
+			return true
+		}
+	}
+	return false
+}
+
+// hasProtoMsgPrefix checks whether the supplied opts slice contains the
+// protoMsgPrefix option, and validates and returns the path it contains.
+func hasProtoMsgPrefix(opts []UnmapOpt) (*gpb.Path, error) {
+	for _, o := range opts {
+		if v, ok := o.(*protoMsgPrefix); ok {
+			if v.p == nil {
+				return nil, fmt.Errorf("invalid protobuf prefix supplied, %+v", v)
+			}
+			return v.p, nil
+		}
+	}
+	return &gpb.Path{}, nil
+}
+
+// hasValuePathPrefix checks whether the supplied opts slice contains
+// the valuePathPrefix option, and validates and returns the apth it contains.
+func hasValuePathPrefix(opts []UnmapOpt) (*gpb.Path, error) {
+	for _, o := range opts {
+		if v, ok := o.(*valuePathPrefix); ok {
+			if v.p == nil {
+				return nil, fmt.Errorf("invalid protobuf prefix supplied, %+v", v)
+			}
+			return v.p, nil
+		}
+	}
+	return &gpb.Path{}, nil
+}
+
 // makeWrapper generates a new message for field fd of the proto message msg with the value set to val.
 // The field fd must describe a field that has a message type. An error is returned if the wrong
 // type of payload is provided for the message. The second, boolean, return argument specifies whether

protomap/proto_test.go

@@ -295,7 +295,7 @@ func TestProtoFromPaths(t *testing.T) {
 		desc             string
 		inProto          proto.Message
 		inVals           map[*gpb.Path]interface{}
-		inPrefix         *gpb.Path
+		inOpt            []UnmapOpt
 		wantProto        proto.Message
 		wantErrSubstring string
 	}{{
@@ -441,18 +441,145 @@ func TestProtoFromPaths(t *testing.T) {
 			mustPath("/bytes"): 42,
 		},
 		wantErrSubstring: "got non-byte slice value for bytes field",
+	}, {
+		desc:    "compressed schema",
+		inProto: &epb.ExampleMessage{},
+		inVals: map[*gpb.Path]interface{}{
+			mustPath("/state/compress"): "hello-world",
+		},
+		wantProto: &epb.ExampleMessage{
+			Compress: &wpb.StringValue{Value: "hello-world"},
+		},
+	}, {
+		desc:    "trim prefix",
+		inProto: &epb.Interface{},
+		inVals: map[*gpb.Path]interface{}{
+			mustPath("/interfaces/interface/config/description"): "interface-42",
+		},
+		inOpt: []UnmapOpt{
+			ProtobufMessagePrefix(mustPath("/interfaces/interface")),
+		},
+		wantProto: &epb.Interface{
+			Description: &wpb.StringValue{Value: "interface-42"},
+		},
+	}, {
+		desc:    "trim prefix with valPrefix",
+		inProto: &epb.Interface{},
+		inVals: map[*gpb.Path]interface{}{
+			mustPath("description"): "interface-42",
+		},
+		inOpt: []UnmapOpt{
+			ProtobufMessagePrefix(mustPath("/interfaces/interface")),
+			ValuePathPrefix(mustPath("/interfaces/interface/config")),
+		},
+		wantProto: &epb.Interface{
+			Description: &wpb.StringValue{Value: "interface-42"},
+		},
+	}, {
+		desc:    "invalid message with no annotation on one of its other fields",
+		inProto: &epb.InvalidMessage{},
+		inVals: map[*gpb.Path]interface{}{
+			mustPath("three"): "str",
+		},
+		wantErrSubstring: "received field with invalid annotation",
+	}, {
+		desc:    "invalid message with bad field type",
+		inProto: &epb.BadMessageKeyTwo{},
+		inVals: map[*gpb.Path]interface{}{
+			mustPath("one"): "42",
+		},
+		wantErrSubstring: "unknown field kind",
+	}, {
+		desc:    "extra paths, not ignored",
+		inProto: &epb.Interface{},
+		inVals: map[*gpb.Path]interface{}{
+			mustPath("config/name"):        "interface-42",
+			mustPath("config/description"): "portal-to-wonderland",
+		},
+		inOpt: []UnmapOpt{
+			ProtobufMessagePrefix(mustPath("/interfaces/interface")),
+			ValuePathPrefix(mustPath("/interfaces/interface")),
+		},
+		wantProto: &epb.Interface{
+			Description: &wpb.StringValue{Value: "interface-42"},
+		},
+		wantErrSubstring: `did not map path elem`,
+	}, {
+		desc:    "extra paths, ignored",
+		inProto: &epb.Interface{},
+		inVals: map[*gpb.Path]interface{}{
+			mustPath("config/name"):        "interface-42",
+			mustPath("config/description"): "portal-to-wonderland",
+		},
+		inOpt: []UnmapOpt{
+			ProtobufMessagePrefix(mustPath("/interfaces/interface")),
+			IgnoreExtraPaths(),
+			ValuePathPrefix(mustPath("/interfaces/interface")),
+		},
+		wantProto: &epb.Interface{
+			Description: &wpb.StringValue{Value: "portal-to-wonderland"},
+		},
 	}, {
 		desc:    "field that is not directly a child",
 		inProto: &epb.ExampleMessage{},
 		inVals: map[*gpb.Path]interface{}{
 			mustPath("/one/two/three"): "ignored",
 		},
 		wantProto: &epb.ExampleMessage{},
+	}, {
+		desc:    "value prefix specified - schema path",
+		inProto: &epb.Interface{},
+		inVals: map[*gpb.Path]interface{}{
+			mustPath("description"): "interface-42",
+		},
+		inOpt: []UnmapOpt{
+			ProtobufMessagePrefix(mustPath("/interfaces/interface")),
+			ValuePathPrefix(mustPath("/interfaces/interface/config")),
+		},
+		wantProto: &epb.Interface{
+			Description: &wpb.StringValue{Value: "interface-42"},
+		},
+	}, {
+		desc:    "value prefix specified - data tree path",
+		inProto: &epb.Interface{},
+		inVals: map[*gpb.Path]interface{}{
+			mustPath("description"): "interface-42",
+		},
+		inOpt: []UnmapOpt{
+			ProtobufMessagePrefix(mustPath("/interfaces/interface")),
+			ValuePathPrefix(mustPath("/interfaces/interface[name=ethernet42]/config")),
+		},
+		wantProto: &epb.Interface{
+			Description: &wpb.StringValue{Value: "interface-42"},
+		},
+	}, {
+		desc:    "bad trimmed value",
+		inProto: &epb.Interface{},
+		inVals: map[*gpb.Path]interface{}{
+			mustPath("config/description"): "interface-84",
+		},
+		inOpt: []UnmapOpt{
+			ProtobufMessagePrefix(mustPath("/interfaces/fish")),
+		},
+		wantErrSubstring: "invalid path provided, absolute paths must be used",
+	}, {
+		desc:    "relative paths to protobuf prefix",
+		inProto: &epb.Interface{},
+		inVals: map[*gpb.Path]interface{}{
+			mustPath("config/description"): "value",
+		},
+		inOpt: []UnmapOpt{
+			ProtobufMessagePrefix(mustPath("/interfaces/interface")),
+			ValuePathPrefix(mustPath("/interfaces/interface")),
+		},
+		wantProto: &epb.Interface{
+			Description: &wpb.StringValue{Value: "value"},
+		},
 	}}
 
 	for _, tt := range tests {
 		t.Run(tt.desc, func(t *testing.T) {
-			err := ProtoFromPaths(tt.inProto, tt.inVals, tt.inPrefix)
+			err := ProtoFromPaths(tt.inProto, tt.inVals, tt.inOpt...)
 			if err != nil {
 				if diff := errdiff.Substring(err, tt.wantErrSubstring); diff != "" {
 					t.Fatalf("did not get expected error, %s", diff)