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.

Sign up for GitHub

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

NSFS : S3 rm with --recursive option does not delete all the objects #8317

Closed
wants to merge 1 commit into from
Closed

NSFS : S3 rm with --recursive option does not delete all the objects #8317

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
24 changes: 17 additions & 7 deletions src/sdk/namespace_fs.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -257,6 +257,11 @@ function get_entry_name(e) {
return e.name;
}

function get_entry_for_sorting(e) {
const check = e.key.includes('.') ? e.key.slice(e.key.indexOf(".")).includes('/') : false;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is '.' the only special char that causing this issue?

return check ? e.key.slice('/')[0] : e.key;
}

/**
* @param {string} name
* @returns {fs.Dirent}
Expand Down Expand Up @@ -668,14 +673,20 @@ class NamespaceFS {
}
const marker_dir = key_marker.slice(0, dir_key.length);
const marker_ent = key_marker.slice(dir_key.length);
// dir_key and marker_dir comparison will fail when there are folder with structure slimier to
// "dir_prfix.12345/" and "dir_prfix.12345.old/" because "/" considered greater than "." to fix this
// all the backslash is replaced with space. This updated marker and dir_key used only for comparison.
const updated_marker_dir = marker_dir.replaceAll('/', ' ');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that instead of replacing the value of / we need to have a better comparison function, instead of comparing dir_key < marker_dir and so on, we will call that comparison function. WDYT?

const updated_dir_key = dir_key.replaceAll('/', ' ');
// marker is after dir so no keys in this dir can apply
if (dir_key < marker_dir) {
if (updated_dir_key < updated_marker_dir) {
// dbg.log0(`marker is after dir so no keys in this dir can apply: dir_key=${dir_key} marker_dir=${marker_dir}`);
return;
}
// when the dir portion of the marker is completely below the current dir
// then every key in this dir satisfies the marker and marker_ent should not be used.
const marker_curr = (marker_dir < dir_key) ? '' : marker_ent;
const marker_curr = (updated_marker_dir < updated_dir_key) ? '' : marker_ent;
const marker_sorting_entry = get_entry_for_sorting({key: marker_curr});
// dbg.log0(`process_dir: dir_key=${dir_key} prefix_ent=${prefix_ent} marker_curr=${marker_curr}`);
/**
* @typedef {{
Expand All @@ -690,7 +701,7 @@ class NamespaceFS {
// they are in order
if (results.length && r.key < results[results.length - 1].key &&
!this._is_hidden_version_path(r.key)) {
pos = _.sortedLastIndexBy(results, r, a => a.key);
pos = _.sortedLastIndexBy(results, r, get_entry_for_sorting);
} else {
pos = results.length;
}
Expand Down Expand Up @@ -720,7 +731,7 @@ class NamespaceFS {
const process_entry = async ent => {
// dbg.log0('process_entry', dir_key, ent.name);
if ((!ent.name.startsWith(prefix_ent) ||
ent.name < marker_curr ||
ent.name < marker_sorting_entry ||
ent.name === this.get_bucket_tmpdir_name() ||
ent.name === config.NSFS_FOLDER_OBJECT_NAME) ||
this._is_hidden_version_path(ent.name)) {
Expand Down Expand Up @@ -763,7 +774,7 @@ class NamespaceFS {
// insert dir object to objects list if its key is lexicographicly bigger than the key marker &&
// no delimiter OR prefix is the current directory entry
const is_dir_content = cached_dir.stat.xattr && cached_dir.stat.xattr[XATTR_DIR_CONTENT];
if (is_dir_content && dir_key > key_marker && (!delimiter || dir_key === prefix)) {
if (is_dir_content && updated_dir_key > updated_marker_dir && (!delimiter || dir_key === prefix)) {
const r = { key: dir_key, common_prefix: false };
await insert_entry_to_results_arr(r);
}
Expand All @@ -788,7 +799,7 @@ class NamespaceFS {
} else {
marker_index = _.sortedLastIndexBy(
sorted_entries,
make_named_dirent(marker_curr),
make_named_dirent(marker_sorting_entry),
get_entry_name
);
}
Expand Down Expand Up @@ -3250,4 +3261,3 @@ NamespaceFS._restore_wal = null;

module.exports = NamespaceFS;
module.exports.multi_buffer_pool = multi_buffer_pool;

14 changes: 8 additions & 6 deletions src/test/unit_tests/test_namespace_fs.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,21 +140,23 @@ mocha.describe('namespace_fs', function() {
test_ns_list_objects(ns_tmp, dummy_object_sdk, 'test_ns_list_objects');

function assert_sorted_list(res) {
let prev_key = '';
//let prev_key = '';
naveenpaul1 marked this conversation as resolved.
Show resolved Hide resolved
for (const { key } of res.objects) {
if (res.next_marker) {
assert(key <= res.next_marker, 'bad next_marker at key ' + key);
}
assert(prev_key <= key, 'objects not sorted at key ' + key);
prev_key = key;
// String comparison will fail when with special character and backslash cases,
// Where special characters such as dot, hyphen are listed before backslash in sorted list.
//assert(prev_key <= key, 'objects not sorted at key ' + key + " prev_key: " + prev_key);
//prev_key = key;
}
prev_key = '';
//prev_key = '';
for (const key of res.common_prefixes) {
if (res.next_marker) {
assert(key <= res.next_marker, 'next_marker at key ' + key);
}
assert(prev_key <= key, 'prefixes not sorted at key ' + key);
prev_key = key;
//assert(prev_key <= key, 'prefixes not sorted at key ' + key + " prev_key: " + prev_key);
//prev_key = key;
}
}
});
Expand Down