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.

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

Custom options are omitted from image json #50

Closed
ChrisPenner opened this issue Apr 22, 2020 · 8 comments · Fixed by #87
Closed

Custom options are omitted from image json #50

ChrisPenner opened this issue Apr 22, 2020 · 8 comments · Fixed by #87
Labels
Feature New feature or request

Comments

@ChrisPenner
Copy link

Hey folks! Really love the tool; I appreciate the json output as it's much easier to work with than the proto descriptors.

Unfortunately, buf seems to be omitting custom options from rpcs; for example, the following proto definition:

syntax = "proto3";

package test;

import "google/protobuf/descriptor.proto";

message TestRequest { }
message TestResponse { }

message MyOption {
  string setting = 1;
}

// Test Service
service TestService {
  rpc TestMethod(TestRequest) returns (TestResponse) {
    option (myoption) = {
      setting: "something"
    };
  };
}

extend google.protobuf.MethodOptions {
    MyOption myoption = 50000;
}

Results in an image containing this:

    {
      "name": "test.proto",
      "package": "test",
      "dependency": [
        "google/protobuf/descriptor.proto"
      ],
      "messageType": [
        {
          "name": "TestRequest"
        },
        {
          "name": "TestResponse"
        },
        {
          "name": "MyOption",
          "field": [
            {
              "name": "setting",
              "number": 1,
              "label": "LABEL_OPTIONAL",
              "type": "TYPE_STRING",
              "jsonName": "setting"
            }
          ]
        }
      ],
      "service": [
        {
          "name": "TestService",
          "method": [
            {
              "name": "TestMethod",
              "inputType": ".test.TestRequest",
              "outputType": ".test.TestResponse",
              "options": {}
            }
          ]
        }
      ],
      "extension": [
        {
          "name": "myoption",
          "number": 50000,
          "label": "LABEL_OPTIONAL",
          "type": "TYPE_MESSAGE",
          "typeName": ".test.MyOption",
          "extendee": ".google.protobuf.MethodOptions",
          "jsonName": "myoption"
        }
      ]
  }

Notice that it recognizes the extension, but it's not included in the rpc's options object, and indeed the string "something" doesn't appear in the image.json anywhere at all.

Is this expected behaviour? It'd be really nice to have these options captured in the image output in one way or another, since I need to run some checks and analysis on these options in my project.

Thanks for your help and all the work you do!

@bufdev bufdev added Bug Something isn't working relies on golang/protobuf v2 labels May 9, 2020
@bufdev
Copy link
Member

bufdev commented May 10, 2020

Hey,

JSON extensions are explicitly impossible:

https://github.com/protocolbuffers/protobuf/blob/master/java/util/src/main/java/com/google/protobuf/util/JsonFormat.java#L92

/**
 * Utility classes to convert protobuf messages to/from JSON format. The JSON
 * format follows Proto3 JSON specification and only proto3 features are
 * supported. Proto2 only features (e.g., extensions and unknown fields) will
 * be discarded in the conversion. That is, when converting proto2 messages
 * to JSON format, extensions and unknown fields will be treated as if they
 * do not exist. This applies to proto2 messages embedded in proto3 messages
 * as well.
 */

We'll keep this issue open unitl we update the documentation.

In general, as we said, it's much better to use the wire format than the JSON format when storing Protobuf messages. We don't do any comparisons with custom options for breaking change detection, so it is fine to store JSON messages to use in the breaking change detector, but for other use we would recommend you use the wire format.

@ChrisPenner
Copy link
Author

ChrisPenner commented May 10, 2020

I think I might be misunderstanding a bit, the official docs say that extensions are explicitly still supported for the purpose of custom options in proto3 (even though that's the only place they're valid). https://developers.google.com/protocol-buffers/docs/proto3#custom_options

Is that not the case?

it's much better to use the wire format than the JSON format when storing Protobuf messages

I'm not actually storing messages, I'm attempting to run custom linting rules on custom options, I was under the impression buf is supposed to help make linting easier, but perhaps I misunderstand buf's goals.

Additional Context from the protocol buffers repo: protocolbuffers/protobuf#1460

@bufdev
Copy link
Member

bufdev commented May 10, 2020

Extensions are supported for custom options for proto3, but are not encoded in JSON - the above link explains this, protobuf’s JSON encoders (which we do not maintain) do not support it.

We don’t explicitly support custom lint rules on purpose, but of course you are able to build off of our FileDescriptorSet output. However, protobuf does not appear to support custom option encoding in JSON.

@mgutz
Copy link

mgutz commented Jun 20, 2020

That is unfortunate. I'm using buf's JSON output to do code generation and the missing options details is a show stopper. The alternative is to use protobufjs/protobuf.js project's pbjs utility which emits options information.

@bufdev
Copy link
Member

bufdev commented Jun 20, 2020

We are considering supporting this, stay tuned

@bufdev bufdev added Feature New feature or request and removed documentation labels Jun 21, 2020
@bufdev
Copy link
Member

bufdev commented Jun 21, 2020

Update: it turns out this is possible, and we have code that handles this situation. We'll be adding custom options marshalling for JSON in the near future, and the ability to convert to/from JSON/binary images.

@bufdev
Copy link
Member

bufdev commented Jun 22, 2020

See #87

@bufdev
Copy link
Member

bufdev commented Jun 22, 2020

Released as 0.18.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants