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

11.1.0-beta1 stackoverflow on property set (SetProperty), works in 11.0.10 #15201

Closed
BobbyCannon opened this issue Apr 2, 2024 · 8 comments · Fixed by #15722
Closed

11.1.0-beta1 stackoverflow on property set (SetProperty), works in 11.0.10 #15201

BobbyCannon opened this issue Apr 2, 2024 · 8 comments · Fixed by #15722

Comments

@BobbyCannon
Copy link

Describe the bug

This code below is stack overflowing on 11.1.0-beta1. If I roll back to 11.0.10 the code works fine.

[Required(ErrorMessage = "Name is required!")]
public string Name
{
	get => _name;
	set => SetProperty(ref _name, value, true);
}

To Reproduce

Call ObservableValidator.SetProperty in 11.1.0-beta1 leads to stackoverflow.

[Required(ErrorMessage = "Name is required!")]

I think it's due to the required attribute but I'm not sure.

Expected behavior

The code should not stackoverflow?

Avalonia version

11.1.0-beta1

OS

Windows

Additional context

No response

@BobbyCannon BobbyCannon added the bug label Apr 2, 2024
@BobbyCannon BobbyCannon changed the title 11.1.0-beta1 stack overflow 11.1.0-beta1 stackoverflow on property set (SetProperty), works in 11.0.10 Apr 2, 2024
@maxkatz6
Copy link
Member

maxkatz6 commented Apr 3, 2024

Stacktrace (of stackoverflow) and SetProperty implementation are required to help here. Or minimal repro project.

@laolarou726
Copy link
Contributor

laolarou726 commented Apr 3, 2024

@BobbyCannon

I have the same issue, check if your App.axaml.cs's OnFrameworkInitializationCompleted() has following code:

BindingPlugins.DataValidators.RemoveAt(0);

@maxkatz6 Maybe consider adding this note in the Release message

@maxkatz6
Copy link
Member

maxkatz6 commented Apr 3, 2024

@laolarou726 what exactly should be added to the release message?

@laolarou726
Copy link
Contributor

@laolarou726 what exactly should be added to the release message?

I mean this line of code. When I upgrading the Avalonia version for my older project. I also noticed this issue. After some investigation, I noticed this line of code has been added to the latest template project. So maybe add this notice some where in the document or Release message might be a good idea?

@timunie
Copy link
Contributor

timunie commented Apr 3, 2024

minimal repro to paste into src/Samples/Sandbox

        public MainWindow()
        {
            InitializeComponent();
            DataContext = new ViewModel();
        }

....

    public partial class ViewModel : ObservableValidator
    {
        // [Required(ErrorMessage = "Name is required!")] 
        [ObservableProperty] 
        [NotifyDataErrorInfo]
        private string? _Name;
    }

and

  <TextBox Text="{Binding Name}" />

seems to stuck around here:
image

@timunie
Copy link
Contributor

timunie commented Apr 3, 2024

... and here:
image

@BobbyCannon
Copy link
Author

I had commented that line of code then removed it.

image

I was concerned about two things...

  1. Why 0 index, what it that changes in an upgrade?
  2. What was being removed.

This morning I wrote this...

// Removes {Avalonia.Data.Core.Plugins.DataAnnotationsValidationPlugin}
var found = BindingPlugins.DataValidators.FirstOrDefault(x => x is DataAnnotationsValidationPlugin);
if (found != null)
{
	BindingPlugins.DataValidators.Remove(found);
}

It won't break if the validator moves to a different location.

Also is this still just a bug with the DataAnnotationsValidationPlugin?

I noticed while debugging this value adds { Value: } around the previous value.

@timunie
Copy link
Contributor

timunie commented Apr 3, 2024

@BobbyCannon https://docs.avaloniaui.net/docs/guides/development-guides/data-validation#manage-validationplugins

However, this is still a regression imo, so keep this ticket open for now.

grokys added a commit that referenced this issue May 14, 2024
grokys added a commit that referenced this issue May 14, 2024
When #13970 was written, [a check](https://github.com/AvaloniaUI/Avalonia/pull/13970/files#diff-cfb25a491b9452e1815aa2c0d71465aaf81e99792a88a04a1a2ed572fd1930ffR60) was added to ensure that nested `BindingNotification`s didn't happen, and the refactor was written with the assumption that they wouldn't happen.

The problem is that they _do_ happen: when a source object implements both `INotifyDataErrorInfo` and had data annotations, then the nested data validation plugins would each wrap the value coming from the previous plugin in a new `BindingNotification`, resulting in nested `BindingNotifications`.

This adds support for nested binding notifications back in - even though IMO nesting binding notifications is a bug, if we're doing it and we previously supported it then we should continue to support it.

Fixes #15201
github-merge-queue bot pushed a commit that referenced this issue May 14, 2024
* Added failing test for #15201.

* Handle nested BindingNotifications.

When #13970 was written, [a check](https://github.com/AvaloniaUI/Avalonia/pull/13970/files#diff-cfb25a491b9452e1815aa2c0d71465aaf81e99792a88a04a1a2ed572fd1930ffR60) was added to ensure that nested `BindingNotification`s didn't happen, and the refactor was written with the assumption that they wouldn't happen.

The problem is that they _do_ happen: when a source object implements both `INotifyDataErrorInfo` and had data annotations, then the nested data validation plugins would each wrap the value coming from the previous plugin in a new `BindingNotification`, resulting in nested `BindingNotifications`.

This adds support for nested binding notifications back in - even though IMO nesting binding notifications is a bug, if we're doing it and we previously supported it then we should continue to support it.

Fixes #15201
grokys added a commit that referenced this issue Jun 3, 2024
* Added failing test for #15201.

* Handle nested BindingNotifications.

When #13970 was written, [a check](https://github.com/AvaloniaUI/Avalonia/pull/13970/files#diff-cfb25a491b9452e1815aa2c0d71465aaf81e99792a88a04a1a2ed572fd1930ffR60) was added to ensure that nested `BindingNotification`s didn't happen, and the refactor was written with the assumption that they wouldn't happen.

The problem is that they _do_ happen: when a source object implements both `INotifyDataErrorInfo` and had data annotations, then the nested data validation plugins would each wrap the value coming from the previous plugin in a new `BindingNotification`, resulting in nested `BindingNotifications`.

This adds support for nested binding notifications back in - even though IMO nesting binding notifications is a bug, if we're doing it and we previously supported it then we should continue to support it.

Fixes #15201
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants