Skip to content

Commit 0a92e2d

Browse files
ankitaj224ankitaj224
committed
Addressing Ciaran's comments
1 parent 0dfa987 commit 0a92e2d

File tree

5 files changed

+43
-39
lines changed

5 files changed

+43
-39
lines changed

‎firebase-installations/src/androidTest/java/com/google/firebase/installations/FirebaseInstallationsInstrumentedTest.java

Lines changed: 19 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717
import static com.google.common.truth.Truth.assertWithMessage;
1818
import static com.google.firebase.installations.FisAndroidTestConstants.TEST_APP_ID_1;
1919
import static com.google.firebase.installations.FisAndroidTestConstants.TEST_AUTH_TOKEN;
20+
import static com.google.firebase.installations.FisAndroidTestConstants.TEST_CREATION_TIMESTAMP_1;
2021
import static com.google.firebase.installations.FisAndroidTestConstants.TEST_FID_1;
2122
import static com.google.firebase.installations.FisAndroidTestConstants.TEST_PROJECT_ID;
2223
import static com.google.firebase.installations.FisAndroidTestConstants.TEST_REFRESH_TOKEN;
@@ -28,6 +29,7 @@
2829

2930
import androidx.test.core.app.ApplicationProvider;
3031
import androidx.test.runner.AndroidJUnit4;
32+
import com.google.android.gms.common.util.Clock;
3133
import com.google.android.gms.tasks.Tasks;
3234
import com.google.firebase.FirebaseApp;
3335
import com.google.firebase.FirebaseOptions;
@@ -37,7 +39,7 @@
3739
import com.google.firebase.installations.remote.FirebaseInstallationServiceException;
3840
import com.google.firebase.installations.remote.InstallationResponse;
3941
import java.util.concurrent.ExecutionException;
40-
import java.util.concurrent.Executor;
42+
import java.util.concurrent.ExecutorService;
4143
import java.util.concurrent.SynchronousQueue;
4244
import java.util.concurrent.ThreadPoolExecutor;
4345
import java.util.concurrent.TimeUnit;
@@ -59,18 +61,19 @@
5961
@FixMethodOrder(MethodSorters.NAME_ASCENDING)
6062
public class FirebaseInstallationsInstrumentedTest {
6163
private FirebaseApp firebaseApp;
62-
private Executor executor;
64+
private ExecutorService executor;
6365
private PersistedFid persistedFid;
6466
@Mock private FirebaseInstallationServiceClient backendClientReturnsOk;
6567
@Mock private FirebaseInstallationServiceClient backendClientReturnsError;
6668
@Mock private PersistedFid persistedFidReturnsError;
6769
@Mock private Utils mockUtils;
70+
@Mock private Clock mockClock;
6871

6972
@Before
7073
public void setUp() throws FirebaseInstallationServiceException {
7174
MockitoAnnotations.initMocks(this);
7275
FirebaseApp.clearInstancesForTest();
73-
executor = new ThreadPoolExecutor(0, 2, 30L, TimeUnit.SECONDS, new SynchronousQueue<>());
76+
executor = new ThreadPoolExecutor(0, 2, 10L, TimeUnit.SECONDS, new SynchronousQueue<>());
7477
firebaseApp =
7578
FirebaseApp.initializeApp(
7679
ApplicationProvider.getApplicationContext(),
@@ -100,6 +103,7 @@ public void setUp() throws FirebaseInstallationServiceException {
100103
when(persistedFidReturnsError.insertOrUpdatePersistedFidEntry(any())).thenReturn(false);
101104
when(persistedFidReturnsError.readPersistedFidEntryValue()).thenReturn(null);
102105
when(mockUtils.createRandomFid()).thenReturn(TEST_FID_1);
106+
when(mockClock.currentTimeMillis()).thenReturn(TEST_CREATION_TIMESTAMP_1);
103107
}
104108

105109
@After
@@ -111,7 +115,7 @@ public void cleanUp() throws Exception {
111115
public void testGetId_PersistedFidOk_BackendOk() throws Exception {
112116
FirebaseInstallations firebaseInstallations =
113117
new FirebaseInstallations(
114-
executor, firebaseApp, backendClientReturnsOk, persistedFid, mockUtils);
118+
mockClock, executor, firebaseApp, backendClientReturnsOk, persistedFid, mockUtils);
115119

116120
// No exception, means success.
117121
assertWithMessage("getId Task fails.")
@@ -126,7 +130,7 @@ public void testGetId_PersistedFidOk_BackendOk() throws Exception {
126130
.isEqualTo(PersistedFid.RegistrationStatus.PENDING);
127131

128132
// Waiting for Task that registers FID on the FIS Servers
129-
Thread.sleep(500);
133+
executor.awaitTermination(500, TimeUnit.MILLISECONDS);
130134

131135
PersistedFidEntry updatedFidEntry = persistedFid.readPersistedFidEntryValue();
132136
assertWithMessage("Persisted Fid doesn't match")
@@ -141,7 +145,7 @@ public void testGetId_PersistedFidOk_BackendOk() throws Exception {
141145
public void testGetId_multipleCalls_sameFIDReturned() throws Exception {
142146
FirebaseInstallations firebaseInstallations =
143147
new FirebaseInstallations(
144-
executor, firebaseApp, backendClientReturnsOk, persistedFid, mockUtils);
148+
mockClock, executor, firebaseApp, backendClientReturnsOk, persistedFid, mockUtils);
145149

146150
// No exception, means success.
147151
assertWithMessage("getId Task fails.")
@@ -158,7 +162,7 @@ public void testGetId_multipleCalls_sameFIDReturned() throws Exception {
158162
Tasks.await(firebaseInstallations.getId());
159163

160164
// Waiting for Task that registers FID on the FIS Servers
161-
Thread.sleep(500);
165+
executor.awaitTermination(500, TimeUnit.MILLISECONDS);
162166

163167
PersistedFidEntry updatedFidEntry = persistedFid.readPersistedFidEntryValue();
164168
assertWithMessage("Persisted Fid doesn't match")
@@ -173,7 +177,7 @@ public void testGetId_multipleCalls_sameFIDReturned() throws Exception {
173177
public void testGetId_PersistedFidOk_BackendError() throws Exception {
174178
FirebaseInstallations firebaseInstallations =
175179
new FirebaseInstallations(
176-
executor, firebaseApp, backendClientReturnsError, persistedFid, mockUtils);
180+
mockClock, executor, firebaseApp, backendClientReturnsError, persistedFid, mockUtils);
177181

178182
Tasks.await(firebaseInstallations.getId());
179183

@@ -186,7 +190,7 @@ public void testGetId_PersistedFidOk_BackendError() throws Exception {
186190
.isEqualTo(PersistedFid.RegistrationStatus.PENDING);
187191

188192
// Waiting for Task that registers FID on the FIS Servers
189-
Thread.sleep(500);
193+
executor.awaitTermination(500, TimeUnit.MILLISECONDS);
190194

191195
PersistedFidEntry updatedFidEntry = persistedFid.readPersistedFidEntryValue();
192196
assertWithMessage("Persisted Fid doesn't match")
@@ -201,7 +205,12 @@ public void testGetId_PersistedFidOk_BackendError() throws Exception {
201205
public void testGetId_PersistedFidError_BackendOk() throws InterruptedException {
202206
FirebaseInstallations firebaseInstallations =
203207
new FirebaseInstallations(
204-
executor, firebaseApp, backendClientReturnsOk, persistedFidReturnsError, mockUtils);
208+
mockClock,
209+
executor,
210+
firebaseApp,
211+
backendClientReturnsOk,
212+
persistedFidReturnsError,
213+
mockUtils);
205214

206215
// Expect exception
207216
try {

‎firebase-installations/src/main/java/com/google/firebase/installations/FirebaseInstallations.java

Lines changed: 13 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@
2424
import com.google.android.gms.tasks.Tasks;
2525
import com.google.firebase.FirebaseApp;
2626
import com.google.firebase.installations.local.PersistedFid;
27+
import com.google.firebase.installations.local.PersistedFid.RegistrationStatus;
2728
import com.google.firebase.installations.local.PersistedFidEntry;
2829
import com.google.firebase.installations.remote.FirebaseInstallationServiceClient;
2930
import com.google.firebase.installations.remote.FirebaseInstallationServiceException;
@@ -50,12 +51,13 @@ public class FirebaseInstallations implements FirebaseInstallationsApi {
5051
private final FirebaseInstallationServiceClient serviceClient;
5152
private final PersistedFid persistedFid;
5253
private final Executor executor;
53-
/*package*/ static Clock clock = DefaultClock.getInstance();
54+
private final Clock clock;
5455
private final Utils utils;
5556

5657
/** package private constructor. */
5758
FirebaseInstallations(FirebaseApp firebaseApp) {
5859
this(
60+
DefaultClock.getInstance(),
5961
new ThreadPoolExecutor(0, 1, 30L, TimeUnit.SECONDS, new SynchronousQueue<>()),
6062
firebaseApp,
6163
new FirebaseInstallationServiceClient(),
@@ -64,11 +66,13 @@ public class FirebaseInstallations implements FirebaseInstallationsApi {
6466
}
6567

6668
FirebaseInstallations(
69+
Clock clock,
6770
Executor executor,
6871
FirebaseApp firebaseApp,
6972
FirebaseInstallationServiceClient serviceClient,
7073
PersistedFid persistedFid,
7174
Utils utils) {
75+
this.clock = clock;
7276
this.firebaseApp = firebaseApp;
7377
this.serviceClient = serviceClient;
7478
this.executor = executor;
@@ -107,7 +111,7 @@ public static FirebaseInstallations getInstance(@NonNull FirebaseApp app) {
107111
@Override
108112
public Task<String> getId() {
109113
return Tasks.call(executor, this::getPersistedFid)
110-
.continueWith(orElse(new NewFidSupplier()))
114+
.continueWith(orElse(this::createAndPersistNewFid))
111115
.onSuccessTask(this::registerFidIfNecessary);
112116
}
113117

@@ -152,8 +156,7 @@ private PersistedFidEntry getPersistedFid() throws FirebaseInstallationsExceptio
152156

153157
private static boolean persistedFidMissingOrInErrorState(PersistedFidEntry persistedFidEntry) {
154158
return persistedFidEntry == null
155-
|| persistedFidEntry.getRegistrationStatus()
156-
== PersistedFid.RegistrationStatus.REGISTER_ERROR;
159+
|| persistedFidEntry.getRegistrationStatus() == RegistrationStatus.REGISTER_ERROR;
157160
}
158161

159162
@NonNull
@@ -178,7 +181,7 @@ private void persistFid(String fid) throws FirebaseInstallationsException {
178181
persistedFid.insertOrUpdatePersistedFidEntry(
179182
PersistedFidEntry.builder()
180183
.setFirebaseInstallationId(fid)
181-
.setRegistrationStatus(PersistedFid.RegistrationStatus.UNREGISTERED)
184+
.setRegistrationStatus(RegistrationStatus.UNREGISTERED)
182185
.build());
183186

184187
if (!firstUpdateCacheResult) {
@@ -192,7 +195,7 @@ private Task<String> registerFidIfNecessary(PersistedFidEntry persistedFidEntry)
192195
String fid = persistedFidEntry.getFirebaseInstallationId();
193196

194197
// Check if the fid is unregistered
195-
if (persistedFidEntry.getRegistrationStatus() == PersistedFid.RegistrationStatus.UNREGISTERED) {
198+
if (persistedFidEntry.getRegistrationStatus() == RegistrationStatus.UNREGISTERED) {
196199
updatePersistedFidWithPendingStatus(fid);
197200
Tasks.call(executor, () -> registerAndSaveFid(persistedFidEntry));
198201
}
@@ -203,15 +206,15 @@ private void updatePersistedFidWithPendingStatus(String fid) {
203206
persistedFid.insertOrUpdatePersistedFidEntry(
204207
PersistedFidEntry.builder()
205208
.setFirebaseInstallationId(fid)
206-
.setRegistrationStatus(PersistedFid.RegistrationStatus.PENDING)
209+
.setRegistrationStatus(RegistrationStatus.PENDING)
207210
.build());
208211
}
209212

210213
/** Registers the created Fid with FIS servers and update the shared prefs. */
211214
private Void registerAndSaveFid(PersistedFidEntry persistedFidEntry)
212215
throws FirebaseInstallationsException {
213216
try {
214-
long creationTime = clock.currentTimeMillis() / 1000;
217+
long creationTime = TimeUnit.MILLISECONDS.toSeconds(clock.currentTimeMillis());
215218

216219
InstallationResponse installationResponse =
217220
serviceClient.createFirebaseInstallation(
@@ -222,7 +225,7 @@ private Void registerAndSaveFid(PersistedFidEntry persistedFidEntry)
222225
persistedFid.insertOrUpdatePersistedFidEntry(
223226
PersistedFidEntry.builder()
224227
.setFirebaseInstallationId(persistedFidEntry.getFirebaseInstallationId())
225-
.setRegistrationStatus(PersistedFid.RegistrationStatus.REGISTERED)
228+
.setRegistrationStatus(RegistrationStatus.REGISTERED)
226229
.setAuthToken(installationResponse.getAuthToken().getToken())
227230
.setRefreshToken(installationResponse.getRefreshToken())
228231
.setExpiresInSecs(
@@ -234,21 +237,13 @@ private Void registerAndSaveFid(PersistedFidEntry persistedFidEntry)
234237
persistedFid.insertOrUpdatePersistedFidEntry(
235238
PersistedFidEntry.builder()
236239
.setFirebaseInstallationId(persistedFidEntry.getFirebaseInstallationId())
237-
.setRegistrationStatus(PersistedFid.RegistrationStatus.REGISTER_ERROR)
240+
.setRegistrationStatus(RegistrationStatus.REGISTER_ERROR)
238241
.build());
239242
throw new FirebaseInstallationsException(
240243
exception.getMessage(), FirebaseInstallationsException.Status.SDK_INTERNAL_ERROR);
241244
}
242245
return null;
243246
}
244-
245-
class NewFidSupplier implements Supplier<PersistedFidEntry> {
246-
247-
@Override
248-
public PersistedFidEntry get() throws FirebaseInstallationsException {
249-
return createAndPersistNewFid();
250-
}
251-
}
252247
}
253248

254249
interface Supplier<T> {

‎firebase-installations/src/main/java/com/google/firebase/installations/Utils.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,7 @@
2020
import java.util.UUID;
2121

2222
/** Util methods used for {@link FirebaseInstallations} */
23-
public class Utils {
23+
class Utils {
2424

2525
/**
2626
* 1 Byte with the first 4 header-bits set to the identifying FID prefix 0111 (0x7). Use this

‎firebase-installations/src/main/java/com/google/firebase/installations/local/PersistedFid.java

Lines changed: 6 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -27,18 +27,17 @@
2727
* Installation API.
2828
*/
2929
public class PersistedFid {
30-
// Status of each presisted fid entry
30+
// Registration Status of each persisted fid entry
3131
// NOTE: never change the ordinal of the enum values because the enum values are stored in shared
32-
// prefs
33-
// as their ordinal numbers.
32+
// prefs as their ordinal numbers.
3433
public enum RegistrationStatus {
35-
// Persisted Fid entry is synced to FIS servers
34+
/** {@link PersistedFidEntry} is synced to FIS servers */
3635
REGISTERED,
37-
// Persisted Fid entry is not synced with FIS server
36+
/** {@link PersistedFidEntry} is not synced with FIS server */
3837
UNREGISTERED,
39-
// Persisted Fid entry is in error state when syncing with FIS server
38+
/** {@link PersistedFidEntry} is in error state when syncing with FIS server */
4039
REGISTER_ERROR,
41-
// Persisted Fid entry is in pending state when waiting for FIS server response
40+
/** {@link PersistedFidEntry} is in pending state when waiting for FIS server response */
4241
PENDING
4342
}
4443

‎firebase-installations/src/main/java/com/google/firebase/installations/remote/FirebaseInstallationServiceClient.java

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -49,7 +49,7 @@ public class FirebaseInstallationServiceClient {
4949
private static final String NETWORK_ERROR_MESSAGE = "The server returned an unexpected error:";
5050

5151
/**
52-
* Creates a FID on the FIS Servers by calling create API.
52+
* Creates a FID on the FIS Servers by calling FirebaseInstallations API create method.
5353
*
5454
* @param apiKey API Key that has access to FIS APIs
5555
* @param fid Firebase Installation Identifier
@@ -118,7 +118,7 @@ private static JSONObject buildCreateFirebaseInstallationRequestBody(String fid,
118118
}
119119

120120
/**
121-
* Deletes a FID on the FIS Servers by calling delete API.
121+
* Deletes a FID on the FIS Servers by calling FirebaseInstallations API delete method.
122122
*
123123
* @param apiKey API Key that has access to FIS APIs
124124
* @param fid Firebase Installation Identifier
@@ -170,7 +170,8 @@ public void deleteFirebaseInstallation(
170170
}
171171

172172
/**
173-
* Generates a new auth token for a FID on the FIS Servers by calling generateAuthToken API.
173+
* Generates a new auth token for a FID on the FIS Servers by calling FirebaseInstallations API
174+
* generateAuthToken method.
174175
*
175176
* @param apiKey API Key that has access to FIS APIs
176177
* @param fid Firebase Installation Identifier

0 commit comments

Comments
 (0)