🔥 Customize the source of session_id #1159
Conversation
middleware/session/session.go
Outdated
|
|
||
| if s.config.Source == SourceHeader { | ||
| s.ctx.Request().Header.SetBytesV(s.config.CookieName, []byte(s.id)) | ||
| s.ctx.Response().Header.SetBytesV(s.config.CookieName, []byte(s.id)) | ||
| } |
There was a problem hiding this comment.
A cookie will still be set even if the source is Header. We should put the above code in an if statement to prevent this. We should also rename the function to setSession or similar
There was a problem hiding this comment.
Thanks! It has been adjusted to setSession.
And also put the above code in an if statement.
leozhantw
force-pushed
the
feature/customize-session-ID-source
branch
2 times, most recently
from
55911fa
to
fec56d5
Compare
|
Hi, I'm a bit busy recently, sorry to reply to you so late. And I have made some adjustments, plz help me to confirm it again, thanks! |
|
Apologies for the delay, will review this tomorrow |
|
@leozhantw could you refresh the pull request and solve the change request ? then we can also merge this slowly^^ |
|
will do the review tonight |
leozhantw
force-pushed
the
feature/customize-session-ID-source
branch
from
fec56d5
to
fc28aa1
Compare
There was a problem hiding this comment.
not sure about one part i have marked in the review, otherwise the whole thing looks very good
although i think you could still optimize some things by adding benchmarks and look that you reduce the allocations
like for example when splitting the string, you could look instead of split with index checks, but that should be insignificant, because it is before the process of the requests and only affects the configuration
good work and thx
middleware/session/store.go
Outdated
| } | ||
|
|
||
| if s.source == SourceURLQuery { | ||
| id = c.FormValue(s.sessionName) |
There was a problem hiding this comment.
this should be the wrong function or ? if we want to get it from the query parameters it should be c.Query(s.sessionName)
There was a problem hiding this comment.
Although c.FormValue() will search the value from query string first,
but you are right, it should only search from query string.
Changed here to use c.Query(). Thanks you!
Ref
https://github.com/valyala/fasthttp/blob/v1.23.0/server.go#L1004-L1036
leozhantw
force-pushed
the
feature/customize-session-ID-source
branch
2 times, most recently
from
d3f4fcb
to
a630124
Compare
|
Thank you for the PR! Looks good 👍 |
|
@leozhantw can you update the pull request again with the master, i had unfortunately produced an incorrect test in the master in the meantime after that i will start the tests from PR again and merge them directly if they were successful |
leozhantw
force-pushed
the
feature/customize-session-ID-source
branch
from
8c0348b
to
6fa4f0e
Compare
Done~! |
Please provide enough information so that others can review your pull request:
Try to support the need of #1110.
Explain the details for making this change. What existing problem does the pull request solve?
I refer to gofiber/session and fasthttp/session to implement this requirement.
Commit formatting
Use emojis on commit messages so it provides an easy way of identifying the purpose or intention of a commit. Check out the emoji cheatsheet here: https://gitmoji.carloscuesta.me/