URL usage of nested messages causes nil pointer in proto3

[mwitkow]

Take this as an example (in proto3):

message DeploymentId {
  string name = 1;
}

message FlagId {
  DeploymentId deployment = 1;
  string name = 2;
}

message Flag {
  FlagId id = 1;
  string value = 2;
  string description = 3;
}

service Deployments {
  rpc GetFlag (FlagId) returns (Flag) {
    option (google.api.http) = {
      get: "/deployment/v1/{deployment.name}/flag/{name}"
    };
  }
}

When I call the service through GET /deployment/v1/mydeployment/flag/myflag I get a nil pointer dereference:

2015/07/26 18:58:31 http: panic serving [::1]:59983: runtime error: invalid memory address or nil pointer dereference
goroutine 18 [running]:
net/http.func·011()
    /usr/local/Cellar/go/1.4/libexec/src/net/http/server.go:1130 +0xbb
github.com/mwitkow-io/grpc-experiment/proto/deployments.request_Deployments_GetFlag0(0xb5fb88, 0xc2080eda80, 0xb5fc00, 0xc2080363a8, 0xc208032410, 0xc20811a240, 0x0, 0x0, 0x0, 0x0)
    /Users/michal/code/mygo/src/github.com/mwitkow-io/grpc-experiment/proto/deployments/deployments.pb.gw.go:46 +0x84b
github.com/mwitkow-io/grpc-experiment/proto/deployments.func·003(0xb60068, 0xc208042780, 0xc208032410, 0xc20811a240)

the Gateway-generated code in question:

func request_Deployments_GetFlag_0(ctx context.Context, client DeploymentsClient, req *http.Request, pathParams map[string]string) (msg proto.Message, err error) {
    var protoReq FlagId

    var val string
    var ok bool

    val, ok = pathParams["deployment.name"]
    if !ok {
        return nil, grpc.Errorf(codes.InvalidArgument, "missing parameter %s", "deployment.name")
    }
    protoReq.Deployment.Name, err = runtime.String(val)
    if err != nil {
        return nil, err
    }

    val, ok = pathParams["name"]
    if !ok {
        return nil, grpc.Errorf(codes.InvalidArgument, "missing parameter %s", "name")
    }
    protoReq.Name, err = runtime.String(val)
    if err != nil {
        return nil, err
    }

    if err := runtime.PopulateQueryParameters(&protoReq, req.URL.Query(), filter_Deployments_GetFlag_0); err != nil {
        return nil, grpc.Errorf(codes.InvalidArgument, "%v", err)
    }

    return client.GetFlag(ctx, &protoReq)
}

The problem seems to be that the Deployment field of FlagId is not initialized and is a nil reference:

type FlagId struct {
    Deployment *DeploymentId `protobuf:"bytes,1,opt,name=deployment" json:"deployment,omitempty"`
    Name       string        `protobuf:"bytes,2,opt,name=name" json:"name,omitempty"`
}

I found some complex expressions like {bucket_name=buckets/*} inparse_test.go`, but I don't think these would help here. @yugui is this a bug or am I just using grpc gateway wrong?

[mwitkow]

Yup, indeed, intalizing the field seems to have worked:

    if protoReq.Deployment == nil {
        protoReq.Deployment = &DeploymentId{}
    }
    protoReq.Deployment.Name, err = runtime.String(val)
    if err != nil {
        return nil, err
    }

The tricky bit is to initialize the field without knowing what the type is. Some reflection perhaps? Or template-generating a NewDeploymentField that will act like Deployment.Reset()

[mwitkow]

The following function is my (probably misguided) stab at reflecting out of the problem. It instantiates all missing structs along the way.

func instantiateMissingStructs(structPtr interface{}, fieldPath string) (error) {
    fieldNames := strings.Split(fieldPath, ".")
    ptrToStruct := reflect.ValueOf(structPtr)
    if ptrToStruct.Kind() != reflect.Ptr {
        return fmt.Errorf("structPtr must be a pointer struct, is %v", ptrToStruct.Kind())
    }
    currentStruct := reflect.Indirect(ptrToStruct)
    if currentStruct.Kind() != reflect.Struct {
        return fmt.Errorf("structPtr must be a pointer to a struct, is %v.", ptrToStruct.Kind())
    }
    for _, name := range fieldNames {
        fieldV := currentStruct.FieldByName(name)
        if fieldV.Kind() != reflect.Ptr {
            return fmt.Errorf("field %v must be a pointer", name)
        }
        concreteType := fieldV.Type().Elem()
        if concreteType.Kind() != reflect.Struct {
            return fmt.Errorf("field %v must be a pointer to a struct", name)
        }
        if fieldV.IsNil() {
            fieldV.Set(reflect.New(concreteType))
        }
        currentStruct = reflect.Indirect(fieldV)
    }
    return nil
}

For example in the above example, this would instantiate the missing DeploymentId struct under the Deployment field.

instantiateMissingStructs(&protoReq, "Deployment")

Or if the URL reference was to deployment.app.id, and the assignment was protoReq.Deployment.App.Id, the following one would instantiate both DeploymentId and AppId:

instantiateMissingStructs(&protoReq, "Deployment.App")

Any idea where to hook this up? I guess gengo/grpc-gateway/protoc-gen-grpc-gateway/gengateway/generator.go, but I don't know how to do the tests for it :(

[mwitkow]

So yea, if golang/protobuf#54 makes it instantiate a new type, this is trivial.
Otherwise, I think I'll make a PR that reuse the query filter magic.

[mwitkow]

So I didn't realise that the grpc-gateway already had a reflection instantiator for the query filtering. The above pull request contains the fix that reuses this.

[mwitkow]

@yugui, do you need anything from me in relation to #34 to get this fixed? :)

[yugui]

Sorry for my late reply. I have just merged #34.

On Tue, Aug 11, 2015 at 4:36 PM Michal Witkowski [email protected]
wrote:

@yugui https://github.com/yugui, do you need anything from me in
relation to #34 #34 to get
this fixed? :)

—
Reply to this email directly or view it on GitHub
#32 (comment).