[WIP] Add query for HTTP redirects #3916
Conversation
There was a problem hiding this comment.
Hi @theoforger, some really minor changes, everything looks fine
|
|
||
| impl Display for HttpResponse { | ||
| fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { | ||
| write!(f, "{:?}", self) |
There was a problem hiding this comment.
{:?}should be used to implement fmt::Debug not fmt::Display... We could simply write here:
write!(f, "url={} status={}", self.url, self.status)| .iter() | ||
| .map(|r| Value::HttpResponse(HttpResponse::new(r.url.clone(), r.status))) | ||
| .collect(); | ||
| values.pop(); |
There was a problem hiding this comment.
Maybe we should add a comment here about why we pop the last values.
// `responses` is the chain list of response triggers by following redirection. We want to only keep
// "redirected" HTTP response so we drop the last, not-redirected, response.
| /// Represents an HTTP request for `Value::HttpResponse` | ||
| #[derive(Clone, Debug)] | ||
| pub struct HttpResponse { | ||
| url: Url, |
There was a problem hiding this comment.
We should document each field, even if it's obvious:
/// URL of the request that triggers this reponse
url: Url,
/// Status code of the HTTP response
status: u32,I'm adding this because maybe we'll need another field that will hold the potential redirection URL. I think we will need this
/// Value of the redirection URL if this response triggers a redirect
redirect_url: Optional<Url>(I think we should distinguish the two URLs url and redirect_url)
There was a problem hiding this comment.
Do we want to include this right now or in a future PR?
There was a problem hiding this comment.
We can let the struct as it is actually, I think we'll need/add it when we will implement this kind of expression:
GET https://foo.com/auth
HTTP 200
redirect nth 2 url == "https://foo.com/login"
| @@ -31,6 +32,8 @@ pub enum Value { | |||
| Bytes(Vec<u8>), | |||
| /// A date. | |||
| Date(chrono::DateTime<chrono::Utc>), | |||
| /// A structure to represent an HTTP redirection. | |||
There was a problem hiding this comment.
We could use this structure for other scenarios so the comment should be:
/// A structure to represent an HTTP response.
| @@ -102,6 +106,7 @@ impl fmt::Display for Value { | |||
| Value::Bool(x) => x.to_string(), | |||
| Value::Bytes(v) => hex::encode(v).to_string(), | |||
| Value::Date(v) => v.to_string(), | |||
| Value::HttpResponse(v) => format!("HttpResponse(status={})", v.status()), | |||
There was a problem hiding this comment.
Maybe simply:
Value::HttpResponse(v) => v.to_string(),
Closes #922.
Description
This PR adds the query for
redirects, allowing Hurl to look up a list of HTTP redirects evaluated asrunner::HttpResponse.(Marked as draft because it's still missing integration tests)