timostamm / protobuf-ts

Protobuf and RPC for TypeScript
Apache License 2.0
1.1k stars 131 forks source link

Proto2 required fields are not serialized when they are the default value #612

Open TheLostTree opened 12 months ago

TheLostTree commented 12 months ago
syntax = "proto2";
message SCCharacterLoginResult {
    required int32 resultCode = 1;
    optional int32 loginType = 2;
}
interface SCCharacterLoginResult {
    /**
     * @generated from protobuf field: int32 resultCode = 1;
     */
    resultCode: number;
    /**
     * @generated from protobuf field: optional ELoginType loginType = 2;
     */
    loginType?: number;
}
/*

*/
let obj = {
    resultCode: 0,
    loginType: 0
}
{
    let p : SCCharacterLoginResult = SCCharacterLoginResult.fromJson(obj)
    let r = Buffer.from(SCCharacterLoginResult.toBinary(p))
    console.log(r.toString("hex"))
    //1000
}

import * as pbjs from "protobufjs"

{
    let root = new pbjs.Root();
    let x = root.loadSync("./src/proto/out.proto")
    let pType = x.lookupType("SCCharacterLoginResult")
    let r = Buffer.from(pType.encode(obj).finish())
    console.log(r.toString("hex"))
    //08001000
}

Protobuf-ts doesn't serialize the resultCode field even though it is set. Would it be possible for protobuf-ts to emit code that handels the required in proto2 by removing the check for resultCode == 0?

goodov commented 8 months ago

Can confirm. toBinary and toJson doesn't output default values for required fields when we use proto2.

Is this a bug or intended behavior? This should be solvable somehow. One approach is to made an option for those two functions to serialize required values even in the default state.

jcready commented 8 months ago

There is only partial support for proto2. The required label is not supported. At least for toJson there is an option to emitDefaultValues, but there is no such option for toBinary.

goodov commented 8 months ago

The info on partial support for proto2 is a bit outdated. The library is able to serialize and deserialize such fields without issues, there's no any bool fallback.

From my experiments with other TS libraries this one is a clear winner, but the inability to configure default value writes to support proto2 clients is a deal breaker.

There was actually a very similar request to support this case some time ago: https://github.com/timostamm/protobuf-ts/issues/194

I can think of few approaches to make this work:

  1. Support this as a flag in BinaryWriteOptions. This is pretty invasive, adds unnecessary runtime bloat that's not needed for most scenarios, but is clear and understandable.
  2. Support this in protoc plugin with a flag like always_emit_default_values. This is a little bit hacky. The idea here is to keep TS interfaces as is (do not add ? to fields), but:
    • modify generated serializers to use !== undefined check instead of !== 0 and != "";
    • add opt: true for reflection serializers to do the same.

The protoc flag looks like a solid solution to support this edge case for users who are forced to support proto2 in their projects, and at the same time don't intervene in the usual proto3 use cases of this library. Thoughts?