fix: Arrays being set as empty when calling ParseObject.clearUnsavedChanges()

[nabos440]

Pull Request

• Report security issues confidentially.
• Any contribution is under this license.
• Link this pull request to an issue.

Issue

Closes: #1038

Approach

Added null safety check before clearing savedArray and setting to _estimatedArrayBeforeSaving in the onSaved() function of Parse Array class

This fixes the issue because the _handleSingleResult() function which handles the response for a single parse object in the _ParseResponseBuilder was calling _notifyChildrenAboutSave(); so when save() was called on any object it called _notifyChildrenAboutSaving(); first and then in the response when _notifyChildrenAboutSave(); was called, the _estimatedArrayBeforeSaving had values and _savedArray was not set as empty.

But in case of other queries where single object was returned like getUpdatedUser() or fetch() on any object it just called _notifyChildrenAboutSave(); in the _handleSingleResult() function which resulted in _savedArray being set as empty since _estimatedArrayBeforeSaving was null. I just added the null safety check before setting the _savedArray to _estimatedArrayBeforeSaving so now the savedArray is never returned empty in the response and the issue is resolved.

Summary by CodeRabbit

• Bug Fixes

  • Improved null-safety in array saving so arrays are only processed when present, preventing unintended null operations and preserving saved array state.

• Tests

  • Added a test ensuring arrays fetched from the server are not cleared by unsaved-change clearing operations after a fetch/query, validating array persistence across fetch and clear cycles.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[parse-github-assistant]

🚀 Thanks for opening this pull request!

[parse-github-assistant]

I will reformat the title to use the proper commit message syntax.

[mbfakourii]

Looks good. Could you add a test ?

[nabos440]

How to do that?

…

On Sat, Mar 22, 2025, 10:22 AM Mohammad Bagher Fakouri < ***@***.***> wrote: ***@***.**** requested changes on this pull request. Looks good. Could you add a test ? — Reply to this email directly, view it on GitHub <#1039 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AY6HMS7WCHAMJBEKI3GLEKT2VTXRZAVCNFSM6AAAAABZRD7BXCVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDOMBXHAZDONJQGA> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[mbfakourii]

How to do that?
…
On Sat, Mar 22, 2025, 10:22 AM Mohammad Bagher Fakouri < @.> wrote: @.* requested changes on this pull request. Looks good. Could you add a test ? — Reply to this email directly, view it on GitHub <#1039 (review)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AY6HMS7WCHAMJBEKI3GLEKT2VTXRZAVCNFSM6AAAAABZRD7BXCVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDOMBXHAZDONJQGA . You are receiving this because you authored the thread.Message ID: @.***>

Please check the examples in the link below:

https://github.com/parse-community/Parse-SDK-Flutter/tree/master/packages/dart/test

[nabos440]

I have seen the example. I can't seem to figure it out, I'm still a rookie developer. Can you do it yourself?

[nabos440]

When will it be merged?

[mtrezza]

@nabos440 To add a test, just clone an existing test from the packages/dart/test directory. If there's a file that matches the topic of the issue, then just add the test there, otherwise you can create a new file. If you look at the tests there, you should get a sense of how a test is written. A test should fail before your fix and pass after your fix. That way you can verify that the bug has ben fixed correctly, and that it won't be re-introduced in the future. Once you've added the test and committed, it will run in the CI here. Also, I've started the CI for your current change and it failed, looks like a lint issue. You can see how to fix them by opening a job log below.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 43.77%. Comparing base (dd139ec) to head (a27d80a).

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1039      +/-   ##
==========================================
+ Coverage   43.75%   43.77%   +0.01%     
==========================================
  Files          61       61              
  Lines        3588     3589       +1     
==========================================
+ Hits         1570     1571       +1     
  Misses       2018     2018              

☔ 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.

[nabos440]

They are failing because of codcov report upload failure and lint issue for which there is no detail as to what is causing the failure

[mtrezza]

If you run dart format locally you should see what's causing it. Unfortunately it's not visible with more details in the CI at the moment, not sure whether the command dart format --output=none --set-exit-if-changed packages/dart allows to change that, @mbfakourii?

[mbfakourii]

If you run dart format locally you should see what's causing it. Unfortunately it's not visible with more details in the CI at the moment, not sure whether the command dart format --output=none --set-exit-if-changed packages/dart allows to change that, @mbfakourii?

Yes, this can be checked with the dart format --output=none --set-exit-if-changed packages/dart command.

[mtrezza]

But what needs to change in the command so that it outputs the issue?

[mbfakourii]

But what needs to change in the command so that it outputs the issue?

But what needs to change in the command so that it outputs the issue?

The problem is visible in ci errors, no need to add any code.

[coderabbitai]

📝 Walkthrough

Walkthrough

Guards use of _estimatedArrayBeforeSaving in ParseArray.onSaved so _savedArray is only cleared/populated when the prior-estimate exists; _estimatedArrayBeforeSaving is then nulled. Adds a test ensuring clearUnsavedChanges() does not empty arrays after a fetch/query response (issue #1038).

Changes

Cohort / File(s)	Summary
Null-check guard in onSaved packages/dart/lib/src/objects/parse_array.dart	Wraps the onSaved logic in a null-check for _estimatedArrayBeforeSaving; only when non-null are _savedArray cleared and repopulated (using a non-null assertion) and _estimatedArrayBeforeSaving set to null, avoiding unconditional dereference.
New test for fetch/query behavior packages/dart/test/src/objects/parse_object/parse_object_array_test.dart	Adds test "Arrays should not be cleared when calling clearUnsavedChanges() after receiving response from fetch/query (issue #1038)". Simulates server fetch/query responses and verifies clearUnsavedChanges() does not clear arrays populated via fetch.
Manifest pubspec.yaml	(Touched in diff metadata)

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

• Inspect onSaved guarded branch to confirm saved vs estimated array semantics are correct and no unintended null-state changes occur.
• Verify the new test accurately simulates fetch/query responses and exercises clearUnsavedChanges() behavior.
• Check serialization/deserialization or caller assumptions that may rely on _savedArray being populated even when _estimatedArrayBeforeSaving is null.

Pre-merge checks and finishing touches

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	Title accurately describes the primary fix: null-safety check to prevent arrays from being cleared when calling clearUnsavedChanges().
Description check	✅ Passed	PR description includes issue link, detailed approach explaining the null-safety check, and clarifies the root cause and solution path differences.
Linked Issues check	✅ Passed	Code changes address issue #1038 by adding null-safety check in onSaved() to prevent savedArray from being set empty when _estimatedArrayBeforeSaving is null during fetch/query responses.
Out of Scope Changes check	✅ Passed	All changes are in-scope: parse_array.dart adds null-check for the reported bug, and test file adds regression test covering the issue scenario.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ Finishing touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 4b88a2f and 9d7fdf0.

📒 Files selected for processing (1)

• packages/dart/test/src/objects/parse_object/parse_object_array_test.dart (1 hunks)

🚧 Files skipped from review as they are similar to previous changes (1)

• packages/dart/test/src/objects/parse_object/parse_object_array_test.dart

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

packages/dart/lib/src/objects/parse_array.dart (1)

69-73: Consider adding a comment to document the edge case.

While the null-check is clear, a brief comment explaining why _estimatedArrayBeforeSaving can be null would help future maintainers. For example:

// onSaved() can be called without prior onSaving() in some code paths
// (e.g., getUpdatedUser(), fetch()). Skip updating _savedArray if
// _estimatedArrayBeforeSaving was never initialized.
if (_estimatedArrayBeforeSaving != null) {
  _savedArray.clear();
  _savedArray.addAll(_estimatedArrayBeforeSaving!);
  _estimatedArrayBeforeSaving = null;
}

This documents the root cause and makes the defensive programming intent explicit.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between c555bbe and 0cc62fe.

📒 Files selected for processing (1)

• packages/dart/lib/src/objects/parse_array.dart (1 hunks)

🔇 Additional comments (1)

packages/dart/lib/src/objects/parse_array.dart (1)

69-73: Add a test to validate the fix.

The null-check logic correctly addresses the root cause and prevents _savedArray from being cleared when _estimatedArrayBeforeSaving is null. However, both the reviewer and maintainer explicitly requested adding a test to the repository to validate this fix and prevent regression.

Based on the PR comments, please add a test following the patterns in packages/dart/test. The test should:

• Simulate the scenario where onSaved() is called without a prior onSaving() call
• Verify that savedArray is not cleared/set to empty
• Fail before your fix and pass after

If you're having difficulty writing the test, consider these steps:

1. Look at existing tests in packages/dart/test directory for reference
2. Create a test that instantiates _ParseArray (or a Parse object with array fields)
3. Populate the array and manually call onSaved() without calling onSaving() first
4. Assert that the array data is preserved

[coderabbitai]

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5e28522 and baa71ab.

📒 Files selected for processing (1)

• packages/dart/test/src/objects/parse_object/parse_object_array_test.dart (1 hunks)

[mtrezza]

@nabos440 after several attempts I wasn't able to add a failing test that reproduces the issue you describe. Could you post a simple code example so we can reproduce the issue? The internal SDK flow that triggers the bug may be more complex than can be easily reproduced in my added unit test, so there root cause of the bug may be a different one and the fix in this PR may only mitigate a symptom but not patch the actual bug.