feat(NODE-3589): support dot-notation attributes in Filter

[avaly]

Description

Currently, dot-notation attributes (e.g. foo.bar) in filter objects do not get any type checks.

What changed?

This PR changes the implementation of the Filter type to also support dot-notation attributes inside nested objects.

This PR requires TypeScript >= 4.1, because it relies on template literal types (see: microsoft/TypeScript#40336)

[avaly]

Unrelated to this PR: when I run yarn test on the 4.1 branch (which this PR is based on), I get 20 TS errors related to the new useUnknownInCatchVariables strict option added in typescript v4.4.

[nbbeeken]

Hi @avaly thanks for the contribution! This does look like something we would be interested in merging but there's some concern about what this does to compile times. Do you have any reference on how a deeply nested schema could impact compilation time?

As for the TS failures, our package-lock file should have our TS version set to 4.3.5 (even thought package.json has ^4.3.5) we haven't had the chance to adopt 4.4 yet due to other tooling we depend on not having yet adopted 4.4 either.

[avaly]

Do you have any reference on how a deeply nested schema could impact compilation time?

I am happy to run some tests. How do you suggest I measure the impact? Simply track the time it takes to compile a big schema? How have you done similar measurements in the past?

[nbbeeken]

There appears to be some failures with the tsd tests.

tsc --noEmit mongodb.d.ts && tsd
[2021/09/07 18:31:24.509]   test/types/community/collection/filterQuery.test-d.ts:96:0
[2021/09/07 18:31:24.509]   ✖  54:14  Type of property bestFriend circularly references itself in mapped type { [K in keyof PetModel]: PetModel[K] extends PetModel ? never : [K, ...NestedPaths<PetModel[K]>]; }.
[2021/09/07 18:31:24.509]   ✖  96:0   Parameter type Filter<PetModel> is declared too wide for argument type { 'meta.updatedAt': Date; }.
[2021/09/07 18:31:24.509]   test/types/union_schema.test-d.ts:64:0
[2021/09/07 18:31:24.509]   ✖  64:0   Argument of type { meow: string; bark: string; } is assignable to parameter of type Filter<(Without<Dog, Cat> & Cat) | (Without<Cat, Dog> & Dog)>.
[2021/09/07 18:31:24.509]   3 errors

I think we can establish a base level, a simple schema like the Pet one in the testing examples, does compilation time change before and after your changes?

We can calculate out some worst case scenarios in terms of how many keys TS going to have to generate and typecheck but if we can already see a difference with simple examples that would give some indicator. The risk is not really compilation times for shipping JS but potentially slowing down VSCode's insights.

I still have to take a close look at the Join helper, does this support array notation too? And this should support any level of nesting right? Can you add a test for more nested levels? just to sanity check.

[avaly]

I fixed some of the TS issues. Unfortunately using these new helper types, schemas which reference themselves internally are not going to be supported (e.g. PetModel.bestFriend used to be PetModel before).

I still can't figure out how to solve the union schema TS error. TBH I wasn't even aware that was valid syntax.

[avaly]

I ran a compilation time test locally on this branch and on the latest commit on the 4.1 branch.

I used this test file (interface used from test/types/example_schemas.ts): https://gist.github.com/avaly/78805f42fd48167c0f9979aa47805812

4.1 branch

$ tsc --extendedDiagnostics --noEmit test-compile-time.ts 
Files:                         143
Lines of Library:            28119
Lines of Definitions:        32970
Lines of TypeScript:            36
Lines of JavaScript:             0
Lines of JSON:                   0
Lines of Other:                  0
Nodes of Library:           119708
Nodes of Definitions:       129462
Nodes of TypeScript:           120
Nodes of JavaScript:             0
Nodes of JSON:                   0
Nodes of Other:                  0
Identifiers:                 88468
Symbols:                     72542
Types:                       24958
Instantiations:              17073
Memory used:               128265K
Assignability cache size:     6736
Identity cache size:           103
Subtype cache size:              0
Strict subtype cache size:       0
I/O Read time:               0.01s
Parse time:                  0.57s
ResolveModule time:          0.04s
ResolveTypeReference time:   0.02s
Program time:                0.67s
Bind time:                   0.30s
Check time:                  1.44s
printTime time:              0.00s
Emit time:                   0.00s
Total time:                  2.41s

this branch

$ tsc --extendedDiagnostics --noEmit test-compile-time.ts 
Files:                         143
Lines of Library:            28119
Lines of Definitions:        32984
Lines of TypeScript:            36
Lines of JavaScript:             0
Lines of JSON:                   0
Lines of Other:                  0
Nodes of Library:           119708
Nodes of Definitions:       129667
Nodes of TypeScript:           120
Nodes of JavaScript:             0
Nodes of JSON:                   0
Nodes of Other:                  0
Identifiers:                 88521
Symbols:                     77118
Types:                       29383
Instantiations:              21830
Memory used:               138890K
Assignability cache size:     7130
Identity cache size:           103
Subtype cache size:             14
Strict subtype cache size:       0
I/O Read time:               0.01s
Parse time:                  0.55s
ResolveModule time:          0.04s
ResolveTypeReference time:   0.02s
Program time:                0.64s
Bind time:                   0.30s
Check time:                  1.49s
printTime time:              0.00s
Emit time:                   0.00s
Total time:                  2.44s

The time diffs are minimal. But worth the safety they bring IMHO.

[nbbeeken]

Thanks for running the diagnostics! @dariakp and I figured out that calculating how many keys typescript has to build its the same formula as calculating the total number of vertices in a complete tree. Each node being an interface and the edges leaving it being the number of keys. So it means that the generation space doesn't grow as fast as I had assumed it would. It still does create a lot of keys but not exponential growth.

Unfortunately, I believe the self referencing schema as well as the exclusive union issue is a blocker. The union tests come from the community types (and I think from the Typescript handbook, but its been a while). No longer supporting these would be a breaking change. I don't believe there is, but do you know if there is any way to detect these usage styles and omit the dot-notation support?

Beyond the scope of what we're addressing here but it would make sense to investigate supporting array notation if we were to make this project fully support MongoDB syntax.

[avaly]

I'll try to see if I can find a way to omit the self-referenced schemas. I believe array-support is easy to add also, I'll look into that as well.

However, for the union schemas, I'm trying to figure out where those tests came from and if they actually make sense to keep around. They seems to have been added in #2767. But they're not in the latest tests from DefinitelyTyped version in DefinitelyTyped/DefinitelyTyped#54510. Do you have any clue of their source @nbbeeken (since you were the author of #2767)?

[avaly]

I've added support for dot-notation array fields.

[dariakp]

@avaly Please note that there was a CI issue that blocked all tests from running which necessitated a change to our main branch for the fix, could you rebase your branch to pull them in here?

[avaly]

@dariakp I rebased on the latest main commits this morning.

@nbbeeken What do you think about removing that union test case? (#2972 (comment))

[nbbeeken]

I think we can drop it safely, I wasn't able to find where it originally came from, it might have been something that I considered should work and be supported, but it isn't native to TS as in it requires arguably significant manual type logic.

I plan to do a full review of this very soon, but if you wanna go ahead and remove those now please feel free.

[nbbeeken]

I pushed some tests for extra cases I found. I think we can make nested arrays work, thoughts?

[avaly]

@nbbeeken Thanks for the extra tests. I fixed the outstanding issues.

[avaly]

I fixed some issues that showed up after the TypeScript v4.5 upgrade.

[nbbeeken]

Thanks so much for your patience as we consider this feature, I just made a small fix to our linting, and this otherwise LGTM. I'll let the rest of the team take a look themselves but 👍

[baileympearson]

Hey Avaly, thanks so much for contributing this feature! I took a look at the PR, and it's looking pretty good.

I did notice one issue with the implementation. The current type setup isn't able to determine type information for a document if it contains an array of a union type, or for a heterogeneous tuple type. For example:

Notice that we do get the expected type errors if we remove the unions:

I think this is something we'd like to try to fix, if possible!

[baileympearson]

Hey @avaly, thanks again for contributing this PR!

I wanted to follow up and see if you've had a chance to look at Neal's comment or my comment. Neal's comment is definitely blocking this PR from being ready to merge but mine is not (it would just be nice to have 😄).

We think that the PR looks great overall, and would like to try and merge it by the end of the week. Would you have a chance to take resolve the merge conflicts and address Neal's comment before then? If not, I'm happy to take over and get it ready to merge.

Thank again for your help!

[avaly]

@baileympearson I'll try to look into it this week, but I worry the union type issue might be hard to support.

[baileympearson]

@avaly If we can't resolve the union typing issue, that's all right. But we will need to resolve the merge conflict and add Map to the list of types we support as Neal suggested

[avaly]

@baileympearson @nbbeeken I've rebased this PR on the latest main and added support for Map.

[nbbeeken]

Made some small fixes to the nested types escape hatch:

• RegExp/Buffer/Uint8Array - these are directly serialized by the BSON lib
• ((...args: any[]) => any) - the Function type is recursive, and you can serialize them with the serializeFunctions flag.
• { _bsontype: string } - this should cover all our custom BSON types so we don't have to worry about listing them each out.

I think this is ready! (just waiting on CI) LGTM! Thanks again @avaly this is a really awesome feature add!

[dariakp]

LGTM, thank you!

[marcj]

You could add support for circular types using an accumulator as depth checker and bail out when too deep. This works around the TS error "TS2589: Type instantiation is excessively deep and possibly infinite.". A max depth of 10 is probably enough for most cases without making the TS checker too slow.

Example

type O<T, K extends string, Prefix extends string, Depth extends number[]> = K extends keyof T ? { [_ in `${Prefix}.${K}`]: T[K] } & (T[K] extends object ? SubObjects<T[K], Extract<keyof T[K], string>, `${Prefix}.${K}`, [...Depth, 1]> : {}) : {};

type SubObjects<T, K extends string, Prefix extends string, Depth extends number[]> =
    Depth['length'] extends 10 ? {} : //bail out when too deep
        K extends keyof T ? T[K] extends Array<infer A> ? SubObjects<A, Extract<keyof A, string>, `${Prefix}.${K}.${number}`, [...Depth, 1]> :
            T[K] extends object ? O<T[K], Extract<keyof T[K], string>, Prefix extends '' ? K : `${Prefix}.${K}`, Depth> : { [P in Prefix extends '' ? K :`${Prefix}.${K}`]: T[K] } : {};

type FilterQuery<T> = SubObjects<T, Extract<keyof T, string>, '', []>;

Playground link

[dariakp]

Thanks for the suggestion @marcj! We're going to look into this further (I created NODE-3875 for tracking). For now we have released 4.3.1 which contains a fix for this issue in the case of directly self-referential types.