protoc-gen-go: generate Setters for oneof fields

[thakkarparth007]

The library provides only Getters for oneof fields. Because of this, setting oneof fields is painful. And this gets especially painful when the oneof fields are themselves proto messages. For example:

message RequestWrapper {
    string request_id = 1;
    string session_id = 2;

    oneof request {
		actions.BuyStocksFromExchangeRequest buy_stocks_from_exchange_request = 3;
		actions.CancelAskOrderRequest cancel_ask_order_request = 4;
		actions.CancelBidOrderRequest cancel_bid_order_request = 5;
                . . .
    }
}

message ResponseWrapper {
    string request_id = 1;

    oneof response {
		actions.BuyStocksFromExchangeResponse buy_stocks_from_exchange_response = 3;
		actions.CancelAskOrderResponse cancel_ask_order_response = 4;
		actions.CancelBidOrderResponse cancel_bid_order_response = 5;
                . . .
    }
}

message DalalMessage {
    oneof message_type {
        RequestWrapper request_wrapper = 1;
        ResponseWrapper response_wrapper = 2;
        DataStreamUpdateWrapper data_stream_update_wrapper = 3;
    }
}

Here, The Response messages themselves have oneof fields. Creating a DalalMessage type, then, has to be done in this manner:

dm := &socketapi_proto.DalalMessage{}
dm.MessageType = &socketapi_proto.DalalMessage_ResponseWrapper{
	ResponseWrapper: &socketapi_proto.ResponseWrapper{
		Response: &socketapi_proto.ResponseWrapper_BuyStocksFromExchangeResponse{
			BuyStocksFromExchangeResponse: response.(*actions_proto.BuyStocksFromExchangeResponse),
		},
	},
}

This is clearly painful. Instead, if setter methods are provided, the same code would be much shorter and readable:

rw := &socketapi_proto.DalalMessage_ResponseWrapper{}
rw.SetResponse(response.(*actions_proto.BuyStocksFromExchangeResponse))

dm := &socketapi_proto.DalalMessage{}
dm.SetResponseWrapper(rw)

Of course, this can be shortened further if protobuf does not wrap the fields inside a oneof field, but I assume that will break a lot of people's code. I also don't know the side effects of not wrapping oneof fields.

Even if the oneof fields stay wrapped, I believe that providing Setters will neither break anyone's code, nor will cause any side effects.

Sample output after adding a Setter:

// Generated already
func (m *DalalMessage) GetRequestWrapper() *RequestWrapper {
	if x, ok := m.GetMessageType().(*DalalMessage_RequestWrapper); ok {
		return x.RequestWrapper
	}
	return nil
}

// Generated as per this proposal
func (m *DalalMessage) SetRequestWrapper(rw *RequestWrapper) {
	m.MessageType = &DalalMessage_RequestWrapper{
		RequestWrapper: rw,
	}
}

I'm sure there are others facing this issue. (#205).

[bcmills]

Your example is already somewhat more verbose than necessary. It can be written today as:

dm := &socketapi_proto.DalalMessage{
  MessageType: &socketapi_proto.DalalMessage_ResponseWrapper{
    &socketapi_proto.ResponseWrapper{
      Response: &socketapi_proto.ResponseWrapper_BuyStocksFromExchangeResponse{
        response.(*actions_proto.BuyStocksFromExchangeResponse),
      },
    },
  },
}

It would be somewhat simpler if golang/go#12854 were accepted. Then your example could be written:

dm := &socketapi_proto.DalalMessage{
  MessageType: &socketapi_proto.DalalMessage_ResponseWrapper{{
    Response: &socketapi_proto.ResponseWrapper_BuyStocksFromExchangeResponse{
      response.(*actions_proto.BuyStocksFromExchangeResponse),
    },
  }},
}

[thakkarparth007]

Hmm, I wasn't aware that we could omit the field name if there was only one field. Also, the golang/go#12854 PR would make it much simpler, but I still don't see how a setter could harm. It would provide a nice uniform interface - this way there'd be no need to access the oneof field, instead we could directly access the 'children' - so to say. So in the above, I could simply call dm.SetResponseWrapper() without of even knowing about existence of MessageType.

Also, setters are available in other languages.

[aajtodd]

I too find this more painful than it needs to be.

Curious why is a wrapper type generated for oneof fields? As far as I can tell the wrapper types all just implement an interface isXYZ_Type.

[danielbprice]

Hmm, I wasn't aware that we could omit the field name if there was only one field. Also, the golang/go#12854 PR would make it much simpler, but I still don't see how a setter could harm.

@thakkarparth007 I think this is a good point, and it doesn't really look to me like golang/go#12854 will be accepted any time before Go2, if then. Whereas we could have a setter today.

I'm trying to clean up some sprawling, overloaded protobufs with too many fields by replacing with oneofs which more accurately describe each use case, but I am concerned that other developers in my project will reject the awkwardness required to wade through this nesting. It's really not ergonomic at all. For me, what gogo provides in its "onlyone" extension (https://github.com/gogo/protobuf/tree/master/plugin/union) is what I'm looking for, but I don't want to switch out to that if I can help it.

[awalterschulze]

onlyone is a solution I enjoy using, but unfortunately it also only works if your union is between different field types. So I have sympathy for fixing this in the general case.
gogoprotobuf really struggled coming up with a better solution for oneof
gogo/protobuf#168
The current generated code is far from something that I want to use (so I avoid using oneof), but coming up with a good solution in a language which does not support tagged unions is really hard.
My advice is to stop using Go ;)

