From f21c2cf74aed3ceecb31cf341214fe4b748e1473 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Standa=20Luke=C5=A1?= <exyi@outlook.cz>
Date: Wed, 18 Oct 2023 20:16:14 +0200
Subject: [PATCH] CompositeControl: better handling of ambiguous GetContents,
 test for inheritance

According to  #1613 CompositeControl do not work with
both base and derived types defining GetContents method.
I could not replicate that, the provided example works for me,
maybe it was fixed accidentaly.

* I added test for that
* In any case, multiple GetContents overloads were handled poorly,
   I added better error for this case.

In the issue, I mentioned that it would be complex to handle
potential redefinition or "removal" of properties in the derived type.
That isn't really the case, both cases don't need any special code
* The "removal" - omission of an property in the derived type is simply ignored.
   I think that it's quite intuitive that properties can't disapear in
   derived types, if the GetContents doesn't ask for it it won't get it.
* Redefinition is already handled reasonably, since we can already
   try to redefine IncludeInPage or some capability property.

Resolves #1613
---
 .../Framework/Controls/CompositeControl.cs    | 13 +++++-
 .../ControlTests/CompositeControlTests.cs     | 44 +++++++++++++++++++
 ...trolTests.CompositeControlInheritance.html |  7 +++
 3 files changed, 63 insertions(+), 1 deletion(-)
 create mode 100644 src/Tests/ControlTests/testoutputs/CompositeControlTests.CompositeControlInheritance.html

diff --git a/src/Framework/Framework/Controls/CompositeControl.cs b/src/Framework/Framework/Controls/CompositeControl.cs
index fd8e7664df..cf8b774555 100644
--- a/src/Framework/Framework/Controls/CompositeControl.cs
+++ b/src/Framework/Framework/Controls/CompositeControl.cs
@@ -85,7 +85,18 @@ Func<IDotvvmRequestContext, CompositeControl, object> compileGetter(IControlAttr
 
                 DefaultControlResolver.InitType(controlType.BaseType.NotNull());
 
-                var method = controlType.GetMethod("GetContents", BindingFlags.Static | BindingFlags.Instance | BindingFlags.Public | BindingFlags.NonPublic);
+                MethodInfo? method;
+                try
+                {
+                    method = controlType.GetMethod("GetContents", BindingFlags.Static | BindingFlags.Instance | BindingFlags.Public | BindingFlags.NonPublic);
+                }
+                catch (AmbiguousMatchException ex)
+                {
+                    var allMethods = controlType.GetMethods(BindingFlags.Static | BindingFlags.Instance | BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.FlattenHierarchy).Where(m => m.Name == "GetContents").ToArray();
+
+                    var message = $"Could not initialize control {controlType.FullName}, multiple GetContents methods are present: {allMethods.Take(3).Select(m => ReflectionUtils.FormatMethodInfo(m, stripNamespace: true)).StringJoin(", ")}{(allMethods.Length > 3 ? ", ..." : "")}";
+                    throw new Exception(message, ex);
+                }
                 if (method == null)
                     throw new Exception($"Could not initialize control {controlType.FullName}, could not find (single) GetContents method");
                 if (!(typeof(DotvvmControl).IsAssignableFrom(method.ReturnType) || typeof(IEnumerable<DotvvmControl>).IsAssignableFrom(method.ReturnType)))
diff --git a/src/Tests/ControlTests/CompositeControlTests.cs b/src/Tests/ControlTests/CompositeControlTests.cs
index 96b281bbe1..d963d7a96b 100644
--- a/src/Tests/ControlTests/CompositeControlTests.cs
+++ b/src/Tests/ControlTests/CompositeControlTests.cs
@@ -17,6 +17,7 @@
 using DotVVM.Framework.ViewModel;
 using Microsoft.VisualStudio.TestTools.UnitTesting;
 using DotVVM.Framework.Hosting;
+using System.ComponentModel;
 
 namespace DotVVM.Framework.Tests.ControlTests
 {
@@ -322,6 +323,18 @@ public async Task MenuRepeater()
             check.CheckString(r.FormattedHtml, fileExtension: "html");
         }
 
+        [TestMethod]
+        public async Task CompositeControlInheritance()
+        {
+            var r = await cth.RunPage(typeof(BasicTestViewModel), """
+                <cc:TestBaseCompositeControl Text="Text" Number=1 />
+                <cc:TestDerivedCompositeControl Text="SecondText" AnotherNumber=1.1 />
+                """
+            );
+
+            check.CheckString(r.FormattedHtml, fileExtension: "html");
+        }
+
         public class BasicTestViewModel: DotvvmViewModelBase
         {
             [Bind(Name = "int")]
@@ -689,4 +702,35 @@ public static DotvvmControl GetContents(ValueOrBinding<TestStatusEnum> testStatu
             return icon;
         }
     }
+
+    public class TestBaseCompositeControl: CompositeControl
+    {
+        public static DotvvmControl GetContents(
+            ValueOrBinding<string> text,
+            [DefaultValue(1)]
+            ValueOrBinding<int> number
+        )
+        {
+            return new HtmlGenericControl("div") {
+                Children = {
+                    new Literal(text), new Literal(number)
+                }
+            };
+        }
+    }
+
+    public class TestDerivedCompositeControl: TestBaseCompositeControl
+    {
+        public static DotvvmControl GetContents(
+            ValueOrBinding<string> text,
+            ValueOrBinding<double> anotherNumber // declare another property, type of existing property cannot be changed
+        )
+        {
+            return new HtmlGenericControl("p") {
+                Children = {
+                    new Literal(text), new Literal(anotherNumber)
+                }
+            };
+        }
+    }
 }
diff --git a/src/Tests/ControlTests/testoutputs/CompositeControlTests.CompositeControlInheritance.html b/src/Tests/ControlTests/testoutputs/CompositeControlTests.CompositeControlInheritance.html
new file mode 100644
index 0000000000..7ae651e073
--- /dev/null
+++ b/src/Tests/ControlTests/testoutputs/CompositeControlTests.CompositeControlInheritance.html
@@ -0,0 +1,7 @@
+<html>
+	<head></head>
+	<body>
+		<div>Text1</div>
+		<p>SecondText1.1</p>
+	</body>
+</html>