Repository navigation
fix: harden populate robustness, clamp paginate limit, fix docs and test coverage - #88
Conversation
📝 WalkthroughWalkthroughThis pull request introduces support for populating referenced fields in data queries. The feature adds Changes
Sequence DiagramsequenceDiagram
participant Client
participant Controller as Data Controller
participant Engine as QueryEngine
participant Mongoose as Mongoose Query
participant DB as Database
Client->>Controller: GET /api/data?populate=author,category
Controller->>Engine: new QueryEngine(queryString)
Controller->>Engine: find() → populate()
Engine->>Engine: Parse populate param (comma-separated)
Engine->>Mongoose: query.populate('author')
Engine->>Mongoose: query.populate('category')
Mongoose->>DB: Execute aggregated query
DB-->>Mongoose: Return docs with expanded refs
Mongoose-->>Engine: Hydrated documents
Engine->>Engine: lean() → chainable result
Engine-->>Controller: Result object
Controller-->>Client: JSON response with populated fields
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
There was a problem hiding this comment.
Pull request overview
This PR adds first-class relational expansion support to the Public API via a ?populate=/?expand= query param, and documents the new query capabilities while updating tests around data reads and mail quota handling.
Changes:
- Extended
QueryEnginewith a.populate()step and wired it intogetAllData. - Updated dynamic schema injection to set Mongoose
refcorrectly forRefandArray of Ref, including tenant-prefixed model names. - Updated docs and Jest tests to reflect the new populate behavior and Redis
eval-based mail quota logic.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/common/src/utils/queryEngine.js | Adds .populate() support for query-driven population. |
| packages/common/src/utils/injectModel.js | Injects tenant-aware ref metadata into generated Mongoose schemas for population. |
| apps/public-api/src/controllers/data.controller.js | Chains population for list reads and adds population handling for single-doc reads. |
| apps/public-api/src/tests/mail.controller.test.js | Updates mocks/assertions for Redis eval quota logic and standardized responses. |
| apps/public-api/src/tests/data.controller.read.test.js | Adjusts controller read tests to accommodate populate chaining and adds new populate-related assertions. |
| docs/database.md | Documents populate/expand query params and adds a population section + examples. |
| docs/api-reference.md | Adds a query-parameter reference table including populate/expand. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| populate() { | ||
| const populateParam = this.queryString.populate || this.queryString.expand; | ||
| if (populateParam) { | ||
| const fields = populateParam.split(',').map(f => f.trim()).filter(Boolean); | ||
| fields.forEach(f => { | ||
| this.query = this.query.populate(f); | ||
| }); |
There was a problem hiding this comment.
populate/expand is currently not excluded from filter(). With the common call order new QueryEngine(...).filter().sort().populate(), filter() will treat populate as a Mongo filter field (e.g. { populate: 'author' }) and likely return no results. Add populate and expand to the excludedFields list in filter() (or strip them before building mongoQuery) so population doesn't affect filtering.
| const populateParam = this.queryString.populate || this.queryString.expand; | ||
| if (populateParam) { | ||
| const fields = populateParam.split(',').map(f => f.trim()).filter(Boolean); | ||
| fields.forEach(f => { |
There was a problem hiding this comment.
populateParam.split(',') assumes req.query.populate is always a string. Express can parse repeated query params into an array (e.g. ?populate=a&populate=b), which would throw here. Consider normalizing populateParam to a string (join arrays) and ignoring non-string values before calling .split().
| const populateParam = req.query.populate || req.query.expand; | ||
| if (populateParam) { | ||
| const fields = populateParam.split(',').map(f => f.trim()).filter(Boolean); | ||
| fields.forEach(f => { | ||
| query = query.populate(f); |
There was a problem hiding this comment.
Same as QueryEngine: populateParam.split(',') assumes a string. If clients send repeated params (or non-string values), this can throw and become a 500. Normalize populateParam (array -> join, non-string -> ignore) before splitting/populating.
| **Endpoint**: `GET /api/data/:collectionName/:id` | ||
|
|
||
| ```bash | ||
| curl "https://api.ub.bitbros.in/api/data/posts/64fd1234abcd5678ef901234" ^ | ||
| -H "x-api-key: pk_live_xxx" | ||
| ``` |
There was a problem hiding this comment.
The "Fetch Single Document" example is missing the opening code fence (bash). Right now the snippet starts with `curl ...` but only has a closing which breaks Markdown rendering.
| ## 2.1 Query Parameters | ||
|
|
||
| You can refine your `GET` requests using the following query parameters: | ||
|
|
There was a problem hiding this comment.
The heading ## 3. Update a Document appears to have been replaced by ## 2.1 Query Parameters, but the subsequent content still transitions directly into the PUT endpoint. Reintroduce an explicit Update section heading (or adjust numbering) so the PUT docs aren't orphaned under the Read section.
| | `populate` | `String` | `?populate=author,category` | Expand `Ref` fields into full objects. | | ||
| | `expand` | `String` | `?expand=author` | Alias for `populate`. | | ||
| | `sort` | `String` | `?sort=createdAt:desc` | Sort results by field and order. | | ||
| | `limit` | `Number` | `?limit=25` | Number of documents to return (max 100). | | ||
| | `page` | `Number` | `?page=2` | Pagination offset. | |
There was a problem hiding this comment.
Docs say limit is "max 100" and page is a "Pagination offset", but QueryEngine.paginate() currently defaults to 100 without clamping and treats page as a 1-indexed page number. Either clamp limit in code or update these descriptions to match actual behavior.
|
@copilot apply changes based on the comments in this thread |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
apps/public-api/src/__tests__/mail.controller.test.js (2)
112-115: Assert full 429 response contract.This check should also verify
data: {}(and ideallylimit) to lock in the standardized{ success, data, message }error shape.Suggested assertion update
expect(res.json).toHaveBeenCalledWith(expect.objectContaining({ success: false, + data: {}, message: 'Monthly mail limit exceeded.', + limit: expect.any(Number), }));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/public-api/src/__tests__/mail.controller.test.js` around lines 112 - 115, Update the test assertion in apps/public-api/src/__tests__/mail.controller.test.js that checks the 429 response to assert the full error contract from res.json: in addition to success: false and message: 'Monthly mail limit exceeded.', assert data: {} and include the limit field (e.g., expect.objectContaining({ limit: expect.any(Number) }) or the exact value) so the test validates the standardized { success, data, message } shape returned by the controller; modify the expect(res.json).toHaveBeenCalledWith(...) assertion accordingly.
76-76: Strengthen quota assertions beyond call count.Line 76 only checks
redis.evalcall count. Please also assert key format, TTL argument, and thatredis.incr/redis.expireare not used, so atomic quota behavior can’t silently regress.Suggested test hardening
- expect(redis.eval).toHaveBeenCalledTimes(1); + expect(redis.eval).toHaveBeenCalledTimes(1); + expect(redis.eval).toHaveBeenCalledWith( + expect.stringContaining('INCR'), + 1, + expect.stringMatching(/^project:mail:count:proj_1:\d{4}-\d{2}$/), + expect.any(Number), + ); + const [, , , ttlSeconds] = redis.eval.mock.calls[0]; + expect(ttlSeconds).toBeGreaterThan(0); + expect(redis.incr).not.toHaveBeenCalled(); + expect(redis.expire).not.toHaveBeenCalled();Based on learnings: Mail count Redis keys must follow pattern
project:mail:count:{projectId}:{YYYY-MM}with TTL set to end of month. Do not change this pattern.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/public-api/src/__tests__/mail.controller.test.js` at line 76, The test currently only asserts redis.eval call count; update the test in mail.controller.test.js to strengthen assertions by verifying the redis.eval was called with the correct key pattern (project:mail:count:{projectId}:{YYYY-MM}) and that the TTL argument corresponds to the end-of-month expiration, and also assert that redis.incr and redis.expire were not called to ensure atomic eval usage; locate references to redis.eval, redis.incr, and redis.expire in the test and add expectations that inspect the eval call arguments (keys and TTL arg) and negative assertions for incr/expire.apps/public-api/src/controllers/data.controller.js (1)
164-171: Consider extracting duplicate parsing logic.The field parsing logic (split by comma, trim, filter) duplicates
QueryEngine.populate(). This is minor since both locations are stable, but a shared helper could reduce future drift.// Potential shared helper in `@urbackend/common` function parsePopulateFields(populateParam) { if (!populateParam) return []; return populateParam.split(',').map(f => f.trim()).filter(Boolean); }🤖 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 164 - 171, Extract the duplicate parsing logic into a shared helper (e.g., parsePopulateFields) and replace the inline parsing in data.controller.js with a call to that helper; specifically, move the split/trim/filter behavior currently applied to populateParam into the new helper (to be consumed by controllers) and update the code that calls QueryEngine.populate() to iterate over the returned array, ensuring both data.controller.js (populateParam) and QueryEngine.populate() use the same helper to avoid drift.packages/common/src/utils/injectModel.js (1)
50-56: Minor: Redundant null check onfield.items.At line 50,
field.itemsis already guaranteed to be truthy by the guard at line 28. The extra check is harmless but unnecessary.// Array of Ref - if (field.items && field.items.type === "Ref") { + if (field.items.type === "Ref") {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/common/src/utils/injectModel.js` around lines 50 - 56, Remove the redundant null check on field.items inside the injectModel logic: change the if condition that currently reads "if (field.items && field.items.type === 'Ref')" to just check "field.items.type === 'Ref'" (or equivalent) because field.items is already guaranteed truthy by the earlier guard; keep the existing behavior that computes targetRef (using field.items.ref) and returns the Schema definition and required flag unchanged.apps/public-api/src/__tests__/data.controller.read.test.js (1)
120-130: Test doesn't verify actual population behavior.This test only asserts
mockQueryEnginewas called, but doesn't verify thatpopulate()was invoked with the expected fields. SincemockQueryEngineis fully mocked and doesn't callmockPopulate, this test doesn't validate theQueryEngine.populate()method behavior.Consider either:
- Testing the real
QueryEnginewith a mocked Mongoose query, or- Asserting that the mock engine's
populate()method was called🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/public-api/src/__tests__/data.controller.read.test.js` around lines 120 - 130, The test for getAllData only checks mockQueryEngine was called but not that populate() was invoked; update the test in the "getAllData passes populate param to QueryEngine" case to assert the mock engine's populate method was called with the expected fields (e.g., that mockPopulate or mockQueryEngine().populate was called with 'author,comments' or ['author','comments'] depending on how your mock is implemented), or alternatively replace the mock and exercise the real QueryEngine with a mocked Mongoose query to verify QueryEngine.populate() is invoked; specifically locate the getAllData test and add an assertion referencing mockPopulate or mockQueryEngine().populate to validate the populate argument.
🤖 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/__tests__/data.controller.read.test.js`:
- Around line 41-47: The mock for the Mongoose query does not support chained
populate calls; update the returned object in the populate mock so that
populate(...) returns the same chainable query object (i.e., it still exposes
populate and lean) instead of an object with only lean. Specifically, modify the
populate implementation that calls mockPopulate(...) so it returns the parent
query object (or an object containing both populate and lean) so multiple calls
to populate(...) are valid; adjust the mock referenced as
mockPopulate/populate/lean in the test to be chainable to mirror Mongoose's API.
In `@docs/database.md`:
- Around line 56-61: The Markdown snippet under the "Fetch Single Document"
section is missing the opening code fence for the curl example; add an opening
fenced code block (e.g., ```bash) immediately before the curl line in that
section so the example is properly fenced and renders correctly, keeping the
existing closing ``` after the curl command.
---
Nitpick comments:
In `@apps/public-api/src/__tests__/data.controller.read.test.js`:
- Around line 120-130: The test for getAllData only checks mockQueryEngine was
called but not that populate() was invoked; update the test in the "getAllData
passes populate param to QueryEngine" case to assert the mock engine's populate
method was called with the expected fields (e.g., that mockPopulate or
mockQueryEngine().populate was called with 'author,comments' or
['author','comments'] depending on how your mock is implemented), or
alternatively replace the mock and exercise the real QueryEngine with a mocked
Mongoose query to verify QueryEngine.populate() is invoked; specifically locate
the getAllData test and add an assertion referencing mockPopulate or
mockQueryEngine().populate to validate the populate argument.
In `@apps/public-api/src/__tests__/mail.controller.test.js`:
- Around line 112-115: Update the test assertion in
apps/public-api/src/__tests__/mail.controller.test.js that checks the 429
response to assert the full error contract from res.json: in addition to
success: false and message: 'Monthly mail limit exceeded.', assert data: {} and
include the limit field (e.g., expect.objectContaining({ limit:
expect.any(Number) }) or the exact value) so the test validates the standardized
{ success, data, message } shape returned by the controller; modify the
expect(res.json).toHaveBeenCalledWith(...) assertion accordingly.
- Line 76: The test currently only asserts redis.eval call count; update the
test in mail.controller.test.js to strengthen assertions by verifying the
redis.eval was called with the correct key pattern
(project:mail:count:{projectId}:{YYYY-MM}) and that the TTL argument corresponds
to the end-of-month expiration, and also assert that redis.incr and redis.expire
were not called to ensure atomic eval usage; locate references to redis.eval,
redis.incr, and redis.expire in the test and add expectations that inspect the
eval call arguments (keys and TTL arg) and negative assertions for incr/expire.
In `@apps/public-api/src/controllers/data.controller.js`:
- Around line 164-171: Extract the duplicate parsing logic into a shared helper
(e.g., parsePopulateFields) and replace the inline parsing in data.controller.js
with a call to that helper; specifically, move the split/trim/filter behavior
currently applied to populateParam into the new helper (to be consumed by
controllers) and update the code that calls QueryEngine.populate() to iterate
over the returned array, ensuring both data.controller.js (populateParam) and
QueryEngine.populate() use the same helper to avoid drift.
In `@packages/common/src/utils/injectModel.js`:
- Around line 50-56: Remove the redundant null check on field.items inside the
injectModel logic: change the if condition that currently reads "if (field.items
&& field.items.type === 'Ref')" to just check "field.items.type === 'Ref'" (or
equivalent) because field.items is already guaranteed truthy by the earlier
guard; keep the existing behavior that computes targetRef (using
field.items.ref) and returns the Schema definition and required flag unchanged.
🪄 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: 9a3f24c1-ff63-4452-a7b0-f981d4a10d92
📒 Files selected for processing (7)
apps/public-api/src/__tests__/data.controller.read.test.jsapps/public-api/src/__tests__/mail.controller.test.jsapps/public-api/src/controllers/data.controller.jsdocs/api-reference.mddocs/database.mdpackages/common/src/utils/injectModel.jspackages/common/src/utils/queryEngine.js
| return { | ||
| populate: (...pArgs) => { | ||
| mockPopulate(...pArgs); | ||
| return { lean: jest.fn().mockResolvedValue({ _id: 'doc_1' }) }; | ||
| }, | ||
| lean: jest.fn().mockResolvedValue({ _id: 'doc_1' }) | ||
| }; |
There was a problem hiding this comment.
Mock doesn't support chaining multiple populate() calls.
The current mock returns a new object from populate() that only has lean(). If getSingleDoc is called with multiple fields (e.g., populate=author,category), only the first field's populate would be captured—subsequent calls would fail because the returned object lacks a populate method.
🔧 Proposed fix for chainable mocks
findOne: (...args) => {
mockFindOne(...args);
- return {
- populate: (...pArgs) => {
- mockPopulate(...pArgs);
- return { lean: jest.fn().mockResolvedValue({ _id: 'doc_1' }) };
- },
- lean: jest.fn().mockResolvedValue({ _id: 'doc_1' })
- };
+ const queryObj = {
+ populate: (...pArgs) => {
+ mockPopulate(...pArgs);
+ return queryObj; // Return self for chaining
+ },
+ lean: jest.fn().mockResolvedValue({ _id: 'doc_1' })
+ };
+ return queryObj;
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return { | |
| populate: (...pArgs) => { | |
| mockPopulate(...pArgs); | |
| return { lean: jest.fn().mockResolvedValue({ _id: 'doc_1' }) }; | |
| }, | |
| lean: jest.fn().mockResolvedValue({ _id: 'doc_1' }) | |
| }; | |
| const queryObj = { | |
| populate: (...pArgs) => { | |
| mockPopulate(...pArgs); | |
| return queryObj; // Return self for chaining | |
| }, | |
| lean: jest.fn().mockResolvedValue({ _id: 'doc_1' }) | |
| }; | |
| return queryObj; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/public-api/src/__tests__/data.controller.read.test.js` around lines 41 -
47, The mock for the Mongoose query does not support chained populate calls;
update the returned object in the populate mock so that populate(...) returns
the same chainable query object (i.e., it still exposes populate and lean)
instead of an object with only lean. Specifically, modify the populate
implementation that calls mockPopulate(...) so it returns the parent query
object (or an object containing both populate and lean) so multiple calls to
populate(...) are valid; adjust the mock referenced as
mockPopulate/populate/lean in the test to be chainable to mirror Mongoose's API.
| ### Fetch Single Document | ||
| **Endpoint**: `GET /api/data/:collectionName/:id` | ||
|
|
||
| ```bash | ||
| curl "https://api.ub.bitbros.in/api/data/posts/64fd1234abcd5678ef901234" ^ | ||
| -H "x-api-key: pk_live_xxx" | ||
| ``` |
There was a problem hiding this comment.
Markdown syntax error: missing opening code fence.
The "Fetch Single Document" curl example is missing its opening code fence (```bash). This will cause rendering issues.
📝 Proposed fix
### Fetch Single Document
**Endpoint**: `GET /api/data/:collectionName/:id`
+```bash
curl "https://api.ub.bitbros.in/api/data/posts/64fd1234abcd5678ef901234" ^
-H "x-api-key: pk_live_xxx"</details>
<details>
<summary>🧰 Tools</summary>
<details>
<summary>🪛 Betterleaks (1.1.1)</summary>
[high] 59-60: Discovered a potential authorization token provided in a curl command header, which could compromise the curl accessed resource.
(curl-auth-header)
</details>
</details>
<details>
<summary>🤖 Prompt for AI Agents</summary>
Verify each finding against the current code and only fix it if needed.
In @docs/database.md around lines 56 - 61, The Markdown snippet under the "Fetch
Single Document" section is missing the opening code fence for the curl example;
add an opening fenced code block (e.g., bash) immediately before the curl line in that section so the example is properly fenced and renders correctly, keeping the existing closing after the curl command.
</details>
<!-- fingerprinting:phantom:poseidon:ocelot:b57d0361-900b-4d7b-bfaa-011f154f9a75 -->
<!-- This is an auto-generated comment by CodeRabbit -->
Agent-Logs-Url: https://github.com/geturbackend/urBackend/sessions/2bf89d6d-c2ca-425b-ae5d-71ce054d8edb Co-authored-by: yash-pouranik <172860064+yash-pouranik@users.noreply.github.com>
Render
Destroyed
Applied all changes from the review thread in commit
All 83 public-api tests pass. |
Review feedback on the relational population feature exposed four categories of bugs:
populate/expandleaking into MongoDB filters,split(',')crashing on Express array params, docs mismatching actualpaginate()behavior, and a test that only checked construction rather than chain invocation.queryEngine.jspopulateandexpandtoexcludedFieldsinfilter()— without this,?populate=authorwould emitModel.find({ populate: 'author' })and return nothingpopulateParamto coerce Express array params (?populate=a&populate=b→['a','b']) before.split(',')limittoMath.min(..., 100)inpaginate()to match documented maxdata.controller.jsgetSingleDoc()for consistencydocs/database.md```bashfence on "Fetch Single Document" example## 3. Update a Documentheading (PUT docs were orphaned under Query Parameters)pagedescription from "Pagination offset" → "1-indexed page number";limitnow shows "default: 100, max: 100"data.controller.read.test.jsmockEnginePopulatespy intomockQueryEnginefactory; tests now assert.populate()is called on the engine chainpopulate/expandare not forwarded to MongoDB filter['author', 'category']) is normalized without crashingfindOnemock to return a chainable object supporting multiple sequential.populate()calls