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

2 generated files with same message name makes deserialize response error #1462

Open
rizdaputra opened this issue Sep 6, 2024 · 15 comments
Open

Comments

@rizdaputra
Copy link

So i have different proto files, they have each a request that have same message response name but the value and structure are actually different. This cause problem whenever i declare both then one of them is rendered as that message even though i am calling the other function.

e.g.

ClientA has getStatus with response StatusResponse
ClientB has getStatus with response StatusResponse

but the StatusResponse definition in ClientA and ClientB is different but whenver i call each getStatus both are using the StatusResponse when doing assertion.

@sampajano
Copy link
Collaborator

Hi, thanks for the question :)

May I ask if you're using Typescript or Common JS, and can you show me how you're importing the client classes?


I'm wondering, if you could just use type aliases to get around this issue?

e.g.

import {StatusResponse as StatusResponseA} from './client1_pb';
import {StatusResponse as StatusResponseB} from './client2_pb';

When you're importing the message classes like here?

import {EchoRequest, EchoResponse, ServerStreamingEchoRequest, ServerStreamingEchoResponse} from './echo_pb';

@rizdaputra
Copy link
Author

i am using typescript. So you mean i need to edit the generated files manually? because the problem i am facing is that this is happening on the client generated files, not on my code, it is error when deserializing. I need a solution where i dont need to edit the generated code because i run the generation on every CI/CD to make sure it is updated if there are any proto changes

@sampajano
Copy link
Collaborator

Ohh i see.. no i wasn't suggesting you to edit the generated files manually..

I was thinking that if you have 2 different versions of StatusResponse, then you can import them using an alias (like the example i gave) to avoid collision in your client code.

Yeah it definitely doesn't make sense for you having to edit the generated files yourself :)


So, if it's an error during deserializing, i guess the generated code has confused the 2 StatusResponse classes, right?

If you can provide some more details on what the generated files look like it might help with understanding of this issue.

@rizdaputra
Copy link
Author

yes it confused the 2 statusresponse classes,

when i call the request in the first client it checks the statusresponse class of the second class instead

These are the classes generated, i omitted some item because i cannot disclose some of the item, but basically there are some differences:

export class RequestStatusResponse extends jspb.Message {
  getRobotstatus(): RobotConnectionStatus_t;
  setRobotstatus(value: RobotConnectionStatus_t): RequestStatusResponse;

  getStatustimestamp(): string;
  setStatustimestamp(value: string): RequestStatusResponse;

  serializeBinary(): Uint8Array;
  toObject(includeInstance?: boolean): RequestStatusResponse.AsObject;
  static toObject(includeInstance: boolean, msg: RequestStatusResponse): RequestStatusResponse.AsObject;
  static serializeBinaryToWriter(message: RequestStatusResponse, writer: jspb.BinaryWriter): void;
  static deserializeBinary(bytes: Uint8Array): RequestStatusResponse;
  static deserializeBinaryFromReader(message: RequestStatusResponse, reader: jspb.BinaryReader): RequestStatusResponse;
}

export namespace RequestStatusResponse {
  export type AsObject = {
    robotstatus: RobotConnectionStatus_t,
    statustimestamp: string,
  }
}

export enum RobotConnectionStatus_t { 
  CONNECTION_STATUS_INVALID = 0,
  CONNECTION_STATUS_CONNECTED = 1,
  CONNECTION_STATUS_ERROR = 2,
  CONNECTION_STATUS_TIMEOUT = 3,
  CONNECTION_STATUS_SERVER_ERROR = 4,
  CONNECTION_STATUS_MISSING_OBSERVER = 5,
}

and the other one is

export class RequestStatusResponse extends jspb.Message {
  getConnectionstatus(): RobotConnectionStatus_t;
  setConnectionstatus(value: RobotConnectionStatus_t): RequestStatusResponse;

  getConnectedtool(): Tool_t;
  setConnectedtool(value: Tool_t): RequestStatusResponse;

  serializeBinary(): Uint8Array;
  toObject(includeInstance?: boolean): RequestStatusResponse.AsObject;
  static toObject(
    includeInstance: boolean,
    msg: RequestStatusResponse,
  ): RequestStatusResponse.AsObject;
  static serializeBinaryToWriter(
    message: RequestStatusResponse,
    writer: jspb.BinaryWriter,
  ): void;
  static deserializeBinary(bytes: Uint8Array): RequestStatusResponse;
  static deserializeBinaryFromReader(
    message: RequestStatusResponse,
    reader: jspb.BinaryReader,
  ): RequestStatusResponse;
}

