Drop Array, Reference, Number, Integer and DoubleValue #1206

schmidt-sebastian · 2020-02-05T23:08:00Z

No description provided.

codecov · 2020-02-05T23:17:32Z

Codecov Report

Merging #1206 into mrschmidt/rewritefieldvalue will not change coverage.
The diff coverage is n/a.

Flag	Coverage Δ	Complexity Δ
#FirebaseFirestore	`62.07% <0%> (ø)`	`2254% <0%> (ø)`	⬇️
#FirebaseFirestore_Ktx	`41.17% <0%> (ø)`	`0% <0%> (ø)`	⬇️

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 0abdfb1...6ef7289. Read the comment docs.

schmidt-sebastian · 2020-02-06T00:50:00Z

/test smoke-tests

wilhuff

Nice!

wilhuff · 2020-02-06T01:54:26Z

firebase-firestore/ktx/src/test/java/com/google/firebase/firestore/testutil/TestUtil.java

@@ -36,12 +36,12 @@ public static FieldValue wrap(Object value) {
    UserDataReader dataReader = new UserDataReader(databaseId);
    // HACK: We use parseQueryValue() since it accepts scalars as well as arrays / objects, and
    // our tests currently use wrap() pretty generically so we don't know the intent.
-    return dataReader.parseQueryValue(value);
+    return new FieldValue(dataReader.parseQueryValue(value));


If a caller other than wrapObject calls wrap on a Map<String, Object> this will return the wrong type.

It seems like this should parse the query value then check the type and wrap appropriately (or call FieldValue.valueOf). That would also mean that wrapObject wouldn't have change below.

Fixed. This conversion should go away entirely in a follow-up PR too.

wilhuff · 2020-02-06T02:01:48Z

firebase-firestore/src/main/java/com/google/firebase/firestore/model/value/ProtoValues.java

@@ -157,8 +164,8 @@ private static boolean objectEquals(Value left, Value right) {
  }

  /** Returns true if the Value list contains the specified element. */
-  public static boolean contains(List<Value> haystack, Value needle) {
-    for (Value haystackEl : haystack) {
+  public static boolean contains(ArrayValueOrBuilder haystack, Value needle) {


Nearly all callers check the array type before calling this. It seems like it would be worth adding an alternative like this:

public static boolean arrayContains(Value haystack, Value needle) { return isArray(haystack) && contains(haystack.getArrayValue(), needle); }

Only ArrayContainsFilter does the check right at the callsite. All other filters verify the value in the constructor, which is IMHO the better thing to do since the assert will include a stacktrace that should indicate where the incorrect value was created.

Furthermore, ArrayTransformOperation doesn't have a Value, it only operates on ArrayValue.Builder. For that reason, I left this as is for now.

wilhuff · 2020-02-06T02:05:24Z

firebase-firestore/src/main/java/com/google/firebase/firestore/core/Bound.java

-    for (FieldValue indexComponent : position) {
-      builder.append(indexComponent.toString());
+    for (Value indexComponent : position) {
+      builder.append(ProtoValues.canonicalId(indexComponent));


While we're fixing the format here, should we separate these values with something?

(Optional; may not be worth it. Just a thought because it may make these more readable.)

Added comma separation.

wilhuff · 2020-02-06T02:07:21Z

firebase-firestore/src/main/java/com/google/firebase/firestore/core/InFilter.java

    super(field, Operator.IN, value);
+    hardAssert(ProtoValues.isArray(value), "InFilter expects an ArrayValue");
  }

  @Override
  public boolean matches(Document doc) {
    FieldValue other = doc.getField(getField());


doc.getFieldProto(getField())?

wilhuff · 2020-02-06T02:08:53Z

firebase-firestore/src/main/java/com/google/firebase/firestore/core/KeyFieldFilter.java

  }

  @Override
  public boolean matches(Document doc) {
-    ReferenceValue referenceValue = (ReferenceValue) getValue();
-    int comparator = doc.getKey().compareTo(referenceValue.getKey());
+    DocumentKey referencedKey = DocumentKey.fromName(getValue().getReferenceValue());


This seems like something we could store as a field. It doesn't change for every document we match.

wilhuff · 2020-02-06T02:24:26Z

firebase-firestore/src/main/java/com/google/firebase/firestore/core/KeyFieldInFilter.java

-    for (FieldValue refValue : arrayValue.getValues()) {
-      if (doc.getKey().equals(((ReferenceValue) refValue).getKey())) {
+    for (Value refValue : getValue().getArrayValue().getValuesList()) {
+      DocumentKey referencedKey = DocumentKey.fromName(refValue.getReferenceValue());


Similarly we could do this conversion once in the constructor and keep a list of document keys as a field.

Done. This simplifies the code a bit since I can now use List.contains().

wilhuff · 2020-02-06T02:28:49Z

firebase-firestore/src/main/java/com/google/firebase/firestore/model/value/ProtoValues.java

-  public static boolean contains(List<Value> haystack, Value needle) {
-    for (Value haystackEl : haystack) {
+  public static boolean contains(ArrayValueOrBuilder haystack, Value needle) {
+    for (Value haystackEl : haystack.getValuesList()) {


Nit: Java doesn't tend to abbreviate like this. Consider naming this just element, or haystackElement if you want to preserve the binding to the haystack.

Used haystackElement

google-oss-bot · 2020-02-06T05:31:38Z

@schmidt-sebastian: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
smoke-tests	`6ef7289`	link	`/test smoke-tests`

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

wilhuff

LGTM

schmidt-sebastian added 8 commits February 3, 2020 20:03

Protobuf-backed FieldValues

1602062

Merge branch 'mrschmidt/rewritefieldvalue' into mrschmidt/final

804a1b7

Merge branch 'mrschmidt/rewritefieldvalue' into mrschmidt/final

3d5d7c2

Feedback

120ca33

Update test-only variable

82c0095

Add ProtoValues.contains()

98953a3

Migrate ArrayTransforms to use Values

940a268

Migrate TransformResult to use Value

7ea5b37

schmidt-sebastian changed the base branch from master to mrschmidt/migratevalues February 5, 2020 23:08

google-oss-bot added the size/XL label Feb 5, 2020

googlebot added the cla: yes Override cla label Feb 5, 2020

schmidt-sebastian changed the title ~~Mrschmidt/droparray~~ Drop Array, Reference, Number, Integer and DoubleValue Feb 5, 2020

Drop Array, Reference, Number, Integer and DoubleValue

637838b

schmidt-sebastian force-pushed the mrschmidt/droparray branch from 8e0ec91 to 637838b Compare February 5, 2020 23:15

schmidt-sebastian requested a review from wilhuff February 5, 2020 23:15

schmidt-sebastian assigned wilhuff Feb 5, 2020

schmidt-sebastian added the api: firestore label Feb 5, 2020

schmidt-sebastian added 2 commits February 5, 2020 16:10

Fix Kotlin

b5cc998

More Kotlin fixes

162f071

schmidt-sebastian added 3 commits February 5, 2020 16:56

Review

13e01c8

Merge branch 'mrschmidt/rewritefieldvalue' into mrschmidt/final

baf650e

Merge branch 'mrschmidt/final' into mrschmidt/migratevalues

aa7b3ec

wilhuff reviewed Feb 6, 2020

View reviewed changes

wilhuff assigned schmidt-sebastian and unassigned wilhuff Feb 6, 2020

schmidt-sebastian added 3 commits February 5, 2020 21:07

Feedback

774598a

Make FieldValue package-private

490c924

Merge branch 'mrschmidt/migratevalues' into mrschmidt/droparray

0abdfb1

schmidt-sebastian changed the base branch from mrschmidt/migratevalues to mrschmidt/rewritefieldvalue February 6, 2020 05:12

google-oss-bot added size/XXL and removed size/XL labels Feb 6, 2020

schmidt-sebastian changed the base branch from mrschmidt/rewritefieldvalue to mrschmidt/migratevalues February 6, 2020 05:13

google-oss-bot added size/XL and removed size/XXL labels Feb 6, 2020

Merge

6ef7289

schmidt-sebastian changed the base branch from mrschmidt/migratevalues to mrschmidt/rewritefieldvalue February 6, 2020 05:18

schmidt-sebastian assigned wilhuff and unassigned schmidt-sebastian Feb 6, 2020

wilhuff approved these changes Feb 6, 2020

View reviewed changes

wilhuff assigned schmidt-sebastian and unassigned wilhuff Feb 6, 2020

schmidt-sebastian merged commit 32e1e92 into mrschmidt/rewritefieldvalue Feb 6, 2020

schmidt-sebastian deleted the mrschmidt/droparray branch February 7, 2020 00:22

firebase locked and limited conversation to collaborators Mar 8, 2020

Drop Array, Reference, Number, Integer and DoubleValue #1206

Drop Array, Reference, Number, Integer and DoubleValue #1206

Uh oh!

Conversation

schmidt-sebastian commented Feb 5, 2020

Uh oh!

codecov bot commented Feb 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

schmidt-sebastian commented Feb 6, 2020

Uh oh!

wilhuff left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

schmidt-sebastian Feb 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

google-oss-bot commented Feb 6, 2020

Uh oh!

wilhuff left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecov bot commented Feb 5, 2020 •

edited

Loading

schmidt-sebastian Feb 6, 2020 •

edited

Loading