getId() implementation with instrumented tests. #703
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -22,7 +22,7 @@ | |
| import com.google.android.gms.tasks.Tasks; | ||
| import com.google.firebase.FirebaseApp; | ||
| import com.google.firebase.installations.local.PersistedFid; | ||
| import com.google.firebase.installations.local.PersistedFidEntry; | ||
| import com.google.firebase.installations.remote.FirebaseInstallationServiceClient; | ||
| import com.google.firebase.installations.remote.FirebaseInstallationServiceException; | ||
| import com.google.firebase.installations.remote.InstallationResponse; | ||
|
|
@@ -147,7 +147,7 @@ String getName() { | |
| * | ||
| * Persisted Fid exists and is in Unregistered state | ||
| * | | ||
| * Register the Fid on FIS Server and update the PersistedFidEntry | ||
| * | | ||
| * return the Persisted Fid | ||
| * | ||
|
|
@@ -156,53 +156,47 @@ String getName() { | |
| @WorkerThread | ||
| private String getFirebaseInstallationId() throws FirebaseInstallationsException { | ||
|
|
||
| PersistedFidEntry persistedFidEntry = persistedFid.readPersistedFidEntryValue(); | ||
|
|
||
| if (persistedFidExistsAndRegistered(persistedFidEntry)) { | ||
| return persistedFidEntry.getFirebaseInstallationId(); | ||
| } | ||
|
|
||
| if (persistedFidMissingOrInErrorState(persistedFidEntry)) { | ||
| return createFidAndPersistFidEntry(); | ||
| } | ||
|
|
||
| if (persistedFidExistsAndUnregistered(persistedFidEntry)) { | ||
| Tasks.call(executor, () -> registerAndSaveFid(persistedFidEntry.getFirebaseInstallationId())); | ||
| return persistedFidEntry.getFirebaseInstallationId(); | ||
| } | ||
|
|
||
| return null; | ||
|
There was a problem hiding this comment. How can this method ever return NULL? There was a problem hiding this comment. If delete() is called and there is no network availability, we mark registration status as deleted. In that case, it returns NULL. Is that not right? There was a problem hiding this comment. @andirayo Can you please check this? I think the issue is, we have introduced a state DELETED as per fis-android-sdk-design doc. But from your comments on my doc, seems like you don't want the DELETED state. We can change the above function as : private String getFirebaseInstallationId() throws FirebaseInstallationsException { Wdyt? There was a problem hiding this comment. Except for the "else if" in there (an else is not necessary after you "return" in the if-statement, I like it better yes. How about return createFidAndPersistFidEntry(); There was a problem hiding this comment. There was a problem hiding this comment. what do you all think about something like this? I haven't checked that it'll compile, but it seems to clearly express what we're trying to do:
public Task<FID> getFid() {
return getExistingFid()
.continueWith(orElse(this::createAndPersistNewFid))
.onSuccessTask(this::registerFidIfNecessary);
}
public static <F, T> Continuation<F, T> orElse(Supplier<T> supplier) {
return t -> {
if (t.isSuccessful()) { return t.getResult(); }
return supplier.get();
};
}
private FID createAndPersistNewFid() {
FID fid = Utils.createRandomFid();
persist(fid);
return fid;
}
private void registerFidIfNecessary(FID fid) {
// do network stuff
}There was a problem hiding this comment. @ciarand I tried incorporating the changes you mentioned below: There was a problem hiding this comment. I have 2 concerns:
There was a problem hiding this comment. Yeah we can't use the standard java.util.functional.Supplier interface. We can just define one in the parent class: When you say "Exception handling by the Supplier while persisting the newly created fid" do you mean you're worried that you can't propagate exceptions or you're worried that they will be propagated? There was a problem hiding this comment. Updated. Thanks for the comments. There was a problem hiding this comment. mark it |
||
| } | ||
|
|
||
| private static boolean persistedFidMissingOrInErrorState(PersistedFidEntry persistedFidEntry) { | ||
| return persistedFidEntry == null | ||
| || persistedFidEntry.getPersistedStatus() == PersistedFid.PersistedStatus.REGISTER_ERROR; | ||
| } | ||
|
|
||
| private static boolean persistedFidExistsAndRegistered(PersistedFidEntry persistedFidEntry) { | ||
| return persistedFidEntry != null | ||
| && persistedFidEntry.getPersistedStatus() == PersistedFid.PersistedStatus.REGISTERED; | ||
| } | ||
|
|
||
| private static boolean persistedFidExistsAndUnregistered(PersistedFidEntry persistedFidEntry) { | ||
ankitaj224 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| return persistedFidEntry != null | ||
| && persistedFidEntry.getPersistedStatus() == PersistedFid.PersistedStatus.UNREGISTERED; | ||
| } | ||
|
|
||
| private String createFidAndPersistFidEntry() throws FirebaseInstallationsException { | ||
ankitaj224 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| String fid = Utils.createRandomFid(); | ||
|
|
||
| boolean firstUpdateCacheResult = | ||
| persistedFid.insertOrUpdatePersistedFidEntry( | ||
| PersistedFidEntry.builder() | ||
| .setFirebaseInstallationId(fid) | ||
| .setPersistedStatus(PersistedFid.PersistedStatus.UNREGISTERED) | ||
| .build()); | ||
|
|
||
| if (!firstUpdateCacheResult) { | ||
|
|
@@ -211,18 +205,17 @@ private String createFidAndPersistFidEntry() throws FirebaseInstallationsExcepti | |
| FirebaseInstallationsException.Status.CLIENT_ERROR); | ||
| } | ||
|
|
||
| Tasks.call(executor, () -> registerAndSaveFid(fid)); | ||
| return fid; | ||
| } | ||
|
|
||
| /** | ||
| * Registers the created Fid with FIS Servers if the Network is available and update the shared | ||
| * prefs. | ||
| */ | ||
| private Void registerAndSaveFid(String fid) throws FirebaseInstallationsException { | ||
| if (!Utils.isNetworkAvailable(firebaseApp.getApplicationContext())) { | ||
| return null; | ||
| } | ||
|
|
||
| try { | ||
|
|
@@ -236,7 +229,7 @@ private void registerAndSaveFid(String fid) throws FirebaseInstallationsExceptio | |
| getApplicationId()); | ||
|
|
||
| persistedFid.insertOrUpdatePersistedFidEntry( | ||
| PersistedFidEntry.builder() | ||
| .setFirebaseInstallationId(fid) | ||
| .setPersistedStatus(PersistedFid.PersistedStatus.REGISTERED) | ||
| .setAuthToken(installationResponse.getAuthToken().getToken()) | ||
|
|
@@ -248,14 +241,13 @@ private void registerAndSaveFid(String fid) throws FirebaseInstallationsExceptio | |
|
|
||
| } catch (FirebaseInstallationServiceException exception) { | ||
| persistedFid.insertOrUpdatePersistedFidEntry( | ||
ankitaj224 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| PersistedFidEntry.builder() | ||
| .setFirebaseInstallationId(fid) | ||
| .setPersistedStatus(PersistedFid.PersistedStatus.REGISTER_ERROR) | ||
| .build()); | ||
| throw new FirebaseInstallationsException( | ||
| exception.getMessage(), FirebaseInstallationsException.Status.SDK_INTERNAL_ERROR); | ||
| } | ||
| return null; | ||
| } | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the difference between PersistedFid and PersistedFidEntry?