Implement platform logging for App Check #2619
Conversation
Coverage ReportAffected SDKs
Test Logs
NotesHTML coverage reports can be produced locally with Head commit (ed65901d) is created by Prow via merging commits: 1cbfea5 2fbeb17. |
Binary Size ReportAffected SDKs
Test Logs
NotesHead commit (ed65901d) is created by Prow via merging commits: 1cbfea5 2fbeb17. |
| public NetworkClient(@NonNull FirebaseApp firebaseApp) { | ||
| checkNotNull(firebaseApp); | ||
| this.firebaseAppCheck = (DefaultFirebaseAppCheck) FirebaseAppCheck.getInstance(firebaseApp); | ||
| this.apiKey = firebaseApp.getOptions().getApiKey(); | ||
| this.appId = firebaseApp.getOptions().getApplicationId(); | ||
| this.projectId = firebaseApp.getOptions().getProjectId(); |
There was a problem hiding this comment.
Consider go/di-practices/inject-direct-dependencies
There was a problem hiding this comment.
Thanks for the link! I passed FirebaseApp directly into NetworkClient to avoid adding the :appcheck:firebase-appcheck-interop dependency to firebase-appcheck-safetynet.gradle and firebase-appcheck-debug.gradle (for accessing FirebaseAppCheck#getInstance).
I think it's fine to add the interop dependency, but it seems cleaner to only have that dependency in the core App Check SDK.
There was a problem hiding this comment.
I see, one thing I would suggest is to consider injecting the NetworkClient via firebase components instead of having both safetynet and debug sdks creating instantiating it on their own. This would also allow you to register both of these sdks with the platform logging user-agent.
(Optional)
There was a problem hiding this comment.
Ack, that makes sense! I've filed b/186549222 to follow up on this in a future PR.
There was a problem hiding this comment.
Overall LGTM,
Please get a review by @VinayGuthal before merging
| public NetworkClient(@NonNull FirebaseApp firebaseApp) { | ||
| checkNotNull(firebaseApp); | ||
| this.firebaseAppCheck = (DefaultFirebaseAppCheck) FirebaseAppCheck.getInstance(firebaseApp); | ||
| this.apiKey = firebaseApp.getOptions().getApiKey(); | ||
| this.appId = firebaseApp.getOptions().getApplicationId(); | ||
| this.projectId = firebaseApp.getOptions().getProjectId(); |
There was a problem hiding this comment.
I see, one thing I would suggest is to consider injecting the NetworkClient via firebase components instead of having both safetynet and debug sdks creating instantiating it on their own. This would also allow you to register both of these sdks with the platform logging user-agent.
(Optional)
No description provided.