fix: context defualts on native platforms other than iOS/Android (#2439)

vaind · web-flow · commit ab0eee17a027 · 2024-11-25T21:17:19.000+01:00
* fix: context defualts on native platforms other than iOS/Android

* update tests

* analyzer issues

* chore: update changelog
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -34,6 +34,10 @@
 - Linux native error & obfuscation support ([#2431](https://github.com/getsentry/sentry-dart/pull/2431))
 - Improve Device context on plain Dart and Flutter desktop apps ([#2441](https://github.com/getsentry/sentry-dart/pull/2441))
 
+### Fixes
+
+- OS & device contexts missing on Windows ([#2439](https://github.com/getsentry/sentry-dart/pull/2439))
+
 ### Dependencies
 
 - Bump Cocoa SDK from v8.40.1 to v8.41.0 ([#2442](https://github.com/getsentry/sentry-dart/pull/2442))
diff --git a/dart/lib/src/event_processor/enricher/io_enricher_event_processor.dart b/dart/lib/src/event_processor/enricher/io_enricher_event_processor.dart
@@ -40,27 +40,12 @@ class IoEnricherEventProcessor implements EnricherEventProcessor {
     // Amend app with current memory usage, as this is not available on native.
     final app = _getApp(event.contexts.app);
 
-    // If there's a native integration available, it probably has better
-    // information available than Flutter.
-
-    final device = _options.platformChecker.hasNativeIntegration
-        ? null
-        : _getDevice(event.contexts.device);
-
-    final os = _options.platformChecker.hasNativeIntegration
-        ? null
-        : _getOperatingSystem(event.contexts.operatingSystem);
-
-    final culture = _options.platformChecker.hasNativeIntegration
-        ? null
-        : _getSentryCulture(event.contexts.culture);
-
     final contexts = event.contexts.copyWith(
-      device: device,
-      operatingSystem: os,
+      device: _getDevice(event.contexts.device),
+      operatingSystem: _getOperatingSystem(event.contexts.operatingSystem),
       runtimes: _getRuntimes(event.contexts.runtimes),
       app: app,
-      culture: culture,
+      culture: _getSentryCulture(event.contexts.culture),
     );
 
     contexts['dart_context'] = _getDartContext();
diff --git a/dart/test/event_processor/enricher/io_enricher_test.dart b/dart/test/event_processor/enricher/io_enricher_test.dart
@@ -43,23 +43,18 @@ void main() {
     expect(event.contexts.runtimes.length, 2);
   });
 
-  test('does not add device, os and culture if native integration is available',
-      () {
-    final enricher = fixture.getSut(hasNativeIntegration: true);
-    final event = enricher.apply(SentryEvent(), Hint());
-
-    expect(event?.contexts.device, isNull);
-    expect(event?.contexts.operatingSystem, isNull);
-    expect(event?.contexts.culture, isNull);
-  });
-
-  test('adds device, os and culture if no native integration is available', () {
-    final enricher = fixture.getSut(hasNativeIntegration: false);
-    final event = enricher.apply(SentryEvent(), Hint());
-
-    expect(event?.contexts.device, isNotNull);
-    expect(event?.contexts.operatingSystem, isNotNull);
-    expect(event?.contexts.culture, isNotNull);
+  group('adds device, os and culture', () {
+    for (final hasNativeIntegration in [true, false]) {
+      test('native=$hasNativeIntegration', () {
+        final enricher =
+            fixture.getSut(hasNativeIntegration: hasNativeIntegration);
+        final event = enricher.apply(SentryEvent(), Hint());
+
+        expect(event?.contexts.device, isNotNull);
+        expect(event?.contexts.operatingSystem, isNotNull);
+        expect(event?.contexts.culture, isNotNull);
+      });
+    }
   });
 
   test('device has name', () {
diff --git a/flutter/lib/src/integrations/load_contexts_integration.dart b/flutter/lib/src/integrations/load_contexts_integration.dart
@@ -1,6 +1,9 @@
 import 'dart:async';
 
 import 'package:sentry/sentry.dart';
+import 'package:collection/collection.dart';
+// ignore: implementation_imports
+import 'package:sentry/src/event_processor/enricher/enricher_event_processor.dart';
 import '../native/sentry_native_binding.dart';
 import '../sentry_flutter_options.dart';
 
@@ -24,6 +27,21 @@ class LoadContextsIntegration extends Integration<SentryFlutterOptions> {
     options.addEventProcessor(
       _LoadContextsIntegrationEventProcessor(_native, options),
     );
+
+    // We need to move [IOEnricherEventProcessor] after [_LoadContextsIntegrationEventProcessor]
+    // so that we know which contexts were set by the user and which were set by the other processor.
+    // The priorities are:
+    // - user-set context values
+    // - context values set from native (this)
+    // - values set from IOEnricherEventProcessor
+    final enricherEventProcessor = options.eventProcessors.firstWhereOrNull(
+      (element) => element is EnricherEventProcessor,
+    );
+    if (enricherEventProcessor != null) {
+      options.removeEventProcessor(enricherEventProcessor);
+      options.addEventProcessor(enricherEventProcessor);
+    }
+
     options.sdk.addIntegration('loadContextsIntegration');
   }
 }
diff --git a/flutter/test/sentry_flutter_test.dart b/flutter/test/sentry_flutter_test.dart
@@ -6,6 +6,7 @@ import 'package:package_info_plus/package_info_plus.dart';
 import 'package:sentry/src/platform/platform.dart';
 import 'package:sentry/src/dart_exception_type_identifier.dart';
 import 'package:sentry_flutter/sentry_flutter.dart';
+import 'package:sentry_flutter/src/file_system_transport.dart';
 import 'package:sentry_flutter/src/flutter_exception_type_identifier.dart';
 import 'package:sentry_flutter/src/integrations/connectivity/connectivity_integration.dart';
 import 'package:sentry_flutter/src/integrations/integrations.dart';
@@ -70,34 +71,31 @@ void main() {
     });
 
     test('Android', () async {
-      List<Integration> integrations = [];
-      Transport transport = MockTransport();
+      late final SentryFlutterOptions options;
+      late final Transport transport;
 
       final sentryFlutterOptions = defaultTestOptions(
           getPlatformChecker(platform: MockPlatform.android()))
         ..methodChannel = native.channel;
 
       await SentryFlutter.init(
-        (options) async {
-          options.dsn = fakeDsn;
-          options.profilesSampleRate = 1.0;
-          integrations = options.integrations;
-          transport = options.transport;
+        (o) async {
+          o.dsn = fakeDsn;
+          o.profilesSampleRate = 1.0;
+          options = o;
+          transport = o.transport;
         },
         appRunner: appRunner,
         options: sentryFlutterOptions,
       );
 
-      testTransport(
-        transport: transport,
-        hasFileSystemTransport: true,
-      );
+      expect(transport, isA<FileSystemTransport>());
 
       testScopeObserver(
           options: sentryFlutterOptions, expectedHasNativeScopeObserver: true);
 
       testConfiguration(
-        integrations: integrations,
+        integrations: options.integrations,
         shouldHaveIntegrations: [
           ...androidIntegrations,
           ...nativeIntegrations,
@@ -110,49 +108,51 @@ void main() {
         ],
       );
 
-      integrations
+      options.integrations
           .indexWhere((element) => element is WidgetsFlutterBindingIntegration);
 
       testBefore(
-          integrations: integrations,
+          integrations: options.integrations,
           beforeIntegration: WidgetsFlutterBindingIntegration,
           afterIntegration: OnErrorIntegration);
 
+      expect(
+          options.eventProcessors.indexOfTypeString('IoEnricherEventProcessor'),
+          greaterThan(options.eventProcessors
+              .indexOfTypeString('_LoadContextsIntegrationEventProcessor')));
+
       expect(SentryFlutter.native, isNotNull);
       expect(Sentry.currentHub.profilerFactory, isNull);
 
       await Sentry.close();
     }, testOn: 'vm');
 
     test('iOS', () async {
-      List<Integration> integrations = [];
-      Transport transport = MockTransport();
+      late final SentryFlutterOptions options;
+      late final Transport transport;
 
       final sentryFlutterOptions =
           defaultTestOptions(getPlatformChecker(platform: MockPlatform.iOs()))
             ..methodChannel = native.channel;
 
       await SentryFlutter.init(
-        (options) async {
-          options.dsn = fakeDsn;
-          options.profilesSampleRate = 1.0;
-          integrations = options.integrations;
-          transport = options.transport;
+        (o) async {
+          o.dsn = fakeDsn;
+          o.profilesSampleRate = 1.0;
+          options = o;
+          transport = o.transport;
         },
         appRunner: appRunner,
         options: sentryFlutterOptions,
       );
 
-      testTransport(
-        transport: transport,
-        hasFileSystemTransport: true,
-      );
+      expect(transport, isA<FileSystemTransport>());
 
       testScopeObserver(
           options: sentryFlutterOptions, expectedHasNativeScopeObserver: true);
 
       testConfiguration(
-        integrations: integrations,
+        integrations: options.integrations,
         shouldHaveIntegrations: [
           ...iOsAndMacOsIntegrations,
           ...nativeIntegrations,
@@ -166,14 +166,19 @@ void main() {
       );
 
       testBefore(
-          integrations: integrations,
+          integrations: options.integrations,
           beforeIntegration: WidgetsFlutterBindingIntegration,
           afterIntegration: OnErrorIntegration);
 
       expect(SentryFlutter.native, isNotNull);
       expect(Sentry.currentHub.profilerFactory,
           isInstanceOf<SentryNativeProfilerFactory>());
 
+      expect(
+          options.eventProcessors.indexOfTypeString('IoEnricherEventProcessor'),
+          greaterThan(options.eventProcessors
+              .indexOfTypeString('_LoadContextsIntegrationEventProcessor')));
+
       await Sentry.close();
     }, testOn: 'vm');
 
@@ -195,10 +200,7 @@ void main() {
         options: sentryFlutterOptions,
       );
 
-      testTransport(
-        transport: transport,
-        hasFileSystemTransport: true,
-      );
+      expect(transport, isA<FileSystemTransport>());
 
       testScopeObserver(
           options: sentryFlutterOptions, expectedHasNativeScopeObserver: true);
@@ -244,10 +246,7 @@ void main() {
         options: sentryFlutterOptions,
       );
 
-      testTransport(
-        transport: transport,
-        hasFileSystemTransport: false,
-      );
+      expect(transport, isNot(isA<FileSystemTransport>()));
 
       testScopeObserver(
           options: sentryFlutterOptions, expectedHasNativeScopeObserver: true);
@@ -295,10 +294,7 @@ void main() {
         options: sentryFlutterOptions,
       );
 
-      testTransport(
-        transport: transport,
-        hasFileSystemTransport: false,
-      );
+      expect(transport, isNot(isA<FileSystemTransport>()));
 
       testScopeObserver(
           options: sentryFlutterOptions, expectedHasNativeScopeObserver: true);
@@ -345,10 +341,7 @@ void main() {
         options: sentryFlutterOptions,
       );
 
-      testTransport(
-        transport: transport,
-        hasFileSystemTransport: false,
-      );
+      expect(transport, isNot(isA<FileSystemTransport>()));
 
       testScopeObserver(
           options: sentryFlutterOptions, expectedHasNativeScopeObserver: false);
@@ -396,10 +389,7 @@ void main() {
         options: sentryFlutterOptions,
       );
 
-      testTransport(
-        transport: transport,
-        hasFileSystemTransport: false,
-      );
+      expect(transport, isNot(isA<FileSystemTransport>()));
 
       testConfiguration(
         integrations: integrations,
@@ -441,10 +431,7 @@ void main() {
         options: sentryFlutterOptions,
       );
 
-      testTransport(
-        transport: transport,
-        hasFileSystemTransport: false,
-      );
+      expect(transport, isNot(isA<FileSystemTransport>()));
 
       testConfiguration(
         integrations: integrations,
@@ -487,10 +474,7 @@ void main() {
         options: sentryFlutterOptions,
       );
 
-      testTransport(
-        transport: transport,
-        hasFileSystemTransport: false,
-      );
+      expect(transport, isNot(isA<FileSystemTransport>()));
 
       testConfiguration(
         integrations: integrations,
diff --git a/flutter/test/sentry_flutter_util.dart b/flutter/test/sentry_flutter_util.dart
@@ -1,19 +1,7 @@
 import 'package:flutter_test/flutter_test.dart';
 import 'package:sentry_flutter/sentry_flutter.dart';
-import 'package:sentry_flutter/src/file_system_transport.dart';
 import 'package:sentry_flutter/src/native/native_scope_observer.dart';
 
-void testTransport({
-  required Transport transport,
-  required bool hasFileSystemTransport,
-}) {
-  expect(
-    transport is FileSystemTransport,
-    hasFileSystemTransport,
-    reason: '$FileSystemTransport was wrongly set',
-  );
-}
-
 void testScopeObserver(
     {required SentryFlutterOptions options,
     required bool expectedHasNativeScopeObserver}) {
@@ -54,9 +42,21 @@ void testBefore({
   required Type beforeIntegration,
   required Type afterIntegration,
 }) {
-  final beforeIndex = integrations
-      .indexWhere((element) => element.runtimeType == beforeIntegration);
-  final afterIndex = integrations
-      .indexWhere((element) => element.runtimeType == afterIntegration);
-  expect(beforeIndex < afterIndex, true);
+  expect(integrations.indexOfType(beforeIntegration),
+      lessThan(integrations.indexOfType(afterIntegration)));
+}
+
+extension ListExtension<T> on List<T> {
+  int indexOfType(Type type) {
+    final index = indexWhere((element) => element.runtimeType == type);
+    expect(index, greaterThanOrEqualTo(0), reason: '$type not found in $this');
+    return index;
+  }
+
+  int indexOfTypeString(String type) {
+    final index =
+        indexWhere((element) => element.runtimeType.toString() == type);
+    expect(index, greaterThanOrEqualTo(0), reason: '$type not found in $this');
+    return index;
+  }
 }
