ruby-protobuf / protobuf

A pure ruby implementation of Google's Protocol Buffers
https://github.com/ruby-protobuf
MIT License
462 stars 101 forks source link

Add support for v3 protos with optional fields #421

Closed film42 closed 3 years ago

film42 commented 3 years ago

This PR adds support for compiling v3 proto definitions with optional fields. The tl;dr is that optional fields are nullable fields. It should be wire compatible with v2 optional fields. Can a reviewer double-check me here? Here's the google reference: https://chromium.googlesource.com/external/github.com/protocolbuffers/protobuf/+/refs/heads/master/docs/implementing_proto3_presence.md

How do we support v3 protos with optional fields? Turns out it's pretty easy. Mainline protobuf added a new field on plugin.proto that communicates with features are enabled. Currently there's only one feature, v3 protos with optional fields, but this should be pretty easy to modify going forward.

Most of the diff is getting bx rake to rebuild proto files in spec/support without silently failing. I re-wrote rake to fail when a proto command fails. I also added some pb.rb definitions that were being generated but were not committed. At this point, running bx rake should do what you expect: recompile support definitions, run rspec, and then run rubocop. (Update: this has been applied in a different PR).

At MX we have a few golang services that are compiling proto3 definitions that need nullable fields. In order to share definitions, we need to add support for this new v3 optional proto field feature in ruby-protobuf. Since ruby-protobuf with v3 seems to work and implicitly makes all v3 proto fields optional, this change should be harmless. But again, please check me on this.

@abrandoned @quixoten

ETetzlaff commented 3 years ago

LGTM

film42 commented 3 years ago
syntax = "proto3";

message Inner {
  // assigned
  optional string yolo = 1;
  string brolo = 2;

  //zeros
  optional string yolo_null = 3;
  string brolo_zero = 4;
}

message Msg {
  // assigned
  optional string yolo = 1;
  string brolo = 2;

  //zeros
  optional string yolo_null = 3;
  string brolo_zero = 4;

  optional Inner inner_opt = 5;
  Inner inner_zero = 6;
}

This generated the following ruby proto dsl:

# encoding: utf-8

##
# This file is auto-generated. DO NOT EDIT!
#
require 'protobuf'

##
# Message Classes
#
class Inner < ::Protobuf::Message; end
class Msg < ::Protobuf::Message; end

##
# Message Fields
#
class Inner
  optional :string, :yolo, 1
  optional :string, :brolo, 2
  optional :string, :yolo_null, 3
  optional :string, :brolo_zero, 4
end

class Msg
  optional :string, :yolo, 1
  optional :string, :brolo, 2
  optional :string, :yolo_null, 3
  optional :string, :brolo_zero, 4
  optional ::Inner, :inner_opt, 5
  optional ::Inner, :inner_zero, 6
end

I generated a v3 golang pb impl of the same file and generated an encoded v3 proto blob using the following:

    s := "YOLO!"
    m := new(Msg)
    m.Yolo = &s
    m.Brolo = s

    i := new(Inner)
    i.Yolo = &s
    i.Brolo = s

    m.InnerOpt = i

    bytes, err := proto.Marshal(m)
    if err != nil {
        panic(err)
    }

    fmt.Println(base64.URLEncoding.EncodeToString(bytes))

And then taking the output of that and piping it into the V2 definition decode and encode produced the same result as the original wire message.

$ cat assert.rb
require "protobuf"
require "base64"
load "./example.pb.rb"

payload = STDIN.read.strip

x = Base64.urlsafe_decode64(payload)
puts "GO:   " + Base64.urlsafe_encode64(x)
m = Msg.decode(x)
puts "RUBY: " + Base64.urlsafe_encode64(m.encode)

$ echo "CgVZT0xPIRIFWU9MTyEqDgoFWU9MTyESBVlPTE8h" | bx ruby -I ../lib ./assert.rb
GO:   CgVZT0xPIRIFWU9MTyEqDgoFWU9MTyESBVlPTE8h
RUBY: CgVZT0xPIRIFWU9MTyEqDgoFWU9MTyESBVlPTE8h

Which demonstrates that the V3 optional and zero-value fields encode and decode the same as a V2 proto. This might not be true for other fields, but at least for the string field and nested-object case, it seems to work exactly the same.

I would like a bit more confidence here, but I think this is safe to merge.