fix(NODE-3895): Emit TypeScript errors when filtering on fields not in collection schema #3115
Conversation
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<TSchema>` type included a union with
`RootFilterOperators<WithId<TSchema>>`, 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.
The earlier type (`Partial<TSchema>`) 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.
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.
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.
With the removal of the `{ [key: string]: any }` extension on the
`Filter<TSchema>` 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<A,B>` 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 microsoft/TypeScript#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.
|
@noahsilas Thank you for your thoughtful PR and suggestions here: 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 |
|
@dariakp Can I encourage a reversal of that thinking? While I appreciate the idea of future compatibility, any usage of those operators would still be type-unsafe until the user upgrades (as the value would implicitly and silently be In the meantime, folks trying to use the existing operators also lose some of their type safety. For instance, I personally often try to use Finally, it's possible for people writing queries with the latest-and-greatest operators to explicitly opt-out of the type checker if they know that their server supports a query operator that isn't on this explicit list of known operators in the TypeScript definition: they can annotate that query with Thank you for considering this alternate point of view! 😄 |
|
@noahsilas We are always looking to improve the user experience, so we'll discuss this further at our next team meeting to see if we can come up with some less breaking way to allow for this frequently requested feature. Thanks again for reaching out! |
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<TSchema>` 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)
|
@dariakp Thank you for the context! For now I've implemented the improvement you asked for where the |
|
Hey @noahsilas, thanks for taking the time with this PR. After discussing this with the team, we decided that it would be best to defer strengthening the Filter type to a future major version release and ensure that we have a proper technical design for the functionality going forward. Some of our concerns were: Recursive TypesAt this point in time, we believe the solution we have is preferrable to the solution in this PR. We do not believe it's best practice to make a change that causes users to annotate type errors with @ts-expect-error or a similar compiler directive. Recursive schemas are permitted in both Typescript and the Node driver, and with the changes to recursive types included in this PR valid recursive type queries will show up as compiler errors. Our philosophy is that in the scenario where the driver can't provide strict type safety, we should default to any so we don't cause any unnecessary errors in user's code. Limiting the extension to $-keys instead of DocumentWhile this is something we would like to see merged eventually, the current implementation breaks recursive types and dot notation. For a recursive type, the dot-notation key will not be generated as a part of the NestedPaths type (the core component of our recursive types implementation). In this scenario, the dot-notation key is permitted by the extends Document. By limiting the extends Document to only $-prefixed keys, we will cause errors for valid dot notation keys for recursive types. Similar to the previous section, we will not merge changes that cause type errors for a valid filter. |
Description
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.
For example:
Related JIRA: https://jira.mongodb.org/browse/NODE-3895
What is changing?
The TypeScript
Filter<TSchema>definition is becoming more type safe.Previously, querying for these fields outside the schema would pass type
checks because the
Filter<TSchema>type included a union withRootFilterOperators<WithId<TSchema>>, andRootFilterOperatorswasdefined as
extends Document. The BSONDocumenttype itself is anobject with the index property
{ [key: string]: any }, which seemed to matchany keys present in the document that didn't have stricter rules applied.
By removing
extends Documentfrom that definition, we gain significanttype safety; several issues in the tests turned up. This also showed some
difficulties with the recent typing additions in #3102 - some tests started
failing with this change because the
NestedPath<>generation was bailingout early even on non-recursive structures (when a nested object happened
to have a subset of keys of the parent). The final commit in this series
improves how recursive unions are detected.
As one final note, folks who have started using that new recursive object
support will find that unsupported queries (those reaching through a
recursive relation) will now need to be decorated with a
@ts-expect-erroror similar annotation. This should call attention to these queries lack of
type safety (instead of the current behavior where the filter is implicitly
cast to an
any).Is there new documentation needed for these changes?
Good question! I didn't find any very meaningful documentation about the
existing TypeScript experience of this library, but if that exists somewhere
I'd be happy to update it given a pointer to the page.
Double check the following
npm run check:lintscript<type>(NODE-xxxx)<!>: <description>