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.

Sign up for GitHub

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

Jump to bottom

Fix filtered get() cache #6497

Merged
merged 20 commits into from
Aug 12, 2022
Merged

Fix filtered get() cache #6497

merged 20 commits into from
Aug 12, 2022

Conversation

maneesht
Copy link
Contributor

@maneesht maneesht commented Aug 2, 2022

Fixed issue where get() would return incorrect results if you had a filtered get() and an event listener onValue() on the same path, and get() would override the data received by the push received by onValue() yielding incorrect results.

The fix below replicates the logic behind onValue onlyOnce by adding the new event to the SyncTree with the new node, propagating events, and then removing the event from the tree.

@maneesht maneesht requested a review from jsdt as a code owner August 2, 2022 20:44
@changeset-bot
Copy link

changeset-bot bot commented Aug 2, 2022 •
edited
Loading

🦋 Changeset detected

Latest commit: b4c6eb6

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages
Name Type
@firebase/database Patch
@firebase/database-compat Patch
firebase Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@maneesht
Copy link
Contributor Author

maneesht commented Aug 2, 2022

This should also fix #6397

@maneesht maneesht requested a review from jmwski August 2, 2022 20:47
@google-oss-bot
Copy link
Contributor

google-oss-bot commented Aug 2, 2022 •
edited
Loading

Size Report 1

Affected Products

  • @firebase/app

    TypeBase (6a17eb6)Merge (c1df113)Diff
    browser13.9 kB14.1 kB+204 B (+1.5%)
    esm518.1 kB18.4 kB+246 B (+1.4%)
    main19.1 kB19.3 kB+242 B (+1.3%)
    module13.9 kB14.1 kB+204 B (+1.5%)

  • @firebase/database

    TypeBase (6a17eb6)Merge (c1df113)Diff
    browser248 kB248 kB-648 B (-0.3%)
    esm5276 kB276 kB-574 B (-0.2%)
    main282 kB281 kB-574 B (-0.2%)
    module248 kB248 kB-648 B (-0.3%)

  • @firebase/database-compat/standalone

    TypeBase (6a17eb6)Merge (c1df113)Diff
    main371 kB370 kB-574 B (-0.2%)

  • @firebase/firestore

    TypeBase (6a17eb6)Merge (c1df113)Diff
    browser264 kB264 kB+169 B (+0.1%)
    esm5327 kB327 kB+294 B (+0.1%)
    main525 kB525 kB+472 B (+0.1%)
    module264 kB264 kB+169 B (+0.1%)
    react-native264 kB264 kB+169 B (+0.1%)

  • @firebase/firestore-lite

    TypeBase (6a17eb6)Merge (c1df113)Diff
    browser80.5 kB80.7 kB+172 B (+0.2%)
    esm596.2 kB96.5 kB+297 B (+0.3%)
    main135 kB136 kB+446 B (+0.3%)
    module80.5 kB80.7 kB+172 B (+0.2%)
    react-native80.7 kB80.9 kB+172 B (+0.2%)

  • bundle

    43 size changes

    TypeBase (6a17eb6)Merge (c1df113)Diff
    analytics (logEvent)41.8 kB41.9 kB+110 B (+0.3%)
    app-check (CustomProvider)35.4 kB35.5 kB+110 B (+0.3%)
    app-check (ReCaptchaEnterpriseProvider)37.6 kB37.7 kB+110 B (+0.3%)
    app-check (ReCaptchaV3Provider)37.6 kB37.7 kB+110 B (+0.3%)
    auth (Anonymous)66.3 kB66.4 kB+110 B (+0.2%)
    auth (EmailAndPassword)70.4 kB70.5 kB+110 B (+0.2%)
    auth (GoogleFBTwitterGitHubPopup)90.3 kB90.4 kB+110 B (+0.1%)
    auth (GooglePopup)90.0 kB90.1 kB+110 B (+0.1%)
    auth (GoogleRedirect)90.2 kB90.3 kB+110 B (+0.1%)
    auth (Phone)76.5 kB76.6 kB+110 B (+0.1%)
    database (Append to a list of data)145 kB145 kB+110 B (+0.1%)
    database (Filtering data)144 kB144 kB+110 B (+0.1%)
    database (Listen for child events)160 kB160 kB-120 B (-0.1%)
    database (Listen for value events + Detach listeners)160 kB160 kB-120 B (-0.1%)
    database (Listen for value events)160 kB160 kB-120 B (-0.1%)
    database (Read data once)156 kB159 kB+2.77 kB (+1.8%)
    database (Save data as transactions)162 kB162 kB-120 B (-0.1%)
    database (Sort data)145 kB145 kB+110 B (+0.1%)
    database (Write data)144 kB144 kB+110 B (+0.1%)
    firestore (Persistence)274 kB274 kB+280 B (+0.1%)
    firestore (Query Cursors)211 kB211 kB+273 B (+0.1%)
    firestore (Query)212 kB212 kB+273 B (+0.1%)
    firestore (Read data once)200 kB200 kB+273 B (+0.1%)
    firestore (Realtime updates)202 kB203 kB+273 B (+0.1%)
    firestore (Transaction)184 kB184 kB+273 B (+0.1%)
    firestore (Write data)184 kB184 kB+273 B (+0.1%)
    firestore-lite (Query Cursors)67.9 kB68.2 kB+282 B (+0.4%)
    firestore-lite (Query)71.1 kB71.4 kB+282 B (+0.4%)
    firestore-lite (Read data once)55.5 kB55.8 kB+279 B (+0.5%)
    firestore-lite (Transaction)80.1 kB80.3 kB+282 B (+0.4%)
    firestore-lite (Write data)65.3 kB65.5 kB+282 B (+0.4%)
    functions (call)29.2 kB29.3 kB+110 B (+0.4%)
    messaging (send + receive)45.1 kB45.3 kB+110 B (+0.2%)
    performance (trace)49.6 kB49.7 kB+110 B (+0.2%)
    remote-config (getAndFetch)44.2 kB44.3 kB+110 B (+0.2%)
    storage (getBytes)37.5 kB37.6 kB+110 B (+0.3%)
    storage (getDownloadURL)39.6 kB39.7 kB+110 B (+0.3%)
    storage (getMetadata)39.0 kB39.1 kB+110 B (+0.3%)
    storage (list + listAll)38.4 kB38.6 kB+110 B (+0.3%)
    storage (updateMetadata)39.3 kB39.4 kB+110 B (+0.3%)
    storage (uploadBytes)43.8 kB43.9 kB+110 B (+0.3%)
    storage (uploadBytesResumable)53.3 kB53.4 kB+110 B (+0.2%)
    storage (uploadString)44.0 kB44.2 kB+110 B (+0.2%)

  • firebase

    TypeBase (6a17eb6)Merge (c1df113)Diff
    firebase-app-compat.js27.7 kB27.8 kB+90 B (+0.3%)
    firebase-app.js87.4 kB87.7 kB+334 B (+0.4%)
    firebase-compat.js794 kB794 kB-147 B (-0.0%)
    firebase-database-compat.js166 kB166 kB-413 B (-0.2%)
    firebase-database.js606 kB606 kB-397 B (-0.1%)
    firebase-firestore-compat.js314 kB314 kB+170 B (+0.1%)
    firebase-firestore-lite.js267 kB267 kB+399 B (+0.1%)
    firebase-firestore.js851 kB852 kB+396 B (+0.0%)
    firebase-performance-standalone-compat.es2017.js87.5 kB87.6 kB+104 B (+0.1%)
    firebase-performance-standalone-compat.js65.3 kB65.4 kB+104 B (+0.2%)

