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
Conversation
Codecov Report
@@ Coverage Diff @@
## master #295 +/- ##
============================================
+ Coverage 72.98% 73.00% +0.02%
- Complexity 1044 1057 +13
============================================
Files 64 64
Lines 5526 5531 +5
Branches 645 644 -1
============================================
+ Hits 4033 4038 +5
Misses 1282 1282
Partials 211 211
Continue to review full report at Codecov.
|
@schmidt-sebastian ,all the changes have been addressed PTAL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good! Some nits.
@@ -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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
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`"))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
commit(update(new HashMap<String, Value>(), Arrays.asList("`a.b`.`c.d`"))); | |
commit(update(Collections.<String, Value>emptyMap(), Collections.singletonList("`a.b`.`c.d`"))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
Map<String, Object> dotKeyMap = new HashMap<>(); | ||
dotKeyMap.put("a.b", SINGLE_FILED_MAP_WITH_DOT); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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); |
There was a problem hiding this comment.
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"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add empty line above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
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")); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
🤖 I have created a release \*beep\* \*boop\* --- ### [1.35.2](https://www.github.com/googleapis/java-firestore/compare/v1.35.1...v1.35.2) (2020-07-16) ### Bug Fixes * add Internal#autoId() ([#292](https://www.github.com/googleapis/java-firestore/issues/292)) ([b91c57c](https://www.github.com/googleapis/java-firestore/commit/b91c57c4b2d3e92478ceaa1a39d467c40e1344dc)) * add support for deleting nested fields that contain periods ([#295](https://www.github.com/googleapis/java-firestore/issues/295)) ([84f602e](https://www.github.com/googleapis/java-firestore/commit/84f602ef8be67e5748b77e549d46ea53d0c74335)) * use test credentials when connecting to the Emulator from the Firebase Admin SDK ([#296](https://www.github.com/googleapis/java-firestore/issues/296)) ([a0a6e80](https://www.github.com/googleapis/java-firestore/commit/a0a6e806217693fc62a4cf432354c76e719aa140)) ### Dependencies * update dependency com.google.cloud:google-cloud-shared-dependencies to v0.8.3 ([#289](https://www.github.com/googleapis/java-firestore/issues/289)) ([2ddb8f1](https://www.github.com/googleapis/java-firestore/commit/2ddb8f133dd3bf31d28bf6bd67cddf8ba2e8846b)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please).
Fixes #288