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

Add support for adding cookies as headers in MockHttpServletResponse [SPR-17110] #21647

Closed
spring-projects-issues opened this issue Aug 1, 2018 · 9 comments
Assignees
Milestone

Comments

@spring-projects-issues
Copy link
Collaborator

@spring-projects-issues spring-projects-issues commented Aug 1, 2018

Vedran Pavic opened SPR-17110 and commented

Adding cookies that contain directives that aren't directly supported in Servlet API, such as SameSite for example, requires writing cookie directly using HttpServletResponse#addHeader, instead of commonly used HttpServletResponse#addCookie.

As MockHttpServletResponse doesn't have any special treatment for Set-Cookie header (i.e. doesn't parse and add cookies that were added directly as headers), testing of code that writes cookies in the manner described above is quite a painful experience.


Affects: 4.3.18, 5.0.8, 5.1 RC1

Issue Links:

  • #21854 MockCookie parsing fails if cookie does not include attributes

Referenced from: pull request #1913, and commits bb2db87

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Aug 1, 2018

Vedran Pavic commented

Updated issue with Pull Request URL.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Aug 1, 2018

Rossen Stoyanchev commented

Since MockHttpServletResponse writes the cookie header, couldn't we create an extension of Cookie with the extra properties in the same package, for testing purposes? We could call it MockCookie for consistency, or RfcTestCookie perhaps.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Aug 1, 2018

Vedran Pavic commented

Thanks for the feedback Rossen.

I agree that the custom Cookie implementation would provide the best developer experience here. That would also eliminate all the workarounds we had to do in Spring Session tests while implementing support for SameSite cookie directive in our Servlet API based CookieSerializer.

I'll pick this up, and update the PR over the next day or two.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Aug 1, 2018

Rossen Stoyanchev commented

Okay sounds good.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Aug 2, 2018

Vedran Pavic commented

I've updated the PR - Rossen Stoyanchev please take a look when you can and let me know if the direction is good.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Aug 2, 2018

Rossen Stoyanchev commented

Hm do we need the parsing? What about enhancing addCookie and getCookieHeader to detect MockCookie and also append sameSite to the header value? Is this because the code under test sets the header directly?

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Aug 3, 2018

Vedran Pavic commented

Yes, it is because the cookie is being set as a Set-Cookie header and needs to be added to MockHttpServletResponse#cookies.

I went with MockCookie#parse approach because I though that was consistent which handling of other special cases in MockHttpServletResponse#setSpecialHeader and the fact that we need to be able to retrieve cookie set as a Set-Cookie header using HttpServletResponse#getCookie.

I see your point on enhancing MockHttpServletResponse#getCookieHeader though, I'll address that.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Aug 3, 2018

Vedran Pavic commented

I've updated the PR with MockHttpServletResponse#getCookieHeader enhancement to detect MockCookie and properly render it (i.e. include additional directives).

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Oct 1, 2018

Michael Bell commented

Introduces regression due to bad parse method in MockCookie - see #21854

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

Successfully merging a pull request may close this issue.

None yet
2 participants
You can’t perform that action at this time.