Skip to content

Make Auth an deferred dependency #2418

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

Jump to bottom
Merged
merged 6 commits into from
Mar 1, 2021
Merged

Make Auth an deferred dependency #2418

merged 6 commits into from
Mar 1, 2021

Conversation

schmidt-sebastian
Copy link
Contributor

@schmidt-sebastian schmidt-sebastian commented Feb 8, 2021 •
edited
Loading

Support Auth as an optional dependencies.

Tested manually via test app (which does not yet use actual dynamic module support).

@google-cla google-cla bot added the cla: yes Override cla label Feb 8, 2021
@googlebot googlebot added the cla: yes Override cla label Feb 8, 2021
@google-oss-bot
Copy link
Contributor

google-oss-bot commented Feb 8, 2021 •
edited
Loading

Coverage Report

Affected SDKs

  • firebase-database

    SDK overall coverage changed from 49.79% (794d8f5) to 50.13% (4a62ae37) by +0.33%.

    Filename Base (794d8f5) Head (4a62ae37) Diff
    AndroidAuthTokenProvider.java 19.23% 42.31% +23.08%
    BooleanNode.java 92.31% 100.00% +7.69%
    Connection.java 22.41% 27.59% +5.17%
    FirebaseDatabaseComponent.java 94.12% 100.00% +5.88%
    PersistentConnectionImpl.java 16.16% 15.70% -0.46%
    Repo.java 9.42% 9.96% +0.54%
    SparseSnapshotTree.java 7.14% 17.86% +10.71%
    WebsocketConnection.java 26.14% 32.39% +6.25%
    WriteTree.java 77.22% 76.67% -0.56%

Test Logs

Notes

HTML coverage reports can be produced locally with ./gradlew <product>:checkCoverage.
Report files are located at <product-build-dir>/reports/jacoco/.

Head commit (4a62ae37) is created by Prow via merging commits: 794d8f5 72c99d0.

@google-oss-bot
Copy link
Contributor

google-oss-bot commented Feb 8, 2021 •
edited
Loading

Binary Size Report

Affected SDKs

  • firebase-database

    Type Base (794d8f5) Head (4a62ae37) Diff
    aar 493 kB 493 kB -479 B (-0.1%)
    apk (aggressive) 282 kB 283 kB +572 B (+0.2%)
    apk (release) 1.09 MB 1.09 MB +44 B (+0.0%)

Test Logs

Notes

Head commit (4a62ae37) is created by Prow via merging commits: 794d8f5 72c99d0.

vkryachko
vkryachko reviewed Feb 9, 2021
View reviewed changes
Copy link
Member

@vkryachko vkryachko left a comment

Choose a reason for hiding this comment

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

The general direction LGTM

...se-database/src/main/java/com/google/firebase/database/android/AndroidAuthTokenProvider.java Outdated
executorService.execute(
() -> tokenListener.onTokenChange(/* nullable */ tokenResult.getToken()));
deferredAuthProvider.whenAvailable(
// TODO: Could "whenAvailable" return the Auth instance rather than a provider for Auth?
Copy link
Member

Choose a reason for hiding this comment

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

The return of the provider is intentional since it's more general and makes things lazy by default - so that we don't have to initialize with inside components unless get() is called on its provider.

Copy link
Contributor

Choose a reason for hiding this comment

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

Could Deferred be modified to extend Provider? As I'm working on changes for Firestore, this would be incredibly convenient because some usages need to react instantly when the provider becomes available (i.e. via whenAvailable()) and other instances need to get the instance and do something with it if it is non-null (which would be achieved by calling get()).

Since OptionalProvider already implements both Deferred and Provider, this would be an incredibly easy change to implement. Also, it would satisfy @schmidt-sebastian's request for an isAvailable() method.

Copy link
Contributor

Choose a reason for hiding this comment

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

@vkryachko I've implemented this suggestion in #2432. I'll officially add you both as reviewers once the GitHub Actions pass.

@dconeybe dconeybe mentioned this pull request Feb 11, 2021
Merged
@vkryachko
Copy link
Member

Hey, firendly ping. Is there anything else needed to productionize this?

FYI the test failure needs some attention

UnitTestHelpers.java:170: error: cannot find symbol
    config.setAuthTokenProvider(AndroidAuthTokenProvider.forUnauthenticatedAccess());

@schmidt-sebastian schmidt-sebastian changed the title Draft: Make Auth an deferred dependency Make Auth an deferred dependency Mar 1, 2021
@schmidt-sebastian
Copy link
Contributor Author

I fixed the compile failure. I don't think there is much else to do here until we have an actual test case that takes advantage of this feature (but this shouldn't block this PR). I will try to get it past the flaky tests today.

@schmidt-sebastian
Copy link
Contributor Author

/test device-check-changed

@schmidt-sebastian
Copy link
Contributor Author

/test device-check-changed

2 similar comments
@schmidt-sebastian
Copy link
Contributor Author

/test device-check-changed

@schmidt-sebastian
Copy link
Contributor Author

/test device-check-changed

@schmidt-sebastian schmidt-sebastian merged commit 5974c5e into master Mar 1, 2021
@schmidt-sebastian schmidt-sebastian deleted the mrschmidt/deferredauth branch March 1, 2021 20:57
@firebase firebase locked and limited conversation to collaborators Apr 1, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@dconeybe dconeybe dconeybe left review comments

@vkryachko vkryachko vkryachko approved these changes

Assignees

@vkryachko vkryachko

Labels
api: database cla: yes Override cla size/L
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@schmidt-sebastian @google-oss-bot @vkryachko @dconeybe @googlebot