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.

Sign up for GitHub

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

Jump to bottom

fix: fixed the typing of FilterQuery<T> type to prevent it from only getting typed to any #14398

Closed
wants to merge 5 commits into from
Closed

fix: fixed the typing of FilterQuery<T> type to prevent it from only getting typed to any #14398

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
6de3a0a
fix: fixed the typing of FilterQuery<T> type to prevent it from only …
FaizBShah Mar 1, 2024
303ab87
fixed typing issues
FaizBShah Mar 2, 2024
4278466
fix: increase tsc-diagnostics-check maxInstantiations value
FaizBShah Mar 2, 2024
5313d48
added support for array types multi-inference types
FaizBShah Mar 4, 2024
307c1cc
chore: seperated tests
FaizBShah Mar 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion scripts/tsc-diagnostics-check.js
View file
Open in desktop
Expand Up @@ -3,7 +3,7 @@
const fs = require('fs');

const stdin = fs.readFileSync(0).toString('utf8');
const maxInstantiations = isNaN(process.argv[2]) ? 120000 : parseInt(process.argv[2], 10);
const maxInstantiations = isNaN(process.argv[2]) ? 130000 : parseInt(process.argv[2], 10);

console.log(stdin);

Expand Down
23 changes: 15 additions & 8 deletions test/types/queries.test.ts
View file
Open in desktop
Expand Up @@ -84,10 +84,15 @@ Test.find({ parent: { $in: ['0'.repeat(24)] } });
Test.find({ name: { $in: ['Test'] } }).exec().then((res: Array<ITest>) => console.log(res));
Test.find({ tags: 'test' }).exec();
Test.find({ tags: { $in: ['test'] } }).exec();
Test.find({ tags: /test/ }).exec();
Test.find({ tags: { $in: [/test/] } }).exec();

// Implicit `$in`
Test.find({ name: ['Test1', 'Test2'] }).exec();

// Implicit `$in` for regex string
Test.find({ name: [/Test1/, /Test2/] });

Test.find({ name: 'test' }, (err: Error | null, docs: ITest[]) => {
console.log(!!err, docs[0].age);
});
Expand Down Expand Up @@ -311,16 +316,18 @@ function gh11964() {

type WithId<T extends object> = T & { id: string };

class Repository<T extends object> {
/* ... */
type TestUser = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please leave the class Repository<> here, this test case was specifically for an issue that occurred with generic classes.

Copy link
Contributor Author

@FaizBShah FaizBShah Mar 5, 2024 •
edited

Choose a reason for hiding this comment

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

I actually did it because it was failing, but I figured out a way to fix this now by writing it like this and keeping the class Repository<>:

class Repository<T extends object> {
    find(id: string) {
      const idCondition: Condition<ReturnType<(arg: WithId<T>) => typeof arg.id>> = id;
      const filter: FilterQuery<WithId<T>> = { id } as FilterQuery<WithId<T>>;
    }
  }

Condition<ReturnType<(arg: WithId<T>) => typeof arg.id>> is just a more type-safe equivalent way of writing Condition<WithId<T>['id']>

Not sure if this is the best way, but its passing the tests + you still get autocompletion for stuffs like $eq, $in, etc. in the above test too

Copy link
Collaborator

Choose a reason for hiding this comment

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

I would strongly prefer if this test passed as written before we merge this PR. I won't say it's a hard blocker, but we shouldn't merge this PR unless either this test passes or we have a very good explanation of why this test is failing. And "just write code this other way" is generally not a good enough reason, because different people write code in different ways and we should meet users where they are.

Copy link
Contributor Author

@FaizBShah FaizBShah Mar 8, 2024 •
edited

Choose a reason for hiding this comment

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

@vkarpov15 For the original test, i.e. the Repository one, I tried different ways to write the FilterQuery and ApplyBasicQueryCasting, but was not able to resolve it without introducing any in the types, which makes the entire type cast to any by default and completely defeats the purpose of this PR.

I was confused why it was working completely fine with a concrete interface like TestUser or something, but was failing with the type parameter T. On researching a little bit, I came to realize the why reason why it was failing was because Typescript doesn't have enough knowledge on whether the parameter T also contains a field id or not, and what is its type? For example, if T also contains a field id: number, then resultant type formed by WithId<T> will have id: never. Therefore, if we try to assign any value to the id, it will obviously fail.

So, currently, the test is written with the assumption that id will not be present in T. And since Typescript doesn't have that information about T during compile-time, and thus what should be the end type of the field id, hence its throwing incorrect type assignment error. Im not sure what other way to make that existing test pass is other than re-introducing any, but then that will make this PR useless

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is the reason why I wrote the above test. If you approve it, I can write the above test in a separate test function and push that commit to this PR. As for the existing old test, do suggest what should be done about 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.

@vkarpov15 Also, this is a wrong statement which will always fail even if you give a concrete interface implementation instead of just parameter T:

const idCondition: Condition<WithId<T>>['id'] = id;

The reason is because Condition<K> will return something like K | K[] | .... Now, if the return type of Condition<K> is K[], then it will not have the property ['id'] obviously since its an array. So, the above statement was also written with incorrect typescript assumption

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@vkarpov15 I have added new commit separating out the tests. Can you review the changes now?

name: string;
age: number;
};

find(id: string) {
const idCondition: Condition<WithId<T>>['id'] = id; // error :(
const filter: FilterQuery<WithId<T>> = { id }; // error :(
const id: string = 'Test Id';

/* ... */
}
}
let idCondition: Condition<WithId<TestUser>['id']>;
let filter: FilterQuery<WithId<TestUser>>;

expectAssignable<typeof idCondition>(id);
expectAssignable<typeof filter>({ id });
}

function gh12091() {
Expand Down
71 changes: 43 additions & 28 deletions types/query.d.ts
View file
Open in desktop
@@ -1,8 +1,23 @@
declare module 'mongoose' {
import mongodb = require('mongodb');

export type ApplyBasicQueryCasting<T> = T | T[] | (T extends (infer U)[] ? U : any) | any;
type Condition<T> = ApplyBasicQueryCasting<T> | QuerySelector<ApplyBasicQueryCasting<T>>;
type StringQueryTypeCasting = string | RegExp;
type ObjectIdQueryTypeCasting = Types.ObjectId | string;
type UUIDQueryTypeCasting = Types.UUID | string;
type BufferQueryTypeCasting = Buffer | any;

type QueryTypeCasting<T> = T extends string
? StringQueryTypeCasting
: T extends Types.ObjectId
? ObjectIdQueryTypeCasting
: T extends Types.UUID
? UUIDQueryTypeCasting
: T extends Buffer
? BufferQueryTypeCasting
: T;

export type ApplyBasicQueryCasting<T> = T | T[] | (T extends (infer U)[] ? QueryTypeCasting<U> : never);
vkarpov15 marked this conversation as resolved.
Show resolved Hide resolved
type Condition<T> = ApplyBasicQueryCasting<QueryTypeCasting<T>> | QuerySelector<ApplyBasicQueryCasting<QueryTypeCasting<T>>>;

type _FilterQuery<T> = {
[P in keyof T]?: Condition<T[P]>;
Expand Down Expand Up @@ -237,7 +252,7 @@ declare module 'mongoose' {
allowDiskUse(value: boolean): this;

/** Specifies arguments for an `$and` condition. */
and(array: FilterQuery<RawDocType>[]): this;
and(array: FilterQuery<DocType>[]): this;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why changing from RawDocType to DocType here? I think RawDocType is more correct to avoid filtering by document properties like $isNew.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason for changing this was because some test cases like the following autoTypedQuery() were failing w.r.t. types:

function autoTypedQuery() {
  const AutoTypedModel = autoTypedModel();
  const query = AutoTypedModel.find();
  expectType<typeof query>(AutoTypedModel.find().byUserName('')); // This was throwing type error
}

Now, the reason these function were failing because in models.d.ts, a function like find() was defined like this:

find<ResultDoc = THydratedDocumentType>(
    ): QueryWithHelpers<Array<ResultDoc>, ResultDoc, TQueryHelpers, TRawDocType, 'find'>;

Notice how in the above declared find() type, DocType = ResultDoc and RawDocType = TRawDocType for the corresponding Query type which will be generated by the QueryWithHelpers type. This also means that the corresponding filters type for that query will be FilterQuery<TRawDocType>

BUT, in the above autoTypedTest's byUserName() method, the this.query context being used inside that method has the type Query with both DocType and RawDocType being set as the THydratedDocumentType due to auto typing. Thus, when you call the byUserMethod, it expects filters of type FilterQuery<ResultDoc> instead of FilterQuery<TRawDocType>, thus creating a type mismatch error.

To prevent the above type conflict, I chose to replace the RawDocType with just DocType


/** Specifies the batchSize option. */
batchSize(val: number): this;
Expand Down Expand Up @@ -286,7 +301,7 @@ declare module 'mongoose' {

/** Specifies this query as a `countDocuments` query. */
countDocuments(
criteria?: FilterQuery<RawDocType>,
criteria?: FilterQuery<DocType>,
options?: QueryOptions<DocType>
): QueryWithHelpers<number, DocType, THelpers, RawDocType, 'countDocuments'>;

Expand All @@ -302,10 +317,10 @@ declare module 'mongoose' {
* collection, regardless of the value of `single`.
*/
deleteMany(
filter?: FilterQuery<RawDocType>,
filter?: FilterQuery<DocType>,
options?: QueryOptions<DocType>
): QueryWithHelpers<any, DocType, THelpers, RawDocType, 'deleteMany'>;
deleteMany(filter: FilterQuery<RawDocType>): QueryWithHelpers<
deleteMany(filter: FilterQuery<DocType>): QueryWithHelpers<
any,
DocType,
THelpers,
Expand All @@ -320,10 +335,10 @@ declare module 'mongoose' {
* option.
*/
deleteOne(
filter?: FilterQuery<RawDocType>,
filter?: FilterQuery<DocType>,
options?: QueryOptions<DocType>
): QueryWithHelpers<any, DocType, THelpers, RawDocType, 'deleteOne'>;
deleteOne(filter: FilterQuery<RawDocType>): QueryWithHelpers<
deleteOne(filter: FilterQuery<DocType>): QueryWithHelpers<
any,
DocType,
THelpers,
Expand All @@ -335,7 +350,7 @@ declare module 'mongoose' {
/** Creates a `distinct` query: returns the distinct values of the given `field` that match `filter`. */
distinct<DocKey extends string, ResultType = unknown>(
field: DocKey,
filter?: FilterQuery<RawDocType>
filter?: FilterQuery<DocType>
): QueryWithHelpers<Array<DocKey extends keyof DocType ? Unpacked<DocType[DocKey]> : ResultType>, DocType, THelpers, RawDocType, 'distinct'>;

/** Specifies a `$elemMatch` query condition. When called with one argument, the most recent path passed to `where()` is used. */
Expand Down Expand Up @@ -375,52 +390,52 @@ declare module 'mongoose' {

/** Creates a `find` query: gets a list of documents that match `filter`. */
find(
filter: FilterQuery<RawDocType>,
filter: FilterQuery<DocType>,
projection?: ProjectionType<RawDocType> | null,
options?: QueryOptions<DocType> | null
): QueryWithHelpers<Array<DocType>, DocType, THelpers, RawDocType, 'find'>;
find(
filter: FilterQuery<RawDocType>,
filter: FilterQuery<DocType>,
projection?: ProjectionType<RawDocType> | null
): QueryWithHelpers<Array<DocType>, DocType, THelpers, RawDocType, 'find'>;
find(
filter: FilterQuery<RawDocType>
filter: FilterQuery<DocType>
): QueryWithHelpers<Array<RawDocType>, DocType, THelpers, RawDocType, 'find'>;
find(): QueryWithHelpers<Array<DocType>, DocType, THelpers, RawDocType, 'find'>;

/** Declares the query a findOne operation. When executed, returns the first found document. */
findOne(
filter?: FilterQuery<RawDocType>,
filter?: FilterQuery<DocType>,
projection?: ProjectionType<RawDocType> | null,
options?: QueryOptions<DocType> | null
): QueryWithHelpers<DocType | null, DocType, THelpers, RawDocType, 'findOne'>;
findOne(
filter?: FilterQuery<RawDocType>,
filter?: FilterQuery<DocType>,
projection?: ProjectionType<RawDocType> | null
): QueryWithHelpers<DocType | null, DocType, THelpers, RawDocType, 'findOne'>;
findOne(
filter?: FilterQuery<RawDocType>
filter?: FilterQuery<DocType>
): QueryWithHelpers<DocType | null, RawDocType, THelpers, RawDocType, 'findOne'>;

/** Creates a `findOneAndDelete` query: atomically finds the given document, deletes it, and returns the document as it was before deletion. */
findOneAndDelete(
filter?: FilterQuery<RawDocType>,
filter?: FilterQuery<DocType>,
options?: QueryOptions<DocType> | null
): QueryWithHelpers<DocType | null, DocType, THelpers, RawDocType, 'findOneAndDelete'>;

/** Creates a `findOneAndUpdate` query: atomically find the first document that matches `filter` and apply `update`. */
findOneAndUpdate(
filter: FilterQuery<RawDocType>,
filter: FilterQuery<DocType>,
update: UpdateQuery<RawDocType>,
options: QueryOptions<DocType> & { includeResultMetadata: true }
): QueryWithHelpers<ModifyResult<DocType>, DocType, THelpers, RawDocType, 'findOneAndUpdate'>;
findOneAndUpdate(
filter: FilterQuery<RawDocType>,
filter: FilterQuery<DocType>,
update: UpdateQuery<RawDocType>,
options: QueryOptions<DocType> & { upsert: true } & ReturnsNewDoc
): QueryWithHelpers<DocType, DocType, THelpers, RawDocType, 'findOneAndUpdate'>;
findOneAndUpdate(
filter?: FilterQuery<RawDocType>,
filter?: FilterQuery<DocType>,
update?: UpdateQuery<RawDocType>,
options?: QueryOptions<DocType> | null
): QueryWithHelpers<DocType | null, DocType, THelpers, RawDocType, 'findOneAndUpdate'>;
Expand Down Expand Up @@ -481,7 +496,7 @@ declare module 'mongoose' {
get(path: string): any;

/** Returns the current query filter (also known as conditions) as a POJO. */
getFilter(): FilterQuery<RawDocType>;
getFilter(): FilterQuery<DocType>;

/** Gets query options. */
getOptions(): QueryOptions<DocType>;
Expand All @@ -490,7 +505,7 @@ declare module 'mongoose' {
getPopulatedPaths(): Array<string>;

/** Returns the current query filter. Equivalent to `getFilter()`. */
getQuery(): FilterQuery<RawDocType>;
getQuery(): FilterQuery<DocType>;

/** Returns the current update operations as a JSON object. */
getUpdate(): UpdateQuery<DocType> | UpdateWithAggregationPipeline | null;
Expand Down Expand Up @@ -560,7 +575,7 @@ declare module 'mongoose' {
maxTimeMS(ms: number): this;

/** Merges another Query or conditions object into this one. */
merge(source: Query<any, any> | FilterQuery<RawDocType>): this;
merge(source: Query<any, any> | FilterQuery<DocType>): this;

/** Specifies a `$mod` condition, filters documents for documents whose `path` property is a number that is equal to `remainder` modulo `divisor`. */
mod<K = string>(path: K, val: number): this;
Expand Down Expand Up @@ -588,10 +603,10 @@ declare module 'mongoose' {
nin(val: Array<any>): this;

/** Specifies arguments for an `$nor` condition. */
nor(array: Array<FilterQuery<RawDocType>>): this;
nor(array: Array<FilterQuery<DocType>>): this;

/** Specifies arguments for an `$or` condition. */
or(array: Array<FilterQuery<RawDocType>>): this;
or(array: Array<FilterQuery<DocType>>): this;

/**
* Make this query throw an error if no documents match the given `filter`.
Expand Down Expand Up @@ -648,7 +663,7 @@ declare module 'mongoose' {
* not accept any [atomic](https://www.mongodb.com/docs/manual/tutorial/model-data-for-atomic-operations/#pattern) operators (`$set`, etc.)
*/
replaceOne(
filter?: FilterQuery<RawDocType>,
filter?: FilterQuery<DocType>,
replacement?: DocType | AnyObject,
options?: QueryOptions<DocType> | null
): QueryWithHelpers<any, DocType, THelpers, RawDocType, 'replaceOne'>;
Expand Down Expand Up @@ -707,7 +722,7 @@ declare module 'mongoose' {
setOptions(options: QueryOptions<DocType>, overwrite?: boolean): this;

/** Sets the query conditions to the provided JSON object. */
setQuery(val: FilterQuery<RawDocType> | null): void;
setQuery(val: FilterQuery<DocType> | null): void;

setUpdate(update: UpdateQuery<RawDocType> | UpdateWithAggregationPipeline): void;

Expand Down Expand Up @@ -747,7 +762,7 @@ declare module 'mongoose' {
* the `multi` option.
*/
updateMany(
filter?: FilterQuery<RawDocType>,
filter?: FilterQuery<DocType>,
update?: UpdateQuery<RawDocType> | UpdateWithAggregationPipeline,
options?: QueryOptions<DocType> | null
): QueryWithHelpers<UpdateWriteOpResult, DocType, THelpers, RawDocType, 'updateMany'>;
Expand All @@ -757,7 +772,7 @@ declare module 'mongoose' {
* `update()`, except it does not support the `multi` or `overwrite` options.
*/
updateOne(
filter?: FilterQuery<RawDocType>,
filter?: FilterQuery<DocType>,
update?: UpdateQuery<RawDocType> | UpdateWithAggregationPipeline,
options?: QueryOptions<DocType> | null
): QueryWithHelpers<UpdateWriteOpResult, DocType, THelpers, RawDocType, 'updateOne'>;
Expand Down