fix(NODE-3174): Preserve sort key order for numeric string keys

src/sort.ts

@@ -13,6 +13,7 @@ export type Sort =
   | string
   | string[]
   | { [key: string]: SortDirection }
+  | Map<string, SortDirection>
   | [string, SortDirection][]
   | [string, SortDirection];
 
@@ -22,7 +23,10 @@ export type Sort =
 type SortDirectionForCmd = 1 | -1 | { $meta: string };
 
 /** @internal */
-type SortForCmd = { [key: string]: SortDirectionForCmd };
+type SortForCmd = Map<string, SortDirectionForCmd>;
+
+/** @internal */
+type SortPairForCmd = [string, SortDirectionForCmd];
 
 /** @internal */
 function prepareDirection(direction: any = 1): SortDirectionForCmd {
@@ -60,41 +64,47 @@ function isPair(t: Sort): t is [string, SortDirection] {
   return false;
 }
 
+function isDeep(t: Sort): t is [string, SortDirection][] {
+  return Array.isArray(t) && Array.isArray(t[0]);
+}
+
+function isMap(t: Sort): t is Map<string, SortDirection> {
+  return t instanceof Map && t.size > 0;
+}
+
 /** @internal */
-function pairToObject(v: [string, SortDirection]): SortForCmd {
-  return { [v[0]]: prepareDirection(v[1]) };
+function pairToMap(v: [string, SortDirection]): SortForCmd {
+  return new Map([[`${v[0]}`, prepareDirection([v[1]])]]);
 }
 
 /** @internal */
-function isDeep(t: Sort): t is [string, SortDirection][] {
-  return Array.isArray(t) && Array.isArray(t[0]);
+function deepToMap(t: [string, SortDirection][]): SortForCmd {
+  const sortEntries: SortPairForCmd[] = t.map(([k, v]) => [`${k}`, prepareDirection(v)]);
+  return new Map(sortEntries);
 }
 
 /** @internal */
-function deepToObject(t: [string, SortDirection][]): SortForCmd {
-  const sortObject: SortForCmd = {};
-  for (const [name, value] of t) {
-    sortObject[name] = prepareDirection(value);
-  }
-  return sortObject;
+function stringsToMap(t: string[]): SortForCmd {
+  const sortEntries: SortPairForCmd[] = t.map(key => [`${key}`, 1]);
+  return new Map(sortEntries);
 }
 
 /** @internal */
-function stringsToObject(t: string[]): SortForCmd {
-  const sortObject: SortForCmd = {};
-  for (const key of t) {
-    sortObject[key] = 1;
-  }
-  return sortObject;
+function objectToMap(t: { [key: string]: SortDirection }): SortForCmd {
+  const sortEntries: SortPairForCmd[] = Object.entries(t).map(([k, v]) => [
+    `${k}`,
+    prepareDirection(v)
+  ]);
+  return new Map(sortEntries);
 }
 
 /** @internal */
-function objectToObject(t: { [key: string]: SortDirection }): SortForCmd {
-  const sortObject: SortForCmd = {};
-  for (const key in t) {
-    sortObject[key] = prepareDirection(t[key]);
-  }
-  return sortObject;
+function mapToMap(t: Map<string, SortDirection>): SortForCmd {
+  const sortEntries: SortPairForCmd[] = Array.from(t).map(([k, v]) => [
+    `${k}`,
+    prepareDirection(v)
+  ]);
+  return new Map(sortEntries);
 }
 
 /** converts a Sort type into a type that is valid for the server (SortForCmd) */
@@ -103,12 +113,15 @@ export function formatSort(
   direction?: SortDirection
 ): SortForCmd | undefined {
   if (sort == null) return undefined;
-  if (Array.isArray(sort) && !sort.length) return undefined;
-  if (typeof sort === 'object' && !Object.keys(sort).length) return undefined;
-  if (typeof sort === 'string') return { [sort]: prepareDirection(direction) };
-  if (isPair(sort)) return pairToObject(sort);
-  if (isDeep(sort)) return deepToObject(sort);
-  if (Array.isArray(sort)) return stringsToObject(sort);
-  if (typeof sort === 'object') return objectToObject(sort);
-  throw new Error(`Invalid sort format: ${JSON.stringify(sort)}`);
+  if (typeof sort === 'string') return new Map([[sort, prepareDirection(direction)]]);
+  if (typeof sort !== 'object') {
+    throw new Error(`Invalid sort format: ${JSON.stringify(sort)}`);
+  }
+  if (!Array.isArray(sort)) {
+    return isMap(sort) ? mapToMap(sort) : Object.keys(sort).length ? objectToMap(sort) : undefined;
+  }
+  if (!sort.length) return undefined;
+  if (isDeep(sort)) return deepToMap(sort);
+  if (isPair(sort)) return pairToMap(sort);
+  return stringsToMap(sort);
 }

test/functional/apm.test.js

@@ -673,6 +673,20 @@ describe('APM', function () {
       Object.keys(expected).forEach(key => {
         expect(actual).to.include.key(key);
 
+        // TODO: This is a workaround that works because all sorts in the specs
+        // are objects with one key; ideally we'd want to adjust the spec definitions
+        // to indicate whether order matters for any given key and set general
+        // expectations accordingly
+        if (key === 'sort') {
+          expect(actual[key]).to.be.instanceOf(Map);
+          expect(Object.keys(expected[key])).to.have.lengthOf(1);
+          expect(actual[key].size).to.equal(1);
+          expect(actual[key].get(Object.keys(expected[key])[0])).to.equal(
+            Object.values(expected[key])[0]
+          );
+          return;
+        }
+
         if (Array.isArray(expected[key])) {
           expect(actual[key]).to.be.instanceOf(Array);
           expect(actual[key]).to.have.lengthOf(expected[key].length);

test/functional/cursor.test.js

@@ -4175,7 +4175,8 @@ describe('Cursor', function () {
           const cursor = collection.find({}, { sort: input });
           cursor.next(err => {
             expect(err).to.not.exist;
-            expect(events[0].command.sort).to.deep.equal(output);
+            expect(events[0].command.sort).to.be.instanceOf(Map);
+            expect(Array.from(events[0].command.sort)).to.deep.equal(Array.from(output));
             cursor.close(done);
           });
         });
@@ -4189,54 +4190,97 @@ describe('Cursor', function () {
           const cursor = collection.find({}).sort(input);
           cursor.next(err => {
             expect(err).to.not.exist;
-            expect(events[0].command.sort).to.deep.equal(output);
+            expect(events[0].command.sort).to.be.instanceOf(Map);
+            expect(Array.from(events[0].command.sort)).to.deep.equal(Array.from(output));
             cursor.close(done);
           });
         });
       });
 
-    it('should use find options object', findSort({ alpha: 1 }, { alpha: 1 }));
-    it('should use find options string', findSort('alpha', { alpha: 1 }));
-    it('should use find options shallow array', findSort(['alpha', 1], { alpha: 1 }));
-    it('should use find options deep array', findSort([['alpha', 1]], { alpha: 1 }));
+    it('should use find options object', findSort({ alpha: 1 }, new Map([['alpha', 1]])));
+    it('should use find options string', findSort('alpha', new Map([['alpha', 1]])));
+    it('should use find options shallow array', findSort(['alpha', 1], new Map([['alpha', 1]])));
+    it('should use find options deep array', findSort([['alpha', 1]], new Map([['alpha', 1]])));
 
-    it('should use cursor.sort object', cursorSort({ alpha: 1 }, { alpha: 1 }));
-    it('should use cursor.sort string', cursorSort('alpha', { alpha: 1 }));
-    it('should use cursor.sort shallow array', cursorSort(['alpha', 1], { alpha: 1 }));
-    it('should use cursor.sort deep array', cursorSort([['alpha', 1]], { alpha: 1 }));
+    it('should use cursor.sort object', cursorSort({ alpha: 1 }, new Map([['alpha', 1]])));
+    it('should use cursor.sort string', cursorSort('alpha', new Map([['alpha', 1]])));
+    it('should use cursor.sort shallow array', cursorSort(['alpha', 1], new Map([['alpha', 1]])));
+    it('should use cursor.sort deep array', cursorSort([['alpha', 1]], new Map([['alpha', 1]])));
 
     it('formatSort - one key', () => {
-      expect(formatSort('alpha')).to.deep.equal({ alpha: 1 });
-      expect(formatSort(['alpha'])).to.deep.equal({ alpha: 1 });
-      expect(formatSort('alpha', 1)).to.deep.equal({ alpha: 1 });
-      expect(formatSort('alpha', 'asc')).to.deep.equal({ alpha: 1 });
-      expect(formatSort([['alpha', 'asc']])).to.deep.equal({ alpha: 1 });
-      expect(formatSort('alpha', 'ascending')).to.deep.equal({ alpha: 1 });
-      expect(formatSort({ alpha: 1 })).to.deep.equal({ alpha: 1 });
-      expect(formatSort('beta')).to.deep.equal({ beta: 1 });
-      expect(formatSort(['beta'])).to.deep.equal({ beta: 1 });
-      expect(formatSort('beta', -1)).to.deep.equal({ beta: -1 });
-      expect(formatSort('beta', 'desc')).to.deep.equal({ beta: -1 });
-      expect(formatSort('beta', 'descending')).to.deep.equal({ beta: -1 });
-      expect(formatSort({ beta: -1 })).to.deep.equal({ beta: -1 });
-      expect(formatSort({ alpha: { $meta: 'hi' } })).to.deep.equal({
-        alpha: { $meta: 'hi' }
-      });
+      // TODO: rewrite these tests
+      expect(formatSort('alpha')).to.deep.equal(new Map([['alpha', 1]]));
+      expect(formatSort(['alpha'])).to.deep.equal(new Map([['alpha', 1]]));
+      expect(formatSort('alpha', 1)).to.deep.equal(new Map([['alpha', 1]]));
+      expect(formatSort('alpha', 'asc')).to.deep.equal(new Map([['alpha', 1]]));
+      expect(formatSort([['alpha', 'asc']])).to.deep.equal(new Map([['alpha', 1]]));
+      expect(formatSort('alpha', 'ascending')).to.deep.equal(new Map([['alpha', 1]]));
+      expect(formatSort({ alpha: 1 })).to.deep.equal(new Map([['alpha', 1]]));
+      expect(formatSort('beta')).to.deep.equal(new Map([['beta', 1]]));
+      expect(formatSort(['beta'])).to.deep.equal(new Map([['beta', 1]]));
+      expect(formatSort('beta', -1)).to.deep.equal(new Map([['beta', -1]]));
+      expect(formatSort('beta', 'desc')).to.deep.equal(new Map([['beta', -1]]));
+      expect(formatSort('beta', 'descending')).to.deep.equal(new Map([['beta', -1]]));
+      expect(formatSort({ beta: -1 })).to.deep.equal(new Map([['beta', -1]]));
+      expect(formatSort({ alpha: { $meta: 'hi' } })).to.deep.equal(
+        new Map([['alpha', { $meta: 'hi' }]])
+      );
     });
 
     it('formatSort - multi key', () => {
-      expect(formatSort(['alpha', 'beta'])).to.deep.equal({ alpha: 1, beta: 1 });
-      expect(formatSort({ alpha: 1, beta: 1 })).to.deep.equal({ alpha: 1, beta: 1 });
+      expect(formatSort(['alpha', 'beta'])).to.deep.equal(
+        new Map([
+          ['alpha', 1],
+          ['beta', 1]
+        ])
+      );
+      expect(formatSort({ alpha: 1, beta: 1 })).to.deep.equal(
+        new Map([
+          ['alpha', 1],
+          ['beta', 1]
+        ])
+      );
       expect(
         formatSort([
           ['alpha', 'asc'],
           ['beta', 'ascending']
         ])
-      ).to.deep.equal({ alpha: 1, beta: 1 });
-      expect(formatSort({ alpha: { $meta: 'hi' }, beta: 'ascending' })).to.deep.equal({
-        alpha: { $meta: 'hi' },
-        beta: 1
-      });
+      ).to.deep.equal(
+        new Map([
+          ['alpha', 1],
+          ['beta', 1]
+        ])
+      );
+      expect(
+        formatSort(
+          new Map([
+            ['alpha', 'asc'],
+            ['beta', 'ascending']
+          ])
+        )
+      ).to.deep.equal(
+        new Map([
+          ['alpha', 1],
+          ['beta', 1]
+        ])
+      );
+      expect(
+        formatSort([
+          ['3', 'asc'],
+          ['1', 'ascending']
+        ])
+      ).to.deep.equal(
+        new Map([
+          ['3', 1],
+          ['1', 1]
+        ])
+      );
+      expect(formatSort({ alpha: { $meta: 'hi' }, beta: 'ascending' })).to.deep.equal(
+        new Map([
+          ['alpha', { $meta: 'hi' }],
+          ['beta', 1]
+        ])
+      );
     });
 
     it('should use allowDiskUse option on sort', {
@@ -4249,7 +4293,7 @@ describe('Cursor', function () {
           cursor.next(err => {
             expect(err).to.not.exist;
             const { command } = events.shift();
-            expect(command.sort).to.deep.equal({ alpha: 1 });
+            expect(command.sort).to.deep.equal(new Map([['alpha', 1]]));
             expect(command.allowDiskUse).to.be.true;
             cursor.close(done);
           });

test/functional/spec-runner/index.js

@@ -414,14 +414,26 @@ function validateExpectations(commandEvents, spec, savedSessionData) {
 
     const actualCommand = actual.command;
     const expectedCommand = expected.command;
+    if (expectedCommand.sort) {
+      // TODO: This is a workaround that works because all sorts in the specs
+      // are objects with one key; ideally we'd want to adjust the spec definitions
+      // to indicate whether order matters for any given key and set general
+      // expectations accordingly
+      expect(Object.keys(expectedCommand.sort)).to.have.lengthOf(1);
+      expect(actualCommand.sort).to.be.instanceOf(Map);
+      expect(actualCommand.sort.size).to.equal(1);
+      const expectedKey = Object.keys(expectedCommand.sort)[0];
+      expect(actualCommand.sort).to.have.all.keys(expectedKey);
+      actualCommand.sort = { [expectedKey]: actualCommand.sort.get(expectedKey) };
+    }
 
     expect(actualCommand).withSessionData(savedSessionData).to.matchMongoSpec(expectedCommand);
   });
 }
 
 function normalizeCommandShapes(commands) {
-  return commands.map(def =>
-    JSON.parse(
+  return commands.map(def => {
+    const output = JSON.parse(
       EJSON.stringify(
         {
           command: def.command,
@@ -430,8 +442,13 @@ function normalizeCommandShapes(commands) {
         },
         { relaxed: true }
       )
-    )
-  );
+    );
+    // TODO: this is a workaround to preserve sort Map type
+    if (def.command.sort) {
+      output.command.sort = def.command.sort;
+    }
+    return output;
+  });
 }
 
 function extractCrudResult(result, operation) {

test/functional/unified-spec-runner/match.ts

@@ -138,7 +138,21 @@ export function resultCheck(
       for (const [key, value] of expectedEntries) {
         path.push(Array.isArray(expected) ? `[${key}]` : `.${key}`); // record what key we're at
         depth += 1;
-        resultCheck(actual[key], value, entities, path, depth);
+        if (key === 'sort') {
+          // TODO: This is a workaround that works because all sorts in the specs
+          // are objects with one key; ideally we'd want to adjust the spec definitions
+          // to indicate whether order matters for any given key and set general
+          // expectations accordingly
+          expect(Object.keys(value)).to.have.lengthOf(1);
+          expect(actual[key]).to.be.instanceOf(Map);
+          expect(actual[key].size).to.equal(1);
+          const expectedSortKey = Object.keys(value)[0];
+          expect(actual[key]).to.have.all.keys(expectedSortKey);
+          const objFromActual = { [expectedSortKey]: actual[key].get(expectedSortKey) };
+          resultCheck(objFromActual, value, entities, path, depth);
+        } else {
+          resultCheck(actual[key], value, entities, path, depth);
+        }
         depth -= 1;
         path.pop(); // if the recursion was successful we can drop the tested key
       }