types(NODE-3563): make update filter types stricter

[ImRodry]

Description

What is changing?

This makes the MatchKeysAndValues type stricter by only allowing strings with dot notation, and not any string like before. This makes it so that you cannot specify any random string that isn't in your type, while also allowing for dot notation to be used (tested).

Is there new documentation needed for these changes?

No

What is the motivation for this change?

I often find myself forgetting some of the names of the properties in my custom types. This makes it so that an error will be thrown when trying to update a non-existent property, while also allowing for dot notation to be used. You can find the jira issue here: https://jira.mongodb.org/browse/NODE-3563. This issue was closed a few months ago, however, I'd like to see some more light brought to it again, which is why I'm making this PR. You can see a preview of these errors in the images below:

Error when specifying a non-existent property:

No error when providing a string with a dot in it:

Double check the following

• Ran npm run check:lint script
• Self-review completed using the steps outlined here
• PR title follows the correct format: <type>(NODE-xxxx)<!>: <description>
• Changes are covered by tests
• New TODOs have a related JIRA ticket

I did not run npm run check:lint as I am not able to install dependencies on this repository due to a node-pre-gyp error.

[dariakp]

@ImRodry Hi there, thank you for reaching out. Have you considered the case of multi-level nested documents and arrays? Dot notation supports an arbitrary number of dots to reach a given array or item element. You may want to take a look at the discussion on a related PR here: #2972

[ImRodry]

I had not seen that PR but it seems rather complex for something that doesn’t need to be this complex. This type also supports infinite dot notations, as it is simply requiring one dot to exist between two strings. If you provide the type “foo.bar.2” this will also work, as “foo” is a string, followed by the dot, followed by the other string, “bar.2”. I don’t see any benefit on that PR over this one, as it seems to create complex types that are harder to maintain

[dariakp]

@ImRodry The complexity of the other PR is because its goal is to provide type checking on the specified keys, not just validate for the presence or absence of a given key (e.g., if your schema defines { foo: { bar: number } }, recognizing it as a valid key but not checking the type only gets us partial type safety).

[ImRodry]

Yeah I do recognize that, however I don’t think it’s possible to cover array indexes with TypeScript types, simply because a number can be a property or an index. I have not tested that PR but if it works, that’s great, however I think that kind of type is really hard to maintain and it should not be within mongodb’s responsibility to do so. I’d also like to know why the PR hasn’t been accepted yet because if it does, I’ll gladly close this one

[dariakp]

@ImRodry I can empathize with wanting to keep things simple, and no need to close this PR quite yet, since we'll want to make sure we have all the use cases covered. The reason the other PR hasn't been accepted is because there were some kinks to work out first with arrays and then with some broken tests. We want to make sure we do our due diligence with a far reaching feature like that and, at the time it was submitted, our highest priority was releasing support for MongoDB 5.1, which we did yesterday. We just didn't want to rush this feature into the same minor version. There have been a number of requests around TypeScript from our users, so we are hoping to base our next minor version around improved TypeScript support.

[dariakp]

@ImRodry Now that we have the the dot notation PR in main, do you feel like it covers your use case or are there edge cases you can think of that are still of concern? If the new Join introduced in the dot PR covers the use case, would you like to update this PR to use that approach?

[ImRodry]

@dariakp so I managed to test the main branch locally and not only did the types not change for me (in terms of type safety, meaning I can still input any string as a valid filter), I now get a new error in only one of my collections with a specific interface

The coll constant is a collection of type MongoStrings, which is defined below

interface MongoStrings {
	projectId: number;
	branches: Branch[];
}

interface Branch {
	id: number;
	name: string;
	title?: string;
	directories: Directory[];
}

interface Directory {
	id: number;
	name: string;
	title?: string;
	branchId: number;
	files: (File | Directory)[];
}

interface File {
	id: number;
	name: string;
	title?: string;
	directoryId: number;
	strings: SourceString[];
}

interface SourceString {
	id: number;
	identifier: string | null;
	fileId: number;
	text: string;
}

So to answer your question, I believe that PR does not solve my use case as I can still input any string as a filter and a new PR should be submitted to fix this bug (I can open an issue if you wish)

[nbbeeken]

Hey @ImRodry, currently our update $set will suggest your schema’s keys and their types, but if you disregard them then you won’t get an error because it will fall back to Record<string, any>. Which is precisely what you're trying to address here, the lack of a type error. But when using $set, that can be a valid use case, you can $set brand new keys on to a document as well as existing ones, and there isn’t a restriction of what type you may be updating an existing key to.

I think the responsibility of our driver's typescript types lie in the area of lending a hand but they can't be strong nor flexible enough to be both a good UX and be the final check on whether an insert is going in with the correct typing. I think that's where testing and mongodb server side validation comes in with things like indexing and setting JSON schemas on collections.

Related to the circular reference:
Unfortunately, the issue you've come across here is known and a limitation of being able to provide completion for sub-documents, typescript's ability to allow us to declare recursive types clashes with our ability to calculate the dot separated keys. We decided that the dot-notation use case was worth the risk of this change, taking a look at the the typescript issues related to this, its seems like something that is on the radar for fixing within the language microsoft/TypeScript#37597

[ImRodry]

Thanks for your explanation! My goal with this PR was to provide better type safety for both the find and update filters, like $set which you mentioned. Since the point of TypeScript is to provide errors before runtime (read before reaching MongoDB server-side validation) I still think this change is worth it as, despite not being flawless (since TypeScript's current features don't even allow such type strength I believe), it's still better than the current type.

As for my issue, since the linked TS issue is on the v4.6 milestone, I think you should wait until that's out to release any new versions if you were planning to do so.

Lastly, please let me know if there's anything worth adding to this PR, otherwise I think it's ready to be merged.

[nbbeeken]

The limitation of getting type errors when attempting to add new keys to a document with $set is something we cannot merge in. And since new keys can be any string, mapped to any type Record<string, any> seems like the right type to stick with. The Partial<TSchema> provides some nice hinting to get good UX in TS powered editors but in the end I think the type has to remain as flexible as it is. As for the self-referential type we're working on #3102 to address some of the friction introduced surrounding that style of schema.

I'll close this for now, but we can continue to discuss improvements that may work with our goals for a driver with flexible and forward compatible types.