From e138f8faa13e48ac61134038951ece64ce5d6037 Mon Sep 17 00:00:00 2001 From: Noah Silas Date: Mon, 24 Jan 2022 18:24:52 +0000 Subject: [PATCH 1/6] Add TypeScript error when filtering on field not in schema As a developer, I'd like the type system to catch errors I make, like trying to query on a field that doesn't exist, or when I have made a typo in an existing field name. Previously, querying for these fields outside the schema would pass type checks because the `Filter` type included a union with `RootFilterOperators>`, and `RootFilterOperators` was defined as `extends Document`. The BSON `Document` type itself is an object with the index property `{ [key: string]: any }`, meaning that it would match documents with any keys! So, while our type checks would give us an error if we tried to pass the wrong type into a filter expression, trying to use an unknown field name would instead compare the value to the `any` type. By removing the `extends Document` clause of the `RootFilterOperators` type definition, we get much stronger guarantees from the `Filter` type. --- src/mongo_types.ts | 2 +- .../community/collection/filterQuery.test-d.ts | 13 ++++++++++--- 2 files changed, 11 insertions(+), 4 deletions(-) diff --git a/src/mongo_types.ts b/src/mongo_types.ts index 83bf11ad6a5..573fbe664cf 100644 --- a/src/mongo_types.ts +++ b/src/mongo_types.ts @@ -90,7 +90,7 @@ export type AlternativeType = T extends ReadonlyArray export type RegExpOrString = T extends string ? BSONRegExp | RegExp | T : T; /** @public */ -export interface RootFilterOperators extends Document { +export interface RootFilterOperators { $and?: Filter[]; $nor?: Filter[]; $or?: Filter[]; diff --git a/test/types/community/collection/filterQuery.test-d.ts b/test/types/community/collection/filterQuery.test-d.ts index 53c83f586fd..16892a6be83 100644 --- a/test/types/community/collection/filterQuery.test-d.ts +++ b/test/types/community/collection/filterQuery.test-d.ts @@ -126,6 +126,11 @@ expectType[]>( expectType[]>( await collectionT.find({ name: new BSONRegExp('MrMeow', 'i') }).toArray() ); + +/// it should not accept fields that are not in the schema +type FT = Filter; +expectNotType({ missing: true }); + /// it should not accept wrong types for string fields expectNotType>({ name: 23 }); expectNotType>({ name: { suffix: 'Jr' } }); @@ -361,9 +366,11 @@ expectError( otherField: new ObjectId() }) ); -nonObjectIdCollection.find({ - fieldThatDoesNotExistOnSchema: new ObjectId() -}); +expectError( + nonObjectIdCollection.find({ + fieldThatDoesNotExistOnSchema: new ObjectId() + }) +); // we only forbid objects that "look like" object ids, so other random objects are permitted nonObjectIdCollection.find({ From 85b79112ce47cdb5c275893258d01f0235eb2570 Mon Sep 17 00:00:00 2001 From: Noah Silas Date: Mon, 24 Jan 2022 18:37:29 +0000 Subject: [PATCH 2/6] Add `Condition` query types to root property filters The earlier type (`Partial`) would allow queries for exact matches, but didn't cover any of the richer query semantics such as operators or searching arrays by value. Previously, type tests covering the application of these richer query types were spuriously passing because of a comparison to the BSON `Document` type, which is `{ [key: string]: any }`. This change fixes the bulk of test failures that were introduced by the parent commit. --- src/mongo_types.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/mongo_types.ts b/src/mongo_types.ts index 573fbe664cf..7139c1d629f 100644 --- a/src/mongo_types.ts +++ b/src/mongo_types.ts @@ -66,7 +66,9 @@ export type WithoutId = Omit; /** A MongoDB filter can be some portion of the schema or a set of operators @public */ export type Filter = - | Partial + | Partial<{ + [Property in keyof TSchema]: Condition; + }> | ({ [Property in Join>, '.'>]?: Condition< PropertyType, Property> From 60345fca8b14b49b6a06a564af5dbbc3eb293d3b Mon Sep 17 00:00:00 2001 From: Noah Silas Date: Mon, 24 Jan 2022 18:55:49 +0000 Subject: [PATCH 3/6] Fixup transaction type tests These tests are using a filter on the `name` field, which isn't defined in the schema. This is now considered a type error. I've added the `name` field to the schema as a string, which fixes the errors. --- test/types/community/transaction.test-d.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/test/types/community/transaction.test-d.ts b/test/types/community/transaction.test-d.ts index 259d76114db..f09b5e4bc13 100644 --- a/test/types/community/transaction.test-d.ts +++ b/test/types/community/transaction.test-d.ts @@ -8,6 +8,7 @@ const client = new MongoClient(''); const session = client.startSession(); interface Account { + name: string; balance: number; } From e012ae693a5248c778cad3ad04968eb652789a04 Mon Sep 17 00:00:00 2001 From: Noah Silas Date: Mon, 24 Jan 2022 18:53:18 +0000 Subject: [PATCH 4/6] Fixup filterQuery types test This was trying to query on a field not in the schema (`scores`), which now causes a type error. This looks like it was intending to test that you can apply the `$gte` operator to a numeric field, so I've switched this to query the `age` property instead. --- test/types/community/collection/filterQuery.test-d.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/types/community/collection/filterQuery.test-d.ts b/test/types/community/collection/filterQuery.test-d.ts index 16892a6be83..d12822c36cd 100644 --- a/test/types/community/collection/filterQuery.test-d.ts +++ b/test/types/community/collection/filterQuery.test-d.ts @@ -240,7 +240,7 @@ await collectionT.find({ name: { $eq: /Spot/ } }).toArray(); await collectionT.find({ type: { $eq: 'dog' } }).toArray(); await collectionT.find({ age: { $gt: 12, $lt: 13 } }).toArray(); await collectionT.find({ treats: { $eq: 'kibble' } }).toArray(); -await collectionT.find({ scores: { $gte: 23 } }).toArray(); +await collectionT.find({ age: { $gte: 23 } }).toArray(); await collectionT.find({ createdAt: { $lte: new Date() } }).toArray(); await collectionT.find({ friends: { $ne: spot } }).toArray(); /// it should not accept wrong queries From bbb1a6b3c4c6eb5df522b009b07a1fccac035243 Mon Sep 17 00:00:00 2001 From: Noah Silas Date: Tue, 25 Jan 2022 01:00:14 +0000 Subject: [PATCH 5/6] Improve type safety for recursive objects With the removal of the `{ [key: string]: any }` extension on the `Filter` type, we find some serious new shortcomings on the handling of Recursive types: they break type safety for some non-recursive types! Failing Example =============== Consider the case in the tests with the following schema: ``` interface TypedDoc { name: string; age: number; listOfNumbers: number[]; tag: { name: string; }; } ``` The way we were detecting recursion was to check if, for a given `Type extends Object` and a `Key` in that object, if `Type extends Type[Key]`. Note that in the `TypedDoc` interface there is no recursive structure, but that `TypedDoc extends TypedDoc['tag']` is true, because the set of keys in `TypedDoc` is a superset of the keys in `TypedDoc['tag']`. This meant that, for this simple schema, the `tag.name` property was considered to be within a recursion, and so was not getting properly type checked in filtering. Solution Explanation ==================== I've added a new generic type helper, `isInUnion` that checks if `A` is a union type that includes `B`. We start by using the `Extract` utility to find the set of types in `A` that are assignable to `B`. If the only overlapping entry is `B` itself, then either: - `A` and `B` are the same type, or - `A` is a union type that includes `B`. Of course, testing equality of Types is also not trivial; there is some discussion at https://github.com/microsoft/TypeScript/issues/27024, with a solution by [@mattmccutchen](https://github.com/mattmccutchen). This should be enough to differentiate when we have recursive structures, even when the recursive part is inside a union. Downsides ========= There are several test cases for recursive data structures that become more cumbersome with the improved type checking. Where the earlier type check would allow querying with deeply nested recursive values, the stricter type checks now added would require that the query author annotate the key with a `@ts-expect-error` directive to opt-out of the type checker. This is likely to be somewhat irksome for folks who commonly write these types of queries, but I believe that this is preferable to the loss of type safety that used to exist (as these were implicitly matching an `any` value). Follow Ups ========== I suspect that it is possible to write a much simpler version of the `NestedPaths` helper; for example, [@jcalz](https://github.com/jcalz) offers a much tighter implementation in https://stackoverflow.com/questions/58434389/typescript-deep-keyof-of-a-nested-object/58436959#58436959 that uses a maximum depth to allow traversing recursive data structures up to a given limit. This kind of implementation would improve the type safety of recursive data structures at the cost of limiting the depth of query nodes. I'm not in a position to understand the common usage of the library, so I'm leaving this alone for future contributors to consider. --- src/index.ts | 2 ++ src/mongo_types.ts | 22 ++++++++++++++++--- .../findX-recursive-types.test-d.ts | 15 ++++++++----- 3 files changed, 31 insertions(+), 8 deletions(-) diff --git a/src/index.ts b/src/index.ts index eb7cc75f971..76f9a599716 100644 --- a/src/index.ts +++ b/src/index.ts @@ -284,6 +284,7 @@ export type { InferIdType, IntegerType, IsAny, + IsInUnion, Join, KeysOfAType, KeysOfOtherType, @@ -306,6 +307,7 @@ export type { RootFilterOperators, SchemaMember, SetFields, + TypeEquals, UpdateFilter, WithId, WithoutId diff --git a/src/mongo_types.ts b/src/mongo_types.ts index 7139c1d629f..c3112de3b98 100644 --- a/src/mongo_types.ts +++ b/src/mongo_types.ts @@ -481,6 +481,24 @@ export type PropertyType = string extends Propert : unknown : unknown; +/** + * Check if two types are exactly equal + * + * From https://github.com/microsoft/TypeScript/issues/27024#issuecomment-421529650, + * credit to https://github.com/mattmccutchen. + * @public + */ +// prettier-ignore +export type TypeEquals = + (() => T extends X ? 1 : 2) extends + (() => T extends Y ? 1 : 2) ? true : false + +/** + * Check if A is a union type that includes B + * @public + */ +export type IsInUnion = TypeEquals, B>; + /** * @public * returns tuple of strings (keys to be joined on '.') that represent every path into a schema @@ -506,9 +524,7 @@ export type NestedPaths = Type extends ? { [Key in Extract]: Type[Key] extends Type // type of value extends the parent ? [Key] - : // for a recursive union type, the child will never extend the parent type. - // but the parent will still extend the child - Type extends Type[Key] + : IsInUnion extends true ? [Key] : Type[Key] extends ReadonlyArray // handling recursive types with arrays ? Type extends ArrayType // is the type of the parent the same as the type of the array? diff --git a/test/types/community/collection/findX-recursive-types.test-d.ts b/test/types/community/collection/findX-recursive-types.test-d.ts index 96c4e095cce..86fc54127e8 100644 --- a/test/types/community/collection/findX-recursive-types.test-d.ts +++ b/test/types/community/collection/findX-recursive-types.test-d.ts @@ -87,6 +87,7 @@ recursiveOptionalCollection.find({ * recursive union types are supported */ interface Node { + value?: string; next: Node | null; } @@ -103,10 +104,12 @@ expectError( ); nodeCollection.find({ - 'next.next': 'asdf' + 'next.value': 'asdf' }); -nodeCollection.find({ 'next.next.next': 'yoohoo' }); +// type safety is lost through recursive relations; callers will +// need to annotate queries with `ts-expect-error` comments +expectError(nodeCollection.find({ 'next.next.value': 'yoohoo' })); /** * Recursive schemas with arrays are also supported @@ -149,9 +152,11 @@ expectError( // type safety breaks after the first // level of nested types -recursiveSchemaWithArray.findOne({ - 'branches.0.directories.0.files.0.id': 'hello' -}); +expectError( + recursiveSchemaWithArray.findOne({ + 'branches.0.directories.0.files.0.id': 'hello' + }) +); recursiveSchemaWithArray.findOne({ branches: [ From 6974918a1125afbaf486efacc177af04c77de73d Mon Sep 17 00:00:00 2001 From: Noah Silas Date: Tue, 25 Jan 2022 21:21:50 +0000 Subject: [PATCH 6/6] Re-add unsafe compatability with unreleased root operators From [@dariakp](https://github.com/dariakp): > ... the reason we have RootFilterOperators extend Document is so that > any driver version can be forwards compatible with new query operators > that are continually being added to the server. There is an > improvement that we would love to make to only extend $-prefixed keys > instead of the generic Document (since operators are always > $-prefixed), but we haven't had a chance to look into making that > work. Here we introduce such a type, but in a much more restricted format than the `BSON.Document` type formerly extended: - This is scoped explicitly to keys prefixed with `$` - We can use the `unknown` type instead of `any`; in the event that a user is attempting to extract a value from an object typed as a `Filter` and use it somewhere they will be notified that it is not type safe and required to use a type cast or other workaround. Follow-ups: - Consider preferring safety of existing types over compatibility with with future operators (https://jira.mongodb.org/browse/NODE-3904) --- src/mongo_types.ts | 17 ++++++++++++++++- .../community/collection/filterQuery.test-d.ts | 3 +++ 2 files changed, 19 insertions(+), 1 deletion(-) diff --git a/src/mongo_types.ts b/src/mongo_types.ts index c3112de3b98..df52267e98b 100644 --- a/src/mongo_types.ts +++ b/src/mongo_types.ts @@ -91,8 +91,23 @@ export type AlternativeType = T extends ReadonlyArray /** @public */ export type RegExpOrString = T extends string ? BSONRegExp | RegExp | T : T; +/** + * This is a type that allows any `$` prefixed keys and makes no assumptions + * about their value types. This stems from a design decision that newly added + * filter operators should be accepted without needing to upgrade this package. + * + * This has the unfortunate side effect of preventing type errors on unknown + * operator keys, so we should prefer not to extend this type whenever possible. + * + * @see https://github.com/mongodb/node-mongodb-native/pull/3115#issuecomment-1021303302 + * @public + */ +export type OpenOperatorQuery = { + [key: `$${string}`]: unknown; +}; + /** @public */ -export interface RootFilterOperators { +export interface RootFilterOperators extends OpenOperatorQuery { $and?: Filter[]; $nor?: Filter[]; $or?: Filter[]; diff --git a/test/types/community/collection/filterQuery.test-d.ts b/test/types/community/collection/filterQuery.test-d.ts index d12822c36cd..9adbe07977d 100644 --- a/test/types/community/collection/filterQuery.test-d.ts +++ b/test/types/community/collection/filterQuery.test-d.ts @@ -288,6 +288,9 @@ expectNotType>({ name: { $or: ['Spot', 'Bubbles'] } }); /// it should not accept single objects for __$and, $or, $nor operator__ query expectNotType>({ $and: { name: 'Spot' } }); +/// it allows using unknown root operators with any value +expectAssignable>({ $fakeOp: 123 }); + /** * test 'element' query operators */