Add support for "oneof" keyword

[localshred]

Protobuf 2.6 was released today. We should add support for oneof keyword field definitions.

Added oneofs(unions) feature. Fields in the same oneof will share memory and at most one field can be set at the same time. Use the oneof keyword to define a oneof like:

  message SampleMessage { 
    oneof test_oneof { 
      string name = 4; 
      YourMessage sub_message = 9; 
    } 
  }

[localshred]

These commits add support for the oneof keyword as a union group of fields. It provides compilation (source, compiled) and runtime behavior for the oneof feature.

• Fields compiled in a oneof group will have the :oneof => {name} option appended in the .pb.rb definition.
• Fields now have oneof? and oneof_name instance methods.
• Message classes now have set_oneof_names (dsl) and oneof_names (access) methods.
• Message instances now have a clear_oneof_group(name) method. This method is public but is also used internally when setting a field in a oneof group to clear all other fields in that oneof group before the new assignment occurs.

I've added some rough tests that should probably be filled out, particularly for each define_setter that I added the clear fields code to.

[localshred]

/cc @liveh2o @abrandoned

[liveh2o]

Looks fine to me. I don't fully understand the applications of such a field, but understanding is not a prerequisite for maintaining parity with the spec. 😉

[localshred]

I'm not in love with the current DSL, specifically the set_oneof_names class method.

package test;

message FullName {
  optional string first_name = 1;
  optional string last_name = 2;
}

message OneofTest {
  oneof SingleIdentifier {
    FullName full_name = 1;
    string email = 2;
    string username = 3;
  }
}

module Test
  class FullName < ::Protobuf::Message; end
  class OneofTest < ::Protobuf::Message; end

  class FullName
    optional :string, :first_name, 1
    optional :string, :last_name, 2
  end

  class OneofTest
    set_oneof_names :SingleIdentifier
    optional ::Test::FullName, :full_name, 1, :oneof => :SingleIdentifier
    optional :string, :email, 2, :oneof => :SingleIdentifier
    optional :string, :username, 3, :oneof => :SingleIdentifier
  end
end

Any ideas @liveh2o or look fine to you?

[localshred]

FYI added oneof support to the pygments lexer for funsies.

[zachmargolis]

@localshred @liveh2o, for the DSL, what about using a Ruby block to mimick a similar structure?

oneof :SingleIdentifier do
  optional ::Test::FullName, :full_name, 1
  optional :string, :email, 2
  optional :string, :username, 3
end

The implementation in message/fields.rb could set some state that define_field picks up?

def oneof(identifier, &block)
  @current_oneof = identifier
  yield
ensure
  @current_oneof = nil
end

def define_field(*args)
  if @current_oneof
    # set options correctly
  end
end

[pascallouisperez]

@zachmargolis are nested oneof allowed?

[zachmargolis]

@pascallouisperez AFAIK no, there are no nested oneofs allowed, at least within the same message. I Think a nested message declaration could contain its own set of oneofs, but a oneof declaration itself should not contain another oneof. That's what I get from reading Google's docs on oneof.

[liveh2o]

I like the symmetry the block DSL has with the Protocol Buffers definition, but I'm not sure it communicates intent unless it also yields something to the block. The scope of the block also doesn't affect anything.

@localshred: why do we need to capture the oneof names as well as define them in the options block? Is that part of the spec?

[localshred]

@liveh2o we need the name because we store which fields belong to the one of so that we can clear the other fields when "one of" them is set.

# using OneofTest example from above:
test = OneofTest.new
test.full_name = FullName.new(:first_name => "Billy", :last_name => "Blanks") # sets the full_name field
test.email = "billy@blanks.com" # sets email field, _and_ unsets full_name field

If we don't specify which fields the oneof type are associated with we don't know how to unset the other associated fields when one of them is set.

We could remove the set_oneof_names altogether if registering a field would store that field in a oneofs hash keyed by the oneof name on the message class.

The block form looks nicer, but since you don't hand-roll most (all?) definitions it just feels a little overkill, and doesn't actually solve the problem.

A block DSL that might work:

oneof :SingleIdentifier do |oneof_identifier|
  optional ::Test::FullName, :full_name, 1, :oneof => oneof_identifier
  optional :string, :email, 2, :oneof => oneof_identifier
  optional :string, :username, 3, :oneof => oneof_identifier
end

But, I'm not sure why at that point. I guess you could combine the set_oneof_names and block form:

oneof :SingleIdentifier, [:full_name, :email, :username] do
  optional ::Test::FullName, :full_name, 1, :oneof => oneof_identifier
  optional :string, :email, 2, :oneof => oneof_identifier
  optional :string, :username, 3, :oneof => oneof_identifier
end

This last form feels like the best of both worlds. Probably.

[liveh2o]

Solid points. I like the last idea. Perhaps we could use @zachmargolis' idea of using the block to set some context that define_field would pick up. That would clean up the DSL and allow us to omit the :oneofoption when inside a oneof block:

oneof :SingleIdentifier, [:full_name, :email, :username] do
  optional ::Test::FullName, :full_name, 1
  optional :string, :email, 2
  optional :string, :username, 3
