Skip to content

Commit

Permalink
Fix potential clash with reserved keywords
Browse files Browse the repository at this point in the history
Note:
* This also changes the constructor semantics
  when multiple oneof-values are given (now the *first*
  value is kept instead of the last)

* Using reserved keywords as protobuf field-names remains
  a bad idea regardless. For example a field called `delete`
  will break ts-proto / protoc-gen-js and probably others
  • Loading branch information
m-o-e committed Jul 17, 2021
1 parent 7587e68 commit a4b6860
Show file tree
Hide file tree
Showing 5 changed files with 23 additions and 20 deletions.
4 changes: 1 addition & 3 deletions spec/fixtures/test.pb.cr
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,7 @@ struct Test
required :sint64, :sint64, 13
required :bool, :bool, 14

# "enum" is a reserved keyword in
# crystal so we use "my_enum" for this one
repeated :my_enum, SomeEnum, 15
repeated :enum, SomeEnum, 15

required :fixed64, :fixed64, 16
required :sfixed64, :sfixed64, 17
Expand Down
2 changes: 1 addition & 1 deletion spec/fixtures/test3.pb.cr
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ module TestMessagesV3
optional :sint32, :sint32, 12
optional :sint64, :sint64, 13
optional :bool_e, :bool, 14
repeated :my_enum, SomeEnum, 15
repeated :enum, SomeEnum, 15
optional :fixed64, :fixed64, 16
optional :sfixed64, :sfixed64, 17
optional :double, :double, 18
Expand Down
26 changes: 13 additions & 13 deletions spec/protobuf/message_spec.cr
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ describe Protobuf::Message do
test.sint64.should eq(-9223372036854775808)
test.bool.should eq true

test.my_enum.should eq [SomeEnum::YES, SomeEnum::NO, SomeEnum::NEVER]
test.enum.should eq [SomeEnum::YES, SomeEnum::NO, SomeEnum::NEVER]

test.fixed64.should eq 4294967296
test.sfixed64.should eq 2147483648
Expand Down Expand Up @@ -62,7 +62,7 @@ describe Protobuf::Message do

it "handles oneof in proto2" do
# When multiple oneof values are populated
# by the ctor only the last survives
# by the ctor only the first survives
msg = TestMessagesProto2::TestOneof1.new(
a: 1,
oo0_a: 2, oo0_b: 3, oo0_c: 4,
Expand All @@ -71,23 +71,23 @@ describe Protobuf::Message do
)

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, nil, 4, 5, nil, nil, 8])
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_c")
msg.oo1.should eq("oo1_c")
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, nil, nil, 4, 5, nil, nil, 8])
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, nil, nil, 8])
payload.should eq([1, nil, 3, nil, 5, 6, nil, nil])
msg.oo0.should eq("oo0_b")

msg.oo1_b = 7
Expand All @@ -99,7 +99,7 @@ describe Protobuf::Message do

it "handles oneof in proto3" do
# When multiple oneof values are populated
# by the ctor only the last survives
# by the ctor only the first survives
msg = TestMessagesProto3::TestOneof1.new(
a: 1,
oo0_a: 2, oo0_b: 3, oo0_c: 4,
Expand All @@ -108,23 +108,23 @@ describe Protobuf::Message do
)

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, nil, 4, 5, nil, nil, 8])
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_c")
msg.oo1.should eq("oo1_c")
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, nil, nil, 4, 5, nil, nil, 8])
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, nil, nil, 8])
payload.should eq([1, nil, 3, nil, 5, 6, nil, nil])
msg.oo0.should eq("oo0_b")

msg.oo1_b = 7
Expand Down
9 changes: 7 additions & 2 deletions src/protobuf/generator.cr
Original file line number Diff line number Diff line change
Expand Up @@ -255,6 +255,11 @@ end

module Protobuf
class Generator
# Turns `foo/bar/batz.proto` into `foo_bar_batz.pb.cr`
def self.output_filename(path)
path.split(".")[0..-2].join("").gsub(File::SEPARATOR, "_") + ".pb.cr"
end

def self.compile(req)
raise Error.new("no files to generate") if req.proto_file.nil?
package_map = {} of String => String
Expand All @@ -266,7 +271,7 @@ module Protobuf
files = req.proto_file.not_nil!.map do |file|
generator = new(file, package_map)
CodeGeneratorResponse::File.new(
name: File.basename(file.name.not_nil!, ".proto") + ".pb.cr",
name: output_filename(file.name.not_nil!),
content: generator.compile
)
end
Expand All @@ -291,7 +296,7 @@ module Protobuf
puts nil

unless @file.dependency.nil?
@file.dependency.not_nil!.each { |dp| puts "require \"./#{File.basename(dp.not_nil!, ".proto") + ".pb.cr"}\"" }
@file.dependency.not_nil!.each { |dp| puts "require \"./#{self.class.output_filename(dp.not_nil!)}\"" }
puts nil
end

Expand Down
2 changes: 1 addition & 1 deletion src/protobuf/message.cr
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,7 @@ module Protobuf
{% end %}
)
{% for tag, field in FIELDS %}
self.{{field[:name].id}} = {{field[:name].id}}
self.{{field[:name].id}} = @{{field[:name].id}}
{% end %}
end
end
Expand Down

0 comments on commit a4b6860

Please sign in to comment.