Test Logs

  1. https://storage.googleapis.com/firebase-sdk-metric-reports/OtGBl7cXsp.html

@google-oss-bot
Copy link
Contributor

google-oss-bot commented Aug 2, 2022 •
edited
Loading

Size Analysis Report 1

This report is too large (234,696 characters) to be displayed here in a GitHub comment. Please use the below link to see the full report on Google Cloud Storage.

Test Logs

  1. https://storage.googleapis.com/firebase-sdk-metric-reports/A96O2DfKaj.html

jmwski
jmwski reviewed Aug 2, 2022
View reviewed changes
packages/database/src/core/Repo.ts Outdated Show resolved Hide resolved
packages/database/src/core/Repo.ts Show resolved Hide resolved
packages/database/src/core/SyncTree.ts Outdated Show resolved Hide resolved
packages/database/src/core/SyncTree.ts Outdated Show resolved Hide resolved
packages/database/src/core/SyncTree.ts Outdated Show resolved Hide resolved
packages/database/src/core/Repo.ts Outdated Show resolved Hide resolved
packages/database/test/exp/integration.test.ts Show resolved Hide resolved
packages/database/test/exp/integration.test.ts Outdated Show resolved Hide resolved
jmwski
jmwski approved these changes Aug 3, 2022
View reviewed changes
Copy link
Contributor

@jmwski jmwski left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

@maneesht maneesht assigned jsdt and unassigned jmwski Aug 4, 2022
jsdt
jsdt reviewed Aug 11, 2022
View reviewed changes
packages/database/src/core/SyncTree.ts
@@ -324,7 +324,8 @@ export function syncTreeRemoveEventRegistration(
syncTree: SyncTree,
query: QueryContext,
eventRegistration: EventRegistration | null,
cancelError?: Error
cancelError?: Error,
skipListenerDedup = false
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you explain what skipListenerDedup means in the function comments?

packages/database/src/core/SyncTree.ts Outdated
);
}
} else {
// There's nothing below us, so nothing we need to start listening on
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this an else block with no code in it? If so, why not delete it?

@maneesht maneesht merged commit a5d9e10 into master Aug 12, 2022
@maneesht maneesht deleted the mtewani/get-add-synctree-fix branch August 12, 2022 00:13
@google-oss-bot google-oss-bot mentioned this pull request Aug 16, 2022
Merged
@firebase firebase locked and limited conversation to collaborators Sep 11, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jsdt jsdt jsdt approved these changes

@jmwski jmwski jmwski approved these changes

@hsubox76 hsubox76 hsubox76 approved these changes

@egilmorez egilmorez Awaiting requested review from egilmorez

Assignees

@jsdt jsdt

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@maneesht @google-oss-bot @jsdt @jmwski @hsubox76