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.

Sign up for GitHub

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

Jump to bottom

Improve oneof support #40

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

Improve oneof support #40

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
82b2f40
Improve `oneof` support
m-o-e Jul 16, 2021
7587e68
Fix comment
m-o-e Jul 16, 2021
8035bd5
Fix potential clash with reserved keywords
m-o-e Jul 17, 2021
53df591
Avoid double-negative
m-o-e Jul 17, 2021
0c22b97
DRY the specs (unify the duplicated tests)
m-o-e Jul 17, 2021
4e8c566
Call setters after deserializing
m-o-e Jul 17, 2021
ca66c71
Re-add oneof! invocation (had gotten lost in merge mixup)
m-o-e Jul 17, 2021
83296aa
Add direct getter for oneof value
m-o-e Jul 23, 2021
93f3afc
Restrict return type of oneof-by-name getter
m-o-e Jul 24, 2021
8ae57a8
Fix return type restriction (String/Symbol mixup)
m-o-e Jul 24, 2021
2f015e4
Add *experimental* support for proto3 `optional` (Field Presence)
m-o-e Jul 26, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 25 additions & 0 deletions spec/fixtures/generated_proto2/TestOneofMessage.pb.cr
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
#require "protobuf"

module TestMessagesProto2

struct TestOneof1
include Protobuf::Message

contract do
optional :a, :int32, 1

optional :oo0_a, :int32, 2, oneof_index: 0
optional :oo0_b, :int32, 3, oneof_index: 0
optional :oo0_c, :int32, 4, oneof_index: 0

optional :b, :int32, 5

optional :oo1_a, :int32, 6, oneof_index: 1
optional :oo1_b, :int32, 7, oneof_index: 1
optional :oo1_c, :int32, 8, oneof_index: 1

