fix: Nested batch sub-requests cause unclear error#10371
fix: Nested batch sub-requests cause unclear error#10371mtrezza wants to merge 2 commits intoparse-community:alphafrom
Conversation
|
🚀 Thanks for opening this pull request! We appreciate your effort in improving the project. Please let us know once your pull request is ready for review. Tip
Note Please respond to review comments from AI agents just like you would to comments from a human reviewer. Let the reviewer resolve their own comments, unless they have reviewed and accepted your commit, or agreed with your explanation for why the feedback was incorrect. Caution Pull requests must be written using an AI agent with human supervision. Pull requests written entirely by a human will likely be rejected, because of lower code quality, higher review effort and the higher risk of introducing bugs. Please note that AI review comments on this pull request alone do not satisfy this requirement. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughAdded validation to forbid nested batch sub-requests that target the Changes
Sequence Diagram(s)(Skipped — change is a focused validation within the batch handler and does not introduce a multi-component sequential flow requiring visualization.) Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ 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: 1
🧹 Nitpick comments (1)
spec/batch.spec.js (1)
856-911: Add one non-regression test for valid paths ending withbatch.This suite covers reject cases well, but it should also assert that a valid endpoint like
POST /1/classes/batchis accepted, so the nested-batch guard stays scoped to/1/batchonly.✅ Suggested test addition
describe('nested batch requests', () => { + it('does not reject valid non-batch endpoints that end with "batch"', async () => { + const result = await request({ + method: 'POST', + url: 'http://localhost:8378/1/batch', + headers, + body: JSON.stringify({ + requests: [ + { + method: 'POST', + path: '/1/classes/batch', + body: { key: 'value' }, + }, + ], + }), + }); + expect(result.data[0].success.objectId).toBeDefined(); + }); + it('rejects sub-request that targets the batch endpoint', async () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/batch.spec.js` around lines 856 - 911, Add a non-regression test to the existing "nested batch requests" suite that verifies a valid endpoint like POST /1/classes/batch is accepted; update spec/batch.spec.js by adding an it(...) that sends a POST to http://localhost:8378/1/classes/batch with headers and a normal body (e.g., class object or query) and asserts the request resolves (not rejected) and returns a 200/expected success response. Place this test alongside the existing tests in the describe('nested batch requests') block so the nested-batch guard (logic that checks requests to '/1/batch') is confirmed to not block paths that simply end with "batch" such as '/1/classes/batch'.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/batch.js`:
- Around line 81-83: The current guard rejects any POST sub-request whose path
endsWith(batchPath), which incorrectly blocks routes like /1/classes/batch;
change the condition to only reject when the path is exactly the batch route.
Replace the endsWith check with an equality check (e.g., restRequest.path ===
batchPath) in the same block where restRequest and batchPath are used so only
true /batch requests are treated as nested-batch and throw the Parse.Error.
---
Nitpick comments:
In `@spec/batch.spec.js`:
- Around line 856-911: Add a non-regression test to the existing "nested batch
requests" suite that verifies a valid endpoint like POST /1/classes/batch is
accepted; update spec/batch.spec.js by adding an it(...) that sends a POST to
http://localhost:8378/1/classes/batch with headers and a normal body (e.g.,
class object or query) and asserts the request resolves (not rejected) and
returns a 200/expected success response. Place this test alongside the existing
tests in the describe('nested batch requests') block so the nested-batch guard
(logic that checks requests to '/1/batch') is confirmed to not block paths that
simply end with "batch" such as '/1/classes/batch'.
🪄 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: 63054783-7667-4566-afba-bb855f432252
📒 Files selected for processing (2)
spec/batch.spec.jssrc/batch.js
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## alpha #10371 +/- ##
=======================================
Coverage 92.52% 92.52%
=======================================
Files 192 192
Lines 16566 16568 +2
Branches 231 231
=======================================
+ Hits 15327 15329 +2
Misses 1217 1217
Partials 22 22 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
1 participant
Summary by CodeRabbit
Bug Fixes
Tests