-
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
feat: support url form encoded marshaler and response in JSON #3711
base: main
Are you sure you want to change the base?
feat: support url form encoded marshaler and response in JSON #3711
Conversation
WalkthroughThe new Changes
Poem
Validation with GitHub issue(Beta)Assessment of the PR changes against GitHub Issue
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: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- runtime/marshal_urlencode.go (1 hunks)
Additional comments: 5
runtime/marshal_urlencode.go (5)
1-11: The import section looks clean and organized. All necessary packages for this file are imported correctly.
13-15: The
UrlEncodeMarshal
struct is defined correctly and implements theMarshaler
interface.18-20: The
ContentType
method correctly returns the content type of the response as "application/json".22-26: The
Marshal
method correctly uses theJSONPb
struct to marshal the response into JSON format.28-56: The
NewDecoder
method correctly reads the request body, parses it as form data, and populates the query parameters of the proto message using thePopulateQueryParameters
function. Error handling is also done correctly.
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: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- runtime/marshaler_registry.go (3 hunks)
Files skipped from review due to trivial changes (1)
- runtime/marshaler_registry.go
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: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- runtime/marshaler_registry.go (3 hunks)
Additional comments: 3
runtime/marshaler_registry.go (3)
16-16: The new MIME type constant
MIMEFormURLEncoded
is correctly defined.34-34: The
defaultFormMarshaler
is correctly assigned to an instance ofUrlEncodeMarshal{}
.102-102: The
MIMEFormURLEncoded
MIME type is correctly mapped to thedefaultFormMarshaler
in themimeMap
.
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.
Hey, thanks for this contribution! We'll definitely want to add some tests, perhaps you could look at adding an integration test in https://github.com/grpc-ecosystem/grpc-gateway/blob/main/examples/internal/integration/integration_test.go? There are some example tests in that you could take inspiration from. Presumably, you should be able to add a test against the ABitOfEverything endpoint with the query parameters in the body of the request. Let me know if you need more help!
return fmt.Errorf("not proto message") | ||
} | ||
|
||
formData, err := ioutil.ReadAll(r) |
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.
Use io.ReadAll
err = PopulateQueryParameters(msg, values, filter) | ||
|
||
if err != nil { | ||
return err | ||
} | ||
|
||
return nil |
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.
Since we're not mutating the error before returning, we can just
err = PopulateQueryParameters(msg, values, filter) | |
if err != nil { | |
return err | |
} | |
return nil | |
return PopulateQueryParameters(msg, values, filter) |
@mickael-kerjean if you want to contribute tests to this PR you may need to get access to @jhowliu's fork. |
Fixed #7. Support request in x-www-form-urlencoded and response in JSON format
Summary by CodeRabbit
New Features:
runtime/marshal_urlencode.go
with aUrlEncodeMarshal
struct that implements theMarshaler
interface. This allows for marshaling the response in JSON format using theJSONPb
struct and populating query parameters of the proto message.runtime/marshaler_registry.go
file to include support for the form URL-encoded MIME type, expanding the MIME type registry.Bug Fixes:
Documentation:
Refactor:
Style:
Tests:
Chores:
Revert: