Default values are not set.

[relief-melone]

Alot of implementations omit default values when serializing protobufs. So e.g. enums set to 0 will not be sent over the wire. When the message is deserialized this missing value should be set to the default value of 0. However default values will not be set by protoc-gen-ts. This causes problems with interoperability between e.g. C# and JavaScript/TypeScript as in our case. I investigated the generated code a bit and my suspect is this

src/compiler/descriptor.js

// ...
 constructor(data?: any[] | {
            my_enum?:  Payload.SomeEnum
        }) {
            super();
            pb_1.Message.initialize(this, Array.isArray(data) ? data : [], 0, -1, [], []);
            if (!Array.isArray(data) && typeof data == "object") {
                if ("my_enum" in data && data.my_enum != undefined) {
                    this.my_enum = data.my_enum;
                }
            }
        }

// ...

If I read this correct then the value is only set if it is there but according to the specification it should be set to the default value 0.

[thesayyn]

Hmm. this looks like has something to do with generated getters. for proto3 we are supposed to call getFieldWithDefault instead of getField. protoc supposed to set default_value for proto3 but seems like it doesn’t.

here:

protoc-gen-ts/src/descriptor.js

Line 889 in b47d74a

if (fieldDescriptor.default_value) {

would like to fix it and send a PR with tests included?

[thesayyn]

for anyone who wants to fix this: https://developers.google.com/protocol-buffers/docs/proto3#default

[Yurzel]

Hey, I'm looking into this problem. The thing is proto3 seems to remove default_value (it's not even being serialized). I'm experienced with protobuf-net (proto3) and I took a look into javascript (both proto2 and proto3).

So both (.net and javascript) introduces functions hasField and clearField. In case of proto2 it's generated for all the fields. Proto3 just for the optional ones. Also all properties are non nullable types for .net and getters call getFieldWithDefault.

/**
 * Clears the field making it undefined.
 * @return {!proto.MessageVerThree} returns this
 */
proto.MessageVerThree.prototype.clearNullableInt = function() {
  return jspb.Message.setField(this, 4, undefined);
};


/**
 * Returns whether this field is set.
 * @return {boolean}
 */
proto.MessageVerThree.prototype.hasNullableInt = function() {
  return jspb.Message.getField(this, 4) != null;
};

Meaning there are 2? options how to solve this:

1. Make all properties non nullable and introduce hasField and clearField functions, change all getters to getFieldWithDefault
2. Put logic inside getter

The first one preserves usage between (I hope) all implementations. The second one is prettier to use for me. 😄
Also I found that proto3 js uses functions like setProto3StringField and setProto3IntField etc., but I have no idea if it's necessary.

[thesayyn]

I believe a simple trick in this function here:

protoc-gen-ts/src/descriptor.js

Line 889 in b47d74a

