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: 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

Conversation

FaizBShah
Copy link
Contributor

Fixes #14397

Summary
Fixed the typing to of FilterQuery<T> by preventing it from getting type-casted to any always. Strengthened the type by removing any and still keeping the ability to add generic fields. Fixed the typescript issue of $regex, $eq not showing the hint messages.

Examples
Screenshot 2024-03-01 233516

@FaizBShah
Copy link
Contributor Author

Hi @AbdelrahmanHafez , can you review this PR whenever you feel free?

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.

In general I'm supportive of better autocomplete for FilterQuery, but we need to make sure that we still support arbitrary keys in FilterQuery and make sure tests still pass.

@@ -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

@@ -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

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

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?

types/query.d.ts Show resolved Hide resolved
vkarpov15 added a commit that referenced this pull request Mar 14, 2024
vkarpov15 added a commit that referenced this pull request Mar 18, 2024
types: improve the typing of FilterQuery<T> type to prevent it from only getting typed to any
@vkarpov15
Copy link
Collaborator

Closing in favor of #14436. Thanks @FaizBShah 👍

@vkarpov15 vkarpov15 closed this Mar 18, 2024
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.

Filter query Condition type might as well be any
3 participants