Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature request: Add setters #65

Closed
ekle opened this issue Aug 20, 2015 · 15 comments
Closed

Feature request: Add setters #65

ekle opened this issue Aug 20, 2015 · 15 comments

Comments

@ekle
Copy link

ekle commented Aug 20, 2015

generated message-types have only getters but no setters at the moment.
This makes it impossible to create an interface which can change values of them.

This would be especially useful for the return messages of rpc calls.
For example a generic function which adds an request-id to every response.

@dsymonds
Copy link
Contributor

Nope. The code bloat does not warrant the minor utility, which you can achieve with your own wrapper types, or with reflection.

@daemonl
Copy link

daemonl commented Sep 2, 2015

Are you sure we can't discuss this? I don't see this as minor, I have the exact same issue, want to set ResponseID from RequestID.

Alternatively, and this may already exist, could there be / is there already a way to add a hook for 'extra' parts like setters?

@jbowens
Copy link

jbowens commented Sep 11, 2015

+1

@jspiro
Copy link

jspiro commented May 27, 2017

@dsymonds With due respect, that's an obnoxiously terse response to a legitimate request. If it was so easy @ekle would not have asked. I expect you'll close the discussion next? This is typical golang community "you're a bad programmer for asking" behavior; we can be better than that.

Let's discuss this like engineers. In response to your assertions:

  • Many of the other protobuf generators (e.g. C++ and Java) generate setters.
  • I've spent the last hour trying to modify a fork to add setters and it's a nightmare if you're not familiar with the code.
  • Setters are not minor. If you do NEED setters, you cannot add them to the original structs outside of the package, and generating them requires a non-trivial amount of work.
  • Reflection for production code: Seriously?
  • Setters for protos are particularly necessary in Go. Go is not templated, and you cannot create an interface for struct fields; there's no easy way to write a function that sets a particular field name that many proto messages share. You'd need one for each proto type or wrappers and interfaces. A setter is an elegant and trivial solution since you a single small interface satisfies it. That's the Go way.
  • Code bloat? It's generated code. Protobufs are supposed to provide lots of varied functionality out of the box. Are you suggesting that the setters are going to get compiled in even if you don't use them, and that they could possibly take up much space?
  • Code bloat part 2: Hello world in Go is 5mb.

Manually writing wrappers for all messages is a terrible solution; generating them is marginally better but both are a large amount of non-trivial work for a problem easily and naturally solved in this library: Adding setters.

@jspiro
Copy link

jspiro commented May 27, 2017

In the spirit of showing my work, this patch was sufficient to get my project moving again, and I was able to eliminate a lot redundant functions with a two line interface as a result.

@ekle
Copy link
Author

ekle commented May 27, 2017

@jspiro thats a cool patch. at the moment i'm using a big Makefile with some bash/grep magic to transform the existing getters to setters in a new file.

@dsnet
Copy link
Member

dsnet commented May 27, 2017

@jspiro, I don't think it's helpful to summarize @dsymonds as simply saying "you're a bad programmer for asking" since he says nothing of the sort. Even if someone closes the issue, you can always respond back with more discussion points as you have done here.

