-
Notifications
You must be signed in to change notification settings - Fork 853
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
Perform HEAD request for HttpStore::head #4837
Changes from all commits
094eb04
ffc05c9
6dc8ad8
9c6be70
f212079
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,7 +15,7 @@ | |
// specific language governing permissions and limitations | ||
// under the License. | ||
|
||
use crate::client::header::header_meta; | ||
use crate::client::header::{header_meta, HeaderConfig}; | ||
use crate::path::Path; | ||
use crate::{Error, GetOptions, GetResult, ObjectMeta}; | ||
use crate::{GetResultPayload, Result}; | ||
|
@@ -28,6 +28,12 @@ use reqwest::Response; | |
pub trait GetClient: Send + Sync + 'static { | ||
const STORE: &'static str; | ||
|
||
/// Configure the [`HeaderConfig`] for this client | ||
const HEADER_CONFIG: HeaderConfig = HeaderConfig { | ||
etag_required: true, | ||
last_modified_required: true, | ||
}; | ||
|
||
async fn get_request( | ||
&self, | ||
path: &Path, | ||
|
@@ -49,10 +55,12 @@ impl<T: GetClient> GetClientExt for T { | |
async fn get_opts(&self, location: &Path, options: GetOptions) -> Result<GetResult> { | ||
let range = options.range.clone(); | ||
let response = self.get_request(location, options, false).await?; | ||
let meta = header_meta(location, response.headers(), Default::default()) | ||
.map_err(|e| Error::Generic { | ||
store: T::STORE, | ||
source: Box::new(e), | ||
let meta = | ||
header_meta(location, response.headers(), T::HEADER_CONFIG).map_err(|e| { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It took me a while to track down that the default for |
||
Error::Generic { | ||
store: T::STORE, | ||
source: Box::new(e), | ||
} | ||
})?; | ||
|
||
let stream = response | ||
|
@@ -73,7 +81,7 @@ impl<T: GetClient> GetClientExt for T { | |
async fn head(&self, location: &Path) -> Result<ObjectMeta> { | ||
let options = GetOptions::default(); | ||
let response = self.get_request(location, options, true).await?; | ||
header_meta(location, response.headers(), Default::default()).map_err(|e| { | ||
header_meta(location, response.headers(), T::HEADER_CONFIG).map_err(|e| { | ||
Error::Generic { | ||
store: T::STORE, | ||
source: Box::new(e), | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,11 +15,14 @@ | |
// specific language governing permissions and limitations | ||
// under the License. | ||
|
||
use crate::client::get::GetClient; | ||
use crate::client::header::HeaderConfig; | ||
use crate::client::retry::{self, RetryConfig, RetryExt}; | ||
use crate::client::GetOptionsExt; | ||
use crate::path::{Path, DELIMITER}; | ||
use crate::util::deserialize_rfc1123; | ||
use crate::{ClientOptions, GetOptions, ObjectMeta, Result}; | ||
use async_trait::async_trait; | ||
use bytes::{Buf, Bytes}; | ||
use chrono::{DateTime, Utc}; | ||
use percent_encoding::percent_decode_str; | ||
|
@@ -238,39 +241,6 @@ impl Client { | |
Ok(()) | ||
} | ||
|
||
pub async fn get(&self, location: &Path, options: GetOptions) -> Result<Response> { | ||
let url = self.path_url(location); | ||
let builder = self.client.get(url); | ||
let has_range = options.range.is_some(); | ||
|
||
let res = builder | ||
.with_get_options(options) | ||
.send_retry(&self.retry_config) | ||
.await | ||
.map_err(|source| match source.status() { | ||
// Some stores return METHOD_NOT_ALLOWED for get on directories | ||
Some(StatusCode::NOT_FOUND | StatusCode::METHOD_NOT_ALLOWED) => { | ||
crate::Error::NotFound { | ||
source: Box::new(source), | ||
path: location.to_string(), | ||
} | ||
} | ||
_ => Error::Request { source }.into(), | ||
})?; | ||
|
||
// We expect a 206 Partial Content response if a range was requested | ||
// a 200 OK response would indicate the server did not fulfill the request | ||
if has_range && res.status() != StatusCode::PARTIAL_CONTENT { | ||
return Err(crate::Error::NotSupported { | ||
source: Box::new(Error::RangeNotSupported { | ||
href: location.to_string(), | ||
}), | ||
}); | ||
} | ||
|
||
Ok(res) | ||
} | ||
|
||
pub async fn copy(&self, from: &Path, to: &Path, overwrite: bool) -> Result<()> { | ||
let mut retry = false; | ||
loop { | ||
|
@@ -307,6 +277,60 @@ impl Client { | |
} | ||
} | ||
|
||
#[async_trait] | ||
impl GetClient for Client { | ||
const STORE: &'static str = "HTTP"; | ||
|
||
/// Override the [`HeaderConfig`] to be less strict to support a | ||
/// broader range of HTTP servers (#4831) | ||
const HEADER_CONFIG: HeaderConfig = HeaderConfig { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Perhaps you can add an explanatory comment about why this client overrides the default HEADER_CONFIG (so as to support http servers that don't implement WEBDAV?) |
||
etag_required: false, | ||
last_modified_required: false, | ||
}; | ||
|
||
async fn get_request( | ||
&self, | ||
location: &Path, | ||
options: GetOptions, | ||
head: bool, | ||
) -> Result<Response> { | ||
let url = self.path_url(location); | ||
let method = match head { | ||
true => Method::HEAD, | ||
false => Method::GET, | ||
}; | ||
let has_range = options.range.is_some(); | ||
let builder = self.client.request(method, url); | ||
|
||
let res = builder | ||
.with_get_options(options) | ||
.send_retry(&self.retry_config) | ||
.await | ||
.map_err(|source| match source.status() { | ||
// Some stores return METHOD_NOT_ALLOWED for get on directories | ||
Some(StatusCode::NOT_FOUND | StatusCode::METHOD_NOT_ALLOWED) => { | ||
crate::Error::NotFound { | ||
source: Box::new(source), | ||
path: location.to_string(), | ||
} | ||
} | ||
_ => Error::Request { source }.into(), | ||
})?; | ||
|
||
// We expect a 206 Partial Content response if a range was requested | ||
// a 200 OK response would indicate the server did not fulfill the request | ||
if has_range && res.status() != StatusCode::PARTIAL_CONTENT { | ||
return Err(crate::Error::NotSupported { | ||
source: Box::new(Error::RangeNotSupported { | ||
href: location.to_string(), | ||
}), | ||
}); | ||
} | ||
|
||
Ok(res) | ||
} | ||
} | ||
|
||
/// The response returned by a PROPFIND request, i.e. list | ||
#[derive(Deserialize, Default)] | ||
pub struct MultiStatus { | ||
|
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.
I think using
Default
would make the intent clearer hereThere 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.
Sadly this isn't possible as Default is not const