export namespace RequestStatusResponse {
  export type AsObject = {
    connectionstatus: RobotConnectionStatus_t;
    connectedtool: Tool_t;
  };
}

export enum RobotConnectionStatus_t {
  CONNECTION_STATUS_INVALID = 0,
  CONNECTION_STATUS_CONNECTED = 1,
  CONNECTION_STATUS_ERROR = 2,
  CONNECTION_STATUS_TIMEOUT = 3,
  CONNECTION_STATUS_SERVER_ERROR = 4,
}
export enum Tool_t {
  TOOL_INVALID = 0,
  TOOL_A= 1,
  TOOL_B= 2,
  TOOL_C= 3,
}

the main function

methodDescriptorRequestStatus = new grpcWeb.MethodDescriptor(
    '/Service/RequestStatus',
    grpcWeb.MethodType.UNARY,
    google_protobuf_empty_pb.Empty,
    generated_pb.RequestStatusResponse,
    (request: google_protobuf_empty_pb.Empty) => {
      return request.serializeBinary();
    },
    generated_pb.RequestStatusResponse.deserializeBinary,
  );

  requestStatus(
    request: google_protobuf_empty_pb.Empty,
    metadata?: grpcWeb.Metadata | null,
  ): Promise<generated_pb.RequestStatusResponse>;

  requestStatus(
    request: google_protobuf_empty_pb.Empty,
    metadata: grpcWeb.Metadata | null,
    callback: (
      err: grpcWeb.RpcError,
      response: generated_pb.RequestStatusResponse,
    ) => void,
  ): grpcWeb.ClientReadableStream<generated_pb.RequestStatusResponse>;

  requestStatus(
    request: google_protobuf_empty_pb.Empty,
    metadata?: grpcWeb.Metadata | null,
    callback?: (
      err: grpcWeb.RpcError,
      response: generated_pb.RequestStatusResponse,
    ) => void,
  ) {
    if (callback !== undefined) {
      return this.client_.rpcCall(
        this.hostname_ + '/Service/RequestStatus',
        request,
        metadata || {},
        this.methodDescriptorRequestStatus,
        callback,
      );
    }
    return this.client_.unaryCall(
      this.hostname_ + '/Service/RequestStatus',
      request,
      metadata || {},
      this.methodDescriptorRequestStatus,
    );
  }
proto.RequestStatusResponse.deserializeBinary = function(bytes) {
  var reader = new jspb.BinaryReader(bytes);
  var msg = new proto.RequestStatusResponse;
  return proto.RequestStatusResponse.deserializeBinaryFromReader(msg, reader);
};

@sampajano
Copy link
Collaborator

@rizdaputra Aha i see! Thanks for providing the file content!

I'm if you could could also share how "RequestStatusResponse" was imported in the 2 generated services?

What's the namespace that was used?

And would you be able to specify different namespaces so they can be imported in different ways (to avoid the confusions)?

@rizdaputra
Copy link
Author

i think in the auto generated files it is always generated imports like this

import * as proto_file_name from 'generated_pb'

so when calling it is doing proto_file_name.RequestStatusResponse

You mean i change in the proto so the name will be different? I am searching for a solution because the owner of the proto is not me and it is used in multiple application, changing it would break multiple applications that are not maintained by me. Wondering if there is a way to add prefix or suffix when generating the service/client or is there another solution for this

@sampajano
Copy link
Collaborator

Could you share the exact command you used for generating the proto & grpc files? I'll take a look at if that's possible! Thanks!

@rizdaputra
Copy link
Author

i used

protoc -I=./protocollection/FirstProtoFolder FirstProtoFile.proto --js_out=import_style=commonjs,binary:./src/grpcs --grpc-web_out=import_style=typescript,mode=grpcweb:./src/grpcs
protoc -I=./protocollection/SecondProtoFolder SecondProtoFile.proto --js_out=import_style=commonjs,binary:./src/grpcs --grpc-web_out=import_style=typescript,mode=grpcweb:./src/grpcs

@sampajano
Copy link
Collaborator

@rizdaputra Hihi! Thanks for providing the commands you used! And sorry for the delay!!

So I've tried creating 2 folders like below (under the echo folder):

./test1/echo.proto
./test2/echo2.proto

With echo.proto and echo2.proto having exactly the same content.

And when i run:

protoc -I=./test1 echo.proto --js_out=import_style=commonjs:./ts-example --grpc-web_out=import_style=typescript,mode=grpcwebtext:./ts-example

