diff --git a/src/Framework/Framework/Compilation/Validation/ControlUsageError.cs b/src/Framework/Framework/Compilation/Validation/ControlUsageError.cs index f4284de76a..0f6868fb3c 100644 --- a/src/Framework/Framework/Compilation/Validation/ControlUsageError.cs +++ b/src/Framework/Framework/Compilation/Validation/ControlUsageError.cs @@ -1,18 +1,37 @@ using System.Collections.Generic; using System.Linq; +using DotVVM.Framework.Binding.Properties; using DotVVM.Framework.Compilation.Parser.Dothtml.Parser; namespace DotVVM.Framework.Compilation.Validation { + /// Represents an error or a warning reported by a control usage validation method. + /// public class ControlUsageError { public string ErrorMessage { get; } + /// The error will be shown on the these syntax nodes. When empty, the start tag is underlined. public DothtmlNode[] Nodes { get; } - public ControlUsageError(string message, IEnumerable nodes) + /// Error - the page compilation will fail. Warning - the user will only be notified about the reported problem (in the log, for example). Other severities are currently not shown at all. + public DiagnosticSeverity Severity { get; } = DiagnosticSeverity.Error; + public ControlUsageError(string message, DiagnosticSeverity severity, IEnumerable nodes) { ErrorMessage = message; Nodes = nodes.Where(n => n != null).ToArray()!; + Severity = severity; + } + public ControlUsageError(string message, IEnumerable nodes) : this(message, DiagnosticSeverity.Error, nodes) { } + public ControlUsageError(string message, params DothtmlNode?[] nodes) : this(message, DiagnosticSeverity.Error, nodes.AsEnumerable()) { } + public ControlUsageError(string message, DiagnosticSeverity severity, params DothtmlNode?[] nodes) : this(message, severity, nodes.AsEnumerable()) { } + + public override string ToString() + { + var core = $"{Severity} {ErrorMessage}"; + var someToken = Nodes.SelectMany(n => n.Tokens).FirstOrDefault(); + if (someToken == null) + return core; + else + return $"{core} (at {someToken.LineNumber}:{someToken.ColumnNumber})"; } - public ControlUsageError(string message, params DothtmlNode?[] nodes) : this(message, nodes.AsEnumerable()) { } } } diff --git a/src/Framework/Framework/Compilation/Validation/ControlUsageValidationVisitor.cs b/src/Framework/Framework/Compilation/Validation/ControlUsageValidationVisitor.cs index abc207548b..20d9d476ef 100644 --- a/src/Framework/Framework/Compilation/Validation/ControlUsageValidationVisitor.cs +++ b/src/Framework/Framework/Compilation/Validation/ControlUsageValidationVisitor.cs @@ -1,9 +1,8 @@ using System; using System.Collections.Generic; using System.Linq; +using DotVVM.Framework.Binding.Properties; using DotVVM.Framework.Compilation.ControlTree.Resolved; -using DotVVM.Framework.Configuration; -using Microsoft.Extensions.DependencyInjection; namespace DotVVM.Framework.Compilation.Validation { @@ -23,7 +22,19 @@ public override void VisitControl(ResolvedControl control) { Errors.Add((control, e)); foreach (var node in e.Nodes) - node.AddError(e.ErrorMessage); + { + switch (e.Severity) + { + case DiagnosticSeverity.Error: + node.AddError(e.ErrorMessage); + break; + case DiagnosticSeverity.Warning: + node.AddWarning(e.ErrorMessage); + break; + default: + break; + } + } } base.VisitControl(control); } @@ -33,9 +44,8 @@ public void VisitAndAssert(ResolvedTreeRoot view) { if (this.Errors.Any()) throw new Exception("The ControlUsageValidationVisitor has already collected some errors."); VisitView(view); - if (this.Errors.Any()) + if (this.Errors.FirstOrDefault(e => e.err.Severity == DiagnosticSeverity.Error) is { err: {} } controlUsageError) { - var controlUsageError = this.Errors.First(); var lineNumber = controlUsageError.control.GetAncestors() .Select(c => c.DothtmlNode) diff --git a/src/Framework/Framework/Compilation/Validation/ControlUsageValidatorAttribute.cs b/src/Framework/Framework/Compilation/Validation/ControlUsageValidatorAttribute.cs index 6f418f302d..0d3482731a 100644 --- a/src/Framework/Framework/Compilation/Validation/ControlUsageValidatorAttribute.cs +++ b/src/Framework/Framework/Compilation/Validation/ControlUsageValidatorAttribute.cs @@ -2,9 +2,24 @@ namespace DotVVM.Framework.Compilation.Validation { + /// + /// Call this static method for each compiled control of a matching type. + /// The method should have the signature: + /// Validator(ResolvedControl control) + /// ]]>. + /// Optionally, an DotvvmConfiguration parameter may be present on the method. + /// [AttributeUsage(AttributeTargets.Method)] public class ControlUsageValidatorAttribute: Attribute { + /// Ignore all validators from the base controls. public bool Override { get; set; } + /// + /// Call this method even on other controls when a property from the declaring class is used. + /// The method will be called once per control. + /// Properties on derived nor base classes do not trigger the validator. + /// + public bool IncludeAttachedProperties { get; set; } } } diff --git a/src/Framework/Framework/Compilation/Validation/DefaultControlUsageValidator.cs b/src/Framework/Framework/Compilation/Validation/DefaultControlUsageValidator.cs index bc39b884eb..679cb686a0 100644 --- a/src/Framework/Framework/Compilation/Validation/DefaultControlUsageValidator.cs +++ b/src/Framework/Framework/Compilation/Validation/DefaultControlUsageValidator.cs @@ -4,10 +4,12 @@ using System.Diagnostics; using System.Linq; using System.Reflection; +using DotVVM.Framework.Binding; using DotVVM.Framework.Compilation.ControlTree; using DotVVM.Framework.Compilation.ControlTree.Resolved; using DotVVM.Framework.Configuration; using DotVVM.Framework.Controls; +using DotVVM.Framework.Utils; namespace DotVVM.Framework.Compilation.Validation { @@ -18,7 +20,7 @@ public DefaultControlUsageValidator(DotvvmConfiguration config) Configuration = config; } - public static ConcurrentDictionary cache = new ConcurrentDictionary(); + public static ConcurrentDictionary cache = new(); protected DotvvmConfiguration Configuration { get; } @@ -44,6 +46,48 @@ public static IEnumerable ValidateDefaultRules(IAbstractContr } } + private IEnumerable CallMethod(MethodInfo method, IAbstractControl control) + { + var par = method.GetParameters(); + var args = new object[par.Length]; + for (int i = 0; i < par.Length; i++) + { + if (par[i].ParameterType.IsAssignableFrom(control.GetType())) + { + args[i] = control; + } + else if (control.DothtmlNode != null && par[i].ParameterType.IsAssignableFrom(control.DothtmlNode.GetType())) + { + args[i] = control.DothtmlNode; + } + else if (par[i].ParameterType == typeof(DotvvmConfiguration)) + { + args[i] = Configuration; + } + else + { + return Enumerable.Empty(); + } + } + var r = method.Invoke(null, args); + if (r is null) + { + return Enumerable.Empty(); + } + else if (r is IEnumerable errors) + { + return errors; + } + else if (r is IEnumerable stringErrors) + { + return stringErrors.Select(e => new ControlUsageError(e)); + } + else + { + throw new Exception($"ControlUsageValidator method '{ReflectionUtils.FormatMethodInfo(method)}' returned an invalid type. Expected IEnumerable or IEnumerable."); + } + } + public IEnumerable Validate(IAbstractControl control) { var type = GetControlType(control.Metadata); @@ -55,65 +99,59 @@ public IEnumerable Validate(IAbstractControl control) return result; var methods = cache.GetOrAdd(type, FindMethods); - foreach (var method in methods) + foreach (var method in methods.controlValidators) { - var par = method.GetParameters(); - var args = new object[par.Length]; - for (int i = 0; i < par.Length; i++) - { - if (par[i].ParameterType.IsAssignableFrom(control.GetType())) - { - args[i] = control; - } - else if (control.DothtmlNode != null && par[i].ParameterType.IsAssignableFrom(control.DothtmlNode.GetType())) - { - args[i] = control.DothtmlNode; - } - else if (par[i].ParameterType == typeof(DotvvmConfiguration)) - { - args[i] = Configuration; - } - else - { - goto Error; // I think it is better that throw exceptions and catch them - } - } - var r = method.Invoke(null, args); - if (r is IEnumerable errors) - { - result.AddRange(errors); - } - else if (r is IEnumerable stringErrors) + result.AddRange(CallMethod(method, control)); + } + + var attachedPropertiesTypes = new HashSet(); + foreach (var attachedProperty in control.PropertyNames) + { + if (attachedProperty.DeclaringType.IsAssignableFrom(control.Metadata.Type)) + continue; // not an attached property + if (GetPropertyDeclaringType(attachedProperty) is {} declaringType) + attachedPropertiesTypes.Add(declaringType); + } + + foreach (var attachedPropertyType in attachedPropertiesTypes) + { + var (_, attachedValidators) = cache.GetOrAdd(attachedPropertyType, FindMethods); + foreach (var method in attachedValidators) { - result.AddRange(stringErrors.Select(e => new ControlUsageError(e))); + result.AddRange(CallMethod(method, control)); } - continue; - Error:; } return result // add current node to the error, if no control is specified .Select(e => e.Nodes.Length == 0 ? - new ControlUsageError(e.ErrorMessage, control.DothtmlNode) : + new ControlUsageError(e.ErrorMessage, e.Severity, control.DothtmlNode) : e); } - protected virtual MethodInfo[] FindMethods(Type type) + protected virtual (MethodInfo[] controlValidators, MethodInfo[] attachedPropertyValidators) FindMethods(Type type) { - if (type == typeof(object)) return new MethodInfo[0]; + if (type == typeof(object)) + return (Array.Empty(), Array.Empty()); + var methods = type.GetMethods(BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.Static) .Where(m => m.IsDefined(typeof(ControlUsageValidatorAttribute))) .ToArray(); - var attributes = methods.Select(s => s.GetCustomAttribute(typeof(ControlUsageValidatorAttribute))).ToList(); - var overrideValidation = attributes.OfType().Select(s => s.Override).Distinct().ToList(); + var attributes = methods.Select(method => (method, attr: method.GetCustomAttribute().NotNull())).ToList(); + var overrideValidation = attributes.Select(s => s.attr.Override).Distinct().ToList(); if (overrideValidation.Count > 1) throw new Exception($"ControlUsageValidator attributes on '{type.FullName}' are in an inconsistent state. Make sure all attributes have an Override property set to the same value."); - if (overrideValidation.Any() && overrideValidation[0]) return methods; + var attachedValidators = attributes.Where(s => s.attr.IncludeAttachedProperties).Select(m => m.method).ToArray(); + + if (overrideValidation.Any() && overrideValidation[0]) + return (methods, attachedValidators); + var ancestorMethods = FindMethods(type.BaseType!); - return ancestorMethods.Concat(methods).ToArray(); + // attached validators are not inherited + return (ancestorMethods.controlValidators.Concat(methods).ToArray(), attachedValidators); } protected virtual Type? GetControlType(IControlResolverMetadata metadata) @@ -122,6 +160,13 @@ protected virtual MethodInfo[] FindMethods(Type type) return type?.Type; } + protected virtual Type? GetPropertyDeclaringType(IPropertyDescriptor property) + { + if (property is DotvvmProperty p) + return p.DeclaringType; + return (property.DeclaringType as ResolvedTypeDescriptor)?.Type; + } + /// Clear cache when hot reload happens internal static void ClearCaches(Type[] types) { diff --git a/src/Framework/Framework/Controls/Validation.cs b/src/Framework/Framework/Controls/Validation.cs index 7e65f30fd1..a8971683e2 100644 --- a/src/Framework/Framework/Controls/Validation.cs +++ b/src/Framework/Framework/Controls/Validation.cs @@ -1,4 +1,13 @@ +using System.Collections; +using System.Collections.Generic; +using System.Linq.Expressions; using DotVVM.Framework.Binding; +using DotVVM.Framework.Binding.Expressions; +using DotVVM.Framework.Binding.Properties; +using DotVVM.Framework.Compilation.ControlTree; +using DotVVM.Framework.Compilation.ControlTree.Resolved; +using DotVVM.Framework.Compilation.Parser.Dothtml.Parser; +using DotVVM.Framework.Compilation.Validation; namespace DotVVM.Framework.Controls { @@ -12,5 +21,24 @@ public class Validation [AttachedProperty(typeof(object))] [MarkupOptions(AllowHardCodedValue = false)] public static DotvvmProperty TargetProperty = DotvvmProperty.Register(() => TargetProperty, null, true); + + + [ControlUsageValidator(IncludeAttachedProperties = true)] + public static IEnumerable ValidateUsage(ResolvedControl control) + { + if (control.GetValue(Validation.TargetProperty) is ResolvedPropertyBinding { Binding: {} targetBinding }) + { + if (targetBinding.ResultType.IsPrimitiveTypeDescriptor() && + targetBinding.Expression is not ConstantExpression // Allow Target={value: 0}, it's an OK hack to supress automatic validation + ) + { + yield return new ControlUsageError( + $"Validation.Target should be bound to a complex object instead of '{targetBinding.ResultType!.CSharpName}'. Validation attributes on the specified property are ignored, only the rules inside the target object are validated.", + DiagnosticSeverity.Warning, + (targetBinding.DothtmlNode as DothtmlBindingNode)?.ValueNode ?? targetBinding.DothtmlNode + ); + } + } + } } } diff --git a/src/Samples/Common/ViewModels/FeatureSamples/Validation/ValidationTargetIsCollectionViewModel.cs b/src/Samples/Common/ViewModels/FeatureSamples/Validation/ValidationTargetIsCollectionViewModel.cs index 86d30f2928..25e8a37152 100644 --- a/src/Samples/Common/ViewModels/FeatureSamples/Validation/ValidationTargetIsCollectionViewModel.cs +++ b/src/Samples/Common/ViewModels/FeatureSamples/Validation/ValidationTargetIsCollectionViewModel.cs @@ -1,4 +1,5 @@ -using System.Collections.Generic; +using System; +using System.Collections.Generic; using System.ComponentModel.DataAnnotations; using DotVVM.Framework.ViewModel; @@ -8,6 +9,8 @@ public class ValidationTargetIsCollectionViewModel : DotvvmViewModelBase { public List Customers { get; set; } + public DateTime Something { get; set; } = DateTime.Now; + public ValidationTargetIsCollectionViewModel() { Customers = new List() diff --git a/src/Tests/Runtime/ControlTree/DefaultControlTreeResolver/CompilationWarningsTests.cs b/src/Tests/Runtime/ControlTree/DefaultControlTreeResolver/CompilationWarningsTests.cs new file mode 100644 index 0000000000..cb512ab84e --- /dev/null +++ b/src/Tests/Runtime/ControlTree/DefaultControlTreeResolver/CompilationWarningsTests.cs @@ -0,0 +1,105 @@ +using System; +using System.Collections.Generic; +using System.Linq; +using DotVVM.Framework.Utils; +using DotVVM.Framework.Controls; +using DotVVM.Framework.Compilation.Parser.Dothtml.Parser; +using Microsoft.VisualStudio.TestTools.UnitTesting; + + +namespace DotVVM.Framework.Tests.Runtime.ControlTree +{ + [TestClass] + public class CompilationWarningsTests : DefaultControlTreeResolverTestsBase + { + public (string warning, string tokens)[] GetWarnings(string dothtml) + { + var tree = ParseSource(dothtml, checkErrors: false); + return tree.DothtmlNode + .EnumerateNodes() + .SelectMany(n => n.NodeWarnings.Select(w => (w, string.Join(" ", n.Tokens.Select(t => t.Text))))) + .ToArray(); + } + + [TestMethod] + public void CompilationWarning_ValidationTargetPrimitiveType() + { + var warnings = GetWarnings($$""" + @viewModel {{typeof(TestViewModel)}} +
+ """); + Assert.AreEqual(1, warnings.Length); + StringAssert.Contains(warnings[0].warning, "Validation.Target should be bound to a complex object instead of 'bool'"); + Assert.AreEqual("BoolProp", warnings[0].tokens.Trim()); + } + + [DataTestMethod] + [DataRow("TestViewModel2")] + [DataRow("VMArray")] + [DataRow("0")] + public void CompilationWarning_ValidationTargetPrimitiveType_Negative(string property) + { + var warnings = GetWarnings($$""" + @viewModel {{typeof(TestViewModel)}} +
+ """); + XAssert.Empty(warnings); + } + + [TestMethod] + public void DefaultViewCompiler_NonExistenPropertyWarning() + { + var markup = $@" +@viewModel System.Boolean + +"; + var button = ParseSource(markup) + .Content.SelectRecursively(c => c.Content) + .Single(c => c.Metadata.Type == typeof(Button)); + + var elementNode = (DothtmlElementNode)button.DothtmlNode; + var attribute1 = elementNode.Attributes.Single(a => a.AttributeName == "TestProperty"); + var attribute2 = elementNode.Attributes.Single(a => a.AttributeName == "normal-attribute"); + var attribute3 = elementNode.Attributes.Single(a => a.AttributeName == "Visble"); + + Assert.AreEqual(0, attribute2.AttributeNameNode.NodeWarnings.Count(), attribute2.AttributeNameNode.NodeWarnings.StringJoin(", ")); + Assert.AreEqual("HTML attribute name 'TestProperty' should not contain uppercase letters. Did you intent to use a DotVVM property instead?", attribute1.AttributeNameNode.NodeWarnings.Single()); + Assert.AreEqual("HTML attribute name 'Visble' should not contain uppercase letters. Did you mean Visible, or another DotVVM property?", attribute3.AttributeNameNode.NodeWarnings.Single()); + } + + [TestMethod] + public void DefaultViewCompiler_NonExistenPropertyWarning_PrefixedGroup() + { + var markup = $@" +@viewModel System.Boolean + +"; + var repeater = ParseSource(markup) + .Content.SelectRecursively(c => c.Content) + .Single(c => c.Metadata.Type == typeof(HierarchyRepeater)); + + var elementNode = (DothtmlElementNode)repeater.DothtmlNode; + var attribute1 = elementNode.Attributes.Single(a => a.AttributeName == "ItemClass"); + var attribute2 = elementNode.Attributes.Single(a => a.AttributeName == "ItemIncludeInPage"); + + Assert.AreEqual(0, attribute1.AttributeNameNode.NodeWarnings.Count(), attribute1.AttributeNameNode.NodeWarnings.StringJoin(", ")); + Assert.AreEqual("HTML attribute name 'IncludeInPage' should not contain uppercase letters. Did you intent to use a DotVVM property instead?", XAssert.Single(attribute2.AttributeNameNode.NodeWarnings)); + } + + [TestMethod] + public void DefaultViewCompiler_UnsupportedCallSite_ResourceBinding_Warning() + { + var markup = @" +@viewModel System.DateTime +{{resource: _this.ToBrowserLocalTime()}} +"; + var literal = ParseSource(markup) + .Content.SelectRecursively(c => c.Content) + .Single(c => c.Metadata.Type == typeof(Literal)); + + Assert.AreEqual(1, literal.DothtmlNode.NodeWarnings.Count()); + Assert.AreEqual("Evaluation of method \"ToBrowserLocalTime\" on server-side may yield unexpected results.", literal.DothtmlNode.NodeWarnings.First()); + } + } + +} diff --git a/src/Tests/Runtime/ControlTree/DefaultControlTreeResolver/DefaultControlTreeResolverTests.cs b/src/Tests/Runtime/ControlTree/DefaultControlTreeResolver/DefaultControlTreeResolverTests.cs index ad25db1a8c..b24cf51373 100644 --- a/src/Tests/Runtime/ControlTree/DefaultControlTreeResolver/DefaultControlTreeResolverTests.cs +++ b/src/Tests/Runtime/ControlTree/DefaultControlTreeResolver/DefaultControlTreeResolverTests.cs @@ -746,61 +746,6 @@ @viewModel System.Boolean Assert.IsFalse(control3[2].DothtmlNode.HasNodeErrors); } - [TestMethod] - public void DefaultViewCompiler_NonExistenPropertyWarning() - { - var markup = $@" -@viewModel System.Boolean - -"; - var button = ParseSource(markup) - .Content.SelectRecursively(c => c.Content) - .Single(c => c.Metadata.Type == typeof(Button)); - - var elementNode = (DothtmlElementNode)button.DothtmlNode; - var attribute1 = elementNode.Attributes.Single(a => a.AttributeName == "TestProperty"); - var attribute2 = elementNode.Attributes.Single(a => a.AttributeName == "normal-attribute"); - var attribute3 = elementNode.Attributes.Single(a => a.AttributeName == "Visble"); - - Assert.AreEqual(0, attribute2.AttributeNameNode.NodeWarnings.Count(), attribute2.AttributeNameNode.NodeWarnings.StringJoin(", ")); - Assert.AreEqual("HTML attribute name 'TestProperty' should not contain uppercase letters. Did you intent to use a DotVVM property instead?", attribute1.AttributeNameNode.NodeWarnings.Single()); - Assert.AreEqual("HTML attribute name 'Visble' should not contain uppercase letters. Did you mean Visible, or another DotVVM property?", attribute3.AttributeNameNode.NodeWarnings.Single()); - } - - [TestMethod] - public void DefaultViewCompiler_NonExistenPropertyWarning_PrefixedGroup() - { - var markup = $@" -@viewModel System.Boolean - -"; - var repeater = ParseSource(markup) - .Content.SelectRecursively(c => c.Content) - .Single(c => c.Metadata.Type == typeof(HierarchyRepeater)); - - var elementNode = (DothtmlElementNode)repeater.DothtmlNode; - var attribute1 = elementNode.Attributes.Single(a => a.AttributeName == "ItemClass"); - var attribute2 = elementNode.Attributes.Single(a => a.AttributeName == "ItemIncludeInPage"); - - Assert.AreEqual(0, attribute1.AttributeNameNode.NodeWarnings.Count(), attribute1.AttributeNameNode.NodeWarnings.StringJoin(", ")); - Assert.AreEqual("HTML attribute name 'IncludeInPage' should not contain uppercase letters. Did you intent to use a DotVVM property instead?", XAssert.Single(attribute2.AttributeNameNode.NodeWarnings)); - } - - [TestMethod] - public void DefaultViewCompiler_UnsupportedCallSite_ResourceBinding_Warning() - { - var markup = @" -@viewModel System.DateTime -{{resource: _this.ToBrowserLocalTime()}} -"; - var literal = ParseSource(markup) - .Content.SelectRecursively(c => c.Content) - .Single(c => c.Metadata.Type == typeof(Literal)); - - Assert.AreEqual(1, literal.DothtmlNode.NodeWarnings.Count()); - Assert.AreEqual("Evaluation of method \"ToBrowserLocalTime\" on server-side may yield unexpected results.", literal.DothtmlNode.NodeWarnings.First()); - } - [TestMethod] public void DefaultViewCompiler_DifferentControlPrimaryName() {