Fix: Trim Unicode NULL character for Windows support #420

ueman · 2021-04-19T07:38:12Z

📜 Description

I replace every \u0000 with `` (empty string).

💡 Motivation and Context

#410

💚 How did you test it?

I've written new tests

📝 Checklist

🔮 Next steps

@bruno-garcia Could you please test this?

flutter/lib/src/default_integrations.dart

marandaneto · 2021-04-19T08:19:16Z

flutter/lib/src/default_integrations.dart

@@ -397,6 +409,7 @@ class LoadReleaseIntegration extends Integration<SentryFlutterOptions> {
        .replaceAll('\t', '_')
        .replaceAll('\r\n', '_')
        .replaceAll('\r', '_')
-        .replaceAll('\n', '_');
+        .replaceAll('\n', '_')
+        .replaceAll('\u{0000}', '');


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

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

bruno-garcia

The fix works! :)

https://sentry.io/organizations/sentry-sdks/issues/2137135923/?query=is%3Aunresolved

bruno-garcia · 2021-04-19T19:54:37Z

flutter/lib/src/default_integrations.dart

+          name = _cleanString(packageInfo.appName);
+        }
+
+        final version = _cleanString(packageInfo.version);


This block is guarded by either release or dist being empty on options. But below we only take the release built in here, if options.release is not empty, we just use that, and same for dist.

So if only options.release is set, we'll build a new release (as per this block) but won't use it at all.
Would it make sense to check if + exists and append the buildNumber we detected in that case?

I think the ?? does the job if I got what you meant eg

options.release = options.release ?? release;

but what we could do is to actually check if we need to build the release or dist only and not execute code that is not necessary, eg, building a new release if we're going to use the options.release anyway as its not empty

yeah it's already using options.release if it's already set, it's just odd that we build a new release value even though it's not used.

And I was trying to think of ways to combine what we built in the block with what was defined but best not mess around with a value that was explicitly set by the user on options.

So the only improvement here is as you said just to avoid doing the work if not needed though it'll make the logic more complicated

bruno-garcia · 2021-04-19T19:57:37Z

flutter/lib/src/default_integrations.dart

        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;


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

flutter/lib/src/default_integrations.dart

Co-authored-by: Manoel Aranda Neto <[email protected]>

marandaneto · 2021-04-19T20:12:02Z

The fix works! :)

https://sentry.io/organizations/sentry-sdks/issues/2137135923/?query=is%3Aunresolved

If I opened the right JSON, sentry_flutter_example\[email protected] its still wrong, could you double check @bruno-garcia ?

codecov-commenter · 2021-04-19T20:14:14Z

Codecov Report

Merging #420 (3c53608) into feature/support-desktop (a874f04) will increase coverage by 3.50%.
The diff coverage is n/a.

@@                     Coverage Diff                     @@
##           feature/support-desktop     #420      +/-   ##
===========================================================
+ Coverage                    90.02%   93.52%   +3.50%     
===========================================================
  Files                           54        5      -49     
  Lines                         1684      170    -1514     
===========================================================
- Hits                          1516      159    -1357     
+ Misses                         168       11     -157

Impacted Files	Coverage Δ
dart/lib/src/scope.dart
dart/lib/src/protocol/sentry_event.dart
dart/lib/src/protocol/sentry_stack_trace.dart
dart/lib/src/protocol/debug_image.dart
dart/lib/src/hub.dart
dart/lib/src/integration.dart
dart/lib/src/transport/http_transport.dart
dart/lib/src/platform/_io_platform.dart
dart/lib/src/protocol/sdk_version.dart
dart/lib/src/environment_variables.dart
... and 39 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a874f04...3c53608. Read the comment docs.

bruno-garcia · 2021-04-19T20:15:38Z

oof, you're right @marandaneto

  "release": "sentry_flutter_example\u0000@1.0.0",

bruno-garcia · 2021-04-19T20:17:13Z

There's no general 'error processing your event' on the event though. Wasn't that the case before this change?

marandaneto · 2021-04-19T20:22:49Z

There's no general 'error processing your event' on the event though. Wasn't that the case before this change?

before it was sentry_flutter_example\[email protected]\u0000+

ueman · 2021-04-19T20:25:08Z

There's no general 'error processing your event' on the event though. Wasn't that the case before this change?

I think that's because options.dist does not get set to an empty string anymore.

