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

ZIO HTTP Server should not fold multiple Set-Cookie headers #3723

Merged
merged 2 commits into from
Apr 26, 2024

Conversation

mkubala
Copy link
Member

@mkubala mkubala commented Apr 25, 2024

This change makes the behaviour of handling multiple Set-Cookie headers consistent among multiple server backends.

See #3654 for more details.

This change makes the behaviour of handling multiple Set-Cookie headers consistent among multiple server backends.

See #3654 for more details.
@mkubala mkubala requested review from adamw and kciesielski April 25, 2024 13:48

def tests(): List[Test] = requestTests

val requestTests = List(
Copy link
Member

Choose a reason for hiding this comment

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

suggestion: Since this is ZIO-specific, it could go directly to ZioHttpServerTest.additionalTests() and you won't need the cookieHeaders flag.

Copy link
Member Author

@mkubala mkubala Apr 25, 2024

Choose a reason for hiding this comment

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

I though it might be good to do the way it's currently done, since it does not only reproduce the ZIO-specific bug but also guard consistency among different backends' behaviour. Disclaimer: I do not have a strong opinion on this, I just want to put a spotlight on my initial motivation.

Copy link
Member

Choose a reason for hiding this comment

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

yeah let's test it in all backends, as we want all backends to behave this way (separating the headers)

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I didn't notice the flag is always true, so this means this test is executed for all the servers, right? In that case, I second Adam's opinion that the flag could be removed. And disregard my comment about moving the test to ZioHttpServerTest.

@@ -28,7 +28,8 @@ class AllServerTests[F[_], OPTIONS, ROUTE](
oneOfBody: Boolean = true,
cors: Boolean = true,
options: Boolean = true,
maxContentLength: Boolean = true
maxContentLength: Boolean = true,
cookieHeaders: Boolean = true
Copy link
Member

Choose a reason for hiding this comment

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

if this is always enabled, I guess we can drop the flag?

@mkubala mkubala merged commit 9e819cd into master Apr 26, 2024
28 checks passed
@mkubala mkubala deleted the unify-multiple-set-cookies-headers-handling branch April 26, 2024 07:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants