-
Notifications
You must be signed in to change notification settings - Fork 719
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(curl): generate curl cmd for request && example for curl cmd #794
Conversation
this PR is very needed!! |
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.
@ahuigo - I'm sorry for the delayed response.
Thanks for extracting and creating a new PR. I have added my comments; could you update the PR? and then we can merge it.
util_curl.go
Outdated
"github.com/go-resty/resty/v2/shellescape" | ||
) | ||
|
||
func BuildCurlRequest(req *http.Request, httpCookiejar http.CookieJar) (curl string) { |
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.
@ahuigo, please un-export this method to the package.
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.
ok
request.go
Outdated
@@ -333,6 +348,12 @@ func (r *Request) SetResult(res interface{}) *Request { | |||
return r | |||
} | |||
|
|||
// This method is to register curl cmd for request executed. | |||
func (r *Request) SetResultCurlCmd(curlCmd *string) *Request { |
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.
@ahuigo - please remove this method and the related implementation code.
Resty could provide a cURL command generation on-demand instead of implicit via -
resp, err := client.R().
EnableTrace().
Get("https://httpbin.org/get")
curlCmd := resp.Request.GenerateCurlCommand()
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've done this as you said.
It needs to be explained that without SetResultCurlCmd
, GenerateCurlCommand
must rely on EnableTrace
.
This is because if EnableTrace()
is not called, GenerateCurlCommand
cannot obtain the body.
request.go
Outdated
@@ -73,6 +74,20 @@ type Request struct { | |||
retryConditions []RetryConditionFunc | |||
} | |||
|
|||
func (r *Request) GetCurlCmd() string { |
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.
@ahuigo Please refactor this method with the name GenerateCurlCommand
and update this method logic to on-demand cURL command generation.
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.
done
@@ -307,6 +307,13 @@ func addCredentials(c *Client, r *Request) error { | |||
return nil | |||
} | |||
|
|||
func createCurlCmd(c *Client, r *Request) (err 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.
@ahuigo Please remove this middleware.
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.
Deleting this middleware will cause GenerateCurlCommand
to be unable to get the body (because resty will delete the body after executing the request)
// 3. Generate curl body
if req.Body != nil {
buf, _ := io.ReadAll(req.Body)
req.Body = io.NopCloser(bytes.NewBuffer(buf)) // important!!
curl += `-d ` + shellescape.Quote(string(buf))
}
Considering that I need to record body before the request is executed(only when EnableTrace()
is called),
I need to read body in middleware.
func createCurlCmd(c *Client, r *Request) (err error) {
if r.trace {
if r.resultCurlCmd==nil{
r.resultCurlCmd = new(string)
}
*r.resultCurlCmd = buildCurlRequest(r.RawRequest, c.httpClient.Jar)
}
return nil
}
I think the best time to record the body is when the middleware request occurs.
- If it is before the middleware, parameters such as url and authorization will be lost.
- If it is after the middleware, the body parameters will be lost.
Do you have any other better suggestions?
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.
@ahuigo I understand. For now, keep this conditional execution for createCurlCmd
method with r.trace
enabled. I will see what could be improved in v3.
@@ -1396,6 +1403,7 @@ func createClient(hc *http.Client) *Client { | |||
parseRequestBody, | |||
createHTTPRequest, | |||
addCredentials, | |||
createCurlCmd, |
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.
@ahuigo Please remove this middleware.
8de5faf
to
b2969a3
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.
@ahuigo Thanks for incorporating the review comments. I have added one new comment in the shellescape.go
file. Please have a look.
@@ -307,6 +307,13 @@ func addCredentials(c *Client, r *Request) error { | |||
return nil | |||
} | |||
|
|||
func createCurlCmd(c *Client, r *Request) (err 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.
@ahuigo I understand. For now, keep this conditional execution for createCurlCmd
method with r.trace
enabled. I will see what could be improved in v3.
shellescape/shellescape.go
Outdated
The original Python package which this work was inspired by can be found | ||
at https://pypi.python.org/pypi/shellescape. | ||
*/ | ||
package shellescape // "import gopkg.in/alessio/shellescape.v1" |
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.
@ahuigo Can we remove this line?
On the line, godoc has different meanings to it.
// "import gopkg.in/alessio/shellescape.v1"
If we learned something from this gopkg.in/alessio/shellescape.v1
library, please capture that acknowledgment in the above godoc instead of here.
Otherwise remove it.
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.
@ahuigo Thanks for updating PR and your contribution.
@ahuigo It seems the PR check build failed at |
1. refactor `GetCurlCommand` with the name `GenerateCurlCommand` 2. un-export this method `BuildCurlRequest` 3. remove SetResultCurlCmd
@jeevatkm Fixed |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## v2 #794 +/- ##
==========================================
+ Coverage 96.53% 96.67% +0.13%
==========================================
Files 12 14 +2
Lines 1673 1742 +69
==========================================
+ Hits 1615 1684 +69
Misses 37 37
Partials 21 21 ☔ View full report in Codecov by Sentry. |
@jeevatkm Fixed.
In order to allow coverage to calculate imported packages, we need to add this parameter(
|
…kages that are imported in different packages
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.
Thanks, @ahuigo, for taking care of the test and CI argument.
[![Mend Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com) This PR contains the following updates: | Package | Change | Age | Adoption | Passing | Confidence | |---|---|---|---|---|---| | [github.com/go-resty/resty/v2](https://togithub.com/go-resty/resty) | `v2.13.1` -> `v2.14.0` | [![age](https://developer.mend.io/api/mc/badges/age/go/github.com%2fgo-resty%2fresty%2fv2/v2.14.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://developer.mend.io/api/mc/badges/adoption/go/github.com%2fgo-resty%2fresty%2fv2/v2.14.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://developer.mend.io/api/mc/badges/compatibility/go/github.com%2fgo-resty%2fresty%2fv2/v2.13.1/v2.14.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://developer.mend.io/api/mc/badges/confidence/go/github.com%2fgo-resty%2fresty%2fv2/v2.13.1/v2.14.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | --- ### Release Notes <details> <summary>go-resty/resty (github.com/go-resty/resty/v2)</summary> ### [`v2.14.0`](https://togithub.com/go-resty/resty/releases/tag/v2.14.0) [Compare Source](https://togithub.com/go-resty/resty/compare/v2.13.1...v2.14.0) ### Release Notes #### New Features - feat(curl): generate curl cmd for request && example for curl cmd by [@​ahuigo](https://togithub.com/ahuigo) in [https://github.com/go-resty/resty/pull/794](https://togithub.com/go-resty/resty/pull/794) #### Enhancements - build: update bazel config with new files by [@​jeevatkm](https://togithub.com/jeevatkm) in [https://github.com/go-resty/resty/pull/800](https://togithub.com/go-resty/resty/pull/800) - chore: dependency and version update v2.14.0 by [@​jeevatkm](https://togithub.com/jeevatkm) in [https://github.com/go-resty/resty/pull/816](https://togithub.com/go-resty/resty/pull/816) #### Upstream Fixes - update net package for vuln CVE-2023-45288 by [@​shedyfreak](https://togithub.com/shedyfreak) in [https://github.com/go-resty/resty/pull/804](https://togithub.com/go-resty/resty/pull/804) #### Test Cases - fix(examples): wrongly stderr written as stdout by [@​ahuigo](https://togithub.com/ahuigo) in [https://github.com/go-resty/resty/pull/801](https://togithub.com/go-resty/resty/pull/801) #### Documentation - fix: change resty.GET to resty.MethodGet in doc comment by [@​autopp](https://togithub.com/autopp) in [https://github.com/go-resty/resty/pull/803](https://togithub.com/go-resty/resty/pull/803) - resty dev version number and year update by [@​jeevatkm](https://togithub.com/jeevatkm) in [https://github.com/go-resty/resty/pull/799](https://togithub.com/go-resty/resty/pull/799) #### New Contributors - [@​ahuigo](https://togithub.com/ahuigo) made their first contribution in [https://github.com/go-resty/resty/pull/794](https://togithub.com/go-resty/resty/pull/794) - [@​autopp](https://togithub.com/autopp) made their first contribution in [https://github.com/go-resty/resty/pull/803](https://togithub.com/go-resty/resty/pull/803) - [@​shedyfreak](https://togithub.com/shedyfreak) made their first contribution in [https://github.com/go-resty/resty/pull/804](https://togithub.com/go-resty/resty/pull/804) **Full Changelog**: go-resty/resty@v2.13.1...v2.14.0 </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Enabled. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR was generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View the [repository job log](https://developer.mend.io/github/Michsior14/transmission-gluetun-port-update). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy40NDAuNyIsInVwZGF0ZWRJblZlciI6IjM3LjQ0MC43IiwidGFyZ2V0QnJhbmNoIjoibWFpbiIsImxhYmVscyI6WyJkZXBlbmRlbmNpZXMiXX0=--> Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
I've extracted curl code from my previous PR(#734).
Could you review this code?
New feature: generate curl command
Example: https://github.com/ahuigo/resty/blob/curl/v2/examples/debug_curl_test.go
Output