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 1 commit
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
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
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
10 changes: 5 additions & 5 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 @@ -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<FieldPath> mask) {
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 Down Expand Up @@ -74,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
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