Fix: Read all environment variables in sentry

CHANGELOG.md

@@ -6,6 +6,7 @@
 * Feat: Send default PII options (#360)
 * Bump: sentry-cocoa to v6.2.1 (#360)
 * Fix: missing event.origin and event.environment tags for non-native events on iOS (#369)
+* Fix: Read all environment variables in sentry_dart (#375)
 
 ## Breaking Changes:
 

dart/lib/src/environment_variables.dart

@@ -0,0 +1,60 @@
+import 'package:meta/meta.dart';
+
+import 'sentry_options.dart';
+
+/// This method reads available environment variables and uses them
+/// accordingly.
+/// To see which environment variables are available, see [EnvironmentVariables]
+///
+/// The precendence of these options are tricky,
+/// see https://docs.sentry.io/platforms/dart/configuration/options/
+/// and https://github.com/getsentry/sentry-dart/issues/306
+@internal
+void setEnvironmentVariables(SentryOptions options, EnvironmentVariables vars) {
+  // options has precendence over vars
+  options.dsn = options.dsn ?? vars.dsn;
+
+  var environment = options.platformChecker.environment;
+  options.environment = options.environment ?? environment;
+
+  // vars has precedence over options
+  options.environment = vars.environment ?? options.environment;
+
+  // vars has precedence over options
+  options.release = vars.release ?? options.release;
+  options.dist = vars.dist ?? options.dist;
+}
+
+/// Reads environment variables from the system.
+/// In an Flutter environment these can be set via
+/// `flutter build --dart-define=VARIABLE_NAME=VARIABLE_VALUE`.
+class EnvironmentVariables {
+  static const _sentryEnvironment = 'SENTRY_ENVIRONMENT';
+  static const _sentryDsn = 'SENTRY_DSN';
+  static const _sentryRelease = 'SENTRY_RELEASE';
+  static const _sentryDist = 'SENTRY_DSN';
+
+  /// `SENTRY_ENVIRONMENT`
+  /// See [SentryOptions.environment]
+  String? get environment => const bool.hasEnvironment(_sentryEnvironment)
+      ? const String.fromEnvironment(_sentryEnvironment)
+      : null;
+
+  /// `SENTRY_DSN`
+  /// See [SentryOptions.dsn]
+  String? get dsn => const bool.hasEnvironment(_sentryDsn)
+      ? const String.fromEnvironment(_sentryDsn)
+      : null;
+
+  // `SENTRY_RELEASE`
+  /// See [SentryOptions.release]
+  String? get release => const bool.hasEnvironment(_sentryRelease)
+      ? const String.fromEnvironment(_sentryRelease)
+      : null;
+
+  /// `SENTRY_DIST`
+  /// See [SentryOptions.dist]
+  String? get dist => const bool.hasEnvironment(_sentryDist)
+      ? const String.fromEnvironment(_sentryDist)
+      : null;
+}

dart/lib/src/platform_checker.dart

@@ -1,3 +1,5 @@
+import 'sentry_options.dart';
+
 /// Helper to check in which enviroment the library is running.
 /// The envirment checks (release/debug/profile) are mutually exclusive.
 class PlatformChecker {
@@ -17,4 +19,18 @@ class PlatformChecker {
   bool isProfileMode() {
     return const bool.fromEnvironment('dart.vm.profile', defaultValue: false);
   }
+
+  /// This can be set as [SentryOptions.environment]
+  String get environment {
+    // We infer the enviroment based on the release/non-release and profile
+    // constants.
+
+    if (isReleaseMode()) {
+      return defaultEnvironment;
+    }
+    if (isProfileMode()) {
+      return 'profile';
+    }
+    return 'debug';
+  }
 }

dart/lib/src/sentry.dart

@@ -1,6 +1,7 @@
 import 'dart:async';
 
 import 'default_integrations.dart';
+import 'environment_variables.dart';
 import 'hub.dart';
 import 'hub_adapter.dart';
 import 'noop_isolate_error_integration.dart'
@@ -53,23 +54,7 @@ class Sentry {
 
   static Future<void> _initDefaultValues(
       SentryOptions options, AppRunner? appRunner) async {
-    // We infer the enviroment based on the release/non-release and profile
-    // constants.
-    var environment = options.platformChecker.isReleaseMode()
-        ? defaultEnvironment
-        : options.platformChecker.isProfileMode()
-            ? 'profile'
-            : 'debug';
-
-    // if the SENTRY_ENVIRONMENT is set, we read from it.
-    options.environment = const bool.hasEnvironment('SENTRY_ENVIRONMENT')
-        ? const String.fromEnvironment('SENTRY_ENVIRONMENT')
-        : environment;
-
-    // if the SENTRY_DSN is set, we read from it.
-    options.dsn = const bool.hasEnvironment('SENTRY_DSN')
-        ? const String.fromEnvironment('SENTRY_DSN')
-        : options.dsn;
+    setEnvironmentVariables(options, EnvironmentVariables());
 
     // Throws when running on the browser
     if (!isWeb) {

dart/lib/src/sentry_options.dart

@@ -15,6 +15,10 @@ import 'platform_checker.dart';
 /// Default Environment is none is set
 const defaultEnvironment = 'production';
 
+// TODO: Scope observers, enableScopeSync
+// TODO: shutdownTimeout, flushTimeoutMillis
+// https://api.dart.dev/stable/2.10.2/dart-io/HttpClient/close.html doesn't have a timeout param, we'd need to implement manually
+
 /// Sentry SDK options
 class SentryOptions {
   /// Default Log level if not specified Default is DEBUG
@@ -68,9 +72,6 @@ class SentryOptions {
 
   final List<Integration> _integrations = [];
 
-  // TODO: shutdownTimeout, flushTimeoutMillis
-  // https://api.dart.dev/stable/2.10.2/dart-io/HttpClient/close.html doesn't have a timeout param, we'd need to implement manually
-
   /// Code that provides middlewares, bindings or hooks into certain frameworks or environments,
   /// along with code that inserts those bindings and activates them.
   List<Integration> get integrations => List.unmodifiable(_integrations);
@@ -95,11 +96,13 @@ class SentryOptions {
   BeforeBreadcrumbCallback? beforeBreadcrumb;
 
   /// Sets the release. SDK will try to automatically configure a release out of the box
+  /// See [docs for further information](https://docs.sentry.io/platforms/flutter/configuration/releases/)
   String? release;
 
   /// Sets the environment. This string is freeform and not set by default. A release can be
   /// associated with more than one environment to separate them in the UI Think staging vs prod or
   /// similar.
+  /// See [docs for further information](https://docs.sentry.io/platforms/flutter/configuration/environments/)
   String? environment;
 
   /// Configures the sample rate as a percentage of events to be sent in the range of 0.0 to 1.0. if
@@ -139,7 +142,7 @@ class SentryOptions {
   /// however, when this option is set, stack traces are also sent with messages.
   /// This option, for instance, means that stack traces appear next to all log messages.
   ///
-  /// This option is true` by default.
+  /// This option is `true` by default.
   ///
   /// Grouping in Sentry is different for events with stack traces and without.
   /// As a result, you will get new groups as you enable or disable this flag for certain events.
@@ -155,8 +158,6 @@ class SentryOptions {
   /// Whether to send personal identifiable information along with events
   bool sendDefaultPii = false;
 
-  // TODO: Scope observers, enableScopeSync
-
   SentryOptions({this.dsn}) {
     sdk.addPackage('pub:sentry', sdkVersion);
   }

dart/test/environment_test.dart

@@ -0,0 +1,51 @@
+import 'package:sentry/sentry.dart';
+import 'package:sentry/src/environment_variables.dart';
+import 'package:test/test.dart';
+
+import 'mocks.dart';
+import 'mocks/mock_environment_variables.dart';
+
+void main() {
+  group('Environment Variables', () {
+    // See https://docs.sentry.io/platforms/dart/configuration/options/
+    // and https://github.com/getsentry/sentry-dart/issues/306
+    test('SentryOptions are correctly overriden by environment', () {
+      final options = SentryOptions(dsn: fakeDsn);
+      options.release = 'release-1.2.3';
+      options.dist = 'foo';
+      options.environment = 'prod';
+
+      setEnvironmentVariables(
+        options,
+        MockEnvironmentVariables(
+          dsn: 'foo-bar',
+          environment: 'staging',
+          release: 'release-9.8.7',
+          dist: 'bar',
+        ),
+      );
+
+      expect(options.dsn, fakeDsn);
+      expect(options.environment, 'staging');
+      expect(options.release, 'release-9.8.7');
+      expect(options.dist, 'bar');
+    });
+
+    test('No environment variables are set', () {
+      final options = SentryOptions(dsn: fakeDsn);
+      options.environment = 'prod';
+      options.release = 'release-1.2.3';
+      options.dist = 'foo';
+
+      setEnvironmentVariables(
+        options,
+        MockEnvironmentVariables(),
+      );
+
+      expect(options.dsn, fakeDsn);
+      expect(options.environment, 'prod');
+      expect(options.release, 'release-1.2.3');
+      expect(options.dist, 'foo');
+    });
+  });
+}

dart/test/fake_platform_checker.dart

@@ -1,7 +1,7 @@
 import 'package:sentry/src/platform_checker.dart';
 
 /// Fake class to be used in unit tests for mocking different environments
-class FakePlatformChecker implements PlatformChecker {
+class FakePlatformChecker extends PlatformChecker {
   FakePlatformChecker.releaseMode() {
     _releaseMode = true;
     _debugMode = false;

dart/test/mocks/mock_environment_variables.dart

@@ -0,0 +1,30 @@
+import 'package:sentry/src/environment_variables.dart';
+
+class MockEnvironmentVariables implements EnvironmentVariables {
+  MockEnvironmentVariables({
+    String? dist,
+    String? dsn,
+    String? environment,
+    String? release,
+  })  : _dist = dist,
+        _dsn = dsn,
+        _environment = environment,
+        _release = release;
+
+  final String? _dist;
+  final String? _dsn;
+  final String? _environment;
+  final String? _release;
+
+  @override
+  String? get dist => _dist;
+
+  @override
+  String? get dsn => _dsn;
+
+  @override
+  String? get environment => _environment;
+
+  @override
+  String? get release => _release;
+}

flutter/lib/src/default_integrations.dart

@@ -340,8 +340,7 @@ class LoadAndroidImageListIntegration
 /// a PackageInfo wrapper to make it testable
 typedef PackageLoader = Future<PackageInfo> Function();
 
-/// an Integration that loads the Release version from Native Apps
-/// or SENTRY_RELEASE and SENTRY_DIST variables
+/// An [Integration] that loads the release version from native apps
 class LoadReleaseIntegration extends Integration<SentryFlutterOptions> {
   final PackageLoader _packageLoader;
 
@@ -350,6 +349,7 @@ class LoadReleaseIntegration extends Integration<SentryFlutterOptions> {
   @override
   FutureOr<void> call(Hub hub, SentryFlutterOptions options) async {
     try {
+      // For non Flutter apps, we read the environment variables in sentry_dart
       if (!kIsWeb) {
         final packageInfo = await _packageLoader();
         final release =
@@ -358,15 +358,6 @@ class LoadReleaseIntegration extends Integration<SentryFlutterOptions> {
 
         options.release = release;
         options.dist = packageInfo.buildNumber;
-      } else {
-        // for non-mobile builds, we read the release and dist from the
-        // system variables (SENTRY_RELEASE and SENTRY_DIST).
-        options.release = const bool.hasEnvironment('SENTRY_RELEASE')
-            ? const String.fromEnvironment('SENTRY_RELEASE')
-            : options.release;
-        options.dist = const bool.hasEnvironment('SENTRY_DIST')
-            ? const String.fromEnvironment('SENTRY_DIST')
-            : options.dist;
       }
     } catch (error) {
       options.logger(