Support for groups

[mar-v-in]

As the README clearly states, Wire currently does not support protobuf groups (in fact it comes with support for skipping groups in the deserializer and can parse groups from .proto files, but it doesn't actually allow to use groups).

I was wondering if you would accept a PR to add group support in one of these variants:

• Full group support as is described in the documentation (I know it's deprecated, but it still occurs in the wild)

or

• Parsing group as message: When encountering a group on incoming binary data, instead of skipping it, return it as if it was a message. This could be tied to a file or message option. It would allow to parse data that contains groups (but requires first adjusting the .proto file to replace the group with a submessage). This is great when having legacy systems that emit groups or have files that have groups, but you plan to migrate those to use submessages, as you can parse the old serialization and the new serialization into the same structure.
• Allow serializing a message field as a group: When an option is set on a message field, the serialization of that message field will be that of a group, not that of a submessage (<tag>:SGROUP <message> <tag>:EGROUP instead of <tag>:LEN:<len>:<message>). This is needed if you still need legacy systems to be able to read your messages. Using the submessage code syntax allows to first migrate all systems to the new code (that can parse both serializations), while still emitting groups, and then switch off the group serialization once all systems are upgraded to be able to read submessage serialization.

The latter approach has the disadvantage that it would not allow to take .proto files that have groups as is, but the advantage that it is probably less code to maintain (as for the largest part, it's all just the same messages, only with changes to parsing and serializing code) and that it makes it reasonably easy to migrate to submessages in production systems.

[oldergod]

I think right now that all the group bytes would be in the unknownFields if one really wanted to read them.
Is the serialization the same for both those cases on both bytes and JSON?

message SearchResponse {
  repeated group Result = 1 {
    optional string url = 1;
    optional string title = 2;
    repeated string snippets = 3;
  }
}

and

message SearchResponse {
  message Result {
    optional string url = 1;
    optional string title = 2;
    repeated string snippets = 3;
  }
  repeated Result result = 1;
}

If yes, Wire could maybe parse groups as in the first case but transform them into nested types like the second one; that'd take care of everything. That'd be simple although hacky.

[mar-v-in]

I don't think groups show up in unknownFields, due to them working different from all other types. This code filters out groups very early.

Groups don't show up exactly the same in bytes. They are not length prefixed but instead use start and end markers. The byte representation for the above two examples (assuming the name of each field as its value) would be:

1:SGROUP                     0b
  1:LEN:3:url                0875726c
  2:LEN:5:title              107469746c65
  3:LEN:8:snippets           18736e697070657473
1:EGROUP                     0c

vs

1:LEN:19:[                   0a13
  1:LEN:3:url                0875726c
  2:LEN:5:title              107469746c65
  3:LEN:8:snippets           18736e697070657473
]

As groups are exclusive to proto versions 1 and 2 and only proto version 3 added official JSON representation, there is no official JSON representation for groups.

My suggestion for part 1 of variant 2 would be to parse the first encoding as if it was the second. Given the current reader code, this seems to be reasonably easy: when parsing submessages, the current reader reads the message start and virtually changes the length to the submessage length, and will thus return EOF when end of submessage bytes is reached. For groups we can make use of that mechanism by not limiting the length but instead read the SGROUP as a submessage start and the EGROUP as EOF (terminating the submessage). The bytes between SGROUP/EGROUP and the bytes inside the LEN for a submessage are the same, so this will work out fine (at least when assuming valid input data)

[oldergod]

Hmm, we have no need for groups ourselves. If you're willing to make a PR, why not? I'm a bit anxious it might require a lot of work: we have to decide how we're modelling it in the Schema object, how we'll be generating on kotlin/java/proto (maybe swift too), add JSON support (java/kotlin).
You're right about the group encoding not ending up in the unknown fields, I didn't expect that. If we were to properly add them to the unknown fields, would that be enough for your use case? You could decode on your end without the whole support Wire would require.

[mar-v-in]

Unfortunately, I also need to be able to encode.

The reason why I proposed the variant to not have proper groups support, but model groups as submessages, is to reduce work for maintainers. It would "only" be:

• The ability to read a group into a submessage field.
• The ability to write a submessage field as a group.

Notably, this proposal would not add support for reading group syntax from .proto file or any proper modeling of it. In the .proto file, I would just have a field option to instruct the writer to write group bytes instead of submessage bytes. For the JSON representation and everything else, I would use the same behavior as a submessage.

[oldergod]

I talked with Jesse and that's good with us.

this proposal would not add support for reading group syntax from .proto

I think we actually already support that. The point is when we go from MessageElement to MessageType, we need to do some manipulation. I can write a simple which shows what I have in mind. We either extract the group, create one field in the parent type and create a new type, or we somehow handle it as groups. If we create a field, we would have a marker someone to a field saying that it's actually a group? I wonder how we could prevent other code to refer to this new pseudo message that we create out of it. We'd also have to make sure there's no conflict like:

message A {
  group B = 1;
}
message B {}

It looks like you already have an idea of what to do so maybe we can start with that? An MVP PR so we can iterate without using too much time.

[swankjesse]

I’m reluctant to add this feature.

Would you consider supporting groups in a private fork? That way we avoid the burden of maintaining this feature indefinitely for what is likely a single user.

[mar-v-in]

I believe the maintenance burden would be reasonably small, but of course I'm not the maintainer, so that's an easy thing to say.

I also don't think that it is a single user thing. Protobuf groups appear in the wild and I know of at least one project (not mine) that decided to go with libprotobuf instead of wire specifically because it lacks support for groups. As for me, I'm already using wire in all my projects and now I need to be able to interact with a third-party API that uses groups, that's why this comes up.

Maybe I can start with creating a PR for the part of deserializing groups into submessages? Then it might be easier for you to assess if it's worth the maintenance burden to include it?

[swankjesse]

I looked at your use case, and I’m really supportive of it!

So even though I don’t like adding deprecated stuff like this, I want you to succeed and am in favour of this enhancement.