-
Notifications
You must be signed in to change notification settings - Fork 148
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
Feature/http 2 http param mapping #57
Feature/http 2 http param mapping #57
Conversation
63555c1
to
15dc11f
Compare
Codecov Report
@@ Coverage Diff @@
## develop #57 +/- ##
===========================================
- Coverage 45.74% 43.71% -2.04%
===========================================
Files 21 28 +7
Lines 929 1217 +288
===========================================
+ Hits 425 532 +107
- Misses 454 613 +159
- Partials 50 72 +22
Continue to review full report at Codecov.
|
15dc11f
to
327372f
Compare
327372f
to
a678ce7
Compare
pkg/client/dubbo/mapper.go
Outdated
@@ -30,19 +30,30 @@ import ( | |||
) | |||
|
|||
import ( | |||
"encoding/json" |
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.
position not ok
pkg/client/dubbo/mapper.go
Outdated
return nil, nil | ||
type uriMapper struct{} | ||
|
||
func (um uriMapper) Map(mp config.MappingParam, c client.Request, target interface{}) error { |
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.
Map is public, add // nolint
?
pkg/client/http/mapper.go
Outdated
|
||
type bodyMapper struct{} | ||
|
||
func (bm bodyMapper) Map(mp config.MappingParam, c client.Request, rawTarget interface{}) error { |
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.
// nolint
pkg/router/api.go
Outdated
import ( | ||
"github.com/dubbogo/dubbo-go-proxy/pkg/config" | ||
"strings" |
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.
move to first import part
pkg/router/api_test.go
Outdated
package router | ||
|
||
import ( | ||
"github.com/dubbogo/dubbo-go-proxy/pkg/config" |
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.
split import
pkg/router/route.go
Outdated
@@ -30,6 +30,7 @@ import ( | |||
import ( | |||
"github.com/dubbogo/dubbo-go-proxy/pkg/common/constant" | |||
"github.com/dubbogo/dubbo-go-proxy/pkg/config" | |||
"net/url" |
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.
split import
r.Header = req.IngressRequest.Header.Clone() | ||
queryValues, err := url.ParseQuery(req.IngressRequest.URL.RawQuery) | ||
if err != nil { | ||
return nil, errors.New("Retrieve request query parameters failed") |
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.
I think we need create err together errors.New()
in next step.
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.
agree.
d4d4984
to
aa8f29d
Compare
pkg/client/dubbo/dubbo.go
Outdated
@@ -162,22 +154,22 @@ func (dc *Client) Call(req *client.Request) (res interface{}, err error) { | |||
return rst, nil | |||
} | |||
|
|||
// MappingParams param mapping to api. | |||
func (dc *Client) MappingParams(req *client.Request) ([]string, []interface{}, error) { | |||
// MapParams param mapping to api. |
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.
“s” is lost
pkg/client/dubbo/mapper.go
Outdated
type queryStringsMapper struct{} | ||
|
||
// nolint | ||
func (qm queryStringsMapper) Map(mp config.MappingParam, c client.Request, target interface{}) error { |
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 isn't the caller pointer?
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.
originally, I don't hope to modify anything inside the client.Request, but since I have to add back the body after mapping the request body, I think pointer might be better now.
What this PR does:
Which issue(s) this PR fixes:
Fixes #54 #56
Special notes for your reviewer:
Does this PR introduce a user-facing change?: