DeferredValue changes for ServerValue.increment() #1415
Conversation
Binary Size ReportAffected SDKs
Test Logs |
Coverage ReportAffected SDKs
Test LogsNotesHTML coverage reports can be produced locally with./gradlew <product>:checkCoverage. Report files are located at <product-build-dir>/reports/jacoco/.
|
|||||||||||||||||||||||||||||||||||||||
| public void testServerIncrementOverwritesExistingData() | ||
| public void testServerIncrementOverwritesExistingDataOnline() | ||
| throws DatabaseException, TimeoutException, InterruptedException { | ||
| serverIncrementOverwritesExistingData(true); |
There was a problem hiding this comment.
nit: boolean values should typically be named. E.g. serverIncrementOverwritesExistingData(/*online*/ true);
There was a problem hiding this comment.
Done. Android Studio does this already implicitly, but it still makes sense to do until GitHub catches up.
| import java.util.ArrayList; | ||
|
|
||
| /** A DeferredValueProvider computes the value of a Node only when {@link #node()} is invoked. */ | ||
| public class DeferredValueProvider implements ValueProvider { |
There was a problem hiding this comment.
You could reduce the number of files that people need to read if you have
ValueProvider
ValueProvider::Deferred
ValueProvider::Existing
IDR if that requires you to make an abstract class rather than an interface though.
There was a problem hiding this comment.
It does, but I think it is still a good idea - especially since these classes are so small.
| if (!value.equals(data.getValue()) || !priority.equals(data.getPriority())) { | ||
| return NodeUtilities.NodeFromJSON(value, priority); | ||
| Object value = resolveDeferredLeafValue(data.getValue(), existing, serverValues); | ||
| if (!value.equals(data.getValue()) || !Utilities.equals(priority, rawPriority)) { |
There was a problem hiding this comment.
Oof... I hate files and classes called utilities
| } else { | ||
| return holder.getRootNode(); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| public static CompoundWrite resolveDeferredValueMerge( | ||
| CompoundWrite merge, Node existing, final Map<String, Object> serverValues) { | ||
| CompoundWrite merge, SyncTree syncTree, Path path, final Map<String, Object> serverValues) { | ||
| CompoundWrite write = CompoundWrite.emptyWrite(); |
There was a problem hiding this comment.
Interesting. In iOS this starts as merge and is modified with addWrite only where the resolved value changes. Is this a pessimisation?
There was a problem hiding this comment.
I tried to make it match iOS just now but it broke a bunch of tests.
| @@ -249,12 +249,12 @@ public boolean isEmpty() { | |||
| if (write.isOverwrite()) { | |||
| Node resolvedNode = | |||
| ServerValues.resolveDeferredValueSnapshot( | |||
| write.getOverwrite(), existing, serverValues); | |||
There was a problem hiding this comment.
I think you can delete "existing" above.
There was a problem hiding this comment.
Good catch. This also removed:
ArrayList<Long> excludeThis = new ArrayList<>();
excludeThis.add(write.getWriteId());
calcCompleteEventCache(write.getPath(), excludeThis);
That's a bit suspicious, but this never existed on iOS.
Ports firebase/firebase-ios-sdk@aa67e19