flutter/lib/src/default_integrations.dart

marandaneto · 2021-04-20T07:37:32Z

flutter/test/default_integrations_test.dart

+          .getIntegration(loader: loader)
+          .call(MockHub(), fixture.options);
+
+      expect(fixture.options.release, 'sentry_flutter_example+123');


I think this is wrong, in this case, if there's no version, it should be sentry_flutter_example@123

I'll confirm that, let's keep as it is for now

this is actually not going to happen because pubspec must have a version or it defaults to 0.0.0 https://dart.dev/tools/pub/pubspec#version
its only possible because we've mocked the values, so no special anything case here.

marandaneto

LGTM

ueman added 2 commits April 19, 2021 09:35

Fix: Trim Unicode Character 'NULL' (U+0000) for Windows

69541b2

Add PR ID

a2b8218

ueman marked this pull request as ready for review April 19, 2021 07:43

ueman requested review from bruno-garcia and marandaneto as code owners April 19, 2021 07:43

marandaneto reviewed Apr 19, 2021

View reviewed changes

flutter/lib/src/default_integrations.dart Outdated Show resolved Hide resolved

marandaneto reviewed Apr 19, 2021

View reviewed changes

Rename method and add some explanation

03a041e

marandaneto mentioned this pull request Apr 19, 2021

Feature: Initial support for Flutter on Desktop #389

Merged

5 tasks

bruno-garcia reviewed Apr 19, 2021

View reviewed changes

marandaneto reviewed Apr 19, 2021

View reviewed changes

flutter/lib/src/default_integrations.dart Outdated Show resolved Hide resolved

Update flutter/lib/src/default_integrations.dart

3c53608

Co-authored-by: Manoel Aranda Neto <[email protected]>

marandaneto reviewed Apr 19, 2021

View reviewed changes

flutter/lib/src/default_integrations.dart Show resolved Hide resolved

marandaneto reviewed Apr 19, 2021

View reviewed changes

flutter/lib/src/default_integrations.dart Show resolved Hide resolved

Add another test

109e5bc

marandaneto reviewed Apr 20, 2021

View reviewed changes

marandaneto approved these changes Apr 20, 2021

View reviewed changes

ueman merged commit 50ab07a into feature/support-desktop Apr 20, 2021

ueman deleted the fix/windows-invalid-chars branch April 20, 2021 08:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: Trim Unicode NULL character for Windows support #420

Fix: Trim Unicode NULL character for Windows support #420

ueman commented Apr 19, 2021 •

edited

Loading

marandaneto Apr 19, 2021

bruno-garcia Apr 19, 2021

bruno-garcia left a comment •

edited

Loading

bruno-garcia Apr 19, 2021

marandaneto Apr 19, 2021

bruno-garcia Apr 19, 2021

bruno-garcia Apr 19, 2021

ueman Apr 20, 2021

marandaneto commented Apr 19, 2021

codecov-commenter commented Apr 19, 2021 •

edited

Loading

bruno-garcia commented Apr 19, 2021

bruno-garcia commented Apr 19, 2021

marandaneto commented Apr 19, 2021

ueman commented Apr 19, 2021

marandaneto Apr 20, 2021

marandaneto Apr 20, 2021

marandaneto Apr 20, 2021 •

edited

Loading

marandaneto left a comment

Fix: Trim Unicode NULL character for Windows support #420

Fix: Trim Unicode NULL character for Windows support #420

Conversation

ueman commented Apr 19, 2021 • edited Loading

📜 Description

💡 Motivation and Context

💚 How did you test it?

📝 Checklist

🔮 Next steps

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bruno-garcia left a comment • 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

marandaneto commented Apr 19, 2021

codecov-commenter commented Apr 19, 2021 • edited Loading

Codecov Report

bruno-garcia commented Apr 19, 2021

bruno-garcia commented Apr 19, 2021

marandaneto commented Apr 19, 2021

ueman commented Apr 19, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marandaneto Apr 20, 2021 • edited Loading

Choose a reason for hiding this comment

marandaneto left a comment

Choose a reason for hiding this comment

ueman commented Apr 19, 2021 •

edited

Loading

bruno-garcia left a comment •

edited

Loading

codecov-commenter commented Apr 19, 2021 •

edited

Loading

marandaneto Apr 20, 2021 •

edited

Loading