fix: Reconcile local authData on anonymous-to-email conversion#1136
Open
chadpav wants to merge 3 commits intoparse-community:masterfrom
Open
fix: Reconcile local authData on anonymous-to-email conversion#1136chadpav wants to merge 3 commits intoparse-community:masterfrom
chadpav wants to merge 3 commits intoparse-community:masterfrom
Conversation
Setting username on a persisted anonymous user and calling save() now unlinks the anonymous provider server-side and leaves the local authData consistent with the cleaned server record — without requiring a follow-up GET. Previously, the save's PUT response carried only the dirty fields written by the client (no authData), so the additive response merge in _handleSingleResult could not reconcile the local authData copy. Apps had to follow up with GET /users/me to refresh local state. This mirrors iOS PFUser's stripAnonymity + cleanUpAuthData pattern. No public API changes; the new helpers are private.
|
🚀 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 local reconciliation for anonymous auth data in ParseUser: setting Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes 🚥 Pre-merge checks | ✅ 6 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (6 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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1136 +/- ##
==========================================
+ Coverage 44.42% 45.25% +0.83%
==========================================
Files 62 62
Lines 3730 3752 +22
==========================================
+ Hits 1657 1698 +41
+ Misses 2073 2054 -19 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/dart/test/src/objects/parse_user/parse_user_anonymous_link_test.dart (1)
63-105: Please add anupdate()variant of this regression.
ParseUser.update()now has its own cleanup hook too, but the suite only exercisessave(). A matchingupdate()case would keep those two paths from drifting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/dart/test/src/objects/parse_user/parse_user_anonymous_link_test.dart` around lines 63 - 105, Add a parallel test that mirrors the existing "after a successful save..." case but invokes ParseUser.update() instead of save(); use anonymousUserWithObjectId() to create the user, set username/password the same way, stub the same network response (matching the current mocked call pattern - e.g. client.put/putPath or client.patch if update uses PATCH) and assert that user.authData no longer contains 'anonymous' after a successful update response; include the same expect checks and reason text so update()'s cleanup hook is covered the same as save().packages/dart/lib/src/objects/parse_user.dart (1)
539-545: Drop emptyauthDatain the unsaved-user branch.If
anonymouswas the only provider, Lines 539-545 leaveauthDataas{}in local state. RemovingkeyVarAuthDataentirely here would keep lazy users fully reconciled instead of carrying a synthetic empty map forward.♻️ Proposed change
if (objectId == null) { authData.remove(_keyAuthAnonymous); + if (authData.isEmpty) { + _objectData.remove(keyVarAuthData); + _unsavedChanges.remove(keyVarAuthData); + return; + } } else { authData[_keyAuthAnonymous] = null; } _unsavedChanges[keyVarAuthData] = authData;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/dart/lib/src/objects/parse_user.dart` around lines 539 - 545, The branch that updates auth providers leaves an empty map in _unsavedChanges[keyVarAuthData] when anonymous was the only provider; update the logic in the function handling objectId/authData (referencing objectId, authData, _keyAuthAnonymous, _unsavedChanges, keyVarAuthData) so that after removing the anonymous provider you check if authData.isEmpty and, if so, remove keyVarAuthData from _unsavedChanges instead of assigning an empty map; otherwise keep assigning authData as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/dart/lib/src/objects/parse_user.dart`:
- Around line 539-545: The branch that updates auth providers leaves an empty
map in _unsavedChanges[keyVarAuthData] when anonymous was the only provider;
update the logic in the function handling objectId/authData (referencing
objectId, authData, _keyAuthAnonymous, _unsavedChanges, keyVarAuthData) so that
after removing the anonymous provider you check if authData.isEmpty and, if so,
remove keyVarAuthData from _unsavedChanges instead of assigning an empty map;
otherwise keep assigning authData as before.
In
`@packages/dart/test/src/objects/parse_user/parse_user_anonymous_link_test.dart`:
- Around line 63-105: Add a parallel test that mirrors the existing "after a
successful save..." case but invokes ParseUser.update() instead of save(); use
anonymousUserWithObjectId() to create the user, set username/password the same
way, stub the same network response (matching the current mocked call pattern -
e.g. client.put/putPath or client.patch if update uses PATCH) and assert that
user.authData no longer contains 'anonymous' after a successful update response;
include the same expect checks and reason text so update()'s cleanup hook is
covered the same as save().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0bfa836a-98b4-4656-84cf-277a08836115
📒 Files selected for processing (2)
packages/dart/lib/src/objects/parse_user.dartpackages/dart/test/src/objects/parse_user/parse_user_anonymous_link_test.dart
save() and update() both run _cleanUpAuthData() in their success branch but were patched independently. Add a parallel test that invokes update() instead of save() so a future regression in either path is caught. Also tightened test descriptions to focus on client-side behavior.
On the lazy (no objectId) path of _stripAnonymity, removing the anonymous provider could leave an empty map in _unsavedChanges under keyVarAuthData. The empty map then serialized into the POST body via toJson(forApiRQ: true) on signUp, sending wire noise the SDK shouldn't emit. Mirrors the isEmpty handling already present in _cleanUpAuthData.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/dart/lib/src/objects/parse_user.dart (1)
551-568: Consider extracting duplicated null-pruning into a tiny helper.
_cleanUpAuthData()repeats the same “remove null providers, drop map if empty” flow for_objectDataand_unsavedChanges. A helper would reduce drift risk.♻️ Optional refactor sketch
+ void _pruneNullAuthProviders(Map<String, dynamic> map) { + map.removeWhere((_, dynamic value) => value == null); + } + void _cleanUpAuthData() { final Map<String, dynamic>? authData = _objectData[keyVarAuthData] as Map<String, dynamic>?; if (authData != null) { - authData.removeWhere((_, dynamic value) => value == null); + _pruneNullAuthProviders(authData); if (authData.isEmpty) { _objectData.remove(keyVarAuthData); } } final Map<String, dynamic>? dirty = _unsavedChanges[keyVarAuthData] as Map<String, dynamic>?; if (dirty != null) { - dirty.removeWhere((_, dynamic value) => value == null); + _pruneNullAuthProviders(dirty); if (dirty.isEmpty) { _unsavedChanges.remove(keyVarAuthData); } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/dart/lib/src/objects/parse_user.dart` around lines 551 - 568, Extract the repeated "remove null entries and drop empty map" logic in _cleanUpAuthData into a small helper (e.g., _pruneNullMap) that takes a Map<String, dynamic>? and the container map to remove the key from (or returns a bool indicating emptiness); then call it for both _objectData[keyVarAuthData] and _unsavedChanges[keyVarAuthData] instead of duplicating the removeWhere + isEmpty checks. Ensure the helper uses the same null-safe casts and removes keyVarAuthData from the appropriate parent map when the child map becomes empty, preserving current behavior of _cleanUpAuthData.packages/dart/test/src/objects/parse_user/parse_user_anonymous_link_test.dart (1)
18-23: Optional: de-duplicate repeated PUT stubbing and provider key literals.A small local helper (and a shared
const anonymousProvider = 'anonymous') would make these tests easier to evolve.Also applies to: 67-82, 105-120, 146-161, 192-205, 244-247, 266-281
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/dart/test/src/objects/parse_user/parse_user_anonymous_link_test.dart` around lines 18 - 23, Duplicate literals and repeated PUT stubbing in parse_user_anonymous_link_test.dart should be consolidated: introduce a shared const anonymousProvider = 'anonymous' and a small helper function (e.g., buildPutPath(userObjectId) or stubPutForUser(userObjectId, body)) to return the putPath or register the PUT stub using existing symbols keyEndPointClasses, keyClassUser, userObjectId and anonymousId; replace repeated inline Uri.parse(...) and literal 'anonymous' occurrences with the new helper/const and update tests that reference putPath, userObjectId, anonymousId to call the helper to remove duplication and make stubbing consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/dart/lib/src/objects/parse_user.dart`:
- Around line 551-568: Extract the repeated "remove null entries and drop empty
map" logic in _cleanUpAuthData into a small helper (e.g., _pruneNullMap) that
takes a Map<String, dynamic>? and the container map to remove the key from (or
returns a bool indicating emptiness); then call it for both
_objectData[keyVarAuthData] and _unsavedChanges[keyVarAuthData] instead of
duplicating the removeWhere + isEmpty checks. Ensure the helper uses the same
null-safe casts and removes keyVarAuthData from the appropriate parent map when
the child map becomes empty, preserving current behavior of _cleanUpAuthData.
In
`@packages/dart/test/src/objects/parse_user/parse_user_anonymous_link_test.dart`:
- Around line 18-23: Duplicate literals and repeated PUT stubbing in
parse_user_anonymous_link_test.dart should be consolidated: introduce a shared
const anonymousProvider = 'anonymous' and a small helper function (e.g.,
buildPutPath(userObjectId) or stubPutForUser(userObjectId, body)) to return the
putPath or register the PUT stub using existing symbols keyEndPointClasses,
keyClassUser, userObjectId and anonymousId; replace repeated inline
Uri.parse(...) and literal 'anonymous' occurrences with the new helper/const and
update tests that reference putPath, userObjectId, anonymousId to call the
helper to remove duplication and make stubbing consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 12dc8694-f354-42fe-8a92-7a38f83366ba
📒 Files selected for processing (2)
packages/dart/lib/src/objects/parse_user.dartpackages/dart/test/src/objects/parse_user/parse_user_anonymous_link_test.dart
Contributor
Author
|
@mtrezza There are a few other PR's open. Do we have people merging PR's? You can add me as a maintainer if needed. |
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
1 participant
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.
Issue
No existing issue; problem described below.
Approach
When a persisted anonymous user has its
username(and typicallypassword) set and is saved,parseUser.save()issues aPUT /classes/_User/:objectIdcarrying only the dirty fields. The Dart SDK treats this as a generic property update — it neither emits anauthDataunlink signal in the request body nor cleans up the local cache after the save. As a result,parseUser.authDatacontinues to contain{ anonymous: { id: '...' } }after a successful conversion and is persisted to disk via_onResponseSuccessin that stale shape.The iOS Parse SDK handles this client-side via two helpers in
PFUser.m:stripAnonymity(PFUser.m:593-607): theusernamesetter writesauthData[anonymous] = NSNullinto the local user. The PUT body re-injects the map, so the request carriesauthData: { anonymous: null }— Parse Server's documented unlink signal for that provider.cleanUpAuthData(PFUser.m:300-313): after a successful save, the localauthDatahas any null/NSNull entries stripped, thenPFCurrentUserController.saveCurrentObjectAsync:persists the cleaned user.This PR ports the same pattern to the Dart SDK as two private helpers in
ParseUser:_stripAnonymity()is invoked from theusernamesetter (matching iOS — onlyusernametriggers the strip;password/emailsetters are unchanged). On a persisted user it setsauthData['anonymous'] = nulllocally; on a lazy user (noobjectId) it removes the entry outright._cleanUpAuthData()is invoked fromsave()andupdate()between the response merge and_onResponseSuccess(), removing null entries from localauthData.A dartdoc paragraph was added to
loginAnonymousdocumenting the conversion path, which previously had no inline coverage in the Dart SDK source.Tasks
Behavior change disclosure
Two observable changes for the anonymous-conversion case:
objectIdis set andauthDatacontains ananonymousentry, settingusernameand saving now produces a PUT body that includesauthData: { anonymous: null }. Previously noauthDatafield was sent for that save. Standard Parse Server treats the null as the unlink request it always was; custombeforeSavetriggers on_Userthat explicitly reject nullauthData.anonymouswould behave differently.usernameon an anonymous-with-objectIdParseUsernow mutates the localauthDatamap (writesnullfor theanonymouskey). After a successful save, the entry is removed entirely. Code that inspectsparseUser.authDatabetween.username = ...and.save()and expects the original anonymous payload will observe a difference.The commit prefix is
fix:(patch bump under semantic-release). Rationale:If maintainers prefer a stronger version signal, the prefix can be amended to
fix!:to trigger a major bump.Documentation follow-up (out of scope for this PR)
This PR adds a dartdoc paragraph to
loginAnonymouscovering the conversion pattern. The companion user guide at https://docs.parseplatform.org/dart/guide/ (separate repo) has no section equivalent to the iOS guide's "Linking Anonymous Users". Mirroring that section into the Dart guide is a natural follow-up once this PR lands.Tests
Added
packages/dart/test/src/objects/parse_user/parse_user_anonymous_link_test.dartwith 6 tests:usernameon a persisted anonymous user marksauthData['anonymous'] = nulllocally.user.authDatano longer containsanonymous.authData: { anonymous: null }.authDataentries (e.g.facebook) survive the conversion.objectId) user, settingusernameremoves theanonymousentry without leaving a null marker.usernameon a non-anonymous user does not synthesizeauthDatain the request body.All 212 existing tests in
packages/dart/test/continue to pass.Summary by CodeRabbit
Release Notes
New Features
Tests