Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(firebase_database): Firebase_Database Assertion error on push for sorted FirebaseAnimatedList #9838

Closed
wants to merge 4 commits into from

Conversation

dipakp2726
Copy link

@dipakp2726 dipakp2726 commented Nov 1, 2022

Description

fixed OnChildAdded so its get the correct index when new key is pushed.

Related Issues

fixes #7100,#9837

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]).
This will ensure a smooth and quick review process. Updating the pubspec.yaml and changelogs is not required.

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • My PR includes unit or integration tests for all changed/updated/fixed behaviors (See Contributor Guide).
  • All existing and new tests are passing.
  • I updated/added relevant documentation (doc comments with ///).
  • The analyzer (melos run analyze) does not report any problems on my PR.
  • I read and followed the Flutter Style Guide.
  • I signed the CLA.
  • I am willing to follow-up on review comments in a timely manner.

Breaking Change

Does your PR require plugin users to manually update their apps to accommodate your change?

  • Yes, this is a breaking change.
  • No, this is not a breaking change.

@google-cla
Copy link

google-cla bot commented Nov 1, 2022

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

_snapshots.sort(comparator);
onChildAdded!(_snapshots.indexOf(event.snapshot), event.snapshot);
Copy link
Author

@dipakp2726 dipakp2726 Nov 1, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it gives index always -1 when sorting is there

@dipakp2726
Copy link
Author

dipakp2726 commented Nov 6, 2022

can some one give a reply/acknowledgment here?

@russellwheatley

@dipakp2726
Copy link
Author

both failed checks are irrelevant to firebase_database

@russellwheatley
Copy link
Member

@dipakp2726 Could you merge with master locally and push to remote, please?

@dipakp2726
Copy link
Author

@dipakp2726 Could you merge with master locally and push to remote, please?

merged

@dipakp2726
Copy link
Author

https://github.com/dipakp2726/firebase_database_reproduce
on main -> branch it throws assertion error which use latest firebase database
solution branch -> which uses firebase_database from my fork doesn't throw assertion error

@dipakp2726
Copy link
Author

test for sorted list doesn't exists

@dipakp2726
Copy link
Author

looking at the existing tests, I am not sure if I can write a test that can produce this since there is a lot of mocking going on that I am not able to understand.
I can write an integration test that can produce this but I think it's not suitable since it will require a firebase project to open always,

can you give me a suggestion regarding the writing test that produces it?
@Lyokone

@Lyokone
Copy link
Contributor

Lyokone commented Nov 10, 2022

@dipakp2726 You can find the e2e test (that open a firebase simulator in CI) here: tests/integration_test/firebase_database/database_e2e.dart

Looking at the issue in depth, its caused by this line:

packages/firebase_database/firebase_database/lib/src/database_event.dart:20

DataSnapshot get snapshot => DataSnapshot._(_delegate.snapshot);

Where we don't cache the result of the snapshot.

I would modify this file with the content of #9899

Can you try this PR to see if you see the expected results on your end?

@dipakp2726
Copy link
Author

dipakp2726 commented Nov 10, 2022

@Lyokone #9899 resolves the issue,

@dipakp2726
Copy link
Author

what's next? @Lyokone

@Lyokone
Copy link
Contributor

Lyokone commented Nov 14, 2022

@dipakp2726 the #9899 is being reviewed, once it has been approved it will be merged and we can close this PR as well

@dipakp2726
Copy link
Author

thank you

@dipakp2726 dipakp2726 closed this Nov 15, 2022
@firebase firebase locked and limited conversation to collaborators Dec 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

🐛 Firebase_Database Assertion error on push for sorted FirebaseAnimatedList
3 participants