Add missing nullability annotations to Firestore. #600

vkryachko · 2019-07-11T16:13:39Z

Additionally remove uses of the redundant @PublicApi annotation.

Additionally remove uses of the redundant `@PublicApi` annotation.

wilhuff · 2019-07-11T16:27:49Z

firebase-firestore/src/main/java/com/google/firebase/firestore/DocumentReference.java

  public Task<Void> update(
-      @NonNull String field, @Nullable Object value, Object... moreFieldsAndValues) {
+      @NonNull String field, @Nullable Object value, @NonNull Object... moreFieldsAndValues) {


This seems incorrect. null is a valid value in here. Does the @NonNull apply to elements of the argument list or to the argument list itself?

It is for the argument list itself, there is no way to annotate members of the list unfortunately, so the kotlin signature will be

fun update(field: String, value: Any?, vararg moreFieldsAndValues: Any!) // note the !

Furthermore I only added this to silence the lint check, as this annotation has no effect on the above signature.

I removed these annotations now and updated the check to ignore varargs parameters.

As they don't have any effect on the resulting kotlin nullability.

wilhuff · 2019-07-11T18:02:43Z

firebase-firestore/src/main/java/com/google/firebase/firestore/CollectionReference.java

@@ -17,13 +17,12 @@
 import static com.google.common.base.Preconditions.checkNotNull;

 import androidx.annotation.NonNull;
+import androidx.annotation.Nullable;


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?

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?

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.

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

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

wilhuff · 2019-07-11T18:04:20Z

Aside from the question about which Nullable to use, LGTM

The detector finds usages of non-androidx, non-android.support nullability annotations and provides automatic ide fixes for such violations. Android Support annotations are allowed as a transitional step until internal androidx migration is fully completed.

* disable by default(will enable in a separate PR) * ignore kotlin source files

vkryachko · 2019-07-12T15:16:43Z

/assign @wilhuff

vkryachko · 2019-07-12T15:17:13Z

/unassign @vkryachko

tools/lint/src/main/kotlin/NonAndroidxNullabilityDetector.kt

wilhuff

LGTM, though I'd recommend pulling the lint check out into a separate PR altogether.

tools/lint/src/main/kotlin/NonAndroidxNullabilityDetector.kt

tools/lint/src/test/kotlin/AndroidxNullabilityTests.kt

@hide

* Add proper nullability annotations to firebase-common. (#599) * Add missing nullability annotations to Functions. (#601) * Add nullability annotations, remove `@PublicApi` annotations for Storage. (#602) * Add missing nullability annotations to Storage. * Remove redundant `@PublicApi` annotation. * Address review comments. * Add nullability annotations, remove `@PublicApi` annotations for RTDB. (#603) * Add missing nullability annotations to RTDB. * Remove redundant `@PublicApi` annotations. * Update changelog. * Add missing nullability annotations to Firestore. (#600) * Add missing nullability annotations to Firestore. Additionally remove uses of the redundant `@PublicApi` annotation. * Remove varargs nullability annotations. As they don't have any effect on the resulting kotlin nullability. * gJF * Add androidx.annotation lint detector. The detector finds usages of non-androidx, non-android.support nullability annotations and provides automatic ide fixes for such violations. Android Support annotations are allowed as a transitional step until internal androidx migration is fully completed. * Change nullability check: * disable by default(will enable in a separate PR) * ignore kotlin source files * Address review comments * Address review comments. * ktlintFormat * @hide abt public classes (#609) * Fix timeouts in Functions. (#606) This commit resolves #604 by setting both the read and call timeouts. The connect and write timeouts are left at their default values of 10 seconds. * Report the correct version for the RTDB (#605) * Add missing package-info.java files for A/B Testing. The SDK doesn't have user-visible API, so it should be correctly annotated with javadoc's @hide. * Add missing nullability annotations to Remote Config. (#608)

googlebot added the cla: yes Override cla label Jul 11, 2019

google-oss-bot added the size/L label Jul 11, 2019

vkryachko changed the title ~~Add proper nullability annotations to Firestore.~~ Add missing nullability annotations to Firestore. Jul 11, 2019

Add missing nullability annotations to Firestore.

1608731

Additionally remove uses of the redundant `@PublicApi` annotation.

vkryachko force-pushed the vk.firestore_nullability branch from 12f61ab to 1608731 Compare July 11, 2019 16:20

wilhuff reviewed Jul 11, 2019

View reviewed changes

wilhuff assigned vkryachko Jul 11, 2019

wilhuff added the api: firestore label Jul 11, 2019

vkryachko added 2 commits July 11, 2019 13:30

Remove varargs nullability annotations.

6656cd6

As they don't have any effect on the resulting kotlin nullability.

gJF

27bf36a

vkryachko requested a review from rlazo July 11, 2019 17:46

vkryachko assigned wilhuff and unassigned vkryachko Jul 11, 2019

vkryachko requested a review from wilhuff July 11, 2019 17:46

wilhuff reviewed Jul 11, 2019

View reviewed changes

wilhuff assigned vkryachko and unassigned wilhuff Jul 11, 2019

vkryachko assigned wilhuff and unassigned vkryachko Jul 11, 2019

vkryachko requested a review from wilhuff July 11, 2019 18:45

wilhuff assigned vkryachko and unassigned wilhuff Jul 11, 2019

google-oss-bot added size/XL and removed size/L labels Jul 12, 2019

Change nullability check:

aaff08d

* disable by default(will enable in a separate PR) * ignore kotlin source files

google-oss-bot assigned wilhuff Jul 12, 2019

vkryachko removed their assignment Jul 12, 2019

vkryachko self-assigned this Jul 12, 2019

google-oss-bot unassigned vkryachko Jul 12, 2019

rlazo reviewed Jul 12, 2019

View reviewed changes

tools/lint/src/main/kotlin/NonAndroidxNullabilityDetector.kt Show resolved Hide resolved

tools/lint/src/main/kotlin/NonAndroidxNullabilityDetector.kt Outdated Show resolved Hide resolved

Address review comments

9a0a5c2

vkryachko requested a review from rlazo July 12, 2019 15:46

wilhuff approved these changes Jul 12, 2019

View reviewed changes

tools/lint/src/main/kotlin/NonAndroidxNullabilityDetector.kt Outdated Show resolved Hide resolved

tools/lint/src/main/kotlin/NonAndroidxNullabilityDetector.kt Outdated Show resolved Hide resolved

tools/lint/src/test/kotlin/AndroidxNullabilityTests.kt Outdated Show resolved Hide resolved

wilhuff assigned vkryachko and unassigned wilhuff Jul 12, 2019

Address review comments.

66c01ec

rlazo approved these changes Jul 12, 2019

View reviewed changes

ktlintFormat

b631b75

vkryachko merged commit 1557b09 into kotlin_nullability Jul 12, 2019

vkryachko deleted the vk.firestore_nullability branch September 25, 2019 20:35

firebase locked and limited conversation to collaborators Oct 9, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add missing nullability annotations to Firestore. #600

Add missing nullability annotations to Firestore. #600

Uh oh!

vkryachko commented Jul 11, 2019 •

edited

Loading

Uh oh!

wilhuff Jul 11, 2019

Uh oh!

vkryachko Jul 11, 2019

Uh oh!

wilhuff Jul 11, 2019

Uh oh!

vkryachko Jul 11, 2019

Uh oh!

wilhuff Jul 11, 2019 •

edited

Loading

Uh oh!

vkryachko Jul 12, 2019

Uh oh!

vkryachko Jul 12, 2019

Uh oh!

wilhuff commented Jul 11, 2019

Uh oh!

vkryachko commented Jul 12, 2019

Uh oh!

vkryachko commented Jul 12, 2019

Uh oh!

Uh oh!

Uh oh!

wilhuff left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Add missing nullability annotations to Firestore. #600

Add missing nullability annotations to Firestore. #600

Uh oh!

Conversation

vkryachko commented Jul 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wilhuff Jul 11, 2019

Choose a reason for hiding this comment

Uh oh!

vkryachko Jul 11, 2019

Choose a reason for hiding this comment

Uh oh!

wilhuff Jul 11, 2019

Choose a reason for hiding this comment

Uh oh!

vkryachko Jul 11, 2019

Choose a reason for hiding this comment

Uh oh!

wilhuff Jul 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vkryachko Jul 12, 2019

Choose a reason for hiding this comment

Uh oh!

vkryachko Jul 12, 2019

Choose a reason for hiding this comment

Uh oh!

wilhuff commented Jul 11, 2019

Uh oh!

vkryachko commented Jul 12, 2019

Uh oh!

vkryachko commented Jul 12, 2019

Uh oh!

Uh oh!

Uh oh!

wilhuff left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vkryachko commented Jul 11, 2019 •

edited

Loading

wilhuff Jul 11, 2019 •

edited

Loading