Switch to proto3 and use oneof to get around optionals

[kevinkreiser]

NOTE: This PR is not pointed at master its pointed at the PR for the other step here which was to remove defaults: #3454 . I'll point this at master once that one is merged

This PR converts all of our request/response main protos to proto3. This is the final PR to make the switch and is the most significant change though its completely binary compatible with the existing proto2 definition. Let me explain...

When they first release proto3 they removed optional support. What that means is that optional is not a keyword in the proto3 DSL. What this means is that from the generated code that comes out of protoc you can't tell whether a scalar field in a proto message has been not set or is just the default value. Now internally the protobuf libraries can tell the difference. In fact all of the has_field_name methods do still exist, but they are privated. Now after people gave the maintainers of protobuf a bunch of flack about optional support they added it back to the language in version 3.15 or so but this version is still pretty new so I opted not to require it.

The good news is though that I didnt have to rewrite all of our code, at least not with new semantics. You can fake optional support by using the oneof keyword. This allows you to have a bunch of fields in a protobuf message and make sure that only one of them (or none of them) is present at any one time. The cool thing about oneof is that when none of them are set then its like having optional in that you can check for that case. The other cool thing about it, is that its binary compatible with the regular old school proto2 plain old optional scalar fields. So what does this mean concretely.

Currently our protobuf looks like this:

syntax = "proto2";
message foo {
  optional int32 bar = 1;
}

And we get methods like bool foo::has_bar(); when we generate the code. But in proto3 we cant have that anymore, so we use the oneof trick:

syntax = "proto3";
message foo {
  oneof has_bar {
    int32 bar = 1;
  }
}

So as you can see its a lot more verbose, but you do end up getting a method like foo::HasBarCase has_bar_case();, which again is quite fugly but at least you can tell if the damned thing is set or just zero'd out.

The good news is that in the future when protoc 3.15 or later becomes the standard version we can upgrade to, we can yank out the stupid oneof and put the optional keyword back and revert all the code changes (really just renames) I made in the code here. So basically once newer versions of protoc become wide spread we can change the proto back to something simple like proto2 but still use proto3:

syntax = "proto3";
message foo {
  optional int32 bar = 1;
}

[kevinkreiser]

everything is proto3 now

[kevinkreiser]

actually none of these needed to be oneof we only need it for scalar fields. ill revert this

[kevinkreiser]

this is the main change, every scalar type that was optional which was all of them, needs to become a oneof

[kevinkreiser]

and this is what the changes in code look like. i named all of the oneof with the has_ prefix so that the beginnign wouldnt change, but there was no easy way to avoid having to put the _case on the end because the damned bindings that protoc creates make the normal has_field method private grr

[nilsnolde]

one thing I'm not quite clear about, though it's only half-important:
we can yank out the stupid oneof and put the optional keyword back. I got a bit confused by what you were saying last night. I know you checked that the changes right now are binary-compatible, but does that automatically mean that the other way around will work too (removing oneof to replace with v3 new optional)? in worst case we'd be stuck with the oneofs which would also be tolerable..

other than that, seems like you missed on spot enhancedtrippath.cc:1164:43: error: 'bool valhalla::TransitRouteInfo::has_onestop_id() const' is private within this context (think that's trace protected)

rest lgtm. ignoring the added overhead to proto files, the whole transition isn't that bad (not counting the annoying work to get there, so thanks:))

[kevinkreiser]

@nilsnolde regarding the comment

we can yank out the stupid oneof and put the optional keyword back

i just meant that once 3.15 of protoc becomes mainstream we can literally change the proto defintion back to

syntax = "proto3";
message foo {
  optional int32 bar = 1;
}

which is basically proto2 syntax but with proto3 support and undo all the _case changes and oneofs that i had to add to the code and to the proto file. the reason we can do this is because all of them are binary equivalent. i tested with proto2, proto3 v3.6 (which doesnt have optional support) and with proto3 v3.19 (which does have optional support). the result of the test is that all of the messages serialize and deserialize properly with all versions of the code, just that the bindings for proto3 v3.6 require us to use oneof and those shitty named functions to check the oneofs where as old proto2 and latest proto3 have no such ugliness.

let me know if that made sense

[nilsnolde]

ah yeah sure, makes sense, thanks.

[kevinkreiser]

so the actual c++ code is not more verbose, despite the diff numbers. the reason it looks like im double the amount of code is because i am doubling the amount of lines in the proto files since using oneof is more verbose. i could put that stuff on a single line to reduce the line numbers if that would be prefered.

[nilsnolde]

I have not much to add, other than thanks for the effort!