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

pbf ext.json field not mapped into the json_name field property #162

Closed
paulbalomiri opened this issue Jul 10, 2021 · 3 comments
Closed

Comments

@paulbalomiri
Copy link

paulbalomiri commented Jul 10, 2021

Some context info first:

This is a feature request (if it's not as simple as being a bug), i'd like to see in the lib.

I'm trying to bind the current Syncthing rest api, which serves according to pbuf proto spec contained in this folder of the Syncthing source repo.

The lib i've open sourced is here syncthex.

the proto definitions contain the ext.json extension variables for the json encoding, which is used to serve the Rest endpoints. The spec files look like this:

message Configuration {
    int32                        version         = 1 [(ext.xml) = "version,attr"];
    repeated FolderConfiguration folders         = 2;
    repeated DeviceConfiguration devices         = 3;
    GUIConfiguration             gui             = 4 [(ext.goname) = "GUI"];
    LDAPConfiguration            ldap            = 5 [(ext.goname) = "LDAP"];
    OptionsConfiguration         options         = 6;
    repeated ObservedDevice      ignored_devices = 7 [(ext.json) = "remoteIgnoredDevices", (ext.xml) = "remoteIgnoredDevice"];
    repeated ObservedDevice      pending_devices = 8 [deprecated=true];
    Defaults                     defaults        = 9;
}

the json extension property on field names, however, is not used for code generated which looks like this:

defmodule Syncthex.Proto.Configuration do
  @moduledoc false
  use Protobuf, syntax: :proto3
  ### SNIP ###
  field :ignored_devices, 7,
    repeated: true,
    type: Syncthex.Proto.ObservedDevice,
    json_name: `ignoredDevices"`
  ### SNIP ###
end

I have read the initial JSON proposal and am using the JSON functionality of the lib.

Questions

  • Is Protobuf.JSON thought to be an extension in the sense of Extensions #14 ?
  • Is ext as in ext.json from the pb codequote. the extension name or a general holder for field/message variables of various extensions
  • would you consider a PR of my stab at this?
  • Is it proper to put the code for mapping ext.json into Protobuf.JSON (& friends in modules sharing the Protobuf.JSON name prefix) ?
@britto
Copy link
Collaborator

britto commented Jul 20, 2021

Hi @paulbalomiri! Let me try to address your questions.

Is Protobuf.JSON thought to be an extension in the sense of Extensions #14?

No. JSON and Extensions are totally distinct features. Protobuf.JSON aims to support encoding and decoding whole messages in JSON format. It's an official feature, supported in Proto3. You can read more about it in #105.

Is ext as in ext.json from the pb codequote the extension name or a general holder for field/message variables of various extensions?

These ext.* entries are custom field options defined by Syncthing itself:

https://github.com/syncthing/syncthing/blob/v1.18.0/proto/ext.proto

Apparently, the ext.json custom option is a way they came up with to have custom JSON field names in Proto2, as an alternative to the built-in json_name field option. However, I don't know if their JSON implementation matches the spec of Google's, so use at your own risk.

Would you consider a PR of my stab at this?

Since this is private to Syncthing's own implementation, I don't think we could safely support it in this lib. But let's hear what the maintainers have to say about it.

Is it proper to put the code for mapping ext.json into Protobuf.JSON (& friends in modules sharing the Protobuf.JSON name prefix)?

As mentioned above, these are specific to Syncthing, so I'd say no. To make it forward-compatible at the compiler level, they'd probably have to consider including both as part of an eventual migration strategy from custom JSON to built-in JSON:

-    repeated ObservedDevice ignored_devices = 7 [(ext.json) = "remoteIgnoredDevices", (ext.xml) = "remoteIgnoredDevice"];
+    repeated ObservedDevice ignored_devices = 7 [json_name = "remoteIgnoredDevices", (ext.json) = "remoteIgnoredDevices", (ext.xml) = "remoteIgnoredDevice"];

@whatyouhide
Copy link
Collaborator

Thanks for the great explanations and context @britto. Agreed with everything you said. This is not something that we can safely implement in this library in a generic way. Sorry @paulbalomiri! Thanks for the report either way 💟

@paulbalomiri
Copy link
Author

Thanks @britto & sorry to have completely overlooked this issue. When i get back to this I'll have to look into it again.

I ended up transforming the protobuf definition source files to make ext.json available as json_name too. I guess some sort of programatic definition transformation in code would do it, but that's a different topic altogether.

Thank you for this lib!
❤️

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

No branches or pull requests

3 participants