-
-
Notifications
You must be signed in to change notification settings - Fork 4.1k
feat: add option to control using proto text name as key during URL encoding #3311
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
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #3311 +/- ##
==========================================
+ Coverage 81.83% 82.08% +0.25%
==========================================
Files 92 93 +1
Lines 4205 4220 +15
==========================================
+ Hits 3441 3464 +23
+ Misses 582 578 -4
+ Partials 182 178 -4 ☔ View full report in Codecov by Sentry. |
6010b33 to
206cae0
Compare
4e5659c to
b417873
Compare
1c38aba to
b1b008c
Compare
b1b008c to
efd7345
Compare
efd7345 to
4f166c9
Compare
4f166c9 to
2432b81
Compare
2432b81 to
639c2d3
Compare
|
Please take a look at this if you have time @shenqidebaozi. For users, there will be deviations in the use of EncodeURL. There are already multiple issues that have feedback on this. |
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.
Why do we need to separate this package? It looks strange to see it here
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.
It's true that a single package doesn't make much sense, it's already moved out @shenqidebaozi
1ccdc98 to
4857741
Compare
|
Is it necessary to process in encoding and can it be processed in gen code |
What does |
4857741 to
a6fedb0
Compare
…name as key during URL encoding
a6fedb0 to
a04a1a0
Compare
Description (what this PR does / why we need it):
binding.EncodeURLandform.EncodeValuesandform.EncodeFieldMaskaddform.EncodeOption, used to control whether to use proto text name as key when encoding. and it will not affect the original logiccmd/protoc-gen-go-httpadd the-prototext_encodeurlparameter to control whether to use the json tag defined in proto as the key when encoding. The default is false and will not change the original generated code logic. (example:kratos proto client api/test.proto -- --go-http_opt=prototext_encodeurl=true)func (c *GreeterHTTPClientImpl) SayHello(ctx context.Context, in *HelloRequest, opts ...http.CallOption) (*HelloReply, error) { var out HelloReply pattern := "/helloworld/{name}" - path := binding.EncodeURL(pattern, in, true) + path := binding.EncodeURL(pattern, in, true, form.Encode().UseProtoTextAsKey(false)) opts = append(opts, http.Operation(OperationGreeterSayHello)) opts = append(opts, http.PathTemplate(pattern)) err := c.cc.Invoke(ctx, "GET", path, nil, &out, opts...) if err != nil { return nil, err } return &out, nil }Which issue(s) this PR fixes (resolves / be part of):
resolves: #3141
resolves: #2761
resolves: #3331
resolves: #3560
Other special notes for the reviewers: