fix: Error in afterSave trigger for Parse.Role due to name field
#9883
Conversation
|
I will reformat the title to use the proper commit message syntax. |
parse-github-assistant
bot
changed the title
|
🚀 Thanks for opening this pull request! ❌ Please fill out all fields with a placeholder |
✅ 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. |
📝 WalkthroughWalkthroughAdds tests covering Parse.Role lifecycle hooks (beforeSave/afterSave) and a RestWrite guard that marks Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant RestWrite
participant Hooks
participant Database
Client->>RestWrite: Save Role { "name": "Admins" } (masterKey)
RestWrite->>Hooks: beforeSave(Parse.Role)
Hooks-->>RestWrite: (may modify object: add fields or change name)
RestWrite->>Database: Persist role (create/update)
Database-->>RestWrite: Respond with { objectId, ... }
Note right of RestWrite #DDEBF7: If class == _Role and objectId present\nappend "name" to readOnlyAttributes
RestWrite->>Hooks: afterSave(Parse.Role)
Hooks-->>RestWrite: Complete
Client->>RestWrite: Later update attempting to change name
RestWrite-->>Client: Excludes `name` from update (treated read-only)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/RestWrite.js (1)
1746-1751: Clarify the comment to better explain the root cause.The comment mentions that
_handleSaveResponsesets the objectId, but the deeper issue is that Parse SDK's internal validation prevents setting the 'name' field on a Parse.Role object that has an objectId. The current approach correctly adds 'name' to readOnlyAttributes to prevent it from being set during object construction inbuildParseObjects, while still making it available for reading (via extraData for creates on lines 1752-1756, or via originalData for updates).Consider revising the comment to:
- // For _Role class, the 'name' field is read-only after the object has been saved - // Since _handleSaveResponse is called after buildParseObjects and sets the objectId, - // we need to exclude 'name' from being set to avoid "A role's name can only be set before it has been saved" error + // For _Role class, the 'name' field is read-only after the object has an objectId. + // Parse SDK throws "A role's name can only be set before it has been saved" when + // attempting to set 'name' on a role with an objectId. Adding it to readOnlyAttributes + // prevents the field from being set during object construction while keeping it readable.spec/ParseRole.spec.js (1)
605-633: Refactor to use async/await instead of promise chains with done().Based on learnings, tests should use async/await patterns rather than
.then()chains withdone()callbacks. Additionally, the hardcoded 100ms timeout is fragile and could lead to flaky tests.Based on learnings.
Apply this diff to modernize the test:
- it('should trigger afterSave hook when using Parse.Role class reference', done => { + it('should trigger afterSave hook when using Parse.Role class reference', async () => { let afterSaveCalled = false; + const hookPromise = new Promise(resolve => { + Parse.Cloud.afterSave(Parse.Role, req => { + afterSaveCalled = true; + expect(req.object).toBeDefined(); + expect(req.object.get('name')).toBe('AnotherTestRole'); + resolve(); + }); + }); - Parse.Cloud.afterSave(Parse.Role, req => { - afterSaveCalled = true; - expect(req.object).toBeDefined(); - expect(req.object.get('name')).toBe('AnotherTestRole'); - }); - const acl = new Parse.ACL(); acl.setPublicReadAccess(true); const role = new Parse.Role('AnotherTestRole', acl); - role - .save({}, { useMasterKey: true }) - .then(savedRole => { - expect(savedRole.id).toBeDefined(); - // Give the afterSave hook some time to execute - return new Promise(resolve => setTimeout(resolve, 100)); - }) - .then(() => { - expect(afterSaveCalled).toBe(true); - done(); - }) - .catch(err => { - fail(`Should not have failed: ${err.message}`); - done(); - }); + const savedRole = await role.save({}, { useMasterKey: true }); + expect(savedRole.id).toBeDefined(); + + // Wait for the afterSave hook to complete + await hookPromise; + expect(afterSaveCalled).toBe(true); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
spec/ParseRole.spec.js(1 hunks)src/RestWrite.js(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-05-09T09:59:06.289Z
Learnt from: mtrezza
PR: parse-community/parse-server#9445
File: spec/ParseLiveQuery.spec.js:1340-1375
Timestamp: 2025-05-09T09:59:06.289Z
Learning: Tests in the parse-server repository should use promise-based approaches rather than callback patterns with `done()`. Use a pattern where a Promise is created that resolves when the event occurs, then await that promise.
Applied to files:
spec/ParseRole.spec.js
📚 Learning: 2025-05-09T09:59:06.289Z
Learnt from: mtrezza
PR: parse-community/parse-server#9445
File: spec/ParseLiveQuery.spec.js:1340-1375
Timestamp: 2025-05-09T09:59:06.289Z
Learning: New tests in the parse-server repository should use async/await with promise-based patterns rather than callback patterns with `done()`. The preferred pattern is to create a Promise that resolves when an expected event occurs, then await that Promise.
Applied to files:
spec/ParseRole.spec.js
🧬 Code graph analysis (1)
spec/ParseRole.spec.js (3)
spec/helper.js (1)
Parse(4-4)src/Auth.js (4)
Parse(1-1)req(538-538)req(551-551)role(353-353)spec/ParseLiveQuery.spec.js (1)
role(811-811)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: Node 18
- GitHub Check: MongoDB 7, ReplicaSet
- GitHub Check: Node 20
- GitHub Check: MongoDB 6, ReplicaSet
- GitHub Check: Docker Build
- GitHub Check: MongoDB 8, ReplicaSet
- GitHub Check: Redis Cache
- GitHub Check: PostgreSQL 15, PostGIS 3.4
- GitHub Check: PostgreSQL 15, PostGIS 3.3
- GitHub Check: PostgreSQL 16, PostGIS 3.5
- GitHub Check: PostgreSQL 18, PostGIS 3.6
- GitHub Check: PostgreSQL 15, PostGIS 3.5
- GitHub Check: Code Analysis (javascript)
- GitHub Check: PostgreSQL 17, PostGIS 3.5
🔇 Additional comments (1)
src/RestWrite.js (1)
1746-1751: LGTM! The fix correctly addresses the Parse.Role afterSave issue.The implementation properly prevents the 'name' field from being set during object construction in afterSave triggers, while ensuring it remains readable through extraData (for creates) or originalData (for updates).
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## alpha #9883 +/- ##
=======================================
Coverage 93.02% 93.03%
=======================================
Files 187 187
Lines 15158 15161 +3
Branches 176 176
=======================================
+ Hits 14101 14105 +4
+ Misses 1045 1044 -1
Partials 12 12 ☔ 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 (2)
spec/ParseRole.spec.js (2)
608-612: Consider verifying role persistence after afterSave completes.To ensure the afterSave hook doesn't corrupt the role object (which was the original issue described in the PR), consider adding a query to fetch the role from the database after the hook completes.
Add this verification after the hook executes:
Parse.Cloud.afterSave(Parse.Role, req => { expect(req.object).toBeDefined(); expect(req.object.get('name')).toBe('AnotherTestRole'); }); const acl = new Parse.ACL(); acl.setPublicReadAccess(true); const role = new Parse.Role('AnotherTestRole', acl); role .save({}, { useMasterKey: true }) .then(savedRole => { expect(savedRole.id).toBeDefined(); - // Give the afterSave hook some time to execute - return new Promise(resolve => setTimeout(resolve, 100)); + return afterSavePromise.then(() => savedRole.id); }) - .then(() => { + .then(roleId => { + // Verify the role can still be fetched after afterSave + const query = new Parse.Query(Parse.Role); + return query.get(roleId, { useMasterKey: true }); + }) + .then(fetchedRole => { + expect(fetchedRole.get('name')).toBe('AnotherTestRole'); done(); })
684-713: Clarify that name modification in beforeSave is expected behavior.This test modifies the role name in the
beforeSavehook, which is valid since the role hasn't been saved yet. This is different from the afterSave scenario described in the PR where the role already has an objectId and name modification would be restricted.Consider adding a comment to clarify this distinction.
it('should allow modifying role name in beforeSave hook', done => { + // Note: Modifying the name in beforeSave is allowed because the role hasn't been saved yet. + // This is different from afterSave where the role already has an objectId. Parse.Cloud.beforeSave(Parse.Role, req => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
spec/ParseRole.spec.js(1 hunks)src/RestWrite.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/RestWrite.js
🧰 Additional context used
🧬 Code graph analysis (1)
spec/ParseRole.spec.js (1)
spec/helper.js (1)
Parse(4-4)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
- GitHub Check: MongoDB 7, ReplicaSet
- GitHub Check: PostgreSQL 18, PostGIS 3.6
- GitHub Check: MongoDB 8, ReplicaSet
- GitHub Check: Node 20
- GitHub Check: Node 18
- GitHub Check: PostgreSQL 16, PostGIS 3.5
- GitHub Check: MongoDB 6, ReplicaSet
- GitHub Check: Redis Cache
- GitHub Check: PostgreSQL 15, PostGIS 3.5
- GitHub Check: PostgreSQL 17, PostGIS 3.5
- GitHub Check: PostgreSQL 15, PostGIS 3.4
- GitHub Check: PostgreSQL 15, PostGIS 3.3
- GitHub Check: Docker Build
🔇 Additional comments (1)
spec/ParseRole.spec.js (1)
605-713: Add cleanup for registered Cloud Function hooks.The tests register
Parse.Cloud.beforeSaveandParse.Cloud.afterSavehooks but never clean them up. Hooks are stored in_triggerStore[applicationId]and persist across tests, potentially causing test pollution.Add an
afterEachblock to unregister hooks:afterEach(() => { Parse.Cloud._triggers = undefined; });Or use the existing
remove()function from triggers module if exposed, or check if Parse.Cloud provides an unregister method for cleanup.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
spec/CloudCodeLogger.spec.js (2)
250-251: Recommend consistent promise-based synchronization.The arbitrary 100ms delay may be insufficient if the afterSave hook takes longer, or wasteful if it completes faster. Consider adopting the promise-based pattern introduced later in this file (lines 386-404) for more precise synchronization.
Apply this pattern for consistency:
+ let afterSaveCompleted; + const afterSavePromise = new Promise(resolve => { + afterSaveCompleted = resolve; + }); + - Parse.Cloud.beforeSave('TestClass', () => { }); - Parse.Cloud.afterSave('TestClass', () => { }); + Parse.Cloud.beforeSave('TestClass', () => {}); + Parse.Cloud.afterSave('TestClass', async () => { + afterSaveCompleted(); + }); spy = spyOn(Config.get('test').loggerController.adapter, 'log').and.callThrough(); const obj = new Parse.Object('TestClass'); await obj.save(); - // Wait for afterSave to complete - await new Promise(resolve => setTimeout(resolve, 100)); + await afterSavePromise;
417-418: Drop the 100 ms delay; error logging is synchronous in thebeforeSavetrigger, so you can assertspyimmediately afterawait expectAsync(...).toBeRejected().
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
spec/CloudCodeLogger.spec.js(4 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-05-09T09:59:06.289Z
Learnt from: mtrezza
PR: parse-community/parse-server#9445
File: spec/ParseLiveQuery.spec.js:1340-1375
Timestamp: 2025-05-09T09:59:06.289Z
Learning: New tests in the parse-server repository should use async/await with promise-based patterns rather than callback patterns with `done()`. The preferred pattern is to create a Promise that resolves when an expected event occurs, then await that Promise.
Applied to files:
spec/CloudCodeLogger.spec.js
📚 Learning: 2025-05-09T09:59:06.289Z
Learnt from: mtrezza
PR: parse-community/parse-server#9445
File: spec/ParseLiveQuery.spec.js:1340-1375
Timestamp: 2025-05-09T09:59:06.289Z
Learning: Tests in the parse-server repository should use promise-based approaches rather than callback patterns with `done()`. Use a pattern where a Promise is created that resolves when the event occurs, then await that promise.
Applied to files:
spec/CloudCodeLogger.spec.js
📚 Learning: 2025-05-04T20:41:05.147Z
Learnt from: mtrezza
PR: parse-community/parse-server#9445
File: spec/ParseLiveQuery.spec.js:1312-1338
Timestamp: 2025-05-04T20:41:05.147Z
Learning: New tests in the parse-server repository should use async/await with promise-based patterns rather than callback patterns with `done()`.
Applied to files:
spec/CloudCodeLogger.spec.js
🧬 Code graph analysis (1)
spec/CloudCodeLogger.spec.js (1)
src/Config.js (1)
Config(35-768)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
- GitHub Check: PostgreSQL 18, PostGIS 3.6
- GitHub Check: PostgreSQL 15, PostGIS 3.4
- GitHub Check: PostgreSQL 17, PostGIS 3.5
- GitHub Check: Node 18
- GitHub Check: PostgreSQL 15, PostGIS 3.3
- GitHub Check: PostgreSQL 16, PostGIS 3.5
- GitHub Check: Redis Cache
- GitHub Check: Node 20
- GitHub Check: PostgreSQL 15, PostGIS 3.5
- GitHub Check: MongoDB 8, ReplicaSet
- GitHub Check: MongoDB 6, ReplicaSet
- GitHub Check: MongoDB 7, ReplicaSet
- GitHub Check: Docker Build
spec/CloudCodeLogger.spec.js
Outdated
| const obj = new Parse.Object('TestClass'); | ||
| await obj.save(); | ||
| // Wait for afterSave to complete | ||
| await new Promise(resolve => setTimeout(resolve, 100)); |
There was a problem hiding this comment.
Shouldn't await obj.save(); finish only after afterSave completes?
There was a problem hiding this comment.
it should but it seems that cloud logger is performing background actions and the timeout helps
|
I'm a bit confused by the scope of changes. Some seem to be just for test flakyness, while others seem to be for the actual bug of Parse.Role. I would't recommend to pack this all into a single PR, as we want to observe the effects of the role fix separately, especially if they have nothing to do with each other (which I'm just guessing). |
|
I know @mtrezza , sorry about that! I was just trying to fix the flaky test because it’s really annoying to get notifications and a failing CI status while working on a feature. It makes it hard to tell whether the failure is from the new feature or something else. I tried a small patch on the fly. I suggest keeping it, since it should improve the contributor experience (DX). If the Cloud Logger keeps failing in future contributions, it might need an additional refactor and fully dedicated PR. Just trying to help stabilize things :) By the way, the Parse.Role bug is now fully covered and fixed. |
…uls/parse-server into moumouls/fix-after-save-parse-role
|
i'll remove it, it doesn't properly fix the cloud logger flaky |
There was a problem hiding this comment.
Looks good. @coderabbitai could this be a breaking change? and suggest an angular commit message.
|
@coderabbitai suggest an angular commit message; look at the code and suggest what exactly this fixes. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 3887 @mtrezza Based on the code changes, here's my suggested Angular commit message: Detailed explanation: The fix addresses a bug where Parse.Role's What the code does: In
This prevents Alternative commit message options: If you prefer more context: Or more technical: |
|
@coderabbitai Is this a breaking change, if the response is altered? For example for someone using the Parse Server REST API directly without using the Parse JS SDK. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 3512 🏁 Script executed: Length of output: 2371 🏁 Script executed: Length of output: 2657 🏁 Script executed: Length of output: 767 🏁 Script executed: Length of output: 3487 🏁 Script executed: Length of output: 3852 🏁 Script executed: Length of output: 813 #!/bin/bash
# Check if afterSave triggers can modify the response that's sent to the client
rg -n -C20 "triggers\.maybeRunTrigger" src/RestWrite.js | grep -A20 -B20 "afterSave"
# Also look at the triggers module to see if afterSave can modify responses
rg -n "afterSave" src/triggers.js | head -20 |
|
@coderabbitai Is this PR a breaking change, since it seems to change the response? For example for someone using the Parse Server REST API directly without using the Parse JS SDK. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 903 🏁 Script executed: Length of output: 523 🏁 Script executed: Length of output: 1330 🏁 Script executed: Length of output: 4542 🏁 Script executed: Length of output: 3874 🏁 Script executed: Length of output: 970 🏁 Script executed: Length of output: 891 🏁 Script executed: Length of output: 854 @mtrezza No, this is not a breaking change for direct REST API users. Here's why: Response Flow:
What the Fix Actually Does:
Impact:
What IS Changed:
The fix is internal to trigger/LiveQuery processing and doesn't alter any client-facing API responses. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 2028 🏁 Script executed: Length of output: 0 🏁 Script executed: Length of output: 0 🏁 Script executed: Length of output: 420 🏁 Script executed: Length of output: 50 🏁 Script executed: Length of output: 0 🏁 Script executed: Length of output: 0 🏁 Script executed: Length of output: 304 🏁 Script executed: Length of output: 1046 🏁 Script executed: Length of output: 1869 🏁 Script executed: Length of output: 2229 🏁 Script executed: Length of output: 0 🏁 Script executed: Length of output: 0 🏁 Script executed: Length of output: 771 🏁 Script executed: Length of output: 134 🏁 Script executed: Length of output: 428 @mtrezza No, this is not a breaking change for users of the REST API. Here's why: The key flow:
What Looking at lines 1783-1786, However, the
Conclusion: The REST API response still contains the |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 669 🏁 Script executed: Length of output: 2655 🏁 Script executed: Length of output: 0 🏁 Script executed: Length of output: 31 🏁 Script executed: Length of output: 0 🏁 Script executed: Length of output: 2835 🏁 Script executed: Length of output: 3125 🏁 Script executed: Length of output: 0 🏁 Script executed: Length of output: 134 🏁 Script executed: Length of output: 210 🏁 Script executed: Length of output: 1084 🏁 Script executed: Length of output: 0 🏁 Script executed: Length of output: 618 🏁 Script executed: Length of output: 2947 🏁 Script executed: Length of output: 3361 🏁 Script executed: Length of output: 873 @mtrezza No, this is not a breaking change. Here's why: What the change does: The PR adds Why there's no breaking change:
Conclusion: This is an internal implementation detail that prevents a Parse SDK validation error without changing the actual response content. REST API clients will still receive the |
mtrezza
changed the title
afterSave trigger for Parse.Role due to name field
# [8.3.0-alpha.10](8.3.0-alpha.9...8.3.0-alpha.10) (2025-10-22) ### Bug Fixes * Error in `afterSave` trigger for `Parse.Role` due to `name` field ([#9883](#9883)) ([eb052d8](eb052d8))
|
🎉 This change has been released in version 8.3.0-alpha.10 |
3 participants
Pull Request
Issue
After save on Parse.Role don't work anymore, Parse SDK throw "A role's name can only be set before it has been saved." which is incorrect. It's related to how buildParseObject system work after the "afterSave" trigger.
Closes: FILL_THIS_OUT
Approach
Add "name" to readOnly attribute to avoid the error during buildObject
Tasks
Summary by CodeRabbit
Bug Fixes
Tests