Update to Application Services v73.0.1 #9731

rfk · 2021-02-19T00:04:48Z

Update: The new version of Application Services has been released and it includes auto-generated bindings for the fxaclient component. This PR updates the appservices version and provides the necessary tweaks for API changes.

I've tested this manually in a local build of Fenix and account- and send-tab-related functionality seems to be working well.

--
Over in mozilla/application-services#3876 I'm working on a significant restructure of the fxa-client Rust component, aiming to use our new bindings-generation tool to replace a bunch of hand-written code. This PR captures the corresponding a-c changes to adapt to the minor API differences between authgenerated and hand-written code.

It's not appropriate to merge yet, but I wanted to push it for early visible and feedback, particularly if any of the changes here seem like the sort of thing we should not be doing. /cc @grigoryk @jonalmeida

rfk · 2021-02-19T00:05:24Z

components/service/firefox-accounts/README.md

@@ -173,7 +173,7 @@ val accountEventsObserver = object : AccountEventsObserver {
    override fun onEvents(event: List<AccountEvent>) {
        // device received some commands; for example, here's how you can process incoming Send Tab commands:
        commands
-            .filter { it is AccountEvent.IncomingDeviceCommand }
+            .filter { it is AccountEvent.CommandReceived }


I had to change the name of this variant for dumb code-gen-related reasons, but I also prefer "command received" because it maps more directly to the of the push notification.

rfk · 2021-02-19T00:05:51Z

components/service/firefox-accounts/src/main/java/mozilla/components/service/fxa/Exceptions.kt

@@ -7,28 +7,28 @@ package mozilla.components.service.fxa
 /**
 * High-level exception class for the exceptions thrown in the Rust library.
 */
-typealias FxaException = mozilla.appservices.fxaclient.FxaException
+typealias FxaException = mozilla.appservices.fxaclient.FxaErrorException


Name change due to current limitations of bindings generator, which has strong opinions about the name of exception classes.

rfk · 2021-02-19T00:06:38Z

components/service/firefox-accounts/src/main/java/mozilla/components/service/fxa/Types.kt

@@ -104,25 +107,25 @@ fun Profile.into(): mozilla.components.concept.sync.Profile {
    )
 }

-internal fun Device.Type.into(): DeviceType {


The bindings generated doesn't support these nested namespace things yet, so e.g. Device.Type is now DeviceType.

rfk · 2021-02-19T00:07:12Z

components/service/firefox-accounts/src/main/java/mozilla/components/service/fxa/Types.kt

-        from = this.from?.into(),
-        entries = this.entries.map { it.into() }
+        from = this.sender?.into(),
+        entries = this.payload.entries.map { it.into() }


The field names here were different between the Rust code and the hand-written Kotlin wrapper, and now that things are autogenerated they're the same.

rfk · 2021-02-19T00:08:16Z

.../firefox-accounts/src/test/java/mozilla/components/service/fxa/FxaDeviceConstellationTest.kt

-            ASAccountEvent.IncomingDeviceCommand(
-                command = IncomingDeviceCommand.TabReceived(testDevice1, arrayOf(testTab1))
+            ASAccountEvent.CommandReceived(
+                command = IncomingDeviceCommand.TabReceived(testDevice1, SendTabPayload(listOf(testTab1), "flowid", "streamid"))


The bindings generator uses lists, the a-c code here seems to prefer arrays. I'm not familiar enough with the differences in Kotlin to know under which circumstances the different types should be preferred. Should we invest in making the generated bindings accept an array rather than a list here?

A list should be fine.

codecov · 2021-02-19T00:14:35Z

Codecov Report

Merging #9731 (3167b55) into master (5ad1fc3) will decrease coverage by 0.00%.
The diff coverage is 20.83%.

@@             Coverage Diff              @@
##             master    #9731      +/-   ##
============================================
- Coverage     74.04%   74.04%   -0.01%     
  Complexity     5655     5655              
============================================
  Files           773      773              
  Lines         28766    28764       -2     
  Branches       4738     4737       -1     
============================================
- Hits          21301    21299       -2     
  Misses         4986     4986              
  Partials       2479     2479

Impacted Files	Coverage Δ	Complexity Δ
.../java/mozilla/components/service/fxa/Exceptions.kt	`75.00% <ø> (ø)`	`0.00 <0.00> (ø)`
...a/mozilla/components/service/fxa/FirefoxAccount.kt	`17.02% <0.00%> (ø)`	`4.00 <0.00> (ø)`
...a/components/service/fxa/FxaDeviceConstellation.kt	`63.15% <ø> (ø)`	`12.00 <0.00> (ø)`
...n/java/mozilla/components/service/nimbus/Nimbus.kt	`50.95% <0.00%> (ø)`	`13.00 <0.00> (ø)`
.../main/java/mozilla/components/service/fxa/Types.kt	`35.71% <25.00%> (ø)`	`0.00 <0.00> (ø)`
...ava/mozilla/components/support/migration/Result.kt	`100.00% <0.00%> (ø)`	`0.00% <0.00%> (ø%)`
...onents/support/sync/telemetry/BaseGleanSyncPing.kt	`100.00% <0.00%> (ø)`	`11.00% <0.00%> (ø%)`
...onents/lib/crash/notification/CrashNotification.kt	`91.42% <0.00%> (+0.25%)`	`2.00% <0.00%> (ø%)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5ad1fc3...3167b55. Read the comment docs.

rfk · 2021-03-05T05:31:07Z

samples/sync/src/main/res/layout/activity_main.xml

@@ -130,7 +130,7 @@
                style="?android:attr/listSeparatorTextViewStyle"
                android:text="@string/devices" />

-            <FragmentContainerView android:name="org.mozilla.samples.sync.DeviceFragment"
+            <androidx.fragment.app.FragmentContainerView android:name="org.mozilla.samples.sync.DeviceFragment"


I was getting weird unrelated crashes when trying to build and run the sample app, and Google told me to change this, and it worked. Does this seem familiar to anyone else, and maybe indicate a problem with my local build env?

(I don't think it's related to the changes from this PR, because I was getting the same build issues on a clean checkout of a-c).

rfk · 2021-03-10T09:51:23Z

buildSrc/src/main/java/Dependencies.kt

@@ -30,7 +30,7 @@ object Versions {
    const val disklrucache = "2.0.2"
    const val leakcanary = "2.4"

-    const val mozilla_appservices = "72.1.0"
+    const val mozilla_appservices = "73.0.1"


One unrelated warning about this release: we have some weird build issues going on with Windows Desktop builds at the moment, ref mozilla/application-services#3917. The -forUnitTests variant of appservices 73.0.1 does not include a Windows dll, which IIUC means that a-c developers would not be able to run unittests that use appservices code on a Windows host.

It's not clear to me how much of a dealbreaker that is for y'all. I opted to disable Windows builds for this release in order to get something out the door that we can play with, and on the appservices side we're happy to ship a release without them. But, if it's important for a-c or Fenix developers to have this capability, we'll try to followup with a 73.0.2 release that restores them.

Thanks for flagging this, Ryan! Perhaps we can mention this in the changelog as part of this PR, as a temporary (I'm assuming) condition?

I've added an entry to the changelog, with a link to the underlying appservices issue for followup 👍

rfk · 2021-03-10T09:55:45Z

components/service/nimbus/src/main/java/mozilla/components/service/nimbus/Nimbus.kt

@@ -29,7 +29,7 @@ import org.mozilla.experiments.nimbus.AvailableRandomizationUnits
 import org.mozilla.experiments.nimbus.EnrolledExperiment
 import org.mozilla.experiments.nimbus.EnrollmentChangeEvent
 import org.mozilla.experiments.nimbus.EnrollmentChangeEventType
-import org.mozilla.experiments.nimbus.ErrorException
+import org.mozilla.experiments.nimbus.NimbusErrorException


Oh yeah, you're also getting a new Nimbus release, which had to rename this error class because the name Error was causing problems with the Swift bindings.

travis79

Aside from the detekt lint about UndocumentedPublicFunction in the Types.kt file, this looks good to me.

rfk · 2021-03-10T20:24:12Z

Aside from the detekt lint about UndocumentedPublicFunction in the Types.kt file

They were like that when I got here, I swear! 😅

Seriously though, I don't really understand why my changes triggered this lint, but I'll add some docs for completeness.

grigoryk

Thank you!

grigoryk · 2021-03-10T21:26:33Z

buildSrc/src/main/java/Dependencies.kt

@@ -30,7 +30,7 @@ object Versions {
    const val disklrucache = "2.0.2"
    const val leakcanary = "2.4"

-    const val mozilla_appservices = "72.1.0"
+    const val mozilla_appservices = "73.0.1"


Thanks for flagging this, Ryan! Perhaps we can mention this in the changelog as part of this PR, as a temporary (I'm assuming) condition?

grigoryk · 2021-03-10T22:02:44Z

...ents/service/firefox-accounts/src/main/java/mozilla/components/service/fxa/FirefoxAccount.kt

@@ -144,7 +144,7 @@ class FirefoxAccount internal constructor(
        accessType: AccessType
    ) = withContext(scope.coroutineContext) {
        handleFxaExceptions(logger, "authorizeOAuthCode", { null }) {
-            val params = AuthorizationParams(clientId, scopes, state, accessType.msg)
+            val params = AuthorizationParameters(clientId, scopes.toList(), state, accessType.msg, null, null, null)


slightly offtopic for this PR, and we already discussed this in slack: it took me a bit of clicking around to figure out that the missing params here are for the PKCE code exchange. I think it's only documented at https://github.com/mozilla/application-services/blob/d77600e32c19cbeebaf1045a1b9b49476530e770/components/fxa-client/src/internal/oauth.rs#L194

Also, filed #9863 to remove this from the API surface for now.

I guess we could use named argument syntax here to make things bit clearer, e.g. AuthorizationParameters(..., pkceParams=null, keysJwk=null).

I also expect a future release to support default values for these arguments which might help clean it up a bit more.

mergify · 2021-03-10T23:35:53Z

This pull request has conflicts when rebasing. Could you fix it @rfk? 🙏

This release includes new auto-generated Kotlin bindings for the FxA Client component, which come with a few breaking API changes. There should be no changes in functionality though.

rfk commented Feb 19, 2021

View reviewed changes

rfk force-pushed the fxa-client-uniffication branch 2 times, most recently from 84295e5 to 15b1513 Compare February 25, 2021 06:05

rfk mentioned this pull request Feb 25, 2021

Restructure, UniFFI, and document the fxa-client crate. mozilla/application-services#3876

Merged

rfk force-pushed the fxa-client-uniffication branch from 15b1513 to ff93f47 Compare February 25, 2021 06:54

rfk force-pushed the fxa-client-uniffication branch from ff93f47 to aebe39c Compare March 5, 2021 05:18

rfk commented Mar 5, 2021

View reviewed changes

rfk force-pushed the fxa-client-uniffication branch from aebe39c to 1f4362d Compare March 10, 2021 09:46

rfk changed the title ~~WIP updates for compatibility with UniFFI-ied fxa-client component.~~ Update to Application Services v73.0.1 Mar 10, 2021

rfk commented Mar 10, 2021

View reviewed changes

rfk requested a review from grigoryk March 10, 2021 09:51

rfk marked this pull request as ready for review March 10, 2021 09:54

rfk requested review from a team as code owners March 10, 2021 09:54

rfk commented Mar 10, 2021

View reviewed changes

travis79 approved these changes Mar 10, 2021

View reviewed changes

rfk force-pushed the fxa-client-uniffication branch from 1f4362d to 459c4b6 Compare March 10, 2021 20:38

grigoryk approved these changes Mar 10, 2021

View reviewed changes

grigoryk mentioned this pull request Mar 10, 2021

Closes #9864: Remove authorizeOAuthCode from the FirefoxAccount API #9864

Merged

4 tasks

rfk force-pushed the fxa-client-uniffication branch from 459c4b6 to bb8544e Compare March 10, 2021 23:19

rfk added the 🛬 needs landing PRs that are ready to land label Mar 10, 2021

Update to application-services v73.0.1.

afa1a71

This release includes new auto-generated Kotlin bindings for the FxA Client component, which come with a few breaking API changes. There should be no changes in functionality though.

rfk force-pushed the fxa-client-uniffication branch from bb8544e to afa1a71 Compare March 10, 2021 23:37

Merge branch 'master' into fxa-client-uniffication

3167b55

mergify bot merged commit 2522696 into mozilla-mobile:master Mar 11, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update to Application Services v73.0.1 #9731

Update to Application Services v73.0.1 #9731

rfk commented Feb 19, 2021 •

edited

Loading

rfk Feb 19, 2021

rfk Feb 19, 2021

rfk Feb 19, 2021

rfk Feb 19, 2021

rfk Feb 19, 2021

jonalmeida Feb 20, 2021

codecov bot commented Feb 19, 2021 •

edited

Loading

rfk Mar 5, 2021

rfk Mar 10, 2021

grigoryk Mar 10, 2021

rfk Mar 10, 2021

rfk Mar 10, 2021

travis79 left a comment

rfk commented Mar 10, 2021

grigoryk left a comment

grigoryk Mar 10, 2021

grigoryk Mar 10, 2021

rfk Mar 10, 2021

mergify bot commented Mar 10, 2021

Update to Application Services v73.0.1 #9731

Update to Application Services v73.0.1 #9731

Conversation

rfk commented Feb 19, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Feb 19, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

travis79 left a comment

Choose a reason for hiding this comment

rfk commented Mar 10, 2021

grigoryk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mergify bot commented Mar 10, 2021

rfk commented Feb 19, 2021 •

edited

Loading

codecov bot commented Feb 19, 2021 •

edited

Loading