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

Adding a new field to grpc proto file breaks the functionality if client stub is not updated #1572

Closed
stefinie123 opened this issue Jun 29, 2021 · 5 comments · Fixed by ballerina-platform/module-ballerina-grpc#336
Assignees
Labels
module/grpc Points/2.5 Team/PCM Protocol connector packages related issues Type/Bug

Comments

@stefinie123
Copy link

Description:
When the grpc proto file is updated by adding a new field it breaks the existing client stubs which uses the the previous proto file definition.

Following error is observed:
Invalid protobuf byte sequence

Steps to reproduce:
Update the proto file by adding a new field to a type definition
Try to execute a client/service stub which was generated earlier

Affected Versions:
Swan lake alpha 1

OS, DB, other environment details and versions:

Related Issues (optional):

Suggested Labels (optional):

Suggested Assignees (optional):

@stefinie123 stefinie123 changed the title Adding a new field to grpc proto file breaks the functionality if service bal files are not updated Adding a new field to grpc proto file breaks the functionality if client stub is not updated Jun 29, 2021
@MaryamZi MaryamZi transferred this issue from ballerina-platform/ballerina-lang Jun 30, 2021
@daneshk
Copy link
Member

daneshk commented Jul 14, 2021

@stefinie123 can you explain the issue bit more, better if you can provide an example

  1. can you share the change you did for the proto file definition.
  2. did you regenerate the stub file on either in client or server-side.
  3. did you change the generated stub code manually.

@daneshk daneshk added Team/PCM Protocol connector packages related issues Type/Bug labels Jul 14, 2021
@stefinie123
Copy link
Author

@daneshk Please find my answers below:

  1. We did the following change in proto file where we added a new optional field is_anonymous to the request definition
     message ValidateUserRequest {
         string user_idp_id = 1 [(validate.rules).string.min_len = 1];
         string email = 2 [(validate.rules).string.email = true];
         string invitation_correlation_key = 3;
         bool is_anonymous = 4;
     }
    
  2. The server side stub was regenerated but the client side stub was not regenerated
  3. No

@daneshk
Copy link
Member

daneshk commented Jul 14, 2021

@BuddhiWathsala can you check this

@BuddhiWathsala
Copy link
Contributor

I was able to reproduce the issue. However, there is a design concern to discuss here. When there are unknown fields, what Java does is, Java creates an UnknownFieldSet and return it to the user. However, since we are only deserialising fields that are specifically mentioned in the proto file, we couldn't do such a thing in our implementation. In addition, since we represent proto messages using Ballerina closed record, returning unknown fields is against the design.

Therefore we should decide on a suitable approach:

  • ignore the unknown fields if the user does not regenerate the proto file
  • embed the unknown values to the record as anydata

@daneshk @stefinie123 WDYT?

@daneshk
Copy link
Member

daneshk commented Jul 16, 2021

I was able to reproduce the issue. However, there is a design concern to discuss here. When there are unknown fields, what Java does is, Java creates an UnknownFieldSet and return it to the user. However, since we are only deserialising fields that are specifically mentioned in the proto file, we couldn't do such a thing in our implementation. In addition, since we represent proto messages using Ballerina closed record, returning unknown fields is against the design.

Therefore we should decide on a suitable approach:

  • ignore the unknown fields if the user does not regenerate the proto file
  • embed the unknown values to the record as anydata

@daneshk @stefinie123 WDYT?

IMO, It is ok to Ignore the unknown field as the recipient doesn't know those fields to retrieve the value.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
module/grpc Points/2.5 Team/PCM Protocol connector packages related issues Type/Bug
Projects
None yet
4 participants