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

Addition of field on empty message breaks backward compatibility #669

Open
gosthell opened this issue Jan 20, 2025 · 2 comments · May be fixed by #670
Open

Addition of field on empty message breaks backward compatibility #669

gosthell opened this issue Jan 20, 2025 · 2 comments · May be fixed by #670

Comments

@gosthell
Copy link

If you define a proto message without any fields but return the message populated with fields in the grpc server,
the decoder won't read the fields and the buffer will get out of sync

To reproduce:

message GrpcTest {
}

// Generate client code

//Update proto

message GrpcTest {
string test = 1;
}

// Generate server code

@jcready
Copy link
Contributor

jcready commented Jan 20, 2025

This is just how protobuf works. See unknown fields for proto2 and proto3. This library supports unknown fields and you can access and preserve them when decoding/encoding binary.

@gosthell
Copy link
Author

gosthell commented Jan 21, 2025

Unknown fields only work if the message has fields implemented. In the case of an empty message, the generated de-serialization logic is the following:

    internalBinaryRead(reader: IBinaryReader, length: number, options: BinaryReadOptions, target?: GrpcTest): GrpcTest {
        return target ?? this.create();
    }

This implementation does not handle unknown field, it does not progress the buffer, and therefore ends up in a corrupt state.

The correct generated reader should be something of the like:

 let message = target ?? this.create(), end = reader.pos + length;
        while (reader.pos < end) {
            let [fieldNo, wireType] = reader.tag();
            switch (fieldNo) {
                default:
                    let u = options.readUnknownField;
                    if (u === "throw")
                        throw new globalThis.Error(`Unknown field ${fieldNo} (wire type ${wireType}) for ${this.typeName}`);
                    let d = reader.skip(wireType);
                    if (u !== false)
                        (u === true ? UnknownFieldHandler.onRead : u)(this.typeName, message, fieldNo, wireType, d);
            }
        }
        return message;

@jcready jcready linked a pull request Jan 27, 2025 that will close this issue
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

Successfully merging a pull request may close this issue.

2 participants