Skip to content

Cleanup a number of warnings in the code. #137

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.

Sign up for GitHub

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

Merged
merged 11 commits into from
Dec 4, 2018
Merged

Cleanup a number of warnings in the code. #137

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
ca50431
Eliminate 'badimport' warning
rsgowman Nov 21, 2018
6b94249
Suppress EqualsGetClass warnings for public classes
rsgowman Nov 21, 2018
f72a5b3
Mark a number of classes final
rsgowman Nov 21, 2018
8a98285
Switch .getClass() to instanceof in some .equals() methods
rsgowman Nov 21, 2018
23e5f94
Provide toString on FieldMask
rsgowman Nov 21, 2018
917b0f8
Fix a 'missing override' warning
rsgowman Nov 22, 2018
34e8b0e
Rework FieldMask to use a Set of FieldPaths rather than a Collection.
rsgowman Nov 22, 2018
024e9fe
Review feedback
rsgowman Nov 23, 2018
4d88c95
Review feedback
rsgowman Nov 27, 2018
497434f
Add TestUtil.fieldMask(String...) to simplify creating FieldMasks.
rsgowman Nov 28, 2018
124db7f
googleJavaFormat
rsgowman Nov 28, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions firebase-firestore/src/main/java/com/google/firebase/firestore/DocumentReference.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -525,6 +525,7 @@ private ListenerRegistration addSnapshotListenerInternal(
}

@Override
@SuppressWarnings("EqualsGetClass") // This class is effectively final.
Copy link
Contributor

Choose a reason for hiding this comment

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

Question: why not use instanceof here and in Query#equals? The article you linked states that instanceof is never worse than getClass:

What about final or effectively-final classes?

In this case there is no disadvantage to the getClass trick - but there’s no great advantage to it either. Most unsafe idioms have circumstances in which they are safe, but this doesn’t change the fact that they are generally unsafe and not worth propagating and legitimizing.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yup, done.

