support one-ofs in body

[jhump]

I was curious if this was something that might be accepted as a patch, and (if so) if I'm on the right track.

I must admit, I don't really understand how the FieldPath stuff really works. I mean, I think I see how it's supposed to work. But it looks like actually using nested elements (e.g. foo.bar.baz) would always result in nil dereference panic since the actual request's fields have zero values. So, in this example, field protoReq.Foo, presumably a pointer to message struct, would be nil, and thus unmarshaling code would traverse a nil pointer trying to compute &protoReq.Foo.Bar.Baz (even if it were just one level of nesting: &protoReq.Foo.Bar).

So, in this patch, I didn't really bother examining the entire path for oneofs and only look at the first field. Does that seem sane?

BTW, the whole reason I was exploring this patch is to support an API idiom like so:

message IndividualId {
  oneof id {
    string uid = 1;
    int64 user_id = 2;
  }
}

service Users {
  rpc QueryIndividual (IndividualId) returns (IndividualDetails) {
    option (google.api.http) = {
      get: "/users/v1/individual/{uid}"
      additional_bindings {
        get: "/users/v1/user/{user_id}"
      }
    };
  }
}

[achew22]

Hey @jhump, sorry for the very slow response on this. Let me give you an update on the state of affairs around here. I have a PR that I'm going to be submitting soon that is the beginning of the breaking change window for a 2.0 release. As such I've been focusing on getting that done and not really tending to the PR queue. Sorry about that.

This looks like a really good PR to have included and I don't mind at all that it doesn't completely solve the recursive form of the problem -- the perfect is the enemy of the good after all. One thing I noticed is that you've hit a bug in our CI system. I am curious. Do you think you could rebase this PR after that commit and see if the CI system blows up?

I would have expected this to fail since you changed the template which should make the examples directory (which is a dir of goldens) not match. To fix this, you can run make examples to update the goldens and then the PR should pass CI.

Thanks so much for your hard work!

[achew22]

Friendliest of pings

[jhump]

Looks like this has already been fixed for path params but is still broken for the body. The current code produces Go code that can't compile if a oneof is specified as the body. I think I see how to address that.

[jhump]

This is what generated bad code. The AssignableExpr would have emitted the prep statements, which would get incorrectly inlined in the call to Decode. By splitting out the prep statements, it now generates working code.

[jhump]

I essentially forked AssignableExpr, with this method preserving just the preparations and the other only emitting the last line, the actual field ref.

[jhump]

@achew22, PTAL

[jhump]

The build breakage was bad timing - I pushed just after protobuf merged their dev branch to master. I will rebase this PR after #636 lands, to see if I can get the tests to go green.

[achew22]

@yugui Have you seen this?

[codecov-io]

Codecov Report

Merging #570 into master will decrease coverage by 0.43%.
The diff coverage is 36.84%.

@@            Coverage Diff             @@
##           master     #570      +/-   ##
==========================================
- Coverage   58.88%   58.44%   -0.44%     
==========================================
  Files          30       30              
  Lines        2853     2871      +18     
==========================================
- Hits         1680     1678       -2     
- Misses       1010     1030      +20     
  Partials      163      163

Impacted Files	Coverage Δ
protoc-gen-grpc-gateway/gengateway/template.go	54.9% <ø> (ø)	⬆️
protoc-gen-grpc-gateway/descriptor/types.go	40.47% <36.84%> (-8.6%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c2b051d...26d004c. Read the comment docs.

[jhump]

@achew22 or @yugui: ping?

[tmc]

Code looks LGTM. My only comment is that I'd like a test case that demonstrates oneof population from body.

[apeletz512]

@jhump @achew22 @yugui @tmc Any progress here?

[johanbrandhorst]

Bump, need a rebase and a test case please.

[jhump]

@johanbrandhorst, sorry, I've been busy, and we've since re-formulated our proto to not actually need this feature. So, if this seems like a valuable contribution, someone else will have to take it over.