Skip to content

Firestore: Fix FieldValue.arrayUnion() accept POJO. - #5750

Merged
BenWhitehead merged 4 commits into
googleapis:masterfrom
pmakani:api-firestore-5622
Aug 1, 2019
Merged

Firestore: Fix FieldValue.arrayUnion() accept POJO.#5750
BenWhitehead merged 4 commits into
googleapis:masterfrom
pmakani:api-firestore-5622

Conversation

@pmakani

Copy link
Copy Markdown

Fixes#5622

@googlebotgooglebot added the cla: yes This human has signed the Contributor License Agreement. label Jul 16, 2019
@pmakanipmakani added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 18, 2019
@yoshi-kokoroyoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 18, 2019
@BenWhitehead

Copy link
Copy Markdown
Contributor

This looks like it's okay to me. What do you think @schmidt-sebastian?

@schmidt-sebastianschmidt-sebastian self-assigned this Jul 28, 2019

@schmidt-sebastianschmidt-sebastian left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for this! I left some feedback if you want to take this PR for another spin.

ArrayValue.Builder encodedElements = ArrayValue.newBuilder();

for (Object element : elements) {
Object data = CustomClassMapper.convertToPlainJavaTypes(element);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can you make the same change for ArrayRemoveFieldValue?

assertEquals(Timestamp.ofTimeSecondsAndNanos(0, 0), writeResult.getUpdateTime());
}

private static class TestPoJo {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Would it be possible to just use

or ?

If not, can you move this class to LocalFirestoreHelper?

ImmutableList.<Map<String, String>>of(
ImmutableMap.<String, String>of("test", "test-value"))));
WriteResult writeResult = doc.update("foo", fieldValue).get();
assertEquals(Timestamp.ofTimeSecondsAndNanos(0, 0), writeResult.getUpdateTime());

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I know it would be a lot more code, but to me this test would be more valuable if it verified the generated proto.

@codecov

codecovBot commented Jul 31, 2019

Copy link
Copy Markdown

Codecov Report

Merging #5750 into master will decrease coverage by 0.29%.
The diff coverage is 100%.

Impacted file tree graph

@@ Coverage Diff @@## master #5750 +/- ##
===========================================
- Coverage 47.09% 46.79% -0.3% - Complexity 25079 25648 +569 
===========================================
Files 2389 2456 +67 Lines 259750 267615 +7865 Branches 29400 30563 +1163 ===========================================
+ Hits 122328 125243 +2915 - Misses 128459 133113 +4654 - Partials 8963 9259 +296
Impacted FilesCoverage ΔComplexity Δ
...in/java/com/google/cloud/firestore/FieldValue.java79.77% <100%> (+0.46%)8 <0> (ø)⬇️
.../cloud/compute/v1/InsertTargetPoolHttpRequest.java22.08% <0%> (-1.85%)8% <0%> (-1%)
...c/main/java/com/google/cloud/logging/SinkInfo.java82.03% <0%> (-1.78%)19% <0%> (-2%)
...n/java/com/google/cloud/compute/v1/Commitment.java34.68% <0%> (-1.77%)21% <0%> (+1%)
...m/google/cloud/compute/v1/stub/RegionDiskStub.java8.33% <0%> (-1.67%)1% <0%> (ø)
...om/google/cloud/compute/v1/RegionDiskSettings.java16.66% <0%> (-1.52%)3% <0%> (ø)
...java/com/google/cloud/compute/v1/DiskSettings.java14.81% <0%> (-1.19%)3% <0%> (ø)
...ava/com/google/cloud/compute/v1/stub/DiskStub.java6.25% <0%> (-0.9%)1% <0%> (ø)
...java/com/google/cloud/compute/v1/NodeTemplate.java35.53% <0%> (-0.89%)20% <0%> (+1%)
...n/java/com/google/cloud/compute/v1/DiskClient.java55.71% <0%> (-0.88%)55% <0%> (+8%)
... and 584 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d284774...0570f24. Read the comment docs.

@pmakani

Copy link
Copy Markdown
Author

@schmidt-sebastian PTAL

@schmidt-sebastianschmidt-sebastian left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@pmakani Thank you for addressing my feedback! This basically looks good, but I would like to tweak the tests slightly to better match the existing format. While you are essentially asserting the same things, we have been comparing the final CommitRequest rather than the outout from the toProto() calls.

Do you mind applying this patch to your PR: https://gist.github.com/schmidt-sebastian/08b8cf9956a33989a8eedaad4533d25d

We are then good to go! Thank you for your contribution.

@pmakanipmakani added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 31, 2019
@yoshi-kokoroyoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 31, 2019
@BenWhitehead

Copy link
Copy Markdown
Contributor

@pmakani Thanks for the contribution!

@BenWhitehead
BenWhitehead merged commit 7ddeb9d into googleapis:masterAug 1, 2019
@pmakani
pmakani deleted the api-firestore-5622 branch August 2, 2019 06:21
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla: yesThis human has signed the Contributor License Agreement.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Firestore: update with arrayUnion doesn't accept POJO

5 participants

@pmakani@BenWhitehead@schmidt-sebastian@googlebot@yoshi-kokoro