api: Embed platform and app info in user-agent #724

rajveermalviya · 2024-06-05T17:08:43Z

Fixes #467

rajveermalviya

@gnprice, some questions about the user-agent format, PR as a whole is still WIP.

lib/api/core.dart

rajveermalviya · 2024-06-21T05:14:09Z

test/api/fake_api_test.dart

 import 'exception_checks.dart';
 import 'fake_api.dart';

 void main() {
+  TestZulipBinding.ensureInitialized();


@gnprice Thoughts on testing setup for this?

Because ApiConnection depends on userAgent function, and it depends on ZulipBinding to get package & device info — now every test that uses ApiConnection will need to also first call TestZulipBinding.ensureInitialized().

Alternative implementation is to make ApiConnection take userAgent in it's constructor, which would just move this reposnibility to the upper layer (LiveGlobalStore for the real impl), but that would remove the need for TestZulipBinding.ensureInitialized() in each test.

Hmm yeah, having to do that in all the tests would be annoying.

It looks like there's only a small number of call sites of the ApiConnection constructors. So that's probably a good solution.

Hmm after trying the alternative I suggested, ended up reverting it, because it led to a bit of constructor argument poisoning up the chain of ApiConnection — making it harder to read the code where some constructors calls take the userAgent value, and some leave it out (null).

Instead ended up refactoring first implementation by moving the userAgent function to class ZulipBinding to make it explicit that it now depends on members in ZulipBinding and thus the binding needs to be initialized to get user-agent.

What were the callers that that propagated up the call chain to? I was imagining it'd be just that ApiConnection.live requires a real user-agent, and FakeApiConnection defaults to a boring fake one, and then very little other code has to get involved.

The implementation I tried was to keep deviceInfo and packageInfo as async function instead of the current sync getters on ZulipBinding and then trying to populate the ApiConnection args (deviceInfo & packageInfo) from the async functions it's being called. But there was one call which was not async, so had to add the args for it too. And doing so led to many other changes in the codebase which were moot, thus reverted it.

But if we stick to sync function userAgentHeader and let ApiConnection.live inline it in it's constructor like:

ApiConnection.live({ ... }) : this(client: http.Client(), ... userAgentHeader: ZulipBinding.instance.userAgentHeader());

then it does work without the need to poison the callers, I'll send a revision with this now.

Okay just tried it, and seems like there isn't much difference in line count, because now some tests that needs to check userAgent needs to pass the testBinding.userAgent to FakeApiConnection.with_ which are quite a few.

But on the plus side, the tests that don't check userAgent don't need to add ZulipBinding initialization lines:

- TestZulipBinding.ensureInitialized(); - tearDown(testBinding.reset);

Okay just tried it, and seems like there isn't much difference in line count, because now some tests that needs to check userAgent needs to pass the testBinding.userAgent to FakeApiConnection.with_ which are quite a few.

Cool. Yeah, even if the total line count is the same but the impact is all concentrated in the one group of tests that's about the user-agent value itself, that sounds a lot nicer.

We can probably also improve that further with a local helper or two in those tests.

rajveermalviya · 2024-06-24T11:02:43Z

lib/main.dart

  WidgetsFlutterBinding.ensureInitialized();
+  await LiveZulipBinding.ensureInitialized();


The goal for this change is to pre-fetch the deviceInfo and packageInfo while ZulipBinding is being initialized and then later they can be used from non-async functions via the getters.

That make sense from an ergonomics perspective; but it makes me nervous to be introducing this wait into the startup path. Probably on any reasonable system the queries will return quickly and it's fine, but

there may be bugs in these third-party plugins that cause them to sometimes hang before supplying an answer;

there may be devices out there where some bug in a vendor's low-quality modifications to the Android platform software cause the queries to hang, or just to be slow, even though the plugins are doing only perfectly reasonable things that totally ought to be reliably fast.

Then the thing about putting awaits like these into the startup path is that it'd escalate any bugs of those types into a serious problem in the Zulip app for affected users.

So let's arrange things instead so that if for some reason those queries are slow, or even hang forever and never return, everything fails gracefully — just the places that actually wanted to use the answers have to make do with not knowing the answers.

One good way to do that, API-wise, would be to indeed make those into nice non-async getters, but returning nullable. Then when the binding is initialized, it kicks off the queries, and if the queries have finished by the time we try to make a request then we use the results; if not, we fall back to not knowing this information.

I guess most of the time we'll be firing off one request promptly after startup, the register-events request. So if we find that that often happens before the platform queries finish, then we can add an extra wrinkle where we wait for the queries to finish… but with a timeout, like 10ms or something, so we prioritize the user's experience over being sure we include this information.

Let's keep that as a separate followup commit, though. Or it'd be great if you try a little manual experimenting to see if that's a problem in practice; if it seems like it isn't, we can skip that wrinkle entirely, and keep it in our back pocket as a possible followup for later if it turns out to be a problem sometimes in wider deployment.

Thanks, yeah we definitely should have a fail safe if plugins fails for some reason or are slow to retrieve the data.

The latest revision now doesn't await on pre-fetchs for both (deviceInfo and packageInfo), and where the getters are called there's now some kind of relevant fallback just-in-case.

if you try a little manual experimenting to see if that's a problem in practice

I tried that, by throw-ing in case either of infos are null but didn't see any exceptions in both release and debug builds for Android & macOS (can't try release on iOS Simulator) during startup.

sm-sayedi

Thanks @rajveermalviya for this. LGTM! Just a few (small) comments below!

sm-sayedi · 2024-06-26T11:43:55Z

lib/widgets/clipboard.dart

