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.

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

feat(NODE-3517): improve index spec handling and type definitions #3315

Merged
merged 29 commits into from
Jul 27, 2022
Merged
Show file tree
Hide file tree
Changes from 20 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
65a67e9
Added unit tests
aditi-khare-mongoDB Jul 1, 2022
6fc9cc1
refactored parseIndexOptions and put it inside makeIndexSpec
aditi-khare-mongoDB Jul 6, 2022
053c047
refactored parseIndexOptions and moved it inside makeIndexSpec
aditi-khare-mongoDB Jul 6, 2022
1fa6b63
reorganized tests
aditi-khare-mongoDB Jul 7, 2022
0df78c0
Fixed map functionality
aditi-khare-mongoDB Jul 8, 2022
af386dc
wip
aditi-khare-mongoDB Jul 11, 2022
c6989f2
lint fix
aditi-khare-mongoDB Jul 11, 2022
a136417
Removed all edits from utils.test.js
aditi-khare-mongoDB Jul 11, 2022
7eddd4b
Added || 1 to tuple case
aditi-khare-mongoDB Jul 14, 2022
359dd57
remove failing test for now (cannot check map equality in JSON)
aditi-khare-mongoDB Jul 14, 2022
2d0dde7
handled incorrect map stringify singular element case
aditi-khare-mongoDB Jul 15, 2022
453679b
fix
aditi-khare-mongoDB Jul 15, 2022
91910ee
fixed key === 'key' conditional
aditi-khare-mongoDB Jul 18, 2022
572a8c7
Fixed build failures hopefully
aditi-khare-mongoDB Jul 18, 2022
3a017d1
rebase fixup
nbbeeken Jul 18, 2022
e9f6edb
wip
nbbeeken Jul 19, 2022
8bf8b11
fix: preserve key order in encryption scenarios
nbbeeken Jul 20, 2022
eb78d8e
fixups
nbbeeken Jul 20, 2022
79babb2
test: handle Int32s
nbbeeken Jul 20, 2022
02c5a1e
clean up fle testing
nbbeeken Jul 21, 2022
7b3f2fd
chore: comments
nbbeeken Jul 22, 2022
4ee2651
fix: address comments, streamlines naming code
nbbeeken Jul 25, 2022
4a1d509
fix: use map
nbbeeken Jul 25, 2022
c770dce
fix: wire version below 5 check no longer needed
nbbeeken Jul 25, 2022
4100dbc
test: small fixes
nbbeeken Jul 25, 2022
c681cba
comments: add test for name, type test for mixed types, match asserts…
nbbeeken Jul 26, 2022
4ed0db3
fix: ts for indexes
nbbeeken Jul 26, 2022
92b1967
fix: tests
nbbeeken Jul 27, 2022
784c8c1
fix: lint
nbbeeken Jul 27, 2022
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
16 changes: 16 additions & 0 deletions src/cmap/connection.ts
Original file line number Diff line number Diff line change
Expand Up @@ -594,11 +594,27 @@ export class CryptoConnection extends Connection {
return;
}

const sort: Map<string, number> | null = cmd.find || cmd.findAndModify ? cmd.sort : null;
const indexKeys: Map<string, number>[] | null = cmd.createIndexes
? cmd.indexes.map((index: { key: Map<string, number> }) => index.key)
: null;

autoEncrypter.encrypt(ns.toString(), cmd, options, (err, encrypted) => {
if (err || encrypted == null) {
callback(err, null);
return;
}

// Repair JS Map loss
aditi-khare-mongoDB marked this conversation as resolved.
Show resolved Hide resolved
if ((cmd.find || cmd.findAndModify) && sort != null) {
encrypted.sort = sort;
}
if (cmd.createIndexes && indexKeys != null) {
for (const [offset, index] of indexKeys.entries()) {
encrypted.indexes[offset].key = index;
}
}

super.command(ns, encrypted, options, (err, response) => {
if (err || response == null) {
callback(err, response);
Expand Down
101 changes: 62 additions & 39 deletions src/operations/indexes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import type { OneOrMore } from '../mongo_types';
import { ReadPreference } from '../read_preference';
import type { Server } from '../sdam/server';
import type { ClientSession } from '../sessions';
import { Callback, maxWireVersion, MongoDBNamespace, parseIndexOptions } from '../utils';
import { Callback, isObject, maxWireVersion, MongoDBNamespace } from '../utils';
import {
CollationOptions,
CommandOperation,
Expand Down Expand Up @@ -51,14 +51,17 @@ const VALID_INDEX_OPTIONS = new Set([

/** @public */
export type IndexDirection = -1 | 1 | '2d' | '2dsphere' | 'text' | 'geoHaystack' | number;

function isIndexDirection(x: any): x is IndexDirection {
baileympearson marked this conversation as resolved.
Show resolved Hide resolved
return (
typeof x === 'number' || x === '2d' || x === '2dsphere' || x === 'text' || x === 'geoHaystack'
);
}
/** @public */
export type IndexSpecification = OneOrMore<
| string
| [string, IndexDirection]
| { [key: string]: IndexDirection }
Copy link
Contributor

Choose a reason for hiding this comment

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

Removing the array options is a breaking change, because the following used to be permitted, even though it's not a valid index specification according to MongoDB:

const invalid: IndexSpecification = [[["name", 1]]]

Our precedent is to release breaking TS changes as bug fixes when applicable, which I am okay with, but maybe we should consider pulling this change (the deletion of lines 60/61) into a separate bug fix PR to not mix the bug fix in with this other work.

Copy link
Contributor

Choose a reason for hiding this comment

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

Of the three we're addressing here should we pull them each out and try and make this PR only about the refactor (i.e. why this bug but not the others)? (I think the tuple one might be difficult but we can see).

Copy link
Contributor

Choose a reason for hiding this comment

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

what "three" are you referencing here? three bugs?

Copy link
Contributor

Choose a reason for hiding this comment

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

  • Map in TS
  • Key order FLE
  • Tuple parsing issue
  • Type strictness on this nesting of array (one or more)
  • Type strictness for createIndexes aligned with createIndex

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 whether we pull these out into separate PRs/fixes or not, we should make sure that the PR description accurately reflects whatever fixes are included and that we are super clear in our release notes about the changes in behavior. It's also worth noting that the types in the documentation won't get updated on a patch, unless we manually regenerate the 4.8, which could be confusing. I think because of the scope of the changes here, this set of improvements that's coming in as a bug fix is better marked as a feat:

Copy link
Contributor

@dariakp dariakp Jul 26, 2022

Choose a reason for hiding this comment

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

Oh and let's triple check to make sure that every single one of those things has full regression test coverage

Copy link
Contributor

Choose a reason for hiding this comment

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

Collected a summary in the description and between the type/fle/indexes.test.ts additions I see coverage for each point.

| [string, IndexDirection][]
| { [key: string]: IndexDirection }[]
| Map<string, IndexDirection>
baileympearson marked this conversation as resolved.
Show resolved Hide resolved
nbbeeken marked this conversation as resolved.
Show resolved Hide resolved
>;

/** @public */
Expand Down Expand Up @@ -86,7 +89,7 @@ export interface IndexDescription
> {
collation?: CollationOptions;
name?: string;
key: Document;
key: Document | Map<string, IndexDirection>;
}

/** @public */
Expand Down Expand Up @@ -130,23 +133,43 @@ export interface CreateIndexesOptions extends CommandOperationOptions {
hidden?: boolean;
}

function makeIndexSpec(indexSpec: IndexSpecification, options: any): IndexDescription {
const indexParameters = parseIndexOptions(indexSpec);

// Generate the index name
const name = typeof options.name === 'string' ? options.name : indexParameters.name;

// Set up the index
const finalIndexSpec: Document = { name, key: indexParameters.fieldHash };
function isSingleIndexTuple(t: unknown): t is [string, IndexDirection] {
return Array.isArray(t) && t.length === 2 && isIndexDirection(t[1]);
}

// merge valid index options into the index spec
for (const optionName in options) {
if (VALID_INDEX_OPTIONS.has(optionName)) {
finalIndexSpec[optionName] = options[optionName];
function makeIndexSpec(
indexSpec: IndexSpecification,
options?: CreateIndexesOptions
): IndexDescription {
const key: Map<string, IndexDirection> = new Map();

const indexSpecs =
!Array.isArray(indexSpec) || isSingleIndexTuple(indexSpec) ? [indexSpec] : indexSpec;

// Iterate through array and handle different types
for (const spec of indexSpecs) {
if (typeof spec === 'string') {
key.set(spec, 1);
} else if (Array.isArray(spec)) {
key.set(spec[0], spec[1] ?? 1);
} else if (spec instanceof Map) {
for (const [property, value] of spec) {
key.set(property, value);
}
} else if (isObject(spec)) {
for (const [property, value] of Object.entries(spec)) {
key.set(property, value);
}
}
}

return finalIndexSpec as IndexDescription;
// Set up the index
return {
...Object.fromEntries(
Object.entries({ ...options }).filter(([optionName]) => VALID_INDEX_OPTIONS.has(optionName))
),
key
};
}

/** @internal */
Expand Down Expand Up @@ -196,7 +219,25 @@ export class CreateIndexesOperation<
this.options = options ?? {};
this.collectionName = collectionName;

this.indexes = indexes;
// Ensure we generate the correct name if the parameter is not set
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we move this code into a function? We discussed this a bit in the office, but I still think for readability it should be broken out. The fact that a comment is necessary to explain what this block of code is doing strongly indicates that this should be broken out into a function.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure done!

const namedIndexes = [];
for (const userIndex of indexes) {
const index: Omit<IndexDescription, 'key'> & { key: Map<string, IndexDirection> } = {
...userIndex,
// Ensure the key is a Map to preserve index key ordering
key: userIndex.key instanceof Map ? userIndex.key : new Map(Object.entries(userIndex.key))
};
if (index.name == null) {
// Ensure every index is named
const keys = [];
for (const [name, direction] of index.key) {
keys.push(`${name}_${direction}`);
}
index.name = keys.join('_');
dariakp marked this conversation as resolved.
Show resolved Hide resolved
}
namedIndexes.push(index);
}
this.indexes = namedIndexes;
dariakp marked this conversation as resolved.
Show resolved Hide resolved
}

override execute(
Expand All @@ -209,10 +250,9 @@ export class CreateIndexesOperation<

const serverWireVersion = maxWireVersion(server);

// Ensure we generate the correct name if the parameter is not set
for (let i = 0; i < indexes.length; i++) {
for (const index of indexes) {
// Did the user pass in a collation, check if our write server supports it
if (indexes[i].collation && serverWireVersion < 5) {
if (index.collation && serverWireVersion < 5) {
callback(
new MongoCompatibilityError(
`Server ${server.name}, which reports wire version ${serverWireVersion}, ` +
Expand All @@ -221,17 +261,6 @@ export class CreateIndexesOperation<
);
return;
}

if (indexes[i].name == null) {
const keys = [];

for (const name in indexes[i].key) {
keys.push(`${name}_${indexes[i].key[name]}`);
}

// Set the name
indexes[i].name = keys.join('_');
}
}

const cmd: Document = { createIndexes: this.collectionName, indexes };
Expand Down Expand Up @@ -271,12 +300,6 @@ export class CreateIndexOperation extends CreateIndexesOperation<string> {
indexSpec: IndexSpecification,
options?: CreateIndexesOptions
) {
// createIndex can be called with a variety of styles:
// coll.createIndex('a');
// coll.createIndex({ a: 1 });
// coll.createIndex([['a', 1]]);
// createIndexes is always called with an array of index spec objects

super(parent, collectionName, [makeIndexSpec(indexSpec, options)], options);
}
override execute(
Expand Down
58 changes: 0 additions & 58 deletions src/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@ import {
import type { Explain } from './explain';
import type { MongoClient } from './mongo_client';
import type { CommandOperationOptions, OperationParent } from './operations/command';
import type { IndexDirection, IndexSpecification } from './operations/indexes';
import type { Hint, OperationOptions } from './operations/operation';
import { PromiseProvider } from './promise_provider';
import { ReadConcern } from './read_concern';
Expand Down Expand Up @@ -104,63 +103,6 @@ export function normalizeHintField(hint?: Hint): Hint | undefined {
return finalHint;
}

interface IndexOptions {
name: string;
keys?: string[];
fieldHash: Document;
}

/**
* Create an index specifier based on
* @internal
*/
export function parseIndexOptions(indexSpec: IndexSpecification): IndexOptions {
const fieldHash: { [key: string]: IndexDirection } = {};
const indexes = [];
let keys;

// Get all the fields accordingly
if ('string' === typeof indexSpec) {
// 'type'
indexes.push(indexSpec + '_' + 1);
fieldHash[indexSpec] = 1;
} else if (Array.isArray(indexSpec)) {
indexSpec.forEach((f: any) => {
if ('string' === typeof f) {
// [{location:'2d'}, 'type']
indexes.push(f + '_' + 1);
fieldHash[f] = 1;
} else if (Array.isArray(f)) {
// [['location', '2d'],['type', 1]]
indexes.push(f[0] + '_' + (f[1] || 1));
fieldHash[f[0]] = f[1] || 1;
} else if (isObject(f)) {
// [{location:'2d'}, {type:1}]
keys = Object.keys(f);
keys.forEach(k => {
indexes.push(k + '_' + (f as AnyOptions)[k]);
fieldHash[k] = (f as AnyOptions)[k];
});
} else {
// undefined (ignore)
}
});
} else if (isObject(indexSpec)) {
// {location:'2d', type:1}
keys = Object.keys(indexSpec);
Object.entries(indexSpec).forEach(([key, value]) => {
indexes.push(key + '_' + value);
fieldHash[key] = value;
});
}

return {
name: indexes.join('_'),
keys: keys,
fieldHash: fieldHash
};
}

const TO_STRING = (object: unknown) => Object.prototype.toString.call(object);
/**
* Checks if arg is an Object:
Expand Down
Loading