feat: add client.db.count() method for counting collection documents#97
feat: add client.db.count() method for counting collection documents#97Nitin-kumar-yadav1307 wants to merge 7 commits intogeturbackend:mainfrom
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 22 minutes and 55 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThe PR adds a backend early-return for Changes
Sequence DiagramsequenceDiagram
participant Client as Client
participant SDK as DatabaseModule
participant Backend as DataController
participant Engine as QueryEngine
participant DB as MongoDB
Client->>SDK: count('products', {filter:{status:'active'}})
SDK->>SDK: build query string (count=true & filter)
SDK->>Backend: GET /api/data/products?count=true&status=active
Backend->>Backend: detect count=true
Backend->>Engine: QueryEngine(req.query).count()
Engine->>Engine: _buildMongoQuery(excludeCount=true)
Engine->>DB: countDocuments(mongoQuery)
DB-->>Engine: N
Engine-->>Backend: N
Backend-->>SDK: { success:true, data:{count: N } }
SDK-->>Client: Promise<number> (N)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![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)
apps/public-api/src/controllers/data.controller.js (1)
150-153:⚠️ Potential issue | 🟡 MinorError response exposes raw MongoDB errors to client.
The error handler returns
err.messagedirectly, which may leak internal database details. Per coding guidelines, useAppErrorclass and avoid exposing MongoDB errors.🛡️ Proposed fix
} catch (err) { console.error(err); - res.status(500).json({ error: err.message }); + res.status(500).json({ success: false, data: {}, message: 'Failed to retrieve data' }); }As per coding guidelines: "Use AppError class for errors — never raw throw, never expose MongoDB errors to client."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/public-api/src/controllers/data.controller.js` around lines 150 - 153, The catch block currently returns err.message to the client (leaking DB internals); instead log the raw error internally (console.error or your logger) and return or forward a sanitized AppError to the middleware—e.g., replace res.status(500).json({ error: err.message }) with creating/throwing/next-ing a new AppError('Internal server error', 500) (or res.status(500).json({ error: 'Internal server error' })) so the client never receives raw MongoDB messages while the original err remains logged for debugging; update the catch in the controller handler where console.error(err) appears to use AppError and not expose err.message.
🧹 Nitpick comments (1)
packages/common/src/utils/queryEngine.js (1)
75-101: Duplicated filter-building logic — consider extracting a shared helper.The operator translation logic (lines 80-97) duplicates the same code from
filter()(lines 12-29). Extract this into a private method to improve maintainability.Also note: the controller attempts to call
.getFilter()on the query returned bycount(), butcountDocuments()doesn't expose that method — this breaks RLS filter merging (see comment indata.controller.js).♻️ Proposed refactor to extract shared logic
+ _buildMongoQuery(excludeCount = false) { + const queryObj = { ...this.queryString }; + const excludedFields = ['page', 'sort', 'limit', 'fields', 'populate', 'expand']; + if (excludeCount) excludedFields.push('count'); + excludedFields.forEach(el => delete queryObj[el]); + + const mongoQuery = {}; + for (const key in queryObj) { + if (key.endsWith('_gt')) { + mongoQuery[key.replace(/_gt$/, '')] = { ...mongoQuery[key.replace(/_gt$/, '')], $gt: queryObj[key] }; + } else if (key.endsWith('_gte')) { + mongoQuery[key.replace(/_gte$/, '')] = { ...mongoQuery[key.replace(/_gte$/, '')], $gte: queryObj[key] }; + } else if (key.endsWith('_lt')) { + mongoQuery[key.replace(/_lt$/, '')] = { ...mongoQuery[key.replace(/_lt$/, '')], $lt: queryObj[key] }; + } else if (key.endsWith('_lte')) { + mongoQuery[key.replace(/_lte$/, '')] = { ...mongoQuery[key.replace(/_lte$/, '')], $lte: queryObj[key] }; + } else { + mongoQuery[key] = queryObj[key]; + } + } + return mongoQuery; + } + filter() { - const queryObj = { ...this.queryString }; - const excludedFields = ['page', 'sort', 'limit', 'fields', 'populate', 'expand']; - excludedFields.forEach(el => delete queryObj[el]); - - const mongoQuery = {}; - for (const key in queryObj) { - // ... existing logic - } - - this.query = this.query.find(mongoQuery); + this.query = this.query.find(this._buildMongoQuery()); return this; } count() { - const queryObj = { ...this.queryString }; - // ... duplicated logic - this.query = this.query.model.countDocuments(mongoQuery); + this.query = this.query.model.countDocuments(this._buildMongoQuery(true)); return this; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/common/src/utils/queryEngine.js` around lines 75 - 101, Extract the duplicated operator-translation logic used in filter() and count() into a private helper (e.g. buildMongoQueryFromQueryString(queryString)) that returns the mongoQuery object; replace the inlined loops in both filter() and count() to call that helper. Also change count() to build its mongoQuery via the helper and then set this.query to a Mongoose Query that still exposes getFilter (for example use this.query = this.query.find(mongoQuery).countDocuments() instead of this.query.model.countDocuments(mongoQuery)) so the controller can call .getFilter() on the returned query.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/public-api/src/controllers/data.controller.js`:
- Around line 126-134: The count branch currently allows RLS bypass because
countQuery uses Model.countDocuments without merging baseFilter
(countEngine.count().query.getFilter?.() can be undefined); fix by extracting
the filter from countEngine.count().query (using getFilter() if available or
fallback to query.getQuery()/_conditions) into a plain object, merge it with
baseFilter into a single filter, and call Model.countDocuments(mergedFilter)
(assign to count variable after awaiting); then return the standardized response
shape { success: true, data: { count }, message: "" } instead of raw { count } —
update references to QueryEngine, countEngine.count(), countQuery,
Model.countDocuments, and baseFilter accordingly.
---
Outside diff comments:
In `@apps/public-api/src/controllers/data.controller.js`:
- Around line 150-153: The catch block currently returns err.message to the
client (leaking DB internals); instead log the raw error internally
(console.error or your logger) and return or forward a sanitized AppError to the
middleware—e.g., replace res.status(500).json({ error: err.message }) with
creating/throwing/next-ing a new AppError('Internal server error', 500) (or
res.status(500).json({ error: 'Internal server error' })) so the client never
receives raw MongoDB messages while the original err remains logged for
debugging; update the catch in the controller handler where console.error(err)
appears to use AppError and not expose err.message.
---
Nitpick comments:
In `@packages/common/src/utils/queryEngine.js`:
- Around line 75-101: Extract the duplicated operator-translation logic used in
filter() and count() into a private helper (e.g.
buildMongoQueryFromQueryString(queryString)) that returns the mongoQuery object;
replace the inlined loops in both filter() and count() to call that helper. Also
change count() to build its mongoQuery via the helper and then set this.query to
a Mongoose Query that still exposes getFilter (for example use this.query =
this.query.find(mongoQuery).countDocuments() instead of
this.query.model.countDocuments(mongoQuery)) so the controller can call
.getFilter() on the returned query.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 010149c1-1806-4da5-b8a9-b0567bd91e60
📒 Files selected for processing (4)
apps/public-api/src/controllers/data.controller.jspackages/common/src/utils/queryEngine.jssdks/urbackend-sdk/src/modules/database.tssdks/urbackend-sdk/tests/database.test.ts
yash-pouranik
left a comment
yash-pouranik
left a comment
There was a problem hiding this comment.
@Nitin-kumar-yadav1307 please update @urbackend/sdk package version from 0.2.2 -> 0.2.3
|
@Nitin-kumar-yadav1307 |
|
@Nitin-kumar-yadav1307 please resolve merge conflicts - also only accepts change from main. what persists from ur branch? = only the logic u have added ok? also please update version to 0.2.7 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/public-api/src/controllers/data.controller.js`:
- Around line 126-132: Exclude control params from the count path by invoking
QueryEngine.filter with the flag that omits control/query params (i.e., call
QueryEngine(...).filter(true) or otherwise ensure _buildMongoQuery(true) is
used) so `count=true` is not baked into mongoFilter; then make the merge
semantics match the list path by combining mongoFilter and baseFilter with an
$and (or use the same .and([baseFilter]) semantics) instead of a shallow
spread—produce mergedFilter as { $and: [mongoFilter, baseFilter] } when
baseFilter is non-empty before passing it to Model.countDocuments.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7e2542a1-a9c5-4f88-a953-2c945aa0eb8f
📒 Files selected for processing (3)
apps/public-api/src/controllers/data.controller.jspackages/common/src/utils/queryEngine.jssdks/urbackend-sdk/package.json
✅ Files skipped from review due to trivial changes (1)
- sdks/urbackend-sdk/package.json
|
@yash-pouranik have a look |
Nitin-kumar-yadav1307
force-pushed
the
feat/db-count-method
branch
from
a12b136 to
8a0a5b8
Compare
2 participants
What's the problem?
There was no efficient way to get the number of documents in a collection. You had to do something like this:
This is wasteful especially when you have thousands of records.
What I changed
Backend (
data.controller.js+queryEngine.js)Added a check for
?count=truequery param in the controller. When present, instead of fetching all documents, it runscountDocuments()and returns{ count: number }. Also added acount()method toQueryEngineso filters still work properly when counting.SDK (
database.ts)Added a
count()method toDatabaseModule. It passescount=trueto the API and just returns the number back. UsedOmit<QueryParams, 'count'>on the params type so nobody accidentally passes count as a filter field.Tests (
database.test.ts)Added 3 new tests:
Usage
Tests
All 22 tests passing.
Closes #94.
Summary by CodeRabbit
New Features
Tests
Chores