golang / protobuf

Go support for Google's protocol buffers
BSD 3-Clause "New" or "Revised" License
9.64k stars 1.58k forks source link

text_encode.go doesn't process option `option (grpc.gateway.protoc_gen_swagger.options.openapiv2_schema)` correctly #1523

Closed baganokodo2022 closed 1 year ago

baganokodo2022 commented 1 year ago

What version of protobuf and what language are you using? Version: v1.5.2

What did you do? In one project, a simple protobuf message was created with a message-level option like this,

syntax = "proto3";

package org.order.common;

import "google/protobuf/descriptor.proto";
import "protoc-gen-swagger/options/openapiv2.proto";

message Order {
  option (grpc.gateway.protoc_gen_swagger.options.openapiv2_schema) = {
    json_schema: {
      title: "SimpleMessage"
      description: "A simple message."
      required: ["id"]
    }
  };

  string id = 1; 
}

The protoc-generated Go code was imported into a different project, which invokes a protobuf serializer in confluent-kafka-go. The serializer internally restores the protobuf text definition through reflection, then registers the canonicalized proto definition onto the Confluent Schema Registry. This real-time, dynamic protobuf schema registration is very desired to ensure schema-evolution backward compatibility in data pipelines.

In confluent-kafka-go, it makes use of this reflection lib - jhump/protoreflect to restore the protobuf text schema through FileDescriptor. During this process, I found this line in golang/protobuf was invoked when the mentioned option in handled. As a result, the generated protobuf look like this,

syntax = "proto3";

package org.order.common;

import "google/protobuf/descriptor.proto";
import "protoc-gen-swagger/options/openapiv2.proto";

message Order {
  option (grpc.gateway.protoc_gen_swagger.options.openapiv2_schema) = {
    json_schema: <
      title: "SimpleMessage"
      description: "A simple message."
      required: ["id"]
    >
  };
  string id = 1; 
}

Clearly, the angle brackets <> break the syntax of protobuf 3.0. Screenshot 2023-02-16 at 2 43 35 PM

What did you expect to see? The text_encode.go should correctly restore the protobuf schema as,

    json_schema: {
      title: "SimpleMessage"
      description: "A simple message."
      required: ["id"]
    }

What did you see instead? The text_encode.go generated the protobuf option with a broken syntax,

    json_schema: <
      title: "SimpleMessage"
      description: "A simple message."
      required: ["id"]
    >

Make sure you include information that can help us debug (full error message, exception listing, stack trace, logs).

Anything else we should know about your project / environment? mac osx 13.2.1 and ubuntu18

puellanivis commented 1 year ago

The text marshaller is not intended for the production of valid proto3 schema text definitions, but rather to provide a human-readable encoding fit for debugging purposes. As is its intended design, embedded messages are can be surrounded by < and >.

I do not see anything here that suggests a bug or unwanted behavior of the text marshaller. As such, I have to expect that the jhump/protoreflect has made some sort of assumption that isn’t holding for this corner case.

Rummaging through jhump/protoreflect I came across this: https://github.com/jhump/protoreflect/blob/0facb742c9dcbc99667de77a4b74b842f9aa0bb3/desc/protoprint/print.go#L1668-L1705 It does indeed look like it is using TextMarshaler not anticipating that submessages can be encoded with < … > rather than { … }.

In particular, the Text Format Language Specification is clear: “This format is distinct from the format of text within a .proto schema, for example.”

Since current intentions are that modifications to github.com/golang/protobuf are only to be made for essential bug fixes, and regressions, I’m not sure any proposed fix would even qualify. The code is generating valid proto text…

neild commented 1 year ago

This seems to be an issue that you should file with github.com/jhump/protoreflect, not here.

Note that github.com/golang/protobuf/proto is deprecated, and the implementation of text serialization in this package is frozen. The google.golang.org/protobuf/encoding/prototext package is the recommended implementation of the text (de)serialization.

The prototext package does not use </> brackets at this time.

baganokodo2022 commented 1 year ago

@neild and @puellanivis , thank you for your timely response. I will follow up with the owner of jhump/protoreflect.