Skip to content
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

feat(core)!: make list return path itself #4959

Merged
merged 21 commits into from
Aug 26, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion .github/services/swift/ceph_rados_swift/action.yml
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,6 @@ runs:
shell: bash
run: |
cat << EOF >> $GITHUB_ENV
OPENDAL_DISABLE_RANDOM_ROOT=true
OPENDAL_SWIFT_CONTAINER=testing
OPENDAL_SWIFT_ROOT=/
EOF
1 change: 0 additions & 1 deletion .github/services/swift/swift/action.yml
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,6 @@ runs:
shell: bash
run: |
cat << EOF >> $GITHUB_ENV
OPENDAL_DISABLE_RANDOM_ROOT=true
OPENDAL_SWIFT_CONTAINER=testing
OPENDAL_SWIFT_ROOT=/
EOF
35 changes: 20 additions & 15 deletions bindings/cpp/tests/basic_test.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -77,8 +77,13 @@ TEST_F(OpendalTest, BasicTest) {
auto list_file_path = dir_path + file_path;
op.write(list_file_path, data);
auto entries = op.list(dir_path);
EXPECT_EQ(entries.size(), 1);
EXPECT_EQ(entries[0].path, list_file_path);
EXPECT_EQ(entries.size(), 2);
std::set<std::string> paths;
for (const auto &entry : entries) {
paths.insert(entry.path);
}
EXPECT_TRUE(paths.find(dir_path) != paths.end());
EXPECT_TRUE(paths.find(list_file_path) != paths.end());

// remove
op.remove(file_path_renamed);
Expand Down Expand Up @@ -133,23 +138,23 @@ TEST_F(OpendalTest, ReaderTest) {
}

TEST_F(OpendalTest, ListerTest) {
op.create_dir("test_dir/");
op.write("test_dir/test1", {1, 2, 3});
op.write("test_dir/test2", {4, 5, 6});
std::string dir_path = "test_dir/";
op.create_dir(dir_path);
auto test1_path = dir_path + "test1";
op.write(test1_path, {1, 2, 3});
auto test2_path = dir_path + "test2";
op.write(test2_path, {4, 5, 6});

int size = 0;
auto lister = op.lister("test_dir/");

std::set<std::string> paths;
for (const auto &entry : lister) {
EXPECT_TRUE(entry.path.find("test_dir/test") == 0);
size += 1;
paths.insert(entry.path);
}
EXPECT_EQ(size, 2);

lister = op.lister("test_dir/");
std::vector<opendal::Entry> paths(lister.begin(), lister.end());
EXPECT_EQ(paths.size(), 2);
EXPECT_EQ(paths[0].path, "test_dir/test1");
EXPECT_EQ(paths[1].path, "test_dir/test2");
EXPECT_EQ(paths.size(), 3);
EXPECT_TRUE(paths.find(dir_path) != paths.end());
EXPECT_TRUE(paths.find(test1_path) != paths.end());
EXPECT_TRUE(paths.find(test2_path) != paths.end());
}

int main(int argc, char **argv) {
Expand Down
14 changes: 11 additions & 3 deletions bindings/go/tests/behavior_tests/list_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,7 @@ func testListRichDir(assert *require.Assertions, op *opendal.Operator, fixture *
}
assert.Nil(obs.Error())

expected = append(expected, parent)
slices.Sort(expected)
slices.Sort(actual)

Expand All @@ -133,10 +134,12 @@ func testListEmptyDir(assert *require.Assertions, op *opendal.Operator, fixture
for obs.Next() {
entry := obs.Entry()
paths = append(paths, entry.Path())
assert.Equal(dir, entry.Path())
}
assert.Nil(obs.Error())
assert.Equal(0, len(paths), "dir should only return empty")
assert.Equal(1, len(paths), "dir should only return itself")

paths = nil
obs, err = op.List(strings.TrimSuffix(dir, "/"))
assert.Nil(err)
defer obs.Close()
Expand Down Expand Up @@ -211,26 +214,31 @@ func testListNestedDir(assert *require.Assertions, op *opendal.Operator, fixture
defer obs.Close()
paths = nil
var foundFile bool
var foundDirPath bool
var foundDir bool
for obs.Next() {
entry := obs.Entry()
paths = append(paths, entry.Path())
if entry.Path() == filePath {
foundFile = true
} else if entry.Path() == dirPath {
foundDirPath = true
} else if entry.Path() == dir {
foundDir = true
}
}
assert.Nil(obs.Error())
assert.Equal(2, len(paths), "parent should only got 2 entries")
assert.Equal(3, len(paths), "dir should only got 3 entries")

assert.True(foundDir, "dir should be found in list")

assert.True(foundFile, "file should be found in list")
meta, err := op.Stat(filePath)
assert.Nil(err)
assert.True(meta.IsFile())
assert.Equal(uint64(20), meta.ContentLength())

assert.True(foundDir, "dir should be found in list")
assert.True(foundDirPath, "dir path should be found in list")
meta, err = op.Stat(dirPath)
assert.Nil(err)
assert.True(meta.IsDir())
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
import org.apache.opendal.Metadata;
import org.apache.opendal.OpenDALException;
import org.apache.opendal.test.condition.OpenDALExceptionCondition;
import org.assertj.core.util.Lists;
import org.junit.jupiter.api.BeforeAll;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.TestInstance;
Expand Down Expand Up @@ -77,24 +78,25 @@ public void testListDir() {
*/
@Test
public void testListRichDir() {
final String parent = "test_list_rich_dir";
asyncOp().createDir(parent + "/").join();
final String parentPath = "test_list_rich_dir/";
asyncOp().createDir(parentPath).join();
final List<String> expected = new ArrayList<>();
for (int i = 0; i < 10; i++) {
expected.add(String.format("%s/file-%d", parent, i));
expected.add(String.format("%sfile-%d", parentPath, i));
}

for (String path : expected) {
asyncOp().write(path, parent).join();
asyncOp().write(path, parentPath).join();
}

final List<Entry> entries = asyncOp().list(parent + "/").join();
final List<Entry> entries = asyncOp().list(parentPath).join();
final List<String> actual =
entries.stream().map(Entry::getPath).sorted().collect(Collectors.toList());

expected.add(parentPath);
Collections.sort(expected);
assertThat(actual).isEqualTo(expected);
asyncOp().removeAll(parent + "/").join();
asyncOp().removeAll(parentPath).join();
}

/**
Expand All @@ -106,7 +108,9 @@ public void testListEmptyDir() {
asyncOp().createDir(dir).join();

final List<Entry> entries = asyncOp().list(dir).join();
assertThat(entries).isEmpty();
final List<String> actual = entries.stream().map(Entry::getPath).collect(Collectors.toList());
assertThat(actual).hasSize(1);
assertThat(actual.get(0)).isEqualTo(dir);

asyncOp().delete(dir).join();
}
Expand Down Expand Up @@ -151,28 +155,37 @@ public void testListSubDir() {
public void testListNestedDir() {
final String dir = String.format("%s/%s/", UUID.randomUUID(), UUID.randomUUID());
final String fileName = UUID.randomUUID().toString();
final String filePath = String.format("%s/%s", dir, fileName);
final String filePath = String.format("%s%s", dir, fileName);
final String dirName = String.format("%s/", UUID.randomUUID());
final String dirPath = String.format("%s/%s", dir, dirName);
final String dirPath = String.format("%s%s", dir, dirName);
final String content = "test_list_nested_dir";

asyncOp().createDir(dir).join();
asyncOp().write(filePath, content).join();
asyncOp().createDir(dirPath).join();

final List<Entry> entries = asyncOp().list(dir).join();
assertThat(entries).hasSize(2);
assertThat(entries).hasSize(3);

final List<String> expectedPaths = Lists.newArrayList(dir, dirPath, filePath);
Collections.sort(expectedPaths);
final List<String> actualPaths =
entries.stream().map(Entry::getPath).sorted().collect(Collectors.toList());
assertThat(actualPaths).isEqualTo(expectedPaths);

for (Entry entry : entries) {
// check file
if (entry.getPath().equals(filePath)) {
Metadata metadata = entry.getMetadata();
Metadata metadata = asyncOp().stat(filePath).join();
assertTrue(metadata.isFile());
assertThat(metadata.getContentLength()).isEqualTo(content.length());
// check dir
} else if (entry.getPath().equals(dirPath)) {
Metadata metadata = entry.getMetadata();
assertTrue(metadata.isDir());
} else {
assertThat(entry.getPath()).isEqualTo(dir);
assertTrue(entry.metadata.isDir());
}
}

Expand Down
4 changes: 2 additions & 2 deletions bindings/ocaml/test/test.ml
Original file line number Diff line number Diff line change
Expand Up @@ -83,10 +83,10 @@ let test_list test_ctxt =
(Operator.write bo "/testdir/bar" (Bytes.of_string "foo")));
let array = Operator.list bo "testdir/" |> test_check_result in
let actual = Array.map Operator.Entry.name array in
let expected = [| "foo"; "bar" |] in
let expected = [| "testdir/"; "foo"; "bar" |] in
List.iter (Array.sort compare) [ expected; actual ];
assert_equal expected actual;
assert_equal 2 (Array.length array);
assert_equal 3 (Array.length array);
()

let suite =
Expand Down
12 changes: 4 additions & 8 deletions core/src/layers/complete.rs
Original file line number Diff line number Diff line change
Expand Up @@ -195,10 +195,7 @@ impl<A: Access> CompleteAccessor<A> {
if path.ends_with('/') && capability.list_with_recursive {
let (_, mut l) = self
.inner
.list(
path.trim_end_matches('/'),
OpList::default().with_recursive(true).with_limit(1),
)
.list(path, OpList::default().with_recursive(true).with_limit(1))
.await?;

return if oio::List::next(&mut l).await?.is_some() {
Expand Down Expand Up @@ -246,10 +243,9 @@ impl<A: Access> CompleteAccessor<A> {

// Otherwise, we can simulate stat a dir path via `list`.
if path.ends_with('/') && capability.list_with_recursive {
let (_, mut l) = self.inner.blocking_list(
path.trim_end_matches('/'),
OpList::default().with_recursive(true).with_limit(1),
)?;
let (_, mut l) = self
.inner
.blocking_list(path, OpList::default().with_recursive(true).with_limit(1))?;

return if oio::BlockingList::next(&mut l)?.is_some() {
Ok(RpStat::new(Metadata::new(EntryMode::DIR)))
Expand Down
7 changes: 5 additions & 2 deletions core/src/raw/adapters/kv/backend.rs
Original file line number Diff line number Diff line change
Expand Up @@ -209,8 +209,11 @@ impl KvLister {
} else {
EntryMode::FILE
};

oio::Entry::new(&build_rel_path(&self.root, &v), Metadata::new(mode))
let mut path = build_rel_path(&self.root, &v);
if path.is_empty() {
meteorgan marked this conversation as resolved.
Show resolved Hide resolved
path = "/".to_string();
}
oio::Entry::new(&path, Metadata::new(mode))
})
}
}
Expand Down
2 changes: 1 addition & 1 deletion core/src/raw/adapters/typed_kv/api.rs
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ use crate::Scheme;
///
/// `typed_kv::Adapter` is the typed version of `kv::Adapter`. It's more
/// efficient if the underlying kv service can store data with its type. For
/// example, we can store `Bytes` along with it's metadata so that we don't
/// example, we can store `Bytes` along with its metadata so that we don't
/// need to serialize/deserialize it when we get it from the service.
///
/// Ideally, we should use `typed_kv::Adapter` instead of `kv::Adapter` for
Expand Down
12 changes: 7 additions & 5 deletions core/src/raw/adapters/typed_kv/backend.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,15 +15,14 @@
// specific language governing permissions and limitations
// under the License.

use std::sync::Arc;
use std::vec::IntoIter;

use super::Adapter;
use super::Value;
use crate::raw::oio::HierarchyLister;
use crate::raw::oio::QueueBuf;
use crate::raw::*;
use crate::*;
use std::sync::Arc;
use std::vec::IntoIter;

/// The typed kv backend which implements Accessor for typed kv adapter.
#[derive(Debug, Clone)]
Expand Down Expand Up @@ -211,8 +210,11 @@ impl KvLister {
} else {
EntryMode::FILE
};

oio::Entry::new(&build_rel_path(&self.root, &v), Metadata::new(mode))
let mut path = build_rel_path(&self.root, &v);
if path.is_empty() {
path = "/".to_string();
}
oio::Entry::new(&path, Metadata::new(mode))
})
}
}
Expand Down
2 changes: 1 addition & 1 deletion core/src/raw/http_util/client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,7 @@ impl HttpClient {
// Get content length from header so that we can check it.
//
// - If the request method is HEAD, we will ignore content length.
// - If response contains content_encoding, we should omit it's content length.
// - If response contains content_encoding, we should omit its content length.
let content_length = if is_head || parse_content_encoding(resp.headers())?.is_some() {
None
} else {
Expand Down
Loading
Loading