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: add support for deleting nested fields that contain periods #295

Merged
merged 3 commits into from Jul 16, 2020
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Expand Up @@ -54,11 +54,25 @@ B getParent() {
* @param path A relative path
*/
B append(String path) {
return append(path, true);
}

/**
* Returns a new path pointing to a child of this Path.
*
* @param path A relative path
* @param splitPath Whether or not the path should be split
*/
B append(String path, boolean splitPath) {
Preconditions.checkArgument(
path != null && !path.isEmpty(), "'path' must be a non-empty String");
ImmutableList.Builder<String> components = ImmutableList.builder();
components.addAll(this.getSegments());
components.add(splitChildPath(path));
if (splitPath) {
components.add(splitChildPath(path));
} else {
components.add(path);
}
return createPathWithSegments(components.build());
}

Expand Down
Expand Up @@ -160,7 +160,8 @@ static Value encodeValue(
Map<String, Object> map = (Map<String, Object>) sanitizedObject;

for (Map.Entry<String, Object> entry : map.entrySet()) {
Value encodedValue = encodeValue(path.append(entry.getKey()), entry.getValue(), options);
Value encodedValue =
encodeValue(path.append(entry.getKey(), false), entry.getValue(), options);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
encodeValue(path.append(entry.getKey(), false), entry.getValue(), options);
encodeValue(path.append(entry.getKey(), /* splitPath= */ false), entry.getValue(), options);

I generally prefer these inline comments for non-descriptive values.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

if (encodedValue != null) {
res.putFields(entry.getKey(), encodedValue);
}
Expand Down
Expand Up @@ -1069,4 +1069,17 @@ public void updateIndividualPojo() throws ExecutionException, InterruptedExcepti
assertCommitEquals(expectedCommit, request);
}
}

@Test
public void deleteNestedFieldUsingFieldPath() throws Exception {
doReturn(SINGLE_WRITE_COMMIT_RESPONSE)
.when(firestoreMock)
.sendRequest(
commitCapture.capture(), Matchers.<UnaryCallable<CommitRequest, CommitResponse>>any());
FieldPath path = FieldPath.of("a.b", "c.d");
documentReference.update(path, FieldValue.delete()).get();
CommitRequest expectedCommit =
commit(update(new HashMap<String, Value>(), Arrays.asList("`a.b`.`c.d`")));
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
commit(update(new HashMap<String, Value>(), Arrays.asList("`a.b`.`c.d`")));
commit(update(Collections.<String, Value>emptyMap(), Collections.singletonList("`a.b`.`c.d`")));

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

assertEquals(expectedCommit, commitCapture.getValue());
}
}
Expand Up @@ -93,6 +93,7 @@ public final class LocalFirestoreHelper {
public static final Map<String, Value> EMPTY_MAP_PROTO;

public static final Map<String, Object> SINGLE_FIELD_MAP;
public static final Map<String, Object> SINGLE_FILED_MAP_WITH_DOT;
public static final SingleField SINGLE_FIELD_OBJECT;
public static final Map<String, Value> SINGLE_FIELD_PROTO;
public static final DocumentSnapshot SINGLE_FIELD_SNAPSHOT;
Expand Down Expand Up @@ -723,6 +724,7 @@ public boolean equals(Object o) {
map("inner", Value.newBuilder().setMapValue(MapValue.getDefaultInstance()).build());

SINGLE_FIELD_MAP = map("foo", (Object) "bar");
SINGLE_FILED_MAP_WITH_DOT = map("c.d", (Object) "bar");
SINGLE_FIELD_OBJECT = new SingleField();
SINGLE_FIELD_PROTO = map("foo", Value.newBuilder().setStringValue("bar").build());
UPDATED_POJO_PROTO =
Expand Down
Expand Up @@ -87,6 +87,8 @@ public class ITSystemTest {
private final SingleField SINGLE_FIELD_OBJECT = LocalFirestoreHelper.SINGLE_FIELD_OBJECT;
private final AllSupportedTypes ALL_SUPPORTED_TYPES_OBJECT =
LocalFirestoreHelper.ALL_SUPPORTED_TYPES_OBJECT;
private final Map<String, Object> SINGLE_FILED_MAP_WITH_DOT =
LocalFirestoreHelper.SINGLE_FILED_MAP_WITH_DOT;

@Rule public TestName testName = new TestName();

Expand Down Expand Up @@ -1269,4 +1271,26 @@ public void testInstanceReturnedByGetServiceCanBeUsedToDeserializeAQuery() throw
fs.close();
Query.fromProto(fs, proto);
}

@Test
public void deleteNestedFieldUsingFieldPath() throws Exception {
DocumentReference documentReference = randomColl.document("doc1");
Map<String, Object> dotKeyMap = new HashMap<>();
dotKeyMap.put("a.b", SINGLE_FILED_MAP_WITH_DOT);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Map<String, Object> dotKeyMap = new HashMap<>();
dotKeyMap.put("a.b", SINGLE_FILED_MAP_WITH_DOT);
Map<String, Object> dotKeyMap = map(""a.b"", SINGLE_FILED_MAP_WITH_DOT);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

documentReference.set(dotKeyMap).get();
DocumentSnapshot documentSnapshots = documentReference.get().get();
assertEquals(SINGLE_FILED_MAP_WITH_DOT, documentSnapshots.getData().get("a.b"));
FieldPath path = FieldPath.of("a.b", "c.d");
Copy link
Contributor

Choose a reason for hiding this comment

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

Add empty line above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

documentReference.update(path, FieldValue.delete()).get();
documentSnapshots = documentReference.get().get();
assertNull(documentSnapshots.getData().get("c.d"));
Map<String, Object> nestedMap = new HashMap<>();
nestedMap.put("e", SINGLE_FILED_MAP_WITH_DOT);
dotKeyMap.put("a.b", nestedMap);
documentReference.set(dotKeyMap).get();
path = FieldPath.of("a.b", "e", "c.d");
documentReference.update(path, FieldValue.delete()).get();
documentSnapshots = documentReference.get().get();
assertNull(documentSnapshots.getData().get("c.d"));
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you can delete this part as it doesn't expand on the tests coverage.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

}
}