Skip to content

Commit b951647

Browse files
sypharsyphar
committed
ref?
1 parent 92e420c commit b951647

File tree

4 files changed

+60
-43
lines changed

4 files changed

+60
-43
lines changed

‎src/storage/s3.rs‎

Lines changed: 44 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@ use futures_util::{
1717
pin_mut,
1818
stream::{FuturesUnordered, Stream, StreamExt},
1919
};
20-
use std::sync::Arc;
20+
use std::{borrow::Cow, sync::Arc};
2121
use tracing::{error, instrument, warn};
2222

2323
const PUBLIC_ACCESS_TAG: &str = "static-cloudfront-access";
@@ -210,36 +210,52 @@ impl S3Backend {
210210

211211
let compression = res.content_encoding.as_ref().and_then(|s| s.parse().ok());
212212

213-
let etag = if let Some(mut etag) = res.e_tag {
214-
if let Some(range) = range {
215-
// we can generate a strong, unique etag for a range of the remote object too,
216-
// by just concatenating the original etag with the range start and end.
217-
//
218-
// About edge cases:
219-
// When the etag of the full archive changes after a rebuild,
220-
// all derived etags for files inside the archive will also change.
221-
//
222-
// This could lead to _changed_ ETags, where the single file inside the archive
223-
// is actually the same.
224-
225-
let (new_etag, was_weak) = match etag.strip_prefix("W/") {
226-
Some(t) => (t, true),
227-
None => (&etag[..], false),
228-
};
229-
let new_etag = new_etag.trim_matches('"').trim();
230-
231-
etag = format!(
232-
"{}\"{}:{}-{}\"",
233-
if was_weak { "W/" } else { "" },
234-
new_etag,
235-
range.start(),
236-
range.end()
237-
);
238-
}
213+
let etag = if let Some(s3_etag) = res.e_tag {
214+
let etag: Cow<str> = range
215+
.map(|range| {
216+
// we can generate a unique etag for a range of the remote object too,
217+
// by just concatenating the original etag with the range start and end.
218+
//
219+
// About edge cases:
220+
// When the etag of the full archive changes after a rebuild,
221+
// all derived etags for files inside the archive will also change.
222+
//
223+
// This could lead to _changed_ ETags, where the single file inside the archive
224+
// is actually the same.
225+
//
226+
// Classic ETag format from S3:
227+
//
228+
// W/"etag"
229+
// "etag"
230+
// W/ at the start identifies a weak etag
231+
// "etag" is the actual etag value, with " and " at the start and end.
232+
// Inside, `"` is not allowed.
233+
//
234+
// AWS implementation (an minio) is to just use an MD5 hash of the file as
235+
// ETag
236+
237+
const WEAK_PREFIX: &str = "W/";
238+
239+
let (inner_etag, was_weak) = match s3_etag.strip_prefix(WEAK_PREFIX) {
240+
Some(t) => (t, true),
241+
None => (&s3_etag[..], false),
242+
};
243+
let inner_etag = inner_etag.trim_matches('"').trim();
244+
245+
Cow::Owned(format!(
246+
"{}\"{}:{}-{}\"",
247+
was_weak.then_some(WEAK_PREFIX).unwrap_or_default(),
248+
inner_etag,
249+
range.start(),
250+
range.end()
251+
))
252+
})
253+
.unwrap_or_else(|| Cow::Borrowed(s3_etag.as_ref()));
254+
239255
match etag.parse::<headers::ETag>() {
240256
Ok(etag) => Some(etag),
241257
Err(err) => {
242-
warn!(?err, etag, "Failed to parse ETag from S3 response");
258+
error!(?err, s3_etag, etag = etag.as_ref(), "Failed to parse ETag");
243259
None
244260
}
245261
}

‎src/web/file.rs‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,7 @@ impl File {
4141

4242
#[cfg(test)]
4343
impl File {
44-
pub fn into_response(self, if_none_match: Option<IfNoneMatch>) -> AxumResponse {
44+
pub fn into_response(self, if_none_match: Option<&IfNoneMatch>) -> AxumResponse {
4545
let streaming_blob: StreamingBlob = self.0.into();
4646
StreamingFile(streaming_blob).into_response(if_none_match)
4747
}
@@ -56,11 +56,11 @@ impl StreamingFile {
5656
Ok(StreamingFile(storage.get_stream(path).await?))
5757
}
5858

59-
pub fn into_response(self, if_none_match: Option<IfNoneMatch>) -> AxumResponse {
59+
pub fn into_response(self, if_none_match: Option<&IfNoneMatch>) -> AxumResponse {
6060
const CACHE_POLICY: CachePolicy = CachePolicy::ForeverInCdnAndBrowser;
6161
let last_modified = LastModified::from(SystemTime::from(self.0.date_updated));
6262

63-
if let Some(ref if_none_match) = if_none_match
63+
if let Some(if_none_match) = if_none_match
6464
&& let Some(ref etag) = self.0.etag
6565
&& !if_none_match.precondition_passes(etag)
6666
{
@@ -138,7 +138,7 @@ mod tests {
138138
{
139139
// cached request
140140
let stream = StreamingFile(streaming_blob(CONTENT, None));
141-
let resp = stream.into_response(Some(if_none_match));
141+
let resp = stream.into_response(Some(&if_none_match));
142142
assert_eq!(resp.status(), StatusCode::NOT_MODIFIED);
143143

144144
// cache related headers are repeated on the not-modified response

‎src/web/rustdoc.rs‎

Lines changed: 11 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -195,7 +195,7 @@ pub(crate) static DOC_RUST_LANG_ORG_REDIRECTS: LazyLock<HashMap<&str, OfficialCr
195195
async fn try_serve_legacy_toolchain_asset(
196196
storage: Arc<AsyncStorage>,
197197
path: impl AsRef<str>,
198-
if_none_match: Option<IfNoneMatch>,
198+
if_none_match: Option<&IfNoneMatch>,
199199
) -> AxumResult<AxumResponse> {
200200
let path = path.as_ref().to_owned();
201201
// FIXME: this could be optimized: when a path doesn't exist
@@ -222,7 +222,6 @@ pub(crate) async fn rustdoc_redirector_handler(
222222
RawQuery(original_query): RawQuery,
223223
) -> AxumResult<impl IntoResponse> {
224224
let params = params.with_page_kind(PageKind::Rustdoc);
225-
let if_none_match = if_none_match.map(|th| th.0);
226225

227226
fn redirect_to_doc(
228227
original_uri: Option<&EscapedURI>,
@@ -260,7 +259,7 @@ pub(crate) async fn rustdoc_redirector_handler(
260259
.binary_search(&extension)
261260
.is_ok()
262261
{
263-
return try_serve_legacy_toolchain_asset(storage, params.name(), if_none_match)
262+
return try_serve_legacy_toolchain_asset(storage, params.name(), if_none_match.as_deref())
264263
.instrument(info_span!("serve static asset"))
265264
.await;
266265
}
@@ -324,7 +323,7 @@ pub(crate) async fn rustdoc_redirector_handler(
324323
)
325324
.await
326325
{
327-
Ok(blob) => Ok(StreamingFile(blob).into_response(if_none_match)),
326+
Ok(blob) => Ok(StreamingFile(blob).into_response(if_none_match.as_deref())),
328327
Err(err) => {
329328
if !matches!(err.downcast_ref(), Some(AxumNope::ResourceNotFound))
330329
&& !matches!(err.downcast_ref(), Some(crate::storage::PathNotFoundError))
@@ -337,7 +336,12 @@ pub(crate) async fn rustdoc_redirector_handler(
337336
// docs that were affected by this bug.
338337
// https://github.com/rust-lang/docs.rs/issues/1979
339338
if inner_path.starts_with("search-") || inner_path.starts_with("settings-") {
340-
try_serve_legacy_toolchain_asset(storage, inner_path, if_none_match).await
339+
try_serve_legacy_toolchain_asset(
340+
storage,
341+
inner_path,
342+
if_none_match.as_deref(),
343+
)
344+
.await
341345
} else {
342346
Err(err.into())
343347
}
@@ -442,7 +446,6 @@ pub(crate) async fn rustdoc_html_server_handler(
442446
mut conn: DbConnection,
443447
) -> AxumResult<AxumResponse> {
444448
let params = params.with_page_kind(PageKind::Rustdoc);
445-
let if_none_match = if_none_match.map(|th| th.0);
446449

447450
trace!(?params, ?original_query, "original params");
448451
// Pages generated by Rustdoc are not ready to be served with a CSP yet.
@@ -602,7 +605,7 @@ pub(crate) async fn rustdoc_html_server_handler(
602605
// default asset caching behaviour is `Cache::ForeverInCdnAndBrowser`.
603606
// This is an edge-case when we serve invocation specific static assets under `/latest/`:
604607
// https://github.com/rust-lang/docs.rs/issues/1593
605-
return Ok(StreamingFile(blob).into_response(if_none_match));
608+
return Ok(StreamingFile(blob).into_response(if_none_match.as_deref()));
606609
}
607610

608611
let latest_release = krate.latest_release()?;
@@ -916,12 +919,11 @@ pub(crate) async fn static_asset_handler(
916919
Extension(storage): Extension<Arc<AsyncStorage>>,
917920
if_none_match: Option<TypedHeader<IfNoneMatch>>,
918921
) -> AxumResult<impl IntoResponse> {
919-
let if_none_match = if_none_match.map(|th| th.0);
920922
let storage_path = format!("{RUSTDOC_STATIC_STORAGE_PREFIX}{path}");
921923

922924
Ok(StreamingFile::from_path(&storage, &storage_path)
923925
.await?
924-
.into_response(if_none_match))
926+
.into_response(if_none_match.as_deref()))
925927
}
926928

927929
#[cfg(test)]

‎src/web/source.rs‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -193,7 +193,6 @@ pub(crate) async fn source_browser_handler(
193193
mut conn: DbConnection,
194194
if_none_match: Option<TypedHeader<IfNoneMatch>>,
195195
) -> AxumResult<impl IntoResponse> {
196-
let if_none_match = if_none_match.map(|th| th.0);
197196
let params = params.with_page_kind(PageKind::Source);
198197
let matched_release = match_version(&mut conn, params.name(), params.req_version())
199198
.await?
@@ -278,7 +277,7 @@ pub(crate) async fn source_browser_handler(
278277
let is_text = stream.mime.type_() == mime::TEXT || stream.mime == mime::APPLICATION_JSON;
279278
if !is_text {
280279
// if the file isn't text, serve it directly to the client
281-
let mut response = StreamingFile(stream).into_response(if_none_match);
280+
let mut response = StreamingFile(stream).into_response(if_none_match.as_deref());
282281
response.headers_mut().typed_insert(canonical_url);
283282
response
284283
.extensions_mut()

0 commit comments

Comments
 (0)