Skip to content

Conversation

mavriel
Copy link

@mavriel mavriel commented Sep 11, 2025

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:

  • Update operations always target the primary database
  • Validation queries use default database settings, which may route to read replicas
  • In environments with replication lag, validation may fail with "Object not found" errors when the object exists in primary but hasn't propagated to replicas yet

Solution

Modified DatabaseController.update() method to ensure validation queries (validateOnly: true) explicitly use { readPreference: 'primary' }. This guarantees that:

  • Validation reads from the same database that updates will target
  • Eliminates race conditions caused by replication lag
  • Maintains consistency between validation and update operations

Key Changes

  • Added { readPreference: 'primary' } option for validation queries in src/Controllers/DatabaseController.js
  • Only affects internal validation behavior during beforeSave triggers
  • No changes to public APIs or user-facing functionality

Testing

Added comprehensive test coverage in spec/DatabaseController.spec.js:

  • Verifies validateOnly: true queries use primary readPreference
  • Confirms normal updates maintain existing behavior
  • Uses mock storage adapter to validate exact query parameters

Tasks

  • Add tests

Summary by CodeRabbit

  • Bug Fixes

    • Validation-only updates now read from the primary database to avoid stale reads and intermittent "not found" responses. No API or result changes.
  • Tests

    • New tests verify that validation-only operations use primary reads and that full-update flows perform actual update calls (ensuring validation path does not trigger updates).

…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
Copy link

parse-github-assistant bot commented Sep 11, 2025

🚀 Thanks for opening this pull request!

Copy link

coderabbitai bot commented Sep 11, 2025

📝 Walkthrough

Walkthrough

Changes 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

Cohort / File(s) Summary
Controller: validateOnly read preference
src/Controllers/DatabaseController.js
In the validateOnly branch of update, change the adapter find call options from {} to { readPreference: 'primary' }. No other behavior or signatures changed.
Tests: update with validateOnly
spec/DatabaseController.spec.js
Add tests covering validateOnly=true and validateOnly=false paths. Introduce a mockStorageAdapter with spies for find, findOneAndUpdate, watch, and getAllClasses. Assert that find is called with readPreference: 'primary' and findOneAndUpdate is not called when validating; assert the inverse when not validating.

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
Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Pre-merge checks and finishing touches

