feat: Add installation deviceToken deduplication options#10451
Merged
mtrezza merged 11 commits intoparse-community:alphafrom Apr 30, 2026
Merged
feat: Add installation deviceToken deduplication options#10451mtrezza merged 11 commits intoparse-community:alphafrom
mtrezza merged 11 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. Our CI and AI review are safeguards, not development tools. If many issues are flagged, rethink your development approach. Invest more effort in planning and design rather than using review cycles to fix low-quality code. |
📝 WalkthroughWalkthroughAdds configurable _Installation deduplication: new Changes
Sequence Diagram(s)sequenceDiagram
participant RestWrite as RestWrite
participant InstallationDedup as InstallationDedup
participant Database as Database
participant Logger as Logger
rect rgba(100,150,255,0.5)
Note over RestWrite,InstallationDedup: Remove conflicting deviceToken flow
RestWrite->>InstallationDedup: removeConflictingDeviceToken(query, action, enforceAuth, runOptions, validSchemaController)
InstallationDedup->>Database: destroy(query, options) or update(query, clearField, options)
alt Success
Database-->>InstallationDedup: OK
InstallationDedup->>Logger: Log deleted/updated rows
InstallationDedup-->>RestWrite: Resolve
else OBJECT_NOT_FOUND
Database-->>InstallationDedup: Error OBJECT_NOT_FOUND
InstallationDedup->>Logger: Verbose no-op
InstallationDedup-->>RestWrite: Resolve silently
else OPERATION_FORBIDDEN (enforceAuth)
Database-->>InstallationDedup: Error OPERATION_FORBIDDEN
InstallationDedup->>Logger: Warn permission skip
InstallationDedup-->>RestWrite: Resolve silently
else Unexpected Error
Database-->>InstallationDedup: Error
InstallationDedup->>Logger: Error and rethrow
InstallationDedup-->>RestWrite: Reject
end
end
rect rgba(150,200,150,0.5)
Note over RestWrite,InstallationDedup: Merge duplicate installation flow
RestWrite->>InstallationDedup: applyDuplicateDeviceTokenMerge(idMatch, deviceTokenMatch, mergePriority, action, enforceAuth, runOptions, validSchemaController)
InstallationDedup->>InstallationDedup: Select survivor per mergePriority
alt Winner is idMatch
InstallationDedup->>Database: operate on deviceTokenMatch (destroy/update)
else Winner is deviceTokenMatch
InstallationDedup->>Database: operate on idMatch (destroy/update)
end
alt Success
Database-->>InstallationDedup: OK
InstallationDedup->>Logger: Log merge result
InstallationDedup-->>RestWrite: Return survivor objectId
else OPERATION_FORBIDDEN (enforceAuth)
Database-->>InstallationDedup: Error OPERATION_FORBIDDEN
InstallationDedup->>Logger: Warn and return survivor objectId
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (5 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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.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: 2
🤖 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/InstallationDedup.js`:
- Around line 116-163: The code in applyDuplicateDeviceTokenMerge can pick a
loser that is the same row as the survivor when idMatch.objectId ===
deviceTokenMatch.objectId; add an early self-merge guard at the top of
applyDuplicateDeviceTokenMerge that detects when idMatch.objectId ===
deviceTokenMatch.objectId and immediately returns that objectId (and optionally
log a no-op) so no performAction/delete is attempted on the survivor; keep the
rest of the mergePriority logic unchanged for non-self-merge cases.
In `@src/Options/docs.js`:
- Around line 152-157: The docs still show duplicateDeviceTokenActionEnforceAuth
as normal; add a deprecation notice to the option's help text where the options
are defined (the duplicateDeviceTokenActionEnforceAuth entry in the Options
help/definitions source) and then regenerate derived docs and types by running
the definitions generation step so InstallationOptions in the generated docs and
Definitions reflect the deprecation; ensure the help text explicitly mentions
"deprecated" and any replacement guidance so the generated src/Options/docs.js
InstallationOptions entry is updated.
🪄 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: 0f3f1cd3-570b-4135-83a2-cd889539529a
📒 Files selected for processing (11)
resources/buildConfigDefinitions.jsspec/Deprecator.spec.jsspec/InstallationDedup.spec.jsspec/ParseInstallation.spec.jssrc/Config.jssrc/Deprecator/Deprecations.jssrc/InstallationDedup.jssrc/Options/Definitions.jssrc/Options/docs.jssrc/Options/index.jssrc/RestWrite.js
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## alpha #10451 +/- ##
==========================================
+ Coverage 92.51% 92.59% +0.07%
==========================================
Files 193 194 +1
Lines 16831 16901 +70
Branches 234 234
==========================================
+ Hits 15572 15650 +78
+ Misses 1236 1228 -8
Partials 23 23 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
spec/InstallationDedup.spec.js (1)
83-104: ⚡ Quick winTighten the update-path assertions.
These expectations currently mirror the helper’s buggy positional call shape, so they would not catch the
database.update()argument-order regression above. Please assert the full signature in both update cases so a shiftedmany/ schema-controller argument fails the suite.Example
- expect(database.update).toHaveBeenCalledWith( - '_Installation', - { deviceToken: 'X' }, - { deviceToken: { __op: 'Delete' } }, - {}, - true, - undefined - ); + expect(database.update).toHaveBeenCalledWith( + '_Installation', + { deviceToken: 'X' }, + { deviceToken: { __op: 'Delete' } }, + { many: true }, + undefined, + undefined, + undefined + );Also applies to: 179-202, 227-249
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/InstallationDedup.spec.js` around lines 83 - 104, The test is too lax about the positional arguments passed to database.update in InstallationDedup.removeConflictingDeviceToken so a slipped/misordered signature (many vs schema controller) won't be caught; update the assertions to check the full call signature for the update-paths by asserting database.update was called with all arguments in the exact expected order (for example: '_Installation', { deviceToken: 'X' }, { deviceToken: { __op: 'Delete' } }, {}, true, undefined) and do the same tightened full-signature assertion for the other update case(s) referenced (lines around 179-202 and 227-249), while keeping the logSpy.verbose assertion.
🤖 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/InstallationDedup.js`:
- Around line 39-46: The call to database.update in InstallationDedup.js is
passing arguments in the wrong order (you passed many as the 5th positional arg
which shifts the expected options/flags); change the call to pass the options
object (runOptions) as the 4th argument, then the skipSanitization and
validateOnly booleans (use false or the appropriate variables), and finally
validSchemaController as the last argument—for example, remove the positional
many argument (or move it into runOptions if it is an option like
runOptions.many) and call database.update(CLASS_NAME, query, { [fieldToClear]: {
__op: 'Delete' } }, runOptions, /*skipSanitization*/ false, /*validateOnly*/
false, validSchemaController).
---
Nitpick comments:
In `@spec/InstallationDedup.spec.js`:
- Around line 83-104: The test is too lax about the positional arguments passed
to database.update in InstallationDedup.removeConflictingDeviceToken so a
slipped/misordered signature (many vs schema controller) won't be caught; update
the assertions to check the full call signature for the update-paths by
asserting database.update was called with all arguments in the exact expected
order (for example: '_Installation', { deviceToken: 'X' }, { deviceToken: {
__op: 'Delete' } }, {}, true, undefined) and do the same tightened
full-signature assertion for the other update case(s) referenced (lines around
179-202 and 227-249), while keeping the logSpy.verbose assertion.
🪄 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: 1c33484f-985b-4714-b069-660a0d3d5a6d
📒 Files selected for processing (2)
spec/InstallationDedup.spec.jssrc/InstallationDedup.js
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 `@spec/ParseInstallation.spec.js`:
- Around line 1721-1727: Test expectation is mismatched:
Config.validateInstallations (in src/Config.js, function validateInstallations)
throws "installation.<key> is not a valid installation option." for unknown
nested keys, so update the test in ParseInstallation.spec.js that calls
reconfigureServer({ installation: { unknownKey: 'foo' } }) to expect rejection
with "installation.unknownKey is not a valid installation option." instead of
the current message; ensure the assertion string exactly matches the error
format produced by validateInstallations.
🪄 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: 104bf91e-966f-40fa-bdef-e8c4c212a39f
📒 Files selected for processing (3)
spec/InstallationDedup.spec.jsspec/ParseInstallation.spec.jssrc/InstallationDedup.js
✅ Files skipped from review due to trivial changes (1)
- spec/InstallationDedup.spec.js
Member
Author
|
@coderabbitai resume |
✅ Actions performedReviews resumed. |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
spec/ParseInstallation.spec.js (2)
1360-1366: ⚡ Quick winAssert the original channel payloads, not just field presence.
These tests are meant to lock down the
"update"contract, buttoBeDefined()would still pass if the loser rows kept achannelsfield with the wrong value. Please assert the exact arrays for each cleared row so a regression that overwrites or emptieschannelsis caught.Proposed tightening of the assertions
const cleared = all.filter(r => r.installationId !== 'iid-c'); - cleared.forEach(r => { - expect(r.deviceToken).toBeUndefined(); - expect(r.channels).toBeDefined(); - }); + expect( + Object.fromEntries(cleared.map(r => [r.installationId, r.channels])) + ).toEqual({ + 'iid-a': ['old-news'], + 'iid-b': ['old-sports'], + });const cleared = all.filter(r => r.installationId !== 'multi-iid-d'); expect(cleared.length).toBe(3); - cleared.forEach(r => { - expect(r.deviceToken).toBeUndefined(); - }); + expect( + Object.fromEntries(cleared.map(r => [r.installationId, r.channels])) + ).toEqual({ + 'multi-iid-a': ['c-multi-iid-a'], + 'multi-iid-b': ['c-multi-iid-b'], + 'multi-iid-c': ['c-multi-iid-c'], + });Also applies to: 1466-1470
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/ParseInstallation.spec.js` around lines 1360 - 1366, The tests currently only check presence of the channels field using expect(r.channels).toBeDefined(), which won't catch regressions that change its contents; update the assertions in the loop that iterates over cleared (and the similar block around lines 1466-1470) to assert exact channel arrays for each installation row (use expect(r.channels).toEqual(...) with the original expected arrays for each installationId such as those created in the test setup) instead of toBeDefined(), and keep the deviceToken assertion as-is.
1580-1694: ⚡ Quick winAdd an
enforceAuth=trueintegration case for the merge path.This block exercises
applyDuplicateDeviceTokenMerge()for delete/update and both priorities, but never with auth enforcement enabled. That helper has its ownenforceAuthbranch and permission-denial handling insrc/InstallationDedup.js, so a focused ACL-protected merge test would cover a materially different path than the non-merge dedup specs above.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/ParseInstallation.spec.js` around lines 1580 - 1694, Add a new integration test case in this describe that enables ACL enforcement by calling reconfigureWithInstallationOptions with enforceAuth: true (plus whichever duplicateDeviceTokenAction/duplicateDeviceTokenMergePriority you want to cover), then create the merge fixture via setupMergeFixture and trigger the merge with rest.create using auth.nobody(config); assert the outcome expected under the enforceAuth branch (permission denied/403 or no-merge behavior according to applyDuplicateDeviceTokenMerge in src/InstallationDedup.js) — repeat for at least one combination of mergePriority/action to exercise the enforceAuth branch and its permission-denial handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@spec/ParseInstallation.spec.js`:
- Around line 1360-1366: The tests currently only check presence of the channels
field using expect(r.channels).toBeDefined(), which won't catch regressions that
change its contents; update the assertions in the loop that iterates over
cleared (and the similar block around lines 1466-1470) to assert exact channel
arrays for each installation row (use expect(r.channels).toEqual(...) with the
original expected arrays for each installationId such as those created in the
test setup) instead of toBeDefined(), and keep the deviceToken assertion as-is.
- Around line 1580-1694: Add a new integration test case in this describe that
enables ACL enforcement by calling reconfigureWithInstallationOptions with
enforceAuth: true (plus whichever
duplicateDeviceTokenAction/duplicateDeviceTokenMergePriority you want to cover),
then create the merge fixture via setupMergeFixture and trigger the merge with
rest.create using auth.nobody(config); assert the outcome expected under the
enforceAuth branch (permission denied/403 or no-merge behavior according to
applyDuplicateDeviceTokenMerge in src/InstallationDedup.js) — repeat for at
least one combination of mergePriority/action to exercise the enforceAuth branch
and its permission-denial handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fd8d1e5f-ea95-4e7e-8458-c1638288ba61
📒 Files selected for processing (1)
spec/ParseInstallation.spec.js
parseplatformorg
pushed a commit
that referenced
this pull request
Apr 30, 2026
# [9.9.0-alpha.3](9.9.0-alpha.2...9.9.0-alpha.3) (2026-04-30) ### Features * Add installation deviceToken deduplication options ([#10451](#10451)) ([9fee1a0](9fee1a0))
Contributor
|
🎉 This change has been released in version 9.9.0-alpha.3 |
parseplatformorg
pushed a commit
that referenced
this pull request
May 1, 2026
# [9.9.0](9.8.0...9.9.0) (2026-05-01) ### Bug Fixes * Context mutations leak across requests in `ParseServerRESTController` ([#10291](#10291)) ([60a58ec](60a58ec)) * MFA SMS one-time password accepted twice under concurrent login ([GHSA-jpq4-7fmq-q5fj](GHSA-jpq4-7fmq-q5fj)) ([#10448](#10448)) ([725be0d](725be0d)) ### Features * Add `rawValues` and `rawFieldNames` options for aggregation queries ([#10438](#10438)) ([f26700e](f26700e)) * Add installation deviceToken deduplication options ([#10451](#10451)) ([9fee1a0](9fee1a0))
Contributor
|
🎉 This change has been released in version 9.9.0 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pull Request
Issue
Parse Server's
_Installationdeduplication ondeviceTokencollisions today runs as master and always destroys the conflicting row, regardless of the caller's auth context or the developer's intent. This means:_Installationare not honored on the dedup path — the dedup bypasses them silently.installationIdand another bydeviceTokenonly), the deviceToken-only orphan always wins, even when the active install carries more meaningful data.Developers have no way to opt out of any of these behaviors.
Approach
Add a new
installationserver option block with three options:installation.duplicateDeviceTokenActionEnforceAuth: Boolean— defaultfalse. Whentrue, the dedup operation runs with the caller's auth context so ACL and CLP are honored. Master and maintenance keys always bypass regardless. The defaultfalseis registered as deprecated (DEPPS24); the planned default in 10.0.0 istrue.installation.duplicateDeviceTokenAction: 'delete' | 'update'— default'delete'.'update'clears the now-conflicting ID field on the conflicting row instead of destroying it, preserving custom fields, channels, and history.installation.duplicateDeviceTokenMergePriority: 'deviceToken' | 'installationId'— default'deviceToken'. Selects which side wins the merge case (when the new install matches an existing row byinstallationIdand another bydeviceTokenwithout aninstallationId).Implementation:
src/InstallationDedup.jswith two helpers (removeConflictingDeviceTokenfor new-install/PUT-deviceToken-change collisions,applyDuplicateDeviceTokenMergefor the merge case). Both honor the option-driven dispatch and log per a uniform policy (verbosefor success / no-match,warnfor permission denial,errorfor unexpected errors).RestWrite.handleInstallationrefactored at three sites to call the helpers. All three sites are now consistently awaited; today's two new-install / PUT sites were fire-and-forget. The latency cost is bounded to requests that actually fire dedup.Config.validateInstallationsvalidates the new option block; throws on unknown keys, wrong types, or invalid enum values; defaults missing nested keys.DEPRECATIONS.mdentry DEPPS24 for the future default flip onduplicateDeviceTokenActionEnforceAuth.Default option values preserve today's behavior, so this is non-breaking for existing apps.
Tasks
Summary by CodeRabbit
New Features
installationoptions to control duplicate device-token handling: action (delete/update), auth enforcement, and merge priority.Refactor
Configuration
installationsettings; environment-driven option surface exposed.Documentation / Deprecations
Tests