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 all 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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
22 changes: 22 additions & 0 deletions src/cmap/connection.ts
Expand Up @@ -594,11 +594,33 @@ export class CryptoConnection extends Connection {
return;
}

// Save sort or indexKeys based on the command being run
baileympearson marked this conversation as resolved.
Show resolved Hide resolved
// the encrypt API serializes our JS objects to BSON to pass to the native code layer
// and then deserializes the encrypted result, the protocol level components
// of the command (ex. sort) are then converted to JS objects potentially losing
// import key order information. These fields are never encrypted so we can save the values
// from before the encryption and replace them after encryption has been performed
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;
}

// Replace the saved values
if (sort != null && (cmd.find || cmd.findAndModify)) {
encrypted.sort = sort;
}
if (indexKeys != null && cmd.createIndexes) {
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
104 changes: 52 additions & 52 deletions src/operations/indexes.ts
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: unknown): x is IndexDirection {
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: { [key: string]: IndexDirection } | Map<string, IndexDirection>;
}

/** @public */
Expand Down Expand Up @@ -130,23 +133,37 @@ 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;
return { ...options, key };
}

/** @internal */
Expand Down Expand Up @@ -183,7 +200,7 @@ export class CreateIndexesOperation<
> extends CommandOperation<T> {
override options: CreateIndexesOptions;
collectionName: string;
indexes: IndexDescription[];
indexes: ReadonlyArray<Omit<IndexDescription, 'key'> & { key: Map<string, IndexDirection> }>;

constructor(
parent: OperationParent,
Expand All @@ -195,8 +212,22 @@ export class CreateIndexesOperation<

this.options = options ?? {};
this.collectionName = collectionName;

this.indexes = indexes;
this.indexes = indexes.map(userIndex => {
// Ensure the key is a Map to preserve index key ordering
const key =
userIndex.key instanceof Map ? userIndex.key : new Map(Object.entries(userIndex.key));
const name = userIndex.name != null ? userIndex.name : Array.from(key).flat().join('_');
const validIndexOptions = Object.fromEntries(
Object.entries({ ...userIndex }).filter(([optionName]) =>
VALID_INDEX_OPTIONS.has(optionName)
)
);
return {
...validIndexOptions,
name,
key
};
});
}

override execute(
Expand All @@ -209,31 +240,6 @@ 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++) {
// Did the user pass in a collation, check if our write server supports it
if (indexes[i].collation && serverWireVersion < 5) {
callback(
new MongoCompatibilityError(
`Server ${server.name}, which reports wire version ${serverWireVersion}, ` +
'does not support collation'
)
);
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 };

if (options.commitQuorum != null) {
Expand Down Expand Up @@ -271,12 +277,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
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