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

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

Open
TheLostTree opened this issue Dec 4, 2023 · 3 comments

Comments

@TheLostTree
Copy link

TheLostTree commented Dec 4, 2023

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
Copy link

goodov commented Mar 22, 2024

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
Copy link
Contributor

jcready commented Mar 23, 2024

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
Copy link

goodov commented Mar 25, 2024

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: #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?

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