-
-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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: fixed the typing of FilterQuery<T> type to prevent it from only getting typed to any #14398
Changes from 4 commits
6de3a0a
303ab87
4278466
5313d48
307c1cc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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]>; | ||
|
@@ -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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why changing from There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Now, the reason these function were failing because in
Notice how in the above declared BUT, in the above autoTypedTest's To prevent the above type conflict, I chose to replace the |
||
|
||
/** Specifies the batchSize option. */ | ||
batchSize(val: number): this; | ||
|
@@ -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'>; | ||
|
||
|
@@ -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, | ||
|
@@ -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, | ||
|
@@ -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. */ | ||
|
@@ -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'>; | ||
|
@@ -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>; | ||
|
@@ -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; | ||
|
@@ -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; | ||
|
@@ -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`. | ||
|
@@ -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'>; | ||
|
@@ -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; | ||
|
||
|
@@ -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'>; | ||
|
@@ -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'>; | ||
|
There was a problem hiding this comment.
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.There was a problem hiding this comment.
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<>:
Condition<ReturnType<(arg: WithId<T>) => typeof arg.id>>
is just a more type-safe equivalent way of writingCondition<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 tooThere was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
andApplyBasicQueryCasting
, but was not able to resolve it without introducingany
in the types, which makes the entire type cast toany
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 parameterT
. 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 parameterT
also contains a fieldid
or not, and what is its type? For example, ifT
also contains a fieldid: number
, then resultant type formed byWithId<T>
will haveid: never
. Therefore, if we try to assign any value to theid
, it will obviously fail.So, currently, the test is written with the assumption that
id
will not be present inT
. And since Typescript doesn't have that information aboutT
during compile-time, and thus what should be the end type of the fieldid
, hence its throwing incorrect type assignment error. Im not sure what other way to make that existing test pass is other than re-introducingany
, but then that will make this PR uselessThere was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
:The reason is because
Condition<K>
will return something likeK | K[] | ...
. Now, if the return type ofCondition<K>
isK[]
, then it will not have the property['id']
obviously since its an array. So, the above statement was also written with incorrect typescript assumptionThere was a problem hiding this comment.
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?