From c94f5bc771ca7307c094b65bb4f7d9de4a2c2ccd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Standa=20Luke=C5=A1?= Date: Fri, 20 Sep 2024 12:38:43 +0200 Subject: [PATCH] capabilities: avoid NRE when ValueOrBinding? is set to null Nullable ValueOrBinding, for example ValueOrBinding? semantially means that the property is not set. If the property is set to null, we would like to return null inside of the ValueOrBinding. However, that is not possible if T is non-nullable value type, so either have to throw, or return an "outer" null. This patch changes the behavior to return null instead of failing. Users are unlikely to care about "not set" and "set to null", and throwing the exception creates unnecessary friction. --- .../DotvvmCapabilityProperty.Helpers.cs | 19 ++++++- .../Framework/Controls/CompositeControl.cs | 3 +- src/Tests/Runtime/CapabilityPropertyTests.cs | 52 +++++++++++++++++++ 3 files changed, 71 insertions(+), 3 deletions(-) diff --git a/src/Framework/Framework/Binding/DotvvmCapabilityProperty.Helpers.cs b/src/Framework/Framework/Binding/DotvvmCapabilityProperty.Helpers.cs index 12b4253588..61985c9fa3 100644 --- a/src/Framework/Framework/Binding/DotvvmCapabilityProperty.Helpers.cs +++ b/src/Framework/Framework/Binding/DotvvmCapabilityProperty.Helpers.cs @@ -15,7 +15,14 @@ internal static class Helpers public static ValueOrBinding? GetOptionalValueOrBinding(DotvvmBindableObject c, DotvvmProperty p) { if (c.properties.TryGet(p, out var x)) - return ValueOrBinding.FromBoxedValue(x); + { + // we want to return ValueOrBinding(null) -- "property set to null" + // but if that isn't possible, it's better to return null ("property missing") than crash + if (x is null && default(T) != null) + return null; + else + return ValueOrBinding.FromBoxedValue(x); + } else return null; } public static ValueOrBinding GetValueOrBinding(DotvvmBindableObject c, DotvvmProperty p) @@ -27,7 +34,15 @@ public static ValueOrBinding GetValueOrBinding(DotvvmBindableObject c, Dot public static ValueOrBinding? GetOptionalValueOrBindingSlow(DotvvmBindableObject c, DotvvmProperty p) { if (c.IsPropertySet(p)) - return ValueOrBinding.FromBoxedValue(c.GetValue(p)); + { + var x = c.GetValue(p); + // we want to return ValueOrBinding(null) -- "property set to null" + // but if that isn't possible, it's better to return null ("property missing") than crash + if (x is null && default(T) != null) + return null; + else + return ValueOrBinding.FromBoxedValue(x); + } else return null; } public static ValueOrBinding GetValueOrBindingSlow(DotvvmBindableObject c, DotvvmProperty p) diff --git a/src/Framework/Framework/Controls/CompositeControl.cs b/src/Framework/Framework/Controls/CompositeControl.cs index cf8b774555..d0dd7252f9 100644 --- a/src/Framework/Framework/Controls/CompositeControl.cs +++ b/src/Framework/Framework/Controls/CompositeControl.cs @@ -126,7 +126,8 @@ Func compileGetter(IControlAttr } static internal ControlInfo GetControlInfo(Type controlType) => - controlInfoCache[controlType]; + controlInfoCache.TryGetValue(controlType, out var result) ? result : + throw new Exception($"CompositeControl {controlType.FullName} wasn't initialized and GetContents cannot be executed. This is probably caused by a failure during property registration."); internal IEnumerable ExecuteGetContents(IDotvvmRequestContext context) { diff --git a/src/Tests/Runtime/CapabilityPropertyTests.cs b/src/Tests/Runtime/CapabilityPropertyTests.cs index 485826e115..801ae39389 100644 --- a/src/Tests/Runtime/CapabilityPropertyTests.cs +++ b/src/Tests/Runtime/CapabilityPropertyTests.cs @@ -215,6 +215,33 @@ public void BitMoreComplexCapability_WeirdProperties_GetterAndSetter() } + [DataTestMethod] + [DataRow(typeof(TestControl6))] + [DataRow(typeof(TestControlFallbackProps))] + [DataRow(typeof(TestControlInheritedProps))] + public void BitMoreComplexCapability_NullableValue(Type controlType) + { + var control1 = (DotvvmBindableObject)Activator.CreateInstance(controlType); + var capProp = DotvvmCapabilityProperty.Find(controlType, typeof(BitMoreComplexCapability)); + + Assert.IsFalse(control1.GetCapability().Nullable.HasValue); + + control1.SetProperty("Nullable", null); + XAssert.Single(control1.Properties); + Assert.IsFalse(control1.GetCapability().Nullable.HasValue); + + control1.SetProperty("Nullable", 1); + Assert.AreEqual(1, control1.GetCapability().Nullable); + + control1.SetValue(capProp, new BitMoreComplexCapability { Nullable = null }); + Assert.AreEqual(null, control1.GetCapability().Nullable); + Assert.IsTrue(control1.IsPropertySet(control1.GetDotvvmProperty("Nullable"))); + + control1.SetValue(capProp, new BitMoreComplexCapability { Nullable = 2 }); + Assert.AreEqual(2, control1.GetCapability().Nullable); + } + + public class TestControl1: HtmlGenericControl, IObjectWithCapability, @@ -301,6 +328,7 @@ public int ValueOrBindingNullable2 } public static readonly DotvvmProperty ValueOrBindingNullable2Property = DotvvmProperty.Register(nameof(ValueOrBindingNullable2), defaultValue: 10); + [PropertyAlias("ValueOrBindingNullable2")] public static readonly DotvvmProperty ValueOrBindingNullableProperty = DotvvmPropertyAlias.RegisterAlias("ValueOrBindingNullable"); @@ -309,6 +337,30 @@ public int ValueOrBindingNullable2 DotvvmCapabilityProperty.RegisterCapability(); } + public class TestControlInheritedProps: + HtmlGenericControl, + IObjectWithCapability + { + public int NotNullable + { + get { return (int)GetValue(NotNullableProperty); } + set { SetValue(NotNullableProperty, value); } + } + public static readonly DotvvmProperty NotNullableProperty = + DotvvmProperty.Register(nameof(NotNullable), isValueInherited: true); + + public int? Nullable + { + get { return (int?)GetValue(NullableProperty); } + set { SetValue(NullableProperty, value); } + } + public static readonly DotvvmProperty NullableProperty = + DotvvmProperty.Register(nameof(Nullable), isValueInherited: true); + + public static readonly DotvvmCapabilityProperty BitMoreComplexCapabilityProperty = + DotvvmCapabilityProperty.RegisterCapability(); + } + [DotvvmControlCapability] public sealed record TestCapability {