-
-
Notifications
You must be signed in to change notification settings - Fork 4.8k
fix: Stale data read in validation query on Parse.Object
update causes consistency between validation read and subsequent update write operation
#9859
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: alpha
Are you sure you want to change the base?
Conversation
…eadPreference - Ensures update validation always reads from primary replica in DB - Fixes potential data consistency issues in distributed database environments - Adds comprehensive tests for validateOnly behavior with primary readPreference
🚀 Thanks for opening this pull request! |
📝 WalkthroughWalkthroughChanges make DatabaseController.update use a primary read for its internal validation path when validateOnly is true, and add tests that assert primary-read is used for validation and that the normal update path is used when validateOnly is false. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant DBController as DatabaseController
participant Adapter as StorageAdapter
rect rgb(245,250,255)
note over Client,DBController: validateOnly = true (validation)
Client->>DBController: update(request, validateOnly=true)
DBController->>Adapter: find(query, options={ readPreference: 'primary' })
alt not found
DBController-->>Client: error OBJECT_NOT_FOUND
else found
DBController-->>Client: {}
end
end
rect rgb(250,245,255)
note over Client,DBController: validateOnly = false (update)
Client->>DBController: update(request, validateOnly=false)
DBController->>Adapter: findOneAndUpdate(query, update, options)
Adapter-->>DBController: result
DBController-->>Client: result/ack
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ 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 |
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/Controllers/DatabaseController.js (1)
596-602
: Force-primary read is correct; add limit and projection to cut loadOnly existence is needed here. Pass limit: 1 and a minimal projection to reduce I/O without altering semantics.
- if (validateOnly) { - return this.adapter.find(className, schema, query, { readPreference: 'primary' }).then(result => { + if (validateOnly) { + // Read from primary to avoid replica lag during beforeSave validation (see #9850). + return this.adapter.find( + className, + schema, + query, + { readPreference: 'primary', limit: 1, keys: ['objectId'] } + ).then(result => {spec/DatabaseController.spec.js (2)
619-633
: Prevent schema cache bleed between testsClear the schema cache in this suite to avoid flaky behavior from prior tests.
describe('update with validateOnly', () => { + beforeEach(() => { + Config.get(Parse.applicationId).schemaCache.clear(); + });
655-657
: Make assertion resilient to future option additionsUse partial match so the test won’t break if more options (e.g., limit, keys) are passed.
- expect(findCall.args[3]).toEqual({ readPreference: 'primary' }); // options parameter + expect(findCall.args[3]).toEqual(jasmine.objectContaining({ readPreference: 'primary' })); // options parameter
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
spec/DatabaseController.spec.js
(1 hunks)src/Controllers/DatabaseController.js
(1 hunks)
🔇 Additional comments (1)
spec/DatabaseController.spec.js (1)
663-687
: LGTM: correctly asserts update path when validateOnly is falseGood coverage ensuring validation read is bypassed and write path is invoked.
Makes sense; what about possible performance impacts? @coderabbitai? |
Tip For best results, initiate chat on the files or code changes. @mtrezza Good question! Here's an analysis of the potential performance impacts: Potential performance trade-offs:
The trade-off: This change prioritizes correctness over performance in a specific scenario. Without this fix, the system could fail with "Object not found" errors in the presence of replication lag, which is arguably worse than a slight performance degradation. The validation query needs to see the same data state as the update operation (which always writes to primary), so reading from primary ensures consistency. Mitigation considerations:
Overall, this seems like a reasonable trade-off where correctness takes precedence, and the performance impact is scoped to a specific operation pattern. |
Sounds good, waiting for CI |
@coderabbitai suggest an angular commit message. |
Tip For best results, initiate chat on the files or code changes. @mtrezza Here's a suggested Angular commit message for this PR:
This follows the Angular commit convention with:
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## alpha #9859 +/- ##
==========================================
- Coverage 93.02% 93.02% -0.01%
==========================================
Files 187 187
Lines 15158 15158
Branches 176 176
==========================================
- Hits 14101 14100 -1
- Misses 1045 1046 +1
Partials 12 12 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Parse.Object
update causes consistency between validation read and subsequent update write operation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, just the lint issue.
Pull Request
Issue
Closes: #9850
Approach
This fix addresses a data consistency issue in distributed database environments where
beforeSave
trigger validation queries may read from stale read replicas while update operations target the primary database.Problem
During
beforeSave
trigger execution, Parse Server performs permission validation by querying the database before applying updates. However:Solution
Modified
DatabaseController.update()
method to ensure validation queries (validateOnly: true
) explicitly use{ readPreference: 'primary' }
. This guarantees that:Key Changes
{ readPreference: 'primary' }
option for validation queries insrc/Controllers/DatabaseController.js
beforeSave
triggersTesting
Added comprehensive test coverage in
spec/DatabaseController.spec.js
:validateOnly: true
queries use primary readPreferenceTasks
Summary by CodeRabbit
Bug Fixes
Tests