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

feat: add support for declaring search indexes #921

Merged
merged 1 commit into from Apr 2, 2024

Conversation

noseworthy
Copy link
Contributor

@noseworthy noseworthy commented Mar 22, 2024

MongoDB and mongoose now support adding search indexes to collections. Introduce a new decorator @searchIndex() that, similar to @index, adds a search index to a collection.

Search index support was added to mongoose in version 8.1.0, however the type definition for Schema.searchIndex() didn't exist until 8.2.3. Therefore in order to support this feature, update the mongoose dependencies to ~8.2.3.

Related Issues

  • N/A

@noseworthy
Copy link
Contributor Author

@hasezoey : Sorry, the typings sort of got out of hand here. I can totally remove all that if you'd like. I know it's probably not typegoose's responsibility. The typings in the mongo driver were so lacklustre though and when developing this I had to build up my own mental map of what was going on.

Anyway, It's pretty easy to simplify if you'd like. I can try and open a PR into the mongo driver repo and see if they'll take them on haha.

Also, wasn't sure how to handle the requirement on mongoose 8.2.3. Let me know if I should've done something different there.

Finally, thanks so much for your work on typegoose and mongoose. These libraries are fantastic 😄

Copy link
Member

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

Thanks for the work, there are some small things i have noticed and put some comments up, aside from that there is also:

  • the new documentation page has not been added to the sidebars yet
  • i would prefer if those extra types could be upstreamed to mongoose (or even mongodb?)
  • some places have been missed for the changed mongoose version

i will put up a update on the beta branch, please target that as a base

package.json Outdated Show resolved Hide resolved
docs/guides/all-decorators.md Outdated Show resolved Hide resolved
docs/api/decorators/searchIndexes.md Outdated Show resolved Hide resolved
docs/api/decorators/searchIndexes.md Outdated Show resolved Hide resolved
docs/api/decorators/searchIndexes.md Outdated Show resolved Hide resolved
Copy link

codecov bot commented Mar 23, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.67%. Comparing base (1e4d282) to head (95056d1).
Report is 1 commits behind head on beta.

❗ Current head 95056d1 differs from pull request most recent head eb324f3. Consider uploading reports for the commit eb324f3 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             beta     #921      +/-   ##
==========================================
+ Coverage   98.65%   98.67%   +0.02%     
==========================================
  Files          17       18       +1     
  Lines         967      984      +17     
  Branches      261      252       -9     
==========================================
+ Hits          954      971      +17     
  Misses         13       13              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@hasezoey
Copy link
Member

i will not be able to upgrade mongoose until Automattic/mongoose#14463 is merged and released (the same cause on why the test CI failed here)

@noseworthy
Copy link
Contributor Author

Thanks for the review @hasezoey ! I think I've addressed all the comments. I see that the beta branch is still a ways behind master, so once that updates I'll rebase on top of that.

I've created NODE-6044 in the mongodb jira and an associated PR. We'll see how the mongo team feels about that. It'd be great to have more detailed types other than just bson.Document for the search index definition 😅.

I've opened another PR in mongoose to re-export the SearchIndexDescription type from mongodb so that all three projects can stay in sync. Since I don't currently have access to this type, I've created a copy for the time being, but if that PR gets merged then I'll just remove the copy.

Thanks again for your patience with this and your review. I apologize in advance if there's any more dumb issues or if I've missed something. Still getting my feet under me in this repo 😅!

@hasezoey
Copy link
Member

I think I've addressed all the comments.

from what i can see, yes

I see that the beta branch is still a ways behind master, so once that updates I'll rebase on top of that.

i dont want to push broken test commits, so i have not pushed the mongoose 8.2.3 update there and wait until Automattic/mongoose#14463 is resolved.
for the same reason this PR will likely not get merged for now.

@hasezoey hasezoey added the parity Feature Parity with mongoose | mongodb label Mar 25, 2024
@hasezoey
Copy link
Member

slight update, Automattic/mongoose#14463 has been merged, now just waiting until 8.2.4 is released

@hasezoey
Copy link
Member

hasezoey commented Mar 30, 2024

the mongoose version 8.2.4 has been released and the beta branch has been updated (and also released as v12.3.0-beta.1), please rebase onto that

PS: also dont forgot the set the PR base branch to beta (if not done, i will do it later)

@noseworthy noseworthy changed the base branch from master to beta March 30, 2024 15:14
MongoDB and `mongoose` now support adding search indexes to collections.
Introduce a new decorator `@searchIndex()` that, similar to `@index`,
adds a search index to a collection.

Search index support was added to `mongoose` in version `8.1.0`, however
the type definition for `Schema.searchIndex()` didn't exist until
`8.2.3`, and `mongoose` didn't export the search index definition type
until `8.2.4`. Therefore this change requires `mongoose` `8.2.4`.
@noseworthy
Copy link
Contributor Author

All done, @hasezoey! Thanks again for helping review this change. If there's anything else you'd like done, just let me know.

@hasezoey hasezoey merged commit 5246241 into typegoose:beta Apr 2, 2024
7 checks passed
Copy link

github-actions bot commented Apr 2, 2024

🎉 This PR is included in version 12.3.0-beta.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

Copy link

github-actions bot commented Apr 6, 2024

🎉 This PR is included in version 12.3.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

noseworthy added a commit to noseworthy/typegoose that referenced this pull request Apr 15, 2024
The search index decorator (`@searchIndex()`) was added in `v12.3.0` in
PR typegoose#921 but I neglected to actually export the decorator so it isn't
currently available for use.
noseworthy added a commit to noseworthy/typegoose that referenced this pull request Apr 15, 2024
The search index decorator (`@searchIndex()`) was added in `v12.3.0` in
PR typegoose#921 but I neglected to actually export the decorator so it isn't
currently available for use.
hasezoey pushed a commit that referenced this pull request Apr 15, 2024
The search index decorator (`@searchIndex()`) was added in `v12.3.0` in
PR #921 but I neglected to actually export the decorator so it isn't
currently available for use.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
parity Feature Parity with mongoose | mongodb released on @beta released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants