From 18cc0299a709f21b36c7bf6afc4a370b4c94549a Mon Sep 17 00:00:00 2001 From: Ankita Jhawar Date: Tue, 17 Sep 2019 10:08:32 -0700 Subject: [PATCH 01/11] Refresh Auth token if expired during getId call. --- ...FirebaseInstallationsInstrumentedTest.java | 40 +++++++++++-- .../FisAndroidTestConstants.java | 1 + .../installations/FirebaseInstallations.java | 57 ++++++++++++++----- 3 files changed, 78 insertions(+), 20 deletions(-) diff --git a/firebase-installations/src/androidTest/java/com/google/firebase/installations/FirebaseInstallationsInstrumentedTest.java b/firebase-installations/src/androidTest/java/com/google/firebase/installations/FirebaseInstallationsInstrumentedTest.java index 502bef1b66a..78ed3022562 100644 --- a/firebase-installations/src/androidTest/java/com/google/firebase/installations/FirebaseInstallationsInstrumentedTest.java +++ b/firebase-installations/src/androidTest/java/com/google/firebase/installations/FirebaseInstallationsInstrumentedTest.java @@ -23,6 +23,7 @@ import static com.google.firebase.installations.FisAndroidTestConstants.TEST_AUTH_TOKEN_4; import static com.google.firebase.installations.FisAndroidTestConstants.TEST_CREATION_TIMESTAMP_1; import static com.google.firebase.installations.FisAndroidTestConstants.TEST_CREATION_TIMESTAMP_2; +import static com.google.firebase.installations.FisAndroidTestConstants.TEST_CREATION_TIMESTAMP_3; import static com.google.firebase.installations.FisAndroidTestConstants.TEST_FID_1; import static com.google.firebase.installations.FisAndroidTestConstants.TEST_PROJECT_ID; import static com.google.firebase.installations.FisAndroidTestConstants.TEST_REFRESH_TOKEN; @@ -32,6 +33,7 @@ import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyString; import static org.mockito.Mockito.doAnswer; +import static org.mockito.Mockito.never; import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; @@ -116,7 +118,7 @@ public class FirebaseInstallationsInstrumentedTest { .setFirebaseInstallationId(TEST_FID_1) .setAuthToken(TEST_AUTH_TOKEN_2) .setRefreshToken(TEST_REFRESH_TOKEN) - .setTokenCreationEpochInSecs(TEST_CREATION_TIMESTAMP_2) + .setTokenCreationEpochInSecs(TEST_CREATION_TIMESTAMP_3) .setExpiresInSecs(TEST_TOKEN_EXPIRATION_TIMESTAMP) .setRegistrationStatus(PersistedFid.RegistrationStatus.REGISTERED) .build(); @@ -277,6 +279,36 @@ public void testGetId_PersistedFidError_BackendOk() throws InterruptedException } } + @Test + public void testGetId_expiredAuthToken_refreshesAuthToken() throws Exception { + // Update local storage with fid entry that has auth token expired. + persistedFid.insertOrUpdatePersistedFidEntry(EXPIRED_AUTH_TOKEN_ENTRY); + FirebaseInstallations firebaseInstallations = + new FirebaseInstallations( + mockClock, executor, firebaseApp, backendClientReturnsOk, persistedFid, mockUtils); + + assertWithMessage("getId Task fails.") + .that(Tasks.await(firebaseInstallations.getId())) + .isNotEmpty(); + PersistedFidEntry entryValue = persistedFid.readPersistedFidEntryValue(); + assertWithMessage("Persisted Fid doesn't match") + .that(entryValue.getFirebaseInstallationId()) + .isEqualTo(TEST_FID_1); + + // Waiting for Task that generates auth token with the FIS Servers + executor.awaitTermination(500, TimeUnit.MILLISECONDS); + + // Validate that registration is complete with updated auth token + PersistedFidEntry updatedFidEntry = persistedFid.readPersistedFidEntryValue(); + assertWithMessage("Persisted Fid doesn't match") + .that(updatedFidEntry) + .isEqualTo(UPDATED_AUTH_TOKEN_FID_ENTRY); + verify(backendClientReturnsOk, never()) + .createFirebaseInstallation(TEST_API_KEY, TEST_FID_1, TEST_PROJECT_ID, TEST_APP_ID_1); + verify(backendClientReturnsOk, times(1)) + .generateAuthToken(TEST_API_KEY, TEST_FID_1, TEST_PROJECT_ID, TEST_REFRESH_TOKEN); + } + @Test public void testGetAuthToken_fidDoesNotExist_successful() throws Exception { FirebaseInstallations firebaseInstallations = @@ -409,11 +441,7 @@ public void testGetAuthToken_multipleCallsDoNotForceRefresh_fetchedNewTokenOnce( // triggered simultaneously. Task2 waits for Task1 to complete. On Task1 completion, task2 reads // the UPDATED_AUTH_TOKEN_FID_ENTRY by Task1 on execution. when(mockPersistedFid.readPersistedFidEntryValue()) - .thenReturn( - EXPIRED_AUTH_TOKEN_ENTRY, - EXPIRED_AUTH_TOKEN_ENTRY, - EXPIRED_AUTH_TOKEN_ENTRY, - UPDATED_AUTH_TOKEN_FID_ENTRY); + .thenReturn(EXPIRED_AUTH_TOKEN_ENTRY, UPDATED_AUTH_TOKEN_FID_ENTRY); FirebaseInstallations firebaseInstallations = new FirebaseInstallations( mockClock, executor, firebaseApp, backendClientReturnsOk, mockPersistedFid, mockUtils); diff --git a/firebase-installations/src/androidTest/java/com/google/firebase/installations/FisAndroidTestConstants.java b/firebase-installations/src/androidTest/java/com/google/firebase/installations/FisAndroidTestConstants.java index e77a2b1155b..a5afc6025f3 100644 --- a/firebase-installations/src/androidTest/java/com/google/firebase/installations/FisAndroidTestConstants.java +++ b/firebase-installations/src/androidTest/java/com/google/firebase/installations/FisAndroidTestConstants.java @@ -36,4 +36,5 @@ public final class FisAndroidTestConstants { public static final long TEST_CREATION_TIMESTAMP_1 = 2000L; public static final long TEST_CREATION_TIMESTAMP_2 = 2000L; + public static final long TEST_CREATION_TIMESTAMP_3 = 2L; } diff --git a/firebase-installations/src/main/java/com/google/firebase/installations/FirebaseInstallations.java b/firebase-installations/src/main/java/com/google/firebase/installations/FirebaseInstallations.java index 4072bdc1c06..55c367ac0a1 100644 --- a/firebase-installations/src/main/java/com/google/firebase/installations/FirebaseInstallations.java +++ b/firebase-installations/src/main/java/com/google/firebase/installations/FirebaseInstallations.java @@ -235,7 +235,8 @@ private void persistFid(String fid) throws FirebaseInstallationsException { } /** - * Registers the FID with FIS servers if FID is in UNREGISTERED state. + * Registers the FID with FIS servers if FID is in UNREGISTERED state. Also, refreshes auth token + * if expired. * *

Updates FID registration status to PENDING to avoid multiple network calls to FIS Servers. */ @@ -243,23 +244,36 @@ private Task registerFidIfNecessary( PersistedFidEntry persistedFidEntry, AwaitListener listener) { String fid = persistedFidEntry.getFirebaseInstallationId(); - // Check if the fid is unregistered - if (persistedFidEntry.getRegistrationStatus() == RegistrationStatus.UNREGISTERED) { - updatePersistedFidWithPendingStatus(fid); - executeFidRegistration(persistedFidEntry, listener); - } else { - updateAwaitListenerIfRegisteredFid(persistedFidEntry, listener); + if (!isFidUnregisteredOrAuthTokenExpired(persistedFidEntry)) { + return updateAwaitListenerIfRegisteredFid(persistedFidEntry, listener); } + updatePersistedFidWithPendingStatus(persistedFidEntry); + executeFISCalls(persistedFidEntry, listener); return Tasks.forResult(fid); } - private void updateAwaitListenerIfRegisteredFid( + private boolean isFidUnregisteredOrAuthTokenExpired(PersistedFidEntry persistedFidEntry) { + return persistedFidEntry.getRegistrationStatus() == RegistrationStatus.UNREGISTERED + || isAuthTokenExpired(persistedFidEntry); + } + + private void executeFISCalls(PersistedFidEntry persistedFidEntry, AwaitListener listener) { + + if (!isAuthTokenExpired(persistedFidEntry)) { + executeFidRegistration(persistedFidEntry, listener); + return; + } + refreshAuthToken(listener); + } + + private Task updateAwaitListenerIfRegisteredFid( PersistedFidEntry persistedFidEntry, AwaitListener listener) { if (listener != null && persistedFidEntry.getRegistrationStatus() == RegistrationStatus.REGISTERED) { listener.onSuccess(); } + return Tasks.forResult(persistedFidEntry.getFirebaseInstallationId()); } /** @@ -273,12 +287,20 @@ private void executeFidRegistration(PersistedFidEntry persistedFidEntry, AwaitLi } } - private void updatePersistedFidWithPendingStatus(String fid) { + /** + * Refreshes FID auth token with FIS servers in a background thread and updates the listener on + * completion. + */ + private void refreshAuthToken(AwaitListener listener) { + Task task = Tasks.call(executor, () -> refreshAuthTokenIfNecessary(1)); + if (listener != null) { + task.addOnCompleteListener((unused) -> listener.onSuccess()); + } + } + + private void updatePersistedFidWithPendingStatus(PersistedFidEntry persistedFidEntry) { persistedFid.insertOrUpdatePersistedFidEntry( - PersistedFidEntry.builder() - .setFirebaseInstallationId(fid) - .setRegistrationStatus(RegistrationStatus.PENDING) - .build()); + persistedFidEntry.toBuilder().setRegistrationStatus(RegistrationStatus.PENDING).build()); } /** Registers the created Fid with FIS servers and update the shared prefs. */ @@ -355,7 +377,8 @@ private InstallationTokenResult getValidAuthToken(PersistedFidEntry persistedFid private boolean isPersistedFidRegistered(PersistedFidEntry persistedFidEntry) { return persistedFidEntry != null - && persistedFidEntry.getRegistrationStatus() == RegistrationStatus.REGISTERED; + && (persistedFidEntry.getRegistrationStatus() == RegistrationStatus.REGISTERED + || persistedFidEntry.getRegistrationStatus() == RegistrationStatus.PENDING); } /** Calls the FIS servers to generate an auth token for this Firebase installation. */ @@ -382,6 +405,12 @@ private InstallationTokenResult fetchAuthTokenFromServer(PersistedFidEntry persi return tokenResult; } catch (FirebaseInstallationServiceException exception) { + persistedFid.insertOrUpdatePersistedFidEntry( + persistedFidEntry + .toBuilder() + .setRegistrationStatus(RegistrationStatus.REGISTER_ERROR) + .build()); + throw new FirebaseInstallationsException( "Failed to generate auth token for a Firebase Installation.", FirebaseInstallationsException.Status.SDK_INTERNAL_ERROR); From c0580b140948fe0934741cafc387ae509c1aa2f6 Mon Sep 17 00:00:00 2001 From: Ankita Jhawar Date: Tue, 17 Sep 2019 13:24:09 -0700 Subject: [PATCH 02/11] Addressing Di's comments. --- .../firebase/installations/FirebaseInstallations.java | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/firebase-installations/src/main/java/com/google/firebase/installations/FirebaseInstallations.java b/firebase-installations/src/main/java/com/google/firebase/installations/FirebaseInstallations.java index 55c367ac0a1..cb1b2e48648 100644 --- a/firebase-installations/src/main/java/com/google/firebase/installations/FirebaseInstallations.java +++ b/firebase-installations/src/main/java/com/google/firebase/installations/FirebaseInstallations.java @@ -244,7 +244,8 @@ private Task registerFidIfNecessary( PersistedFidEntry persistedFidEntry, AwaitListener listener) { String fid = persistedFidEntry.getFirebaseInstallationId(); - if (!isFidUnregisteredOrAuthTokenExpired(persistedFidEntry)) { + if (persistedFidEntry.getRegistrationStatus() != RegistrationStatus.UNREGISTERED + && !isAuthTokenExpired(persistedFidEntry)) { return updateAwaitListenerIfRegisteredFid(persistedFidEntry, listener); } @@ -253,11 +254,6 @@ private Task registerFidIfNecessary( return Tasks.forResult(fid); } - private boolean isFidUnregisteredOrAuthTokenExpired(PersistedFidEntry persistedFidEntry) { - return persistedFidEntry.getRegistrationStatus() == RegistrationStatus.UNREGISTERED - || isAuthTokenExpired(persistedFidEntry); - } - private void executeFISCalls(PersistedFidEntry persistedFidEntry, AwaitListener listener) { if (!isAuthTokenExpired(persistedFidEntry)) { From 8558f73021b82478f023070a9b296a56f48dc42f Mon Sep 17 00:00:00 2001 From: Ankita Jhawar Date: Tue, 17 Sep 2019 14:25:41 -0700 Subject: [PATCH 03/11] Refractoring the code to replace small methods with inline code. --- .../installations/FirebaseInstallations.java | 49 +++++++------------ 1 file changed, 19 insertions(+), 30 deletions(-) diff --git a/firebase-installations/src/main/java/com/google/firebase/installations/FirebaseInstallations.java b/firebase-installations/src/main/java/com/google/firebase/installations/FirebaseInstallations.java index cb1b2e48648..6103a0384d9 100644 --- a/firebase-installations/src/main/java/com/google/firebase/installations/FirebaseInstallations.java +++ b/firebase-installations/src/main/java/com/google/firebase/installations/FirebaseInstallations.java @@ -179,18 +179,14 @@ String getName() { */ private PersistedFidEntry getPersistedFid() throws FirebaseInstallationsException { PersistedFidEntry persistedFidEntry = persistedFid.readPersistedFidEntryValue(); - if (persistedFidMissingOrInErrorState(persistedFidEntry)) { + if (persistedFidEntry == null + || persistedFidEntry.getRegistrationStatus() == RegistrationStatus.REGISTER_ERROR) { throw new FirebaseInstallationsException( "Failed to get existing fid.", FirebaseInstallationsException.Status.CLIENT_ERROR); } return persistedFidEntry; } - private static boolean persistedFidMissingOrInErrorState(PersistedFidEntry persistedFidEntry) { - return persistedFidEntry == null - || persistedFidEntry.getRegistrationStatus() == RegistrationStatus.REGISTER_ERROR; - } - @NonNull private static Continuation orElse(@NonNull Supplier supplier) { return t -> { @@ -244,23 +240,32 @@ private Task registerFidIfNecessary( PersistedFidEntry persistedFidEntry, AwaitListener listener) { String fid = persistedFidEntry.getFirebaseInstallationId(); + // If FID registration is in complete/pending state or FID auth token is valid, update the + // listener if awaiting and return the persisted fid. if (persistedFidEntry.getRegistrationStatus() != RegistrationStatus.UNREGISTERED && !isAuthTokenExpired(persistedFidEntry)) { return updateAwaitListenerIfRegisteredFid(persistedFidEntry, listener); } - updatePersistedFidWithPendingStatus(persistedFidEntry); - executeFISCalls(persistedFidEntry, listener); - return Tasks.forResult(fid); - } - - private void executeFISCalls(PersistedFidEntry persistedFidEntry, AwaitListener listener) { + // Update FID registration status to PENDING to avoid multiple network calls to FIS Servers. + persistedFid.insertOrUpdatePersistedFidEntry( + persistedFidEntry.toBuilder().setRegistrationStatus(RegistrationStatus.PENDING).build()); + // If fid registration for this firebase installation is incomplete without an expired auth + // token, execute Fid registration. if (!isAuthTokenExpired(persistedFidEntry)) { executeFidRegistration(persistedFidEntry, listener); - return; + return Tasks.forResult(fid); } - refreshAuthToken(listener); + + // If auth token is expired, refresh FID auth token with FIS servers in a background thread and + // updates the listener on + // completion + Task task = Tasks.call(executor, () -> refreshAuthTokenIfNecessary(1)); + if (listener != null) { + task.addOnCompleteListener((unused) -> listener.onSuccess()); + } + return Tasks.forResult(fid); } private Task updateAwaitListenerIfRegisteredFid( @@ -283,22 +288,6 @@ private void executeFidRegistration(PersistedFidEntry persistedFidEntry, AwaitLi } } - /** - * Refreshes FID auth token with FIS servers in a background thread and updates the listener on - * completion. - */ - private void refreshAuthToken(AwaitListener listener) { - Task task = Tasks.call(executor, () -> refreshAuthTokenIfNecessary(1)); - if (listener != null) { - task.addOnCompleteListener((unused) -> listener.onSuccess()); - } - } - - private void updatePersistedFidWithPendingStatus(PersistedFidEntry persistedFidEntry) { - persistedFid.insertOrUpdatePersistedFidEntry( - persistedFidEntry.toBuilder().setRegistrationStatus(RegistrationStatus.PENDING).build()); - } - /** Registers the created Fid with FIS servers and update the shared prefs. */ private Void registerAndSaveFid(PersistedFidEntry persistedFidEntry) throws FirebaseInstallationsException { From 0fb266670b99a19f3847a3a36080e80847e15889 Mon Sep 17 00:00:00 2001 From: Ankita Jhawar Date: Wed, 18 Sep 2019 10:44:34 -0700 Subject: [PATCH 04/11] Addressing Ciaran's comments and refractored the code for better readability. --- ...FirebaseInstallationsInstrumentedTest.java | 26 +++-- .../FisAndroidTestConstants.java | 3 +- .../installations/FirebaseInstallations.java | 100 ++++++++---------- 3 files changed, 59 insertions(+), 70 deletions(-) diff --git a/firebase-installations/src/androidTest/java/com/google/firebase/installations/FirebaseInstallationsInstrumentedTest.java b/firebase-installations/src/androidTest/java/com/google/firebase/installations/FirebaseInstallationsInstrumentedTest.java index 78ed3022562..d458991de5d 100644 --- a/firebase-installations/src/androidTest/java/com/google/firebase/installations/FirebaseInstallationsInstrumentedTest.java +++ b/firebase-installations/src/androidTest/java/com/google/firebase/installations/FirebaseInstallationsInstrumentedTest.java @@ -23,7 +23,6 @@ import static com.google.firebase.installations.FisAndroidTestConstants.TEST_AUTH_TOKEN_4; import static com.google.firebase.installations.FisAndroidTestConstants.TEST_CREATION_TIMESTAMP_1; import static com.google.firebase.installations.FisAndroidTestConstants.TEST_CREATION_TIMESTAMP_2; -import static com.google.firebase.installations.FisAndroidTestConstants.TEST_CREATION_TIMESTAMP_3; import static com.google.firebase.installations.FisAndroidTestConstants.TEST_FID_1; import static com.google.firebase.installations.FisAndroidTestConstants.TEST_PROJECT_ID; import static com.google.firebase.installations.FisAndroidTestConstants.TEST_REFRESH_TOKEN; @@ -88,7 +87,7 @@ public class FirebaseInstallationsInstrumentedTest { .setFirebaseInstallationId(TEST_FID_1) .setAuthToken(TEST_AUTH_TOKEN) .setRefreshToken(TEST_REFRESH_TOKEN) - .setTokenCreationEpochInSecs(TEST_CREATION_TIMESTAMP_2) + .setTokenCreationEpochInSecs(TEST_CREATION_TIMESTAMP_1) .setExpiresInSecs(TEST_TOKEN_EXPIRATION_TIMESTAMP) .setRegistrationStatus(PersistedFid.RegistrationStatus.REGISTERED) .build(); @@ -98,7 +97,7 @@ public class FirebaseInstallationsInstrumentedTest { .setFirebaseInstallationId(TEST_FID_1) .setAuthToken(TEST_AUTH_TOKEN) .setRefreshToken(TEST_REFRESH_TOKEN) - .setTokenCreationEpochInSecs(TEST_CREATION_TIMESTAMP_2) + .setTokenCreationEpochInSecs(TEST_CREATION_TIMESTAMP_1) .setExpiresInSecs(TEST_TOKEN_EXPIRATION_TIMESTAMP_2) .setRegistrationStatus(PersistedFid.RegistrationStatus.REGISTERED) .build(); @@ -108,7 +107,7 @@ public class FirebaseInstallationsInstrumentedTest { .setFirebaseInstallationId(TEST_FID_1) .setAuthToken("") .setRefreshToken("") - .setTokenCreationEpochInSecs(TEST_CREATION_TIMESTAMP_2) + .setTokenCreationEpochInSecs(TEST_CREATION_TIMESTAMP_1) .setExpiresInSecs(0) .setRegistrationStatus(PersistedFid.RegistrationStatus.UNREGISTERED) .build(); @@ -118,7 +117,7 @@ public class FirebaseInstallationsInstrumentedTest { .setFirebaseInstallationId(TEST_FID_1) .setAuthToken(TEST_AUTH_TOKEN_2) .setRefreshToken(TEST_REFRESH_TOKEN) - .setTokenCreationEpochInSecs(TEST_CREATION_TIMESTAMP_3) + .setTokenCreationEpochInSecs(TEST_CREATION_TIMESTAMP_2) .setExpiresInSecs(TEST_TOKEN_EXPIRATION_TIMESTAMP) .setRegistrationStatus(PersistedFid.RegistrationStatus.REGISTERED) .build(); @@ -179,7 +178,7 @@ public void testGetId_PersistedFidOk_BackendOk() throws Exception { mockClock, executor, firebaseApp, backendClientReturnsOk, persistedFid, mockUtils); // No exception, means success. - assertWithMessage("getId Task fails.") + assertWithMessage("getId Task failed") .that(Tasks.await(firebaseInstallations.getId())) .isNotEmpty(); PersistedFidEntry entryValue = persistedFid.readPersistedFidEntryValue(); @@ -206,7 +205,7 @@ public void testGetId_multipleCalls_sameFIDReturned() throws Exception { mockClock, executor, firebaseApp, backendClientReturnsOk, persistedFid, mockUtils); // No exception, means success. - assertWithMessage("getId Task fails.") + assertWithMessage("getId Task failed") .that(Tasks.await(firebaseInstallations.getId())) .isNotEmpty(); PersistedFidEntry entryValue = persistedFid.readPersistedFidEntryValue(); @@ -287,7 +286,7 @@ public void testGetId_expiredAuthToken_refreshesAuthToken() throws Exception { new FirebaseInstallations( mockClock, executor, firebaseApp, backendClientReturnsOk, persistedFid, mockUtils); - assertWithMessage("getId Task fails.") + assertWithMessage("getId Task failed") .that(Tasks.await(firebaseInstallations.getId())) .isNotEmpty(); PersistedFidEntry entryValue = persistedFid.readPersistedFidEntryValue(); @@ -437,14 +436,13 @@ public void testGetAuthToken_serverError_failure() throws Exception { @Test public void testGetAuthToken_multipleCallsDoNotForceRefresh_fetchedNewTokenOnce() throws Exception { - // Using mockPersistedFid to ensure the order of returning persistedFidEntry to 2 tasks - // triggered simultaneously. Task2 waits for Task1 to complete. On Task1 completion, task2 reads - // the UPDATED_AUTH_TOKEN_FID_ENTRY by Task1 on execution. - when(mockPersistedFid.readPersistedFidEntryValue()) - .thenReturn(EXPIRED_AUTH_TOKEN_ENTRY, UPDATED_AUTH_TOKEN_FID_ENTRY); + // Update local storage with fid entry that has auth token expired. When 2 tasks are triggered + // simultaneously, Task2 waits for Task1 to complete. On Task1 completion, task2 reads the + // UPDATED_AUTH_TOKEN_FID_ENTRY by Task1 on execution. + persistedFid.insertOrUpdatePersistedFidEntry(EXPIRED_AUTH_TOKEN_ENTRY); FirebaseInstallations firebaseInstallations = new FirebaseInstallations( - mockClock, executor, firebaseApp, backendClientReturnsOk, mockPersistedFid, mockUtils); + mockClock, executor, firebaseApp, backendClientReturnsOk, persistedFid, mockUtils); // Call getAuthToken multiple times with DO_NOT_FORCE_REFRESH option Task task1 = diff --git a/firebase-installations/src/androidTest/java/com/google/firebase/installations/FisAndroidTestConstants.java b/firebase-installations/src/androidTest/java/com/google/firebase/installations/FisAndroidTestConstants.java index a5afc6025f3..8e3396cdf22 100644 --- a/firebase-installations/src/androidTest/java/com/google/firebase/installations/FisAndroidTestConstants.java +++ b/firebase-installations/src/androidTest/java/com/google/firebase/installations/FisAndroidTestConstants.java @@ -35,6 +35,5 @@ public final class FisAndroidTestConstants { public static final long TEST_TOKEN_EXPIRATION_TIMESTAMP_2 = 2000L; public static final long TEST_CREATION_TIMESTAMP_1 = 2000L; - public static final long TEST_CREATION_TIMESTAMP_2 = 2000L; - public static final long TEST_CREATION_TIMESTAMP_3 = 2L; + public static final long TEST_CREATION_TIMESTAMP_2 = 2L; } diff --git a/firebase-installations/src/main/java/com/google/firebase/installations/FirebaseInstallations.java b/firebase-installations/src/main/java/com/google/firebase/installations/FirebaseInstallations.java index 6103a0384d9..7a55a0ebbda 100644 --- a/firebase-installations/src/main/java/com/google/firebase/installations/FirebaseInstallations.java +++ b/firebase-installations/src/main/java/com/google/firebase/installations/FirebaseInstallations.java @@ -123,8 +123,7 @@ public Task getId() { private Task getId(AwaitListener awaitListener) { return Tasks.call(executor, this::getPersistedFid) .continueWith(orElse(this::createAndPersistNewFid)) - .onSuccessTask( - persistedFidEntry -> registerFidIfNecessary(persistedFidEntry, awaitListener)); + .onSuccessTask(unused -> registerFidIfNecessary(awaitListener)); } /** @@ -236,55 +235,52 @@ private void persistFid(String fid) throws FirebaseInstallationsException { * *

Updates FID registration status to PENDING to avoid multiple network calls to FIS Servers. */ - private Task registerFidIfNecessary( - PersistedFidEntry persistedFidEntry, AwaitListener listener) { - String fid = persistedFidEntry.getFirebaseInstallationId(); - - // If FID registration is in complete/pending state or FID auth token is valid, update the - // listener if awaiting and return the persisted fid. - if (persistedFidEntry.getRegistrationStatus() != RegistrationStatus.UNREGISTERED - && !isAuthTokenExpired(persistedFidEntry)) { - return updateAwaitListenerIfRegisteredFid(persistedFidEntry, listener); - } - - // Update FID registration status to PENDING to avoid multiple network calls to FIS Servers. - persistedFid.insertOrUpdatePersistedFidEntry( - persistedFidEntry.toBuilder().setRegistrationStatus(RegistrationStatus.PENDING).build()); + private Task registerFidIfNecessary(AwaitListener listener) { + synchronized (persistedFid) { + PersistedFidEntry persistedFidEntry = persistedFid.readPersistedFidEntryValue(); + String fid = persistedFidEntry.getFirebaseInstallationId(); + + // If FID registration status is PENDING, return the persisted fid. This avoids multiple + // network calls to FIS Servers. + if (persistedFidEntry.getRegistrationStatus() == RegistrationStatus.PENDING) { + return Tasks.forResult(fid); + } - // If fid registration for this firebase installation is incomplete without an expired auth - // token, execute Fid registration. - if (!isAuthTokenExpired(persistedFidEntry)) { - executeFidRegistration(persistedFidEntry, listener); - return Tasks.forResult(fid); - } + // If FID registration status is REGISTERED and auth token is valid, return the + // persisted fid. + if (persistedFidEntry.getRegistrationStatus() == RegistrationStatus.REGISTERED + && !isAuthTokenExpired(persistedFidEntry)) { - // If auth token is expired, refresh FID auth token with FIS servers in a background thread and - // updates the listener on - // completion - Task task = Tasks.call(executor, () -> refreshAuthTokenIfNecessary(1)); - if (listener != null) { - task.addOnCompleteListener((unused) -> listener.onSuccess()); - } - return Tasks.forResult(fid); - } + // Update the listener if awaiting + if (listener != null) { + listener.onSuccess(); + } + return Tasks.forResult(fid); + } - private Task updateAwaitListenerIfRegisteredFid( - PersistedFidEntry persistedFidEntry, AwaitListener listener) { - if (listener != null - && persistedFidEntry.getRegistrationStatus() == RegistrationStatus.REGISTERED) { - listener.onSuccess(); - } - return Tasks.forResult(persistedFidEntry.getFirebaseInstallationId()); - } + // Update FID registration status to PENDING to avoid multiple network calls to FIS Servers. + persistedFid.insertOrUpdatePersistedFidEntry( + persistedFidEntry.toBuilder().setRegistrationStatus(RegistrationStatus.PENDING).build()); + + // If fid registration for this firebase installation is incomplete without an expired auth + // token, execute Fid registration. + if (!isAuthTokenExpired(persistedFidEntry)) { + Task fidRegistrationTask = + Tasks.call(executor, () -> registerAndSaveFid(persistedFidEntry)); + if (listener != null) { + fidRegistrationTask.addOnCompleteListener(listener); + } + return Tasks.forResult(fid); + } - /** - * Registers the FID with FIS servers in a background thread and updates the listener on - * completion. - */ - private void executeFidRegistration(PersistedFidEntry persistedFidEntry, AwaitListener listener) { - Task task = Tasks.call(executor, () -> registerAndSaveFid(persistedFidEntry)); - if (listener != null) { - task.addOnCompleteListener(listener); + // If auth token is expired, refresh FID auth token with FIS servers in a background thread + // and updates the listener on completion + Task refreshAuthTokenTask = + Tasks.call(executor, () -> refreshAuthTokenIfNecessary(FORCE_REFRESH)); + if (listener != null) { + refreshAuthTokenTask.addOnCompleteListener((unused) -> listener.onSuccess()); + } + return Tasks.forResult(fid); } } @@ -327,7 +323,9 @@ private InstallationTokenResult refreshAuthTokenIfNecessary(int authTokenOption) PersistedFidEntry persistedFidEntry = persistedFid.readPersistedFidEntryValue(); - if (!isPersistedFidRegistered(persistedFidEntry)) { + if (persistedFidEntry == null + || (persistedFidEntry.getRegistrationStatus() != RegistrationStatus.REGISTERED + && persistedFidEntry.getRegistrationStatus() != RegistrationStatus.PENDING)) { throw new FirebaseInstallationsException( "Firebase Installation is not registered.", FirebaseInstallationsException.Status.SDK_INTERNAL_ERROR); @@ -360,12 +358,6 @@ private InstallationTokenResult getValidAuthToken(PersistedFidEntry persistedFid .build(); } - private boolean isPersistedFidRegistered(PersistedFidEntry persistedFidEntry) { - return persistedFidEntry != null - && (persistedFidEntry.getRegistrationStatus() == RegistrationStatus.REGISTERED - || persistedFidEntry.getRegistrationStatus() == RegistrationStatus.PENDING); - } - /** Calls the FIS servers to generate an auth token for this Firebase installation. */ private InstallationTokenResult fetchAuthTokenFromServer(PersistedFidEntry persistedFidEntry) throws FirebaseInstallationsException { From 60f4023919f6e1ba570672b09385c98f90eec6ea Mon Sep 17 00:00:00 2001 From: Ankita Jhawar Date: Wed, 18 Sep 2019 11:48:33 -0700 Subject: [PATCH 05/11] Adding isFidRegistered method. --- .../firebase/installations/FirebaseInstallations.java | 8 ++++++-- .../google/firebase/installations/local/PersistedFid.java | 6 ++++++ 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/firebase-installations/src/main/java/com/google/firebase/installations/FirebaseInstallations.java b/firebase-installations/src/main/java/com/google/firebase/installations/FirebaseInstallations.java index 5ad8fc98164..71b2035139a 100644 --- a/firebase-installations/src/main/java/com/google/firebase/installations/FirebaseInstallations.java +++ b/firebase-installations/src/main/java/com/google/firebase/installations/FirebaseInstallations.java @@ -248,7 +248,7 @@ private Task registerFidIfNecessary(AwaitListener listener) { // If FID registration status is REGISTERED and auth token is valid, return the // persisted fid. - if (persistedFidEntry.getRegistrationStatus() == RegistrationStatus.REGISTERED + if (persistedFid.isFidRegistered(persistedFidEntry) && !isAuthTokenExpired(persistedFidEntry)) { // Update the listener if awaiting @@ -267,6 +267,8 @@ private Task registerFidIfNecessary(AwaitListener listener) { if (!isAuthTokenExpired(persistedFidEntry)) { Task fidRegistrationTask = Tasks.call(executor, () -> registerAndSaveFid(persistedFidEntry)); + + // Update the listener if awaiting if (listener != null) { fidRegistrationTask.addOnCompleteListener(listener); } @@ -277,6 +279,8 @@ private Task registerFidIfNecessary(AwaitListener listener) { // and updates the listener on completion Task refreshAuthTokenTask = Tasks.call(executor, () -> refreshAuthTokenIfNecessary(FORCE_REFRESH)); + + // Update the listener if awaiting if (listener != null) { refreshAuthTokenTask.addOnCompleteListener((unused) -> listener.onSuccess()); } @@ -415,7 +419,7 @@ private Void deleteFirebaseInstallationId() throws FirebaseInstallationsExceptio PersistedFidEntry persistedFidEntry = persistedFid.readPersistedFidEntryValue(); - if (isPersistedFidRegistered(persistedFidEntry)) { + if (persistedFid.isFidRegistered(persistedFidEntry)) { // Call the FIS servers to delete this firebase installation id. try { serviceClient.deleteFirebaseInstallation( diff --git a/firebase-installations/src/main/java/com/google/firebase/installations/local/PersistedFid.java b/firebase-installations/src/main/java/com/google/firebase/installations/local/PersistedFid.java index 1da104fa350..56afb7a1355 100644 --- a/firebase-installations/src/main/java/com/google/firebase/installations/local/PersistedFid.java +++ b/firebase-installations/src/main/java/com/google/firebase/installations/local/PersistedFid.java @@ -134,4 +134,10 @@ public boolean clear() { private String getSharedPreferencesKey(String key) { return String.format("%s|%s", persistenceKey, key); } + + @NonNull + public boolean isFidRegistered(@Nullable PersistedFidEntry persistedFidEntry) { + return persistedFidEntry != null + && persistedFidEntry.getRegistrationStatus() == RegistrationStatus.REGISTERED; + } } From 63da5676e03063e833c69b5c40de964d9b4937e4 Mon Sep 17 00:00:00 2001 From: Ankita Jhawar Date: Wed, 18 Sep 2019 12:00:38 -0700 Subject: [PATCH 06/11] Fixing api-information check. --- firebase-installations/api.txt | 1 + 1 file changed, 1 insertion(+) diff --git a/firebase-installations/api.txt b/firebase-installations/api.txt index fed20da199e..32afbfae8fa 100644 --- a/firebase-installations/api.txt +++ b/firebase-installations/api.txt @@ -29,6 +29,7 @@ package com.google.firebase.installations.local { ctor public PersistedFid(@NonNull FirebaseApp); method @NonNull public boolean clear(); method @NonNull public boolean insertOrUpdatePersistedFidEntry(@NonNull com.google.firebase.installations.local.PersistedFidEntry); + method @NonNull public boolean isFidRegistered(@Nullable com.google.firebase.installations.local.PersistedFidEntry); method @Nullable public com.google.firebase.installations.local.PersistedFidEntry readPersistedFidEntryValue(); } From c26adc79c2a08198c65c15a124afa2d1ab4cb7de Mon Sep 17 00:00:00 2001 From: Ankita Jhawar Date: Wed, 18 Sep 2019 14:37:53 -0700 Subject: [PATCH 07/11] Restructuring the synchronized block of registerFidIfNeccesary --- .../installations/FirebaseInstallations.java | 75 ++++++++++--------- 1 file changed, 40 insertions(+), 35 deletions(-) diff --git a/firebase-installations/src/main/java/com/google/firebase/installations/FirebaseInstallations.java b/firebase-installations/src/main/java/com/google/firebase/installations/FirebaseInstallations.java index 71b2035139a..5241bc2aa7a 100644 --- a/firebase-installations/src/main/java/com/google/firebase/installations/FirebaseInstallations.java +++ b/firebase-installations/src/main/java/com/google/firebase/installations/FirebaseInstallations.java @@ -236,56 +236,61 @@ private void persistFid(String fid) throws FirebaseInstallationsException { *

Updates FID registration status to PENDING to avoid multiple network calls to FIS Servers. */ private Task registerFidIfNecessary(AwaitListener listener) { + Boolean isNetworkCallRequired = false; + synchronized (persistedFid) { PersistedFidEntry persistedFidEntry = persistedFid.readPersistedFidEntryValue(); - String fid = persistedFidEntry.getFirebaseInstallationId(); - // If FID registration status is PENDING, return the persisted fid. This avoids multiple - // network calls to FIS Servers. - if (persistedFidEntry.getRegistrationStatus() == RegistrationStatus.PENDING) { - return Tasks.forResult(fid); + // If FID registration status is REGISTERED and auth token is expired, or registration status + // is UNREGISTERED network call iks required. + if ((persistedFid.isFidRegistered(persistedFidEntry) && isAuthTokenExpired(persistedFidEntry)) + || persistedFidEntry.getRegistrationStatus() == RegistrationStatus.UNREGISTERED) { + isNetworkCallRequired = true; + + // Update FID registration status to PENDING to avoid multiple network calls to FIS Servers. + persistedFid.insertOrUpdatePersistedFidEntry( + persistedFidEntry + .toBuilder() + .setRegistrationStatus(RegistrationStatus.PENDING) + .build()); } + } - // If FID registration status is REGISTERED and auth token is valid, return the - // persisted fid. - if (persistedFid.isFidRegistered(persistedFidEntry) - && !isAuthTokenExpired(persistedFidEntry)) { + PersistedFidEntry updatedPersistedFidEntry = persistedFid.readPersistedFidEntryValue(); + String fid = updatedPersistedFidEntry.getFirebaseInstallationId(); - // Update the listener if awaiting - if (listener != null) { - listener.onSuccess(); - } - return Tasks.forResult(fid); - } + if (!isNetworkCallRequired) { - // Update FID registration status to PENDING to avoid multiple network calls to FIS Servers. - persistedFid.insertOrUpdatePersistedFidEntry( - persistedFidEntry.toBuilder().setRegistrationStatus(RegistrationStatus.PENDING).build()); - - // If fid registration for this firebase installation is incomplete without an expired auth - // token, execute Fid registration. - if (!isAuthTokenExpired(persistedFidEntry)) { - Task fidRegistrationTask = - Tasks.call(executor, () -> registerAndSaveFid(persistedFidEntry)); - - // Update the listener if awaiting - if (listener != null) { - fidRegistrationTask.addOnCompleteListener(listener); - } - return Tasks.forResult(fid); + // If the Fid is registered, update the listener if awaiting + if (listener != null && persistedFid.isFidRegistered(updatedPersistedFidEntry)) { + listener.onSuccess(); } + return Tasks.forResult(fid); + } - // If auth token is expired, refresh FID auth token with FIS servers in a background thread - // and updates the listener on completion - Task refreshAuthTokenTask = - Tasks.call(executor, () -> refreshAuthTokenIfNecessary(FORCE_REFRESH)); + // If fid registration for this firebase installation is incomplete without an expired auth + // token, execute Fid registration. + if (!isAuthTokenExpired(updatedPersistedFidEntry)) { + Task fidRegistrationTask = + Tasks.call(executor, () -> registerAndSaveFid(updatedPersistedFidEntry)); // Update the listener if awaiting if (listener != null) { - refreshAuthTokenTask.addOnCompleteListener((unused) -> listener.onSuccess()); + fidRegistrationTask.addOnCompleteListener(listener); } return Tasks.forResult(fid); } + + // If auth token is expired, refresh FID auth token with FIS servers in a background thread + // and updates the listener on completion + Task refreshAuthTokenTask = + Tasks.call(executor, () -> refreshAuthTokenIfNecessary(FORCE_REFRESH)); + + // Update the listener if awaiting + if (listener != null) { + refreshAuthTokenTask.addOnCompleteListener((unused) -> listener.onSuccess()); + } + return Tasks.forResult(fid); } /** Registers the created Fid with FIS servers and update the shared prefs. */ From 4afc7e5cb92d85f0a27945627f938379ac88b86d Mon Sep 17 00:00:00 2001 From: Ankita Jhawar Date: Wed, 18 Sep 2019 15:04:52 -0700 Subject: [PATCH 08/11] Addressed ciaran'c comments. --- firebase-installations/api.txt | 2 +- .../installations/FirebaseInstallations.java | 22 +++++++++++++++---- .../installations/local/PersistedFid.java | 6 ----- .../local/PersistedFidEntry.java | 4 ++++ 4 files changed, 23 insertions(+), 11 deletions(-) diff --git a/firebase-installations/api.txt b/firebase-installations/api.txt index 32afbfae8fa..c5efe78ce60 100644 --- a/firebase-installations/api.txt +++ b/firebase-installations/api.txt @@ -29,7 +29,6 @@ package com.google.firebase.installations.local { ctor public PersistedFid(@NonNull FirebaseApp); method @NonNull public boolean clear(); method @NonNull public boolean insertOrUpdatePersistedFidEntry(@NonNull com.google.firebase.installations.local.PersistedFidEntry); - method @NonNull public boolean isFidRegistered(@Nullable com.google.firebase.installations.local.PersistedFidEntry); method @Nullable public com.google.firebase.installations.local.PersistedFidEntry readPersistedFidEntryValue(); } @@ -49,6 +48,7 @@ package com.google.firebase.installations.local { method @Nullable public abstract String getRefreshToken(); method @NonNull public abstract com.google.firebase.installations.local.PersistedFid.RegistrationStatus getRegistrationStatus(); method public abstract long getTokenCreationEpochInSecs(); + method public boolean isFidRegistered(); method @NonNull public abstract com.google.firebase.installations.local.PersistedFidEntry.Builder toBuilder(); } diff --git a/firebase-installations/src/main/java/com/google/firebase/installations/FirebaseInstallations.java b/firebase-installations/src/main/java/com/google/firebase/installations/FirebaseInstallations.java index 5241bc2aa7a..fcfa519388b 100644 --- a/firebase-installations/src/main/java/com/google/firebase/installations/FirebaseInstallations.java +++ b/firebase-installations/src/main/java/com/google/firebase/installations/FirebaseInstallations.java @@ -235,15 +235,22 @@ private void persistFid(String fid) throws FirebaseInstallationsException { * *

Updates FID registration status to PENDING to avoid multiple network calls to FIS Servers. */ - private Task registerFidIfNecessary(AwaitListener listener) { + private Task registerFidIfNecessary(AwaitListener listener) + throws FirebaseInstallationsException { Boolean isNetworkCallRequired = false; synchronized (persistedFid) { PersistedFidEntry persistedFidEntry = persistedFid.readPersistedFidEntryValue(); + if (persistedFidEntry == null) { + throw new FirebaseInstallationsException( + "Local storage has no persisted Fid entry.", + FirebaseInstallationsException.Status.CLIENT_ERROR); + } + // If FID registration status is REGISTERED and auth token is expired, or registration status // is UNREGISTERED network call iks required. - if ((persistedFid.isFidRegistered(persistedFidEntry) && isAuthTokenExpired(persistedFidEntry)) + if ((persistedFidEntry.isFidRegistered() && isAuthTokenExpired(persistedFidEntry)) || persistedFidEntry.getRegistrationStatus() == RegistrationStatus.UNREGISTERED) { isNetworkCallRequired = true; @@ -257,12 +264,19 @@ private Task registerFidIfNecessary(AwaitListener listener) { } PersistedFidEntry updatedPersistedFidEntry = persistedFid.readPersistedFidEntryValue(); + + if (updatedPersistedFidEntry == null) { + throw new FirebaseInstallationsException( + "Local storage has no persisted Fid entry.", + FirebaseInstallationsException.Status.CLIENT_ERROR); + } + String fid = updatedPersistedFidEntry.getFirebaseInstallationId(); if (!isNetworkCallRequired) { // If the Fid is registered, update the listener if awaiting - if (listener != null && persistedFid.isFidRegistered(updatedPersistedFidEntry)) { + if (listener != null && updatedPersistedFidEntry.isFidRegistered()) { listener.onSuccess(); } return Tasks.forResult(fid); @@ -424,7 +438,7 @@ private Void deleteFirebaseInstallationId() throws FirebaseInstallationsExceptio PersistedFidEntry persistedFidEntry = persistedFid.readPersistedFidEntryValue(); - if (persistedFid.isFidRegistered(persistedFidEntry)) { + if (persistedFidEntry != null && persistedFidEntry.isFidRegistered()) { // Call the FIS servers to delete this firebase installation id. try { serviceClient.deleteFirebaseInstallation( diff --git a/firebase-installations/src/main/java/com/google/firebase/installations/local/PersistedFid.java b/firebase-installations/src/main/java/com/google/firebase/installations/local/PersistedFid.java index 56afb7a1355..1da104fa350 100644 --- a/firebase-installations/src/main/java/com/google/firebase/installations/local/PersistedFid.java +++ b/firebase-installations/src/main/java/com/google/firebase/installations/local/PersistedFid.java @@ -134,10 +134,4 @@ public boolean clear() { private String getSharedPreferencesKey(String key) { return String.format("%s|%s", persistenceKey, key); } - - @NonNull - public boolean isFidRegistered(@Nullable PersistedFidEntry persistedFidEntry) { - return persistedFidEntry != null - && persistedFidEntry.getRegistrationStatus() == RegistrationStatus.REGISTERED; - } } diff --git a/firebase-installations/src/main/java/com/google/firebase/installations/local/PersistedFidEntry.java b/firebase-installations/src/main/java/com/google/firebase/installations/local/PersistedFidEntry.java index 4c8a9a56ec4..099b4e55500 100644 --- a/firebase-installations/src/main/java/com/google/firebase/installations/local/PersistedFidEntry.java +++ b/firebase-installations/src/main/java/com/google/firebase/installations/local/PersistedFidEntry.java @@ -41,6 +41,10 @@ public abstract class PersistedFidEntry { public abstract long getTokenCreationEpochInSecs(); + public boolean isFidRegistered() { + return getRegistrationStatus() == PersistedFid.RegistrationStatus.REGISTERED; + } + @NonNull public abstract Builder toBuilder(); From 86e3728ac398481abbc5d89100d921899a74c8cb Mon Sep 17 00:00:00 2001 From: Ankita Jhawar Date: Wed, 18 Sep 2019 15:24:51 -0700 Subject: [PATCH 09/11] Added helper methods for registration status. --- firebase-installations/api.txt | 4 +++- .../installations/FirebaseInstallations.java | 12 ++++++------ .../installations/local/PersistedFidEntry.java | 10 +++++++++- 3 files changed, 18 insertions(+), 8 deletions(-) diff --git a/firebase-installations/api.txt b/firebase-installations/api.txt index c5efe78ce60..9f8c212a3a7 100644 --- a/firebase-installations/api.txt +++ b/firebase-installations/api.txt @@ -48,7 +48,9 @@ package com.google.firebase.installations.local { method @Nullable public abstract String getRefreshToken(); method @NonNull public abstract com.google.firebase.installations.local.PersistedFid.RegistrationStatus getRegistrationStatus(); method public abstract long getTokenCreationEpochInSecs(); - method public boolean isFidRegistered(); + method public boolean isErrored(); + method public boolean isRegistered(); + method public boolean isUnregistered(); method @NonNull public abstract com.google.firebase.installations.local.PersistedFidEntry.Builder toBuilder(); } diff --git a/firebase-installations/src/main/java/com/google/firebase/installations/FirebaseInstallations.java b/firebase-installations/src/main/java/com/google/firebase/installations/FirebaseInstallations.java index fcfa519388b..ae7511fed90 100644 --- a/firebase-installations/src/main/java/com/google/firebase/installations/FirebaseInstallations.java +++ b/firebase-installations/src/main/java/com/google/firebase/installations/FirebaseInstallations.java @@ -250,8 +250,8 @@ private Task registerFidIfNecessary(AwaitListener listener) // If FID registration status is REGISTERED and auth token is expired, or registration status // is UNREGISTERED network call iks required. - if ((persistedFidEntry.isFidRegistered() && isAuthTokenExpired(persistedFidEntry)) - || persistedFidEntry.getRegistrationStatus() == RegistrationStatus.UNREGISTERED) { + if ((persistedFidEntry.isRegistered() && isAuthTokenExpired(persistedFidEntry)) + || persistedFidEntry.isUnregistered()) { isNetworkCallRequired = true; // Update FID registration status to PENDING to avoid multiple network calls to FIS Servers. @@ -276,7 +276,7 @@ private Task registerFidIfNecessary(AwaitListener listener) if (!isNetworkCallRequired) { // If the Fid is registered, update the listener if awaiting - if (listener != null && updatedPersistedFidEntry.isFidRegistered()) { + if (listener != null && updatedPersistedFidEntry.isRegistered()) { listener.onSuccess(); } return Tasks.forResult(fid); @@ -347,8 +347,8 @@ private InstallationTokenResult refreshAuthTokenIfNecessary(int authTokenOption) PersistedFidEntry persistedFidEntry = persistedFid.readPersistedFidEntryValue(); if (persistedFidEntry == null - || (persistedFidEntry.getRegistrationStatus() != RegistrationStatus.REGISTERED - && persistedFidEntry.getRegistrationStatus() != RegistrationStatus.PENDING)) { + || persistedFidEntry.isUnregistered() + || persistedFidEntry.isErrored()) { throw new FirebaseInstallationsException( "Firebase Installation is not registered.", FirebaseInstallationsException.Status.SDK_INTERNAL_ERROR); @@ -438,7 +438,7 @@ private Void deleteFirebaseInstallationId() throws FirebaseInstallationsExceptio PersistedFidEntry persistedFidEntry = persistedFid.readPersistedFidEntryValue(); - if (persistedFidEntry != null && persistedFidEntry.isFidRegistered()) { + if (persistedFidEntry != null && persistedFidEntry.isRegistered()) { // Call the FIS servers to delete this firebase installation id. try { serviceClient.deleteFirebaseInstallation( diff --git a/firebase-installations/src/main/java/com/google/firebase/installations/local/PersistedFidEntry.java b/firebase-installations/src/main/java/com/google/firebase/installations/local/PersistedFidEntry.java index 099b4e55500..66791adea5a 100644 --- a/firebase-installations/src/main/java/com/google/firebase/installations/local/PersistedFidEntry.java +++ b/firebase-installations/src/main/java/com/google/firebase/installations/local/PersistedFidEntry.java @@ -41,10 +41,18 @@ public abstract class PersistedFidEntry { public abstract long getTokenCreationEpochInSecs(); - public boolean isFidRegistered() { + public boolean isRegistered() { return getRegistrationStatus() == PersistedFid.RegistrationStatus.REGISTERED; } + public boolean isErrored() { + return getRegistrationStatus() == PersistedFid.RegistrationStatus.REGISTER_ERROR; + } + + public boolean isUnregistered() { + return getRegistrationStatus() == PersistedFid.RegistrationStatus.UNREGISTERED; + } + @NonNull public abstract Builder toBuilder(); From 62098a76f4bf88d2f47d205694a40f9d8b00d54e Mon Sep 17 00:00:00 2001 From: Ankita Jhawar Date: Wed, 18 Sep 2019 16:08:05 -0700 Subject: [PATCH 10/11] Introducing new registration status - NOT_GENERATED as default to avoid null checks. --- firebase-installations/api.txt | 4 +++- .../FirebaseInstallationsInstrumentedTest.java | 16 ++++++++++++---- .../installations/FisAndroidTestConstants.java | 5 +++++ .../installations/local/PersistedFidTest.java | 8 ++++---- .../installations/FirebaseInstallations.java | 12 ++++++------ .../installations/local/PersistedFid.java | 6 ++++-- .../installations/local/PersistedFidEntry.java | 7 ++++++- 7 files changed, 40 insertions(+), 18 deletions(-) diff --git a/firebase-installations/api.txt b/firebase-installations/api.txt index a8d98437213..370cd50d1b5 100644 --- a/firebase-installations/api.txt +++ b/firebase-installations/api.txt @@ -33,6 +33,7 @@ package com.google.firebase.installations.local { } public enum PersistedFid.RegistrationStatus { + enum_constant public static final com.google.firebase.installations.local.PersistedFid.RegistrationStatus NOT_GENERATED; enum_constant public static final com.google.firebase.installations.local.PersistedFid.RegistrationStatus PENDING; enum_constant public static final com.google.firebase.installations.local.PersistedFid.RegistrationStatus REGISTERED; enum_constant public static final com.google.firebase.installations.local.PersistedFid.RegistrationStatus REGISTER_ERROR; @@ -44,11 +45,12 @@ package com.google.firebase.installations.local { method @NonNull public static com.google.firebase.installations.local.PersistedFidEntry.Builder builder(); method @Nullable public abstract String getAuthToken(); method public abstract long getExpiresInSecs(); - method @NonNull public abstract String getFirebaseInstallationId(); + method @Nullable public abstract String getFirebaseInstallationId(); method @Nullable public abstract String getRefreshToken(); method @NonNull public abstract com.google.firebase.installations.local.PersistedFid.RegistrationStatus getRegistrationStatus(); method public abstract long getTokenCreationEpochInSecs(); method public boolean isErrored(); + method public boolean isNotGenerated(); method public boolean isRegistered(); method public boolean isUnregistered(); method @NonNull public abstract com.google.firebase.installations.local.PersistedFidEntry.Builder toBuilder(); diff --git a/firebase-installations/src/androidTest/java/com/google/firebase/installations/FirebaseInstallationsInstrumentedTest.java b/firebase-installations/src/androidTest/java/com/google/firebase/installations/FirebaseInstallationsInstrumentedTest.java index 934a1093423..f434b85a48f 100644 --- a/firebase-installations/src/androidTest/java/com/google/firebase/installations/FirebaseInstallationsInstrumentedTest.java +++ b/firebase-installations/src/androidTest/java/com/google/firebase/installations/FirebaseInstallationsInstrumentedTest.java @@ -15,6 +15,7 @@ package com.google.firebase.installations; import static com.google.common.truth.Truth.assertWithMessage; +import static com.google.firebase.installations.FisAndroidTestConstants.DEFAULT_PERSISTED_FID_ENTRY; import static com.google.firebase.installations.FisAndroidTestConstants.TEST_API_KEY; import static com.google.firebase.installations.FisAndroidTestConstants.TEST_APP_ID_1; import static com.google.firebase.installations.FisAndroidTestConstants.TEST_AUTH_TOKEN; @@ -163,7 +164,8 @@ public void setUp() throws FirebaseInstallationServiceException { new FirebaseInstallationServiceException( "SDK Error", FirebaseInstallationServiceException.Status.SERVER_ERROR)); when(persistedFidReturnsError.insertOrUpdatePersistedFidEntry(any())).thenReturn(false); - when(persistedFidReturnsError.readPersistedFidEntryValue()).thenReturn(null); + when(persistedFidReturnsError.readPersistedFidEntryValue()) + .thenReturn(DEFAULT_PERSISTED_FID_ENTRY); when(mockUtils.createRandomFid()).thenReturn(TEST_FID_1); when(mockClock.currentTimeMillis()).thenReturn(TEST_CREATION_TIMESTAMP_1); // Mocks success on FIS deletion @@ -533,7 +535,9 @@ public void testDelete_registeredFID_successful() throws Exception { Tasks.await(firebaseInstallations.delete()); PersistedFidEntry entryValue = persistedFid.readPersistedFidEntryValue(); - assertWithMessage("Persisted Fid Entry is not null.").that(entryValue).isNull(); + assertWithMessage("Persisted Fid Entry is not null.") + .that(entryValue) + .isEqualTo(DEFAULT_PERSISTED_FID_ENTRY); verify(backendClientReturnsOk, times(1)) .deleteFirebaseInstallation(TEST_API_KEY, TEST_FID_1, TEST_PROJECT_ID, TEST_REFRESH_TOKEN); } @@ -549,7 +553,9 @@ public void testDelete_unregisteredFID_successful() throws Exception { Tasks.await(firebaseInstallations.delete()); PersistedFidEntry entryValue = persistedFid.readPersistedFidEntryValue(); - assertWithMessage("Persisted Fid Entry is not null.").that(entryValue).isNull(); + assertWithMessage("Persisted Fid Entry is not null.") + .that(entryValue) + .isEqualTo(DEFAULT_PERSISTED_FID_ENTRY); verify(backendClientReturnsOk, never()) .deleteFirebaseInstallation(TEST_API_KEY, TEST_FID_1, TEST_PROJECT_ID, TEST_REFRESH_TOKEN); } @@ -563,7 +569,9 @@ public void testDelete_emptyPersistedFidEntry_successful() throws Exception { Tasks.await(firebaseInstallations.delete()); PersistedFidEntry entryValue = persistedFid.readPersistedFidEntryValue(); - assertWithMessage("Persisted Fid Entry is not null.").that(entryValue).isNull(); + assertWithMessage("Persisted Fid Entry is not null.") + .that(entryValue) + .isEqualTo(DEFAULT_PERSISTED_FID_ENTRY); verify(backendClientReturnsOk, never()) .deleteFirebaseInstallation(TEST_API_KEY, TEST_FID_1, TEST_PROJECT_ID, TEST_REFRESH_TOKEN); } diff --git a/firebase-installations/src/androidTest/java/com/google/firebase/installations/FisAndroidTestConstants.java b/firebase-installations/src/androidTest/java/com/google/firebase/installations/FisAndroidTestConstants.java index 8e3396cdf22..f1d39cfa396 100644 --- a/firebase-installations/src/androidTest/java/com/google/firebase/installations/FisAndroidTestConstants.java +++ b/firebase-installations/src/androidTest/java/com/google/firebase/installations/FisAndroidTestConstants.java @@ -14,6 +14,8 @@ package com.google.firebase.installations; +import com.google.firebase.installations.local.PersistedFidEntry; + public final class FisAndroidTestConstants { public static final String TEST_FID_1 = "cccccccccccccccccccccc"; @@ -36,4 +38,7 @@ public final class FisAndroidTestConstants { public static final long TEST_CREATION_TIMESTAMP_1 = 2000L; public static final long TEST_CREATION_TIMESTAMP_2 = 2L; + + public static final PersistedFidEntry DEFAULT_PERSISTED_FID_ENTRY = + PersistedFidEntry.builder().build(); } diff --git a/firebase-installations/src/androidTest/java/com/google/firebase/installations/local/PersistedFidTest.java b/firebase-installations/src/androidTest/java/com/google/firebase/installations/local/PersistedFidTest.java index f6aa77f45de..14b416ad71f 100644 --- a/firebase-installations/src/androidTest/java/com/google/firebase/installations/local/PersistedFidTest.java +++ b/firebase-installations/src/androidTest/java/com/google/firebase/installations/local/PersistedFidTest.java @@ -14,6 +14,7 @@ package com.google.firebase.installations.local; +import static com.google.firebase.installations.FisAndroidTestConstants.DEFAULT_PERSISTED_FID_ENTRY; import static com.google.firebase.installations.FisAndroidTestConstants.TEST_APP_ID_1; import static com.google.firebase.installations.FisAndroidTestConstants.TEST_APP_ID_2; import static com.google.firebase.installations.FisAndroidTestConstants.TEST_AUTH_TOKEN; @@ -23,7 +24,6 @@ import static com.google.firebase.installations.FisAndroidTestConstants.TEST_REFRESH_TOKEN; import static com.google.firebase.installations.FisAndroidTestConstants.TEST_TOKEN_EXPIRATION_TIMESTAMP; import static com.google.firebase.installations.local.PersistedFidEntrySubject.assertThat; -import static org.junit.Assert.assertNull; import static org.junit.Assert.assertTrue; import androidx.test.core.app.ApplicationProvider; @@ -68,9 +68,9 @@ public void cleanUp() throws Exception { } @Test - public void testReadPersistedFidEntry_Null() { - assertNull(persistedFid0.readPersistedFidEntryValue()); - assertNull(persistedFid1.readPersistedFidEntryValue()); + public void testReadPersistedFidEntry_returnsDefault() { + assertThat(persistedFid0.readPersistedFidEntryValue()).isEqualTo(DEFAULT_PERSISTED_FID_ENTRY); + assertThat(persistedFid1.readPersistedFidEntryValue()).isEqualTo(DEFAULT_PERSISTED_FID_ENTRY); } @Test diff --git a/firebase-installations/src/main/java/com/google/firebase/installations/FirebaseInstallations.java b/firebase-installations/src/main/java/com/google/firebase/installations/FirebaseInstallations.java index 84c45855ad0..8534abed912 100644 --- a/firebase-installations/src/main/java/com/google/firebase/installations/FirebaseInstallations.java +++ b/firebase-installations/src/main/java/com/google/firebase/installations/FirebaseInstallations.java @@ -178,8 +178,8 @@ String getName() { */ private PersistedFidEntry getPersistedFid() throws FirebaseInstallationsException { PersistedFidEntry persistedFidEntry = persistedFid.readPersistedFidEntryValue(); - if (persistedFidEntry == null - || persistedFidEntry.getRegistrationStatus() == RegistrationStatus.REGISTER_ERROR) { + + if (persistedFidEntry.isNotGenerated() || persistedFidEntry.isErrored()) { throw new FirebaseInstallationsException( "Failed to get existing fid.", FirebaseInstallationsException.Status.CLIENT_ERROR); } @@ -242,7 +242,7 @@ private Task registerFidIfNecessary(AwaitListener listener) synchronized (persistedFid) { PersistedFidEntry persistedFidEntry = persistedFid.readPersistedFidEntryValue(); - if (persistedFidEntry == null) { + if (persistedFidEntry.isNotGenerated()) { throw new FirebaseInstallationsException( "Local storage has no persisted Fid entry.", FirebaseInstallationsException.Status.CLIENT_ERROR); @@ -265,7 +265,7 @@ private Task registerFidIfNecessary(AwaitListener listener) PersistedFidEntry updatedPersistedFidEntry = persistedFid.readPersistedFidEntryValue(); - if (updatedPersistedFidEntry == null) { + if (updatedPersistedFidEntry.isNotGenerated()) { throw new FirebaseInstallationsException( "Local storage has no persisted Fid entry.", FirebaseInstallationsException.Status.CLIENT_ERROR); @@ -346,7 +346,7 @@ private InstallationTokenResult refreshAuthTokenIfNecessary(int authTokenOption) PersistedFidEntry persistedFidEntry = persistedFid.readPersistedFidEntryValue(); - if (persistedFidEntry == null + if (persistedFidEntry.isNotGenerated() || persistedFidEntry.isUnregistered() || persistedFidEntry.isErrored()) { throw new FirebaseInstallationsException( @@ -438,7 +438,7 @@ private Void deleteFirebaseInstallationId() throws FirebaseInstallationsExceptio PersistedFidEntry persistedFidEntry = persistedFid.readPersistedFidEntryValue(); - if (persistedFidEntry != null && persistedFidEntry.isRegistered()) { + if (persistedFidEntry.isRegistered()) { // Call the FIS servers to delete this firebase installation id. try { serviceClient.deleteFirebaseInstallation( diff --git a/firebase-installations/src/main/java/com/google/firebase/installations/local/PersistedFid.java b/firebase-installations/src/main/java/com/google/firebase/installations/local/PersistedFid.java index 1da104fa350..0ae59267529 100644 --- a/firebase-installations/src/main/java/com/google/firebase/installations/local/PersistedFid.java +++ b/firebase-installations/src/main/java/com/google/firebase/installations/local/PersistedFid.java @@ -31,6 +31,8 @@ public class PersistedFid { // NOTE: never change the ordinal of the enum values because the enum values are stored in shared // prefs as their ordinal numbers. public enum RegistrationStatus { + /** {@link PersistedFidEntry} default registration status */ + NOT_GENERATED, /** {@link PersistedFidEntry} is synced to FIS servers */ REGISTERED, /** {@link PersistedFidEntry} is not synced with FIS server */ @@ -75,7 +77,7 @@ public PersistedFid(@NonNull FirebaseApp firebaseApp) { @Nullable public PersistedFidEntry readPersistedFidEntryValue() { String fid = prefs.getString(getSharedPreferencesKey(FIREBASE_INSTALLATION_ID_KEY), null); - int status = prefs.getInt(getSharedPreferencesKey(PERSISTED_STATUS_KEY), -1); + int status = prefs.getInt(getSharedPreferencesKey(PERSISTED_STATUS_KEY), 0); String authToken = prefs.getString(getSharedPreferencesKey(AUTH_TOKEN_KEY), null); String refreshToken = prefs.getString(getSharedPreferencesKey(REFRESH_TOKEN_KEY), null); long tokenCreationTime = @@ -85,7 +87,7 @@ public PersistedFidEntry readPersistedFidEntryValue() { if (fid == null || status == -1 || !(status >= 0 && status < RegistrationStatus.values().length)) { - return null; + return PersistedFidEntry.builder().build(); } return PersistedFidEntry.builder() diff --git a/firebase-installations/src/main/java/com/google/firebase/installations/local/PersistedFidEntry.java b/firebase-installations/src/main/java/com/google/firebase/installations/local/PersistedFidEntry.java index 66791adea5a..4019967cdb6 100644 --- a/firebase-installations/src/main/java/com/google/firebase/installations/local/PersistedFidEntry.java +++ b/firebase-installations/src/main/java/com/google/firebase/installations/local/PersistedFidEntry.java @@ -25,7 +25,7 @@ @AutoValue public abstract class PersistedFidEntry { - @NonNull + @Nullable public abstract String getFirebaseInstallationId(); @NonNull @@ -53,6 +53,10 @@ public boolean isUnregistered() { return getRegistrationStatus() == PersistedFid.RegistrationStatus.UNREGISTERED; } + public boolean isNotGenerated() { + return getRegistrationStatus() == PersistedFid.RegistrationStatus.NOT_GENERATED; + } + @NonNull public abstract Builder toBuilder(); @@ -61,6 +65,7 @@ public boolean isUnregistered() { public static PersistedFidEntry.Builder builder() { return new AutoValue_PersistedFidEntry.Builder() .setTokenCreationEpochInSecs(0) + .setRegistrationStatus(PersistedFid.RegistrationStatus.NOT_GENERATED) .setExpiresInSecs(0); } From f7457d37671114e39fe7cf85187afb05b9e48909 Mon Sep 17 00:00:00 2001 From: Ankita Jhawar Date: Thu, 19 Sep 2019 10:30:02 -0700 Subject: [PATCH 11/11] Cleaning up the code for redudant checks. --- .../installations/FirebaseInstallations.java | 19 +++---------------- .../installations/local/PersistedFid.java | 4 +--- 2 files changed, 4 insertions(+), 19 deletions(-) diff --git a/firebase-installations/src/main/java/com/google/firebase/installations/FirebaseInstallations.java b/firebase-installations/src/main/java/com/google/firebase/installations/FirebaseInstallations.java index 8534abed912..436e3626c6e 100644 --- a/firebase-installations/src/main/java/com/google/firebase/installations/FirebaseInstallations.java +++ b/firebase-installations/src/main/java/com/google/firebase/installations/FirebaseInstallations.java @@ -237,7 +237,7 @@ private void persistFid(String fid) throws FirebaseInstallationsException { */ private Task registerFidIfNecessary(AwaitListener listener) throws FirebaseInstallationsException { - Boolean isNetworkCallRequired = false; + boolean isNetworkCallRequired = false; synchronized (persistedFid) { PersistedFidEntry persistedFidEntry = persistedFid.readPersistedFidEntryValue(); @@ -249,7 +249,7 @@ private Task registerFidIfNecessary(AwaitListener listener) } // If FID registration status is REGISTERED and auth token is expired, or registration status - // is UNREGISTERED network call iks required. + // is UNREGISTERED network call is required. if ((persistedFidEntry.isRegistered() && isAuthTokenExpired(persistedFidEntry)) || persistedFidEntry.isUnregistered()) { isNetworkCallRequired = true; @@ -264,13 +264,6 @@ private Task registerFidIfNecessary(AwaitListener listener) } PersistedFidEntry updatedPersistedFidEntry = persistedFid.readPersistedFidEntryValue(); - - if (updatedPersistedFidEntry.isNotGenerated()) { - throw new FirebaseInstallationsException( - "Local storage has no persisted Fid entry.", - FirebaseInstallationsException.Status.CLIENT_ERROR); - } - String fid = updatedPersistedFidEntry.getFirebaseInstallationId(); if (!isNetworkCallRequired) { @@ -302,7 +295,7 @@ private Task registerFidIfNecessary(AwaitListener listener) // Update the listener if awaiting if (listener != null) { - refreshAuthTokenTask.addOnCompleteListener((unused) -> listener.onSuccess()); + refreshAuthTokenTask.addOnSuccessListener((unused) -> listener.onSuccess()); } return Tasks.forResult(fid); } @@ -405,12 +398,6 @@ private InstallationTokenResult fetchAuthTokenFromServer(PersistedFidEntry persi return tokenResult; } catch (FirebaseInstallationServiceException exception) { - persistedFid.insertOrUpdatePersistedFidEntry( - persistedFidEntry - .toBuilder() - .setRegistrationStatus(RegistrationStatus.REGISTER_ERROR) - .build()); - throw new FirebaseInstallationsException( "Failed to generate auth token for a Firebase Installation.", FirebaseInstallationsException.Status.SDK_INTERNAL_ERROR); diff --git a/firebase-installations/src/main/java/com/google/firebase/installations/local/PersistedFid.java b/firebase-installations/src/main/java/com/google/firebase/installations/local/PersistedFid.java index 0ae59267529..d73219cd091 100644 --- a/firebase-installations/src/main/java/com/google/firebase/installations/local/PersistedFid.java +++ b/firebase-installations/src/main/java/com/google/firebase/installations/local/PersistedFid.java @@ -84,9 +84,7 @@ public PersistedFidEntry readPersistedFidEntryValue() { prefs.getLong(getSharedPreferencesKey(TOKEN_CREATION_TIME_IN_SECONDS_KEY), 0); long expiresIn = prefs.getLong(getSharedPreferencesKey(EXPIRES_IN_SECONDS_KEY), 0); - if (fid == null - || status == -1 - || !(status >= 0 && status < RegistrationStatus.values().length)) { + if (fid == null || !(status >= 0 && status < RegistrationStatus.values().length)) { return PersistedFidEntry.builder().build(); }