From f9a9b8068161ec6bb131a180cbb8406c693c1b03 Mon Sep 17 00:00:00 2001 From: Mugdha Kulkarni Date: Fri, 13 Feb 2015 17:15:30 -0800 Subject: [PATCH] Fixing the error message. The error message for malformed template was too complex listing all the errors that can happen in one message. I have separated the message in 2 different messages. 1. When there is a parameter or a literal following the optional parameter. 2. when optional parameter is preceded by a literal other than period which is not allowed. --- .../Properties/Resources.Designer.cs | 48 +++++++++++------ src/Microsoft.AspNet.Routing/Resources.resx | 9 ++-- .../Template/TemplateParser.cs | 32 +++++++++-- .../Template/TemplateParserTests.cs | 53 ++++++++++--------- 4 files changed, 93 insertions(+), 49 deletions(-) diff --git a/src/Microsoft.AspNet.Routing/Properties/Resources.Designer.cs b/src/Microsoft.AspNet.Routing/Properties/Resources.Designer.cs index f1a4eaf192..8da7900269 100644 --- a/src/Microsoft.AspNet.Routing/Properties/Resources.Designer.cs +++ b/src/Microsoft.AspNet.Routing/Properties/Resources.Designer.cs @@ -202,22 +202,6 @@ namespace Microsoft.AspNet.Routing return GetString("TemplateRoute_CannotHaveConsecutiveSeparators"); } - /// - /// In a path segment that contains more than one section, such as a literal section or a parameter, there can only be one optional parameter. The optional parameter must be the last parameter in the segment and must be preceded by one single period (.). - /// - internal static string TemplateRoute_CanHaveOnlyLastParameterOptional_IfFollowingOptionalSeperator - { - get { return GetString("TemplateRoute_CanHaveOnlyLastParameterOptional_IfFollowingOptionalSeperator"); } - } - - /// - /// In a path segment that contains more than one section, such as a literal section or a parameter, there can only be one optional parameter. The optional parameter must be the last parameter in the segment and must be preceded by one single period (.). - /// - internal static string FormatTemplateRoute_CanHaveOnlyLastParameterOptional_IfFollowingOptionalSeperator() - { - return GetString("TemplateRoute_CanHaveOnlyLastParameterOptional_IfFollowingOptionalSeperator"); - } - /// /// A catch-all parameter cannot be marked optional. /// @@ -394,6 +378,38 @@ namespace Microsoft.AspNet.Routing return GetString("TemplateRoute_UnescapedBrace"); } + /// + /// In the segment '{0}', the optional parameter '{1}' is preceded by an invalid segment '{2}'. Only a period (.) can precede an optional parameter. + /// + internal static string TemplateRoute_OptionalParameterCanbBePrecededByPeriod + { + get { return GetString("TemplateRoute_OptionalParameterCanbBePrecededByPeriod"); } + } + + /// + /// In the segment '{0}', the optional parameter '{1}' is preceded by an invalid segment '{2}'. Only a period (.) can precede an optional parameter. + /// + internal static string FormatTemplateRoute_OptionalParameterCanbBePrecededByPeriod(object p0, object p1, object p2) + { + return string.Format(CultureInfo.CurrentCulture, GetString("TemplateRoute_OptionalParameterCanbBePrecededByPeriod"), p0, p1, p2); + } + + /// + /// An optional parameter must be at the end of the segment. In the segment '{0}', optional parameter '{1}' is followed by '{2}'. + /// + internal static string TemplateRoute_OptionalParameterHasTobeTheLast + { + get { return GetString("TemplateRoute_OptionalParameterHasTobeTheLast"); } + } + + /// + /// An optional parameter must be at the end of the segment. In the segment '{0}', optional parameter '{1}' is followed by '{2}'. + /// + internal static string FormatTemplateRoute_OptionalParameterHasTobeTheLast(object p0, object p1, object p2) + { + return string.Format(CultureInfo.CurrentCulture, GetString("TemplateRoute_OptionalParameterHasTobeTheLast"), p0, p1, p2); + } + private static string GetString(string name, params string[] formatterNames) { var value = _resourceManager.GetString(name); diff --git a/src/Microsoft.AspNet.Routing/Resources.resx b/src/Microsoft.AspNet.Routing/Resources.resx index 072dcf21b2..b65a2d6956 100644 --- a/src/Microsoft.AspNet.Routing/Resources.resx +++ b/src/Microsoft.AspNet.Routing/Resources.resx @@ -153,9 +153,6 @@ The route template separator character '/' cannot appear consecutively. It must be separated by either a parameter or a literal value. - - In a path segment that contains more than one section, such as a literal section or a parameter, there can only be one optional parameter. The optional parameter must be the last parameter in the segment and must be preceded by one single period (.). - A catch-all parameter cannot be marked optional. @@ -189,4 +186,10 @@ In a route parameter, '{' and '}' must be escaped with '{{' and '}}' + + In the segment '{0}', the optional parameter '{1}' is preceded by an invalid segment '{2}'. Only a period (.) can precede an optional parameter. + + + An optional parameter must be at the end of the segment. In the segment '{0}', optional parameter '{1}' is followed by '{2}'. + \ No newline at end of file diff --git a/src/Microsoft.AspNet.Routing/Template/TemplateParser.cs b/src/Microsoft.AspNet.Routing/Template/TemplateParser.cs index 2c4958b046..84b8f8b82f 100644 --- a/src/Microsoft.AspNet.Routing/Template/TemplateParser.cs +++ b/src/Microsoft.AspNet.Routing/Template/TemplateParser.cs @@ -340,26 +340,48 @@ namespace Microsoft.AspNet.Routing.Template if (part.IsParameter && part.IsOptional && segment.Parts.Count > 1) { - // This is the last part + // This optional parameter is the last part in the segment if (i == segment.Parts.Count - 1) { Debug.Assert(segment.Parts[i - 1].IsLiteral); + // the optional parameter is preceded by a period if (segment.Parts[i - 1].Text == PeriodString) { segment.Parts[i - 1].IsOptionalSeperator = true; } else { - context.Error = - Resources.TemplateRoute_CanHaveOnlyLastParameterOptional_IfFollowingOptionalSeperator; + // The optional parameter is preceded by a literal other than period + // Example of error message: + // "In the complex segment {RouteValue}-{param?}, the optional parameter 'param'is preceded + // by an invalid segment "-". Only valid literal to precede an optional parameter is a + // period (.). + context.Error = string.Format( + Resources.TemplateRoute_OptionalParameterCanbBePrecededByPeriod, + segment.DebuggerToString(), + part.Name, + segment.Parts[i - 1].Text); + return false; } } else { - context.Error = - Resources.TemplateRoute_CanHaveOnlyLastParameterOptional_IfFollowingOptionalSeperator; + // This optional parameter is not the last one in the segment + // Example: + // An optional parameter must be at the end of the segment.In the segment '{RouteValue?})', + // optional parameter 'RouteValue' is followed by ')' + var nextPart = segment.Parts[i + 1]; + var invalidPartText = nextPart.IsParameter ? nextPart.Name : nextPart.Text; + + context.Error = string.Format( + Resources.TemplateRoute_OptionalParameterHasTobeTheLast, + segment.DebuggerToString(), + segment.Parts[i].Name, + invalidPartText + ); + return false; } } diff --git a/test/Microsoft.AspNet.Routing.Tests/Template/TemplateParserTests.cs b/test/Microsoft.AspNet.Routing.Tests/Template/TemplateParserTests.cs index 99da06ac17..005a26b796 100644 --- a/test/Microsoft.AspNet.Routing.Tests/Template/TemplateParserTests.cs +++ b/test/Microsoft.AspNet.Routing.Tests/Template/TemplateParserTests.cs @@ -528,23 +528,37 @@ namespace Microsoft.AspNet.Routing.Template.Tests } [Theory] - [InlineData("{p1?}.{p2?}")] - [InlineData("{p1}.{p2?}.{p3}")] - [InlineData("{p1?}.{p2}/{p3}")] - [InlineData("{p3}.{p1?}.{p2?}")] - [InlineData("{p1}-{p2?}")] - [InlineData("{p1}..{p2?}")] - [InlineData("..{p2?}")] - [InlineData("{p1}.abc.{p2?}")] - public void Parse_ComplexSegment_OptionalParametersSeperatedByPeriod_Invalid(string template) + [InlineData("{p1}.{p2?}.{p3}", "p2", ".")] + [InlineData("{p1?}{p2}", "p1", "p2")] + [InlineData("{p1?}{p2?}", "p1", "p2")] + [InlineData("{p1}.{p2?})", "p2", ")")] + [InlineData("{foorb?}-bar-{z}", "foorb", "-bar-")] + public void Parse_ComplexSegment_OptionalParameter_NotTheLastPart( + string template, + string parameter, + string invalid) { // Act and Assert ExceptionAssert.Throws( () => TemplateParser.Parse(template), - "In a path segment that contains more than one section, such as a literal section or a parameter, " + - "there can only be one optional parameter. The optional parameter must be the last parameter in the " + - "segment and must be preceded by one single period (.)." + Environment.NewLine + - "Parameter name: routeTemplate"); + "An optional parameter must be at the end of the segment. In the segment '" + template + + "', optional parameter '" + parameter + "' is followed by '" + invalid + "'." + + Environment.NewLine + "Parameter name: routeTemplate"); + } + + [InlineData("{p1}-{p2?}", "-")] + [InlineData("{p1}..{p2?}", "..")] + [InlineData("..{p2?}", "..")] + [InlineData("{p1}.abc.{p2?}", ".abc.")] + [InlineData("{p1}{p2?}", "p1")] + public void Parse_ComplexSegment_OptionalParametersSeperatedByPeriod_Invalid(string template, string parameter) + { + // Act and Assert + ExceptionAssert.Throws( + () => TemplateParser.Parse(template), + "In the complex segment '"+ template +"', the optional parameter 'p2' is preceded by an invalid " + + "segment '" + parameter +"'. Only valid literal to precede an optional parameter is a period (.)." + + Environment.NewLine + "Parameter name: routeTemplate"); } [Fact] @@ -626,7 +640,7 @@ namespace Microsoft.AspNet.Routing.Template.Tests [InlineData("{p}}}", "p}")] [InlineData("{p/}", "p/")] public void ParseRouteParameter_ThrowsIf_ParameterContainsSpecialCharacters( - string template, + string template, string parameterName) { // Arrange @@ -794,17 +808,6 @@ namespace Microsoft.AspNet.Routing.Template.Tests "Parameter name: routeTemplate"); } - [Fact] - public void InvalidTemplate_MultiSegmentParameterCannotContainOptionalParameter() - { - ExceptionAssert.Throws( - () => TemplateParser.Parse("{foorb?}-bar-{z}"), - "In a path segment that contains more than one section, such as a literal section or a parameter, " + - "there can only be one optional parameter. The optional parameter must be the last parameter in " + - "the segment and must be preceded by one single period (.)." + Environment.NewLine + - "Parameter name: routeTemplate"); - } - [Fact] public void InvalidTemplate_CatchAllMarkedOptional() {