Bug: Oneof support does not work with proto2 about protoc-plugin HOT 6 CLOSED

It doesn't work with proto3 semantics either.

from protoc-plugin.

Hi @lakshay2395. I'm sorry that you are running into trouble. Could you elaborate on what you are seeing? I know we use oneof in our production proto3 specifications, so its not clear to me what you are running into.

from protoc-plugin.

I'm seeing this issue (or a different but related issue?) as well in cases where a sub-message has no fields:

syntax = "proto3";

package test;

message A {
  int32 i = 1;
}

message E {}

message M {
  oneof oo {
    A a = 1;
    E b = 2;
  }
  int32 c = 3;
}

REPL session:

user=> (require '[protojure.protobuf :as pb] '[test :as t])
nil
user=> (test/pb->M (pb/->pb (test/new-M {:oo {:a {:i 1}} :c 1})))
#test.M-record{:oo {:a #test.A-record{:i 1}}, :c 1}
user=> (test/pb->M (pb/->pb (test/new-M {:oo {:b {}} :c 1})))
#test.M-record{:oo nil, :c 1}

It can be fixed by dropping this check. Perhaps there should be a flag on the fn so that the empty record can be serialized (for oneofs)? Or is there ever a reason to drop an empty record, even outside of oneof? It seems to me that there's a semantic difference between "this record is present with no fields" and "this record isn't present at all," but I'm not super familiar with protobuf in general--I just know it affects my use case.

EDIT: After doing more research I definitely think that dropping that check is the right behaviour. Proto3 says that the default value of a message is "not set," meaning that a message which has been set to {} is not the default value and should not be elided. I'm currently using an alter-var-root in my code that removes that when, and everything appears to work properly on proto2.

from protoc-plugin.

I believe this is fixed with d402568

from protoc-plugin.

@ghaskins as far as I understand it this bug (or at least the part I posted about) is actually part of protojure/lib and was not fixed by #54. That being said, I'll reopen the issue over there because that's probably a better place for it.

from protoc-plugin.

@sharrap apologies, that was my misunderstanding. And yes, moving it over there makes sense to me too.

from protoc-plugin.