Skip to content

Commit f91acba

Browse files
committed
NSFS : S3 rm with --recursive option does not delete all the objects
Signed-off-by: naveenpaul1 <napaul@redhat.com>
1 parent e24e59a commit f91acba

File tree

3 files changed

+184
-9
lines changed

3 files changed

+184
-9
lines changed

src/sdk/namespace_fs.js

Lines changed: 17 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -257,6 +257,11 @@ function get_entry_name(e) {
257257
return e.name;
258258
}
259259

260+
function get_entry_for_sorting(e) {
261+
const check = e.key.includes('.') ? e.key.slice(e.key.indexOf(".")).includes('/') : false;
262+
return check ? e.key.slice('/')[0] : e.key;
263+
}
264+
260265
/**
261266
* @param {string} name
262267
* @returns {fs.Dirent}
@@ -668,14 +673,20 @@ class NamespaceFS {
668673
}
669674
const marker_dir = key_marker.slice(0, dir_key.length);
670675
const marker_ent = key_marker.slice(dir_key.length);
676+
// dir_key and marker_dir comparison will fail when there are folder with structure slimier to
677+
// "dir_prfix.12345/" and "dir_prfix.12345.old/" because "/" considered greater than "." to fix this
678+
// all the backslash is replaced with space. This updated marker and dir_key used only for comparison.
679+
const updated_marker_dir = marker_dir.replaceAll('/', ' ');
680+
const updated_dir_key = dir_key.replaceAll('/', ' ');
671681
// marker is after dir so no keys in this dir can apply
672-
if (dir_key < marker_dir) {
682+
if (updated_dir_key < updated_marker_dir) {
673683
// dbg.log0(`marker is after dir so no keys in this dir can apply: dir_key=${dir_key} marker_dir=${marker_dir}`);
674684
return;
675685
}
676686
// when the dir portion of the marker is completely below the current dir
677687
// then every key in this dir satisfies the marker and marker_ent should not be used.
678-
const marker_curr = (marker_dir < dir_key) ? '' : marker_ent;
688+
const marker_curr = (updated_marker_dir < updated_dir_key) ? '' : marker_ent;
689+
const marker_sorting_entry = get_entry_for_sorting({key: marker_curr});
679690
// dbg.log0(`process_dir: dir_key=${dir_key} prefix_ent=${prefix_ent} marker_curr=${marker_curr}`);
680691
/**
681692
* @typedef {{
@@ -690,7 +701,7 @@ class NamespaceFS {
690701
// they are in order
691702
if (results.length && r.key < results[results.length - 1].key &&
692703
!this._is_hidden_version_path(r.key)) {
693-
pos = _.sortedLastIndexBy(results, r, a => a.key);
704+
pos = _.sortedLastIndexBy(results, r, get_entry_for_sorting);
694705
} else {
695706
pos = results.length;
696707
}
@@ -720,7 +731,7 @@ class NamespaceFS {
720731
const process_entry = async ent => {
721732
// dbg.log0('process_entry', dir_key, ent.name);
722733
if ((!ent.name.startsWith(prefix_ent) ||
723-
ent.name < marker_curr ||
734+
ent.name < marker_sorting_entry ||
724735
ent.name === this.get_bucket_tmpdir_name() ||
725736
ent.name === config.NSFS_FOLDER_OBJECT_NAME) ||
726737
this._is_hidden_version_path(ent.name)) {
@@ -763,7 +774,7 @@ class NamespaceFS {
763774
// insert dir object to objects list if its key is lexicographicly bigger than the key marker &&
764775
// no delimiter OR prefix is the current directory entry
765776
const is_dir_content = cached_dir.stat.xattr && cached_dir.stat.xattr[XATTR_DIR_CONTENT];
766-
if (is_dir_content && dir_key > key_marker && (!delimiter || dir_key === prefix)) {
777+
if (is_dir_content && updated_dir_key > updated_marker_dir && (!delimiter || dir_key === prefix)) {
767778
const r = { key: dir_key, common_prefix: false };
768779
await insert_entry_to_results_arr(r);
769780
}
@@ -788,7 +799,7 @@ class NamespaceFS {
788799
} else {
789800
marker_index = _.sortedLastIndexBy(
790801
sorted_entries,
791-
make_named_dirent(marker_curr),
802+
make_named_dirent(marker_sorting_entry),
792803
get_entry_name
793804
);
794805
}
Lines changed: 156 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,156 @@
1+
/* Copyright (C) 2016 NooBaa */
2+
'use strict';
3+
4+
const path = require('path');
5+
const fs_utils = require('../../../util/fs_utils');
6+
const { TMP_PATH } = require('../../system_tests/test_utils');
7+
const buffer_utils = require('../../../util/buffer_utils');
8+
const NamespaceFS = require('../../../sdk/namespace_fs');
9+
const SensitiveString = require('../../../util/sensitive_string');
10+
11+
const dummy_object_sdk = make_dummy_object_sdk();
12+
const files_in_folders_to_upload = make_keys(4, i => `populatefs_dir/migrateDir.popFSDir.3803140/sparseDir/lnk.${i}.qqqqqqqqqqqqqq`);
13+
const files_in_folders_to_upload_with_sufix = make_keys(4, i => `populatefs_dir/migrateDir.popFSDir.3803140.OldSet/sparseDir/lnk.${i}.qqqqqqqqqqqqqq`);
14+
15+
16+
const files_folder_same_name_file = make_keys(4, i => `populatefs_dir/third_party/cm256.gyp`);
17+
const files_folder_same_name_folder = make_keys(4, i => `populatefs_dir/third_party/cm256/test/test.txt`);
18+
19+
20+
const tmp_nsfs_path = path.join(TMP_PATH, 'test_unsort_list_objects');
21+
const upload_bkt = 'test_ns_uploads_object';
22+
const src_bkt = 'src';
23+
const ns_tmp_bucket_path = `${tmp_nsfs_path}/${src_bkt}`;
24+
const ns_tmp = new NamespaceFS({ bucket_path: ns_tmp_bucket_path, bucket_id: '2', namespace_resource_id: undefined });
25+
26+
27+
28+
describe('namespace_fs list object - corner case', () => {
29+
describe('list object', () => {
30+
beforeAll(async () => {
31+
await fs_utils.create_fresh_path(ns_tmp_bucket_path);
32+
await create_keys(upload_bkt, ns_tmp, [
33+
...files_in_folders_to_upload,
34+
...files_in_folders_to_upload_with_sufix,
35+
...files_folder_same_name_file,
36+
...files_folder_same_name_folder,
37+
]);
38+
});
39+
40+
afterAll(async () => {
41+
await delete_keys(upload_bkt, ns_tmp, [
42+
...files_in_folders_to_upload,
43+
...files_in_folders_to_upload_with_sufix,
44+
...files_folder_same_name_file,
45+
...files_folder_same_name_folder,
46+
]);
47+
await fs_utils.folder_delete(`${ns_tmp_bucket_path}`);
48+
});
49+
50+
it('list object - two folder - one with sufix', async () => {
51+
let r;
52+
let total_items = 0;
53+
for (;;) {
54+
r = await ns_tmp.list_objects({
55+
bucket: upload_bkt,
56+
limit: 2,
57+
key_marker: r ? r.next_marker : "",
58+
}, dummy_object_sdk);
59+
total_items += r.objects.length;
60+
await validat_pagination(r, total_items);
61+
if (!r.next_marker) {
62+
break;
63+
}
64+
}
65+
});
66+
67+
it('list object - file and folder with same name', async () => {
68+
let r;
69+
let total_items = 0;
70+
for (;;) {
71+
r = await ns_tmp.list_objects({
72+
bucket: upload_bkt,
73+
limit: 3,
74+
key_marker: r ? r.next_marker : "",
75+
}, dummy_object_sdk);
76+
total_items += r.objects.length;
77+
await validate_order(r);
78+
await validat_pagination(r, total_items);
79+
if (!r.next_marker) {
80+
break;
81+
}
82+
}
83+
});
84+
});
85+
});
86+
87+
async function validat_pagination(r, total_items) {
88+
if (r.next_marker) {
89+
expect(r.is_truncated).toStrictEqual(true);
90+
} else {
91+
//expect(total_items).toEqual(10);
92+
expect(r.is_truncated).toStrictEqual(false);
93+
}
94+
}
95+
96+
async function validate_order(r) {
97+
let prev_key = '';
98+
for (const { key } of r.objects) {
99+
if (key === 'populatefs_dir/third_party/cm256/test/test.txt') {
100+
expect(prev_key).toEqual('populatefs_dir/third_party/cm256.gyp');
101+
}
102+
prev_key = key;
103+
}
104+
}
105+
106+
function make_keys(count, gen) {
107+
const arr = new Array(count);
108+
for (let i = 0; i < count; ++i) arr[i] = gen(i);
109+
arr.sort();
110+
Object.freeze(arr);
111+
return arr;
112+
}
113+
114+
async function delete_keys(bkt, nsfs_delete_tmp, keys) {
115+
await nsfs_delete_tmp.delete_multiple_objects({
116+
bucket: bkt,
117+
objects: keys.map(key => ({ key })),
118+
}, dummy_object_sdk);
119+
}
120+
121+
async function create_keys(bkt, nsfs_create_tmp, keys) {
122+
return Promise.all(keys.map(async key => {
123+
await nsfs_create_tmp.upload_object({
124+
bucket: bkt,
125+
key,
126+
content_type: 'application/octet-stream',
127+
source_stream: buffer_utils.buffer_to_read_stream(null),
128+
size: 0
129+
}, dummy_object_sdk);
130+
}));
131+
}
132+
133+
function make_dummy_object_sdk() {
134+
return {
135+
requesting_account: {
136+
force_md5_etag: false,
137+
nsfs_account_config: {
138+
uid: process.getuid(),
139+
gid: process.getgid(),
140+
}
141+
},
142+
abort_controller: new AbortController(),
143+
throw_if_aborted() {
144+
if (this.abort_controller.signal.aborted) throw new Error('request aborted signal');
145+
},
146+
147+
read_bucket_sdk_config_info(name) {
148+
return {
149+
bucket_owner: new SensitiveString('dummy-owner'),
150+
owner_account: {
151+
id: 'dummy-id-123',
152+
}
153+
};
154+
}
155+
};
156+
}

src/test/unit_tests/test_namespace_fs.js

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -141,19 +141,27 @@ mocha.describe('namespace_fs', function() {
141141

142142
function assert_sorted_list(res) {
143143
let prev_key = '';
144+
const regex = /[^A-Za-z0-9]/;
144145
for (const { key } of res.objects) {
145146
if (res.next_marker) {
146147
assert(key <= res.next_marker, 'bad next_marker at key ' + key);
147148
}
148-
assert(prev_key <= key, 'objects not sorted at key ' + key);
149+
// String comparison will fail when with special character and backslash cases,
150+
// Where special characters such as dot, hyphen are listed before backslash in sorted list.
151+
// compare string only if key contains no special characters
152+
if (!regex.test(key)) {
153+
assert(prev_key <= key, 'objects not sorted at key ' + key + " prev_key: " + prev_key);
154+
}
149155
prev_key = key;
150156
}
151-
prev_key = '';
152157
for (const key of res.common_prefixes) {
153158
if (res.next_marker) {
154159
assert(key <= res.next_marker, 'next_marker at key ' + key);
155160
}
156-
assert(prev_key <= key, 'prefixes not sorted at key ' + key);
161+
162+
if (!regex.test(key)) {
163+
assert(prev_key <= key, 'prefixes not sorted at key ' + key + " prev_key: " + prev_key);
164+
}
157165
prev_key = key;
158166
}
159167
}

0 commit comments

Comments
 (0)