Skip to content

Commit

Permalink
Review feedback
Browse files Browse the repository at this point in the history
  • Loading branch information
tustvold committed Oct 16, 2023
1 parent 8488509 commit 534a7cc
Show file tree
Hide file tree
Showing 3 changed files with 23 additions and 23 deletions.
42 changes: 21 additions & 21 deletions object_store/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -720,7 +720,7 @@ impl GetOptions {
/// Returns an error if the modification conditions on this request are not satisfied
///
/// <https://datatracker.ietf.org/doc/html/rfc7232#section-6>
fn test(&self, meta: &ObjectMeta) -> Result<()> {
fn check_preconditions(&self, meta: &ObjectMeta) -> Result<()> {
// The use of the invalid etag "*" means no ETag is equivalent to never matching
let etag = meta.e_tag.as_deref().unwrap_or("*");
let last_modified = meta.last_modified;
Expand Down Expand Up @@ -1712,76 +1712,76 @@ mod tests {
};

let mut options = GetOptions::default();
options.test(&meta).unwrap();
options.check_preconditions(&meta).unwrap();

options.if_modified_since = Some(Utc.timestamp_nanos(50));
options.test(&meta).unwrap();
options.check_preconditions(&meta).unwrap();

options.if_modified_since = Some(Utc.timestamp_nanos(100));
options.test(&meta).unwrap_err();
options.check_preconditions(&meta).unwrap_err();

options.if_modified_since = Some(Utc.timestamp_nanos(101));
options.test(&meta).unwrap_err();
options.check_preconditions(&meta).unwrap_err();

options = GetOptions::default();

options.if_unmodified_since = Some(Utc.timestamp_nanos(50));
options.test(&meta).unwrap_err();
options.check_preconditions(&meta).unwrap_err();

options.if_unmodified_since = Some(Utc.timestamp_nanos(100));
options.test(&meta).unwrap();
options.check_preconditions(&meta).unwrap();

options.if_unmodified_since = Some(Utc.timestamp_nanos(101));
options.test(&meta).unwrap();
options.check_preconditions(&meta).unwrap();

options = GetOptions::default();

options.if_match = Some("123".to_string());
options.test(&meta).unwrap();
options.check_preconditions(&meta).unwrap();

options.if_match = Some("123,354".to_string());
options.test(&meta).unwrap();
options.check_preconditions(&meta).unwrap();

options.if_match = Some("354, 123,".to_string());
options.test(&meta).unwrap();
options.check_preconditions(&meta).unwrap();

options.if_match = Some("354".to_string());
options.test(&meta).unwrap_err();
options.check_preconditions(&meta).unwrap_err();

options.if_match = Some("*".to_string());
options.test(&meta).unwrap();
options.check_preconditions(&meta).unwrap();

// If-Match takes precedence
options.if_unmodified_since = Some(Utc.timestamp_nanos(200));
options.test(&meta).unwrap();
options.check_preconditions(&meta).unwrap();

options = GetOptions::default();

options.if_none_match = Some("123".to_string());
options.test(&meta).unwrap_err();
options.check_preconditions(&meta).unwrap_err();

options.if_none_match = Some("*".to_string());
options.test(&meta).unwrap_err();
options.check_preconditions(&meta).unwrap_err();

options.if_none_match = Some("1232".to_string());
options.test(&meta).unwrap();
options.check_preconditions(&meta).unwrap();

options.if_none_match = Some("23, 123".to_string());
options.test(&meta).unwrap_err();
options.check_preconditions(&meta).unwrap_err();

// If-None-Match takes precedence
options.if_modified_since = Some(Utc.timestamp_nanos(10));
options.test(&meta).unwrap_err();
options.check_preconditions(&meta).unwrap_err();

// Check missing ETag
meta.e_tag = None;
options = GetOptions::default();

options.if_none_match = Some("*".to_string()); // Fails if any file exists
options.test(&meta).unwrap_err();
options.check_preconditions(&meta).unwrap_err();

options = GetOptions::default();
options.if_match = Some("*".to_string()); // Passes if file exists
options.test(&meta).unwrap();
options.check_preconditions(&meta).unwrap();
}
}
2 changes: 1 addition & 1 deletion object_store/src/local.rs
Original file line number Diff line number Diff line change
Expand Up @@ -370,7 +370,7 @@ impl ObjectStore for LocalFileSystem {
maybe_spawn_blocking(move || {
let (file, metadata) = open_file(&path)?;
let meta = convert_metadata(metadata, location)?;
options.test(&meta)?;
options.check_preconditions(&meta)?;

Ok(GetResult {
payload: GetResultPayload::File(file, path),
Expand Down
2 changes: 1 addition & 1 deletion object_store/src/memory.rs
Original file line number Diff line number Diff line change
Expand Up @@ -155,7 +155,7 @@ impl ObjectStore for InMemory {
size: data.len(),
e_tag: Some(etag),
};
options.test(&meta)?;
options.check_preconditions(&meta)?;

let (range, data) = match options.range {
Some(range) => {
Expand Down

0 comments on commit 534a7cc

Please sign in to comment.