Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat: wire v2 handlers #22112
feat: wire v2 handlers #22112
Changes from 8 commits
233a249
d7202e4
f1d7ebe
75e1924
86d450f
f0af908
22b9f92
45b5d74
a67fcc9
3007e67
b4027dd
85bd55a
f22a135
5d05234
ee8d0a4
40b5bae
52d57cf
a733c83
8d78a2c
8dc09a3
c186af2
2cf4c62
9e6b8fb
ac25a49
9dd5c9f
2b028fd
1c7d32c
506267d
a8a173e
3608feb
c700ad9
db3ac74
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Check notice
Code scanning / CodeQL
Sensitive package import Note
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.
Validate
Content-Type
header instead of defaultingCurrently, if the
Content-Type
is notapplication/json
, the handler defaults it toapplication/json
. This may lead to unexpected behavior if the client sends data with an incorrect content type.Consider validating the
Content-Type
and returning a415 Unsupported Media Type
error if it's notapplication/json
:📝 Committable suggestion
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.
🛠️ Refactor suggestion
Minimize the use of reflection for better performance and safety
Reflection can introduce performance overhead and potential runtime errors. According to the Uber Go Style Guide, reflection should be used carefully.
Consider alternative approaches to create new message instances without reflection. For example, maintain a registry or map of constructor functions keyed by message type.
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.
Consolidate error handling when parsing the request body
Using both
http.Error
andfmt.Fprintf
to write error responses can result in malformed HTTP responses because headers may have already been sent. Instead, send a single, well-formatted error response.Modify the error handling to return a JSON response with error details:
📝 Committable suggestion
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.
super nit, for consistency for the other, can we get the logger error lowercase?
additionally, could you fix conflicts?
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.
Check if the server is enabled before starting
Currently, the
Start()
method starts the HTTP server without checking whether the server is enabled in the configuration. This could lead to the server running when it shouldn't be.Apply this diff to add the enable check:
📝 Committable suggestion
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.
Notify the caller if the HTTP server fails to start
In the goroutine, if
ListenAndServe()
returns an error other thanhttp.ErrServerClosed
, the error is only logged but not returned to the caller. This means the caller has no way of knowing that the server failed to start.Consider modifying the
Start()
method to capture the error and return it to the caller. Here's how you might adjust the code:This way, the
Start()
method will wait for the server to start and report any errors back to the caller.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.
Correct the comparison in the
Config()
methodIn line 78, the comparison
s.config == (&Config{})
is comparing the pointers.config
to a new pointer&Config{}
. This will always evaluate tofalse
because they point to different addresses. Instead, you should compare the value thats.config
points to with an emptyConfig
struct to check if it is uninitialized.Apply this diff to fix the comparison:
📝 Committable suggestion