-
Notifications
You must be signed in to change notification settings - Fork 689
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
Add user defined metadata #5915
Changes from all commits
33ca8a6
e75e486
7825d61
fc17928
7d39dfb
f7bb58b
da24f88
8d19c44
6b1030e
8f9d04f
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 |
---|---|---|
|
@@ -61,6 +61,7 @@ use std::sync::Arc; | |
|
||
const VERSION_HEADER: &str = "x-amz-version-id"; | ||
const SHA256_CHECKSUM: &str = "x-amz-checksum-sha256"; | ||
const USER_DEFINED_METADATA_HEADER_PREFIX: &str = "x-amz-meta-"; | ||
|
||
/// A specialized `Error` for object store-related errors | ||
#[derive(Debug, Snafu)] | ||
|
@@ -326,6 +327,10 @@ impl<'a> Request<'a> { | |
has_content_type = true; | ||
builder.header(CONTENT_TYPE, v.as_ref()) | ||
} | ||
Attribute::Metadata(k_suffix) => builder.header( | ||
&format!("{}{}", USER_DEFINED_METADATA_HEADER_PREFIX, k_suffix), | ||
criccomini marked this conversation as resolved.
Show resolved
Hide resolved
|
||
v.as_ref(), | ||
), | ||
}; | ||
} | ||
|
||
|
@@ -642,6 +647,7 @@ impl GetClient for S3Client { | |
etag_required: false, | ||
last_modified_required: false, | ||
version_header: Some(VERSION_HEADER), | ||
user_defined_metadata_prefix: Some(USER_DEFINED_METADATA_HEADER_PREFIX), | ||
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. nit: how about using |
||
}; | ||
|
||
/// Make an S3 GET request <https://docs.aws.amazon.com/AmazonS3/latest/API/API_GetObject.html> | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -135,6 +135,9 @@ enum GetResultError { | |
#[snafu(display("Content-Type header contained non UTF-8 characters"))] | ||
InvalidContentType { source: ToStrError }, | ||
|
||
#[snafu(display("Metadata value for \"{key:?}\" contained non UTF-8 characters"))] | ||
InvalidMetadata { key: String }, | ||
|
||
#[snafu(display("Requested {expected:?}, got {actual:?}"))] | ||
UnexpectedRange { | ||
expected: Range<usize>, | ||
|
@@ -192,7 +195,7 @@ fn get_result<T: GetClient>( | |
}} | ||
} | ||
|
||
let attributes = parse_attributes!( | ||
let mut attributes = parse_attributes!( | ||
response.headers(), | ||
( | ||
CACHE_CONTROL, | ||
|
@@ -221,6 +224,24 @@ fn get_result<T: GetClient>( | |
) | ||
); | ||
|
||
// Add attributes that match the user-defined metadata prefix (e.g. x-amz-meta-) | ||
if let Some(prefix) = T::HEADER_CONFIG.user_defined_metadata_prefix { | ||
for (key, val) in response.headers() { | ||
criccomini marked this conversation as resolved.
Show resolved
Hide resolved
|
||
if let Some(suffix) = key.as_str().strip_prefix(prefix) { | ||
if let Ok(val_str) = val.to_str() { | ||
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. I wonder if we should error here? |
||
attributes.insert( | ||
Attribute::Metadata(suffix.to_string().into()), | ||
val_str.to_string().into(), | ||
); | ||
} else { | ||
return Err(GetResultError::InvalidMetadata { | ||
key: key.to_string(), | ||
}); | ||
} | ||
} | ||
} | ||
} | ||
|
||
let stream = response | ||
.bytes_stream() | ||
.map_err(|source| crate::Error::Generic { | ||
|
@@ -253,6 +274,7 @@ mod tests { | |
etag_required: false, | ||
last_modified_required: false, | ||
version_header: None, | ||
user_defined_metadata_prefix: Some("x-test-meta-"), | ||
}; | ||
|
||
async fn get_request(&self, _: &Path, _: GetOptions) -> Result<Response> { | ||
|
@@ -265,6 +287,7 @@ mod tests { | |
range: Option<Range<usize>>, | ||
status: StatusCode, | ||
content_range: Option<&str>, | ||
headers: Option<Vec<(&str, &str)>>, | ||
) -> Response { | ||
let mut builder = http::Response::builder(); | ||
if let Some(range) = content_range { | ||
|
@@ -276,6 +299,12 @@ mod tests { | |
None => vec![0_u8; object_size], | ||
}; | ||
|
||
if let Some(headers) = headers { | ||
for (key, value) in headers { | ||
builder = builder.header(key, value); | ||
} | ||
} | ||
|
||
builder | ||
.status(status) | ||
.header(CONTENT_LENGTH, object_size) | ||
|
@@ -288,7 +317,7 @@ mod tests { | |
async fn test_get_result() { | ||
let path = Path::from("test"); | ||
|
||
let resp = make_response(12, None, StatusCode::OK, None); | ||
let resp = make_response(12, None, StatusCode::OK, None, None); | ||
let res = get_result::<TestClient>(&path, None, resp).unwrap(); | ||
assert_eq!(res.meta.size, 12); | ||
assert_eq!(res.range, 0..12); | ||
|
@@ -302,14 +331,15 @@ mod tests { | |
Some(2..3), | ||
StatusCode::PARTIAL_CONTENT, | ||
Some("bytes 2-2/12"), | ||
None, | ||
); | ||
let res = get_result::<TestClient>(&path, Some(get_range.clone()), resp).unwrap(); | ||
assert_eq!(res.meta.size, 12); | ||
assert_eq!(res.range, 2..3); | ||
let bytes = res.bytes().await.unwrap(); | ||
assert_eq!(bytes.len(), 1); | ||
|
||
let resp = make_response(12, Some(2..3), StatusCode::OK, None); | ||
let resp = make_response(12, Some(2..3), StatusCode::OK, None, None); | ||
let err = get_result::<TestClient>(&path, Some(get_range.clone()), resp).unwrap_err(); | ||
assert_eq!( | ||
err.to_string(), | ||
|
@@ -321,6 +351,7 @@ mod tests { | |
Some(2..3), | ||
StatusCode::PARTIAL_CONTENT, | ||
Some("bytes 2-3/12"), | ||
None, | ||
); | ||
let err = get_result::<TestClient>(&path, Some(get_range.clone()), resp).unwrap_err(); | ||
assert_eq!(err.to_string(), "Requested 2..3, got 2..4"); | ||
|
@@ -330,14 +361,15 @@ mod tests { | |
Some(2..3), | ||
StatusCode::PARTIAL_CONTENT, | ||
Some("bytes 2-2/*"), | ||
None, | ||
); | ||
let err = get_result::<TestClient>(&path, Some(get_range.clone()), resp).unwrap_err(); | ||
assert_eq!( | ||
err.to_string(), | ||
"Failed to parse value for CONTENT_RANGE header: \"bytes 2-2/*\"" | ||
); | ||
|
||
let resp = make_response(12, Some(2..3), StatusCode::PARTIAL_CONTENT, None); | ||
let resp = make_response(12, Some(2..3), StatusCode::PARTIAL_CONTENT, None, None); | ||
let err = get_result::<TestClient>(&path, Some(get_range.clone()), resp).unwrap_err(); | ||
assert_eq!( | ||
err.to_string(), | ||
|
@@ -349,6 +381,7 @@ mod tests { | |
Some(2..3), | ||
StatusCode::PARTIAL_CONTENT, | ||
Some("bytes 2-3/2"), | ||
None, | ||
); | ||
let err = get_result::<TestClient>(&path, Some(get_range.clone()), resp).unwrap_err(); | ||
assert_eq!( | ||
|
@@ -361,6 +394,7 @@ mod tests { | |
Some(2..6), | ||
StatusCode::PARTIAL_CONTENT, | ||
Some("bytes 2-5/6"), | ||
None, | ||
); | ||
let res = get_result::<TestClient>(&path, Some(GetRange::Suffix(4)), resp).unwrap(); | ||
assert_eq!(res.meta.size, 6); | ||
|
@@ -373,8 +407,26 @@ mod tests { | |
Some(2..6), | ||
StatusCode::PARTIAL_CONTENT, | ||
Some("bytes 2-3/6"), | ||
None, | ||
); | ||
let err = get_result::<TestClient>(&path, Some(GetRange::Suffix(4)), resp).unwrap_err(); | ||
assert_eq!(err.to_string(), "Requested 2..6, got 2..4"); | ||
|
||
let resp = make_response( | ||
12, | ||
None, | ||
StatusCode::OK, | ||
None, | ||
Some(vec![("x-test-meta-foo", "bar")]), | ||
); | ||
let res = get_result::<TestClient>(&path, None, resp).unwrap(); | ||
assert_eq!(res.meta.size, 12); | ||
assert_eq!(res.range, 0..12); | ||
assert_eq!( | ||
res.attributes.get(&Attribute::Metadata("foo".into())), | ||
Some(&"bar".into()) | ||
); | ||
let bytes = res.bytes().await.unwrap(); | ||
assert_eq!(bytes.len(), 12); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -183,6 +183,8 @@ impl Client { | |
has_content_type = true; | ||
builder.header(CONTENT_TYPE, v.as_ref()) | ||
} | ||
// Ignore metadata attributes | ||
Attribute::Metadata(_) => builder, | ||
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. WebDAV handles custom metadata in 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. In the meantime I think we should return an error as per https://docs.rs/object_store/latest/object_store/struct.PutOptions.html#structfield.attributes |
||
}; | ||
} | ||
|
||
|
@@ -319,6 +321,7 @@ impl GetClient for Client { | |
etag_required: false, | ||
last_modified_required: false, | ||
version_header: None, | ||
user_defined_metadata_prefix: None, | ||
}; | ||
|
||
async fn get_request(&self, path: &Path, options: GetOptions) -> Result<Response> { | ||
|
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.
nit: maybe
METADATA_HEADER_PREFIX
is clear enough?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.
Happy to rename to whatever. I chose to be quite verbose because I feel there's a lot of confusion about "metadata". You can see this play out in #4754, where the conversation muddles user-defined metadata and tags. Add in standard (non-user-defined) metadata and it gets even more murky.
Y'all let me know what to rename to, though. :)