Perform HEAD request for HttpStore::head #4837
Conversation
|
If I'm following the code changes correctly, a possible user-facing change for this is with signed urls, because those are only valid for either GET or HEAD (but not both) at one time, IIRC. So sometimes using a |
|
TBC previously this issued a WebDav specific PROPFIND request not a GET request, so is extremely unlikely to have worked with signed URLs |
|
LGTM, We (GlareDB) are already running a wrapper around objectstore for this exact reason. Using HEAD seems much more flexible than PROPFIND. |
| 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.
It took me a while to track down that the default for HEADER_CONFIG is the same as HeaderConfig::default and thus this isn't a change for S3Client and other implementations
| /// Configure the [`HeaderConfig`] for this client | ||
| const HEADER_CONFIG: HeaderConfig = HeaderConfig { | ||
| etag_required: true, | ||
| last_modified_required: true, | ||
| }; |
There was a problem hiding this comment.
I think using Default would make the intent clearer here
| /// Configure the [`HeaderConfig`] for this client | |
| const HEADER_CONFIG: HeaderConfig = HeaderConfig { | |
| etag_required: true, | |
| last_modified_required: true, | |
| }; | |
| /// Configure the [`HeaderConfig`] used for requests issued by this client | |
| const HEADER_CONFIG: HeaderConfig = HeaderConfig::default(); |
There was a problem hiding this comment.
Sadly this isn't possible as Default is not const
| impl GetClient for Client { | ||
| const STORE: &'static str = "HTTP"; | ||
|
|
||
| const HEADER_CONFIG: HeaderConfig = HeaderConfig { |
There was a problem hiding this comment.
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?)
* Perform HEAD request for HttpStore::head * Logical merge conflicts * Review feedback
Which issue does this PR close?
Closes #.
Rationale for this change
Issuing a head request instead of a GET request allows HttpStore::head to be used against regular HTTP servers that don't speak WebDav.
What changes are included in this PR?
Are there any user-facing changes?
Technically perhaps, but we already enforced that GET requests return the necessary headers for metadata, and so this is just an evolution of that to HEAD requests.