feat: add advanced query operators and robust error handling#115
Conversation
📝 WalkthroughWalkthroughExtended filtering with new suffix operators ( Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Controller as API Controller\n(getAllData)
participant QE as QueryEngine
participant DB as MongoDB
Client->>Controller: GET /data?name_regex=[
Controller->>QE: instantiate + engine.filter()
QE->>QE: parse filters\nvalidate _regex (length/format)
alt regex invalid
QE-->>Controller: throw QueryFilterError\n(statusCode=400)
Controller->>Controller: catch -> detect 400/error name
Controller-->>Client: HTTP 400 { success:false, data:{}, message }
else regex valid
QE->>DB: query.find(...){ maxTimeMS? }
DB-->>QE: results
QE-->>Controller: filtered results
Controller->>DB: if count path -> countQuery (apply maxTimeMS if regex)
DB-->>Controller: count
Controller-->>Client: HTTP 200 { success:true, data, meta }
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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: 4
🧹 Nitpick comments (3)
packages/common/src/utils/__tests__/queryEngine.test.js (1)
103-108: Consider asserting the error type, not just the message.The controller branches on
err.name === 'QueryFilterError'/err.statusCode === 400, but the test only checks the message substring. Adding an assertion againstQueryEngine.QueryFilterError/statusCodewould lock in the contract the controller actually depends on.- expect(() => engine.filter()).toThrow('Invalid regex pattern'); + expect(() => engine.filter()).toThrow(QueryEngine.QueryFilterError); + try { engine.filter(); } catch (e) { expect(e.statusCode).toBe(400); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/common/src/utils/__tests__/queryEngine.test.js` around lines 103 - 108, The test currently only asserts the error message for invalid regex in the QueryEngine.filter call; update the test to also assert the error type and/or status code the controller relies on by expecting the thrown error to be an instance of QueryEngine.QueryFilterError and to have statusCode === 400 (or assert err.name === 'QueryFilterError' and err.statusCode === 400) when calling new QueryEngine(mockQuery, queryString).filter(); this ensures the test locks in the contract used by the controller.apps/public-api/src/__tests__/data.controller.read.test.js (1)
148-169: Good coverage — consider also coveringname === 'QueryFilterError'.The test exercises the
statusCode === 400branch. Since the controller also acceptserr.name === 'QueryFilterError'as an OR, a second case (error withoutstatusCodebut withname = 'QueryFilterError') would protect that branch from regression.🤖 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 148 - 169, Add a new test variant to exercise the branch where the controller treats errors with err.name === 'QueryFilterError' as a 400: in the same test suite create a test similar to "getAllData returns 400 when QueryEngine throws query validation error" that uses mockQueryEngine.mockImplementationOnce to return an object whose filter() throws an Error instance with name set to 'QueryFilterError' (and without statusCode), call getAllData(req, res) with the same invalid query (e.g., name_regex: '['), then assert res.status was called with 400 and res.json was called with the expected error message; reference the existing test name, mockQueryEngine, and getAllData to place the new case.packages/common/src/utils/queryEngine.js (1)
45-52:_invalues are always strings.
String(rawValue).split(',')forces all elements to strings, so numeric/ObjectId/boolean fields require Mongoose schema casting to match. In thegetAllDatapath,Model.find()does cast based on schema so this typically works, but schemaless/loose collections or fields not typed in the compiled model will silently return no matches. Worth a brief doc note or a cast hint; not blocking.🤖 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 45 - 52, The `_in` branch always coerces values to strings via String(rawValue).split(',') which prevents numeric/boolean/ObjectId values from matching schemaless or loosely-typed collections; update the logic in the key.endsWith('_in') block (operating on queryObj and writing to mongoQuery) to attempt type-preserving parsing for each token before adding the $in array—e.g. if a token looks like a number parseNumber, if "true"/"false" parseBoolean, if a 24-hex pattern treat as an ObjectId candidate, or generally try JSON.parse and fall back to the original string—so values retain their native types when passed to Model.find() in getAllData. Ensure you still support both comma-separated strings and repeated-array params.
🤖 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 164-170: The catch block currently returns raw { error:
err.message } and may leak Mongo internals; update it to return the project's
response envelope and avoid exposing internal messages: for known client errors
(err.statusCode === 400 or err.name === 'QueryFilterError') respond with
res.status(400).json({ success: false, data: null, message: err.message }) and
for all other errors log the full err server-side then respond with
res.status(500).json({ success: false, data: null, message: 'Internal server
error' }); also ensure any thrown/propagated errors use the AppError class
instead of raw throws so other callers expect AppError semantics (refer to this
controller's catch block and the aggregateData handler for examples).
In `@mintlify/docs/guides/database.mdx`:
- Line 103: Update the documentation heading "Advanced Filtering" to sentence
case by changing the heading text to "Advanced filtering" in the
mintlify/docs/guides/database.mdx file; locate the heading string "Advanced
Filtering" and replace it with "Advanced filtering" so it follows the project's
sentence-case style guide for headings.
In `@packages/common/src/utils/queryEngine.js`:
- Around line 53-56: The _exists handler in queryEngine.js silently treats any
non-'true' string as false; update the block that handles keys ending with
'_exists' (the code using key.replace(/_exists$/, ''), value = queryObj[key] ===
'true' and assigning mongoQuery[field].$exists) to explicitly accept only the
literal strings 'true' or 'false', convert them to booleans when valid, and
throw a QueryFilterError for any other value (mirroring the _regex validation
behavior) so invalid inputs like "1" or "yes" produce a clear error instead of
silently flipping semantics.
---
Nitpick comments:
In `@apps/public-api/src/__tests__/data.controller.read.test.js`:
- Around line 148-169: Add a new test variant to exercise the branch where the
controller treats errors with err.name === 'QueryFilterError' as a 400: in the
same test suite create a test similar to "getAllData returns 400 when
QueryEngine throws query validation error" that uses
mockQueryEngine.mockImplementationOnce to return an object whose filter() throws
an Error instance with name set to 'QueryFilterError' (and without statusCode),
call getAllData(req, res) with the same invalid query (e.g., name_regex: '['),
then assert res.status was called with 400 and res.json was called with the
expected error message; reference the existing test name, mockQueryEngine, and
getAllData to place the new case.
In `@packages/common/src/utils/__tests__/queryEngine.test.js`:
- Around line 103-108: The test currently only asserts the error message for
invalid regex in the QueryEngine.filter call; update the test to also assert the
error type and/or status code the controller relies on by expecting the thrown
error to be an instance of QueryEngine.QueryFilterError and to have statusCode
=== 400 (or assert err.name === 'QueryFilterError' and err.statusCode === 400)
when calling new QueryEngine(mockQuery, queryString).filter(); this ensures the
test locks in the contract used by the controller.
In `@packages/common/src/utils/queryEngine.js`:
- Around line 45-52: The `_in` branch always coerces values to strings via
String(rawValue).split(',') which prevents numeric/boolean/ObjectId values from
matching schemaless or loosely-typed collections; update the logic in the
key.endsWith('_in') block (operating on queryObj and writing to mongoQuery) to
attempt type-preserving parsing for each token before adding the $in array—e.g.
if a token looks like a number parseNumber, if "true"/"false" parseBoolean, if a
24-hex pattern treat as an ObjectId candidate, or generally try JSON.parse and
fall back to the original string—so values retain their native types when passed
to Model.find() in getAllData. Ensure you still support both comma-separated
strings and repeated-array params.
🪄 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: 0886c2f7-c9a8-4605-bc72-675d9895288d
📒 Files selected for processing (6)
apps/public-api/src/__tests__/data.controller.read.test.jsapps/public-api/src/controllers/data.controller.jsdocs-legacy/api-reference.mdmintlify/docs/guides/database.mdxpackages/common/src/utils/__tests__/queryEngine.test.jspackages/common/src/utils/queryEngine.js
|
@yash-pouranik, These are great catches, especially on the defensive logic for _in and _regex. I’m working on implementing the error handling refinements, type-aware parsing, and the requested test/docs updates now. I'll push the changes shortly |
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 `@mintlify/docs/guides/database.mdx`:
- Around line 117-124: Update the docs to make the examples copy-paste safe by
URL-encoding regex metacharacters in the `_regex`/`$regex` examples (e.g.,
encode ^, $, etc.) and update the filtering description to state that `price_gt`
and `price_lt` are exclusive bounds (strictly greater than / strictly less than)
rather than “between”; adjust the example curl query for the posts endpoint to
use an encoded regex and keep the `status=published`, `price_gt`, `price_lt`,
and `sort=-createdAt` parameters as-is.
🪄 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: f07af80c-3a80-4825-ae08-de77ad53be1e
📒 Files selected for processing (6)
apps/public-api/src/__tests__/data.controller.read.test.jsapps/public-api/src/controllers/data.controller.jsdocs-legacy/api-reference.mdmintlify/docs/guides/database.mdxpackages/common/src/utils/__tests__/queryEngine.test.jspackages/common/src/utils/queryEngine.js
✅ Files skipped from review due to trivial changes (1)
- docs-legacy/api-reference.md
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/public-api/src/tests/data.controller.read.test.js
- packages/common/src/utils/tests/queryEngine.test.js
- packages/common/src/utils/queryEngine.js
|
Updated |
|
@yash-pouranik, could you check & verify the changes? Also, since I'm a first-time contributor, could you please guide me if I'm making any mistakes? |
|
Yes just reviewing |
|
What score to give this PR?? I suggest lvl 2 as how nicely he cooprrated with ur reviews and did changes in multiple files and synced all the changes in doc as well What say @coderabbitai |
|
Tip For best results, initiate chat on the files or code changes.
Here's a quick breakdown of what was delivered:
My take: Level 2 (5pt) is the right call, and I'd agree with your suggestion. Here's my reasoning:
So Level 2 / 5pt seems like a fair and well-reasoned reward. 👍 |
yash-pouranik
added
enhancement
|
Once again thanks @MdTowfikomer |
|
Thanks @yash-pouranik, I really appreciate it! I’ve starred the repo and will check out the site as well. |
2 participants
🚀 Pull Request Description
Fixes #99
🛠️ Type of Change
return a client-safe
400response.🧪 Testing & Validation
Backend Verification:
packages/common/src/utils/__tests__/queryEngine.test.js.apps/public-api/src/__tests__/data.controller.read.test.jspatterns for _regex do not crash the server (preventing 500 errors).
📝 Description of Changes
Expanded filtering support in
packages/common/src/utils/queryEngine.jsforGET /api/data/:collectionName:Key Technical Enhancements:
support cases where Express parses repeated parameters as an array, preventing .split()
crashes.
("true"/"false").
gracefully handle invalid regex syntax from user input, ensuring the server remains stable.
future reuse.
tables and examples for the new operators.
✅ Checklist
Built with ❤️ for urBackend.
Summary by CodeRabbit
Bug Fixes
New Features
Documentation
Tests