erickguan commented on code in PR #7815:
URL: https://github.com/apache/opendal/pull/7815#discussion_r3482303055
##########
core/core/src/blocking/operator.rs:
##########
@@ -622,10 +622,34 @@ impl Operator {
/// # }
/// ```
pub fn rename(&self, from: &str, to: &str) -> Result<()> {
+ self.rename_options(from, to, options::RenameOptions::default())
+ }
+
+ /// Rename a file from `from` to `to` with additional options.
+ ///
+ /// # Options
+ ///
+ /// Visit [`options::RenameOptions`] for all available options.
+ ///
+ /// # Examples
+ ///
+ /// ```
+ /// use opendal_core::blocking;
+ /// use opendal_core::options::RenameOptions;
+ /// use opendal_core::Result;
+ ///
+ /// fn test(op: blocking::Operator) -> Result<()> {
Review Comment:
```suggestion
/// fn rename_with_options(op: blocking::Operator) -> Result<()> {
```
##########
core/services/hdfs/src/docs.md:
##########
@@ -32,6 +32,13 @@ HDFS support needs to enable feature `services-hdfs`.
Refer to [`HdfsBuilder`]'s public API docs for more information.
+### Rename Behavior
Review Comment:
We don't need documentation on this. Users will refer to operator's
documentation.
##########
core/core/src/types/operator/operator.rs:
##########
@@ -1389,24 +1389,111 @@ impl Operator {
/// # }
/// ```
pub async fn rename(&self, from: &str, to: &str) -> Result<()> {
+ self.rename_options(from, to, options::RenameOptions::default())
+ .await
+ }
+
+ /// Rename a file from `from` to `to` with additional options.
+ ///
+ /// # Notes
+ ///
+ /// - `from` and `to` must be a file.
+ /// - If `from` and `to` are the same, an `IsSameFile` error will occur.
+ ///
+ /// # Options
+ ///
+ /// Visit [`options::RenameOptions`] for all available options.
+ ///
+ /// # Examples
+ ///
+ /// ```
+ /// use opendal_core::Operator;
+ /// use opendal_core::Result;
+ ///
+ /// async fn test(op: Operator) -> Result<()> {
+ /// op.rename_with("path/to/file", "path/to/file2")
+ /// .if_not_exists(true)
+ /// .await?;
+ /// Ok(())
+ /// }
+ /// ```
+ pub fn rename_with(
+ &self,
+ from: &str,
+ to: &str,
+ ) -> FutureRename<impl Future<Output = Result<()>>> {
+ let from = normalize_path(from);
+ let to = normalize_path(to);
+
+ OperatorFuture::new(
+ self.context().clone(),
+ self.service().clone(),
+ from,
+ (options::RenameOptions::default(), to),
+ Self::rename_inner,
+ )
+ }
+
+ /// Rename a file from `from` to `to` with additional options.
+ ///
+ /// # Options
+ ///
+ /// Visit [`options::RenameOptions`] for all available options.
+ ///
+ /// # Examples
+ ///
+ /// ```
+ /// use opendal_core::options::RenameOptions;
+ /// use opendal_core::Operator;
+ /// use opendal_core::Result;
+ ///
+ /// async fn test(op: Operator) -> Result<()> {
+ /// let mut opts = RenameOptions::default();
+ /// opts.if_not_exists = true;
+ /// op.rename_options("path/to/file", "path/to/file2", opts)
+ /// .await?;
+ /// Ok(())
+ /// }
+ /// ```
+ pub async fn rename_options(
+ &self,
+ from: &str,
+ to: &str,
+ opts: impl Into<options::RenameOptions>,
+ ) -> Result<()> {
let from = normalize_path(from);
+ let to = normalize_path(to);
+ let opts = opts.into();
+ Self::rename_inner(
+ self.context().clone(),
+ self.service().clone(),
+ from,
+ (opts, to),
+ )
+ .await
+ }
+
+ async fn rename_inner(
+ ctx: OperationContext,
+ srv: Servicer,
+ from: String,
+ (opts, to): (options::RenameOptions, String),
Review Comment:
Eh, what is so special with options and destination? Do we need a tuple?
##########
core/tests/behavior/async_rename.rs:
##########
@@ -206,3 +214,83 @@ pub async fn test_rename_overwrite(op: Operator) ->
Result<()> {
op.delete(&target_path).await.expect("delete must succeed");
Ok(())
}
+
+/// Rename to a nonexistent path should succeed when if_not_exists is set.
+pub async fn test_rename_with_if_not_exists(op: Operator) -> Result<()> {
+ let parent = format!("{}/", uuid::Uuid::new_v4());
Review Comment:
No need for parent since we have uuid here.
##########
core/core/src/types/operator/operator.rs:
##########
@@ -1389,24 +1389,111 @@ impl Operator {
/// # }
/// ```
pub async fn rename(&self, from: &str, to: &str) -> Result<()> {
+ self.rename_options(from, to, options::RenameOptions::default())
+ .await
+ }
+
+ /// Rename a file from `from` to `to` with additional options.
+ ///
+ /// # Notes
+ ///
+ /// - `from` and `to` must be a file.
+ /// - If `from` and `to` are the same, an `IsSameFile` error will occur.
+ ///
+ /// # Options
+ ///
+ /// Visit [`options::RenameOptions`] for all available options.
+ ///
+ /// # Examples
+ ///
+ /// ```
+ /// use opendal_core::Operator;
+ /// use opendal_core::Result;
+ ///
+ /// async fn test(op: Operator) -> Result<()> {
Review Comment:
```suggestion
/// async fn rename_with_options(op: Operator) -> Result<()> {
```
##########
core/services/hdfs/src/core.rs:
##########
@@ -22,6 +22,19 @@ use std::sync::Arc;
use opendal_core::raw::*;
use opendal_core::*;
+fn map_hdfs_rename_error(err: io::Error, if_not_exists: bool, to_path: &str)
-> Error {
+ if if_not_exists && err.kind() == io::ErrorKind::AlreadyExists {
+ return Error::new(
+ ErrorKind::ConditionNotMatch,
+ "target path already exists while if_not_exists is set",
+ )
+ .with_context("input", to_path)
Review Comment:
Is `input` too vague here?
##########
core/tests/behavior/async_rename.rs:
##########
@@ -206,3 +214,83 @@ pub async fn test_rename_overwrite(op: Operator) ->
Result<()> {
op.delete(&target_path).await.expect("delete must succeed");
Ok(())
}
+
+/// Rename to a nonexistent path should succeed when if_not_exists is set.
+pub async fn test_rename_with_if_not_exists(op: Operator) -> Result<()> {
+ let parent = format!("{}/", uuid::Uuid::new_v4());
+ let source_path = format!("{parent}source");
+ let (source_content, _) = gen_bytes(op.info().capability());
+
+ op.write(&source_path, source_content.clone()).await?;
+
+ let target_path = format!("{parent}target");
Review Comment:
What is the behavior to rename files into a destination without
destination's parent folder? It might be tested but I didn't check.
##########
core/tests/behavior/async_rename.rs:
##########
@@ -206,3 +214,83 @@ pub async fn test_rename_overwrite(op: Operator) ->
Result<()> {
op.delete(&target_path).await.expect("delete must succeed");
Ok(())
}
+
+/// Rename to a nonexistent path should succeed when if_not_exists is set.
+pub async fn test_rename_with_if_not_exists(op: Operator) -> Result<()> {
+ let parent = format!("{}/", uuid::Uuid::new_v4());
+ let source_path = format!("{parent}source");
+ let (source_content, _) = gen_bytes(op.info().capability());
+
+ op.write(&source_path, source_content.clone()).await?;
+
+ let target_path = format!("{parent}target");
+
+ op.rename_with(&source_path, &target_path)
+ .if_not_exists(true)
+ .await?;
+
+ let err = op.stat(&source_path).await.expect_err("stat must fail");
+ assert_eq!(err.kind(), ErrorKind::NotFound);
+
+ let target_content = op
+ .read(&target_path)
+ .await
+ .expect("read must succeed")
+ .to_bytes();
+ assert_eq!(
+ sha256_digest(target_content),
+ sha256_digest(&source_content),
+ );
+
+ op.delete(&source_path).await.expect("delete must succeed");
+ op.delete(&target_path).await.expect("delete must succeed");
+ op.delete(&parent).await.expect("delete must succeed");
+ Ok(())
+}
+
+/// Rename to an existing path should return ConditionNotMatch when
if_not_exists is set.
+pub async fn test_rename_with_if_not_exists_returns_condition_not_match(
+ op: Operator,
+) -> Result<()> {
+ let parent = format!("{}/", uuid::Uuid::new_v4());
Review Comment:
No need to have parent.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
