Skip to content

Fix: Trim Unicode NULL character for Windows support #420

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

Merged
merged 5 commits into from
Apr 20, 2021
Merged
Show file tree
Hide file tree
Changes from 2 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
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
* Fix: Mark `Sentry.currentHub` as deprecated (#406)
* Fix: Use name from pubspec.yaml for release if package id is not available (#411)
* Feat: `SentryHttpClient` tracks the duration which a request takes and logs failed requests (#414)
* Fix: Trim `\u0000` from Windows package info (#420)

# 5.0.0

Expand Down
21 changes: 17 additions & 4 deletions flutter/lib/src/default_integrations.dart
Original file line number Diff line number Diff line change
Expand Up @@ -373,12 +373,24 @@ class LoadReleaseIntegration extends Integration<SentryFlutterOptions> {
name = _cleanAppName(packageInfo.appName);
}

final release =
'$name@${packageInfo.version}+${packageInfo.buildNumber}';
final version = _cleanAppName(packageInfo.version);
final buildNumber = _cleanAppName(packageInfo.buildNumber);

var release = name;
if (version.isNotEmpty) {
release = '$release@$version';
}
// At least windows sometimes does not have a buildNumber
if (buildNumber.isNotEmpty) {
release = '$release+$buildNumber';
}

options.logger(SentryLevel.debug, 'release: $release');

options.release = options.release ?? release;
options.dist = options.dist ?? packageInfo.buildNumber;
if (buildNumber.isNotEmpty) {
options.dist = options.dist ?? packageInfo.buildNumber;
Copy link
Member

Choose a reason for hiding this comment

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

We might need check the format of build number. Will this always be in a valid format as per Sentry's spec?
https://docs.sentry.io/platforms/dart/configuration/releases/#bind-the-version

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

See #422

}
}
} catch (error) {
options.logger(
Expand All @@ -397,6 +409,7 @@ class LoadReleaseIntegration extends Integration<SentryFlutterOptions> {
.replaceAll('\t', '_')
.replaceAll('\r\n', '_')
.replaceAll('\r', '_')
.replaceAll('\n', '_');
.replaceAll('\n', '_')
.replaceAll('\u{0000}', '');
Copy link
Contributor

Choose a reason for hiding this comment

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

we need to be sure that this works on Windows, maybe @bruno-garcia can test it?

Copy link
Member

Choose a reason for hiding this comment

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

I dream of integration test on the target platform that will validate these for us. Mainly because of regressions introduced later.

Thoughts? Q2 is the time to plan and book these tasks

}
}
41 changes: 40 additions & 1 deletion flutter/test/default_integrations_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -267,7 +267,8 @@ void main() {
expect(fixture.options.dist, '789');
});

test('release name does not contain ivalid chars', () async {
test('release name does not contain invalid chars defined by Sentry',
() async {
final loader = () {
return Future.value(PackageInfo(
appName: '\\/sentry\tflutter \r\nfoo\nbar\r',
Expand All @@ -283,6 +284,44 @@ void main() {
expect(fixture.options.release, '__sentry_flutter [email protected]+789');
expect(fixture.options.dist, '789');
});

/// See the following issues:
/// - https://github.com/getsentry/sentry-dart/issues/410
/// - https://github.com/fluttercommunity/plus_plugins/issues/182
test('uses acceptable release name on windows', () async {
final loader = () {
return Future.value(PackageInfo(
// As per
// https://api.dart.dev/stable/2.12.4/dart-core/String-class.html
// this is how \u0000 is added to a string in dart
appName: 'sentry_flutter_example\u{0000}',
packageName: '',
version: '1.0.0\u{0000}',
buildNumber: '',
));
};
await fixture
.getIntegration(loader: loader)
.call(MockHub(), fixture.options);

expect(fixture.options.release, '[email protected]');
});

test('dist is null if build number is an empty string', () async {
final loader = () {
return Future.value(PackageInfo(
appName: 'sentry_flutter_example',
packageName: 'a.b.c',
version: '1.0.0',
buildNumber: '',
));
};
await fixture
.getIntegration(loader: loader)
.call(MockHub(), fixture.options);

expect(fixture.options.dist, isNull);
});
});
}

Expand Down