Skip to content

Commit

Permalink
fix(NODE-3174): Preserve sort key order for numeric string keys (#2788)
Browse files Browse the repository at this point in the history
  • Loading branch information
@dariakp
dariakp authored Apr 28, 2021
1 parent 88996d9 commit 440de41
Show file tree
Hide file tree
Showing 5 changed files with 172 additions and 70 deletions.
75 changes: 44 additions & 31 deletions src/sort.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ export type Sort =
| string
| string[]
| { [key: string]: SortDirection }
| Map<string, SortDirection>
| [string, SortDirection][]
| [string, SortDirection];

Expand All @@ -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 {
Expand Down Expand Up @@ -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) */
Expand All @@ -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);
}
14 changes: 14 additions & 0 deletions test/functional/apm.test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 (see NODE-3235)
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);
Expand Down
112 changes: 78 additions & 34 deletions test/functional/cursor.test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
});
});
Expand All @@ -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 (NODE-3236): These are unit tests for a standalone function and should be moved out of the cursor context file
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', {
Expand All @@ -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);
});
Expand Down
25 changes: 21 additions & 4 deletions test/functional/spec-runner/index.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 (see NODE-3235)
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,
Expand All @@ -430,8 +442,13 @@ function normalizeCommandShapes(commands) {
},
{ relaxed: true }
)
)
);
);
// TODO: this is a workaround to preserve sort Map type until NODE-3235 is completed
if (def.command.sort) {
output.command.sort = def.command.sort;
}
return output;
});
}

function extractCrudResult(result, operation) {
Expand Down
16 changes: 15 additions & 1 deletion test/functional/unified-spec-runner/match.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 (see NODE-3235)
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
}
Expand Down

0 comments on commit 440de41

Please sign in to comment.