fix list objects ordering to follow AWS S3

tangledbytes · tangledbytes · commit 1d9f7deda987 · 2024-09-19T16:43:46.000+05:30
Signed-off-by: Utkarsh Srivastava &lt;srivastavautkarsh8097@gmail.com&gt;

add more tests for asserting the correctness of utf8 sorting

Signed-off-by: Utkarsh Srivastava &lt;srivastavautkarsh8097@gmail.com&gt;

fix order when markers are involved

Signed-off-by: Utkarsh Srivastava &lt;srivastavautkarsh8097@gmail.com&gt;

add caching for name buffers - lifetime limited to per invocation of list_objects

Signed-off-by: Utkarsh Srivastava &lt;srivastavautkarsh8097@gmail.com&gt;
diff --git a/src/sdk/namespace_fs.js b/src/sdk/namespace_fs.js
@@ -18,6 +18,7 @@ const stream_utils = require('../util/stream_utils');
 const buffer_utils = require('../util/buffer_utils');
 const size_utils = require('../util/size_utils');
 const native_fs_utils = require('../util/native_fs_utils');
+const js_utils = require('../util/js_utils');
 const ChunkFS = require('../util/chunk_fs');
 const LRUCache = require('../util/lru_cache');
 const nb_native = require('../util/nb_native');
@@ -91,14 +92,48 @@ const XATTR_METADATA_IGNORE_LIST = [
 ];
 
 /**
- * @param {fs.Dirent} a
- * @param {fs.Dirent} b
- * @returns {1|-1|0}
+ * get_simple_cache returns a simple function
+ * which can be used to access the cached data
+ * 
+ * If the cached data doesn't exist then the
+ * given loader function is called with the
+ * requested key and the result is cached.
+ * 
+ * There is no cache invalidation.
+ * @template {string | number | symbol} T
+ * @template U
+ * 
+ * @param {(key: T) => U} loader 
+ * @param {Partial<Record<T, U>>} [cache={}] 
+ * @returns {(key: T) => U}
  */