+    // If there was a failure while fetching `deviceInfo`, and we don't
+    // know which os/device variant we are on, display the snackbar anyway.
+    null                           => true,
    _                              => true,


Suggested change

// If there was a failure while fetching `deviceInfo`, and we don't

// know which os/device variant we are on, display the snackbar anyway.

null => true,

_ => true,

// When there is a failure while fetching [deviceInfo] (`ZulipBinding.instance.deviceInfo == null`),

// and we don't know which os/device variant we are on, display the snackbar anyway.

_ => true,

I think we can remove null => true.

sm-sayedi · 2024-06-26T14:43:21Z

lib/model/binding.dart

+  ///
+  /// Uses [deviceInfo] to get operating system information
+  /// and [packageInfo] to get application version information.
+  Map<String, String> userAgentHeader();


How about adding this change in its separate commit?!

sm-sayedi · 2024-06-26T14:46:32Z

lib/model/binding.dart

@@ -159,6 +176,59 @@ class IosDeviceInfo extends BaseDeviceInfo {
  IosDeviceInfo({required this.systemVersion});
 }

+/// Like [device_info_plus.MacOsDeviceInfo], but without things we don't use.
+class MacOsDeviceInfo extends BaseDeviceInfo {


I think it will also be nice to add the classes for these new platforms and the related changes to LiveZulipBinding._prefetchDeviceInfo in a separate commit too!

sm-sayedi · 2024-06-26T14:49:45Z

lib/model/binding.dart

+}
+
+@visibleForTesting
+Map<String, String> buildUserAgentHeader(BaseDeviceInfo deviceInfo, PackageInfo packageInfo) {


api: Embed platform and app info in user-agent

After adding the previous changes to their separate commits, now this commit can focus on embedding platform and app info in the user-agent as the commit message suggests.

rajveermalviya · 2024-06-30T20:22:30Z

Thanks for the review @sm-sayedi, pushed a new revision!

sm-sayedi

Thanks for the revision. Just added two comments (very small) below, otherwise looks good to me.

Let's move on to the mentor review with @sumanthvrao.

sm-sayedi · 2024-07-01T06:29:47Z

lib/model/binding.dart

+  AndroidDeviceInfo({
+    required this.release,
+    required this.sdkInt,
+  });


Suggested change

AndroidDeviceInfo({

required this.release,

required this.sdkInt,

});

AndroidDeviceInfo({required this.release, required this.sdkInt});

nit: Can be inlined.

sm-sayedi · 2024-07-01T06:33:02Z

lib/model/binding.dart

+  LinuxDeviceInfo({
+    required this.name,
+    required this.versionId,
+  });


Suggested change

LinuxDeviceInfo({

required this.name,

required this.versionId,

});

LinuxDeviceInfo({required this.name, required this.versionId});

rajveermalviya · 2024-07-01T07:02:11Z

Thanks for the review @sm-sayedi, updated.

chrisbobbe

Thanks! Small comments below.

chrisbobbe · 2024-07-03T08:40:34Z

lib/main.dart

  LicenseRegistry.addLicense(additionalLicenses);
-  LiveZulipBinding.ensureInitialized();
  WidgetsFlutterBinding.ensureInitialized();
+  LiveZulipBinding.ensureInitialized();
  NotificationService.instance.start();


Do these lines need to be reordered?

Yes they need to be reordered, since deviceInfo is being prefetched in LiveZulipBinding.ensureInitialized() and PlatformChannels require WidgetsFlutterBinding to be initialized first.

What does the error look like if the order of these gets swapped back?

If it doesn't make clear that the problem is that WidgetsFlutterBinding is needed first, then ideally we can do something to make the error clear.

If that's hard or annoying, then just a comment here explaining this ordering constraint would also be an acceptable solution.

Here's how the errors look like:

flutter: Failed to prefetch device info: Binding has not yet been initialized. The "instance" getter on the ServicesBinding binding mixin is only available once that binding has been initialized. Typically, this is done by calling "WidgetsFlutterBinding.ensureInitialized()" or "runApp()" (the latter calls the former). Typically this call is done in the "void main()" method. The "ensureInitialized" method is idempotent; calling it multiple times is not harmful. After calling that method, the "instance" getter will return the binding. In a test, one can call "TestWidgetsFlutterBinding.ensureInitialized()" as the first line in the test's "main()" method to initialize the binding. If ServicesBinding is a custom binding mixin, there must also be a custom binding class, like WidgetsFlutterBinding, but that mixes in the selected binding, and that is the class that must be constructed before using the "instance" getter. #0 BindingBase.checkInstance.<anonymous closure> (package:flutter/src/foundation/binding.dart:309:9) #1 BindingBase.checkInstance (package:flutter/src/foundation/binding.dart:390:6) #2 ServicesBinding.instance (package:flutter/src/services/binding.dart:68:54) #3 _findBinaryMessenger (package:flutter/src/services/platform_channel.dart:158:25) #4 MethodChannel.binaryMessenger (package:flutter/src/services/platform_channel.dart:293:56) #5 MethodChannel._invokeMethod (package:flutter/src/services/platform_channel.dart:327:15) #6 MethodChannel.invokeMethod (package:flutter/src/services/platform_channel.dart:507:12) #7 MethodChannelDeviceInfo.deviceInfo (package:device_info_plus_platform_interface/method_channel/method_channel_device_info.dart:19:24) #8 DeviceInfoPlugin.macOsInfo (package:device_info_plus/device_info_plus.dart:86:48) #9 DeviceInfoPlugin.deviceInfo (package:device_info_plus/device_info_plus.dart:107:16) #10 LiveZulipBinding._prefetchDeviceInfo (package:zulip/model/binding.dart:338:62) #11 new LiveZulipBinding (package:zulip/model/binding.dart:290:19) #12 LiveZulipBinding.ensureInitialized (package:zulip/model/binding.dart:297:7) #13 main (package:zulip/main.dart:16:20) #14 _runMain.<anonymous closure> (dart:ui/hooks.dart:301:23) #15 _delayEntrypointInvocation.<anonymous closure> (dart:isolate-patch/isolate_patch.dart:297:19) #16 _RawReceivePort._handleMessage (dart:isolate-patch/isolate_patch.dart:184:12) flutter: Failed to prefetch package info: Binding has not yet been initialized. The "instance" getter on the ServicesBinding binding mixin is only available once that binding has been initialized. Typically, this is done by calling "WidgetsFlutterBinding.ensureInitialized()" or "runApp()" (the latter calls the former). Typically this call is done in the "void main()" method. The "ensureInitialized" method is idempotent; calling it multiple times is not harmful. After calling that method, the "instance" getter will return the binding. In a test, one can call "TestWidgetsFlutterBinding.ensureInitialized()" as the first line in the test's "main()" method to initialize the binding. If ServicesBinding is a custom binding mixin, there must also be a custom binding class, like WidgetsFlutterBinding, but that mixes in the selected binding, and that is the class that must be constructed before using the "instance" getter. #0 BindingBase.checkInstance.<anonymous closure> (package:flutter/src/foundation/binding.dart:309:9) #1 BindingBase.checkInstance (package:flutter/src/foundation/binding.dart:390:6) #2 ServicesBinding.instance (package:flutter/src/services/binding.dart:68:54) #3 _findBinaryMessenger (package:flutter/src/services/platform_channel.dart:158:25) #4 MethodChannel.binaryMessenger (package:flutter/src/services/platform_channel.dart:293:56) #5 MethodChannel._invokeMethod (package:flutter/src/services/platform_channel.dart:327:15) #6 MethodChannel.invokeMethod (package:flutter/src/services/platform_channel.dart:507:12) #7 MethodChannel.invokeMapMethod (package:flutter/src/services/platform_channel.dart:534:49) #8 MethodChannelPackageInfo.getAll (package:package_info_plus_platform_interface/method_channel_package_info.dart:13:32) #9 PackageInfo.fromPlatform (package:package_info_plus/package_info_plus.dart:80:61) #10 LiveZulipBinding._prefetchPackageInfo (package:zulip/model/binding.dart:367:56) #11 new LiveZulipBinding (package:zulip/model/binding.dart:291:20) #12 LiveZulipBinding.ensureInitialized (package:zulip/model/binding.dart:297:7) #13 main (package:zulip/main.dart:16:20) #14 _runMain.<anonymous closure> (dart:ui/hooks.dart:301:23) #15 _delayEntrypointInvocation.<anonymous closure> (dart:isolate-patch/isolate_patch.dart:297:19) #16 _RawReceivePort._handleMessage (dart:isolate-patch/isolate_patch.dart:184:12)

Basically both of the prefetch fail because of same reason — MethodChannel requiring WidgetsFlutterBinding initialized first.

Cool, that seems clear enough — thanks.

chrisbobbe · 2024-07-03T08:47:50Z

lib/widgets/clipboard.dart

+    // Otherwise always display the snackbar if:
+    //  1. It's any other os/device than Android.
+    //  2. deviceInfo == null, meaning there was a failure while fetching
+    //     the deviceInfo, so we don't know which os/device variant the
+    //     app is running on.
    _                              => true,


Can we explain the meaning of deviceInfo == null in a dartdoc on the deviceInfo getter? That seems more efficient than explaining it separately at each call site that handles the null case. Curious readers can look up the dartdoc to learn what it means.

Then I think 1., and the rest of this comment, can be removed too. I think the comment "Android 13+ shows its own popup…", above this, implies that other platforms don't show a popup.

chrisbobbe · 2024-07-03T08:52:56Z

lib/model/binding.dart

@@ -164,11 +164,25 @@ class LiveZulipBinding extends ZulipBinding {
  /// Initialize the binding if necessary, and ensure it is a [LiveZulipBinding].
  static LiveZulipBinding ensureInitialized() {
    if (ZulipBinding._instance == null) {
-      LiveZulipBinding();
+      final binding = LiveZulipBinding();
+      binding._prefetchDeviceInfo();


binding [nfc]: Refactor deviceInfo to a getter

Let's highlight that we're changing the logic so that the device info is prefetched. And since this seems like a behavior change (though a small one), let's not mark it NFC.

chrisbobbe · 2024-07-03T08:58:39Z

lib/widgets/about_zulip.dart

+    // At this point `ZulipBinding` should already be initialized
+    // so `packageInfo` will only be null if there was a failure
+    // while fetching it.


The _prefetchPackageInfo call isn't awaited in LiveZulipBinding.ensureInitialized, so that method completes before the info is fetched and stored. It may be true that "ZulipBinding should already be initialized", but that doesn't seem sufficient to ensure that we've waited long enough to get success or failure from the fetch.

For this screen, I think we're content with the existing behavior: show the info if we have it, and just show a placeholder if we don't.

chrisbobbe · 2024-07-03T09:02:26Z

lib/widgets/about_zulip.dart

-                ListTile(
-                  title: Text(zulipLocalizations.aboutPageAppVersion),
-                  subtitle: Text(_packageInfo?.version ?? '(…)')),
+                if (appVersion != null)
+                  ListTile(
+                    title: Text(zulipLocalizations.aboutPageAppVersion),
+                    subtitle: Text(appVersion)),


This is a behavior change, so let's leave it out of a pure-refactor [nfc] commit. Before, when the app version is undetermined, we'd show a ListTile with subtitle "(…)". Now, in that case, the whole ListTile is omitted.

removed this behavior change

chrisbobbe · 2024-07-03T09:10:07Z

test/api/core_test.dart

-      return FakeApiConnection.with_(account: eg.selfAccount, (connection) async {
-        connection.prepare(json: {});
-        await connection.get(kExampleRouteName, (json) => json, 'example/route', params);
-        check(connection.lastRequest!).isA<http.Request>()
-          ..method.equals('GET')
-          ..url.asString.equals('${eg.realmUrl.origin}$expectedRelativeUrl')
-          ..headers.deepEquals({
-            ...authHeader(email: eg.selfAccount.email, apiKey: eg.selfAccount.apiKey),
-            ...userAgentHeader(),
-          })
-          ..body.equals('');
+      return FakeApiConnection.with_(account: eg.selfAccount,
+        userAgentHeader: testBinding.userAgentHeader(),
+        (connection) async {
+          connection.prepare(json: {});
+          await connection.get(kExampleRouteName, (json) => json, 'example/route', params);
+          check(connection.lastRequest!).isA<http.Request>()
+            ..method.equals('GET')
+            ..url.asString.equals('${eg.realmUrl.origin}$expectedRelativeUrl')
+            ..headers.deepEquals({
+              ...authHeader(email: eg.selfAccount.email, apiKey: eg.selfAccount.apiKey),
+              ...testBinding.userAgentHeader(),
+            })
+            ..body.equals('');


I think this is a one-line change plus a reformatting with different whitespace; is that right? The whitespace change is kind of distracting to me as I review looking for meaningful changes. (Here and in several places below.)

It looks like the whitespace changes in question are all an indentation change. It's natural for those to happen together with substantive changes sometimes — for example they'll happen whenever adding or removing a wrapper widget around a child subtree.

A handy tool for reading such diffs is git log -b, which ignores indentation changes. With that, this diff hunk looks like:

- return FakeApiConnection.with_(account: eg.selfAccount, (connection) async { + return FakeApiConnection.with_(account: eg.selfAccount, + userAgentHeader: testBinding.userAgentHeader(), + (connection) async { connection.prepare(json: {}); await connection.get(kExampleRouteName, (json) => json, 'example/route', params); check(connection.lastRequest!).isA<http.Request>() ..method.equals('GET') ..url.asString.equals('${eg.realmUrl.origin}$expectedRelativeUrl') ..headers.deepEquals({ ...authHeader(email: eg.selfAccount.email, apiKey: eg.selfAccount.apiKey), - ...userAgentHeader(), + ...testBinding.userAgentHeader(), }) ..body.equals('');

(With git config diff.colorMovedWS, you can also get your default git log -p output to treat indentation changes the same way as moved code and give them a different background color. That's what I have in my gitconfig. I still use -b to get a clearer view in extreme cases.)

Cool. Yeah; I used git log -b to help verify the change, but I think I could have looked closer at the regular git log -p output—it turns out I already have that git config option set the way you do, and here's what it looks like:

So, noticing that a lot of the text has the "moved" colors (magenta and a more bluish-green), and also the faint background color, would have been helpful. That background color does end up being pretty faint on my screen; I bet there's a way to adjust it.

Yeah, exactly. And yeah, those colors are adjustable - they're the diff.oldMoved and friends found in my gitconfig. Definitely worth trying some quick changes to them to see what works best for you.

From the screenshot, it looks like your terminal's default background color is already a faint gray rather than black, which ends up being pretty close to the background colors set there. I have my terminal background set to pure black, for maximum contrast, and then the colors in my config are chosen to be distinct from that.

This is better—I've changed the Android Studio color scheme to "high contrast":

Yeah, here's what that looks like for me:

The striping in your screenshot looks like it could get a bit annoying. You might be able to adjust the line spacing to fix that — generally I'd expect a terminal to use 1.0x line spacing anyway.

The striping in your screenshot looks like it could get a bit annoying. You might be able to adjust the line spacing to fix that — generally I'd expect a terminal to use 1.0x line spacing anyway.

Yep, got it! I adjusted a font setting in the Android Studio settings. It was a little confusing because things outside the terminal were responding to the setting, but the terminal wasn't—but then it did after I restarted Android Studio.

chrisbobbe · 2024-07-03T09:16:56Z

test/api/core_test.dart

+      test('matches $userAgent', () async {
+        testBinding.deviceInfoResult = deviceInfo;
+        testBinding.packageInfoResult = pacakgeInfo;
+        await checkUserAgent(userAgent);
+      });


It would be cleanest/most principled to add a line

addTearDown(testBinding.reset);

at the top of the test, like we do in other tests. (But I don't think the lack of the line causes a problem with this test as written.)

There is a addTearDown(testBinding.reset) at the start of the main, which should work for all tests in that file.

Oh! Indeed; thanks for pointing this out.

chrisbobbe · 2024-07-03T09:17:55Z

test/api/core_test.dart

+    final testCases = [
+      ('ZulipFlutter/0.0.1+1 (Android 14)',             AndroidDeviceInfo(release: '14', sdkInt: 34),                        packageInfo),
+      ('ZulipFlutter/0.0.1+1 (iOS 17.4)',               IosDeviceInfo(systemVersion: '17.4'),                                packageInfo),
+      ('ZulipFlutter/0.0.1+1 (macOS 14.5.0)',           MacOsDeviceInfo(majorVersion: 14, minorVersion: 5, patchVersion: 0), packageInfo),
+      ('ZulipFlutter/0.0.1+1 (Windows)',                WindowsDeviceInfo(),                                                 packageInfo),
+      ('ZulipFlutter/0.0.1+1 (Linux; Fedora Linux 40)', LinuxDeviceInfo(name: 'Fedora Linux', versionId: '40'),              packageInfo),
+      ('ZulipFlutter/0.0.1+1 (Linux; Fedora Linux)',    LinuxDeviceInfo(name: 'Fedora Linux', versionId: null),              packageInfo),
+    ];


Cool! The way these cases is presented is nice and compact and easy to read, without lots of extra boilerplate for each case.

(Ah I did just notice there's a typo "pacakgeInfo" a few lines below this.)

rajveermalviya · 2024-07-07T22:04:32Z

Thanks for the review @chrisbobbe, pushed a new revision PTAL.

chrisbobbe

Thanks! Two small comments below, and I'll go ahead and mark this for Greg's review.

chrisbobbe · 2024-07-08T20:32:36Z

lib/model/binding.dart

+  /// Returns a Future resolving to null if an error was
+  /// encountered while fetching, otherwise a Future
+  /// resolving to the fetched device info.


Suggested change

/// Returns a Future resolving to null if an error was

/// encountered while fetching, otherwise a Future

/// resolving to the fetched device info.

/// The returned Future resolves to null if an error is encountered while

/// fetching the data.

This is a bit more concise, and I think the success case is clear from the return type (Future<BaseDeviceInfo?>). Callers might wonder what happens in the error case, though (does it reject, or resolve to null), so it's good to keep the mention of that.

Also, maybe good to put this after this line, instead of before it:

/// This wraps [device_info_plus.DeviceInfoPlugin.deviceInfo].

since it's now just about what happens in the error case, which should be unusual.

(similarly with packageInfo below)

Addressed, though removed This wraps ... because the getter don't really wrap those functions anymore.

chrisbobbe · 2024-07-08T21:05:26Z

lib/model/binding.dart

-  @override
-  Future<BaseDeviceInfo> deviceInfo() async {
-    final deviceInfo = await device_info_plus.DeviceInfoPlugin().deviceInfo;
-    return switch (deviceInfo) {
-      device_info_plus.AndroidDeviceInfo(:var version)   => AndroidDeviceInfo(sdkInt: version.sdkInt),
-      device_info_plus.IosDeviceInfo(:var systemVersion) => IosDeviceInfo(systemVersion: systemVersion),
-      _                                                  => throw UnimplementedError(),
-    };
-  }
-


Ah, and I didn't notice last time: is there a reason to move this device_info_plus-related code before other things in the class? I think ZulipBinding, LiveZulipBinding, and TestZulipBinding should declare their members in the same order; the consistency should help reading and editing them together.

Same question 🙂

chrisbobbe · 2024-07-09T21:32:15Z

LGTM, thanks! Over to Greg, then.

gnprice

Thanks @rajveermalviya, and thanks @sm-sayedi and @chrisbobbe for the previous reviews!

Comments below. I've read fully through the first three commits:
7be37dd binding: Prefetch deviceInfo during initialization
f9ce9e5 binding: Add packageInfo getter
662346e about_zulip [nfc]: Refactor to use packageInfo from ZulipBinding

and I've skimmed through the remaining three commits:

6a0fa87 binding: Support more device/os variants for deviceInfo getter
4544709 binding [nfc]: Move userAgentHeader function to ZulipBinding
7023109 api: Embed platform and app info in user-agent

and left high-level comments that will lead to some refactors, so I'll read those in more detail in a later round of review.

gnprice · 2024-07-10T06:06:39Z

lib/main.dart

  LicenseRegistry.addLicense(additionalLicenses);
-  LiveZulipBinding.ensureInitialized();
  WidgetsFlutterBinding.ensureInitialized();
+  LiveZulipBinding.ensureInitialized();
  NotificationService.instance.start();


What does the error look like if the order of these gets swapped back?

If it doesn't make clear that the problem is that WidgetsFlutterBinding is needed first, then ideally we can do something to make the error clear.

If that's hard or annoying, then just a comment here explaining this ordering constraint would also be an acceptable solution.

gnprice · 2024-07-10T06:08:42Z

lib/model/binding.dart

+      final binding = LiveZulipBinding();
+      binding._deviceInfo = binding._prefetchDeviceInfo();
+      binding._packageInfo = binding._prefetchPackageInfo();


nit: this logic is probably cleaner to read as a constructor body

gnprice · 2024-07-10T06:10:03Z

lib/model/binding.dart

-  @override
-  Future<BaseDeviceInfo> deviceInfo() async {
-    final deviceInfo = await device_info_plus.DeviceInfoPlugin().deviceInfo;
-    return switch (deviceInfo) {
-      device_info_plus.AndroidDeviceInfo(:var version)   => AndroidDeviceInfo(sdkInt: version.sdkInt),
-      device_info_plus.IosDeviceInfo(:var systemVersion) => IosDeviceInfo(systemVersion: systemVersion),
-      _                                                  => throw UnimplementedError(),
-    };
-  }
-


Same question 🙂

gnprice · 2024-07-10T06:12:19Z

lib/model/binding.dart

+  BaseDeviceInfo? get maybeDeviceInfo => _maybeDeviceInfo;
+  BaseDeviceInfo? _maybeDeviceInfo;
+
+  @override
+  Future<PackageInfo?> get packageInfo => _packageInfo;
+  late Future<PackageInfo?> _packageInfo;
+
+  @override
+  PackageInfo? get maybePackageInfo => _maybePackageInfo;
+  PackageInfo? _maybePackageInfo;
+
+  Future<BaseDeviceInfo?> _prefetchDeviceInfo() async {


nit: keep "device info" members next to "device info" members, and "package info" members next to "package info" members

More generally, as the Flutter style guide puts it:

Fields should come before the methods that manipulate them, if they are specific to a particular group of methods.

where the key part of "before" is that they're next to those methods, though in particular they should come just before them.

gnprice · 2024-07-10T06:15:17Z

lib/model/binding.dart

+  /// May return null if prefetching hasn't completed yet,
+  /// or an error occured while fetching the data.
+  BaseDeviceInfo? get maybeDeviceInfo;


Suggested change

/// May return null if prefetching hasn't completed yet,

/// or an error occured while fetching the data.

BaseDeviceInfo? get maybeDeviceInfo;

/// This is the value [deviceInfo] resolved to,

/// or null if that hasn't resolved yet.

BaseDeviceInfo? get maybeDeviceInfo;

gnprice · 2024-07-10T06:29:49Z

lib/model/binding.dart

+/// Like [device_info_plus.WindowsDeviceInfo], currently only used to
+/// determine if we're on Windows.
+class WindowsDeviceInfo implements BaseDeviceInfo {}


I think I remember us having some discussion of why it's inconvenient to have more detail on Windows versions, but I've forgotten what the reason was. A short comment here (perhaps with a link to that previous thread) would be helpful for the next person who wonders why this one subclass has less information in it.

gnprice · 2024-07-10T06:32:22Z

lib/model/binding.dart

+  /// Examples: 'Fedora', 'Debian GNU/Linux'.
+  ///
+  /// If not set, defaults to 'Linux'.
+  final String name;


Suggested change

/// Examples: 'Fedora', 'Debian GNU/Linux'.

///

/// If not set, defaults to 'Linux'.

final String name;

/// Examples: 'Fedora', 'Debian GNU/Linux', or just 'Linux'.

final String name;

As far as we're concerned, there's no question of setting it to something or not doing so — it just is. But the mention of the string "Linux" is useful in that given it's the default for the /etc/os-release file, it's a plausible value worth mentioning as an example.

gnprice · 2024-07-10T06:35:28Z

lib/model/binding.dart

+  ///
+  /// This field is optional and may be null on some systems.


Suggested change

///

/// This field is optional and may be null on some systems.

The type already expresses that it might be null.

gnprice · 2024-07-10T06:36:28Z

lib/model/binding.dart

+  /// Examples: '17', '11.04'.
+  ///
+  /// This field is optional and may be null on some systems.
+  final String? versionId;


Let's make explicit exactly what this corresponds to from the platform side:

Suggested change

/// Examples: '17', '11.04'.

///

/// This field is optional and may be null on some systems.

final String? versionId;

/// Examples: '17', '11.04'.

///

/// Docs: https://www.freedesktop.org/software/systemd/man/latest/os-release.html#VERSION_ID=

final String? versionId;

gnprice · 2024-07-10T06:56:01Z

test/api/fake_api.dart

@@ -171,12 +176,16 @@ class FakeApiConnection extends ApiConnection {
    Uri? realmUrl,
    int? zulipFeatureLevel = eg.futureZulipFeatureLevel,
    Account? account,
+    Map<String, String> Function()? userAgentHeader,


It looks like this is always the same expression, when not null: testBinding.userAgentHeader.

How about making it a boolean instead? Like useBinding: true. That'd reduce the amount of noise at the call sites.

Then I think it can actually be a boolean up at the base class, too.

After that change, I think buildUserAgent can move to ApiConnection (e.g. as a private static), which is where it most logically belongs. We'd lose memoization between different ApiConnection instances, but that's fine — we aren't constantly making new connection instances.

rajveermalviya · 2024-07-11T18:03:26Z

Thanks for the reviews @chrisbobbe and @gnprice, pushed a new revision PTAL.

gnprice

Thanks for the revision! Comments below.

With this round, I've now read through the whole PR.

gnprice · 2024-07-12T05:46:27Z

lib/model/binding.dart

+  /// This is the value [packageInfo] resolved to,
+  /// or null if that hasn't resolved yet.
+  ///
+  /// This wraps [package_info_plus.PackageInfo.fromPlatform].
+  PackageInfo? get syncPackageInfo;


nit:

Suggested change

/// This is the value [packageInfo] resolved to,

/// or null if that hasn't resolved yet.

///

/// This wraps [package_info_plus.PackageInfo.fromPlatform].

PackageInfo? get syncPackageInfo;

/// This is the value [packageInfo] resolved to,

/// or null if that hasn't resolved yet.

PackageInfo? get syncPackageInfo;

The way this is described in terms of packageInfo, and the copy of the "this wraps …" information on packageInfo, is enough to cover this fact.

gnprice · 2024-07-12T05:51:00Z

test/model/binding.dart

-  /// The value that `ZulipBinding.instance.deviceInfo()` should return.
-  ///
-  /// See also [takeDeviceInfoCalls].
+  @override
+  Future<BaseDeviceInfo?> get deviceInfo async => deviceInfoResult;


nit: keep the ordering these had, which matches the ordering of other members on this class:

first the test-only data and methods for a given subsystem

then the implementation (with @override) of the app-facing members for that subsystem

gnprice · 2024-07-12T05:51:39Z

lib/model/binding.dart

+  /// encountered while fetching the data.
+  ///
+  /// This wraps [package_info_plus.PackageInfo.fromPlatform].
+  Future<PackageInfo?> get packageInfo;


nit in commit message:

binding: Add packageInfo getter This is a preparatory commit for the work of embedding the device info in user-agent header.

copy-paste error 🙂

gnprice · 2024-07-12T05:55:19Z

test/model/binding.dart

  BaseDeviceInfo deviceInfoResult = _defaultDeviceInfoResult;
-  static final _defaultDeviceInfoResult = AndroidDeviceInfo(sdkInt: 33);
+  static final _defaultDeviceInfoResult = AndroidDeviceInfo(sdkInt: 33, release: '13');


preexisting nit (would be a nice small added NFC commit):

Suggested change

static final _defaultDeviceInfoResult = AndroidDeviceInfo(sdkInt: 33, release: '13');

static const _defaultDeviceInfoResult = AndroidDeviceInfo(sdkInt: 33, release: '13');

And make all the device-info and package-info constructors const so that that compiles.

gnprice · 2024-07-12T06:02:31Z

lib/model/binding.dart

+  /// The major release number, such as 10 in version 10.9.3.
+  ///
+  /// See: https://developer.apple.com/documentation/foundation/operatingsystemversion/1414662-majorversion
+  final int majorVersion;


I see, the description is a copy of the Apple description.

Even though this is a very small amount of text, out of an abundance of caution let's avoid copying it.

I think the name is already pretty nearly self-explanatory, so just the link is enough (as that allows eliminating any ambiguity about exactly what notion of "major release number" is meant):

Suggested change

/// The major release number, such as 10 in version 10.9.3.

///

/// See: https://developer.apple.com/documentation/foundation/operatingsystemversion/1414662-majorversion

final int majorVersion;

/// See: https://developer.apple.com/documentation/foundation/operatingsystemversion/1414662-majorversion

final int majorVersion;

(similarly minorVersion and patchVersion)

gnprice · 2024-07-12T06:54:04Z

lib/model/binding.dart

+class LinuxDeviceInfo implements BaseDeviceInfo {
+  /// A string identifying the operating system, without a version component,
+  /// and suitable for presentation to the user.
+  ///
+  /// Examples: 'Fedora', 'Debian GNU/Linux', or just 'Linux'.
+  ///
+  /// See: https://www.freedesktop.org/software/systemd/man/latest/os-release.html#NAME=
+  final String name;


Oh and I guess the two in this class are similar to the Android case (though I haven't checked what license the upstream doc is under).

For these let's do a reduced version similar to what I did for Android above: say what external thing it comes from; give example values; give the link. The examples in this revision are fine.

gnprice · 2024-07-12T07:03:35Z

test/api/core_test.dart

+        testBinding.deviceInfoResult = deviceInfo;
+        testBinding.packageInfoResult = packageInfo;
+        await checkUserAgent(userAgent);


Similar to my other comment about testBinding.reset and teardowns:

Suggested change

testBinding.deviceInfoResult = deviceInfo;

testBinding.packageInfoResult = packageInfo;

await checkUserAgent(userAgent);

testBinding.deviceInfoResult = deviceInfo;

testBinding.packageInfoResult = packageInfo;

addTearDown(testBinding.reset);

await checkUserAgent(userAgent);

I think that suffices for taking the place of the global tearDown call.

gnprice · 2024-07-12T07:04:53Z

test/api/core_test.dart

+    final testCases = [
+      ('ZulipFlutter/0.0.1+1 (Android 14)',             AndroidDeviceInfo(release: '14', sdkInt: 34),                        packageInfo),


It looks like packageInfo doesn't actually vary between these test cases. In that case, let's simplify by taking it out of the data list.

gnprice · 2024-07-12T07:08:34Z

test/api/core_test.dart

+            .headers.deepEquals({
+              ...authHeader(email: eg.selfAccount.email, apiKey: eg.selfAccount.apiKey),
+              ...{'User-Agent': expectedUserAgent},
+            });


Since this test is all about the User-Agent (and other tests are checking the auth headers), let's simplify:

Suggested change

.headers.deepEquals({

...authHeader(email: eg.selfAccount.email, apiKey: eg.selfAccount.apiKey),

...{'User-Agent': expectedUserAgent},

});

.headers['User-Agent'].equals(expectedUserAgent);

gnprice · 2024-07-12T07:10:24Z

test/api/core_test.dart

+        useBinding: true,
+        (connection) async {
+          connection.prepare(json: {});
+          await connection.get(kExampleRouteName, (json) => json, 'example/route', null);


Let's also pick any one of the test cases below, and do one check with each of the different request methods: get, post, postFileFromStream, delete.

rajveermalviya · 2024-07-13T09:03:50Z

Thanks for the review @gnprice, pushed a new revision PTAL.

gnprice

Thanks @rajveermalviya for the revision! This all looks great — merging.

gnprice · 2024-07-14T23:38:01Z

test/api/route/messages_test.dart

-        ..headers['User-Agent'].equals(expectedUserAgent ?? userAgentHeader()['User-Agent']!);
+        ..headers['User-Agent'].equals(expectedUserAgent ?? kFallbackUserAgentHeader['User-Agent']!);


I like how much smaller the impact of this on unrelated tests is now!

This is a preparatory commit for the work of embedding the device info in user-agent header.

This is a preparatory commit for the work of embedding the package info in user-agent header.

Generate the user-agent using `deviceInfo` and `packageInfo` from ZulipBinding. Fixes: zulip#467

rajveermalviya commented Jun 5, 2024

View reviewed changes

lib/api/core.dart Outdated Show resolved Hide resolved

lib/api/core.dart Outdated Show resolved Hide resolved

rajveermalviya force-pushed the user-agent-info branch 4 times, most recently from 0131fb8 to d94d4ed Compare June 21, 2024 05:03

rajveermalviya commented Jun 21, 2024

View reviewed changes

rajveermalviya force-pushed the user-agent-info branch 2 times, most recently from 2a32df4 to 8458b72 Compare June 24, 2024 10:46

rajveermalviya marked this pull request as ready for review June 24, 2024 10:58

rajveermalviya added the buddy review GSoC buddy review needed. label Jun 24, 2024

rajveermalviya requested a review from sm-sayedi June 24, 2024 10:59

rajveermalviya commented Jun 24, 2024

View reviewed changes

rajveermalviya force-pushed the user-agent-info branch 2 times, most recently from a68aaca to a9cf8d3 Compare June 26, 2024 06:10

sm-sayedi reviewed Jun 26, 2024

View reviewed changes

rajveermalviya force-pushed the user-agent-info branch 2 times, most recently from 6e08702 to 798aec6 Compare June 30, 2024 20:21

rajveermalviya requested a review from sm-sayedi June 30, 2024 20:22

sm-sayedi approved these changes Jul 1, 2024

View reviewed changes

sm-sayedi added mentor review GSoC mentor review needed. and removed buddy review GSoC buddy review needed. labels Jul 1, 2024

rajveermalviya force-pushed the user-agent-info branch from 798aec6 to 7ba0e82 Compare July 1, 2024 07:01

rajveermalviya requested a review from sumanthvrao July 1, 2024 07:02

gnprice added the maintainer review PR ready for review by Zulip maintainers label Jul 3, 2024

gnprice requested a review from chrisbobbe July 3, 2024 04:25

chrisbobbe reviewed Jul 3, 2024

View reviewed changes

rajveermalviya force-pushed the user-agent-info branch from 7ba0e82 to 6ac9ab7 Compare July 7, 2024 21:48

rajveermalviya requested a review from chrisbobbe July 7, 2024 22:04

rajveermalviya force-pushed the user-agent-info branch from 6ac9ab7 to abbcfaf Compare July 7, 2024 22:05

chrisbobbe reviewed Jul 8, 2024

View reviewed changes

chrisbobbe added the integration review Added by maintainers when PR may be ready for integration label Jul 8, 2024

rajveermalviya force-pushed the user-agent-info branch from abbcfaf to 7023109 Compare July 9, 2024 16:56

gnprice reviewed Jul 10, 2024

View reviewed changes

rajveermalviya force-pushed the user-agent-info branch from 7023109 to 66df466 Compare July 11, 2024 17:55

rajveermalviya requested a review from gnprice July 11, 2024 18:03

gnprice reviewed Jul 12, 2024

View reviewed changes

chrisbobbe removed the maintainer review PR ready for review by Zulip maintainers label Jul 12, 2024

rajveermalviya force-pushed the user-agent-info branch from 66df466 to 7f2160c Compare July 13, 2024 08:58

rajveermalviya requested a review from gnprice July 13, 2024 09:03

gnprice reviewed Jul 14, 2024

View reviewed changes

rajveermalviya added 6 commits July 14, 2024 16:38

binding: Prefetch deviceInfo during initialization

2064bb6

This is a preparatory commit for the work of embedding the device info in user-agent header.

binding: Add packageInfo getter

ea4efc7

This is a preparatory commit for the work of embedding the package info in user-agent header.

about_zulip [nfc]: Refactor to use packageInfo from ZulipBinding

9031c66

binding: Support more device/os variants for deviceInfo getter

0659c48

binding [nfc]: Make PackageInfo and *DeviceInfo constructors const

1ff7071

api: Embed platform and app info in user-agent

022df71

Generate the user-agent using `deviceInfo` and `packageInfo` from ZulipBinding. Fixes: zulip#467

gnprice force-pushed the user-agent-info branch from 7f2160c to 022df71 Compare July 14, 2024 23:40

gnprice merged commit 022df71 into zulip:main Jul 14, 2024

rajveermalviya deleted the user-agent-info branch July 15, 2024 04:36

		WidgetsFlutterBinding.ensureInitialized();
		await LiveZulipBinding.ensureInitialized();

		///
		/// This field is optional and may be null on some systems.

	static final _defaultDeviceInfoResult = AndroidDeviceInfo(sdkInt: 33, release: '13');
	static const _defaultDeviceInfoResult = AndroidDeviceInfo(sdkInt: 33, release: '13');

		final testCases = [
		('ZulipFlutter/0.0.1+1 (Android 14)', AndroidDeviceInfo(release: '14', sdkInt: 34), packageInfo),

		..headers['User-Agent'].equals(expectedUserAgent ?? userAgentHeader()['User-Agent']!);
		..headers['User-Agent'].equals(expectedUserAgent ?? kFallbackUserAgentHeader['User-Agent']!);

api: Embed platform and app info in user-agent #724

api: Embed platform and app info in user-agent #724

Conversation

rajveermalviya commented Jun 5, 2024

rajveermalviya 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sm-sayedi 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

Choose a reason for hiding this comment

rajveermalviya commented Jun 30, 2024

sm-sayedi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rajveermalviya commented Jul 1, 2024

chrisbobbe 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

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

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

chrisbobbe Jul 3, 2024 • 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

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

rajveermalviya commented Jul 7, 2024

chrisbobbe 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

Choose a reason for hiding this comment

chrisbobbe commented Jul 9, 2024

gnprice 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

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

Choose a reason for hiding this comment

rajveermalviya commented Jul 11, 2024

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe Jul 3, 2024 •

edited

Loading