protoc -I=./test2 echo2.proto --js_out=import_style=commonjs:./ts-example --grpc-web_out=import_style=typescript,mode=grpcwebtext:./ts-example

I'm getting 2 files, and one of them would be using echo2_pb.EchoRequest and the other using echo2_pb.EchoRequest. Like below:

Screenshot 2024-10-17 at 5 30 36 PM

That seems like it might work without conflict to me?

Is this what you're seeing and would this potentially work for you?

Thanks!

@rizdaputra
Copy link
Author

@sampajano thank you for getting back and trying to replicate for me, in my case they are in a different folder. But yes code wise the generated code is correct, but when the code is compiled somehow the function calling is actually conflicting and the only one conflicting is the deserialize process of the response. i have another call that are exactly same and it is not conflicting. But when the same response type name with different structure, the type that is used for deserializing the response is wrong

@rizdaputra
Copy link
Author

It is generated like this, as you can see there are differences but the name is the same, is it because the class name is same then it is conflicting? I want to resolve this without changing the name in the proto file since that would need to change multiple parts of the system and might be breaking
image

@sampajano
Copy link
Collaborator

sampajano commented Nov 8, 2024

@rizdaputra Hi thanks for the response!

when the code is compiled somehow the function calling is actually conflicting and the only one conflicting is the deserialize process of the response.

I'm wondering what exactly is the symptom and error message you're seeing here?

Does this happen inside the grpc-web stack, or when you're trying to consume the response from a successfully returned RPC?


One thing you could try is to modify the package for these 2 proto files as in following line:

package grpc.gateway.testing;

When i inspected the generated JS files, both files are exporting this same symbol:

goog.exportSymbol('proto.grpc.gateway.testing.EchoResponse', null, global);

Which might be a source of confusion after both files are compiled in the same JS budle.

@rizdaputra
Copy link
Author

rizdaputra commented Nov 8, 2024

the error i get i assertion error during the deserialization of the response, when i call the Client A, it uses the Client B Response type to check for assertion. Because the response type is the same name but is actually different format i get assertion error even though the response was correct for Client A

so is the only solution editing the proto? So it cannot handle the same name even though it is from a different files? I really want to avoid that cause the proto is used in many places and if i edit it then it would be breaking for all of my other services and the owner is not me too

@sampajano

@sampajano
Copy link
Collaborator

the error i get i assertion error during the deserialization of the response, when i call the Client A, it uses the Client B Response type to check for assertion. Because the response type is the same name but is actually different format i get assertion error even though the response was correct for Client A

Could you provide the exact error messages / stack for it?

so is the only solution editing the proto? So it cannot handle the same name even though it is from a different files? I really want to avoid that cause the proto is used in many places and if i edit it then it would be breaking for all of my other services and the owner is not me too

Yeah i can understand the problem it might create for you.. But based on what i'm seeing, that basically all protos with the exact package name + message name will be exported as the same global symbol, like below:

goog.exportSymbol('proto.grpc.gateway.testing.EchoResponse', null, global);

So i believe this might be inherently conflicting. (And TBH, I think package name + message name should be a unique identifier of a proto in your app anyways (regardless whether they're in the same "file", so it's probably a good practice to avoid duplications.)

If you provide error log though, it might shine more light on how it might be potentially solved. But for now, I'd say your best bet is to modify the package name of one of the protos :)

@rizdaputra
Copy link
Author

the error i get i assertion error during the deserialization of the response, when i call the Client A, it uses the Client B Response type to check for assertion. Because the response type is the same name but is actually different format i get assertion error even though the response was correct for Client A

Could you provide the exact error messages / stack for it?

so is the only solution editing the proto? So it cannot handle the same name even though it is from a different files? I really want to avoid that cause the proto is used in many places and if i edit it then it would be breaking for all of my other services and the owner is not me too

Yeah i can understand the problem it might create for you.. But based on what i'm seeing, that basically all protos with the exact package name + message name will be exported as the same global symbol, like below:

goog.exportSymbol('proto.grpc.gateway.testing.EchoResponse', null, global);

So i believe this might be inherently conflicting. (And TBH, I think package name + message name should be a unique identifier of a proto in your app anyways (regardless whether they're in the same "file", so it's probably a good practice to avoid duplications.)

If you provide error log though, it might shine more light on how it might be potentially solved. But for now, I'd say your best bet is to modify the package name of one of the protos :)

Thank you so much for the help. I'll raise this to my team, seems like that is the best solution for this problem

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants