Fix creation of proto values containing oneof fields. (#44)

Closes #36: Fix proto oneof handling on message creation.

common/types/pb/type.go

@@ -123,11 +123,11 @@ func (td *TypeDescription) getFieldsInfo() (map[string]*FieldDescription,
 			for _, oneofProp := range fieldProps.OneofTypes {
 				desc := fieldDescMap[oneofProp.Prop.OrigName]
 				fd := &FieldDescription{
-					desc:   desc,
-					index:  oneofProp.Field,
-					prop:   oneofProp.Prop,
-					oneof:  true,
-					proto3: isProto3}
+					desc:      desc,
+					index:     oneofProp.Field,
+					prop:      oneofProp.Prop,
+					oneofProp: oneofProp,
+					proto3:    isProto3}
 				td.fields[oneofProp.Prop.OrigName] = fd
 				td.fieldIndices[oneofProp.Field] = append(td.fieldIndices[oneofProp.Field], fd)
 			}
@@ -169,11 +169,11 @@ func (td *TypeDescription) getFieldsAtIndex(i int) []*FieldDescription {
 
 // FieldDescription holds metadata related to fields declared within a type.
 type FieldDescription struct {
-	desc   *descpb.FieldDescriptorProto
-	index  int
-	prop   *proto.Properties
-	oneof  bool
-	proto3 bool
+	desc      *descpb.FieldDescriptorProto
+	index     int
+	prop      *proto.Properties
+	oneofProp *proto.OneofProperties
+	proto3    bool
 }
 
 // CheckedType returns the type-definition used at type-check time.
@@ -215,7 +215,15 @@ func (fd *FieldDescription) IsEnum() bool {
 
 // IsOneof returns true if the field is declared within a oneof block.
 func (fd *FieldDescription) IsOneof() bool {
-	return fd.oneof
+	return fd.oneofProp != nil
+}
+
+// OneofType returns the reflect.Type value of a oneof field.
+//
+// Oneof field values are wrapped in a struct which contains one field whose
+// value is a proto.Message.
+func (fd *FieldDescription) OneofType() reflect.Type {
+	return fd.oneofProp.Type
 }
 
 // IsMap returns true if the field is of map type.
@@ -249,8 +257,7 @@ func (fd *FieldDescription) OrigName() string {
 	return fd.prop.OrigName
 }
 
-// Name returns the CamelCase name of the field within the proto generated
-// struct.
+// Name returns the CamelCase name of the field within the proto-based struct.
 func (fd *FieldDescription) Name() string {
 	return fd.prop.Name
 }
@@ -260,16 +267,17 @@ func (fd *FieldDescription) SupportsPresence() bool {
 	return !fd.IsRepeated() && (fd.IsMessage() || !fd.proto3)
 }
 
-// TypeName returns the type name of the field.
-func (fd *FieldDescription) TypeName() string {
-	return sanitizeProtoName(fd.desc.GetTypeName())
-}
-
+// String returns a proto-like field definition string.
 func (fd *FieldDescription) String() string {
 	return fmt.Sprintf("%s %s = %d `oneof=%t`",
 		fd.TypeName(), fd.OrigName(), fd.Index(), fd.IsOneof())
 }
 
+// TypeName returns the type name of the field.
+func (fd *FieldDescription) TypeName() string {
+	return sanitizeProtoName(fd.desc.GetTypeName())
+}
+
 func (fd *FieldDescription) typeDefToType() *checked.Type {
 	if fd.IsMessage() {
 		if wk, found := CheckedWellKnowns[fd.TypeName()]; found {

common/types/provider.go

@@ -134,20 +134,30 @@ func (p *protoTypeProvider) NewValue(typeName string,
 	pbValue := value.Elem()
 
 	// for all of the field names referenced, set the provided value.
-	for fieldName, fieldValue := range fields {
-		if fd, found := td.FieldByName(fieldName); found {
-			fieldName = fd.Name()
+	for name, value := range fields {
+		fd, found := td.FieldByName(name)
+		if !found {
+			return NewErr("no such field '%s'", name)
 		}
-		refField := pbValue.FieldByName(fieldName)
+		refField := pbValue.Field(fd.Index())
 		if !refField.IsValid() {
-			// TODO: fix the error message
-			return NewErr("no such field '%s'", fieldName)
+			return NewErr("no such field '%s'", name)
+		}
+
+		dstType := refField.Type()
+		// Oneof fields are defined with wrapper structs that have a single proto.Message
+		// field value. The oneof wrapper is not a proto.Message instance.
+		if fd.IsOneof() {
+			oneofVal := reflect.New(fd.OneofType().Elem())
+			refField.Set(oneofVal)
+			refField = oneofVal.Elem().Field(0)
+			dstType = refField.Type()
 		}
-		protoValue, err := fieldValue.ConvertToNative(refField.Type())
+		fieldValue, err := value.ConvertToNative(dstType)
 		if err != nil {
 			return &Err{err}
 		}
-		refField.Set(reflect.ValueOf(protoValue))
+		refField.Set(reflect.ValueOf(fieldValue))
 	}
 	return NewObject(value.Interface().(proto.Message))
 }

common/types/providers_test.go

@@ -31,9 +31,9 @@ func TestTypeProvider_NewValue(t *testing.T) {
 	if sourceInfo := typeProvider.NewValue(
 		"google.api.expr.v1.SourceInfo",
 		map[string]ref.Value{
-			"Location":    String("TestTypeProvider_NewValue"),
-			"LineOffsets": NewDynamicList([]int64{0, 2}),
-			"Positions":   NewDynamicMap(map[int64]int64{1: 2, 2: 4}),
+			"location":     String("TestTypeProvider_NewValue"),
+			"line_offsets": NewDynamicList([]int64{0, 2}),
+			"positions":    NewDynamicMap(map[int64]int64{1: 2, 2: 4}),
 		}); IsError(sourceInfo) {
 		t.Error(sourceInfo)
 	} else {
@@ -46,6 +46,22 @@ func TestTypeProvider_NewValue(t *testing.T) {
 	}
 }
 
+func TestTypeProvider_NewValue_OneofFields(t *testing.T) {
+	typeProvider := NewProvider(&expr.ParsedExpr{})
+	if exp := typeProvider.NewValue(
+		"google.api.expr.v1.Expr",
+		map[string]ref.Value{
+			"literal_expr": NewObject(&expr.Literal{LiteralKind: &expr.Literal_StringValue{StringValue: "oneof"}}),
+		}); IsError(exp) {
+		t.Error(exp)
+	} else {
+		e := exp.Value().(*expr.Expr)
+		if e.GetLiteralExpr().GetStringValue() != "oneof" {
+			t.Errorf("Expr with oneof could not be created: %v", e)
+		}
+	}
+}
+
 func TestTypeProvider_Getters(t *testing.T) {
 	typeProvider := NewProvider(&expr.ParsedExpr{})
 	if sourceInfo := typeProvider.NewValue(

interpreter/interpreter_test.go

@@ -93,22 +93,32 @@ func TestInterpreter_ComprehensionExpr(t *testing.T) {
 }
 
 func TestInterpreter_BuildObject(t *testing.T) {
-	parsed, errors := parser.ParseText("v1.Expr{id: 1}")
+	parsed, errors := parser.ParseText("v1.Expr{id: 1, " +
+		"literal_expr: v1.Literal{string_value: \"oneof_test\"}}")
+	if len(errors.GetErrors()) != 0 {
+		t.Errorf(errors.ToDisplayString())
+	}
 
 	pkgr := packages.NewPackage("google.api.expr")
 	provider := types.NewProvider(&expr.Expr{})
 	env := checker.NewStandardEnv(pkgr, provider, errors)
 	checked := checker.Check(parsed, env)
+	if len(errors.GetErrors()) != 0 {
+		t.Errorf(errors.ToDisplayString())
+	}
 
 	i := NewStandardIntepreter(pkgr, provider)
 	eval := i.NewInterpretable(NewCheckedProgram(checked))
 	result, _ := eval.Eval(NewActivation(map[string]interface{}{}))
-	if !proto.Equal(
-		result.(ref.Value).Value().(proto.Message),
-		&expr.Expr{Id: 1}) {
+	expected := &expr.Expr{Id: 1,
+		ExprKind: &expr.Expr_LiteralExpr{
+			LiteralExpr: &expr.Literal{
+				LiteralKind: &expr.Literal_StringValue{
+					StringValue: "oneof_test"}}}}
+	if !proto.Equal(result.(ref.Value).Value().(proto.Message), expected) {
 		t.Errorf("Could not build object properly. Got '%v', wanted '%v'",
 			result.(ref.Value).Value(),
-			&expr.Expr{Id: 1})
+			expected)
 	}
 }