if (fieldDescriptor.default_value) {

should do the trick.

the logic is already there you just have to add a branch to say something like if the field has no default value and syntax is proto3 and type of field has a default value in proto3 then assign it to an intermediate variable to continue the flow as if it had a default value. the generator code is already there.

[thesayyn]

this is not written anywhere but we never do work at runtime if it can be done in compile time so the second option is not an ideal solution.

[thesayyn]

I'd like to have presence getters in here if you are willing to send a separate PR for it.
See: #80 (comment)

[Yurzel]

I believe a simple trick in this function here:

protoc-gen-ts/src/descriptor.js

Line 889 in b47d74a

if (fieldDescriptor.default_value) {

should do the trick.
the logic is already there you just have to add a branch to say something like if the field has no default value and syntax is proto3 and type of field has a default value in proto3 then assign it to an intermediate variable to continue the flow as if it had a default value. the generator code is already there.

Yea but then there is a problem when you can't distinguish between not set and default value. I'm already working on the presence getter.

I'd like to have presence getters in here if you are willing to send a separate PR for it. See: #80 (comment)

That's pretty much my first purposed solution and I'm already working on it. That means I'll try to mimic the result from the javascript plugin. 😄

[thesayyn]

Yea but then there is a problem when you can't distinguish between not set and default value

this is exactly what getFieldWithDefault does. If the field is present it gives you the value of it but if not it will return you whatever you passed to it as the third argument.

https://github.com/protocolbuffers/protobuf/blob/2f91da585e96a7efe43505f714f03c7716a94ecb/js/message.js#L873-L880

[thesayyn]

Also I found that proto3 js uses functions like setProto3StringField and setProto3IntField etc., but I have no idea if it's necessary.

they are basically guarding against serializing default values. it is a small optimization that I didn't bother implementing.

[Yurzel]

@thesayyn is it possible to contact you on any other platform? I'm having issues with compilation. Otherwise AST should be done.

[thesayyn]

grpc has no slack channel. I am on bazel slack channel. you can reach me there.
http://slack.bazel.build/

[thesayyn]

in order to close this issue, protoc-gen-ts should be fixed to not serialize default values and should return defaults when field has no presence

[thesayyn]

but as far as I am concerned, there shouldn't be any interoperability issues between protoc-gen-ts and other language implementations.

[thesayyn]

diff --git a/src/descriptor.ts b/src/descriptor.ts
index f1cb6da..2d1c68f 100644
--- a/src/descriptor.ts
+++ b/src/descriptor.ts
@@ -916,7 +916,13 @@ function createGetterCall(
       ts.factory.createNumericLiteral(fieldDescriptor.number),
     ];
 
-    if (fieldDescriptor.default_value) {
+    let default_val = fieldDescriptor.default_value
+
+    if (!default_val) {
+        // assign default values
+    }
+
+    if (default_val) {
       getterMethod = "getFieldWithDefault";
       let _default: ts.Expression;
 
diff --git a/src/type.ts b/src/type.ts
index 93422c0..d27feac 100644
--- a/src/type.ts
+++ b/src/type.ts
@@ -4,6 +4,7 @@ import * as ts from "typescript";
 const symbolMap: Map<string, string> = new Map();
 const dependencyMap: Map<string, ts.Identifier> = new Map();
 const mapMap: Map<string, descriptor.DescriptorProto> = new Map();
+const enumLeadingMemberMap: Map<string, string> = new Map();
 
 export function resetDependencyMap() {
   dependencyMap.clear();
@@ -26,6 +27,13 @@ export function getMapDescriptor(
   return mapMap.get(typeName);
 }
 
+
+export function getLeadingEnumMember(
+  type_name: string,
+): string | undefined {
+  return enumLeadingMemberMap.get(type_name);
+}
+
 export function getTypeReferenceExpr(
   rootDescriptor: descriptor.FileDescriptorProto,
   typeName: string,
@@ -83,7 +91,9 @@ export function preprocess(
   prefix: string,
 ) {
   for (const enumDescriptor of targetDescriptor.enum_type) {
-    symbolMap.set(replaceDoubleDots(`${prefix}.${enumDescriptor.name}`), path);
+    const name = replaceDoubleDots(`${prefix}.${enumDescriptor.name}`)
+    symbolMap.set(name, path);
+    enumLeadingMemberMap.set(name, enumDescriptor.value[0].name);
   }
 
   const messages: descriptor.DescriptorProto[] =
@@ -98,7 +108,6 @@ export function preprocess(
     if (isMapEntry(messageDescriptor)) {
       mapMap.set(name, messageDescriptor);
       messages.splice(index, 1);
-
       continue;
     }
 

[atakurt034]

@thesayyn Will you be adding this to the next release?

[relief-melone]

sorry that there has been so little activity on this one from me. I'd love to help out as I really like this project. Unfortunately I'm so packed with work that I don't find the time to get into to topic enough to contribute (my biggest issue is not beeing familiar with the build and test frameworks). I hope I'll find some more time soon.

Additionaly I just noticed that boolean values seem to have the same problem. false seems to dissapear as its beeing the default value. I can open up a separate issue for that just wanted to clarifiy hiere first

[ciricc]

+1

[thesayyn]

fixed at #146 and released in 0.8.5