feat: tighten and unify MongoModifier types#1782
Conversation
Julusian
added
the
Contribution from SuperFly.tv
|
Warning Review limit reached
More reviews will be available in 3 minutes and 18 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
WalkthroughIntroduces a narrowed ChangesupsertAsync → replaceAsync and MongoBulkWriteOperation rollout
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/corelib/src/mongo.ts (1)
596-604: 🎯 Functional Correctness | 🟡 MinorUse deep equality for
$pull$inmatches.includes()only matches object elements by reference, so$pull: { arr: { $in: [...] } }won’t remove equivalent embedded documents; use deep equality here to match MongoDB semantics.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/corelib/src/mongo.ts` around lines 596 - 604, The $pull handling in the mongo matcher currently uses reference equality for the $in branch, so equivalent embedded documents in arrAttr are not removed. Update the $in path in mongo.ts to use deep equality when comparing each entry against matchValue.$in, keeping the logic inside the $pull operator handling aligned with MongoDB semantics. Locate the fix in the code path that checks for $in on matchValue and filters arrAttr.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@meteor/server/api/triggeredActions.ts`:
- Around line 121-123: The restore path in triggeredActions.ts is turning one
bulk write into many concurrent Mongo writes by using Promise.all with
TriggeredActions.replaceAsync on each item. Update the restore flow to use the
new bulk write path with batched replaceOne operations instead of launching
unbounded parallel replacements, and keep the change localized around the
restore upload handling in the triggeredActions restore logic.
---
Outside diff comments:
In `@packages/corelib/src/mongo.ts`:
- Around line 596-604: The $pull handling in the mongo matcher currently uses
reference equality for the $in branch, so equivalent embedded documents in
arrAttr are not removed. Update the $in path in mongo.ts to use deep equality
when comparing each entry against matchValue.$in, keeping the logic inside the
$pull operator handling aligned with MongoDB semantics. Locate the fix in the
code path that checks for $in on matchValue and filters arrAttr.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 42c0205e-30cd-4360-a17b-1821fb3cc914
📒 Files selected for processing (33)
meteor/server/api/blueprints/api.tsmeteor/server/api/integration/expectedPackages.tsmeteor/server/api/integration/media-scanner.tsmeteor/server/api/rest/v1/showstyles.tsmeteor/server/api/rest/v1/studios.tsmeteor/server/api/rundownLayouts.tsmeteor/server/api/translationsBundles.tsmeteor/server/api/triggeredActions.tsmeteor/server/collections/collection.tsmeteor/server/collections/implementations/asyncCollection.tsmeteor/server/collections/implementations/mock.tsmeteor/server/lib/database.tsmeteor/server/migration/lib.tsmeteor/server/migration/upgrades/lib.tspackages/corelib/src/__tests__/mongo.spec.tspackages/corelib/src/mongo.tspackages/job-worker/src/__mocks__/collection.tspackages/job-worker/src/__tests__/rundownPlaylist.test.tspackages/job-worker/src/db/changes.tspackages/job-worker/src/db/collection.tspackages/job-worker/src/db/collections.tspackages/job-worker/src/ingest/commit.tspackages/job-worker/src/ingest/expectedPackages.tspackages/job-worker/src/ingest/model/implementation/DocumentChangeTracker.tspackages/job-worker/src/ingest/model/implementation/SaveIngestModel.tspackages/job-worker/src/ingest/nrcsIngestCache.tspackages/job-worker/src/ingest/sofieIngestCache.tspackages/job-worker/src/notifications/NotificationsModelHelper.tspackages/job-worker/src/playout/expectedPackages.tspackages/job-worker/src/playout/model/implementation/SavePlayoutModel.tspackages/job-worker/src/playout/model/implementation/__tests__/LoadPlayoutModel.spec.tspackages/job-worker/src/playout/timings/timelineTriggerTime.tspackages/job-worker/src/updatePartInstanceRanksAndOrphanedState.ts
This makes it match what the driver and our mock supports, and removes some meteor specifics
Their semantics are confusing and make it hard to ensure that complete documents are inserted
This matches the flow of other operations, and ensures the types match our mock
Julusian
force-pushed
the
fix/mongo-modifier-strict-replace
branch
from
f44d130 to
993cdd5
Compare
1 participant
About the Contributor
This pull request is posted on behalf of Superfly
Type of Contribution
This is a: Code improvement
Current Behavior
Some of the types we use for mongo queries are a bit misleading. The update operation includes an
& Document, meaning it allows almost any json object. Meteor-mongo is ok with this, but standard mongo client is not.This is present in both the update and bulkWrite methods.
Additionally, within meteor we have an
updateAsyncmethod which we often use simply to replace full documents. I find this method quite dangerous as it will happily insert a partial document, resulting in the insert not matching what our types expect.New Behavior
This works to tighten the types. It rebuilds the
MongoModifiertype based on how it is built in the client library. This allows us to leave out both the loose handling, as well as any operators that we have not implemented in our mock.The bulk write operations type has has the same treatment.
Additionally, the upsertAsync and upsertManyAsync methods have been replaced with replaceAsync, matching the subset we already have in the job-worker. Hopefully later we can look into unifying these into a common interface, and tidy up the naming.
All call sites have been updated to use the new types and methods.
Part of the goal here is to stop utilising the meteor-specific behaviour, to start reducing our reliance on it and make our logic more portable.
Testing
Affected areas
Time Frame
Other Information
Status