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

Adds "remove cookie" function to request module #57

Merged
merged 5 commits into from
Sep 12, 2024

Conversation

MaxHill
Copy link

@MaxHill MaxHill commented Aug 30, 2024

This pull request adds a remove_cookie function to the request.
This is useful if you want to for example override a cookie sent from the browser. Since set_cookie appends cookies. Existing cookies in the header would therefore take presidence over the newly set cookies. This might be desirable but the remove_cookie function is a solution in the case that it's not.

@lpil
Copy link
Member

lpil commented Aug 30, 2024

I think this would be confusing in that people would expect it to remove the cookie from the session, but it does not.

What's your use case here?

@MaxHill
Copy link
Author

MaxHill commented Aug 30, 2024

That's true but isn't that already how request.set_cookie() works? It only sets it for the request and not the response? :)

My specific use case is:
I'm developing a session library for wisp. And one part of this is a middleware that creates a session if no sessison exists. This to have it so there's always a session to store data in.

In doing this I need to inject a session_cookie (containing the session id) in the request if that cookie does not already exist. (This cookie will then also be added to the response after the handler has ran). I do this using request.set_cookie and it works fine as long as there isn't already a cookie by that name in the request. Then the new injected cookie will be appended and the old cookie will be found first when doing a list.key_find (which is what's happening behind the scenes in wisp.get_cookie()).
This lead me to creating this function to first remove the already set cookie if it exist and then replace it with the new.

I think in general it would be most useful for replacing cookies in some way so maybe it's more of a utility function than something that should be in the main library, what do you think?

Copy link
Member

@lpil lpil left a comment

Choose a reason for hiding this comment

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

OK, you've convinced me :)

Could you document that this does not remove it from the client please.

let new_cookies_string =
string.split(cookies_string, ";")
|> list.map(string.trim)
|> list.filter(fn(str) { string.starts_with(str, name) |> bool.negate })
Copy link
Member

Choose a reason for hiding this comment

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

In case you didn't know, you can use ! rather than bool.negate, it's a bit shorter.

Copy link
Author

Choose a reason for hiding this comment

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

I did not, nice! Updated in next commit :)

|> request.get_header("cookie")
|> should.be_ok
|> should.equal("FIRST_COOKIE=first; THIRD_COOKIE=third")
}
Copy link
Member

Choose a reason for hiding this comment

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

Could we have a test to check that if you try and remove "SECOND" it won't remove "SECOND_COOKIE" please 🙏

Copy link
Author

Choose a reason for hiding this comment

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

Oh, good catch that was a bug. I had to rework the function a tiny bit to fix it. Update pushed.

Copy link
Member

Choose a reason for hiding this comment

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

Thank you

@@ -1,5 +1,6 @@
import gleam/http.{type Header, type Method, type Scheme, Get}
import gleam/http/cookie
import gleam/io
Copy link
Member

Choose a reason for hiding this comment

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

I think this was for debugging?

Copy link
Author

Choose a reason for hiding this comment

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

Yep, It slipped through 😅

Copy link
Member

@lpil lpil left a comment

Choose a reason for hiding this comment

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

Beautiful! Would you mind updating the changelog?

@MaxHill MaxHill force-pushed the add_remove_cookie_function_to_request branch from 63e7161 to d471e09 Compare September 10, 2024 19:35
@MaxHill
Copy link
Author

MaxHill commented Sep 10, 2024

Yes, absolutely, is this correct?

Copy link
Member

@lpil lpil left a comment

Choose a reason for hiding this comment

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

Thank you

@lpil lpil merged commit 40d4ae4 into gleam-lang:main Sep 12, 2024
1 check passed
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.

2 participants