Skip to content

Commit

Permalink
Review feedback
Browse files Browse the repository at this point in the history
  • Loading branch information
@tustvold
tustvold committed Oct 16, 2023
1 parent 534a7cc commit 9d0a02d
Show file tree
Hide file tree
Showing 3 changed files with 88 additions and 52 deletions.
75 changes: 45 additions & 30 deletions object_store/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -690,12 +690,28 @@ pub struct GetOptions {
/// Request will succeed if the `ObjectMeta::e_tag` matches
/// otherwise returning [`Error::Precondition`]
///
/// <https://datatracker.ietf.org/doc/html/rfc9110#name-if-match>
/// See <https://datatracker.ietf.org/doc/html/rfc9110#name-if-match>
///
/// Examples:
///
/// ```text
/// If-Match: "xyzzy"
/// If-Match: "xyzzy", "r2d2xxxx", "c3piozzzz"
/// If-Match: *
/// ```
pub if_match: Option<String>,
/// Request will succeed if the `ObjectMeta::e_tag` does not match
/// otherwise returning [`Error::NotModified`]
///
/// <https://datatracker.ietf.org/doc/html/rfc9110#section-13.1.2>
/// See <https://datatracker.ietf.org/doc/html/rfc9110#section-13.1.2>
///
/// Examples:
///
/// ```text
/// If-None-Match: "xyzzy"
/// If-None-Match: "xyzzy", "r2d2xxxx", "c3piozzzz"
/// If-None-Match: *
/// ```
pub if_none_match: Option<String>,
/// Request will succeed if the object has been modified since
///
Expand Down Expand Up @@ -1364,33 +1380,32 @@ mod tests {
Err(e) => panic!("{e}"),
}

if let Some(tag) = meta.e_tag {
let options = GetOptions {
if_match: Some(tag.clone()),
..GetOptions::default()
};
storage.get_opts(&path, options).await.unwrap();

let options = GetOptions {
if_match: Some("invalid".to_string()),
..GetOptions::default()
};
let err = storage.get_opts(&path, options).await.unwrap_err();
assert!(matches!(err, Error::Precondition { .. }), "{err}");

let options = GetOptions {
if_none_match: Some(tag.clone()),
..GetOptions::default()
};
let err = storage.get_opts(&path, options).await.unwrap_err();
assert!(matches!(err, Error::NotModified { .. }), "{err}");

let options = GetOptions {
if_none_match: Some("invalid".to_string()),
..GetOptions::default()
};
storage.get_opts(&path, options).await.unwrap();
}
let tag = meta.e_tag.unwrap();
let options = GetOptions {
if_match: Some(tag.clone()),
..GetOptions::default()
};
storage.get_opts(&path, options).await.unwrap();

let options = GetOptions {
if_match: Some("invalid".to_string()),
..GetOptions::default()
};
let err = storage.get_opts(&path, options).await.unwrap_err();
assert!(matches!(err, Error::Precondition { .. }), "{err}");

let options = GetOptions {
if_none_match: Some(tag.clone()),
..GetOptions::default()
};
let err = storage.get_opts(&path, options).await.unwrap_err();
assert!(matches!(err, Error::NotModified { .. }), "{err}");

let options = GetOptions {
if_none_match: Some("invalid".to_string()),
..GetOptions::default()
};
storage.get_opts(&path, options).await.unwrap();
}

/// Returns a chunk of length `chunk_length`
Expand Down Expand Up @@ -1703,7 +1718,7 @@ mod tests {
}