[thakkarparth007]

@aajtodd how the current implementation works is by generating a field for the one-of, rather than the children of one-of. For example:

message Foo {
    oneof bar {
        uint32 apples = 1;
        string oranges = 2;
    }
}

This creates a struct Foo with one field "bar". So if both "apples" and "oranges" - of types uint32, and string respectively are to be assigned to the same field "bar", then they must have the same type. That is achieved by wrapping them into a empty interface of a common name. That allows you to set bar to an integer, or to a string, or whatever you have inside the "oneof".

[danielbprice]

Your example is already somewhat more verbose than necessary. It can be written today as:

@bcmills @thakkarparth007 FWiW I finally got around to trying this, and go vet flags it:

.../blah.go:100: bg/common_msg.Blah_Blah composite literal uses unkeyed fields

So for us it's a non-starter, as the rest of our codebase is vet clean.

[davidraleigh]

I believe a dedicated setter would be a great improvement.

A setter would make the messages less verbose AND it would allow proto file providers to document an API usage pattern that won't break if a oneof block is removed or added in the future.

For Java, C++, JavaScript and Python the way you set parameters for messages doesn't change if the proto file uses oneof blocks or doesn't use them. For Go, since there is no dedicated uniform setter method style, a users code will break if there is a removal or an insertion of a oneof definition. You're essentially locked into that definition with Go, but with other protobuf languages you can change it over time.

I would like to use oneof and I'm willing to commit to not taking them out in the future, but it's so absurdly verbose, I think users will be put off from using the client library all together. A setter method would be a great way for the message building process to still be manageable to someone learning a new library.

Below is an example of with and without oneof.

        operator := &pb.OperatorRequest{
		LeftGeoms:&pb.OperatorRequest_LeftGeometryBag{&serviceGeometry}, 
		RightGeoms:&pb.OperatorRequest_RightGeometryBag{&cutterGeometry}}

	operator := &pb.OperatorRequest{
		LeftGeometryBag:&serviceGeometry, 
		RightGeometryBag:&cutterGeometry}

I'm new to Go, so I maybe misunderstanding how to use protoc generated code.

[dsnet]

The current solution is certainly not ideal, but I'm going to put this on hold for until there's a resolution on golang/go#19412.

[danielbprice]

@dsnet, I recently read most (but not all) of golang/go#19412, and it seems:

• There's not any sort of concrete plan to go forward with that proposal.
• It would exclusively target Golang 2.

Golang 1.x seems likely to be around for a long time; a solution now would benefit many implementors at least for several years to come. Could this be designed in such a way that it's not obviously clashing with the aforementioned proposal?

[kv1122]

So after a good hour or so of copy/pasting, I now have setters on my messages.

Hay at least in Go, you can add methods to a type in another file:)

extra.go:

func (r *Req) SetXXXReq(req *XXXReq) {
	r.Body = &Req_XXXReq{XXXReq: req}
}

func (r *Rsp) SetYYYRsp(rsp *YYYRsp) {
	r.Body = &Rsp_YYYRsp{YYYRsp: rsp}
}

[dsnet]

By default we cannot enable this feature since the "SetXXX" method could conflict with an actual field named "SetXXX". This is not a hypothetical scenario, but a real problem.

At best, this would be a generator option that one can opt-into.

[bogdandrutu]

Don't know if this was already answered, but I saw this question asked couple of times, why is it necessary to have a wrapper around other messages in an oneof? I do understand why you need the message for the primitives but not sure why for messages. Because of that an unnecessary extra allocation is needed.

I know may be too late to change but want to understand if I miss something

[gonzojive]

By default we cannot enable this feature since the "SetXXX" method could conflict with an actual field named "SetXXX". This is not a hypothetical scenario, but a real problem.

At best, this would be a generator option that one can opt-into.

It seems you could work around method name conflicts the same way other name conflicts are handled - append an underscore or something.

I have no problem with an option but I disagree that setters would need to be an opt-in at best.

[reddaly]

It sounds like how to handle the conflict in a case like the following is the main complication of the proposal:

message Collection {
  Collection set_of_numbers = 1;
  oneof contrived_oneof {
    Bar of_numbers = 2;
  }
}

type Thing struct {
  SetOfNumbers *Collection // should this be called SetOfNumbers_, or should the setter be called SetOfNumbers_(?
  ContrivedOneof Thing_ContrivedOneof
}
// ...
func (m *Thing) SetOfNumbers_(v *Bar) { /* ... */ }

/// OR

type Thing struct {
  SetOfNumbers_ *Collection
  ContrivedOneof Thing_ContrivedOneof
}
// ...
func (m *Thing) SetOfNumbers(v *Bar) { /* ... */ }

There is probably logic in the reflection package that is relevant to handling the edge case, perhaps among other places.

[lcmaguire]

https://github.com/lcmaguire/protoc-gen-go-setters

there is a plugin if you want auto generated setters