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

Unable to add cookie in redirectRequestBiConsumer #3035

Closed
istibekesi opened this issue Jan 22, 2024 · 1 comment · Fixed by #3039
Closed

Unable to add cookie in redirectRequestBiConsumer #3035

istibekesi opened this issue Jan 22, 2024 · 1 comment · Fixed by #3039
Assignees
Labels
type/bug A general bug warn/regression A regression from a previous release
Milestone

Comments

@istibekesi
Copy link

istibekesi commented Jan 22, 2024

Expected Behavior

According to the javadoc of followRedirect:
Param redirectRequestBiConsumer – BiConsumer, invoked on redirects, after the redirect request has been initialized, in order to apply further changes such as add/remove headers and cookies.

Actual Behavior

Redirect headers are added, redirect cookies are missing.

Steps to Reproduce

	@Test
	void myTest() {
		disposableServer =
			createServer()
				.host("localhost")
				.route(r -> r.get("/1", (req, res) -> res.sendRedirect("/3"))
					.get("/3", (req, res) -> {
						if (req.cookies().containsKey("myCookie")) {
							return res.status(200).sendString(Mono.just("OK"));
						} else {
							return res.status(400).sendString(Mono.just("NOK"));
						}
					}))
				.wiretap(false)
				.bindNow();

		HttpClientResponse response =
			createClient(disposableServer::address)
				.wiretap("", LogLevel.INFO, AdvancedByteBufFormat.TEXTUAL)
				.followRedirect(
					(req, res) -> true,
					(headers, redirect) -> {
						redirect.addHeader("myHeader", "myHeader");
						redirect.addCookie(new DefaultCookie("myCookie", "myCookie"));
					}
				)
				.get()
				.uri("/1")
				.response()
				.block(Duration.ofSeconds(30));

		assertThat(response).isNotNull();
		assertThat(response.status()).isEqualTo(HttpResponseStatus.OK);
	}

Log:

09:14:17.154 [reactor-http-nio-3] INFO   - [b9a9d5df-1, L:/127.0.0.1:65266 - R:/127.0.0.1:65263] WRITE: 124B GET /3 HTTP/1.1
user-agent: ReactorNetty/dev
host: 127.0.0.1:65263
accept: */*
myHeader: myHeader
content-length: 0

Note: on 1.0.34 the above test passed, but fails on 1.1.x

@istibekesi istibekesi added status/need-triage A new issue that still need to be evaluated as a whole type/bug A general bug labels Jan 22, 2024
@violetagg violetagg self-assigned this Jan 23, 2024
@violetagg violetagg removed the status/need-triage A new issue that still need to be evaluated as a whole label Jan 23, 2024
@violetagg violetagg added this to the 1.0.42 milestone Jan 24, 2024
@violetagg violetagg added the warn/regression A regression from a previous release label Jan 24, 2024
violetagg added a commit that referenced this issue Jan 24, 2024
…llowRedirect

This is a regression caused by PR #2994

Fixes #3035
@violetagg
Copy link
Member

@istibekesi Thanks for the reproducible example! I'm observing the issue with both 1.0.x and 1.1.x versions. It is a regression caused by PR #2994

The PR #3039 should fix it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/bug A general bug warn/regression A regression from a previous release
Projects
None yet
2 participants