Skip to content

FIS error handling #911

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 30 commits into from
Oct 16, 2019
Merged

FIS error handling #911

Show file tree
Hide file tree
Changes from 27 commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
49708d2
Simplifying FirebaseInstallations class by adding listeners.
ankitaj224 Sep 25, 2019
a987647
Splitting multiple classes/interfaces into separate files.
ankitaj224 Sep 25, 2019
1d9b1ca
Addressing ciaran's comments to return same token if multiple getAuth…
ankitaj224 Sep 26, 2019
28d0d85
Addressing ciaran's comments to use a boolean to refresh auth token
ankitaj224 Sep 27, 2019
126e372
Cleaning up instrumented test file.
ankitaj224 Sep 27, 2019
c39cdb8
Ciaran's comments on synchronization and runnable.
ankitaj224 Sep 29, 2019
c21a59e
Ciaran's comments on synchronization and runnable.
ankitaj224 Sep 29, 2019
7a46ab0
Merge branch 'listeners' of github.com:firebase/firebase-android-sdk …
ankitaj224 Sep 29, 2019
314b3bb
Fixing precheck failures.
ankitaj224 Sep 30, 2019
715d427
Addressing Ciaran's comment on mock utils.
ankitaj224 Sep 30, 2019
bb322d0
Cleaning doRegistration method.
ankitaj224 Oct 1, 2019
1fd193b
Fixing nit comments.
ankitaj224 Oct 8, 2019
e2d3631
Addressing ciaran's comment on satisfying immediate getId response.
ankitaj224 Oct 8, 2019
e68e599
Fixing FISClient to correctly parse expiration timestamp. (#848)
ankitaj224 Oct 9, 2019
58c8efd
Updating getAuthToken to return creation timestamp (#884)
ankitaj224 Oct 9, 2019
56d1eab
Propagating the exceptions to the clients. (#856)
ankitaj224 Oct 10, 2019
c8367dc
Implemneting retry for FIS Client.
ankitaj224 Oct 10, 2019
72e335d
Merge branch 'listeners' of github.com:firebase/firebase-android-sdk …
ankitaj224 Oct 10, 2019
560fba1
Addressing ciaran's comments.
ankitaj224 Oct 10, 2019
3ac3da2
Merge branch 'fis_sdk' of github.com:firebase/firebase-android-sdk in…
ankitaj224 Oct 10, 2019
d9e5c22
Merge conflicts.
ankitaj224 Oct 10, 2019
f57137c
Nit fixes
ankitaj224 Oct 10, 2019
1d51edf
Extract FID from FIS createInstallation response (#888)
ankitaj224 Oct 14, 2019
0c0b9c2
Merge branch 'listeners' of github.com:firebase/firebase-android-sdk …
ankitaj224 Oct 14, 2019
0407614
Fixing verify format issue.
ankitaj224 Oct 14, 2019
2e7b881
1. Mark PersistedInstallationEntry status UNREGISTERED incase of 500 …
ankitaj224 Oct 14, 2019
615bdf4
Merge branch 'fis_sdk' of github.com:firebase/firebase-android-sdk in…
ankitaj224 Oct 14, 2019
dfa85e7
Addressing Ciaran & Rayo's comments.
ankitaj224 Oct 14, 2019
97b616c
Acceptiong Rayo's suggestion on code improvement.
ankitaj224 Oct 15, 2019
31d441a
Introducing responseCode to validate if the request was successful.
ankitaj224 Oct 15, 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
10 changes: 6 additions & 4 deletions firebase-installations/api.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ package com.google.firebase.installations.local {
method @NonNull public static com.google.firebase.installations.local.PersistedInstallationEntry.Builder builder();
method @Nullable public abstract String getAuthToken();
method public abstract long getExpiresInSecs();
method @Nullable public abstract String getFISException();
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should be #getFisException

Compare:
http://go/java-style#s5.3-camel-case

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.thanks.

method @Nullable public abstract String getFirebaseInstallationId();
method @Nullable public abstract String getRefreshToken();
method @NonNull public abstract com.google.firebase.installations.local.PersistedInstallation.RegistrationStatus getRegistrationStatus();
Expand All @@ -60,6 +61,7 @@ package com.google.firebase.installations.local {
method @NonNull public abstract com.google.firebase.installations.local.PersistedInstallationEntry build();
method @NonNull public abstract com.google.firebase.installations.local.PersistedInstallationEntry.Builder setAuthToken(@Nullable String);
method @NonNull public abstract com.google.firebase.installations.local.PersistedInstallationEntry.Builder setExpiresInSecs(long);
method @NonNull public abstract com.google.firebase.installations.local.PersistedInstallationEntry.Builder setFISException(@Nullable String);
method @NonNull public abstract com.google.firebase.installations.local.PersistedInstallationEntry.Builder setFirebaseInstallationId(@NonNull String);
method @NonNull public abstract com.google.firebase.installations.local.PersistedInstallationEntry.Builder setRefreshToken(@Nullable String);
method @NonNull public abstract com.google.firebase.installations.local.PersistedInstallationEntry.Builder setRegistrationStatus(@NonNull com.google.firebase.installations.local.PersistedInstallation.RegistrationStatus);
Expand All @@ -80,10 +82,10 @@ package com.google.firebase.installations.remote {
public abstract class InstallationResponse {
ctor public InstallationResponse();
method @NonNull public static com.google.firebase.installations.remote.InstallationResponse.Builder builder();
method @NonNull public abstract InstallationTokenResult getAuthToken();
method @NonNull public abstract String getFid();
method @NonNull public abstract String getRefreshToken();
method @NonNull public abstract String getUri();
method @Nullable public abstract InstallationTokenResult getAuthToken();
method @Nullable public abstract String getFid();
method @Nullable public abstract String getRefreshToken();
method @Nullable public abstract String getUri();
method @NonNull public abstract com.google.firebase.installations.remote.InstallationResponse.Builder toBuilder();
}

Expand Down
26 changes: 26 additions & 0 deletions ...oidTest/java/com/google/firebase/installations/FirebaseInstallationsInstrumentedTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@
import com.google.firebase.installations.local.PersistedInstallation.RegistrationStatus;
import com.google.firebase.installations.local.PersistedInstallationEntry;
import com.google.firebase.installations.remote.FirebaseInstallationServiceClient;
import com.google.firebase.installations.remote.InstallationResponse;
import java.util.concurrent.ExecutionException;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.LinkedBlockingQueue;
Expand Down Expand Up @@ -291,6 +292,31 @@ public void testGetId_PersistedInstallationOk_BackendError() throws Exception {
assertThat(updatedInstallationEntry).hasRegistrationStatus(RegistrationStatus.REGISTER_ERROR);
}

@Test
public void testGetId_500ServerError_UnregisteredFID() throws Exception {
when(backendClientReturnsOk.createFirebaseInstallation(
Copy link
Contributor

Choose a reason for hiding this comment

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

where does this test stub a 500 error?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry for the confusion. Added a comment & changed the test name. Does this makes sense?

anyString(), anyString(), anyString(), anyString()))
.thenReturn(InstallationResponse.builder().build());

FirebaseInstallations firebaseInstallations =
new FirebaseInstallations(
executor, firebaseApp, backendClientReturnsOk, persistedInstallation, mockUtils);

Tasks.await(firebaseInstallations.getId());

PersistedInstallationEntry entryValue =
persistedInstallation.readPersistedInstallationEntryValue();
assertThat(entryValue).hasFid(TEST_FID_1);

// Waiting for Task that registers FID on the FIS Servers
executor.awaitTermination(500, TimeUnit.MILLISECONDS);

PersistedInstallationEntry updatedInstallationEntry =
persistedInstallation.readPersistedInstallationEntryValue();
assertThat(updatedInstallationEntry).hasFid(TEST_FID_1);
assertThat(updatedInstallationEntry).hasRegistrationStatus(RegistrationStatus.UNREGISTERED);
}

@Test
public void testGetId_PersistedInstallationError_BackendOk() throws InterruptedException {
FirebaseInstallations firebaseInstallations =
Expand Down
29 changes: 19 additions & 10 deletions ...-installations/src/main/java/com/google/firebase/installations/FirebaseInstallations.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -218,12 +218,18 @@ private final void doRegistration() {
persistedInstallation.readPersistedInstallationEntryValue();

// New FID needs to be created
if (persistedInstallationEntry.isErrored() || persistedInstallationEntry.isNotGenerated()) {
if (persistedInstallationEntry.isNotGenerated()) {
String fid = utils.createRandomFid();
persistFid(fid);
persistedInstallationEntry = persistedInstallation.readPersistedInstallationEntryValue();
}

if (persistedInstallationEntry.isErrored()) {
throw new FirebaseInstallationsException(
persistedInstallationEntry.getFISException(),
FirebaseInstallationsException.Status.SDK_INTERNAL_ERROR);
}

triggerOnStateReached(persistedInstallationEntry);

// FID needs to be registered
Expand Down Expand Up @@ -262,6 +268,7 @@ private final void doRegistration() {
PersistedInstallationEntry errorInstallationEntry =
persistedInstallationEntry
.toBuilder()
.setFISException(e.getMessage())
.setRegistrationStatus(RegistrationStatus.REGISTER_ERROR)
.build();
persistedInstallation.insertOrUpdatePersistedInstallationEntry(errorInstallationEntry);
Expand Down Expand Up @@ -296,15 +303,17 @@ private Void registerAndSaveFid(PersistedInstallationEntry persistedInstallation
/*fid= */ persistedInstallationEntry.getFirebaseInstallationId(),
/*projectID= */ firebaseApp.getOptions().getProjectId(),
/*appId= */ getApplicationId());
persistedInstallation.insertOrUpdatePersistedInstallationEntry(
PersistedInstallationEntry.builder()
.setFirebaseInstallationId(installationResponse.getFid())
.setRegistrationStatus(RegistrationStatus.REGISTERED)
.setAuthToken(installationResponse.getAuthToken().getToken())
.setRefreshToken(installationResponse.getRefreshToken())
.setExpiresInSecs(installationResponse.getAuthToken().getTokenExpirationTimestamp())
.setTokenCreationEpochInSecs(creationTime)
.build());
if (installationResponse.getFid() != null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

fyi:
If a #createInstallation request gets a successful response, the field "fid" should always be set.

What happens here if the field "fid" is not set? What state will the SDK end up in?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is to differentiate between 200, 5xx & 4xx errors.

200 - createInstallation successful response - fid is not empty
5xx - retry once - empty response & fid
4xx - throws exception

Copy link
Contributor

Choose a reason for hiding this comment

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

If that is what you are doing, then I would recommend your code to express that.
In this case, I would recommend that you check for the 200 HTTP-status instead of the "fid" field.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For that: I have to include Response Code as a field in InstallationResponse. Is that what you recommend?

Copy link
Contributor

Choose a reason for hiding this comment

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

since y'all control both sides of this exchange we should be able to use and trust the status codes. imo having canonical status codes as a classification for installation responses is a good idea.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ack. PTAL.

persistedInstallation.insertOrUpdatePersistedInstallationEntry(
PersistedInstallationEntry.builder()
.setFirebaseInstallationId(installationResponse.getFid())
.setRegistrationStatus(RegistrationStatus.REGISTERED)
.setAuthToken(installationResponse.getAuthToken().getToken())
.setRefreshToken(installationResponse.getRefreshToken())
.setExpiresInSecs(installationResponse.getAuthToken().getTokenExpirationTimestamp())
.setTokenCreationEpochInSecs(creationTime)
.build());
}

} catch (FirebaseException exception) {
throw new FirebaseInstallationsException(
Expand Down
7 changes: 4 additions & 3 deletions firebase-installations/src/main/java/com/google/firebase/installations/Utils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,9 +53,10 @@ class Utils {
* #AUTH_TOKEN_EXPIRATION_BUFFER_IN_SECS}.
*/
public boolean isAuthTokenExpired(PersistedInstallationEntry persistedInstallationEntry) {
return persistedInstallationEntry.getTokenCreationEpochInSecs()
+ persistedInstallationEntry.getExpiresInSecs()
> currentTimeInSecs() + AUTH_TOKEN_EXPIRATION_BUFFER_IN_SECS;
return persistedInstallationEntry.isRegistered()
&& persistedInstallationEntry.getTokenCreationEpochInSecs()
+ persistedInstallationEntry.getExpiresInSecs()
> currentTimeInSecs() + AUTH_TOKEN_EXPIRATION_BUFFER_IN_SECS;
}

/** Returns current time in seconds. */
Expand Down
6 changes: 6 additions & 0 deletions ...ons/src/main/java/com/google/firebase/installations/local/PersistedInstallationEntry.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,9 @@ public abstract class PersistedInstallationEntry {

public abstract long getTokenCreationEpochInSecs();

@Nullable
public abstract String getFISException();
Copy link
Contributor

Choose a reason for hiding this comment

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

are we exposing this to clients? Having exception == String is weird. How about just calling it error message?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, the stored error message. Changed it. PTAL.

Idea: getId is non-blocking call. After we return the id, we encounter some error during registration. We persist the error message & return it to the clients next time getId is called.


public boolean isRegistered() {
return getRegistrationStatus() == PersistedInstallation.RegistrationStatus.REGISTERED;
}
Expand Down Expand Up @@ -90,6 +93,9 @@ public abstract Builder setRegistrationStatus(
@NonNull
public abstract Builder setTokenCreationEpochInSecs(long value);

@NonNull
public abstract Builder setFISException(@Nullable String value);
Copy link
Contributor

Choose a reason for hiding this comment

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

why can this be @nullable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When PersistedInstallationEntry stores a successfully registered entry.


@NonNull
public abstract PersistedInstallationEntry build();
}
Expand Down
3 changes: 2 additions & 1 deletion ...main/java/com/google/firebase/installations/remote/FirebaseInstallationServiceClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,8 @@ public InstallationResponse createFirebaseInstallation(
throw new FirebaseException(readErrorResponse(httpsURLConnection));
Copy link
Contributor

Choose a reason for hiding this comment

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

referring to the 4 lines above:
case 500: retryCount++;

Is it possible for us to treat all server-errors (i.e. 5XX) like this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. PTAL.

}
}
throw new FirebaseException(INTERNAL_SERVER_ERROR_MESSAGE);
// Return empty installation response after max retries
return InstallationResponse.builder().build();
} catch (IOException e) {
throw new FirebaseException(String.format(NETWORK_ERROR_MESSAGE, e.getMessage()));
}
Expand Down
9 changes: 5 additions & 4 deletions ...llations/src/main/java/com/google/firebase/installations/remote/InstallationResponse.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,22 +15,23 @@
package com.google.firebase.installations.remote;

import androidx.annotation.NonNull;
import androidx.annotation.Nullable;
import com.google.auto.value.AutoValue;
import com.google.firebase.installations.InstallationTokenResult;

@AutoValue
public abstract class InstallationResponse {

@NonNull
@Nullable
public abstract String getUri();

@NonNull
@Nullable
public abstract String getFid();

@NonNull
@Nullable
public abstract String getRefreshToken();

@NonNull
@Nullable
public abstract InstallationTokenResult getAuthToken();

@NonNull
Expand Down