Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix late initialization error for Analytics.userProperty #239

Merged
merged 5 commits into from
Feb 28, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions pkgs/unified_analytics/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
## 5.8.5

- Fix late initialization error for `Analytics.userProperty` [bug](https://github.com/dart-lang/tools/issues/238)

## 5.8.4

- Exporting all enums from [`enums.dart`](https://github.com/dart-lang/tools/blob/main/pkgs/unified_analytics/lib/src/enums.dart) through `lib/testing.dart`
Expand Down
5 changes: 4 additions & 1 deletion pkgs/unified_analytics/lib/src/analytics.dart
Original file line number Diff line number Diff line change
Expand Up @@ -430,7 +430,6 @@ class AnalyticsImpl implements Analytics {
homeDirectory: homeDirectory,
fs: fs,
errorHandler: _errorHandler,
telemetryEnabled: telemetryEnabled,
);
userProperty = UserProperty(
session: _sessionHandler,
Expand All @@ -454,6 +453,10 @@ class AnalyticsImpl implements Analytics {
homeDirectory: homeDirectory,
errorHandler: _errorHandler,
);

// Initialize the session handler with the session_id and last_ping
// variables by parsing the json file
_sessionHandler.initialize(telemetryEnabled);
}

@override
Expand Down
2 changes: 1 addition & 1 deletion pkgs/unified_analytics/lib/src/constants.dart
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ const int kLogFileLength = 2500;
const String kLogFileName = 'dart-flutter-telemetry.log';

/// The current version of the package, should be in line with pubspec version.
const String kPackageVersion = '5.8.4';
const String kPackageVersion = '5.8.5';

/// The minimum length for a session.
const int kSessionDurationMinutes = 30;
Expand Down
19 changes: 11 additions & 8 deletions pkgs/unified_analytics/lib/src/session.dart
Original file line number Diff line number Diff line change
Expand Up @@ -26,16 +26,9 @@ class Session {
required this.homeDirectory,
required this.fs,
required ErrorHandler errorHandler,
required bool telemetryEnabled,
}) : sessionFile = fs.file(p.join(
homeDirectory.path, kDartToolDirectoryName, kSessionFileName)),
_errorHandler = errorHandler {
// We must check if telemetry is enabled to refresh the session data
// because the refresh method will write to the session file and for
// users that have opted out, we have to leave the session file empty
// per the privacy document
if (telemetryEnabled) _refreshSessionData();
}
_errorHandler = errorHandler;

/// This will use the data parsed from the
/// session json file in the dart-tool directory
Expand Down Expand Up @@ -70,6 +63,16 @@ class Session {
return _sessionId;
}

/// Preps the [Session] class with the data found in the session json file.
///
/// We must check if telemetry is enabled to refresh the session data
/// because the refresh method will write to the session file and for
/// users that have opted out, we have to leave the session file empty
/// per the privacy document
void initialize(bool telemetryEnabled) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it worries me anytime there is an initialize() function that one must remember to call at the right time, because a refactor could easily lead to code now accessing state in this class BEFORE the initialize() method was called.

I think this would solve the particular issue linked, and maybe this is the right change to make immediately to stop crashes, but I think long-term we should refactor all of these classes (especially Analytics) such that it is not possible to access any of its state BEFORE it has been initialized.

if (telemetryEnabled) _refreshSessionData();
}

