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

APIv2: proto: make the Message interface behaviorally complete #364

Open
dsnet opened this Issue May 30, 2017 · 10 comments

Comments

Projects
None yet
5 participants
@dsnet
Member

dsnet commented May 30, 2017

Filing on behalf of @alandonovan.

The proto.Message interface is unsatisfactory. A behavioral interface is an abstraction over the underlying data types that exposes just the operations necessary for their correct use. By contrast, proto.Message exposes essentially no useful functionality and serves only as a marker. But a marker of what? If one could assume all its implementations were protoc-generated struct types with field tags, then at least it would be possible to write reflection-based algorithms that do useful things. However, there are many concrete types satisfying Message that are not of this form.

It's not only the set of implementations that is unbounded; the set of useful operations is also large and growing. The two most important, Marshal and Unmarshal, are handled quite cleanly since there are separate behavioral interfaces for Marshaler and Unmarshaler that allow each concrete type to implement these operations. But there are many functions in the proto API, for which no interface exists: proto.Merge, proto.Clone, the extensions API, and so on.

The cross-product of concrete implementations and operations is growing, but the fraction of these combinations that actually work is diminishing.

I think we should assess what it would take to change the proto.Message interface, and all its implementations, so that it is a true behavioral interface. This would require at a minimum that the interface include a new method that provides complete control over the abstract state of a message: accessing and updating its fields, inspecting any extensions or unrecognized fields, and so on, without revealing the concrete representation. It should be possible to implement all the major functions in the proto API, as well as most users' ad hoc functions, in terms of this interface so that they work with any concrete implementation. If an optimized version of a crucial operation is available, the generic implementation should dispatch to it, as it does today for Marshal and Unmarshal.

We can't add methods to proto.Message without breaking backwards compatibility. One approach we can take is to define proto.MessageV2 that is a much more semantically complete interface that provides a form of "protobuf reflection". In Marshal, Unmarshal, Merge, Clone, and so on, we can type assert if it implements proto.MessageV2 and use that interface to implement generic versions of those functions. If proto.Message doesn't satisfy proto.MessageV2, then Merge can just fail (it already does on most third-party implementations of proto.Message).

@dsnet dsnet self-assigned this May 30, 2017

@dsnet dsnet added the enhancement label May 30, 2017

@jhump

This comment has been minimized.

Contributor

jhump commented Jun 27, 2017

Interesting proposal. I faced a similar challenge when designing the public API for a dynamic message implementation.

If this goes anywhere, I'll be curious to see how similar they are in terms of shape/surface area. And I'll be excited to see how it can simplify that dynamic message's implementation (particularly around extensions, which I think is the weakest part of the current generated code).

@dsnet dsnet changed the title from Make the Message interface behaviorally complete to proto: make the Message interface behaviorally complete Mar 8, 2018

@xfxyjwf

This comment has been minimized.

Contributor

xfxyjwf commented Mar 22, 2018

About adding methods to proto Message interface, we have done exactly that a few times in both C++ and Java. For example, we added a ByteSizeLong method to C++ MessageLite interface not very long ago:
https://github.com/google/protobuf/blob/master/src/google/protobuf/message_lite.h#L339

It's a pure virtual method so anyone who implements their own MessageLite subclasses will be broken.

Protobuf team's stance on this is that nobody except protobuf itself should subclass/implement these message interfaces. It's called out explicitly in our compatibility notice in Java:
https://github.com/google/protobuf/tree/master/java#compatibility-notice

Protobuf message interfaces/classes are designed to be subclassed by protobuf generated code only. Do not subclass these message interfaces/classes yourself. We may add new methods to the message interfaces/classes which will break your own subclasses.

@dsnet

This comment has been minimized.

Member

dsnet commented Mar 22, 2018

Go doesn't have the concept of a "default method", so this is going to be a breaking change unfortunately. The transition to get the world to the new API will be a little tricky and will probably have to occur in several phases.

@dsnet

This comment has been minimized.

Member

dsnet commented Mar 22, 2018

There's an open proposal for default methods golang/go#23185, but it's not looking promising as the concept doesn't fit well into Go where interface satisfaction is implicit rather than explicit.

@dsnet

This comment has been minimized.

Member

dsnet commented Apr 25, 2018

Here are some documents for the proposed plans to improve Go protobufs. I'd appreciate any feedback from the community regarding the design and migration plan:

\cc @neild @jhump @awalterschulze @tamird @meling

@luna-duclos

This comment has been minimized.

luna-duclos commented Apr 25, 2018

Couldn't the same approach to the database/sql package be taken ? Where various functions run specific assertions on smaller interfaces, rather than having a giant V1/V2 interface where both have all the functions defined in one big bundle.

Edit: This already seems the case in the new proposal, apologies for skimming over it too quickly

@awalterschulze

This comment has been minimized.

awalterschulze commented Apr 26, 2018

Both documents are really ambitious, but in principal sound like a great idea. Good work.

I am a bit concerned about the amount of work required to make this move on the part of gogoprotobuf. I will need some help. The last large move (currently on the dev branch) was a big job, taking ALOT of my personal time, which I am trying to focus om my studies. I am not working for a company that uses protobufs.
But I want to stress that I do think these changes are necessary. So I will do my best to support these efforts.

Comments on protoreflect. It looks good, but I am a bit concerned, that the following cases might not have been taken into account. So maybe here are some tests for the protoreflect API:

  • protobuf field type is bytes, but user has customized it to their own customtype Uuid
  • protobuf field type is repeated message, but user has customized it to []MyMessage, without a pointer field.
  • protobuf field type is timestamp, but user has customized it to time.Time or *time.Time or []time.Time or []*time.Time

I think getting this protoreflect library right is essential to having only a single proto library, which is the goal I am trying my best to support.

@dsnet

This comment has been minimized.

Member

dsnet commented Apr 26, 2018

Thank you @awalterschulze for your feedback.

Both documents are really ambitious

I admit that the designs are indeed ambitious. Ideally, it would be nice if something like it occurred 8 years ago when Go protobufs were first implemented. However, that was not the case and we are struggling with the consequences today. As much work as this transition is, it will lay the foundation for a brighter future. Another 8 years from now, I hope we look back and see this moment as the point when Go protobuf support became much better.

I will need some help.

I'll help 👋.

Regarding your protoreflect concerns, I was consciously thinking about how gogo/protobuf fit into it all as I was designing the reflection API.

How do we handle slices of message values (not pointers)?

This case is actually the reason why protoreflect.KnownFields.Set has this documented restriction:

It is unspecified whether Set performs a pointer copy or copy of the pointed at value for composite types (e.g., Messages, Vectors, Maps).

Thus, for []T where T is a value type, the reflection implementation only need to copy the pointed-at value.

The Mutable methods are also implementable since every element in a Go slice is addressable, so the reflection implementation can obtain an address for an element and return it.

How do we handle message values (not pointers)?

This situation is different from above since proto semantics do not indicate the difference between a null or empty message within a repeated list of messages. However, for a standalone message, proto semantics do preserve whether a message is null or empty (even in proto3).

Message values occurs when the nullable option is used. In this situation, proto semantics are being violated. Hence the documented warning that:

Warning about nullable: according to the Protocol Buffer specification, you should be able to tell whether a field is set or unset. With the option nullable=false this feature is lost, since your non-nullable fields will always be set.

An implementation of the reflection API will need to do a best effort at providing the illusion that the implementation is proto compliant.

However, this abstraction can leak:

  • Explicitly clearing a non-nullable field means that the message contents are cleared, but does not mean that the message itself is cleared.
  • Calling protoreflect.KnownFields.List on new instance of a message will return a non-empty list since some non-nullable sub-messages are always considered set.

How do we handle custom types for bytes?

Similar to the documented restriction on protoreflect.KnownFields.Set, I'll update the document such that it is unspecified whether protoreflect.ValueOf([]byte(...)) or protoreflect.Value.Bytes results in a slice that is aliased or copied.

How do we handle Timestamp or Duration proto represented as a Go Time or Duration?

An implementation of the reflection API will need to create internal wrappers over time.Time and time.Duration that presents the illusion that they are just messages with a seconds or nanos field.

One possible implementation is here: https://play.golang.org/p/IXvjCK_Y_Hc

However, these wrappers are leaky abstractions:

  • Timestamp.SetNanos with nsec outside the range of [0, 999999999] changes the value of Seconds
    and returns a Nanos that does not the same value as the original nsec.
    However, note that the documentation on Timestamp.nanos says that nanos must always be
    within the range of [0, 999999999]
    , but this is a documentation restriction,
    not semantic restriction.
  • Duration.SetNanos with nsec outside the range of [-999999999, +999999999] changes the value of Seconds
    and returns a Nanos that does not the same value as the original nsec.
    However, note that the documentation on Duration.nanos says that nanos must always be
    within the range of [-999999999, +999999999]
    , but this is a documentation restriction,
    not semantic restriction.
  • Calling Duration.SetSeconds with a sec value that has a different sign than Nanos,
    results in Nanos being negated and
    calling Duration.SetNanos with a nsec value that has a different sign than Seconds,
    results in Seconds being negated.
    In practice, this is not an issue because SetSeconds and SetNanos are almost always
    set together (with the correct signs) without an intermediate get on Seconds or Nanos.
  • The google.protobuf.Duration type is documented as being able to represent ±10000 years,
    which is beyond the range Go's time.Duration type, which can only represent ±290 years.
    Thus, calling SetSeconds with a sec value beyond Go's limit results in a silent truncation.
  • Duration has a similar non-nullable issues as non-nullable messages unless a *time.Duration was used. Note that time.Time does not have this issue since the zero value time.Time can represent being null. However, when the Mutable method is called to retrieve a time.Time, the timestamp must be initialized as Unix(0, 0), which is the zero value of the proto Timestamp message.

In practice, I don't expect the abstraction leakages mentioned above to be much of a problem in practice. If anything the overflow of time.Duration may be the most likely candidate, and the wrapper could help alleviate that by setting the time.Duration to time.Duration(math.MaxInt64) to help signal that failure scenario.

@awalterschulze

This comment has been minimized.

awalterschulze commented Apr 29, 2018

I agree on the necessity and if you are willing to make contributions to the gogoprotobuf repo to bring the two implementations closer, to the point of eliminating the need for two proto libraries, then let's do this :)

On customtype, this was introduced first and the more sustainable extension casttype was only introduced later. customtype only assumes that you have implemented some methods, like Marshal, Unmarshal, Size, etc. It does not necessarily have to be an alias, as in the case of casttype. I hope we can still support this?

If we can hide the timestamp/duration wrappers from the user, then its fine, but I would hate for them to have to import something outside of the standard library to have a time.Time field. It is very important that the generated fields are the ones users want.

And yes, nullable=false and time.Duration are subtle leaky abstractions, but again it is what users want. If they were going to write a function to copy between the structs they want to use and the generated ones, then they were going to run into these subtletees anyway. gogoprotobuf prefers to handle these in a single sensible way over having users write error prone and slow copy functions.

Although these comments sound negative, these are really the only problems I can see. I think we can get there and I appreciate all your efforts here, including taking gogoprotobuf into account.

@dsnet

This comment has been minimized.

Member

dsnet commented Aug 2, 2018

For anyone subscribed. Work is actively being done to address this issue. See the "api-v2" branch on this GitHub repo.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment