issue with oneof, get, and non-primitive

[venezia]

Steps you follow to reproduce the error:

• Create a Message that has one ore more nested messages as oneof possibilities

message GetSomethingMsg {
    string name = 1;
    oneof credentials {
        AWSCredentials aws = 2;
        AzureCredentials azure = 3;
    }
}

message AWSCredentials {
    string secret_key_id = 1;
    string secret_access_key = 2;
    string region = 3;
}

message AzureCredentials {
    string app_id = 1;
    string tenant = 2;
    string password = 3;
    string subscription_id = 4;
}

• Have a service offer to use it with a get and with a post

rpc GetSomething (GetSomethingMsg) returns (GetSomethingReply) {
    option (google.api.http) = {
        get : "/api/v1/something"
    };
}
rpc GetSomethingV2 (GetSomethingMsg) returns (GetSomethingReply) {
    option (google.api.http) = {
        post : "/api/v2/something"
        body : "*"
    };
}

• Try using the GET service

curl "http://localhost:9050/api/v1/something?name=abc&azure.app_id=abc&azure.tenant=def&azure.password=ghi&azure.subscription_id=jkl"
{"error":"field already set for credentials oneof","message":"field already set for credentials oneof","code":3}

The POST will work just fine, as expected

What did you expect to happen instead:

Both solutions to work

What's your theory on why it isn't working:

I've noticed that if I only set a single value for the nested message, it does not error out. So if I do

curl "http://localhost:9050/api/v1/something?name=abc&azure.app_id=abc"

It will be happy with me - which makes me think there is some glitch in how get parameters are being parsed. The post is doing fine, so I suspect it has something to do with assembling the get values together into a single object.

I could totally be wrong too.

There are similar issues - #460 and #413 - but they seem to be different, so I figured the issue was worth filing.

This was all done using protoc 3.6.1, grpc-gateway 1.5.0, golang/protobuf 1.2.0 - at least if dep has not deceived me.

Thanks!

[johanbrandhorst]

Thanks for raising this issue. There are a couple of known shortcomings with the translation from query parameters to proto request messages, as you noticed. I don't think anyone is working on a fix for this right now and the recommendation is usually to use a POST where you need this kind of information.

Of course, we'll be happy to help you get a PR in that would resolve this issue. You've got a good test case here, so you might be able to fix this :). What do you say?

[waveywaves]

@johanbrandhorst @venezia I would like to work on this if that is fine :)

[johanbrandhorst]

@waveywaves of course! Thanks!

[waveywaves]

I have started work on this. Please assign it to me.

[johanbrandhorst]

Doesn't seem like I can assign it to you, but consider yourself the owner.

[coderbradlee]

@johanbrandhorst this still not fixed,right?

[venezia]

I can take a look at this later this weekend perhaps - thought someone was doing this - sorry

[johanbrandhorst]

Feel free @venezia, don't think @waveywaves got anywhere yet.

[coderbradlee]

@johanbrandhorst I want to know is there anyone helping this?when will this could be done? thanks

[achew22]

@lzxm160 it sounds like there is no one working on it. I think this would be a great starter project to learn the inner workings of the project. Feel free to comment here if you need any pointers on where to start.

[chimeworld]

Hi, I'd like to work on this if possible.

[johanbrandhorst]

Ok, go ahead, I don't think anyone is working on it.

[chimeworld]

Can you point to where I need to go to get started on this?

@johanbrandhorst

[johanbrandhorst]

It would probably be somewhere in https://github.com/grpc-ecosystem/grpc-gateway/blob/master/runtime/query.go. The easiest thing might be to create a failing test and add it to https://github.com/grpc-ecosystem/grpc-gateway/blob/master/runtime/query_test.go first.

[chimeworld]

I'll be working on this over the next few days/weeks.

[hkarimi1985]

Hello, any update on this?

[chimeworld]

@hkarimi1985 not yet

[eperinan]

Hello, any update on this?

[johanbrandhorst]

Doesn't look like it, if you've got the time to look into this, please feel free. It doesn't seem like @chimeworld found the time for it yet.

[ping-localhost]

This issue seems to also apply for additional_bindings, when using something like the following:

import "google/api/annotations.proto";

service FooBarAPI {
  rpc FooBar(FooBarRequest) returns (FooBarResponse) {
    option (google.api.http) = {
      post: "/foo/bar/account/{account_id}"
      body: "*"
      additional_bindings: {
        post: "/foo/bar/iban/{email_iban.iban}/email/{email_iban.email}"
        body: "*"
      }
    };
  }
}

message FooBarRequest {
  // The account you are requesting.
  oneof account {
    // The UUID of the account.
    string account_id = 1;
    // The email + IBAN as a message.
    EmailIBAN email_iban = 2;
  }
  // Message that contains email and IBAN, useful when selecting an account.
  message EmailIBAN {
    // The IBAN of the account.
    string iban = 1;
    // The email of the account.
    string email = 2;
  }
}

message FooBarResponse {
  bool success = 1;
}

This generates the following code:

var (
	val string
	e   int32
	ok  bool
	err error
	_   = err
)

val, ok = pathParams["email_iban.iban"]
if !ok {
	return nil, metadata, status.Errorf(codes.InvalidArgument, "missing parameter %s", "email_iban.iban")
}

err = runtime.PopulateFieldFromPath(&protoReq, "email_iban.iban", val)

if err != nil {
	return nil, metadata, status.Errorf(codes.InvalidArgument, "type mismatch, parameter: %s, error: %v", "email_iban.iban", err)
}

val, ok = pathParams["email_iban.email"]
if !ok {
	return nil, metadata, status.Errorf(codes.InvalidArgument, "missing parameter %s", "email_iban.email")
}

err = runtime.PopulateFieldFromPath(&protoReq, "email_iban.email", val)

if err != nil {
	return nil, metadata, status.Errorf(codes.InvalidArgument, "type mismatch, parameter: %s, error: %v", "email_iban.email", err)
}

And throws the error type mismatch, parameter: email_iban.email, error: field already set for account oneof when it tries to set email_iban.email.

---

EDIT: I was still using v1.15.0. It works on v2.0.1.

[momom-i]

Hello, it doesn't seem anyone works on it now and I'd like to do it.

[johanbrandhorst]

That'd be great! Just like in the other PR, please start by adding a failing test 🙂.

It would probably be somewhere in https://github.com/grpc-ecosystem/grpc-gateway/blob/master/runtime/query.go. The easiest thing might be to create a failing test and add it to https://github.com/grpc-ecosystem/grpc-gateway/blob/master/runtime/query_test.go first.

[momom-i]

@johanbrandhorst
I found I couldn't reproduce the error above (I tried that in here).
And also found it seemed be fixed in v2 of grpc_gateway. What do you think?

[johanbrandhorst]

That's great news, would you be willing to contribute these extra tests to so we can protect against breaking it in the future? Then we can close this issue too.

[momom-i]

I've created the issue. I'd appreciate if you'd check it, thank you!