-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
wip: Fix nil SourceCodeInfo in descriptor.File #3573
Conversation
WalkthroughThe changes primarily focus on error handling in the Changes
TipsChat with CodeRabbit Bot (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 <mr.imuz@gmail.com>
32e4156
to
73895b1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.Fprintln
tolog.Fatalf
alters the behavior of the function whenfile.SourceCodeInfo
is 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 whereprotoComments
is used.
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
protoComments
function, if thefile.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 containa nil
SourceCodeInfo
, providing better error handling.Signed-off-by: aimuz mr.imuz@gmail.com
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.SourceCodeInfo
is nil, preventing potential silent failures.fmt.Fprintln
withlog.Fatalf
for more informative error messages and immediate program termination on fatal conditions.