From ca50431cd89f24f91073b794b326265b2fb2c221 Mon Sep 17 00:00:00 2001 From: Rich Gowman Date: Wed, 21 Nov 2018 14:53:01 -0500 Subject: [PATCH 01/11] Eliminate 'badimport' warning It was already reasonably obvious what this is, so the warning is mostly spurious. But it's easier and less boilerplate to just add the extra qualifier than to suppress the warning. https://errorprone.info/bugpattern/BadImport --- .../google/firebase/firestore/local/IndexedQueryEngine.java | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/IndexedQueryEngine.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/IndexedQueryEngine.java index 1dab574afff..ab734d95e31 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/IndexedQueryEngine.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/IndexedQueryEngine.java @@ -22,7 +22,6 @@ import com.google.firebase.firestore.core.Filter; import com.google.firebase.firestore.core.Filter.Operator; import com.google.firebase.firestore.core.IndexRange; -import com.google.firebase.firestore.core.IndexRange.Builder; import com.google.firebase.firestore.core.NaNFilter; import com.google.firebase.firestore.core.NullFilter; import com.google.firebase.firestore.core.Query; @@ -216,7 +215,7 @@ static IndexRange extractBestIndexRange(Query query) { * filter. The determined {@code IndexRange} is likely overselective and requires post-filtering. */ private static IndexRange convertFilterToIndexRange(Filter filter) { - Builder indexRange = IndexRange.builder().setFieldPath(filter.getField()); + IndexRange.Builder indexRange = IndexRange.builder().setFieldPath(filter.getField()); if (filter instanceof RelationFilter) { RelationFilter relationFilter = (RelationFilter) filter; FieldValue filterValue = relationFilter.getValue(); From 6b94249d66e20e60862ea0e53d1e94bdb36570b7 Mon Sep 17 00:00:00 2001 From: Rich Gowman Date: Wed, 21 Nov 2018 14:57:53 -0500 Subject: [PATCH 02/11] Suppress EqualsGetClass warnings for public classes These are effectively final and are documented as not being final only to allow mocking within tests. https://errorprone.info/bugpattern/EqualsGetClass --- .../java/com/google/firebase/firestore/DocumentReference.java | 1 + .../src/main/java/com/google/firebase/firestore/Query.java | 1 + 2 files changed, 2 insertions(+) diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/DocumentReference.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/DocumentReference.java index cde2b8a3e3c..d06f90c81bd 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/DocumentReference.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/DocumentReference.java @@ -525,6 +525,7 @@ private ListenerRegistration addSnapshotListenerInternal( } @Override + @SuppressWarnings("EqualsGetClass") // This class is effectively final. public boolean equals(Object o) { if (this == o) { return true; diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/Query.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/Query.java index bb6143edcce..20187e2a726 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/Query.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/Query.java @@ -892,6 +892,7 @@ private ListenerRegistration addSnapshotListenerInternal( } @Override + @SuppressWarnings("EqualsGetClass") // This class is effectively final. public boolean equals(Object o) { if (this == o) { return true; From f72a5b39b6d8f96fd9ecd434b81ed4fda03d73ba Mon Sep 17 00:00:00 2001 From: Rich Gowman Date: Wed, 21 Nov 2018 16:33:34 -0500 Subject: [PATCH 03/11] Mark a number of classes final Mostly to avoid a warning, though it's generally recommended to do this anyways. (Note that this makes mocking impossible; if any of these classes need to be mocked, we might be better of just suppressing the warning or using instanceof.) https://errorprone.info/bugpattern/EqualsGetClass --- .../java/com/google/firebase/firestore/model/Document.java | 2 +- .../com/google/firebase/firestore/model/DocumentKey.java | 2 +- .../com/google/firebase/firestore/model/DocumentSet.java | 2 +- .../com/google/firebase/firestore/model/NoDocument.java | 2 +- .../google/firebase/firestore/model/UnknownDocument.java | 2 +- .../google/firebase/firestore/model/mutation/FieldMask.java | 2 +- .../com/google/firebase/firestore/remote/WatchChange.java | 6 +++--- 7 files changed, 9 insertions(+), 9 deletions(-) diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/model/Document.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/model/Document.java index 83eeaa28412..44dfb3af8c4 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/model/Document.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/model/Document.java @@ -23,7 +23,7 @@ * Represents a document in Firestore with a key, version, data and whether the data has local * mutations applied to it. */ -public class Document extends MaybeDocument { +public final class Document extends MaybeDocument { /** Describes the `hasPendingWrites` state of a document. */ public enum DocumentState { diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/model/DocumentKey.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/model/DocumentKey.java index f4e3b7b5e09..173edfaf96c 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/model/DocumentKey.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/model/DocumentKey.java @@ -23,7 +23,7 @@ import java.util.List; /** DocumentKey represents the location of a document in the Firestore database. */ -public class DocumentKey implements Comparable { +public final class DocumentKey implements Comparable { public static final String KEY_FIELD_NAME = "__name__"; diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/model/DocumentSet.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/model/DocumentSet.java index 66dd581cf1d..47a10987bc1 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/model/DocumentSet.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/model/DocumentSet.java @@ -30,7 +30,7 @@ * An immutable set of documents (unique by key) ordered by the given comparator or ordered by key * by default if no document is present. */ -public class DocumentSet implements Iterable { +public final class DocumentSet implements Iterable { /** Returns an empty DocumentSet sorted by the given comparator, then by keys. */ public static DocumentSet emptySet(final Comparator comparator) { diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/model/NoDocument.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/model/NoDocument.java index 74b6d6cad54..df83be54a54 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/model/NoDocument.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/model/NoDocument.java @@ -15,7 +15,7 @@ package com.google.firebase.firestore.model; /** Represents that no documents exists for the key at the given version. */ -public class NoDocument extends MaybeDocument { +public final class NoDocument extends MaybeDocument { private boolean hasCommittedMutations; diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/model/UnknownDocument.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/model/UnknownDocument.java index dc8cf0d9142..892ffb2d712 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/model/UnknownDocument.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/model/UnknownDocument.java @@ -18,7 +18,7 @@ * A class representing an existing document whose data is unknown (e.g. a document that was updated * without a known base document). */ -public class UnknownDocument extends MaybeDocument { +public final class UnknownDocument extends MaybeDocument { public UnknownDocument(DocumentKey key, SnapshotVersion version) { super(key, version); } diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/model/mutation/FieldMask.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/model/mutation/FieldMask.java index 0cbef886ccd..f65a107c6ee 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/model/mutation/FieldMask.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/model/mutation/FieldMask.java @@ -25,7 +25,7 @@ * companion ObjectValue, the field is deleted. foo.bar - Overwrites only the field bar of the * object foo. If foo is not an object, foo is replaced with an object containing foo. */ -public class FieldMask { +public final class FieldMask { public static FieldMask fromCollection(Collection mask) { return new FieldMask(mask); } diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/remote/WatchChange.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/remote/WatchChange.java index e2a882cc797..565c85afa2e 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/remote/WatchChange.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/remote/WatchChange.java @@ -38,7 +38,7 @@ private WatchChange() { * A document change represents a change document and a list of target ids to which this change * applies. If the document has been deleted, the deleted document will be provided. */ - public static class DocumentChange extends WatchChange { + public static final class DocumentChange extends WatchChange { // TODO: figure out if we can actually use arrays here for efficiency /** The new document applies to all of these targets. */ private final List updatedTargetIds; @@ -137,7 +137,7 @@ public int hashCode() { * An ExistenceFilterWatchChange applies to the targets and is required to verify the current * client state against expected state sent from the server. */ - public static class ExistenceFilterWatchChange extends WatchChange { + public static final class ExistenceFilterWatchChange extends WatchChange { private final int targetId; private final ExistenceFilter existenceFilter; @@ -177,7 +177,7 @@ public enum WatchTargetChangeType { } /** The state of a target has changed. This can mean removal, addition, current or reset. */ - public static class WatchTargetChange extends WatchChange { + public static final class WatchTargetChange extends WatchChange { private final WatchTargetChangeType changeType; private final List targetIds; private final ByteString resumeToken; From 8a98285828d2262c03b02f41aa31a9379f36f8e8 Mon Sep 17 00:00:00 2001 From: Rich Gowman Date: Wed, 21 Nov 2018 16:48:42 -0500 Subject: [PATCH 04/11] Switch .getClass() to instanceof in some .equals() methods This is for cases where we're dealing with a non-leaf node in the inheritance hierarchy. (Leaf nodes in the heirarchy can just mark the entire class as final.) In order to preserve symmetry of .equals(), also mark the .equals() methods themselves as final. In this case, "non-leaf" nodes also counts effectively final classes which are mocked. (i.e. ViewSnapshot.) https://errorprone.info/bugpattern/EqualsGetClass --- .../google/firebase/firestore/core/ViewSnapshot.java | 4 ++-- .../com/google/firebase/firestore/model/BasePath.java | 10 ++++------ .../model/mutation/ArrayTransformOperation.java | 4 ++-- 3 files changed, 8 insertions(+), 10 deletions(-) diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/ViewSnapshot.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/ViewSnapshot.java index a05cffea965..1e05a3d8e98 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/ViewSnapshot.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/ViewSnapshot.java @@ -118,11 +118,11 @@ public boolean excludesMetadataChanges() { } @Override - public boolean equals(Object o) { + public final boolean equals(Object o) { if (this == o) { return true; } - if (o == null || getClass() != o.getClass()) { + if (!(o instanceof ViewSnapshot)) { return false; } diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/model/BasePath.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/model/BasePath.java index 2edc7875e3b..4da80b39d99 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/model/BasePath.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/model/BasePath.java @@ -161,13 +161,11 @@ public int length() { } @Override - @SuppressWarnings("unchecked") - public boolean equals(Object o) { - if (o == null) { - return false; + public final boolean equals(Object o) { + if (this == o) { + return true; } - // The cast is not unchecked because of the class equality check. - return getClass() == o.getClass() && compareTo((B) o) == 0; + return (o instanceof BasePath) && compareTo((B) o) == 0; } @Override diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/model/mutation/ArrayTransformOperation.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/model/mutation/ArrayTransformOperation.java index 89e10ec3364..959618c3eb1 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/model/mutation/ArrayTransformOperation.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/model/mutation/ArrayTransformOperation.java @@ -51,11 +51,11 @@ public FieldValue applyToRemoteDocument(FieldValue previousValue, FieldValue tra } @Override - public boolean equals(Object o) { + public final boolean equals(Object o) { if (this == o) { return true; } - if (o == null || getClass() != o.getClass()) { + if (!(o instanceof ArrayTransformOperation)) { return false; } From 23e5f94ef665497c154869557ae735db65d2195e Mon Sep 17 00:00:00 2001 From: Rich Gowman Date: Wed, 21 Nov 2018 17:10:33 -0500 Subject: [PATCH 05/11] Provide toString on FieldMask Since it's used by PatchMutation https://errorprone.info/bugpattern/ObjectToString --- .../google/firebase/firestore/model/mutation/FieldMask.java | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/model/mutation/FieldMask.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/model/mutation/FieldMask.java index f65a107c6ee..d26202768bb 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/model/mutation/FieldMask.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/model/mutation/FieldMask.java @@ -49,6 +49,11 @@ public boolean equals(Object o) { return mask.equals(fieldMask.mask); } + @Override + public String toString() { + return "FieldMask{mask=" + mask.toString() + "}"; + } + /** * Verifies that 'fieldPath' is included by at least one field in this field mask. * From 917b0f8e143cf1b3703ef90dccb615cee3d6f9fc Mon Sep 17 00:00:00 2001 From: Rich Gowman Date: Thu, 22 Nov 2018 10:07:58 -0500 Subject: [PATCH 06/11] Fix a 'missing override' warning https://errorprone.info/bugpattern/MissingOverride --- .../java/com/google/firebase/firestore/core/EventManager.java | 1 + 1 file changed, 1 insertion(+) diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/EventManager.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/EventManager.java index e74f837e496..38665628a45 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/EventManager.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/EventManager.java @@ -137,6 +137,7 @@ public void onError(Query query, Status error) { queries.remove(query); } + @Override public void handleOnlineStateChange(OnlineState onlineState) { this.onlineState = onlineState; for (QueryListenersInfo info : queries.values()) { From 34e8b0eb4e720f4252067fe5351a376143fc5d41 Mon Sep 17 00:00:00 2001 From: Rich Gowman Date: Thu, 22 Nov 2018 11:25:41 -0500 Subject: [PATCH 07/11] Rework FieldMask to use a Set of FieldPaths rather than a Collection. FieldMask uses .equals() on the container, and that's not well defined for a Collection. The fix is to switch to either a Set or a List. Set seems more appropriate. https://errorprone.info/bugpattern/UndefinedEquals --- .../com/google/firebase/firestore/SetOptions.java | 15 ++++++++------- .../google/firebase/firestore/core/UserData.java | 14 +++++++------- .../firestore/model/mutation/FieldMask.java | 10 +++++----- .../firestore/remote/RemoteSerializer.java | 6 ++++-- .../firestore/local/LocalSerializerTest.java | 3 ++- .../firebase/firestore/model/MutationTest.java | 3 ++- .../firebase/firestore/testutil/TestUtil.java | 6 ++++-- 7 files changed, 32 insertions(+), 25 deletions(-) diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/SetOptions.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/SetOptions.java index 4c302738906..adde4525994 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/SetOptions.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/SetOptions.java @@ -20,8 +20,9 @@ import android.support.annotation.Nullable; import com.google.firebase.annotations.PublicApi; import com.google.firebase.firestore.model.mutation.FieldMask; -import java.util.ArrayList; +import java.util.HashSet; import java.util.List; +import java.util.Set; /** * An options object that configures the behavior of set() calls. By providing one of the SetOptions @@ -78,13 +79,13 @@ public static SetOptions merge() { @NonNull @PublicApi public static SetOptions mergeFields(List fields) { - List fieldPaths = new ArrayList<>(); + Set fieldPaths = new HashSet<>(); for (String field : fields) { fieldPaths.add(FieldPath.fromDotSeparatedPath(field).getInternalPath()); } - return new SetOptions(true, FieldMask.fromCollection(fieldPaths)); + return new SetOptions(true, FieldMask.fromSet(fieldPaths)); } /** @@ -100,13 +101,13 @@ public static SetOptions mergeFields(List fields) { @NonNull @PublicApi public static SetOptions mergeFields(String... fields) { - List fieldPaths = new ArrayList<>(); + Set fieldPaths = new HashSet<>(); for (String field : fields) { fieldPaths.add(FieldPath.fromDotSeparatedPath(field).getInternalPath()); } - return new SetOptions(true, FieldMask.fromCollection(fieldPaths)); + return new SetOptions(true, FieldMask.fromSet(fieldPaths)); } /** @@ -121,13 +122,13 @@ public static SetOptions mergeFields(String... fields) { @NonNull @PublicApi public static SetOptions mergeFieldPaths(List fields) { - List fieldPaths = new ArrayList<>(); + Set fieldPaths = new HashSet<>(); for (FieldPath field : fields) { fieldPaths.add(field.getInternalPath()); } - return new SetOptions(true, FieldMask.fromCollection(fieldPaths)); + return new SetOptions(true, FieldMask.fromSet(fieldPaths)); } @Override diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/UserData.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/UserData.java index 00cc3969799..0cb0f614903 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/UserData.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/UserData.java @@ -29,9 +29,9 @@ import com.google.firebase.firestore.model.value.ObjectValue; import com.google.firebase.firestore.util.Assert; import java.util.ArrayList; +import java.util.HashSet; import java.util.List; -import java.util.SortedSet; -import java.util.TreeSet; +import java.util.Set; import java.util.regex.Pattern; import javax.annotation.Nullable; @@ -72,8 +72,8 @@ public static class ParseAccumulator { */ private final Source dataSource; - /** Accumulates a list of the field paths found while parsing the data. */ - private final SortedSet fieldMask; + /** Accumulates a set of the field paths found while parsing the data. */ + private final Set fieldMask; /** Accumulates a list of field transforms found while parsing the data. */ private final ArrayList fieldTransforms; @@ -81,7 +81,7 @@ public static class ParseAccumulator { /** @param dataSource Indicates what kind of API method this data came from. */ public ParseAccumulator(Source dataSource) { this.dataSource = dataSource; - this.fieldMask = new TreeSet<>(); + this.fieldMask = new HashSet<>(); this.fieldTransforms = new ArrayList<>(); } @@ -135,7 +135,7 @@ void addToFieldTransforms(FieldPath fieldPath, TransformOperation transformOpera */ public ParsedSetData toMergeData(ObjectValue data) { return new ParsedSetData( - data, FieldMask.fromCollection(fieldMask), unmodifiableList(fieldTransforms)); + data, FieldMask.fromSet(fieldMask), unmodifiableList(fieldTransforms)); } /** @@ -181,7 +181,7 @@ public ParsedSetData toSetData(ObjectValue data) { */ public ParsedUpdateData toUpdateData(ObjectValue data) { return new ParsedUpdateData( - data, FieldMask.fromCollection(fieldMask), unmodifiableList(fieldTransforms)); + data, FieldMask.fromSet(fieldMask), unmodifiableList(fieldTransforms)); } } diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/model/mutation/FieldMask.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/model/mutation/FieldMask.java index d26202768bb..7b0f8ee9fb2 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/model/mutation/FieldMask.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/model/mutation/FieldMask.java @@ -15,7 +15,7 @@ package com.google.firebase.firestore.model.mutation; import com.google.firebase.firestore.model.FieldPath; -import java.util.Collection; +import java.util.Set; /** * Provides a set of fields that can be used to partially patch a document. The FieldMask is used in @@ -26,13 +26,13 @@ * object foo. If foo is not an object, foo is replaced with an object containing foo. */ public final class FieldMask { - public static FieldMask fromCollection(Collection mask) { + public static FieldMask fromSet(Set mask) { return new FieldMask(mask); } - private final Collection mask; + private final Set mask; - private FieldMask(Collection mask) { + private FieldMask(Set mask) { this.mask = mask; } @@ -74,7 +74,7 @@ public int hashCode() { return mask.hashCode(); } - public Collection getMask() { + public Set getMask() { return mask; } } diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/remote/RemoteSerializer.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/remote/RemoteSerializer.java index fc4eb36bd30..fd2e85967af 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/remote/RemoteSerializer.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/remote/RemoteSerializer.java @@ -98,8 +98,10 @@ import java.util.Arrays; import java.util.Collections; import java.util.HashMap; +import java.util.HashSet; import java.util.List; import java.util.Map; +import java.util.Set; /** Serializer that converts to and from Firestore API protos. */ public final class RemoteSerializer { @@ -532,11 +534,11 @@ private DocumentMask encodeDocumentMask(FieldMask mask) { private FieldMask decodeDocumentMask(DocumentMask mask) { int count = mask.getFieldPathsCount(); - List paths = new ArrayList<>(count); + Set paths = new HashSet<>(count); for (int i = 0; i < count; i++) { paths.add(FieldPath.fromServerFormat(mask.getFieldPaths(i))); } - return FieldMask.fromCollection(paths); + return FieldMask.fromSet(paths); } private DocumentTransform.FieldTransform encodeFieldTransform(FieldTransform fieldTransform) { diff --git a/firebase-firestore/src/test/java/com/google/firebase/firestore/local/LocalSerializerTest.java b/firebase-firestore/src/test/java/com/google/firebase/firestore/local/LocalSerializerTest.java index db743396b29..747782f567e 100644 --- a/firebase-firestore/src/test/java/com/google/firebase/firestore/local/LocalSerializerTest.java +++ b/firebase-firestore/src/test/java/com/google/firebase/firestore/local/LocalSerializerTest.java @@ -44,6 +44,7 @@ import com.google.firestore.v1beta1.Value; import com.google.firestore.v1beta1.Write; import com.google.protobuf.ByteString; +import java.util.HashSet; import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; @@ -71,7 +72,7 @@ public void testEncodesMutationBatch() { new PatchMutation( key("bar/baz"), TestUtil.wrapObject(map("a", "b", "num", 1)), - FieldMask.fromCollection(asList(field("a"))), + FieldMask.fromSet(new HashSet<>(asList(field("a")))), com.google.firebase.firestore.model.mutation.Precondition.exists(true)); Mutation del = deleteMutation("baz/quux"); Timestamp writeTime = Timestamp.now(); diff --git a/firebase-firestore/src/test/java/com/google/firebase/firestore/model/MutationTest.java b/firebase-firestore/src/test/java/com/google/firebase/firestore/model/MutationTest.java index d686c4829ef..855e2c66e23 100644 --- a/firebase-firestore/src/test/java/com/google/firebase/firestore/model/MutationTest.java +++ b/firebase-firestore/src/test/java/com/google/firebase/firestore/model/MutationTest.java @@ -46,6 +46,7 @@ import com.google.firebase.firestore.model.value.TimestampValue; import java.util.Arrays; import java.util.Collections; +import java.util.HashSet; import java.util.Map; import org.junit.Test; import org.junit.runner.RunWith; @@ -110,7 +111,7 @@ public void testDeletesValuesFromTheFieldMask() { Document baseDoc = doc("collection/key", 0, data); DocumentKey key = key("collection/key"); - FieldMask mask = FieldMask.fromCollection(Arrays.asList(field("foo.bar"))); + FieldMask mask = FieldMask.fromSet(new HashSet<>(Arrays.asList(field("foo.bar")))); Mutation patch = new PatchMutation(key, ObjectValue.emptyObject(), mask, Precondition.NONE); MaybeDocument patchDoc = patch.applyToLocalView(baseDoc, baseDoc, Timestamp.now()); diff --git a/firebase-firestore/src/testUtil/java/com/google/firebase/firestore/testutil/TestUtil.java b/firebase-firestore/src/testUtil/java/com/google/firebase/firestore/testutil/TestUtil.java index d6d5384fd5f..23c9a7fc972 100644 --- a/firebase-firestore/src/testUtil/java/com/google/firebase/firestore/testutil/TestUtil.java +++ b/firebase-firestore/src/testUtil/java/com/google/firebase/firestore/testutil/TestUtil.java @@ -78,6 +78,8 @@ import java.util.Map; import java.util.Map.Entry; import java.util.Set; +import java.util.SortedSet; +import java.util.TreeSet; import javax.annotation.Nullable; /** A set of utilities for tests */ @@ -431,12 +433,12 @@ public static PatchMutation patchMutation( boolean merge = updateMask != null; // We sort the fieldMaskPaths to make the order deterministic in tests. - Collections.sort(objectMask); + SortedSet fieldMaskPaths = new TreeSet<>(merge ? updateMask : objectMask); return new PatchMutation( key(path), objectValue, - FieldMask.fromCollection(merge ? updateMask : objectMask), + FieldMask.fromSet(fieldMaskPaths), merge ? Precondition.NONE : Precondition.exists(true)); } From 024e9fe36f725982fd715ee2afac68be0c7b3b56 Mon Sep 17 00:00:00 2001 From: Rich Gowman Date: Fri, 23 Nov 2018 15:25:20 -0500 Subject: [PATCH 08/11] Review feedback --- .../java/com/google/firebase/firestore/DocumentReference.java | 3 +-- .../src/main/java/com/google/firebase/firestore/Query.java | 3 +-- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/DocumentReference.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/DocumentReference.java index d06f90c81bd..44c29e88ef1 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/DocumentReference.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/DocumentReference.java @@ -525,12 +525,11 @@ private ListenerRegistration addSnapshotListenerInternal( } @Override - @SuppressWarnings("EqualsGetClass") // This class is effectively final. public boolean equals(Object o) { if (this == o) { return true; } - if (o == null || getClass() != o.getClass()) { + if (!(o instanceof DocumentReference)) { return false; } diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/Query.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/Query.java index 20187e2a726..e80e28c9f87 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/Query.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/Query.java @@ -892,12 +892,11 @@ private ListenerRegistration addSnapshotListenerInternal( } @Override - @SuppressWarnings("EqualsGetClass") // This class is effectively final. public boolean equals(Object o) { if (this == o) { return true; } - if (o == null || getClass() != o.getClass()) { + if (!(o instanceof Query)) { return false; } From 4d88c957472f2cc1bf84d9508673bc5d527c9190 Mon Sep 17 00:00:00 2001 From: Rich Gowman Date: Tue, 27 Nov 2018 17:28:18 -0500 Subject: [PATCH 09/11] Review feedback --- .../firestore/model/mutation/ArrayTransformOperation.java | 5 +++-- .../com/google/firebase/firestore/testutil/TestUtil.java | 4 +++- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/model/mutation/ArrayTransformOperation.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/model/mutation/ArrayTransformOperation.java index 959618c3eb1..b4c3b81254b 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/model/mutation/ArrayTransformOperation.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/model/mutation/ArrayTransformOperation.java @@ -51,11 +51,12 @@ public FieldValue applyToRemoteDocument(FieldValue previousValue, FieldValue tra } @Override - public final boolean equals(Object o) { + @SuppressWarnings("EqualsGetClass") // subtype-sensitive equality is intended. + public boolean equals(Object o) { if (this == o) { return true; } - if (!(o instanceof ArrayTransformOperation)) { + if (o == null || getClass() != o.getClass()) { return false; } diff --git a/firebase-firestore/src/testUtil/java/com/google/firebase/firestore/testutil/TestUtil.java b/firebase-firestore/src/testUtil/java/com/google/firebase/firestore/testutil/TestUtil.java index 23c9a7fc972..a599d7024fc 100644 --- a/firebase-firestore/src/testUtil/java/com/google/firebase/firestore/testutil/TestUtil.java +++ b/firebase-firestore/src/testUtil/java/com/google/firebase/firestore/testutil/TestUtil.java @@ -432,7 +432,9 @@ public static PatchMutation patchMutation( boolean merge = updateMask != null; - // We sort the fieldMaskPaths to make the order deterministic in tests. + // We sort the fieldMaskPaths to make the order deterministic in tests. (Otherwise, when we + // flatten a Set to a proto repeated field, we'll end up comparing in iterator order and + // possibly consider {foo,bar} != {bar,foo}.) SortedSet fieldMaskPaths = new TreeSet<>(merge ? updateMask : objectMask); return new PatchMutation( From 497434f72a3b36806da339b1ca56f26b6b14dce4 Mon Sep 17 00:00:00 2001 From: Rich Gowman Date: Wed, 28 Nov 2018 17:22:10 -0500 Subject: [PATCH 10/11] Add TestUtil.fieldMask(String...) to simplify creating FieldMasks. --- .../firebase/firestore/local/LocalSerializerTest.java | 4 ++-- .../google/firebase/firestore/model/MutationTest.java | 3 ++- .../com/google/firebase/firestore/testutil/TestUtil.java | 9 +++++++++ 3 files changed, 13 insertions(+), 3 deletions(-) diff --git a/firebase-firestore/src/test/java/com/google/firebase/firestore/local/LocalSerializerTest.java b/firebase-firestore/src/test/java/com/google/firebase/firestore/local/LocalSerializerTest.java index 747782f567e..9d4dd5ac1e1 100644 --- a/firebase-firestore/src/test/java/com/google/firebase/firestore/local/LocalSerializerTest.java +++ b/firebase-firestore/src/test/java/com/google/firebase/firestore/local/LocalSerializerTest.java @@ -18,6 +18,7 @@ import static com.google.firebase.firestore.testutil.TestUtil.deletedDoc; import static com.google.firebase.firestore.testutil.TestUtil.doc; import static com.google.firebase.firestore.testutil.TestUtil.field; +import static com.google.firebase.firestore.testutil.TestUtil.fieldMask; import static com.google.firebase.firestore.testutil.TestUtil.key; import static com.google.firebase.firestore.testutil.TestUtil.map; import static com.google.firebase.firestore.testutil.TestUtil.setMutation; @@ -44,7 +45,6 @@ import com.google.firestore.v1beta1.Value; import com.google.firestore.v1beta1.Write; import com.google.protobuf.ByteString; -import java.util.HashSet; import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; @@ -72,7 +72,7 @@ public void testEncodesMutationBatch() { new PatchMutation( key("bar/baz"), TestUtil.wrapObject(map("a", "b", "num", 1)), - FieldMask.fromSet(new HashSet<>(asList(field("a")))), + fieldMask("a"), com.google.firebase.firestore.model.mutation.Precondition.exists(true)); Mutation del = deleteMutation("baz/quux"); Timestamp writeTime = Timestamp.now(); diff --git a/firebase-firestore/src/test/java/com/google/firebase/firestore/model/MutationTest.java b/firebase-firestore/src/test/java/com/google/firebase/firestore/model/MutationTest.java index 855e2c66e23..f9abdb1a968 100644 --- a/firebase-firestore/src/test/java/com/google/firebase/firestore/model/MutationTest.java +++ b/firebase-firestore/src/test/java/com/google/firebase/firestore/model/MutationTest.java @@ -18,6 +18,7 @@ import static com.google.firebase.firestore.testutil.TestUtil.deletedDoc; import static com.google.firebase.firestore.testutil.TestUtil.doc; import static com.google.firebase.firestore.testutil.TestUtil.field; +import static com.google.firebase.firestore.testutil.TestUtil.fieldMask; import static com.google.firebase.firestore.testutil.TestUtil.key; import static com.google.firebase.firestore.testutil.TestUtil.map; import static com.google.firebase.firestore.testutil.TestUtil.mutationResult; @@ -111,7 +112,7 @@ public void testDeletesValuesFromTheFieldMask() { Document baseDoc = doc("collection/key", 0, data); DocumentKey key = key("collection/key"); - FieldMask mask = FieldMask.fromSet(new HashSet<>(Arrays.asList(field("foo.bar")))); + FieldMask mask = fieldMask("foo.bar"); Mutation patch = new PatchMutation(key, ObjectValue.emptyObject(), mask, Precondition.NONE); MaybeDocument patchDoc = patch.applyToLocalView(baseDoc, baseDoc, Timestamp.now()); diff --git a/firebase-firestore/src/testUtil/java/com/google/firebase/firestore/testutil/TestUtil.java b/firebase-firestore/src/testUtil/java/com/google/firebase/firestore/testutil/TestUtil.java index a599d7024fc..27caae9bbdc 100644 --- a/firebase-firestore/src/testUtil/java/com/google/firebase/firestore/testutil/TestUtil.java +++ b/firebase-firestore/src/testUtil/java/com/google/firebase/firestore/testutil/TestUtil.java @@ -74,6 +74,7 @@ import java.util.Collections; import java.util.Comparator; import java.util.HashMap; +import java.util.HashSet; import java.util.List; import java.util.Map; import java.util.Map.Entry; @@ -111,6 +112,14 @@ public static ByteString byteString(int... bytes) { return ByteString.copyFrom(primitive); } + public static FieldMask fieldMask(String... fields) { + FieldPath[] mask = new FieldPath[fields.length]; + for (int i = 0; i < fields.length; i++) { + mask[i] = field(fields[i]); + } + return FieldMask.fromSet(new HashSet<>(Arrays.asList(mask))); + } + public static final Map EMPTY_MAP = new HashMap<>(); public static FieldValue wrap(Object value) { From 124db7fb5dc9c6b08f5e077991c722779fd1d67e Mon Sep 17 00:00:00 2001 From: Rich Gowman Date: Wed, 28 Nov 2018 17:25:25 -0500 Subject: [PATCH 11/11] googleJavaFormat --- .../firestore/model/mutation/ArrayTransformOperation.java | 2 +- .../google/firebase/firestore/local/LocalSerializerTest.java | 2 -- .../java/com/google/firebase/firestore/model/MutationTest.java | 1 - 3 files changed, 1 insertion(+), 4 deletions(-) diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/model/mutation/ArrayTransformOperation.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/model/mutation/ArrayTransformOperation.java index b4c3b81254b..f8f1c7abd8c 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/model/mutation/ArrayTransformOperation.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/model/mutation/ArrayTransformOperation.java @@ -51,7 +51,7 @@ public FieldValue applyToRemoteDocument(FieldValue previousValue, FieldValue tra } @Override - @SuppressWarnings("EqualsGetClass") // subtype-sensitive equality is intended. + @SuppressWarnings("EqualsGetClass") // subtype-sensitive equality is intended. public boolean equals(Object o) { if (this == o) { return true; diff --git a/firebase-firestore/src/test/java/com/google/firebase/firestore/local/LocalSerializerTest.java b/firebase-firestore/src/test/java/com/google/firebase/firestore/local/LocalSerializerTest.java index 9d4dd5ac1e1..a4429a0b27d 100644 --- a/firebase-firestore/src/test/java/com/google/firebase/firestore/local/LocalSerializerTest.java +++ b/firebase-firestore/src/test/java/com/google/firebase/firestore/local/LocalSerializerTest.java @@ -17,7 +17,6 @@ import static com.google.firebase.firestore.testutil.TestUtil.deleteMutation; import static com.google.firebase.firestore.testutil.TestUtil.deletedDoc; import static com.google.firebase.firestore.testutil.TestUtil.doc; -import static com.google.firebase.firestore.testutil.TestUtil.field; import static com.google.firebase.firestore.testutil.TestUtil.fieldMask; import static com.google.firebase.firestore.testutil.TestUtil.key; import static com.google.firebase.firestore.testutil.TestUtil.map; @@ -34,7 +33,6 @@ import com.google.firebase.firestore.model.NoDocument; import com.google.firebase.firestore.model.SnapshotVersion; import com.google.firebase.firestore.model.UnknownDocument; -import com.google.firebase.firestore.model.mutation.FieldMask; import com.google.firebase.firestore.model.mutation.Mutation; import com.google.firebase.firestore.model.mutation.MutationBatch; import com.google.firebase.firestore.model.mutation.PatchMutation; diff --git a/firebase-firestore/src/test/java/com/google/firebase/firestore/model/MutationTest.java b/firebase-firestore/src/test/java/com/google/firebase/firestore/model/MutationTest.java index f9abdb1a968..67c1fba0ac4 100644 --- a/firebase-firestore/src/test/java/com/google/firebase/firestore/model/MutationTest.java +++ b/firebase-firestore/src/test/java/com/google/firebase/firestore/model/MutationTest.java @@ -47,7 +47,6 @@ import com.google.firebase.firestore.model.value.TimestampValue; import java.util.Arrays; import java.util.Collections; -import java.util.HashSet; import java.util.Map; import org.junit.Test; import org.junit.runner.RunWith;