Remove common dependency in AppCheck Interop #4782
Conversation
Javadoc Changes:--- /Users/runner/diff/original/firebase-kotlindoc/android/com/google/firebase/appcheck/FirebaseAppCheck.html 2023-04-17 19:44:00.000000000 +0000
+++ /Users/runner/diff/modified/firebase-kotlindoc/android/com/google/firebase/appcheck/FirebaseAppCheck.html 2023-04-17 19:38:15.000000000 +0000
@@ -8,7 +8,7 @@
<div id="metadata-info-block"></div>
<h1>FirebaseAppCheck</h1>
<p>
- <pre>public abstract class <a href="/docs/reference/android/com/google/firebase/appcheck/FirebaseAppCheck.html">FirebaseAppCheck</a> implements <a href="/docs/reference/android/com/google/firebase/appcheck/interop/InternalAppCheckTokenProvider.html">InternalAppCheckTokenProvider</a></pre>
+ <pre>public abstract class <a href="/docs/reference/android/com/google/firebase/appcheck/FirebaseAppCheck.html">FirebaseAppCheck</a> implements <a href="/docs/reference/android/com/google/firebase/appcheck/interop/InteropAppCheckTokenProvider.html">InteropAppCheckTokenProvider</a></pre>
</p>
<hr>
<h2>Summary</h2>
@@ -139,7 +139,7 @@
</thead>
<tbody class="list">
<tr>
- <td><devsite-expandable><span class="expand-control">From <a href="/docs/reference/android/com/google/firebase/appcheck/interop/InternalAppCheckTokenProvider.html">com.google.firebase.appcheck.interop.InternalAppCheckTokenProvider</a></span>
+ <td><devsite-expandable><span class="expand-control">From <a href="/docs/reference/android/com/google/firebase/appcheck/interop/InteropAppCheckTokenProvider.html">com.google.firebase.appcheck.interop.InteropAppCheckTokenProvider</a></span>
<div class="devsite-table-wrapper">
<table class="responsive">
<colgroup>
@@ -150,19 +150,19 @@
<tr>
<td><code>abstract void</code></td>
<td>
- <div><code><a href="/docs/reference/android/com/google/firebase/appcheck/interop/InternalAppCheckTokenProvider.html#addAppCheckTokenListener(com.google.firebase.appcheck.interop.AppCheckTokenListener)">addAppCheckTokenListener</a>(<a href="/docs/reference/android/com/google/firebase/appcheck/interop/AppCheckTokenListener.html">AppCheckTokenListener</a> p)</code></div>
+ <div><code><a href="/docs/reference/android/com/google/firebase/appcheck/interop/InteropAppCheckTokenProvider.html#addAppCheckTokenListener(com.google.firebase.appcheck.interop.AppCheckTokenListener)">addAppCheckTokenListener</a>(<a href="/docs/reference/android/com/google/firebase/appcheck/interop/AppCheckTokenListener.html">AppCheckTokenListener</a> p)</code></div>
</td>
</tr>
<tr>
<td><code>abstract <a href="https://developers.google.com/android/reference/com/google/android/gms/tasks/Task.html">Task</a><<a href="/docs/reference/android/com/google/firebase/appcheck/AppCheckTokenResult.html">AppCheckTokenResult</a>></code></td>
<td>
- <div><code><a href="/docs/reference/android/com/google/firebase/appcheck/interop/InternalAppCheckTokenProvider.html#getToken(boolean)">getToken</a>(boolean p)</code></div>
+ <div><code><a href="/docs/reference/android/com/google/firebase/appcheck/interop/InteropAppCheckTokenProvider.html#getToken(boolean)">getToken</a>(boolean p)</code></div>
</td>
</tr>
<tr>
<td><code>abstract void</code></td>
<td>
- <div><code><a href="/docs/reference/android/com/google/firebase/appcheck/interop/InternalAppCheckTokenProvider.html#removeAppCheckTokenListener(com.google.firebase.appcheck.interop.AppCheckTokenListener)">removeAppCheckTokenListener</a>(<a href="/docs/reference/android/com/google/firebase/appcheck/interop/AppCheckTokenListener.html">AppCheckTokenListener</a> p)</code></div>
+ <div><code><a href="/docs/reference/android/com/google/firebase/appcheck/interop/InteropAppCheckTokenProvider.html#removeAppCheckTokenListener(com.google.firebase.appcheck.interop.AppCheckTokenListener)">removeAppCheckTokenListener</a>(<a href="/docs/reference/android/com/google/firebase/appcheck/interop/AppCheckTokenListener.html">AppCheckTokenListener</a> p)</code></div>
</td>
</tr>
</tbody>--- /Users/runner/diff/original/firebase-kotlindoc/kotlin/com/google/firebase/appcheck/FirebaseAppCheck.html 2023-04-17 19:44:00.000000000 +0000
+++ /Users/runner/diff/modified/firebase-kotlindoc/kotlin/com/google/firebase/appcheck/FirebaseAppCheck.html 2023-04-17 19:38:15.000000000 +0000
@@ -8,7 +8,7 @@
<div id="metadata-info-block"></div>
<h1>FirebaseAppCheck</h1>
<p>
- <pre>abstract class <a href="/docs/reference/kotlin/com/google/firebase/appcheck/FirebaseAppCheck.html">FirebaseAppCheck</a> : <a href="/docs/reference/kotlin/com/google/firebase/appcheck/interop/InternalAppCheckTokenProvider.html">InternalAppCheckTokenProvider</a></pre>
+ <pre>abstract class <a href="/docs/reference/kotlin/com/google/firebase/appcheck/FirebaseAppCheck.html">FirebaseAppCheck</a> : <a href="/docs/reference/kotlin/com/google/firebase/appcheck/interop/InteropAppCheckTokenProvider.html">InteropAppCheckTokenProvider</a></pre>
</p>
<hr>
<h2>Summary</h2>
@@ -139,7 +139,7 @@
</thead>
<tbody class="list">
<tr>
- <td><devsite-expandable><span class="expand-control">From <a href="/docs/reference/kotlin/com/google/firebase/appcheck/interop/InternalAppCheckTokenProvider.html">com.google.firebase.appcheck.interop.InternalAppCheckTokenProvider</a></span>
+ <td><devsite-expandable><span class="expand-control">From <a href="/docs/reference/kotlin/com/google/firebase/appcheck/interop/InteropAppCheckTokenProvider.html">com.google.firebase.appcheck.interop.InteropAppCheckTokenProvider</a></span>
<div class="devsite-table-wrapper">
<table class="responsive">
<colgroup>
@@ -150,19 +150,19 @@
<tr>
<td><code>abstract <a href="https://kotlinlang.org/api/latest/jvm/stdlib/kotlin/-unit/index.html">Unit</a></code></td>
<td>
- <div><code><a href="/docs/reference/kotlin/com/google/firebase/appcheck/interop/InternalAppCheckTokenProvider.html#addAppCheckTokenListener(com.google.firebase.appcheck.interop.AppCheckTokenListener)">addAppCheckTokenListener</a>(p: <a href="/docs/reference/kotlin/com/google/firebase/appcheck/interop/AppCheckTokenListener.html">AppCheckTokenListener</a>!)</code></div>
+ <div><code><a href="/docs/reference/kotlin/com/google/firebase/appcheck/interop/InteropAppCheckTokenProvider.html#addAppCheckTokenListener(com.google.firebase.appcheck.interop.AppCheckTokenListener)">addAppCheckTokenListener</a>(p: <a href="/docs/reference/kotlin/com/google/firebase/appcheck/interop/AppCheckTokenListener.html">AppCheckTokenListener</a>!)</code></div>
</td>
</tr>
<tr>
<td><code>abstract <a href="https://developers.google.com/android/reference/com/google/android/gms/tasks/Task.html">Task</a><<a href="/docs/reference/kotlin/com/google/firebase/appcheck/AppCheckTokenResult.html">AppCheckTokenResult</a>!>!</code></td>
<td>
- <div><code><a href="/docs/reference/kotlin/com/google/firebase/appcheck/interop/InternalAppCheckTokenProvider.html#getToken(boolean)">getToken</a>(p: <a href="https://kotlinlang.org/api/latest/jvm/stdlib/kotlin/-boolean/index.html">Boolean</a>)</code></div>
+ <div><code><a href="/docs/reference/kotlin/com/google/firebase/appcheck/interop/InteropAppCheckTokenProvider.html#getToken(boolean)">getToken</a>(p: <a href="https://kotlinlang.org/api/latest/jvm/stdlib/kotlin/-boolean/index.html">Boolean</a>)</code></div>
</td>
</tr>
<tr>
<td><code>abstract <a href="https://kotlinlang.org/api/latest/jvm/stdlib/kotlin/-unit/index.html">Unit</a></code></td>
<td>
- <div><code><a href="/docs/reference/kotlin/com/google/firebase/appcheck/interop/InternalAppCheckTokenProvider.html#removeAppCheckTokenListener(com.google.firebase.appcheck.interop.AppCheckTokenListener)">removeAppCheckTokenListener</a>(p: <a href="/docs/reference/kotlin/com/google/firebase/appcheck/interop/AppCheckTokenListener.html">AppCheckTokenListener</a>!)</code></div>
+ <div><code><a href="/docs/reference/kotlin/com/google/firebase/appcheck/interop/InteropAppCheckTokenProvider.html#removeAppCheckTokenListener(com.google.firebase.appcheck.interop.AppCheckTokenListener)">removeAppCheckTokenListener</a>(p: <a href="/docs/reference/kotlin/com/google/firebase/appcheck/interop/AppCheckTokenListener.html">AppCheckTokenListener</a>!)</code></div>
</td>
</tr>
</tbody> |
Generated by 🚫 Danger |
|
The public api surface has changed for the subproject appcheck_firebase-appcheck-interop: Please update the api.txt files for the subprojects being affected by this change by running ./gradlew ${subproject}:generateApiTxtFile. Also perform a major/minor bump accordingly. |
Coverage Report 1Affected Products
Test Logs |
Size Report 1Affected Products
Test Logs |
Unit Test Results 310 files - 439 310 suites - 439 11m 57s ⏱️ - 17m 50s Results for commit 3d357a2. ± Comparison against base commit fe8c9df. ♻️ This comment has been updated with latest results. |
Startup Time Report 1Note: Layout is sometimes suboptimal due to limited formatting support on GitHub. Please check this report on GCS. Notes
Startup Times
|
emilypgoogle
force-pushed
the
ep/appcheck-interop
branch
from
bd84c56 to
367e0fd
Compare
|
The public api surface has changed for the subproject appcheck_firebase-appcheck-interop: Please update the api.txt files for the subprojects being affected by this change by running ./gradlew ${subproject}:generateApiTxtFile. Also perform a major/minor bump accordingly. |
|
The public api surface has changed for the subproject appcheck_firebase-appcheck-interop: Please update the api.txt files for the subprojects being affected by this change by running ./gradlew ${subproject}:generateApiTxtFile. Also perform a major/minor bump accordingly. |
There was a problem hiding this comment.
Looks like there are a few failing tests -- can you take a look at those?
Also re: "AppCheck Interop is currently a non-public facing SDK, and all uses of the method being changed are in tests, so the breaking change does not require an API bump or a major release bump."
Just to check, have you verified that Firebase SDK usages of the App Check Interop (i.e. Storage, Firestore, etc.) also do not rely on FirebaseException being returned from the interop?
|
The public api surface has changed for the subproject appcheck_firebase-appcheck-interop: Please update the api.txt files for the subprojects being affected by this change by running ./gradlew ${subproject}:generateApiTxtFile. Also perform a major/minor bump accordingly. |
The method's uses are all null checks or rethrows, so nothing relies on the backing type past implementing In regards to the version bump situation, however, after a handful of conversations it seems like the best option is to bump appcheck and interop to a new major, and move the 3 dependent SDKs (functions, firestore, storage) up a minor to depend on the new version. Current plan is to do this in M131 for IO. I've got the version bumps ready in the PR as I understand them but would appreciate a sanity check. This does however give us the possibility to rename What do you think about the versioning and name change? |
Version bumps sound good, though I left a couple comments in the PR.
I think re-naming is okay, though I think there is some benefit in making it clear that the interop library is not intended for public consumption. Perhaps instead of just AppCheckTokenProvider, it can be InteropAppCheckTokenProvider? Also can you clarify what you mean by this type being visible in the Maps API surface? Will developers using the Maps API have to interact with the interop types directly? Also @malcolmdeck for thoughts on this, since I'm not sure who the new owner of the Android App Check SDK will be. |
|
I agree on
|
|
I think that we may want to run the Interop class naming by the API Council (even if it's just an email rather than formal review). It's not the hugest deal since developers won't actually need to interact with that Interface, but if we're making a new symbol public it's probably worth making sure that we get it right, especially since whatever we put in Maps is likely to be the name for all future external integrators (of which we will expect there to be many). |
...irebase-appcheck-interop/src/main/java/com/google/firebase/appcheck/AppCheckTokenResult.java
Outdated
Show resolved
Hide resolved
Got an LGTM and no dissenting opinions over a couple days, looks like we're in the clear for this, so I went through with the name change. Currently have 1 integration test failing that I'm looking into, not sure if it's just flaky though. |
...irebase-appcheck-interop/src/main/java/com/google/firebase/appcheck/AppCheckTokenResult.java
Outdated
Show resolved
Hide resolved
AppCheck Interop had a common dependency and was using it to return
FirebaseExceptions inAppCheckTokenResult. Going forward with third party SDK integration with Maps or similar, binary size is a large concern, and this change will allow SDK consumers to implement AppCheck integration with minimal included binary size moving forward. It is currently the plan to keep AppCheck Interop as a non-public SDK, only being exposed through third party SDKs.AppCheck Interop is currently a non-public facing SDK, and all uses of the method being changed are in tests, so the breaking change does not require an API bump or a major release bump.
There has been the suggestion that if this interface is going to be used by Maps, it potentially should have a name other than
InternalAppCheckTokenProvider, but a change like that would require sweeping changes, so it hasn't been included here.