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

fix(NODE-3895): Emit TypeScript errors when filtering on fields not in collection schema #3115

Closed

Commits on Jan 25, 2022

  1. 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<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.
    noahsilas committed Jan 25, 2022
    Configuration menu
    Copy the full SHA
    e138f8f View commit details
    Browse the repository at this point in the history
  2. Add Condition query types to root property filters

    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.
    noahsilas committed Jan 25, 2022
    Configuration menu
    Copy the full SHA
    85b7911 View commit details
    Browse the repository at this point in the history
  3. 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.
    noahsilas committed Jan 25, 2022
    Configuration menu
    Copy the full SHA
    60345fc View commit details
    Browse the repository at this point in the history
  4. 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.
    noahsilas committed Jan 25, 2022
    Configuration menu
    Copy the full SHA
    e012ae6 View commit details
    Browse the repository at this point in the history
  5. Improve type safety for recursive objects

    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 committed Jan 25, 2022
    Configuration menu
    Copy the full SHA
    bbb1a6b View commit details
    Browse the repository at this point in the history

Commits on Jan 26, 2022

  1. 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<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)
    noahsilas committed Jan 26, 2022
    Configuration menu
    Copy the full SHA
    6974918 View commit details
    Browse the repository at this point in the history