Skip to content

Add missing nullability annotations to Firestore. #600

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 8 commits into from
Jul 12, 2019
Merged

Add missing nullability annotations to Firestore. #600

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
1608731
Add missing nullability annotations to Firestore.
vkryachko Jul 11, 2019
6656cd6
Remove varargs nullability annotations.
vkryachko Jul 11, 2019
27bf36a
gJF
vkryachko Jul 11, 2019
0e25a7a
Add androidx.annotation lint detector.
vkryachko Jul 12, 2019
aaff08d
Change nullability check:
vkryachko Jul 12, 2019
9a0a5c2
Address review comments
vkryachko Jul 12, 2019
66c01ec
Address review comments.
vkryachko Jul 12, 2019
b631b75
ktlintFormat
vkryachko Jul 12, 2019
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: 0 additions & 1 deletion firebase-firestore/firebase-firestore.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ plugins {
firebaseLibrary {
testLab.enabled = true
publishSources = true
staticAnalysis.disableKotlinInteropLint()
}

protobuf {
Expand Down
19 changes: 5 additions & 14 deletions firebase-firestore/src/main/java/com/google/firebase/Timestamp.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@
import android.os.Parcel;
import android.os.Parcelable;
import androidx.annotation.NonNull;
import com.google.firebase.annotations.PublicApi;
import java.util.Date;

/**
Expand All @@ -35,10 +34,9 @@
* https://github.com/google/protobuf/blob/master/src/google/protobuf/timestamp.proto">The
* reference timestamp definition</a>
*/
@PublicApi
public final class Timestamp implements Comparable<Timestamp>, Parcelable {

@PublicApi @NonNull
@NonNull
public static final Parcelable.Creator<Timestamp> CREATOR =
new Parcelable.Creator<Timestamp>() {
@Override
Expand All @@ -61,21 +59,19 @@ public Timestamp[] newArray(int size) {
* Negative second values with fractions must still have non-negative nanoseconds values that
* count forward in time. Must be from 0 to 999,999,999 inclusive.
*/
@PublicApi
public Timestamp(long seconds, int nanoseconds) {
validateRange(seconds, nanoseconds);
this.seconds = seconds;
this.nanoseconds = nanoseconds;
}

protected Timestamp(Parcel in) {
protected Timestamp(@NonNull Parcel in) {
this.seconds = in.readLong();
this.nanoseconds = in.readInt();
}

/** Creates a new timestamp from the given date. */
@PublicApi
public Timestamp(Date date) {
public Timestamp(@NonNull Date date) {
long millis = date.getTime();
long seconds = millis / 1000;
int nanoseconds = (int) (millis % 1000) * 1000000;
Expand All @@ -89,26 +85,22 @@ public Timestamp(Date date) {
}

/** Creates a new timestamp with the current date, with millisecond precision. */
@PublicApi
@NonNull
public static Timestamp now() {
return new Timestamp(new Date());
}

/** Returns the seconds part of the timestamp. */
@PublicApi
public long getSeconds() {
return seconds;
}

/** Returns the sub-second part of the timestamp, in nanoseconds. */
@PublicApi
public int getNanoseconds() {
return nanoseconds;
}

/** Returns a new Date corresponding to this timestamp. This may lose precision. */
@PublicApi
@NonNull
public Date toDate() {
return new Date(seconds * 1000 + (nanoseconds / 1000000));
Expand All @@ -120,14 +112,13 @@ public int describeContents() {
}

@Override
public void writeToParcel(Parcel dest, int flags) {
public void writeToParcel(@NonNull Parcel dest, int flags) {
dest.writeLong(this.seconds);
dest.writeInt(this.nanoseconds);
}

@Override
@PublicApi
public int compareTo(Timestamp other) {
public int compareTo(@NonNull Timestamp other) {
if (seconds == other.seconds) {
return Integer.signum(nanoseconds - other.nanoseconds);
}
Expand Down
5 changes: 0 additions & 5 deletions firebase-firestore/src/main/java/com/google/firebase/firestore/Blob.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,12 +19,10 @@
import androidx.annotation.NonNull;
import androidx.annotation.Nullable;
import androidx.annotation.RestrictTo;
import com.google.firebase.annotations.PublicApi;
import com.google.firebase.firestore.util.Util;
import com.google.protobuf.ByteString;

/** Immutable class representing an array of bytes in Firestore. */
@PublicApi
public class Blob implements Comparable<Blob> {
private final ByteString bytes;

Expand All @@ -39,7 +37,6 @@ private Blob(ByteString bytes) {
* @return The new Blob instance
*/
@NonNull
@PublicApi
public static Blob fromBytes(@NonNull byte[] bytes) {
checkNotNull(bytes, "Provided bytes array must not be null.");
return new Blob(ByteString.copyFrom(bytes));
Expand All @@ -55,7 +52,6 @@ public static Blob fromByteString(@NonNull ByteString bytes) {

/** @return The bytes of this blob as a new byte[] array. */
@NonNull
@PublicApi
public byte[] toBytes() {
return bytes.toByteArray();
}
Expand Down Expand Up @@ -84,7 +80,6 @@ public int hashCode() {
}

@Override
@PublicApi
public int compareTo(@NonNull Blob other) {
int size = Math.min(bytes.size(), other.bytes.size());
for (int i = 0; i < size; i++) {
Expand Down
10 changes: 1 addition & 9 deletions firebase-firestore/src/main/java/com/google/firebase/firestore/CollectionReference.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,13 +17,12 @@
import static com.google.common.base.Preconditions.checkNotNull;

import androidx.annotation.NonNull;
import androidx.annotation.Nullable;
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 not a fan of the Nullable change to androidx (but also can't say I care too strongly). Is there any good reason to prefer the android-specific version of this over the standard one other than just being consistent with the source of NonNull?

In some cases this change would be harmful. For example, we share our CustomClassMapper with the Firestore Java Server SDK. If we had properly annotated methods like this one then we really should use javax.annotation.Nullable because we can't have android dependencies on the server.

Also, the javax version of this seems to be the only Nullable I get with code completion so this is going to rot without some kind of wider announcement to go reconfigure IDEs and add a linter check or something like that to enforce the preference. Is this change really worth all that?

Copy link
Member Author

Choose a reason for hiding this comment

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

You're right, it has no Kotlin behavior difference compared to androidx it's mostly for consistency across all SDKs, so I am ok in principle to revert firestore back to using jsr305 annotations. That said, there is nothing standard about javax annotations, they are part of a JSR that has been dormant since 2006 and is unlikely to become standard at all.

As for androidx.annotations being unusable on the server, I don't think that is true, androidx.annotations is a jar(not aar) that has no dependencies so nothing prevents them from being used on the server. Whether they have any semantic meaning for java server frameworks is a good question though.

That said, I don't feel too strongly but I have a slight preference towards androidx at least for the public APIs for consistency's sake across SDKs, and I am ok to keep javax for all internal APIs if you like. wdyt?

Copy link
Contributor

@wilhuff wilhuff Jul 11, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I'd prefer some outcome where we can set it and forget it as far as Android Studio is concerned. Having different classes for public and private classes is something you can't make automatic--every code-completion import would have to choose and it seems like such a speed bump for something that has no practical value. It also seems low enough value that it's unlikely to get caught in code review.

JSR 305 was never ratified, but it seems effectively standard, especially for Google distributed software. For example, even if we standardize on the androidx one, JSR 305 will be in our classpath because that's what Guava, gRPC, and others use.

I'm more concerned that this standardization isn't going to hold automatically since doing so requires people to change their editor configuration. We need to do something to force the world to track on that: a lint check that disallows javax.annotation.Nullable would be sufficient. Absent that, we'll soon have both androidx and javax flavors in our API and otherwise which I think is worse than mixing the sources of NonNull and Nullable.

To be clear: I'm actually OK with standardizing on the androidx one throughout. The number of classes where we need to interoperate with the server is quite small so we can opt of a lint check in those cases should we implement such a check. My argument is essentially that we shouldn't standardize on androidx without such a lint check.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added lint check, note also that kotlin interop lint checks will provide automatic fix hints(in IDE) with correct(androidx) annotations.

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, I am going to disable it for now as it ripples to other SDKs, and this PR is already large enough. Will re-enable in a follow-up PR

import com.google.android.gms.tasks.Task;
import com.google.firebase.annotations.PublicApi;
import com.google.firebase.firestore.model.DocumentKey;
import com.google.firebase.firestore.model.ResourcePath;
import com.google.firebase.firestore.util.Executors;
import com.google.firebase.firestore.util.Util;
import javax.annotation.Nullable;

/**
* A CollectionReference can be used for adding documents, getting document references, and querying
Expand All @@ -33,7 +32,6 @@
* test mocks. Subclassing is not supported in production code and new SDK releases may break code
* that does so.
*/
@PublicApi
public class CollectionReference extends Query {

CollectionReference(ResourcePath path, FirebaseFirestore firestore) {
Expand All @@ -50,7 +48,6 @@ public class CollectionReference extends Query {

/** @return The ID of the collection. */
@NonNull
@PublicApi
public String getId() {
return query.getPath().getLastSegment();
}
Expand All @@ -63,7 +60,6 @@ public String getId() {
* collection.
*/
@Nullable
@PublicApi
public DocumentReference getParent() {
ResourcePath parentPath = query.getPath().popLast();
if (parentPath.isEmpty()) {
Expand All @@ -80,7 +76,6 @@ public DocumentReference getParent() {
* @return The path of this collection.
*/
@NonNull
@PublicApi
public String getPath() {
return query.getPath().canonicalString();
}
Expand All @@ -92,7 +87,6 @@ public String getPath() {
* @return A DocumentReference pointing to a new document with an auto-generated ID.
*/
@NonNull
@PublicApi
public DocumentReference document() {
return document(Util.autoId());
}
Expand All @@ -105,7 +99,6 @@ public DocumentReference document() {
* @return The DocumentReference instance.
*/
@NonNull
@PublicApi
public DocumentReference document(@NonNull String documentPath) {
checkNotNull(documentPath, "Provided document path must not be null.");
return DocumentReference.forPath(
Expand All @@ -121,7 +114,6 @@ public DocumentReference document(@NonNull String documentPath) {
* @return A Task that will be resolved with the DocumentReference of the newly created document.
*/
@NonNull
@PublicApi
public Task<DocumentReference> add(@NonNull Object data) {
checkNotNull(data, "Provided data must not be null.");
final DocumentReference ref = document();
Expand Down
9 changes: 1 addition & 8 deletions firebase-firestore/src/main/java/com/google/firebase/firestore/DocumentChange.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,15 +17,14 @@
import static com.google.firebase.firestore.util.Assert.hardAssert;

import androidx.annotation.NonNull;
import androidx.annotation.Nullable;
import androidx.annotation.VisibleForTesting;
import com.google.firebase.annotations.PublicApi;
import com.google.firebase.firestore.core.DocumentViewChange;
import com.google.firebase.firestore.core.ViewSnapshot;
import com.google.firebase.firestore.model.Document;
import com.google.firebase.firestore.model.DocumentSet;
import java.util.ArrayList;
import java.util.List;
import javax.annotation.Nullable;

/**
* A DocumentChange represents a change to the documents matching a query. It contains the document
Expand All @@ -35,10 +34,8 @@
* test mocks. Subclassing is not supported in production code and new SDK releases may break code
* that does so.
*/
@PublicApi
public class DocumentChange {
/** An enumeration of snapshot diff types. */
@PublicApi
public enum Type {
/** Indicates a new document was added to the set of documents matching the query. */
ADDED,
Expand Down Expand Up @@ -91,7 +88,6 @@ public int hashCode() {
}

@NonNull
@PublicApi
public Type getType() {
return type;
}
Expand All @@ -104,7 +100,6 @@ public Type getType() {
* Type.REMOVED).
*/
@NonNull
@PublicApi
public QueryDocumentSnapshot getDocument() {
return document;
}
Expand All @@ -114,7 +109,6 @@ public QueryDocumentSnapshot getDocument() {
* (i.e. supposing that all prior DocumentChange objects have been applied). Returns -1 for
* 'added' events.
*/
@PublicApi
public int getOldIndex() {
return oldIndex;
}
Expand All @@ -124,7 +118,6 @@ public int getOldIndex() {
* supposing that all prior DocumentChange objects and the current DocumentChange object have been
* applied). Returns -1 for 'removed' events.
*/
@PublicApi
public int getNewIndex() {
return newIndex;
}
Expand Down
2 changes: 0 additions & 2 deletions firebase-firestore/src/main/java/com/google/firebase/firestore/DocumentId.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@

package com.google.firebase.firestore;

import com.google.firebase.annotations.PublicApi;
import java.lang.annotation.ElementType;
import java.lang.annotation.Retention;
import java.lang.annotation.RetentionPolicy;
Expand Down Expand Up @@ -43,7 +42,6 @@
* WriteBatch#set}), the property annotated by @DocumentId is ignored, which allows writing the POJO
* back to any document, even if it's not the origin of the POJO.
*/
@PublicApi
@Retention(RetentionPolicy.RUNTIME)
@Target({ElementType.FIELD, ElementType.METHOD})
public @interface DocumentId {}
Loading