✅ Passed checks (5 passed)
Check name Status Explanation
Linked Issues Check ✅ Passed The pull request directly addresses all primary objectives from issue #9850. The core requirement—enabling validation queries to use primary readPreference to prevent "Object not found" errors due to replication lag—is met by modifying DatabaseController.update() to include readPreference: 'primary' for validateOnly queries. The implementation solves the multi-region deployment issue by ensuring validation reads from the primary database (matching update targets), comprehensive tests verify the new behavior, and no public APIs are changed. The fix is complete and focused.
Out of Scope Changes Check ✅ Passed All code changes are directly in scope and aligned with the objectives of issue #9850. The modification to DatabaseController.update() adds readPreference: 'primary' for validation queries, directly addressing the stated problem. The test additions in spec/DatabaseController.spec.js verify the new validation behavior and confirm normal updates remain unaffected. No changes introduce unrelated functionality, new features, or modifications to public APIs or unrelated areas of the codebase.
Description Check ✅ Passed The pull request description fully satisfies the template requirements by providing a linked issue (Closes: #9850), a detailed Approach section that clearly explains the problem and solution, and marking the "Add tests" task as complete. The description goes beyond the template by providing comprehensive context about multi-region deployments, the specific race condition with replication lag, and how the fix maintains consistency between validation and update operations. All required information is present and well-structured.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.
Title Check ✅ Passed The pull request title directly and specifically addresses the main change in the changeset. It identifies the core issue (stale data reads in validation queries) and the solution context (ensuring consistency between validation reads and update writes on Parse.Object). The title is specific enough that a developer reviewing history would understand this fixes a data consistency problem where validation queries were reading from replicas while updates targeted the primary. The title is neither vague nor generic, and it clearly maps to the implementation changes visible in DatabaseController.js where readPreference: 'primary' is added to validation queries.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@parseplatformorg
Copy link
Contributor

parseplatformorg commented Sep 11, 2025

Snyk checks have passed. No issues have been found so far.

Status Scanner Critical High Medium Low Total (0)
Open Source Security 0 0 0 0 0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

Copy link

@coderabbitai coderabbitai bot left a 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 load

Only 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 tests

Clear 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 additions

Use 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

📥 Commits

Reviewing files that changed from the base of the PR and between 82fdb0d and 59e599b.

📒 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 false

Good coverage ensuring validation read is bypassed and write path is invoked.

@mavriel mavriel changed the title fix: Prevent stale data reads during update validation with primary r… fix: Prevent stale data reads during update validation with primary readPreference Sep 11, 2025
@mtrezza
Copy link
Member

mtrezza commented Oct 19, 2025

Makes sense; what about possible performance impacts? @coderabbitai?

Copy link

coderabbitai bot commented Oct 19, 2025

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:

  1. Increased primary load: Validation queries now always hit the primary instead of potentially being distributed across read replicas. This could increase load on the primary database, though the impact is limited since it only affects validation queries during update operations with beforeSave triggers.

  2. Latency in multi-region deployments: In setups where readPreference: 'nearest' is configured to serve reads from geographically closer replicas, validation queries will now have potentially higher latency since they must read from the primary (which might be in a different region).

  3. Limited scope: The performance impact is constrained to:

    • Only update operations (not create, find, etc.)
    • Only when beforeSave triggers perform validation
    • Only the validation read itself (not the actual update operation)

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:

  • The performance impact should be minimal for most deployments with low replication lag
  • Deployments without read replicas see no change
  • The consistency guarantee prevents error scenarios that would require retry logic (which also impacts performance)

Overall, this seems like a reasonable trade-off where correctness takes precedence, and the performance impact is scoped to a specific operation pattern.

@mtrezza
Copy link
Member

mtrezza commented Oct 19, 2025

Sounds good, waiting for CI

@mtrezza mtrezza changed the title fix: Prevent stale data reads during update validation with primary readPreference fix: Stale data reads during update validation with primary read preference Oct 19, 2025
@mtrezza
Copy link
Member

mtrezza commented Oct 19, 2025

@coderabbitai suggest an angular commit message.

Copy link

coderabbitai bot commented Oct 19, 2025

Tip

For best results, initiate chat on the files or code changes.

@mtrezza Here's a suggested Angular commit message for this PR:

fix(DatabaseController): use primary readPreference for update validation queries

Validation queries during beforeSave triggers now explicitly read from the 
primary database instead of potentially routing to read replicas. This prevents 
"Object not found" errors in multi-region deployments when replication lag 
exists between the primary and replicas.

Previously, when an object was created or updated and immediately validated 
(e.g., in a subsequent update with beforeSave trigger), the validation query 
could read from a replica that hadn't yet received the replicated data, causing 
validation to fail. By forcing validation queries to use readPreference: 
'primary', we ensure consistency between the validation read and the subsequent 
update write operation, which both now target the primary database.

Closes #9850

This follows the Angular commit convention with:

  • Type: fix (bug fix)
  • Scope: DatabaseController (the affected component)
  • Subject: concise description of the change
  • Body: explains the problem, solution, and reasoning
  • Footer: references the closed issue

Copy link

codecov bot commented Oct 19, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.02%. Comparing base (178bd5c) to head (2308b69).
⚠️ Report is 1 commits behind head on alpha.

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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@mtrezza mtrezza changed the title fix: Stale data reads during update validation with primary read preference fix: Stale data reads in validation query during object update Oct 19, 2025
@mtrezza mtrezza changed the title fix: Stale data reads in validation query during object update fix: Stale data read in validation query during object update causes consistency between validation read and subsequent update write operation Oct 19, 2025
@mtrezza mtrezza changed the title fix: Stale data read in validation query during object update causes consistency between validation read and subsequent update write operation fix: Stale data read in validation query on Parse.Object update causes consistency between validation read and subsequent update write operation Oct 19, 2025
Copy link
Member

@mtrezza mtrezza left a 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Ability to specify custom readPreference for beforeSave's internal validation

3 participants