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 type for schema.index() to reflect actual mongoose API #10562

Merged
merged 3 commits into from
Aug 11, 2021

Conversation

JaredReisinger
Copy link
Contributor

@JaredReisinger JaredReisinger commented Aug 9, 2021

Summary

See issue #10561: the Typescript typings for Schema.index() were recently broken. Commit 1c66cc9 broke the typing for Schema.index() by making fields look like mongo's IndexSpecification, when it's not: mongoose has its own idea about what the index specification looks like. This not only undoes that breaking change, but adds additional type-safety so that only actual schema fields can be used in the index definition.

(I'm not 100% sure about the changed return value on Schema.indexes()... there may be prefixing and a background field added, as per https://github.com/Automattic/mongoose/blob/master/lib/helpers/schema/getIndexes.js.)

Fixes #10561.

Examples

With this change, the following code (as Typescript):

import mongoose from "mongoose";

export const schema = new mongoose.Schema({ whatever: String });

schema.index({ whatever: 1 });

will not cause an error. Further, specifying an index of { whatever: 2 } will be an error, as only 1 and -1 are now valid values for the index keys. Also, { somethingElse: 1 } will be an error as somethingElse is not defined as a schema field.

Commit 1c66cc9 broke the typing for `Schema.index()` by making `fields` look like mongo's `IndexSpecification`, when it's not: mongoose has its own idea about what the index specification looks like.

(I'm not 100% sure about the changed return value on `Schema.indexes()`... there may be prefixing and a `background` field added, as per https://github.com/Automattic/mongoose/blob/master/lib/helpers/schema/getIndexes.js.)

Fixes Automattic#10561.
index.d.ts Outdated
@@ -1246,6 +1246,8 @@ declare module 'mongoose' {
type ExtractQueryHelpers<M> = M extends Model<any, infer TQueryHelpers> ? TQueryHelpers : {};
type ExtractMethods<M> = M extends Model<any, any, infer TMethods> ? TMethods : {};

type IndexDefinition<T> = { [K in keyof T]: 1 | -1; };
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you include a IndexDirection type and change this line to use it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... Let me know if you want the additional sorting directions that the newer mongo types allow. I've only ever used 1/-1, and I'm not sure what the built-in mongo version expectations are that mongoose has.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Considering that mongoose v5.x depends on mongodb v3.6 and that this driver version supports MongoDB v4.4, I think we should add the other sorting directions. In MongoDB v4.4 index docs they mention 'text', '2d', '2dsphere' and 'geoHaystack' indexes.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just noticed a problem in allowing the index creation for only the properties declared in schema definition. That's because it's possible to create indexes for fields in embedded documents using "dot notation". So an index for 'location.state', for example, is also valid. See https://docs.mongodb.com/manual/core/index-single/#create-an-index-on-an-embedded-field

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm.... tricky. I don't think Typescript can climb cross-schema references to get an exhaustive list. Next-best would probably be "any prop, or a string that starts with a prop name followed by a period"... but I don't think that's really representable in Typescript. Template literal types (https://www.typescriptlang.org/docs/handbook/2/template-literal-types.html) would allow for expanding/augmenting the property names, but only to a discrete set of concrete values. There's no "startsWith" equivalent in Typescript. All of which leaves us back at "any property name is okay"; not so great for adding type-safety, but at least it shouldn't cause errors.

I'll add the other sorting directions as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I spent a little bit of time seeing if I could create a recursive type that actually would exhaustively generate the allowable index keys... but the generic type parameters on Schema are opaque enough to me that I couldn't tell whether what I was writing would work or not. (In my particular case, we're using a tool that generates the doc/model/schema types from the schema definition code, and I suspect it uses an incomplete definition for the schema... the SchemaDefinitionType generic type parameter is never provided, which means I have no idea what this value is supposed to be/look-like.) I do believe it's possible to do, it's just more intricate than I can tackle at the moment.

In any case, IndexDefinition now allows any string property, and IndexDirection includes:

Copy link
Collaborator

@vkarpov15 vkarpov15 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for finding this issue and sorry for the inconvenience. It looks like we're missing tests on index specifications, we'll add that 👍

@vkarpov15 vkarpov15 added this to the 5.13.7 milestone Aug 11, 2021
@vkarpov15 vkarpov15 merged commit 134cdbd into Automattic:master Aug 11, 2021
vkarpov15 added a commit that referenced this pull request Aug 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug in Typescript definitions for Schema.index()
3 participants