-
Notifications
You must be signed in to change notification settings - Fork 95
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
Support duplicate header keys in session files #313
Conversation
fef21e8
to
0cea500
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When the session file has multiple headers only the first one is sent. Replacing headers.entry(key).or_insert_with(|| value.clone());
by headers.append(key, value.clone());
in main.rs
fixes it. (Could probably use a test!)
src/session.rs
Outdated
@@ -49,18 +49,46 @@ pub struct Cookie { | |||
secure: Option<bool>, | |||
} | |||
|
|||
#[derive(Debug, Serialize, Deserialize)] | |||
struct Header { | |||
key: String, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
HTTPie calls these name
, not key
, so they fail to load each other's sessions. Maybe we can copy a file from HTTPie's test suite?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, that is a good idea 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update: I haven't used a verbatim test file from HTTPie since we are not yet 100% compatible with the current session format. I am planning to address this in a follow-up PR that adds support for the new style cookies introduced in httpie/cli#1312
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not yet 100% compatible
Do you mean that we crash or that we interpret the session files incorrectly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We would fail to load a session where cookies are stored as a list. For example:
tests/fixtures/session_data/new/cookies_dict_with_extras.json
{
"__meta__": {
"about": "HTTPie session file",
"help": "https://httpie.io/docs#sessions",
"httpie": "__version__"
},
"auth": {
"raw_auth": "foo:bar",
"type": "basic"
},
"cookies": [
{
"domain": __host__,
"expires": null,
"name": "baz",
"path": "/",
"secure": false,
"value": "quux"
},
{
"domain": __host__,
"expires": null,
"name": "foo",
"path": "/",
"secure": false,
"value": "bar"
}
],
"headers": {
"X-Data": "value",
"X-Foo": "bar"
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, there are test files that mix the new header format with the old cookie format. See https://github.com/httpie/httpie/blob/master/tests/fixtures/session_data/old/cookies_dict_with_extras.json
src/session.rs
Outdated
Headers::Map(ref mut headers) => { | ||
headers.insert(key.into(), value.to_str()?.into()); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that this code path will never run, I wonder if we should replace it with unreachable!()
. I am doing a similar thing with cookies, and I can't think of a reason to maintain unreachable code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unreachable!()
sounds fine, as long as we have a test that loads the old format to make sure. This code is a bit misleading.
It would be nice to just not have this case at all but none of the solutions I can think of are a net improvement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
48e1b62
to
d3b0ae7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! I didn't find any differences compared to HTTPie's behavior.
Headers will be saved as key-value pairs, and any session using the old format will be migrated automatically
Before:
After:
Resolves #245