-function sort_entries_by_name(a, b) {
-    if (a.name < b.name) return -1;
-    if (a.name > b.name) return 1;
-    return 0;
+function get_simple_cache(loader, cache = {}) {
+    return key => {
+        const cached = cache[key];
+        if (cached) return cached;
+
+        const computed = loader(key);
+        cache[key] = computed;
+        return computed;
+    };
+}
+
+/**
+ * sort_entries_by_name_with_cache generates a sorter which
+ * sorts by UTF8 and caches the intermediate buffers generated
+ * to avoid recomputation
+ * @param {Record<string, Buffer>} [cache={}] 
+ * @returns {(a: fs.Dirent, b: fs.Dirent) => -1|0|1}
+ */
+function sort_entries_by_name_with_cache(cache = {}) {
+    const get_cached_name = get_simple_cache(name => Buffer.from(name, 'utf8'), cache);
+
+    return function(a, b) {
+        const a_name = get_cached_name(a.name);
+        const b_name = get_cached_name(b.name);
+
+        return Buffer.compare(a_name, b_name);
+    };
 }
 
 function _get_version_id_by_stat({ino, mtimeNsBigint}) {
@@ -145,27 +180,33 @@ function _get_filename(file_name) {
     }
     return file_name;
 }
+
 /**
- * @param {fs.Dirent} first_entry
- * @param {fs.Dirent} second_entry
- * @returns {Number}
+ * sort_entries_by_name_and_time_with_cache generates a sorter which sorts
+ * items but UTF8 encoding of the name and in case of conflict uses mtime
+ * to resolve it.
+ * @param {Record<string, Buffer>} [cache={}] 
+ * @returns {(first_entry: fs.Dirent, second_entry: fs.Dirent) => -1|0|1}
  */
-function sort_entries_by_name_and_time(first_entry, second_entry) {
-    const first_entry_name = _get_filename(first_entry.name);
-    const second_entry_name = _get_filename(second_entry.name);
-    if (first_entry_name === second_entry_name) {
-        const first_entry_mtime = _get_mtime_from_filename(first_entry.name);
-        const second_entry_mtime = _get_mtime_from_filename(second_entry.name);
-        // To sort the versions in the latest first order,
-        // below logic is followed
-        if (second_entry_mtime < first_entry_mtime) return -1;
-        if (second_entry_mtime > first_entry_mtime) return 1;
-        return 0;
-    } else {
-        if (first_entry_name < second_entry_name) return -1;
-        if (first_entry_name > second_entry_name) return 1;
-        return 0;
-    }
+function sort_entries_by_name_and_time_with_cache(cache = {}) {
+    const _get_filename_with_cache = get_simple_cache(name => Buffer.from(_get_filename(name), 'utf8'), cache);
+    return function(first_entry, second_entry) {
+        const first_entry_name = _get_filename_with_cache(first_entry.name);
+        const second_entry_name = _get_filename_with_cache(second_entry.name);
+
+        const compare_result = Buffer.compare(first_entry_name, second_entry_name);
+        if (compare_result === 0) {
+            const first_entry_mtime = _get_mtime_from_filename(first_entry.name);
+            const second_entry_mtime = _get_mtime_from_filename(second_entry.name);
+            // To sort the versions in the latest first order,
+            // below logic is followed
+            if (second_entry_mtime < first_entry_mtime) return -1;
+            if (second_entry_mtime > first_entry_mtime) return 1;
+            return 0;
+        }
+
+        return compare_result;
+    };
 }
 
 // This is helper function for list object version
@@ -250,14 +291,6 @@ function is_sparse_file(stat) {
     return (stat.blocks * 512 < stat.size);
 }
 
-/**
- * @param {fs.Dirent} e
- * @returns {string}
- */
-function get_entry_name(e) {
-    return e.name;
-}
-
 /**
  * @param {string} name
  * @returns {fs.Dirent}
@@ -307,14 +340,14 @@ function to_fs_xattr(xattr) {
 const dir_cache = new LRUCache({
     name: 'nsfs-dir-cache',
     make_key: ({ dir_path }) => dir_path,
-    load: async ({ dir_path, fs_context }) => {
+    load: async ({ dir_path, fs_context, dirent_name_cache = {} }) => {
         const time = Date.now();
         const stat = await nb_native().fs.stat(fs_context, dir_path);
         let sorted_entries;
         let usage = config.NSFS_DIR_CACHE_MIN_DIR_SIZE;
         if (stat.size <= config.NSFS_DIR_CACHE_MAX_DIR_SIZE) {
             sorted_entries = await nb_native().fs.readdir(fs_context, dir_path);
-            sorted_entries.sort(sort_entries_by_name);
+            sorted_entries.sort(sort_entries_by_name_with_cache(dirent_name_cache));
             for (const ent of sorted_entries) {
                 usage += ent.name.length + 4;
             }
@@ -342,7 +375,7 @@ const dir_cache = new LRUCache({
 const versions_dir_cache = new LRUCache({
     name: 'nsfs-versions-dir-cache',
     make_key: ({ dir_path }) => dir_path,
-    load: async ({ dir_path, fs_context }) => {
+    load: async ({ dir_path, fs_context, dirent_name_cache = {} }) => {
         const time = Date.now();
         const stat = await nb_native().fs.stat(fs_context, dir_path);
         const version_path = dir_path + "/" + HIDDEN_VERSIONS_PATH;
@@ -376,7 +409,7 @@ const versions_dir_cache = new LRUCache({
                     old_versions_after_rename
                 } = await _rename_null_version(old_versions, fs_context, version_path);
                 const entries = latest_versions.concat(old_versions_after_rename);
-                sorted_entries = entries.sort(sort_entries_by_name_and_time);
+                sorted_entries = entries.sort(sort_entries_by_name_and_time_with_cache(dirent_name_cache));
                 // rename back version to include 'null' suffix.
                 if (renamed_null_versions_set.size > 0) {
                     for (const ent of sorted_entries) {
@@ -388,7 +421,7 @@ const versions_dir_cache = new LRUCache({
                     }
                 }
             } else {
-                sorted_entries = latest_versions.sort(sort_entries_by_name);
+                sorted_entries = latest_versions.sort(sort_entries_by_name_with_cache(dirent_name_cache));
             }
             /*eslint no-unused-expressions: ["error", { "allowTernary": true }]*/
             for (const ent of sorted_entries) {
@@ -644,6 +677,10 @@ class NamespaceFS {
             /** @type {Result[]} */
             const results = [];
 
+            const _dirent_name_cache = {};
+            /** @type {(key: string) => Buffer} */
+            const dirent_name_cache = get_simple_cache(key => Buffer.from(key, 'utf8'), _dirent_name_cache);
+
             /**
              * @param {string} dir_key
              * @returns {Promise<void>}
@@ -652,6 +689,7 @@ class NamespaceFS {
                 if (this._is_hidden_version_path(dir_key)) {
                     return;
                 }
+
                 // /** @type {fs.Dir} */
                 let dir_handle;
                 /** @type {ReaddirCacheItem} */
@@ -685,9 +723,11 @@ class NamespaceFS {
                     // Since versions are arranged next to latest object in the latest first order,
                     // no need to find the sorted last index. Push the ".versions/#VERSION_OBJECT" as
                     // they are in order
-                    if (results.length && r.key < results[results.length - 1].key &&
+
+                    const r_key_buf = dirent_name_cache(r.key);
+                    if (results.length && Buffer.compare(r_key_buf, dirent_name_cache(results[results.length - 1].key)) === -1 &&
                         !this._is_hidden_version_path(r.key)) {
-                        pos = _.sortedLastIndexBy(results, r, a => a.key);
+                        pos = js_utils.sortedLastIndexBy(results, curr => Buffer.compare(dirent_name_cache(curr.key), r_key_buf) === -1);
                     } else {
                         pos = results.length;
                     }
@@ -717,7 +757,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 ||
+                        Buffer.compare(dirent_name_cache(ent.name), dirent_name_cache(marker_curr)) === -1 ||
                         ent.name === this.get_bucket_tmpdir_name() ||
                         ent.name === config.NSFS_FOLDER_OBJECT_NAME) ||
                         this._is_hidden_version_path(ent.name)) {
@@ -745,9 +785,17 @@ class NamespaceFS {
                 if (!(await this.check_access(fs_context, dir_path))) return;
                 try {
                     if (list_versions) {
-                        cached_dir = await versions_dir_cache.get_with_cache({ dir_path, fs_context });
+                        cached_dir = await versions_dir_cache.get_with_cache({
+                            dir_path,
+                            fs_context,
+                            dirent_name_cache: _dirent_name_cache,
+                         });
                     } else {
-                        cached_dir = await dir_cache.get_with_cache({ dir_path, fs_context });
+                        cached_dir = await dir_cache.get_with_cache({
+                            dir_path,
+                            fs_context,
+                            dirent_name_cache: _dirent_name_cache,
+                        });
                     }
                 } catch (err) {
                     if (['ENOENT', 'ENOTDIR'].includes(err.code)) {
@@ -783,11 +831,13 @@ class NamespaceFS {
                             {name: start_marker}
                         ) + 1;
                     } else {
-                        marker_index = _.sortedLastIndexBy(
-                            sorted_entries,
-                            make_named_dirent(marker_curr),
-                            get_entry_name
-                        );
+                        const marker_curr_buf = dirent_name_cache(make_named_dirent(marker_curr).name);
+
+                        marker_index = js_utils.sortedLastIndexBy(sorted_entries, curr => {
+                            const curr_cache_buf = dirent_name_cache(curr.name);
+                            const compare_res = Buffer.compare(curr_cache_buf, marker_curr_buf);
+                            return compare_res === -1 || compare_res === 0;
+                        });
                     }
 
                     // handling a scenario in which key_marker points to an object inside a directory
diff --git a/src/test/unit_tests/jest_tests/js_utils.test.js b/src/test/unit_tests/jest_tests/js_utils.test.js
@@ -0,0 +1,79 @@
+/* Copyright (C) 2024 NooBaa */
+'use strict';
+
+const { sortedLastIndexBy } = require("../../../util/js_utils");
+
+describe('test js_utils.js', () => {
+    describe('sortedLastIndexBy', () => {
+        it('should correctly find position for number array', () => {
+            const test_table = [{
+                array: [1, 3, 4, 5],
+                target: 0,
+                expected_position: 0,
+            }, {
+                array: [1, 3, 4, 5],
+                target: 2,
+                expected_position: 1,
+            }, {
+                array: [1, 3, 4, 5],
+                target: 6,
+                expected_position: 4,
+            }];
+
+            for (const entry of test_table) {
+                expect(sortedLastIndexBy(entry.array, curr => curr < entry.target)).toBe(entry.expected_position);
+            }
+        });
+
+        it('should correctly find position for string array', () => {
+            const test_table = [{
+                array: ["a", "b", "c", "d"],
+                target: "A",
+                expected_position: 0,
+            }, {
+                array: ["a", "b", "d", "e"],
+                target: "c",
+                expected_position: 2,
+            }, {
+                array: ["a", "b", "c", "d"],
+                target: "z",
+                expected_position: 4,
+            }];
+
+            for (const entry of test_table) {
+                expect(sortedLastIndexBy(entry.array, curr => curr < entry.target)).toBe(entry.expected_position);
+            }
+        });
+
+        it('should correctly find position for utf8 string (buffer) array', () => {
+            // Editors might not render characters in this array properly but that's not a mistake,
+            // it's intentional to use such characters - sourced from:
+            // https://forum.moonwalkinc.com/t/determining-s3-listing-order/116
+            const array = ["file_ꦏ_1.txt", "file__2.txt", "file_￼_3.txt", "file_𐎣_4.txt"];
+            const array_of_bufs = array.map(item => Buffer.from(item, 'utf8'));
+
+            const test_table = [{
+                array: array_of_bufs,
+                target: array_of_bufs[0],
+                expected_position: 0,
+            }, {
+                array: array_of_bufs,
+                target: array_of_bufs[2],
+                expected_position: 2,
+            }, {
+                array: array_of_bufs,
+                target: array_of_bufs[3],
+                expected_position: 3,
+            }, {
+                array: array_of_bufs,
+                target: Buffer.from("file_𐎣_40.txt", 'utf8'),
+                expected_position: 4,
+            }];
+
+            for (const entry of test_table) {
+                expect(sortedLastIndexBy(entry.array, curr => curr.compare(entry.target) === -1)).toBe(entry.expected_position);
+            }
+        });
+    });
+});
+
diff --git a/src/test/unit_tests/test_ns_list_objects.js b/src/test/unit_tests/test_ns_list_objects.js
@@ -186,6 +186,55 @@ function test_ns_list_objects(ns, object_sdk, bucket) {
 
     });
 
+    mocha.describe('utf8 specific basic tests', function() {
+        // source of keys: https://forum.moonwalkinc.com/t/determining-s3-listing-order/116
+        const strange_utf8_keys = ["file_ꦏ_1.txt", "file__2.txt", "file_￼_3.txt", "file_𐎣_4.txt"];
+
+        mocha.before(async function() {
+            await create_keys([
+                ...strange_utf8_keys,
+            ]);
+        });
+        mocha.after(async function() {
+            await delete_keys([
+                ...strange_utf8_keys,
+            ]);
+        });
+
+        mocha.it('verify byte-by-byte order sort for utf8 encoded keys', async function() {
+            const r = await ns.list_objects({ bucket }, object_sdk);
+            assert.deepStrictEqual(r.is_truncated, false);
+            assert.deepStrictEqual(r.common_prefixes, []);
+            assert.deepStrictEqual(r.objects.map(it => it.key), strange_utf8_keys);
+        });
+
+        mocha.it('verify byte-by-byte order sort for utf8 encoded keys with markers', async function() {
+            const test_table = [
+                {
+                    limit: 1,
+                },
+                {
+                    limit: 2,
+                },
+                {
+                    limit: 3,
+                },
+                {
+                    limit: 4,
+                },
+                {
+                    limit: 5,
+                }
+            ];
+            for (const test of test_table) {
+                const r = await truncated_listing({ bucket, limit: test.limit });
+                assert.deepStrictEqual(r.is_truncated, false);
+                assert.deepStrictEqual(r.common_prefixes, []);
+                assert.deepStrictEqual(r.objects.map(it => it.key), strange_utf8_keys);
+            }
+        });
+    });
+
     mocha.describe('list objects - dirs', function() {
 
         this.timeout(10 * 60 * 1000); // eslint-disable-line no-invalid-this
diff --git a/src/util/js_utils.js b/src/util/js_utils.js
