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

Removed reset process for request Body for temporary support #342

Closed
wants to merge 12 commits into from

Conversation

k2tzumi
Copy link
Collaborator

@k2tzumi k2tzumi commented Dec 2, 2022

Triggers for Change

https://github.com/k1LoW/runn/releases/tag/v0.51.0

The previously addressed issue has reappeared in the above version.

#91

The following library versions have changed due to the pkgs update

github.com/getkin/kin-openapi@v0.106.0 -> github.com/getkin/kin-openapi@v0.109.0

The contents of the release are as follows
https://github.com/getkin/kin-openapi/releases/tag/v0.109.0

What we verified

  1. If skipValidateRequest: false, no error occurred
  2. Downgraded to github.com/getkin/kin-openapi@v0.108.0 version and no more errors

Cause Considerations

We believe the error recurred due to the following PR
getkin/kin-openapi#672

The request body reset process in #91 is no longer needed due to modifications to the library itself.
On the contrary, the inclusion of provisional support seems to have caused the content length to be caught in the content length check.

Changes

Deleted the process supported by #91
My project is working well with this response.

Worries

There may be an impact on the functionality of Multipart Form Data

@k2tzumi
Copy link
Collaborator Author

k2tzumi commented Dec 2, 2022

Oh. What I feared happened.
https://github.com/k1LoW/runn/actions/runs/3601501427/jobs/6067375050#step:7:35

I couldn't reproduce it in the last test, but I didn't think it would be at a time like this. 😅

@k1LoW k1LoW added the bug Something isn't working label Dec 2, 2022
@atsushi-ishibashi
Copy link
Contributor

atsushi-ishibashi commented Dec 4, 2022

How about using io.TeeReader?
I don't still understand the cause of this PR so teeReader maybe not the solution but I also would like to improve the current re-create way

@atsushi-ishibashi
Copy link
Contributor

how about this?

bw := new(bytes.Buffer)
req, err = http.NewRequestWithContext(ctx, r.method, u.String(), io.TeeReader(reqBody, bw))
if err != nil {
	return err
}
r.setContentTypeHeader(req)
for k, v := range r.headers {
	req.Header.Set(k, v)
}
rnr.operator.capturers.captureHTTPRequest(rnr.name, req)
...

req, err = http.NewRequestWithContext(ctx, r.method, u.String(), bw)
if err != nil {
	return err
}
r.setContentTypeHeader(req)
for k, v := range r.headers {
	req.Header.Set(k, v)
}
res, err = rnr.client.Do(req)
if err != nil {
	return err
}

@k2tzumi
Copy link
Collaborator Author

k2tzumi commented Dec 5, 2022

@atsushi-ishibashi

Thank you for the suggested modifications.

The following PR resolved the problem
#345

I will keep this in mind for future reference.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants