You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@opendal.apache.org by "Xuanwo (via GitHub)" <gi...@apache.org> on 2023/11/22 11:53:24 UTC
[PR] feat: Implement stat dir correctly based on RFC-3243 List Prefix [incubator-opendal]
Xuanwo opened a new pull request, #3651:
URL: https://github.com/apache/incubator-opendal/pull/3651
Part of https://github.com/apache/incubator-opendal/issues/3247
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: Implement stat dir correctly based on RFC-3243 List Prefix [incubator-opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #3651:
URL: https://github.com/apache/incubator-opendal/pull/3651#discussion_r1403029188
##########
core/src/types/operator/operator.rs:
##########
@@ -158,16 +158,36 @@ impl Operator {
}
}
- /// Get current path's metadata.
+ /// Get given path's metadata.
///
/// # Notes
///
/// For fetch metadata of entries returned by [`Lister`], it's better to use [`list_with`] and
/// [`lister_with`] with `metakey` query like `Metakey::ContentLength | Metakey::LastModified`
- /// so that we can avoid extra requests.
+ /// so that we can avoid extra stat requests.
+ ///
+ /// # Behavior
+ ///
+ /// `test` and `test/` may vary in some services such as S3. However, on a local file system,
+ /// they're identical. Therefore, the behavior of `stat("test")` and `stat("test/")` might differ
+ /// in certain edge cases. Always use `stat("test/")` when you need to access a directory if possible.
+ ///
Review Comment:
Yes, there are services that don't support `create_dir`, `stat` just treat them as simple file. I will add comments in following PRs.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: Implement stat dir correctly based on RFC-3243 List Prefix [incubator-opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #3651:
URL: https://github.com/apache/incubator-opendal/pull/3651#discussion_r1403028266
##########
core/src/raw/oio/list/api.rs:
##########
@@ -90,6 +94,35 @@ impl<P: List> List for Option<P> {
}
}
+/// Impl ListExt for all T: List
+impl<T: List> ListExt for T {}
+
+/// Extension of [`Read`] to make it easier for use.
Review Comment:
I will fix the typo in coming PRs.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: Implement stat dir correctly based on RFC-3243 List Prefix [incubator-opendal]
Posted by "suyanhanx (via GitHub)" <gi...@apache.org>.
suyanhanx commented on code in PR #3651:
URL: https://github.com/apache/incubator-opendal/pull/3651#discussion_r1402940695
##########
core/src/raw/oio/list/api.rs:
##########
@@ -90,6 +94,35 @@ impl<P: List> List for Option<P> {
}
}
+/// Impl ListExt for all T: List
+impl<T: List> ListExt for T {}
+
+/// Extension of [`Read`] to make it easier for use.
Review Comment:
Read?
##########
core/src/types/operator/operator.rs:
##########
@@ -158,16 +158,36 @@ impl Operator {
}
}
- /// Get current path's metadata.
+ /// Get given path's metadata.
///
/// # Notes
///
/// For fetch metadata of entries returned by [`Lister`], it's better to use [`list_with`] and
/// [`lister_with`] with `metakey` query like `Metakey::ContentLength | Metakey::LastModified`
- /// so that we can avoid extra requests.
+ /// so that we can avoid extra stat requests.
+ ///
+ /// # Behavior
+ ///
+ /// `test` and `test/` may vary in some services such as S3. However, on a local file system,
+ /// they're identical. Therefore, the behavior of `stat("test")` and `stat("test/")` might differ
+ /// in certain edge cases. Always use `stat("test/")` when you need to access a directory if possible.
+ ///
Review Comment:
The essence may still need to be noted here. Those services rely on prefixes.Or like WebDAV, which locates resources by their url, there is no way to determine if it is a folder or not before accessing them. That's why this happens.
##########
core/tests/behavior/write.rs:
##########
@@ -282,23 +291,64 @@ pub async fn test_stat_file(op: Operator) -> Result<()> {
assert_eq!(meta.mode(), EntryMode::FILE);
assert_eq!(meta.content_length(), size as u64);
+ // Stat a file with trailing slash should return `NotFound`.
+ if op.info().full_capability().create_dir {
+ let result = op.stat(&format!("{path}/")).await;
+ assert!(result.is_err());
+ assert_eq!(result.unwrap_err().kind(), ErrorKind::NotFound);
+ }
+
op.delete(&path).await.expect("delete must succeed");
Ok(())
}
/// Stat existing file should return metadata
pub async fn test_stat_dir(op: Operator) -> Result<()> {
+ if !op.info().full_capability().create_dir {
+ return Ok(());
+ }
+
let path = format!("{}/", uuid::Uuid::new_v4());
op.create_dir(&path).await.expect("write must succeed");
let meta = op.stat(&path).await?;
assert_eq!(meta.mode(), EntryMode::DIR);
+ // Stat a dir without trailing slash could have two behavior.
+ let result = op.stat(path.trim_end_matches('/')).await;
+ match result {
+ Ok(meta) => assert_eq!(meta.mode(), EntryMode::DIR),
+ Err(err) => assert_eq!(err.kind(), ErrorKind::NotFound),
+ }
+
op.delete(&path).await.expect("delete must succeed");
Ok(())
}
+/// Stat the parent dir of existing dir should return metadata
+pub async fn test_stat_nested_parent_dir(op: Operator) -> Result<()> {
+ if !op.info().full_capability().create_dir {
+ return Ok(());
+ }
+
+ let parent = format!("{}", uuid::Uuid::new_v4());
+ let file = format!("{}", uuid::Uuid::new_v4());
+ let (content, _) = gen_bytes(op.info().full_capability());
+
+ op.write(&format!("{parent}/{file}"), content.clone())
+ .await
+ .expect("write must succeed");
+
+ let meta = op.stat(&format!("{parent}/")).await?;
+ assert_eq!(meta.mode(), EntryMode::DIR);
+
+ op.delete(&format!("{parent}/{file}"))
+ .await
+ .expect("delete must succeed");
+ Ok(())
+}
Review Comment:
We may delete the parent folder too.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: Implement stat dir correctly based on RFC-3243 List Prefix [incubator-opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #3651:
URL: https://github.com/apache/incubator-opendal/pull/3651#discussion_r1403029188
##########
core/src/types/operator/operator.rs:
##########
@@ -158,16 +158,36 @@ impl Operator {
}
}
- /// Get current path's metadata.
+ /// Get given path's metadata.
///
/// # Notes
///
/// For fetch metadata of entries returned by [`Lister`], it's better to use [`list_with`] and
/// [`lister_with`] with `metakey` query like `Metakey::ContentLength | Metakey::LastModified`
- /// so that we can avoid extra requests.
+ /// so that we can avoid extra stat requests.
+ ///
+ /// # Behavior
+ ///
+ /// `test` and `test/` may vary in some services such as S3. However, on a local file system,
+ /// they're identical. Therefore, the behavior of `stat("test")` and `stat("test/")` might differ
+ /// in certain edge cases. Always use `stat("test/")` when you need to access a directory if possible.
+ ///
Review Comment:
Yes, there are services that don't support `create_dir`, `stat` just treat them as simple file. I will add comments in following PRs.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: Implement stat dir correctly based on RFC-3243 List Prefix [incubator-opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #3651:
URL: https://github.com/apache/incubator-opendal/pull/3651#discussion_r1403028589
##########
core/tests/behavior/write.rs:
##########
@@ -282,23 +291,64 @@ pub async fn test_stat_file(op: Operator) -> Result<()> {
assert_eq!(meta.mode(), EntryMode::FILE);
assert_eq!(meta.content_length(), size as u64);
+ // Stat a file with trailing slash should return `NotFound`.
+ if op.info().full_capability().create_dir {
+ let result = op.stat(&format!("{path}/")).await;
+ assert!(result.is_err());
+ assert_eq!(result.unwrap_err().kind(), ErrorKind::NotFound);
+ }
+
op.delete(&path).await.expect("delete must succeed");
Ok(())
}
/// Stat existing file should return metadata
pub async fn test_stat_dir(op: Operator) -> Result<()> {
+ if !op.info().full_capability().create_dir {
+ return Ok(());
+ }
+
let path = format!("{}/", uuid::Uuid::new_v4());
op.create_dir(&path).await.expect("write must succeed");
let meta = op.stat(&path).await?;
assert_eq!(meta.mode(), EntryMode::DIR);
+ // Stat a dir without trailing slash could have two behavior.
+ let result = op.stat(path.trim_end_matches('/')).await;
+ match result {
+ Ok(meta) => assert_eq!(meta.mode(), EntryMode::DIR),
+ Err(err) => assert_eq!(err.kind(), ErrorKind::NotFound),
+ }
+
op.delete(&path).await.expect("delete must succeed");
Ok(())
}
+/// Stat the parent dir of existing dir should return metadata
+pub async fn test_stat_nested_parent_dir(op: Operator) -> Result<()> {
+ if !op.info().full_capability().create_dir {
+ return Ok(());
+ }
+
+ let parent = format!("{}", uuid::Uuid::new_v4());
+ let file = format!("{}", uuid::Uuid::new_v4());
+ let (content, _) = gen_bytes(op.info().full_capability());
+
+ op.write(&format!("{parent}/{file}"), content.clone())
+ .await
+ .expect("write must succeed");
+
+ let meta = op.stat(&format!("{parent}/")).await?;
+ assert_eq!(meta.mode(), EntryMode::DIR);
+
+ op.delete(&format!("{parent}/{file}"))
+ .await
+ .expect("delete must succeed");
+ Ok(())
+}
Review Comment:
No related to this PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: Implement stat dir correctly based on RFC-3243 List Prefix [incubator-opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on PR #3651:
URL: https://github.com/apache/incubator-opendal/pull/3651#issuecomment-1823900227
dropbox failed for `too_many_write_operations`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: Implement stat dir correctly based on RFC-3243 List Prefix [incubator-opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo merged PR #3651:
URL: https://github.com/apache/incubator-opendal/pull/3651
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org