Generate ETags for InMemory and LocalFileSystem (#4879)
#4922
Conversation
tustvold
changed the title
|
|
||
| #[cfg(not(unix))] | ||
| fn get_inode(metadata: &Metadata) -> u64 { | ||
| 0 |
There was a problem hiding this comment.
The inode isn't strictly necessary, it is still a pretty strong ETag without it, but on Unix systems we might as well make use of it.
A happy consequence of the way most operations create a new file and then link it into place, is that the inode is actually a pretty good change detector
There was a problem hiding this comment.
I don't understand this comment. If the inode isn't needed, then what value does including it add?
Either modification time and size (along with path) uniquely determines the result, or it isn't...
Perhaps the inode can detect file modifications that do not change the modification timestamp (some sort of symlink shenanigans, perhaps 🤔 )
There was a problem hiding this comment.
In any event I think this "inode is not necessary" context should be encoded as comments in the source code
There was a problem hiding this comment.
I see you have clarified the comment that inode is used to be more resistent to collisions -- sounds good to me
object_store/src/lib.rs
Outdated
| source: format!("{} >= {}", date, last_modified).into(), | ||
| /// | ||
| /// <https://datatracker.ietf.org/doc/html/rfc7232#section-6> | ||
| fn test(&self, meta: &ObjectMeta) -> Result<()> { |
There was a problem hiding this comment.
| fn test(&self, meta: &ObjectMeta) -> Result<()> { | |
| fn check_modification_conditions(&self, meta: &ObjectMeta) -> Result<()> { |
"test" is rather non-descriptive and is easily confused w/ unit tests.
alamb
changed the title
ETags for InMemory and LocalFileSystem (#4879)
There was a problem hiding this comment.
Thank you @tustvold -- the Etag feature is getting quite cool and complete.
I had two major comments:
etag test coverage
It seems like there is no test coverage of the etag generation functionality or did I miss that? When I say "no coverage" I mean we could remove all this code and all the tests would keep passing
etag documentation
Also, it seems like etags in general are not well documented in the object_store crate: https://docs.rs/object_store/latest/object_store/?search=etag
I suggest that we add documentation:
etagsare a supported feature of the crate (perhaps aEtagsection with brief introduction and example of how to use them, along with which object store adapters support them)- Improve https://docs.rs/object_store/latest/object_store/struct.ObjectMeta.html#structfield.e_tag to refer back to the example, and pointing at
check_preconditions)?
I think the tests should be done prior to merge. The documentation is not strictly necessary
|
|
||
| #[cfg(not(unix))] | ||
| fn get_inode(metadata: &Metadata) -> u64 { | ||
| 0 |
There was a problem hiding this comment.
I don't understand this comment. If the inode isn't needed, then what value does including it add?
Either modification time and size (along with path) uniquely determines the result, or it isn't...
Perhaps the inode can detect file modifications that do not change the modification timestamp (some sort of symlink shenanigans, perhaps 🤔 )
|
|
||
| #[cfg(not(unix))] | ||
| fn get_inode(metadata: &Metadata) -> u64 { | ||
| 0 |
There was a problem hiding this comment.
In any event I think this "inode is not necessary" context should be encoded as comments in the source code
object_store/src/memory.rs
Outdated
| storage: SharedStorage, | ||
| } | ||
|
|
||
| type Entry = (Bytes, DateTime<Utc>, usize); |
There was a problem hiding this comment.
I think we should document what this triple is (specifically that the usize is an Etag)
| let last_modified = meta.last_modified; | ||
|
|
||
| if let Some(m) = &self.if_match { | ||
| if m != "*" && m.split(',').map(str::trim).all(|x| x != etag) { |
There was a problem hiding this comment.
Can you please add an explicit mention of * handling as well as support for the , handling to the docstrings (so someone can see it on docs.rs)?
object_store/src/lib.rs
Outdated
| // DELETE nonexisting location | ||
| // PUT overwriting | ||
| #[test] | ||
| fn test_get_options() { |
There was a problem hiding this comment.
This is really a test of check_preconditions isn't it?
| fn test_get_options() { | |
| fn test_pre_conditions() { |
| @@ -1347,33 +1380,32 @@ mod tests { | |||
| Err(e) => panic!("{e}"), | |||
| } | |||
|
|
|||
| if let Some(tag) = meta.e_tag { | |||
There was a problem hiding this comment.
I opted to make these tests "mandatory" as now all stores should support them
tustvold
force-pushed
the
support-etag-in-memory
branch
from
9d0a02d to
8aad50d
Compare
|
|
||
| #[cfg(not(unix))] | ||
| fn get_inode(metadata: &Metadata) -> u64 { | ||
| 0 |
There was a problem hiding this comment.
I see you have clarified the comment that inode is used to be more resistent to collisions -- sounds good to me
Which issue does this PR close?
Part of #4879
Rationale for this change
Implements ETag for InMemory using a simple monotonic counter.
What changes are included in this PR?
Are there any user-facing changes?