Skip to content
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

fix: buildCurlRequest() replace req.Body with req.GetBody() #844

Closed
wants to merge 1 commit into from

Conversation

tttturtle-russ
Copy link

The `TestGenerateExecutedCurl` failure started after I merged from branch `v2` to branch `main`. It needs to investigate why it's failing.

Originally posted by @jeevatkm in #827 (comment)

Hi jeevatkm, I've found the reason why the test case failed.
In

resty/client.go

Line 1601 in 6d941ac

resp, err := c.httpClient.Do(req.RawRequest)
Do method will read the req.RawRequest.Body and make it empty. When generating curl body it will read an empty string, and that's why it failed.

This commit offers a temporary solution to this problem, I think the ultimate solution is to add a Clone method in Request, as #718 describes.

Copy link

codecov bot commented Sep 3, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.67%. Comparing base (82ae758) to head (24ae356).
Report is 13 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #844      +/-   ##
==========================================
- Coverage   96.89%   96.67%   -0.22%     
==========================================
  Files          14       13       -1     
  Lines        1773     2048     +275     
==========================================
+ Hits         1718     1980     +262     
- Misses         35       46      +11     
- Partials       20       22       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@jeevatkm jeevatkm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tttturtle-russ Let's decline this PR and create a new PR with adding middleware would solve. Also, while on PR #842, I noticed, the CURL follow needs an improvement I will work on later.

// is empty after Do method.
// body here is a copy of original body
body, _ := req.GetBody()
buf, _ := io.ReadAll(body)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tttturtle-russ Thanks for creating a PR. Last night, I discovered the branch main was missing curl middleware. It seems it happened when I merged the branch v2 into branch main.

Branch main

resty/resty.go

Lines 195 to 196 in 6d941ac

addCredentials,
}

Branch v2

resty/client.go

Lines 1484 to 1486 in 370d744

addCredentials,
createCurlCmd,
}

@tttturtle-russ
Copy link
Author

@jeevatkm OK, I will close this PR and create a new one with curl middleware added.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants