Skip to content

Commit 47c659c

Browse files
schmidt-sebastianschmidt-sebastian
committed
Review comments
1 parent 7bf5e40 commit 47c659c

File tree

4 files changed

+152
-166
lines changed

4 files changed

+152
-166
lines changed

‎firebase-firestore/src/main/java/com/google/firebase/firestore/model/protovalue/ObjectValue.java

Lines changed: 86 additions & 115 deletions
Original file line numberDiff line numberDiff line change
@@ -23,12 +23,10 @@
2323
import com.google.firebase.firestore.model.value.FieldValue;
2424
import com.google.firestore.v1.MapValue;
2525
import com.google.firestore.v1.Value;
26+
import java.util.HashMap;
2627
import java.util.HashSet;
27-
import java.util.Iterator;
2828
import java.util.Map;
2929
import java.util.Set;
30-
import java.util.SortedMap;
31-
import java.util.TreeMap;
3230

3331
public class ObjectValue extends PrimitiveValue {
3432
private static final ObjectValue EMPTY_VALUE =
@@ -37,13 +35,18 @@ public class ObjectValue extends PrimitiveValue {
3735
.setMapValue(com.google.firestore.v1.MapValue.getDefaultInstance())
3836
.build());
3937

38+
public ObjectValue(Value value) {
39+
super(value);
40+
hardAssert(isType(value, TYPE_ORDER_OBJECT), "ObjectValues must be backed by a MapValue");
41+
}
42+
4043
public static ObjectValue emptyObject() {
4144
return EMPTY_VALUE;
4245
}
4346

44-
public ObjectValue(Value value) {
45-
super(value);
46-
hardAssert(isType(value, TYPE_ORDER_OBJECT), "ObjectValues must be backed by a MapValue");
47+
/** Returns a new Builder instance that is based on an empty object. */
48+
public static ObjectValue.Builder newBuilder() {
49+
return EMPTY_VALUE.toBuilder();
4750
}
4851

4952
/**
@@ -53,17 +56,19 @@ public ObjectValue(Value value) {
5356
* @return The value at the path or if there it doesn't exist.
5457
*/
5558
public @Nullable FieldValue get(FieldPath fieldPath) {
56-
Value value = internalValue;
57-
58-
for (int i = 0; i < fieldPath.length() - 1; ++i) {
59-
value = value.getMapValue().getFieldsOrDefault(fieldPath.getSegment(i), null);
60-
if (!isType(value, TYPE_ORDER_OBJECT)) {
61-
return null;
59+
if (fieldPath.isEmpty()) {
60+
return this;
61+
} else {
62+
Value value = internalValue;
63+
for (int i = 0; i < fieldPath.length() - 1; ++i) {
64+
value = value.getMapValue().getFieldsOrDefault(fieldPath.getSegment(i), null);
65+
if (!isType(value, TYPE_ORDER_OBJECT)) {
66+
return null;
67+
}
6268
}
69+
value = value.getMapValue().getFieldsOrDefault(fieldPath.getLastSegment(), null);
70+
return value != null ? FieldValue.of(value) : null;
6371
}
64-
65-
value = value.getMapValue().getFieldsOrDefault(fieldPath.getLastSegment(), null);
66-
return value != null ? FieldValue.of(value) : null;
6772
}
6873

6974
/** Recursively extracts the FieldPaths that are set in this ObjectValue. */
@@ -106,15 +111,15 @@ public static class Builder {
106111
private ObjectValue baseObject;
107112

108113
/**
109-
* A list of FieldPath/Value pairs to apply to the base object. `null` values indicate field
110-
* deletes. MapValues are expanded before they are stored in the overlay map, so that an entry
111-
* exists for each leaf node.
114+
* A nested map that contains the accumulated changes in this builder. Values can either be
115+
* `Value` protos, `Map<String, Object>` values (to represent additional nesting) or `null`
116+
* (to represent field deletes).
112117
*/
113-
private SortedMap<FieldPath, Value> overlayMap;
118+
private Map<String, Object> overlayMap;
114119

115120
Builder(ObjectValue baseObject) {
116121
this.baseObject = baseObject;
117-
this.overlayMap = new TreeMap<>();
122+
this.overlayMap = new HashMap<>();
118123
}
119124

120125
/**
@@ -126,7 +131,6 @@ public static class Builder {
126131
*/
127132
public Builder set(FieldPath path, Value value) {
128133
hardAssert(!path.isEmpty(), "Cannot set field for empty path on ObjectValue");
129-
removeConflictingOverlays(path);
130134
setOverlay(path, value);
131135
return this;
132136
}
@@ -140,127 +144,94 @@ public Builder set(FieldPath path, Value value) {
140144
*/
141145
public Builder delete(FieldPath path) {
142146
hardAssert(!path.isEmpty(), "Cannot delete field for empty path on ObjectValue");
143-
removeConflictingOverlays(path);
144147
setOverlay(path, null);
145148
return this;
146149
}
147150

148-
/** Remove any existing overlays that would be replaced by setting `path` to a new value. */
149-
private void removeConflictingOverlays(FieldPath path) {
150-
Iterator<FieldPath> iterator =
151-
overlayMap.subMap(path, createSuccessor(path)).keySet().iterator();
152-
while (iterator.hasNext()) {
153-
iterator.next();
154-
iterator.remove();
155-
}
156-
}
157-
158-
/**
159-
* Adds `value` to the overlay map at `path`. MapValues are recursively expanded into one
160-
* overlay per leaf node.
161-
*/
151+
/** Adds `value` to the overlay map at `path`. Creates nested Map entries if needed. */
162152
private void setOverlay(FieldPath path, @Nullable Value value) {
163-
if (!isType(value, TYPE_ORDER_OBJECT) || value.getMapValue().getFieldsCount() == 0) {
164-
overlayMap.put(path, value);
165-
} else {
166-
for (Map.Entry<String, Value> entry : value.getMapValue().getFieldsMap().entrySet()) {
167-
setOverlay(path.append(entry.getKey()), entry.getValue());
153+
Map<String, Object> currentLevel = overlayMap;
154+
155+
for (int i = 0; i < path.length() - 1; ++i) {
156+
String currentSegment = path.getSegment(i);
157+
Object currentValue = currentLevel.get(currentSegment);
158+
159+
if (currentValue instanceof Map) {
160+
// Re-use a previously created Map
161+
currentLevel = (Map<String, Object>) currentValue;
162+
} else if (currentValue instanceof Value
163+
&& ((Value) currentValue).getValueTypeCase() == Value.ValueTypeCase.MAP_VALUE) {
164+
// Convert the existing Protobuf MapValue into a Java map
165+
Map<String, Object> nextLevel =
166+
new HashMap<>(((Value) currentValue).getMapValue().getFieldsMap());
167+
currentLevel.put(currentSegment, nextLevel);
168+
currentLevel = nextLevel;
169+
} else {
170+
// Create an empty hash map to represent the current nesting level
171+
Map<String, Object> nextLevel = new HashMap<>();
172+
currentLevel.put(currentSegment, nextLevel);
173+
currentLevel = nextLevel;
168174
}
169175
}
176+
177+
currentLevel.put(path.getLastSegment(), value);
170178
}
171179

172180
/** Returns an ObjectValue with all mutations applied. */
173181
public ObjectValue build() {
174-
if (overlayMap.isEmpty()) {
175-
return baseObject;
182+
MapValue mergedResult = applyOverlay(FieldPath.EMPTY_PATH, overlayMap);
183+
if (mergedResult != null) {
184+
return new ObjectValue(Value.newBuilder().setMapValue(mergedResult).build());
176185
} else {
177-
MapValue.Builder result = baseObject.internalValue.getMapValue().toBuilder();
178-
applyOverlay(FieldPath.EMPTY_PATH, result);
179-
return new ObjectValue(Value.newBuilder().setMapValue(result).build());
186+
return this.baseObject;
180187
}
181188
}
182189

183190
/**
184-
* Applies any overlays from `overlayMap` that exist at `currentPath` to the `resultAtPath` map.
185-
* Overlays are expanded recursively based on their location in the backing ObjectValue's
186-
* subtree and are processed by nesting level.
187-
*
188-
* <p>Example: Overlays { 'a.b.c' : 'foo', 'a.b.d' : 'bar', 'a.e' : 'foobar' }
189-
*
190-
* <p>To apply these overlays, the methods first creates a MapValue.Builder for `a`. It then
191-
* calls applyOverlay() with a current path of `a` and the newly created MapValue.Builder. In
192-
* its second call, `applyOverlay` assigns `a.b` to a new MapBuilder and `a.e` to 'foobar'. The
193-
* third call assigns `a.b.c` and `a.b.d` to the MapValue.Builder created in the second step.
194-
*
195-
* <p>The overall aim of this method is to minimize conversions between MapValues and their
196-
* builders.
191+
* Applies any overlays from `currentOverlays` that exist at `currentPath` and returns the
192+
* merged data at `currentPath` (or `null` if there were no changes).
197193
*
198194
* @param currentPath The path at the current nesting level. Can be set toFieldValue.EMPTY_PATH
199195
* to represent the root.
200-
* @param resultAtPath A mutable copy of the existing data at the current nesting level.
201-
* Overlays are applied to this argument.
202-
* @return Whether any modifications were applied (in any part of the subtree under
203-
* currentPath).
196+
* @param currentOverlays The overlays at the current nesting level in the same format as
197+
* `overlayMap`.
198+
* @return The merged data at `currentPath` or null if no modifications were applied.
204199
*/
205-
private boolean applyOverlay(FieldPath currentPath, MapValue.Builder resultAtPath) {
206-
// Extract the data that exists at or below the current path. Te extracted subtree is
207-
// subdivided during each iteration. The iteration stops when the slice becomes empty.
208-
SortedMap<FieldPath, Value> currentSlice =
209-
currentPath.isEmpty()
210-
? overlayMap
211-
: overlayMap.subMap(currentPath, createSuccessor(currentPath));
212-
200+
private @Nullable MapValue applyOverlay(
201+
FieldPath currentPath, Map<String, Object> currentOverlays) {
213202
boolean modified = false;
214203

215-
while (!currentSlice.isEmpty()) {
216-
FieldPath fieldPath = currentSlice.firstKey();
217-
218-
if (fieldPath.length() == currentPath.length() + 1) {
219-
// The key in the slice is a leaf node. We can apply the value directly.
220-
String fieldName = fieldPath.getLastSegment();
221-
Value overlayValue = overlayMap.get(fieldPath);
222-
if (overlayValue != null) {
223-
resultAtPath.putFields(fieldName, overlayValue);
224-
modified = true;
225-
} else if (resultAtPath.containsFields(fieldName)) {
226-
resultAtPath.removeFields(fieldName);
204+
@Nullable FieldValue existingValue = baseObject.get(currentPath);
205+
MapValue.Builder resultAtPath =
206+
existingValue instanceof ObjectValue
207+
// If there is already data at the current path, base our modifications on top
208+
// of the existing data.
209+
? ((ObjectValue) existingValue).internalValue.getMapValue().toBuilder()
210+
: MapValue.newBuilder();
211+
212+
for (Map.Entry<String, Object> entry : currentOverlays.entrySet()) {
213+
String pathSegment = entry.getKey();
214+
Object value = entry.getValue();
215+
216+
if (value instanceof Map) {
217+
@Nullable
218+
MapValue nested =
219+
applyOverlay(currentPath.append(pathSegment), (Map<String, Object>) value);
220+
if (nested != null) {
221+
resultAtPath.putFields(pathSegment, Value.newBuilder().setMapValue(nested).build());
227222
modified = true;
228223
}
229-
} else {
230-
// Since we need a MapValue.Builder at each nesting level (e.g. to create the field for
231-
// `a.b.c` we need to create a MapValue.Builder for `a` as well as `a.b`), we invoke
232-
// applyOverlay() recursively with the next nesting level.
233-
FieldPath nextSliceStart = fieldPath.keepFirst(currentPath.length() + 1);
234-
@Nullable FieldValue existingValue = baseObject.get(nextSliceStart);
235-
MapValue.Builder nextSliceBuilder =
236-
existingValue instanceof ObjectValue
237-
// If there is already data at the current path, base our modifications on top
238-
// of the existing data.
239-
? ((ObjectValue) existingValue).internalValue.getMapValue().toBuilder()
240-
: MapValue.newBuilder();
241-
modified = applyOverlay(nextSliceStart, nextSliceBuilder) || modified;
242-
if (modified) {
243-
// Only apply the result if a field has been modified. This avoids adding an empty
244-
// map entry for deletes of non-existing fields.
245-
resultAtPath.putFields(
246-
nextSliceStart.getLastSegment(),
247-
Value.newBuilder().setMapValue(nextSliceBuilder).build());
248-
}
224+
} else if (value instanceof Value) {
225+
resultAtPath.putFields(pathSegment, (Value) value);
226+
modified = true;
227+
} else if (resultAtPath.containsFields(pathSegment)) {
228+
hardAssert(value == null, "Expected entry to be a Map, a Value or null");
229+
resultAtPath.removeFields(pathSegment);
230+
modified = true;
249231
}
250-
251-
// Shrink the subtree to contain only values after the current field path. Note that we are
252-
// still bound by the subtree created at the initial method invocation. The current loop
253-
// exits when the subtree becomes empty.
254-
currentSlice = currentSlice.tailMap(createSuccessor(fieldPath));
255232
}
256233

257-
return modified;
258-
}
259-
260-
/** Create the first field path that is not part of the subtree created by `currentPath`. */
261-
private FieldPath createSuccessor(FieldPath currentPath) {
262-
hardAssert(!currentPath.isEmpty(), "Can't create a successor for an empty path");
263-
return currentPath.popLast().append(currentPath.getLastSegment() + '0');
234+
return modified ? resultAtPath.build() : null;
264235
}
265236
}
266237
}

‎firebase-firestore/src/main/java/com/google/firebase/firestore/model/protovalue/PrimitiveValue.java

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,10 @@
3434
import java.util.List;
3535
import java.util.Map;
3636

37+
/**
38+
* Represents a FieldValue that is backed by a single Firestore V1 Value proto and uses Firestore's
39+
* Value semantics for ordering and equality.
40+
*/
3741
public class PrimitiveValue extends FieldValue {
3842
protected final Value internalValue;
3943

‎firebase-firestore/src/test/java/com/google/firebase/firestore/model/FieldValueTest.java

Lines changed: 13 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -223,21 +223,23 @@ public void testDeletesMultipleNewFields() {
223223
@Test
224224
public void testValueEquality() {
225225
new EqualsTester()
226-
.addEqualityGroup(wrap(true), wrap(true))
227-
.addEqualityGroup(wrap(false), wrap(false))
228-
.addEqualityGroup(wrap(null), wrap(null))
226+
.addEqualityGroup(wrap(true), wrap(valueOf(true)))
227+
.addEqualityGroup(wrap(false), wrap(valueOf(false)))
228+
.addEqualityGroup(wrap(null), wrap(valueOf(null)))
229229
.addEqualityGroup(
230-
wrap(0.0 / 0.0), wrap(Double.longBitsToDouble(0x7ff8000000000000L)), wrap(Double.NaN))
230+
wrap(0.0 / 0.0),
231+
wrap(Double.longBitsToDouble(0x7ff8000000000000L)),
232+
wrap(valueOf(Double.NaN)))
231233
// -0.0 and 0.0 compareTo the same but are not equal.
232234
.addEqualityGroup(wrap(-0.0))
233235
.addEqualityGroup(wrap(0.0))
234-
.addEqualityGroup(wrap(1), wrap(1))
236+
.addEqualityGroup(wrap(1), wrap(valueOf(1)))
235237
// Doubles and Longs aren't equal.
236-
.addEqualityGroup(wrap(1.0), wrap(1.0))
237-
.addEqualityGroup(wrap(1.1), wrap(1.1))
238-
.addEqualityGroup(wrap(blob(0, 1, 2)), wrap(blob(0, 1, 2)))
238+
.addEqualityGroup(wrap(1.0), wrap(valueOf(1.0)))
239+
.addEqualityGroup(wrap(1.1), wrap(valueOf(1.1)))
240+
.addEqualityGroup(wrap(blob(0, 1, 2)), wrap(valueOf(blob(0, 1, 2))))
239241
.addEqualityGroup(wrap(blob(0, 1)))
240-
.addEqualityGroup(wrap("string"), wrap("string"))
242+
.addEqualityGroup(wrap("string"), wrap(valueOf("string")))
241243
.addEqualityGroup(wrap("strin"))
242244
// latin small letter e + combining acute accent
243245
.addEqualityGroup(wrap("e\u0301b"))
@@ -383,8 +385,8 @@ private ObjectValue wrapObject(Object... entries) {
383385
return (ObjectValue) object;
384386
}
385387

386-
private PrimitiveValue wrap(Object map) {
387-
return (PrimitiveValue) FieldValue.of(valueOf(map));
388+
private PrimitiveValue wrap(Object value) {
389+
return (PrimitiveValue) FieldValue.of(valueOf(value));
388390
}
389391

390392
private PrimitiveValue wrapRef(DatabaseId dbId, DocumentKey key) {

0 commit comments

Comments
 (0)