#[test]
fn test_get_options() {
fn test_preconditions() {
let mut meta = ObjectMeta {
location: Path::from("test"),
last_modified: Utc.timestamp_nanos(100),
Expand Down
3 changes: 3 additions & 0 deletions object_store/src/local.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1012,11 +1012,14 @@ fn convert_metadata(metadata: Metadata, location: Path) -> Result<ObjectMeta> {
}

#[cfg(unix)]
/// We include the inode when available to yield an ETag more resistant to collisions
/// and as used by popular web servers such as [Apache](https://httpd.apache.org/docs/2.2/mod/core.html#fileetag)
fn get_inode(metadata: &Metadata) -> u64 {
std::os::unix::fs::MetadataExt::ino(metadata)
}

#[cfg(not(unix))]
/// On platforms where an inode isn't available, fallback to just relying on size and mtime
fn get_inode(metadata: &Metadata) -> u64 {
0
}
Expand Down
62 changes: 40 additions & 22 deletions object_store/src/memory.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,21 @@ pub struct InMemory {
storage: SharedStorage,
}

type Entry = (Bytes, DateTime<Utc>, usize);
struct Entry {
data: Bytes,
last_modified: DateTime<Utc>,
e_tag: usize,
}

impl Entry {
fn new(data: Bytes, last_modified: DateTime<Utc>, e_tag: usize) -> Self {
Self {
data,
last_modified,
e_tag,
}
}
}

#[derive(Debug, Default, Clone)]
struct Storage {
Expand All @@ -94,7 +108,8 @@ impl Storage {
fn insert(&mut self, location: &Path, bytes: Bytes) {
let etag = self.next_etag;
self.next_etag += 1;
self.map.insert(location.clone(), (bytes, Utc::now(), etag));
let entry = Entry::new(bytes, Utc::now(), etag);
self.map.insert(location.clone(), entry);
}
}

Expand Down Expand Up @@ -146,25 +161,25 @@ impl ObjectStore for InMemory {
}

async fn get_opts(&self, location: &Path, options: GetOptions) -> Result<GetResult> {
let (data, last_modified, etag) = self.entry(location).await?;
let etag = etag.to_string();
let entry = self.entry(location).await?;
let e_tag = entry.e_tag.to_string();

let meta = ObjectMeta {
location: location.clone(),
last_modified,
size: data.len(),
e_tag: Some(etag),
size: entry.data.len(),
e_tag: Some(e_tag),
};
options.check_preconditions(&meta)?;

let (range, data) = match options.range {
Some(range) => {
let len = data.len();
let len = entry.data.len();
ensure!(range.end <= len, OutOfRangeSnafu { range, len });
ensure!(range.start <= range.end, BadRangeSnafu { range });
(range.clone(), data.slice(range))
(range.clone(), entry.data.slice(range))
}
None => (0..data.len(), data),
None => (0..entry.data.len(), entry.data),
};
let stream = futures::stream::once(futures::future::ready(Ok(data)));

Expand All @@ -180,15 +195,18 @@ impl ObjectStore for InMemory {
location: &Path,
ranges: &[Range<usize>],
) -> Result<Vec<Bytes>> {
let data = self.entry(location).await?;
let entry = self.entry(location).await?;
ranges
.iter()
.map(|range| {
let range = range.clone();
let len = data.0.len();
ensure!(range.end <= data.0.len(), OutOfRangeSnafu { range, len });
let len = entry.data.len();
ensure!(
range.end <= entry.data.len(),
OutOfRangeSnafu { range, len }
);
ensure!(range.start <= range.end, BadRangeSnafu { range });
Ok(data.0.slice(range))
Ok(entry.data.slice(range))
})
.collect()
}
Expand All @@ -198,9 +216,9 @@ impl ObjectStore for InMemory {

Ok(ObjectMeta {
location: location.clone(),
last_modified: entry.1,
size: entry.0.len(),
e_tag: Some(entry.2.to_string()),
last_modified: entry.last_modified,
size: entry.data.len(),
e_tag: Some(entry.e_tag.to_string()),
})
}

Expand Down Expand Up @@ -230,9 +248,9 @@ impl ObjectStore for InMemory {
.map(|(key, value)| {
Ok(ObjectMeta {
location: key.clone(),
last_modified: value.1,
size: value.0.len(),
e_tag: Some(value.2.to_string()),
last_modified: value.last_modified,
size: value.data.len(),
e_tag: Some(value.e_tag.to_string()),
})
})
.collect();
Expand Down Expand Up @@ -274,9 +292,9 @@ impl ObjectStore for InMemory {
} else {
let object = ObjectMeta {
location: k.clone(),
last_modified: v.1,
size: v.0.len(),
e_tag: Some(v.2.to_string()),
last_modified: v.last_modified,
size: v.data.len(),
e_tag: Some(v.e_tag.to_string()),
};
objects.push(object);
}
Expand Down

0 comments on commit 9d0a02d

Please sign in to comment.