oneof 0, "oo0"
oneof 1, "oo1"
end
end
end
Comment on lines +1 to +25
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like you may have forgotten to commit the TestOneofMessage.proto here. That'd be helpful to understand what all of the oo0_* properties come from. 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, sorry, I wrote that above file by hand, didn't use an input proto.
(the layout of the fixtures wasn't entirely clear to me so I just did what worked 😅)

The corresponding proto file would look like this:

syntax = "proto3";

message TestOneof1 {
  int32 a = 1;
  
  oneof oo0 {
     int32 oo0_a = 2;
     int32 oo0_b = 3;
     int32 oo0_c = 4;
  }
  
  int32 b = 5;

  oneof oo1 {
     int32 oo1_a = 6;
     int32 oo1_b = 7;
     int32 oo1_c = 8;
  }
}

The goal was to have more than one oneof and have it
interleaved with other fields for a meaningful test.

Lemme know if I should add that proto to the repo somewhere
(perhaps as a comment on top of TestOneofMessage.pb.cr? 🤔).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally, we'd have it there to verify that the generator generates working Crystal code.

Copy link
Contributor Author

@m-o-e m-o-e Jul 17, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well spec/protobuf/generator_spec.cr is empty. 😬
I wasn't sure how these existing fixture files were generated, so I just put mine next to them.

I don't think there's currently anything in the specs that actually invokes the generator,
so not sure where I should put that above proto source. Just drop it into spec/fixtures/?

25 changes: 25 additions & 0 deletions spec/fixtures/generated_proto3/TestOneofMessage.pb.cr
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
#require "protobuf"

module TestMessagesProto3

struct TestOneof1
include Protobuf::Message

contract do
optional :a, :int32, 1

optional :oo0_a, :int32, 2, oneof_index: 0
optional :oo0_b, :int32, 3, oneof_index: 0
optional :oo0_c, :int32, 4, oneof_index: 0

optional :b, :int32, 5

optional :oo1_a, :int32, 6, oneof_index: 1
optional :oo1_b, :int32, 7, oneof_index: 1
optional :oo1_c, :int32, 8, oneof_index: 1

oneof 0, "oo0"
oneof 1, "oo1"
end
end
end
75 changes: 75 additions & 0 deletions spec/protobuf/message_spec.cr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,4 +59,79 @@ describe Protobuf::Message do
test4 = TestMessagesProto2::Test4.new(d: [] of Int32)
test4.to_protobuf.empty?.should be_true
end

it "handles oneof in proto2" do
# When multiple oneof values are populated
# by the ctor only the first survives
msg = TestMessagesProto2::TestOneof1.new(
a: 1,
oo0_a: 2, oo0_b: 3, oo0_c: 4,
b: 5,
oo1_a: 6, oo1_b: 7, oo1_c: 8
)

payload = [ msg.a, msg.oo0_a, msg.oo0_b, msg.oo0_c, msg.b, msg.oo1_a, msg.oo1_b, msg.oo1_c ]
payload.should eq([1, 2, nil, nil, 5, 6, nil, nil])

# `msg.foo` tells us the string name
# of the member `foo` that is set
msg.oo0.should eq("oo0_a")
msg.oo1.should eq("oo1_a")

# Setting values outside of oneof's leaves the oneof's untouched
msg.a = 1
msg.b = 5
payload = [ msg.a, msg.oo0_a, msg.oo0_b, msg.oo0_c, msg.b, msg.oo1_a, msg.oo1_b, msg.oo1_c ]
payload.should eq([1, 2, nil, nil, 5, 6, nil, nil])

# Setting a oneof value resets the others in the same oneof
msg.oo0_b = 3
payload = [ msg.a, msg.oo0_a, msg.oo0_b, msg.oo0_c, msg.b, msg.oo1_a, msg.oo1_b, msg.oo1_c ]
payload.should eq([1, nil, 3, nil, 5, 6, nil, nil])
msg.oo0.should eq("oo0_b")

msg.oo1_b = 7
payload = [ msg.a, msg.oo0_a, msg.oo0_b, msg.oo0_c, msg.b, msg.oo1_a, msg.oo1_b, msg.oo1_c ]
payload.should eq([1, nil, 3, nil, 5, nil, 7, nil])
msg.oo0.should eq("oo0_b")
msg.oo1.should eq("oo1_b")
end

it "handles oneof in proto3" do
# When multiple oneof values are populated
# by the ctor only the first survives
msg = TestMessagesProto3::TestOneof1.new(
a: 1,
oo0_a: 2, oo0_b: 3, oo0_c: 4,
b: 5,
oo1_a: 6, oo1_b: 7, oo1_c: 8
)

payload = [ msg.a, msg.oo0_a, msg.oo0_b, msg.oo0_c, msg.b, msg.oo1_a, msg.oo1_b, msg.oo1_c ]
payload.should eq([1, 2, nil, nil, 5, 6, nil, nil])

# `msg.foo` tells us the string name
# of the member `foo` that is set
msg.oo0.should eq("oo0_a")
msg.oo1.should eq("oo1_a")

# Setting values outside of oneof's leaves the oneof's untouched
msg.a = 1
msg.b = 5
payload = [ msg.a, msg.oo0_a, msg.oo0_b, msg.oo0_c, msg.b, msg.oo1_a, msg.oo1_b, msg.oo1_c ]
payload.should eq([1, 2, nil, nil, 5, 6, nil, nil])

# Setting a oneof value resets the others in the same oneof
msg.oo0_b = 3
payload = [ msg.a, msg.oo0_a, msg.oo0_b, msg.oo0_c, msg.b, msg.oo1_a, msg.oo1_b, msg.oo1_c ]
payload.should eq([1, nil, 3, nil, 5, 6, nil, nil])
msg.oo0.should eq("oo0_b")

msg.oo1_b = 7
payload = [ msg.a, msg.oo0_a, msg.oo0_b, msg.oo0_c, msg.b, msg.oo1_a, msg.oo1_b, msg.oo1_c ]
payload.should eq([1, nil, 3, nil, 5, nil, 7, nil])
msg.oo0.should eq("oo0_b")
msg.oo1.should eq("oo1_b")
end

end
17 changes: 17 additions & 0 deletions src/protobuf/generator.cr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,9 @@ module Protobuf
optional :default_value, :string, 7

optional :options, FieldOptions, 8

## For oneof members contains the index of the oneof
optional :oneof_index, :int32, 9
end
end

Expand All @@ -84,6 +87,14 @@ module Protobuf
end
end

struct OneofDescriptorProto
include Protobuf::Message

contract do
optional :name, :string, 1
end
end

struct EnumValueDescriptorProto
include Protobuf::Message

Expand Down Expand Up @@ -184,6 +195,7 @@ module Protobuf
repeated :extended, CodeGeneratorRequest::FieldDescriptorProto, 6
repeated :nested_type, CodeGeneratorRequest::DescriptorProto, 3
repeated :enum_type, CodeGeneratorRequest::EnumDescriptorProto, 4
repeated :oneof_decl, CodeGeneratorRequest::OneofDescriptorProto, 8
end
end

Expand Down Expand Up @@ -340,6 +352,10 @@ module Protobuf
puts "end"
end

def oneof!(oneof_decl, index)
puts "oneof #{index}, \"#{oneof_decl.name}\""
end

def field!(field, syntax)
met = case field.label
when CodeGeneratorRequest::FieldDescriptorProto::Label::LABEL_OPTIONAL
Expand Down Expand Up @@ -396,6 +412,7 @@ module Protobuf
field_desc += ", packed: true" if field.options.not_nil!.packed
end
end
field_desc += ", oneof_index: #{field.oneof_index}" if field.oneof_index
puts field_desc
end

Expand Down
44 changes: 38 additions & 6 deletions src/protobuf/message.cr
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
module Protobuf
module Message

macro contract_of (syntax, &blk)
FIELDS = {} of Int32 => HashLiteral(Symbol, ASTNode)
ONEOFS = {} of Int32 => String?
{{yield}}
_generate_decoder {{syntax}}
_generate_encoder {{syntax}}
Expand All @@ -14,7 +14,7 @@ module Protobuf
contract_of "proto2" {{blk}}
end

macro _add_field(tag, name, pb_type, options = {} of Symbol => Bool)
macro _add_field(tag, name, pb_type, options = {} of Symbol => Bool, oneof_index = nil)
{%
t = ::Protobuf::PB_TYPE_MAP[pb_type] || pb_type
FIELDS[tag] = {
Expand All @@ -27,22 +27,33 @@ module Protobuf
repeated: !!options[:repeated],
default: options[:default],
packed: !!options[:packed],
oneof_index: oneof_index
}
%}
end

macro optional(name, type, tag, default = nil, repeated = false, packed = false)
_add_field({{tag.id}}, {{name}}, {{type}}, {optional: true, default: {{default}}, repeated: {{repeated}}, packed: {{packed}}})
macro optional(name, type, tag, default = nil, repeated = false, packed = false, oneof_index = nil)
_add_field({{tag.id}}, {{name}}, {{type}},
{optional: true, default: {{default}}, repeated: {{repeated}}, packed: {{packed}}}, oneof_index: {{oneof_index}})
end

macro required(name, type, tag, default = nil)
_add_field({{tag.id}}, {{name}}, {{type}}, {default: {{default}}})
macro required(name, type, tag, default = nil, oneof_index = nil)
_add_field({{tag.id}}, {{name}}, {{type}}, {default: {{default}}}, {{oneof_index}})
end

macro repeated(name, type, tag, packed = false)
optional({{name}}, {{type}}, {{tag}}, nil, true, {{packed}})
end

macro oneof(index, name)
{% ONEOFS[index] = name %}

@{{name.id}} : String?
def {{name.id}}
@{{name.id}}
end
end

macro extensions(range)
# puts "extensions: {{range.id}}"
end
Expand Down Expand Up @@ -140,6 +151,9 @@ module Protobuf
{% end %}
{% end %}
)
{% for tag, field in FIELDS %}
self.{{field[:name].id}} = @{{field[:name].id}}
{% end %}
end
end

Expand Down Expand Up @@ -192,6 +206,24 @@ module Protobuf
macro _generate_getters_setters
{% for tag, field in FIELDS %}
property {{field[:name].id}} : {{field[:cast_type]}}
def {{field[:name].id}}=(value : {{field[:cast_type]}})
@{{field[:name].id}} = value

# If this field is a member of a oneof..
{% unless field[:oneof_index] == nil %}
unless value.nil?
# ..we set the oneof to the name of the this field..
@{{ONEOFS[field[:oneof_index]].id}} = {{field[:name].id.stringify}}
# ..and clear the other members of the oneof
{% for neighbor_tag, neighbor_field in FIELDS %}
{% if neighbor_tag != tag && neighbor_field[:oneof_index] == field[:oneof_index] %}
@{{neighbor_field[:name].id}} = {{neighbor_field[:default]}}
{% end %}
{% end %}
end
{% end %}

end
{% end %}
end

Expand Down