Points to consider:

  1. Ability to set some generic field on differing proto types. While setters seem like the right solution since you can create an interface for the field you are interested in. However, I believe this is wrong abstraction to perform that work. This approach makes the assumption that all proto.Message types are generated by protoc-gen-go, however there are some custom defined types that satisfy the proto.Message interface; it is not reasonable to expect them to setter methods (or even setter methods with the same signature). One of the core issues with the proto.Message interface is that it is marginally better than interface{}. It is trivial for any user-defined type to implement proto.Message, but the problem is that the proto package (and others) have no idea how to handle these custom types. The proper approach is to extend (this is a breaking change) the proto.Message interface to include reflection information about the protocol buffer itself. We're not talking about Go reflection, but a reflection interface designed specifically for protobuf. I will file a separate issue about this.
    1a. Reflection for production code. (irrelevant to setters) While reflection is not necessarily the most performant, please don't scoff at the use of it. The proto library itself is heavily reliant on reflection to operate. That being said, Go reflection is not the correct approach to solve point 1. Again, it assumes that all protocol buffers are structs with some field of the name your interested in.
  2. Code bloat. Increased code size of protocol buffers is a real concern for mobile and applications with low memory. While the cost is not terrible (since each setter function will be the equivalent of a single conditional check and a store instruction), it is still a non-zero cost. It is not fair to compare Go programs to the "hello world" program since Go programs have a heavy initial code-size cost for the runtime and core standard library (while C programs can dynamically link with libc).
  3. Setters permit other optimizations. One use case that the current proto library does not handle well is where a service unmarshals a protobuf and only mutate a small number of fields before marshaling it again. Using settings and getters alone can allow an optimization where the unmarshaller does a form of lazy decoding and retrieves fields on demands. However, in order to make this optimization, it pre-supposes that it is not possible to access message fields as if there were just struct fields.

/cc @alandonovan, who is interested in protobuf reflection.
/cc @cherrymui, who is interested in the performance side of things.

@dsnet dsnet reopened this May 27, 2017
@dsnet
Copy link
Member

dsnet commented May 30, 2017

See #364.

@dsnet
Copy link
Member

dsnet commented Dec 23, 2017

I've been working on #364 and I am convinced that is the right solution for the root issues going on here. I understand the temptation want to use Go interfaces to set some generic field in a set of messages, but it makes the very subtle assumption that all of them will have the same method signature, which is not a world we want to back ourselves into.

@codyaray
Copy link

codyaray commented Sep 12, 2018

I ran into this issue today and wrote a little protoc generator for setters. Thought I'd share:
https://github.com/codyaray/proto-go-setter

Still a WIP. Right now it supports primitive types. Working on adding more complex types.

@RaymondReddington
Copy link

why dont you support setter.
how can i Abstract interface from many messages?
shall we write setter one by one and then Abstract the setter's interface?

@RaymondReddington
Copy link

RaymondReddington commented Nov 27, 2019

and how can i use the interface feature of golang to protobuf messages?

@puellanivis
Copy link
Collaborator

We’re reworking the way proto reflection works, and it should support better abstract interactions with messages.

But typically Go prefers less abstraction… maybe you’re trying to abstract too much?

@RaymondReddington
Copy link

RaymondReddington commented Dec 2, 2019

i just abstract one public operation of one field of ten or more structures.
and proper abstraction is the way keep code simple, and interface abstraction is the main idea of golang?
proper abstraction, i mean just make code simple or good structure, i dont mean abstract everything.

my implemetion:

rpc input -> grpc server(internal Dispatch) -> grpc server impl (my code)->grpc server impl : routine (my code, i need abstruct common api to two field from different implemetion: int success, string errorMsg. )-> acturtal implemetion
i think abstract the two field is resonable.

for rpc suitiuation, we need keep rpc api/protocal stable. and implement different backend . so we dont want to expose different api set for different backend implemention.

or i must write much if/else in routine code for keeping only one api set without Setter interface.
these if/else are lots of duplicate code. that will make routine code complex and ugly.
i must make changes to each if condition when bug fixing or updating.

@puellanivis
Copy link
Collaborator

puellanivis commented Dec 2, 2019

A central concept of golang is to not abstract things unnecessarily. In general, you should be avoiding adding layers of abstraction unless you find it extremely difficult not to. As such, each RPC should have a discrete, individual, and specific proto.Message input and output.

Having two fields success and errorMsg seems odd, considering that gRPC will pass any error from server to client. But even without using the capabilities that already exist in gRPC, repeating yourself is not always a bad thing. Being precise is far more important than building abstraction layers.

P.S.: If you have named your field errorMsg then you’re also breaking with the style guides for proto. The protobuf compiler will automatically convert your snake_case names into camelCase or CamelCase as the language style prefers: https://developers.google.com/protocol-buffers/docs/style#message-and-field-names

@golang golang locked as resolved and limited conversation to collaborators Jun 25, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

9 participants