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

Refine query selector to highlight unexpected query keys #14764

Merged
merged 7 commits into from
Aug 12, 2024

Conversation

alex-statsig
Copy link
Contributor

@alex-statsig alex-statsig commented Jul 26, 2024

Summary

Previously, there was a catch-all case in RootQuerySelector which would accept any key with type "any". This still allows nice IDE typehints, but can lead to bugs where the incorrect key is used for a filter with no warning that the key is clearly wrong.

With this PR, only keys that contain a "." are unsafely typed as any, which should reduce the blast radius of the hacky types dramatically.

Examples
Previously, the following would be a valid type:

const testDoc: FilterQuery<{ testKey: string }> = {
  invalidKey: 'test',
};

Now, the above case gives a type error for "invalidKey".

Note that a case such as this will still be accepted (since its hard to properly type any nested queries):

const testDoc: FilterQuery<{ testKey: string }> = {
  "invalidKey.test": 'test',
};

Previously, there was a catch-all case in RootQuerySelector which would accept any key with type "any". This still allows nice IDE typehints, but can lead to bugs where the incorrect key is used for a filter with no warning that the key is clearly wrong.

With this PR, only keys that contain a "." are unsafely typed as any, which should reduce the blast radius of the hacky types dramatically.
@alex-statsig
Copy link
Contributor Author

@hasezoey @IslandRhythms please let me know if there's anything I can do to help get this merged, or if there's any blocking concerns (can't tag reviewers)

@hasezoey hasezoey added the typescript Types or Types-test related issue / Pull Request label Aug 3, 2024
Copy link
Collaborator

@hasezoey hasezoey left a comment

Choose a reason for hiding this comment

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

i am partial to this, but i think the new types only allow a single level of nesting.

in any case, to get this merged (not considering if this is even wanted or not), you should probably add some ts tests for this new addition (and fix existing ones). some tests to consider:

  • that there is a error on a unknown key
  • 1 level nesting should give any
  • more than 1 level nesting?
  • overwrite to allow the old behavior in addition to known properties?

@alex-statsig
Copy link
Contributor Author

@hasezoey Thanks for the feedback!

I've fixed the existing tests by adding a RootFilterQuery which supports Query and ObjectId, which previously didn't have explicit type support (relied on the [key: string]: any types afaict). I considered just adding those to FilterQuery to avoid changing as much, but I believe its technically not valid to use this in all places FilterQuery was used, such as within a $and operator.

I've also added some new test cases covering those you mentioned as well as some others I thought would be relevant.

Addressing some comments:

  • i think the new types only allow a single level of nesting.

It actually allows anything containing at least one "." to retain the previous behavior, so multiple levels of nesting should work fine still

  • overwrite to allow the old behavior in addition to known properties?

I'm not sure how I could go about supporting this, and that's my main concern with this change that I'm open to feedback on. Only approach I could see is a separate version of functions with the other types, which feels far too messy. I'm not if this change would need to be delayed until a breaking major version; it seems possible it would cause type errors for many users, but also likely that it would catch many bugs in doing so. I could fallback to implementing this as a patch in my repo's types, but was hoping it could help others more broadly.

test/types/models.test.ts Outdated Show resolved Hide resolved
test/types/models.test.ts Outdated Show resolved Hide resolved
@hasezoey hasezoey requested a review from vkarpov15 August 6, 2024 15:54
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.

LGTM. I think this can be a reasonable compromise between Mongoose's flexible queries and strict type checking. In the interest of caution, I'll merge this into 8.6 branch though. We may end up having to revert this if it causes unforeseen consequences.

@vkarpov15 vkarpov15 added this to the 8.6 milestone Aug 10, 2024
@vkarpov15 vkarpov15 changed the base branch from master to 8.6 August 10, 2024 22:28
@vkarpov15
Copy link
Collaborator

@alex-statsig it looks like there's some test failures against 8.6 branch, the crux of the issue is likely 8.6 changing the RawDocType default in class Query<ResultType, DocType, THelpers = {}, RawDocType = DocType, QueryOp = 'find', TInstanceMethods = Record<string, never>> implements SessionOperation { to RawDocType = unknown.

I think the fix would be to just update queryhelpers.test.ts to correctly type type ProjectModelQuery = Query<any, HydratedDocument<Project>, ProjectQueryHelpers> -> type ProjectModelQuery = Query<any, HydratedDocument<Project>, ProjectQueryHelpers, any>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
typescript Types or Types-test related issue / Pull Request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants