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

Using sessions, the body/payload/multipart of the previous request is sent if not replaced. #1171

Open
simue opened this issue Feb 3, 2025 · 3 comments · May be fixed by #1179
Open

Using sessions, the body/payload/multipart of the previous request is sent if not replaced. #1171

simue opened this issue Feb 3, 2025 · 3 comments · May be fixed by #1179
Labels

Comments

@simue
Copy link
Contributor

simue commented Feb 3, 2025

Description

I am using a session to send multiple sequential requests. Some are GET, some are POST.
On a GET that is sent after a POST, I see that the content_ of the first request being used as custom method, see example below.

[03/Feb/2025 22:33:14] code 501, message Unsupported method ('{"thisIsMyJsonBody": "OfThePreviousPostRequest"}GET')

I'd expect the session to either internally reset content_(maybe in makeRequest?), or if not possible, I'd expect there to be the possibility of resetting content_ explicitly.
Since content_'s default is std::monostate, I'd like to introduce a reset function, setting std::monostate.

Interestingly enough, there already is a function hasBodyOrPayload which generates a custom GET request and a comment stating GET with body is illegal. But no setter/reset for content_.

Example/How to Reproduce

  1. Create a cpr::Session
  2. Send POST
  3. Send GET (without changing content_ via its various setters)
  4. Perform the request
  5. Observe method shown in server which contains the body of the POST request.

Possible Fix

See above, I'd like to introduce a reset function for content_.

Where did you get it from?

conan

Additional Context/Your Environment

  • OS: Linux
  • Version: custom on kernel 5.15
@simue
Copy link
Contributor Author

simue commented Feb 6, 2025

I did some testing and have two observations:

  • DifferentMethodTests.MultipleDeleteHeadPutGetPostTest reliably failed at a seemingly arbitrary, but reproducible point (?!) when I had a proxy set (still need to investigate this further).
  • Even though a body (and relevant headers) is sent, the test case is still succeeding

I started modifying the test server so it always responds with some (base64 encoded?) json that contains information on what the request looked like to the server (all headers listed + full body). For tests that need to receive a custom message as well, that could also be placed in the json in a node, e.g. "userData". Leading to:

{
	"request": {
		"start_line": "POST /api/v1/echo HTTP/1.1",
		"headers": [
			{ "User-Agent": "curl/8.10.1" },
			{ "Content-Type": "application/json" },
			{ "Content-Length": "100" }
		],
		"body": "bodyAsPlainText(?)"
	},
	"userData": ""
}

But since this requires some work, I'd want to hear your opinion on this first.

@COM8
Copy link
Member

COM8 commented Feb 8, 2025

@simue thanks for reporting and looking into this!

Yes, a clear content_ function would be a welcome addition.
Your approach with returning what has been sent to the sever is a valid one. If I think about it, I think it fits better into the current way of doing tests if we would check this on the mock server side to validate this and then return a specific error code.

What do you think about this?

@simue simue linked a pull request Feb 13, 2025 that will close this issue
@simue
Copy link
Contributor Author

simue commented Feb 13, 2025

I like the approach you suggested and implemented it in #1179.
Let me know if/what you'd like to have changed.

I consider this a bugfix for 1.11. Since for other PRs you seem to be working towards 1.12.0 - are you open to do another 1.11.x bugfix release?
For that changes from #1173 would be quite handy, as well

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

Successfully merging a pull request may close this issue.

2 participants