/// Return a json formatted representation of the class.
String toJson() => jsonEncode(<String, int>{
'session_id': _sessionId,
Expand Down
2 changes: 1 addition & 1 deletion pkgs/unified_analytics/pubspec.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ description: >-
to Google Analytics.
# When updating this, keep the version consistent with the changelog and the
# value in lib/src/constants.dart.
version: 5.8.4
version: 5.8.5
repository: https://github.com/dart-lang/tools/tree/main/pkgs/unified_analytics

environment:
Expand Down
71 changes: 39 additions & 32 deletions pkgs/unified_analytics/test/unified_analytics_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,6 @@ import 'package:file/memory.dart';
import 'package:test/test.dart';
import 'package:unified_analytics/src/constants.dart';
import 'package:unified_analytics/src/enums.dart';
import 'package:unified_analytics/src/error_handler.dart';
import 'package:unified_analytics/src/session.dart';
import 'package:unified_analytics/src/user_property.dart';
import 'package:unified_analytics/src/utils.dart';
import 'package:unified_analytics/unified_analytics.dart';
import 'package:yaml/yaml.dart';
Expand All @@ -32,7 +29,6 @@ void main() {
late File configFile;
late File logFile;
late File dismissedSurveyFile;
late UserProperty userProperty;

const homeDirName = 'home';
const initialTool = DashTool.flutterTool;
Expand All @@ -45,10 +41,7 @@ void main() {
const flutterVersion = 'flutterVersion';
const dartVersion = 'dartVersion';
const platform = DevicePlatform.macos;
const hostOsVersion = 'Version 14.1 (Build 23B74)';
const locale = 'en';
const clientIde = 'VSCode';
const enabledFeatures = 'enable-linux-desktop,cli-animations';

final testEvent = Event.hotReloadTime(timeMs: 50);

Expand Down Expand Up @@ -113,26 +106,6 @@ void main() {
dismissedSurveyFile = home
.childDirectory(kDartToolDirectoryName)
.childFile(kDismissedSurveyFileName);

// Create the user property object that is also
// created within analytics for testing
userProperty = UserProperty(
session: Session(
homeDirectory: home,
fs: fs,
errorHandler: ErrorHandler(sendFunction: analytics.send),
telemetryEnabled: analytics.telemetryEnabled,
),
flutterChannel: flutterChannel,
host: platform.label,
flutterVersion: flutterVersion,
dartVersion: dartVersion,
tool: initialTool.label,
hostOsVersion: hostOsVersion,
locale: locale,
clientIde: clientIde,
enabledFeatures: enabledFeatures,
);
});

test('Initializer properly sets up on first run', () {
Expand Down Expand Up @@ -169,7 +142,7 @@ void main() {
withClock(Clock.fixed(start), () {
final timestamp = clock.now().millisecondsSinceEpoch.toString();
expect(sessionFile.readAsStringSync(), 'contents');
userProperty.preparePayload();
analytics.userProperty.preparePayload();
expect(sessionFile.readAsStringSync(),
'{"session_id":$timestamp,"last_ping":$timestamp}');

Expand All @@ -183,6 +156,38 @@ void main() {
});
});

test('Handles malformed session file on startup', () {
// Ensure that we are able to send an error message on startup if
// we encounter an error while parsing the contents of the json file
// for session data
sessionFile.writeAsStringSync('contents');

analytics = Analytics.test(
tool: initialTool,
homeDirectory: home,
measurementId: measurementId,
apiSecret: apiSecret,
flutterChannel: flutterChannel,
toolsMessageVersion: toolsMessageVersion,
toolsMessage: toolsMessage,
flutterVersion: flutterVersion,
dartVersion: dartVersion,
fs: fs,
platform: platform,
clientIde: clientIde,
) as FakeAnalytics;
analytics.clientShowedMessage();

final errorEvent = analytics.sentEvents
.where((element) => element.eventName == DashEvent.analyticsException)
.firstOrNull;
expect(errorEvent, isNotNull);
expect(errorEvent!.eventData['workflow'], 'Session._refreshSessionData');
expect(errorEvent.eventData['error'], 'FormatException');
expect(errorEvent.eventData['description'],
'message: Unexpected character\nsource: contents');
});

test('Resetting session file when file is removed', () {
// Purposefully write delete the file
sessionFile.deleteSync();
Expand All @@ -194,7 +199,7 @@ void main() {
withClock(Clock.fixed(start), () {
final timestamp = clock.now().millisecondsSinceEpoch.toString();
expect(sessionFile.existsSync(), false);
userProperty.preparePayload();
analytics.userProperty.preparePayload();
expect(sessionFile.readAsStringSync(),
'{"session_id":$timestamp,"last_ping":$timestamp}');

Expand Down Expand Up @@ -850,7 +855,7 @@ ${initialTool.label}=$dateStamp,$toolsMessageVersion
clientId: Uuid().generateV4(),
eventName: DashEvent.hotReloadTime,
eventData: eventData,
userProperty: userProperty,
userProperty: analytics.userProperty,
);

// Checks for the top level keys
Expand Down Expand Up @@ -1104,15 +1109,17 @@ ${initialTool.label}=$dateStamp,$toolsMessageVersion
// 4. Event names must be 40 characters or fewer, may only contain alpha-numeric
// characters and underscores, and must start with an alphabetic character
test('max 25 user properties per event', () {
final Map<String, Object> userPropPayload = userProperty.preparePayload();
final Map<String, Object> userPropPayload =
analytics.userProperty.preparePayload();
const maxUserPropKeys = 25;

expect(userPropPayload.keys.length < maxUserPropKeys, true,
reason: 'There are too many keys in the UserProperty payload');
});

test('max 24 characters for user prop keys', () {
final Map<String, Object> userPropPayload = userProperty.preparePayload();
final Map<String, Object> userPropPayload =
analytics.userProperty.preparePayload();
const maxUserPropLength = 24;

var userPropLengthValid = true;
Expand Down