You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@opendal.apache.org by "Young-Flash (via GitHub)" <gi...@apache.org> on 2023/08/26 08:43:09 UTC
[GitHub] [incubator-opendal] Young-Flash opened a new pull request, #2944: fix(oay): pass litmus copymove test
Young-Flash opened a new pull request, #2944:
URL: https://github.com/apache/incubator-opendal/pull/2944
This PR fix litmus copymove test `copy_overwrite`, `move` and `move_coll`. Refer to [`dav-server-rs::LocalFs::rename`](https://github.com/messense/dav-server-rs/blob/1346f52007aab7c701288b4c3e99b5e4d44369c3/src/localfs.rs#L414-L425)
```txt
-> running `copymove':
0. init.................. pass
1. begin................. pass
2. copy_init............. pass
3. copy_simple........... pass
4. copy_overwrite........ pass
5. copy_nodestcoll....... pass
6. copy_cleanup.......... pass
7. copy_coll............. pass
8. copy_shallow.......... pass
9. move.................. pass
10. move_coll............. pass
11. move_cleanup.......... pass
12. finish................ pass
<- summary for `copymove': of 13 tests run: 13 passed, 0 failed. 100.0%
```
https://github.com/apache/incubator-opendal/issues/2586
--
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
[GitHub] [incubator-opendal] Xuanwo commented on a diff in pull request #2944: fix(oay): pass litmus copymove test
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #2944:
URL: https://github.com/apache/incubator-opendal/pull/2944#discussion_r1306995377
##########
bin/oay/src/services/webdav/webdavfs.rs:
##########
@@ -156,8 +162,16 @@ impl DavFileSystem for WebdavFs {
fn rename<'a>(&'a self, from: &'a DavPath, to: &'a DavPath) -> dav_server::fs::FsFuture<()> {
async move {
+ let from_path = from
+ .as_rel_ospath()
+ .to_str()
+ .ok_or(FsError::GeneralFailure)?;
+ let to_path = to.as_rel_ospath().to_str().ok_or(FsError::GeneralFailure)?;
+ if from.is_collection() {
Review Comment:
I don't think it works for s3, but it's OK to merge first.
--
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
[GitHub] [incubator-opendal] Xuanwo merged pull request #2944: fix(oay): pass litmus copymove test
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo merged PR #2944:
URL: https://github.com/apache/incubator-opendal/pull/2944
--
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
[GitHub] [incubator-opendal] Xuanwo commented on a diff in pull request #2944: fix(oay): pass litmus copymove test
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #2944:
URL: https://github.com/apache/incubator-opendal/pull/2944#discussion_r1306594924
##########
bin/oay/src/services/webdav/webdavfs.rs:
##########
@@ -156,10 +162,23 @@ impl DavFileSystem for WebdavFs {
fn rename<'a>(&'a self, from: &'a DavPath, to: &'a DavPath) -> dav_server::fs::FsFuture<()> {
async move {
- self.op
- .rename(from.as_url_string().as_str(), to.as_url_string().as_str())
- .await
- .map_err(convert_error)
+ let from_path = from
+ .as_rel_ospath()
+ .to_str()
+ .ok_or(FsError::GeneralFailure)?;
+ let to_path = to.as_rel_ospath().to_str().ok_or(FsError::GeneralFailure)?;
+ let res = self.op.rename(from_path, to_path).await;
+ match res {
+ Ok(_) => Ok(()),
+ Err(e) => {
+ if from.is_collection() {
Review Comment:
Please don't handling error like this.
- If the services doesn't support move a dir, opendal should return `Unsupported`.
- If the services raise error for the file exists, opendal should return `AlreadyExists`
- If the services raise error for trying to rename a folder to file, opendal should return `NotADirectory`
Otherwise, it's a bug that we need to fix.
Maybe we can check `to` first before calling `rename`?
##########
bin/oay/src/services/webdav/webdavfs.rs:
##########
@@ -156,10 +162,23 @@ impl DavFileSystem for WebdavFs {
fn rename<'a>(&'a self, from: &'a DavPath, to: &'a DavPath) -> dav_server::fs::FsFuture<()> {
async move {
- self.op
- .rename(from.as_url_string().as_str(), to.as_url_string().as_str())
- .await
- .map_err(convert_error)
+ let from_path = from
+ .as_rel_ospath()
+ .to_str()
+ .ok_or(FsError::GeneralFailure)?;
+ let to_path = to.as_rel_ospath().to_str().ok_or(FsError::GeneralFailure)?;
+ let res = self.op.rename(from_path, to_path).await;
+ match res {
+ Ok(_) => Ok(()),
+ Err(e) => {
+ if from.is_collection() {
Review Comment:
Please don't handling error like this.
- If the services doesn't support move a dir, opendal should return `Unsupported`.
- If the services raise error for the file exists, opendal should return `AlreadyExists`
- If the services raise error for trying to rename a folder to file, opendal should return `NotADirectory`
Otherwise, it's a bug that we need to fix.
---
Maybe we can check `to` first before calling `rename`?
--
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
[GitHub] [incubator-opendal] Xuanwo commented on a diff in pull request #2944: fix(oay): pass litmus copymove test
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #2944:
URL: https://github.com/apache/incubator-opendal/pull/2944#discussion_r1306594924
##########
bin/oay/src/services/webdav/webdavfs.rs:
##########
@@ -156,10 +162,23 @@ impl DavFileSystem for WebdavFs {
fn rename<'a>(&'a self, from: &'a DavPath, to: &'a DavPath) -> dav_server::fs::FsFuture<()> {
async move {
- self.op
- .rename(from.as_url_string().as_str(), to.as_url_string().as_str())
- .await
- .map_err(convert_error)
+ let from_path = from
+ .as_rel_ospath()
+ .to_str()
+ .ok_or(FsError::GeneralFailure)?;
+ let to_path = to.as_rel_ospath().to_str().ok_or(FsError::GeneralFailure)?;
+ let res = self.op.rename(from_path, to_path).await;
+ match res {
+ Ok(_) => Ok(()),
+ Err(e) => {
+ if from.is_collection() {
Review Comment:
Please don't handling error like this.
- If the services doesn't support move a dir, opendal should return `Unsupported`.
- If the services raise error for the file exists, opendal should return `AlreadyExists`
- If the services raise error for trying to rename a folder to file, opendal should return `NotADirectory`
Maybe we can check `to` first before calling `rename`?
--
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
[GitHub] [incubator-opendal] Xuanwo commented on a diff in pull request #2944: fix(oay): pass litmus copymove test
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #2944:
URL: https://github.com/apache/incubator-opendal/pull/2944#discussion_r1306380829
##########
bin/oay/src/services/webdav/webdavfs.rs:
##########
@@ -147,7 +147,10 @@ impl DavFileSystem for WebdavFs {
fn copy<'a>(&'a self, from: &'a DavPath, to: &'a DavPath) -> dav_server::fs::FsFuture<()> {
async move {
self.op
- .copy(&from.as_url_string(), &to.as_url_string())
+ .copy(
+ from.as_rel_ospath().to_str().unwrap(),
Review Comment:
Please return error instead of unwrap.
##########
bin/oay/src/services/webdav/webdavfs.rs:
##########
@@ -156,10 +159,24 @@ impl DavFileSystem for WebdavFs {
fn rename<'a>(&'a self, from: &'a DavPath, to: &'a DavPath) -> dav_server::fs::FsFuture<()> {
async move {
- self.op
- .rename(from.as_url_string().as_str(), to.as_url_string().as_str())
- .await
- .map_err(convert_error)
+ let res = self
+ .op
+ .rename(
+ from.as_rel_ospath().to_str().unwrap(),
Review Comment:
The same.
##########
bin/oay/src/services/webdav/webdavfs.rs:
##########
@@ -156,10 +159,24 @@ impl DavFileSystem for WebdavFs {
fn rename<'a>(&'a self, from: &'a DavPath, to: &'a DavPath) -> dav_server::fs::FsFuture<()> {
async move {
- self.op
- .rename(from.as_url_string().as_str(), to.as_url_string().as_str())
- .await
- .map_err(convert_error)
+ let res = self
+ .op
+ .rename(
+ from.as_rel_ospath().to_str().unwrap(),
+ to.as_rel_ospath().to_str().unwrap(),
+ )
+ .await;
+ match res {
+ Ok(_) => Ok(()),
+ Err(e) => {
+ if e.kind() == opendal::ErrorKind::Unexpected && from.is_collection() {
Review Comment:
It is unexpected to have a match with `Unexpected`. Can you clarify what happened? Perhaps we returned the wrong error kind within OpenDAL.
--
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
[GitHub] [incubator-opendal] Young-Flash commented on pull request #2944: fix(oay): pass litmus copymove test
Posted by "Young-Flash (via GitHub)" <gi...@apache.org>.
Young-Flash commented on PR #2944:
URL: https://github.com/apache/incubator-opendal/pull/2944#issuecomment-1694232864
> Maybe we can add `litmus` test in our CI for oay.
yeah I think so, will make it next
--
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
[GitHub] [incubator-opendal] Xuanwo commented on pull request #2944: fix(oay): pass litmus copymove test
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on PR #2944:
URL: https://github.com/apache/incubator-opendal/pull/2944#issuecomment-1694231475
Maybe we can add `litmus` test in our CI for oay.
--
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