-
Notifications
You must be signed in to change notification settings - Fork 206
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
Actix session insert should check for string #308
Comments
Hey @LukeMathWalker is there any chance you can chime in on this? Perhaps I'm doing something wrong. But everything even integer32's are being inserted as strings, and strings are being stored as string wrapped strings. Examples in one of the redis sessions: {
"y": "0",
"id": "\"67464c5c-980d-4553-b0a1-0f91898e16aa\"",
"location": "10",
"x": "0",
} Instead what I would like is: {
"y": 0,
"id": "67464c5c-980d-4553-b0a1-0f91898e16aa",
"location": 10,
"x": 0,
} |
A question first: why is this an issue? |
@LukeMathWalker this is more of a curiosity thing / potentially a learning opportunity for me. I was passing data and deserializing data into structs from the session. I have to do extra |
@LukeMathWalker friendly bump on this in case it slipped through the cracks. |
This is not something I have time to follow right now. |
@LukeMathWalker just curious to hear your thoughts on this. |
This always get a panic ? I tried many times |
Bumping this again because any serde types that deserialize into a string will break here because they are double serialized but not double deserialized. And when you fix this, please write a test to ensure that things like strings, dates, and UUIDs correctly deserialize. This library is fundamentally broken without this support. |
Expected Behavior
To not store a string as string wrapped in a string. (extra double quotes). Unsure if there's some magic I haven't seen to prevent this.
https://github.com/actix/actix-extras/blob/master/actix-session/src/session.rs#L136-L147
We should check if the value is a string. If it's a string we should not use
serde_json::to_string
Current Behavior
Using
serde_json::to_string
on everything including values of type stringPossible Solution
it seems there may have been uniformity reasons for not allowing types other than strings being stored, but I believe this makes the middleware less useful and more awkward.
Steps to Reproduce (for bugs)
Context
Double stringified strings result in all those values needing to be doubly unwrapped which is awkward. Would be nice to let users decide what kind of values they want to store and not wrap everything as string.
Your Environment
rustc -V
):Latest stable.
The text was updated successfully, but these errors were encountered: