Skip to content

feat(services/ftp): Setup integration tests #648

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 12 commits into from
Sep 10, 2022
Merged

feat(services/ftp): Setup integration tests #648

merged 12 commits into from
Sep 10, 2022

Conversation

Xuanwo
Copy link
Member

@Xuanwo Xuanwo commented Sep 8, 2022

Signed-off-by: Xuanwo [email protected]

I hereby agree to the terms of the CLA available at: https://databend.rs/dev/policies/cla/

Summary

Fix #626

@Xuanwo
Copy link
Member Author

Xuanwo commented Sep 8, 2022

Hi, @ArberSephirotheca

I have set up integration tests in this PR in action Service Test Ftp. Can you take a look at the failed tests?

Since you have been granted the write permission for this project, you can use gh pr checkout 648 and push to this branch directly for testing.

@Xuanwo
Copy link
Member Author

Xuanwo commented Sep 8, 2022

thread 'services_ftp_list::test_check' panicked at 'mode DIR not match with path a5386fcf-3f1f-424d-ac9d-f6b80fbb43fa', src/dir.rs:61:9

OpenDAL requires all dir must end with /. We can append a / to the path while returning a Dir entry.

@ArberSephirotheca
Copy link
Contributor

ArberSephirotheca commented Sep 9, 2022

Also maybe we need Mutex for client field(to extend its lifetime to 'static and to provide internal mutability)? Because we are passing client field to fut, and we need it to be mutable(read_response_in() and quit() functions inside fut all require &mut self).

In my previous code, I cannot directly pass self.client to fut due to lifetime problem(&mut Self cannot live long). So I first clone the Arc pointer to client field before fut, then I do Arc::get_mut() inside fut. However, Arc::get_mut() only works if there are no other Arc pointers to the same allocation, but we got 2 references here.

@Xuanwo
Copy link
Member Author

Xuanwo commented Sep 9, 2022

In my previous code, I cannot directly pass self.client to fut due to lifetime problem(&mut Self cannot live long). So I first clone the Arc pointer to client field before fut, then I do Arc::get_mut() inside fut. However, Arc::get_mut() only works if there are no other Arc pointers to the same allocation, but we got 2 references here.

I got it. I think we don't need Arc<FtpStream> here. To access a &mut FtpStream, we can put Option<FtpStream> in State directly. While building the future, we can move it into future directly like the following:

pub enum State {
    Reading(Option<FtpStream>),
    Finalize(BoxFuture<'static, Result<()>>),
}
let ft = ft.replace(None);
let fut = async move {
    ft.read_response_in()
    xxxx
}
state = Box::pin(fut);

For example, you can take a look at:

https://github.com/datafuselabs/opendal/blob/5595a0ec611dd1a666dda1ce34b23c20a989321d/src/services/ipfs/backend.rs#L523-L524

Branch coverage.

"src/services/ftp/dir_stream.rs":
Append a '/' to the end of name if a Dir entry is return.

"src/services/ftp/util.rs":
Consume FtpStream once we hit Error or EOF.
Fix create() function, it would now create corresponding
directories if not exist.
…ction.

Delete will return success even if the file does not exist.
@ArberSephirotheca
Copy link
Contributor

ArberSephirotheca commented Sep 9, 2022

All failures are sovled execpt one about stat operation. As there is no command in FTP that get information of single file/directory, I use list command for stat operation. list command works fine if we want information of a single file(it just return a vector of size 1). However, it does not work if we want to get information of a directory. For example, If we want to get metadata of a directory throwawayaa/ it self, the listcommand would try to get metada of any files or subdirectory within that directory rather than itself.
We could solve it by first list its parent directory and then find the information of directory we want from vector of results, but what if user want the metadata of /(root directory)?

, it will first "list" the parent dir and then get the information
of directory from a vector of results.
If arg path points to a file, it will directly "list" the path given by arg.
@Xuanwo
Copy link
Member Author

Xuanwo commented Sep 10, 2022

We can always return ObjectMode::DIR for root dir. Other metadata can leave as empty.

@Xuanwo
Copy link
Member Author

Xuanwo commented Sep 10, 2022

Thanks for your hard-working! We can release FTP support for the next release now!

@Xuanwo Xuanwo merged commit 8e91565 into main Sep 10, 2022
@Xuanwo Xuanwo deleted the ftp-tests branch September 10, 2022 03:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add integration tests for ftp
2 participants