Change DocumentOverlayCache.saveOverlays() to require non-null values in the given Map #3518
Conversation
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
… in the given Map
Coverage Report 1Affected Products
Test Logs
Notes |
Size Report 1Affected Products
Test Logs
Notes |
dconeybe
added a commit
to firebase/firebase-js-sdk
that referenced
this pull request
Mar 10, 2022
ehsannas
reviewed
Mar 10, 2022
| if (mutation == null) { | ||
| return; | ||
| } | ||
|
|
There was a problem hiding this comment.
we should assert mutation != null, since mutation.getKey() is being used below.
There was a problem hiding this comment.
or does removing the @Nullable achieve the same thing?
There was a problem hiding this comment.
I added an explicit null check in saveOverlays(). @nullable is just a compile-time check and doesn't affect runtime behavior. I also added a unit test.
…uming that they are not null
ehsannas
approved these changes
Mar 10, 2022
…putations on the happy path
…dependent path META-INF/DEPENDENCIES` build error
…th OS independent path META-INF/DEPENDENCIES` build error" This reverts commit f141041.
jeremyjiang-dev
pushed a commit
that referenced
this pull request
Mar 24, 2022
… in the given Map (#3518)
jeremyjiang-dev
pushed a commit
that referenced
this pull request
Mar 25, 2022
… in the given Map (#3518)
jeremyjiang-dev
pushed a commit
that referenced
this pull request
Mar 25, 2022
… in the given Map (#3518)
jeremyjiang-dev
pushed a commit
that referenced
this pull request
Mar 25, 2022
… in the given Map (#3518)
ehsannas
added a commit
to firebase/firebase-js-sdk
that referenced
this pull request
Apr 6, 2022
* Integrate Document Overlay with the SDK. * we should call ObjectValue.delete if value is null. * Remove unnecessary null check from MemoryDocumentOverlayCache.saveOverlay(), like is done in firebase/firebase-android-sdk#3518 * Address comments. * Port changes from Android SDK PR#3420. Note that we are not going to do any processing in the background. * Port overlay recalculation bug (Android SDK PR #3495). * Fix overlay bug when offline (Port Android SDK PR #3537). * Address feedback. * Better null check. Co-authored-by: Denver Coneybeare <[email protected]>
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
The current implementations of
DocumentOverlayCache.saveOverlays()checks that each value in the givenMapis non-null, and if it is null it ignores it, as if the key/value pair wasn't even present in the map.This PR changes the logic to assume that none of the values in the
Mapare null and modifies the call sites to ensure that they avoid putting null values into the map.This is a slightly cleaner implementation and makes the null behavior more explicit at the call site.