ballerina-platform / ballerina-library

The Ballerina Library
https://ballerina.io/learn/api-docs/ballerina/
Apache License 2.0
136 stars 64 forks source link

Improve diagnostics for invalid proto files #4656

Open pubudu91 opened 1 year ago

pubudu91 commented 1 year ago

When the proto file is invalid, the grpc command crashes. It would be better if we can have proper error messages for the failures.

For example, in my case, the issue was I hadn't defined a type that I had used in another type:

message PaginatedUser {
    int64 count = 1;
    repeated User list = 2;
    Pagination pagination = 3;
}

The User wasn't defined in the file. The output from the grpc command was as follows:

bal grpc --input resources/user_service.proto
Downloading the protoc executor file - protoc-3.21.7-osx-x86_64.exe
Download successfully completed. Executor file path - /var/folders/_w/76qx789d5hj633snw3c8fswr0000gp/T/protoc-3.21.7-osx-x86_64.exe
Successfully extracted the library files.
Jul 12, 2023 11:27:00 AM io.ballerina.protoc.protobuf.cmd.GrpcCmd generateBalFile
SEVERE: An error occurred when generating the proto descriptor.
io.ballerina.protoc.protobuf.exception.CodeGeneratorException: Invalid command syntax. Stream closed
    at io.ballerina.protoc.protobuf.utils.BalFileGenerationUtils.handleProcessExecutionErrors(BalFileGenerationUtils.java:110)
    at io.ballerina.protoc.protobuf.utils.BalFileGenerationUtils.generateDescriptor(BalFileGenerationUtils.java:83)
    at io.ballerina.protoc.protobuf.cmd.DescriptorsGenerator.generateRootDescriptor(DescriptorsGenerator.java:148)
    at io.ballerina.protoc.protobuf.cmd.GrpcCmd.generateBalFile(GrpcCmd.java:251)
    at io.ballerina.protoc.protobuf.cmd.GrpcCmd.execute(GrpcCmd.java:165)
    at java.base/java.util.Optional.ifPresent(Optional.java:183)
    at io.ballerina.cli.launcher.Main.main(Main.java:53)
Caused by: java.io.IOException: Stream closed
    at java.base/java.io.BufferedInputStream.getBufIfOpen(BufferedInputStream.java:176)
    at java.base/java.io.BufferedInputStream.read(BufferedInputStream.java:342)
    at java.base/sun.nio.cs.StreamDecoder.readBytes(StreamDecoder.java:284)
    at java.base/sun.nio.cs.StreamDecoder.implRead(StreamDecoder.java:326)
    at java.base/sun.nio.cs.StreamDecoder.read(StreamDecoder.java:178)
    at java.base/java.io.InputStreamReader.read(InputStreamReader.java:181)
    at java.base/java.io.BufferedReader.fill(BufferedReader.java:161)
    at java.base/java.io.BufferedReader.readLine(BufferedReader.java:326)
    at java.base/java.io.BufferedReader.readLine(BufferedReader.java:392)
    at io.ballerina.protoc.protobuf.utils.BalFileGenerationUtils.handleProcessExecutionErrors(BalFileGenerationUtils.java:105)
    ... 6 more

An error occurred when generating the proto descriptor. Invalid command syntax. Stream closed

This has 2 problems:

Abhi132004 commented 1 year ago

Hii ballerina team , I would like to work on this task

Abhi132004 commented 1 year ago

done with the task kindly check.

ThisaruGuruge commented 1 year ago

Hi @Abhi132004! 🚀

We value your contributions and look forward to collaborating with you. To help you get started, here are some essential resources:

Remember, no contribution is too small, and your feedback is invaluable. Feel free to ask questions, propose ideas, or report issues. Together, we can make Ballerina even better!

Happy coding! 🎉

ThisaruGuruge commented 1 year ago

done with the task kindly check.

Hi @Abhi132004

You have to do the fix and send a PR to the ballerina-platform organization repository. Please look into the resources provided before working on the issue. Feel free to ask any questions you have regarding this issue.

dilanSachi commented 1 year ago

Hi @Abhi132004 . Regarding your PR, this is something different from what we expect in this issue. We need to handle the error from the tool level instead of ballerina program level. When a user runs the tool to generate a Ballerina stub file, it does not run inside a Ballerina program, It's a separate Java program which starts it's execution from here [1]. Feel free to ask questions if anything is not clear.

[1] https://github.com/ballerina-platform/protoc-tools/blob/main/protoc-cli/src/main/java/io/ballerina/protoc/protobuf/cmd/GrpcCmd.java#L128

Abhi132004 commented 1 year ago

@dilanSachi fine, So I need to make change in code itself or I will submit a new PR as a new file?

dilanSachi commented 1 year ago

@dilanSachi fine, So I need to make change in code itself or I will submit a new PR as a new file?

You need to change our existing codebase to do this change and send a new pr to the repo

Abhi132004 commented 1 year ago

@dilanSachi here's the updated file , This modified code catches exceptions that can occur during the execution of methods such as getProtoFiles and generateBalFile. In contrast, the original code doesn't catch these exceptions explicitly and might lead to unexpected crashes.

Abhi132004 commented 1 year ago

The expected changes from the reviewers in patch1 implemented successfully

Abhi132004 commented 1 year ago

Code readability improved as mentioned, code updated such that it dosen't exceed 120 words limit ,@ThisaruGuruge @keizer619 @dilanSachi @daneshk

Abhi132004 commented 1 year ago

The changes mentioned is implemented in the same pr(patch 3) @dilanSachi

Abhi132004 commented 1 year ago

All mentioned changes from patch1-3 are implemented successfully

Ishad-M-I-M commented 1 month ago

Hi @keizer619 , Can I get this issue assigned to me

mohammadruman commented 1 month ago

Hi @keizer619 can you assign me? I would like to work on this issue.

keizer619 commented 3 weeks ago

@mohammadruman Are you working on this? If not we can let @Ishad-M-I-M to take this