end

The only thing that is kind of odd is that this DSL reuses the optional method to setup the field. Oneof fields are neither required nor optional (at least in the traditional Protocol Buffers sense). I'm not sure what the alternative would be, because we have to call something, even in a block.

[liveh2o]

Actually, maybe this makes more sense:

oneof :SingleIdentifier, [:full_name, :email, :username]

optional ::Test::FullName, :full_name, 1
optional :string, :email, 2
optional :string, :username, 3

This let's us define fields without any customization, but still specify which fields should be considered part of a Oneof set.

[zachmargolis]

Oh, good point, I did not realize that fields inside oneof don't have optional/required/repeated. What about a new method field that's essentially the same thing as optional, but must be inside a oneof

And I don't think we need to list the fields included in the oneof ahead of time, we can do that at the end of the block.

oneof :SingleIdentifier do
  field ::Test::FullName, :full_name, 1
  field :string, :email, 2
  field :string, :username, 3
end

def oneof(identifier, &block)
  @current_oneof_identifier = identifier
  @current_oneof_fields = Set.new
  yield
ensure
  @current_oneof_fields.each do |field|
    field.oneof_identifier = @current_oneof_identifier
    field.oneof_siblings = @current_oneof_fields - field
    # other oneof configuration
  end
  @current_oneof_identifier = nil
  @current_oneof_fields = nil
end

def field(*args)
  raise "must be inside 'oneof'" unless @current_oneof_identifier
  optional(*args)
end

def define_field(*args)
  field = # ... same as before

  if @current_oneof_identifier
    @current_oneof_fields << field
  end
end

[liveh2o]

I like that idea, but if we go that route, then I'd expect field to capture the oneof fields and define_field to be left as with no modifications. We could also call it oneof_field for more clarity:

def oneof(identifier, &block)
  @current_oneof_identifier = identifier
  @current_oneof_fields = Set.new
  yield
  @current_oneof_fields.each do |field|
    field.oneof_identifier = @current_oneof_identifier
    field.oneof_siblings = @current_oneof_fields - field
    # other oneof configuration
  end
ensure
  @current_oneof_identifier = nil
  @current_oneof_fields = nil
end

def oneof_field(*args)
  raise "must be inside 'oneof'" unless @current_oneof_identifier
  @current_oneof_fields << args.first # Or however we handle this

  optional(*args)
end

def define_field(*args)
  # ... same as before
end

[localshred]

I guess I'm struggling to understand what problem you're trying to solve. All of these solutions are aimed at removing the need for the set_oneof_names method? Gotta be honest it's not gorgeous but it's very simple and works as is. Am I missing something?

[localshred]

Ahh, now understanding that oneof fields are optional implicitly. Did not realize that. Re-analyzing my assumptions...

[liveh2o]

The spec is definitely written in a confusing way: "...at most one field can be set at the same time." That implies that none of the fields could be sent and the message would still be valid, but doesn't state it explicitly.

I originally read it as "at least one", which made it even more confusing 😄.

[localshred]

Not sure what spec you're reading, but they actually do state it explicitly in the 2nd paragraph:

Oneof fields are like optional fields except...

[liveh2o]

Well, I was selectively reading your excerpt, of course 😉.

[clarkds14]

what is the status of this PR?

[pascallouisperez]

Hi! Here at Square we'd love this to land. I'm not familiar with the code, but did the grunt work of rebasing: https://github.com/pascallouisperez/protobuf/tree/pascal/bj-pb-2.6-rebased

Can you help from there?

cc @zachmargolis,@liveh2o

[localshred]

@clarkds14 @pascallouisperez hey guys, thanks for the ping. I'm revising the DSL, almost have it updated. Should be able to merge this by the end of the week methinks. Will keep you guys posted.

[BBonifield]

@clarkds14 @pascallouisperez @localshred I'd love to see this land too. 👍 Did you come to some conclusion on the internal API?

[kmontag]

Just another friendly ping about this one. We'd love to see it at AngelList as well. Any help needed with respect to the DSL?

[localshred]

Thanks for the pings guys, should be able to merge this in tonight or tomorrow. @BBonifield @kmontag @clarkds14 @pascallouisperez

[localshred]

So, after investigating there really isn't any need for a DSL in the generated proto, the only thing you need is the :oneof option to pass to the optional DSL method.

require 'protobuf/message'

module Test
  class FullName < ::Protobuf::Message; end
  class OneofTest < ::Protobuf::Message; end

  class FullName
    optional :string, :first_name, 1
    optional :string, :last_name, 2
  end

  class OneofTest
    optional ::Test::FullName, :full_name, 1, :oneof => :SingleIdentifier
    optional :string, :email, 2, :oneof => :SingleIdentifier
    optional :string, :username, 3, :oneof => :SingleIdentifier
  end
end

[localshred]

Ready to merge if anyone wants a final pass.

[BBonifield]

[apbarrero]

Almost one year since last comment. Any plans on support for oneof?

[film42]

@apbarrero We do plan to. It might take a bit, though. If you want to use this as a template and implement it, that would be super awesome!!