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

Wrap type conversions in try-catch to prevent crashes due to unhandled exceptions #15640

Merged
merged 1 commit into from
May 9, 2024

Conversation

BAndysc
Copy link
Contributor

@BAndysc BAndysc commented May 7, 2024

What does the pull request do?

This PR prevents crashes due to illegal conversions in TargetTypeConverter, i.e. if a numeric type is bound to a TextBox (only when using CompiledBindings). Yes, currently in 11.1 if you type a letter in a textbox with {Binding NumericType}, it will crash the whole app (with CompiledBindings).

What is the current behavior?

The type conversion exceptions in Compiled Bindings are not handled.

What is the updated/expected behavior with this PR?

The type conversion exceptions are silently ignored - just like before the binding refactor.

How was the solution implemented (if it's not obvious)?

The conversion happens in TryConvert methods, which name implies that a failure should not propagate. It also worked this way before:

try
{
result = Convert.ChangeType(value, to, CultureInfo.InvariantCulture);
return true;
}
catch
{
result = null;
return false;
}
}
}

So I added analogous try {} catch {} in the new typed conveter.

Checklist

Breaking changes

n/a

Obsoletions / Deprecations

Fixed issues

Fixes #15546

@maxkatz6 maxkatz6 requested a review from grokys May 7, 2024 09:17
@maxkatz6 maxkatz6 added bug regression backport-candidate-11.1.x Consider this PR for backporting to 11.1 branch labels May 7, 2024
@avaloniaui-bot
Copy link

You can test this PR using the following package version. 11.2.999-cibuild0048162-alpha. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

@cla-avalonia
Copy link
Collaborator

cla-avalonia commented May 7, 2024

  • All contributors have signed the CLA.

@BAndysc
Copy link
Contributor Author

BAndysc commented May 7, 2024

@cla-avalonia agree

@workgroupengineering
Copy link
Contributor

In my opinion, the error should be added to the Binding log instead of ignoring it completely.

@BAndysc
Copy link
Contributor Author

BAndysc commented May 7, 2024

Typing a letter in a number field is an user error and it would spam the log.

@workgroupengineering
Copy link
Contributor

Without exceptions or messages in log it would be difficult to identify any errors in a Converter. The log level can be customized.

@BAndysc
Copy link
Contributor Author

BAndysc commented May 8, 2024

Without exceptions or messages in log it would be difficult to identify any errors in a Converter. The log level can be customized.

The exception will be shown in Data Validation of the control. This PR refers only to built in Type Converters, i.e. if you bind an integer into a TextBox's Text

Copy link
Member

@grokys grokys left a comment

Choose a reason for hiding this comment

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

LGTM, thanks! Quite an oversight there! (I really wish TypeConverter had TryX methods!)

@grokys grokys added this pull request to the merge queue May 9, 2024
Merged via the queue into AvaloniaUI:master with commit 93d2140 May 9, 2024
10 checks passed
grokys pushed a commit that referenced this pull request Jun 3, 2024
@grokys grokys added backported-11.1.x and removed backport-candidate-11.1.x Consider this PR for backporting to 11.1 branch labels Jun 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants