wip: Fix nil SourceCodeInfo in descriptor.File #3573
Conversation
WalkthroughThe changes primarily focus on error handling in the Changes
TipsChat with CodeRabbit Bot (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Files selected for processing (1)
- protoc-gen-openapiv2/internal/genopenapi/template.go (1 hunks)
Additional comments (Suppressed): 1
protoc-gen-openapiv2/internal/genopenapi/template.go (1)
- 2337-2337: This line appears to be unchanged and correctly implemented in the new hunk.
| if file.SourceCodeInfo == nil { | ||
| fmt.Fprintln(os.Stderr, file.GetName(), "descriptor.File should not contain nil SourceCodeInfo") | ||
| return "" | ||
| os.Exit(1) |
There was a problem hiding this comment.
The change from returning an empty string to exiting the program when file.SourceCodeInfo is nil could potentially cause unexpected behavior if other parts of the codebase are not designed to handle this abrupt termination. Ensure that all calls to protoComments function throughout the codebase can handle this new behavior.
- fmt.Fprintln(os.Stderr, file.GetName(), "descriptor.File should not contain nil SourceCodeInfo")
- os.Exit(1)
+ log.Fatalf("%s: descriptor.File should not contain nil SourceCodeInfo", file.GetName())In the suggested change, I've replaced fmt.Fprintln and os.Exit(1) with log.Fatalf, which combines these two operations. It prints the error message and then calls os.Exit(1). This makes the code more concise and clear about its intention to terminate the program due to a fatal error.
In the `protoComments` function, if the `file.SourceCodeInfo` is nil, print an error message and exit with a non-zero status code. The change fixes an issue where the `descriptor.File` should not contain a nil `SourceCodeInfo`, providing better error handling. Signed-off-by: aimuz <[email protected]>
aimuz
force-pushed
the
fix-nil-sourceCodeInfo
branch
from
32e4156
to
73895b1
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Files selected for processing (1)
- protoc-gen-openapiv2/internal/genopenapi/template.go (2 hunks)
Additional comments (Suppressed): 2
protoc-gen-openapiv2/internal/genopenapi/template.go (2)
4-10: The addition of the "log" package is appropriate for the new error handling strategy.
2332-2335: The change from
fmt.Fprintlntolog.Fatalfalters the behavior of the function whenfile.SourceCodeInfois nil. Previously, the function would continue execution after logging the error, now it will terminate the program. Ensure that this change in behavior is intended and won't cause unexpected termination in other parts of the codebase whereprotoCommentsis used.
aimuz
changed the title
|
As well intentioned as this change is, I don't think we can do this. There may be existing users that use the gateway successfully with stripped protobuf definitions, and we should continue to support them. This would be a breaking change. |
In the
protoCommentsfunction, if thefile.SourceCodeInfois nil,print an error message and exit with a non-zero status code.
The change fixes an issue where the
descriptor.Fileshould not containa nil
SourceCodeInfo, providing better error handling.Signed-off-by: aimuz [email protected]
References to other Issues or PRs
Update #1870
Update #3542
Have you read the Contributing Guidelines?
Brief description of what is fixed or changed
Other comments
Summary by CodeRabbit
"Bug Fix":
protoc-gen-openapiv2/internal/genopenapi/template.go. The program will now exit with a non-zero status code when thefile.SourceCodeInfois nil, preventing potential silent failures.fmt.Fprintlnwithlog.Fatalffor more informative error messages and immediate program termination on fatal conditions.