public boolean equals(Object o) {
if (this == o) {
return true;
Expand Down
1 change: 1 addition & 0 deletions firebase-firestore/src/main/java/com/google/firebase/firestore/Query.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
15 changes: 8 additions & 7 deletions firebase-firestore/src/main/java/com/google/firebase/firestore/SetOptions.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -78,13 +79,13 @@ public static SetOptions merge() {
@NonNull
@PublicApi
public static SetOptions mergeFields(List<String> fields) {
List<com.google.firebase.firestore.model.FieldPath> fieldPaths = new ArrayList<>();
Set<com.google.firebase.firestore.model.FieldPath> 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));
}

/**
Expand All @@ -100,13 +101,13 @@ public static SetOptions mergeFields(List<String> fields) {
@NonNull
@PublicApi
public static SetOptions mergeFields(String... fields) {
List<com.google.firebase.firestore.model.FieldPath> fieldPaths = new ArrayList<>();
Set<com.google.firebase.firestore.model.FieldPath> 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));
}

/**
Expand All @@ -121,13 +122,13 @@ public static SetOptions mergeFields(String... fields) {
@NonNull
@PublicApi
public static SetOptions mergeFieldPaths(List<FieldPath> fields) {
List<com.google.firebase.firestore.model.FieldPath> fieldPaths = new ArrayList<>();
Set<com.google.firebase.firestore.model.FieldPath> 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
Expand Down
1 change: 1 addition & 0 deletions firebase-firestore/src/main/java/com/google/firebase/firestore/core/EventManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()) {
Expand Down
14 changes: 7 additions & 7 deletions firebase-firestore/src/main/java/com/google/firebase/firestore/core/UserData.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -72,16 +72,16 @@ public static class ParseAccumulator {
*/
private final Source dataSource;

/** Accumulates a list of the field paths found while parsing the data. */
private final SortedSet<FieldPath> fieldMask;
/** Accumulates a set of the field paths found while parsing the data. */
private final Set<FieldPath> fieldMask;

/** Accumulates a list of field transforms found while parsing the data. */
private final ArrayList<FieldTransform> fieldTransforms;

/** @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<>();
}

Expand Down Expand Up @@ -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));
}

/**
Expand Down Expand Up @@ -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));
}
}

Expand Down
4 changes: 2 additions & 2 deletions firebase-firestore/src/main/java/com/google/firebase/firestore/core/ViewSnapshot.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Expand Down
3 changes: 1 addition & 2 deletions firebase-firestore/src/main/java/com/google/firebase/firestore/local/IndexedQueryEngine.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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();
Expand Down
10 changes: 4 additions & 6 deletions firebase-firestore/src/main/java/com/google/firebase/firestore/model/BasePath.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion firebase-firestore/src/main/java/com/google/firebase/firestore/model/Document.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
2 changes: 1 addition & 1 deletion firebase-firestore/src/main/java/com/google/firebase/firestore/model/DocumentKey.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@
import java.util.List;

/** DocumentKey represents the location of a document in the Firestore database. */
public class DocumentKey implements Comparable<DocumentKey> {
public final class DocumentKey implements Comparable<DocumentKey> {

public static final String KEY_FIELD_NAME = "__name__";

Expand Down
2 changes: 1 addition & 1 deletion firebase-firestore/src/main/java/com/google/firebase/firestore/model/DocumentSet.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<Document> {
public final class DocumentSet implements Iterable<Document> {

/** Returns an empty DocumentSet sorted by the given comparator, then by keys. */
public static DocumentSet emptySet(final Comparator<Document> comparator) {
Expand Down
2 changes: 1 addition & 1 deletion firebase-firestore/src/main/java/com/google/firebase/firestore/model/NoDocument.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down
2 changes: 1 addition & 1 deletion firebase-firestore/src/main/java/com/google/firebase/firestore/model/UnknownDocument.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down
4 changes: 2 additions & 2 deletions ...e/src/main/java/com/google/firebase/firestore/model/mutation/ArrayTransformOperation.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Hrm. I'm not sure about this... We have two implementations of ArrayTransformOperation: ArrayTransformOperation.Union, and ArrayTransformOperation.Remove. They shouldn't be considered equal even if they have the same elements being unioned or removed.

I'm not sure what this means. Honestly, getClass() equality is exactly what we want, and I'm not sure how much I care about the substitutability principle... So we could keep the existing code but suppress the warning... or we could override equals again in the Union / Remove classes to do the right thing. WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed this change is wrong. I suggest suppressing with a comment indicating that type-sensitive equality is intended.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yup.

I suppose the "correct" thing to do would be to roughly duplicate this code out to .Union.equals() and .Remove.equals(). (Or perhaps better, to move the operation into a member and eliminate the subclasses.) But just suppressing the warning seems most pragmatic.

Done.

return false;
}

Expand Down
17 changes: 11 additions & 6 deletions firebase-firestore/src/main/java/com/google/firebase/firestore/model/mutation/FieldMask.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -25,14 +25,14 @@
* 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 static FieldMask fromCollection(Collection<FieldPath> mask) {
public final class FieldMask {
public static FieldMask fromSet(Set<FieldPath> mask) {
return new FieldMask(mask);
}

private final Collection<FieldPath> mask;
private final Set<FieldPath> mask;
Copy link
Contributor

Choose a reason for hiding this comment

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

Optional: what's the reason to prefer Set to a List? AFAICS, mask is never used for lookup; the only access is iterating over all the elements (line 63), for which List will likely be faster.

Copy link
Member Author

Choose a reason for hiding this comment

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

Mostly, it doesn't matter much. But here's the reasons I went with set:

a) The masks are conceptually unordered.
i) which is particularly relevant in the .equals() method, since presumably FieldPath.fromX({"foo", "bar"}) and FieldPath.fromX({"bar", "foo"}) should be considered equivalent.
b) Should avoid accidental duplicates. (Though I don't think duplicates would actually hurt us.)

I don't think speed is a concern in this case: it's hard to imagine having much more single digits of these; plus we're probably about to hit either the network or the disk which should dominate.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree that Set is the natural data structure (and perf, etc. aren't major concerns). That said, we may now get slightly different behavior than on other platforms (e.g. with regard to duplicates), though I don't think that's a big deal, and the Android behavior will probably be more correct.

Copy link
Contributor

Choose a reason for hiding this comment

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

While I agree with the academic distinction we're drawing here, this is a change in behavior that we really should mirror out to the other platforms. Once you consider that, is this change really worth it considering that the code works as is?

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, Collection isn't right, but I'd be ok with List if we don't think ordering/duplicates are overly relevant. If we do think they're relevant, I'm happy to port to the other platforms.

Another option is to leave it as Collection, but eliminate the .equals() method and use the default referential equality instead. Looking at this a bit closer, I don't think we ever actually call FieldMask.equals()... at least not in the test suite. (Tested by changing the implementation to unconditionally throw and re-run all tests.) We do allow callers to fetch the mask, but we only ever iterate over the contents.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm fine with any of these options (including keeping it as a Set as you have it now).

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok; I've gone with set. I'll port to the others.


private FieldMask(Collection<FieldPath> mask) {
private FieldMask(Set<FieldPath> mask) {
this.mask = mask;
}

Expand All @@ -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.
*
Expand All @@ -69,7 +74,7 @@ public int hashCode() {
return mask.hashCode();
}

public Collection<FieldPath> getMask() {
public Set<FieldPath> getMask() {
return mask;
}
}
6 changes: 4 additions & 2 deletions firebase-firestore/src/main/java/com/google/firebase/firestore/remote/RemoteSerializer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -532,11 +534,11 @@ private DocumentMask encodeDocumentMask(FieldMask mask) {

private FieldMask decodeDocumentMask(DocumentMask mask) {
int count = mask.getFieldPathsCount();
List<FieldPath> paths = new ArrayList<>(count);
Set<FieldPath> 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) {
Expand Down
6 changes: 3 additions & 3 deletions firebase-firestore/src/main/java/com/google/firebase/firestore/remote/WatchChange.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<Integer> updatedTargetIds;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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<Integer> targetIds;
private final ByteString resumeToken;
Expand Down
3 changes: 2 additions & 1 deletion ...base-firestore/src/test/java/com/google/firebase/firestore/local/LocalSerializerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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")))),
Copy link
Contributor

Choose a reason for hiding this comment

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

Previously this was already slightly ridiculous but this iteration is now over the top.

Please add a TestUtil.fieldMask(String...) that does the right thing to keep this readable. Alternatively, at least use Collections.singleton(field("a")).

Copy link
Member Author

Choose a reason for hiding this comment

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

Deferred (slightly).

If we go with a list, then I'll just revert this line.
If we go with a set, then I'll implement as part of this PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

com.google.firebase.firestore.model.mutation.Precondition.exists(true));
Mutation del = deleteMutation("baz/quux");
Timestamp writeTime = Timestamp.now();
Expand Down
3 changes: 2 additions & 1 deletion firebase-firestore/src/test/java/com/google/firebase/firestore/model/MutationTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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());
Expand Down
6 changes: 4 additions & 2 deletions firebase-firestore/src/testUtil/java/com/google/firebase/firestore/testutil/TestUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 */
Expand Down Expand Up @@ -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<FieldPath> fieldMaskPaths = new TreeSet<>(merge ? updateMask : objectMask);
Copy link
Contributor

Choose a reason for hiding this comment

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

Hrm. I wonder if this is really necessary. Were we just sorting them before so equals() would work out? If so, then we're already good. What happens if you use a HashSet here?

Copy link
Contributor

Choose a reason for hiding this comment

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

The production code passes a TreeSet so this seems reasonable.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh. I thought it was a HashSet. In any case, I'm fine with using a TreeSet, but the comment about sorting seems confusing / wrong now. Perhaps just remove it?

Copy link
Member Author

Choose a reason for hiding this comment

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

The sort is because we compare the actual protos (post serialization from models to protos) in the serializer tests. Sets/Lists are flattened down to repeated fields in iterator order. That maps well for lists (since order is preserved) but isn't great for sets, since our test would incorrectly state that {foo,bar} != {bar,foo}.

(We couldn't have used Model.equals() here, because of the whole Collection.equals() not making sense when both lists and sets are involved, though if the test happened to use the same type as the model that it had set up, then we would've gotten away with it.)

The old code didn't know if a list or set was used (since a Collection could be either), so it sorted them to ensure everything was fine. The new code (currently) knows that sets are used. Therefore, it needs to sort them to ensure the tests consider the results correct. If we switch to list, I think we could drop the sort altogether.

I've updated the comment to hopefully make this clearer. (Though will remove it if we do the set->list thing.)

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah! Comment makes perfect sense now, thanks.


return new PatchMutation(
key(path),
objectValue,
FieldMask.fromCollection(merge ? updateMask : objectMask),
FieldMask.fromSet(fieldMaskPaths),
merge ? Precondition.NONE : Precondition.exists(true));
}

Expand Down