diff --git a/.agents/skills/verify-querykit/features/error-handling.md b/.agents/skills/verify-querykit/features/error-handling.md index 151a534..d8f4304 100644 --- a/.agents/skills/verify-querykit/features/error-handling.md +++ b/.agents/skills/verify-querykit/features/error-handling.md @@ -5,6 +5,7 @@ A developer catches `QueryKitException` to return a `400` for bad input. QueryKi ## Sub-features - `error-parsing` throws `ParsingException` for bad syntax: an unknown operator, a missing quote, a missing parenthesis, or a missing value. +- `error-bad-value` throws `ParsingException` for a value that does not convert to the property type, in the scalar form and in the `^^ [...]` list form. The message names the value, the type, and the property name as the client wrote it. - `error-unknown-filter-property` throws `UnknownFilterPropertyException` for an unknown filter property. - `error-sort` throws `SortParsingException` for an unknown sort property. - `error-depth` throws `QueryKitPropertyDepthExceededException` when a path is deeper than `MaxPropertyDepth` (see `configuration.md`). @@ -22,11 +23,12 @@ Preconditions: - **Bad operator.** Run `qk run error-handling-bad-operator --filter 'Title === "x"'`. Exit `2`. Both targets have `error.type` `QueryKit.Exceptions.ParsingException` and `isQueryKitException` `true`. - **Missing quote.** Run `qk run error-handling-missing-quote --filter 'Title == "unterminated'`. Exit `2`. Both targets have `ParsingException`. The message contains `expected "`. - **Missing parenthesis.** Run `qk run error-handling-missing-paren --filter '(Rating > 1'`. Exit `2`. Both targets have `ParsingException`. The message contains `expected )`. +- **Bad value.** Run `qk run error-handling-bad-value --filter 'Visibility ^^ [Public, Bogus]'`. Exit `2`. Both targets have `ParsingException` with the message `The value 'Bogus' is not a valid Visibility for the filter property 'Visibility'.` Also drive `Rating == "abc"` (the type is `Int32`). - **Unknown filter property.** Run `qk run error-handling-unknown-property --filter 'Nope == 1'`. Exit `2`. Both targets have `QueryKit.Exceptions.UnknownFilterPropertyException` with the message `The filter property 'Nope' was not recognized.` - **Unknown sort property.** Run `qk run error-handling-unknown-sort --sort 'Nope desc'`. Exit `2`. Both targets have `QueryKit.Exceptions.SortParsingException` with the message `Parsing failed during sorting. 'Nope' was not recognized.` ## Gotchas -- Some bad input escapes the `QueryKitException` hierarchy. `Rating > "abc"` throws `System.FormatException`, and `Title @= null` throws `System.ArgumentNullException` on the memory target. The driver exits `1` for these. A consumer that catches only `QueryKitException` returns a `500` for them. +- If the driver exits `1`, an exception escaped the `QueryKitException` hierarchy. A consumer that catches only `QueryKitException` returns a `500` for it. Report this as a fault. - An unquoted string value such as `Title == salt` is not an error. It is a literal (see `filtering.md`). - On the Postgres target, the `sql` field is absent when QueryKit throws. This is correct, because the query was never built. diff --git a/.agents/skills/verify-querykit/features/filtering.md b/.agents/skills/verify-querykit/features/filtering.md index 1252240..4c73065 100644 --- a/.agents/skills/verify-querykit/features/filtering.md +++ b/.agents/skills/verify-querykit/features/filtering.md @@ -41,6 +41,6 @@ Preconditions: - Without `--sort`, both targets return rows in seed order. The Postgres order comes from the `ORDER BY r."Id"` that EF Core adds, not from QueryKit. - An unquoted value such as `Title == salt` does not throw. QueryKit reads it as the literal `"salt"` and returns no rows. -- `Title @= null` throws `System.ArgumentNullException` on the memory target. This is not a `QueryKitException`. -- `Rating > "abc"` throws `System.FormatException`, not a `QueryKitException`. The driver exits `1`. +- `Title @= null` throws `QueryKitParsingException` on both targets. The message tells the client to use `==` or `!=` for null. +- `Rating > "abc"` throws `ParsingException` on both targets (see `error-handling.md`). - The case-insensitive operators use `lower()` in SQL by default. The `upper` preset changes this (see `configuration.md`). diff --git a/QueryKit.IntegrationTests/Tests/FilterParsingRegressionTests.cs b/QueryKit.IntegrationTests/Tests/FilterParsingRegressionTests.cs index 49782b1..8eb0294 100644 --- a/QueryKit.IntegrationTests/Tests/FilterParsingRegressionTests.cs +++ b/QueryKit.IntegrationTests/Tests/FilterParsingRegressionTests.cs @@ -441,4 +441,24 @@ public async Task timestamp_without_time_zone_value_matches_with_unspecified_kin // Assert people.Select(x => x.Id).Should().Equal(fakePersonOne.Id); } + + [Theory] + [InlineData("""Age == "abc" """)] + [InlineData("""Age == abc""")] + [InlineData("""Rating > "abc" """)] + [InlineData("""Rating > abc""")] + [InlineData("""BirthMonth ^^ ["Bogus"]""")] + [InlineData("""BirthMonth ^^ [Bogus]""")] + public async Task invalid_value_throws_parsing_exception(string input) + { + // Arrange + var testingServiceScope = new TestingServiceScope(); + + // Act + var queryablePeople = testingServiceScope.DbContext().People; + var act = async () => await queryablePeople.ApplyQueryKitFilter(input).ToListAsync(); + + // Assert + await act.Should().ThrowAsync(); + } } diff --git a/QueryKit.UnitTests/DotNumberCultureTests.cs b/QueryKit.UnitTests/DotNumberCultureTests.cs index a8ec4d0..0278452 100644 --- a/QueryKit.UnitTests/DotNumberCultureTests.cs +++ b/QueryKit.UnitTests/DotNumberCultureTests.cs @@ -34,11 +34,20 @@ public void dot_number_on_an_integer_property_throws_parsing_exception(string cu [InlineData("de-DE", "Rating ^^ [\"4.0\"]")] [InlineData("de-DE", "Rating > @4.4")] [InlineData("de-DE", "HaveMadeItMyself == 4.4")] - public void number_that_v1_14_2_also_converted_throws_format_exception(string cultureName, string input) + public void number_that_v1_14_2_also_converted_throws_parsing_exception(string cultureName, string input) { var act = () => WithCulture(cultureName, () => FilterParser.ParseFilter(input)); - act.Should().ThrowExactly(); + act.Should().ThrowExactly().WithInnerExceptionExactly(); + } + + [Fact] + public void dot_number_on_an_integer_property_names_the_value_in_a_comma_culture() + { + var act = () => WithCulture("de-DE", () => FilterParser.ParseFilter("Rating > 4.4")); + + act.Should().ThrowExactly() + .WithMessage("The value '4.4' is not a valid Int32 for the filter property 'Rating'."); } [Fact] diff --git a/QueryKit.UnitTests/FilterParserTests.cs b/QueryKit.UnitTests/FilterParserTests.cs index 0fbf54b..ced8fc9 100644 --- a/QueryKit.UnitTests/FilterParserTests.cs +++ b/QueryKit.UnitTests/FilterParserTests.cs @@ -832,7 +832,7 @@ public void can_throw_exception_when_invalid_enum_value() var input = $"""BirthMonth == invalid"""; var act = () => FilterParser.ParseFilter(input); act.Should().Throw() - .WithMessage("There was a parsing failure, likely due to an invalid comparison or logical operator. You may also be missing double quotes surrounding a string or guid.*"); + .WithMessage("The value 'invalid' is not a valid BirthMonthEnum for the filter property 'BirthMonth'."); } [Fact] diff --git a/QueryKit.UnitTests/FilterParsingRegressionTests.cs b/QueryKit.UnitTests/FilterParsingRegressionTests.cs index 707da62..f2ed76d 100644 --- a/QueryKit.UnitTests/FilterParsingRegressionTests.cs +++ b/QueryKit.UnitTests/FilterParsingRegressionTests.cs @@ -3,6 +3,7 @@ namespace QueryKit.UnitTests; using System.Globalization; using System.Linq.Expressions; using System.Reflection; +using Configuration; using Exceptions; using FluentAssertions; using Operators; @@ -405,6 +406,62 @@ public void date_time_list_value_matches_scalar_value(string input) scalarResult.Select(x => x.Title).Should().Equal("match"); } + [Theory] + [InlineData("""Age == "abc" """)] + [InlineData("""Age == abc""")] + [InlineData("""Rating > "abc" """)] + [InlineData("""Rating > abc""")] + [InlineData("""Age == 99999999999""")] + [InlineData("""Id == "abc" """)] + [InlineData("""SpecificDateTime == "abc" """)] + [InlineData("""Favorite == "abc" """)] + [InlineData("""Age ^^ ["abc"]""")] + [InlineData("""BirthMonth == "Bogus" """)] + [InlineData("""BirthMonth ^^ ["Bogus"]""")] + [InlineData("""BirthMonth ^^ [Bogus]""")] + public void invalid_value_throws_parsing_exception(string input) + { + var act = () => FilterParser.ParseFilter(input); + + act.Should().Throw(); + } + + [Theory] + [InlineData("""BirthMonth ^^ ["Bogus"]""", "Bogus", "BirthMonthEnum", "BirthMonth")] + [InlineData("""BirthMonth ^^ [Bogus]""", "Bogus", "BirthMonthEnum", "BirthMonth")] + [InlineData("""BirthMonth ^^ [January, Bogus]""", "Bogus", "BirthMonthEnum", "BirthMonth")] + [InlineData("""BirthMonth == "Bogus" """, "Bogus", "BirthMonthEnum", "BirthMonth")] + [InlineData("""BirthMonth == Bogus""", "Bogus", "BirthMonthEnum", "BirthMonth")] + [InlineData("""Age == "abc" """, "abc", "Int32", "Age")] + [InlineData("""Age == 99999999999""", "99999999999", "Int32", "Age")] + [InlineData("""Age ^^ [1, abc]""", "abc", "Int32", "Age")] + [InlineData("""Rating > abc""", "abc", "Decimal", "Rating")] + [InlineData("""Id == "abc" """, "abc", "Guid", "Id")] + [InlineData("""SpecificDateTime == "abc" """, "abc", "DateTime", "SpecificDateTime")] + [InlineData("""Favorite == "abc" """, "abc", "Boolean", "Favorite")] + public void invalid_value_message_names_the_value_the_type_and_the_property(string input, string value, string type, string property) + { + var act = () => FilterParser.ParseFilter(input); + + act.Should().ThrowExactly() + .WithMessage($"The value '{value}' is not a valid {type} for the filter property '{property}'."); + } + + [Fact] + public void invalid_value_message_names_the_query_name() + { + var config = new QueryKitConfiguration(settings => + { + settings.Property(x => x.BirthMonth!).HasQueryName("month"); + }); + + var act = () => FilterParser.ParseFilter("""month ^^ ["Bogus"]""", config); + + act.Should().ThrowExactly() + .WithMessage("The value 'Bogus' is not a valid BirthMonthEnum for the filter property 'month'.") + .WithInnerExceptionExactly(); + } + private static TResult WithCulture(string cultureName, Func action) { var originalCulture = CultureInfo.CurrentCulture; diff --git a/QueryKit.UnitTests/QueryNameOverUnknownTests.cs b/QueryKit.UnitTests/QueryNameOverUnknownTests.cs index f5e1514..a3915b8 100644 --- a/QueryKit.UnitTests/QueryNameOverUnknownTests.cs +++ b/QueryKit.UnitTests/QueryNameOverUnknownTests.cs @@ -43,11 +43,11 @@ public void failure_before_the_query_name_throws_parsing_exception() } [Fact] - public void failure_before_the_query_name_throws_its_own_format_exception() + public void failure_before_the_query_name_throws_its_own_parsing_exception() { var act = () => FilterParser.ParseFilter("""Age > "x" && is adult == true""", Config); - act.Should().ThrowExactly(); + act.Should().ThrowExactly().WithInnerExceptionExactly(); } [Fact] diff --git a/QueryKit/Exceptions/ParsingException.cs b/QueryKit/Exceptions/ParsingException.cs index c3dbb3e..54b4bf5 100644 --- a/QueryKit/Exceptions/ParsingException.cs +++ b/QueryKit/Exceptions/ParsingException.cs @@ -9,6 +9,13 @@ public ParsingException(Exception exception) { } + // A filter value that does not convert to the type of its property. The client sent the value and the property, + // so the message can name them. + public ParsingException(string value, string propertyName, Type targetType, Exception exception) + : base($"The value '{value}' is not a valid {targetType.Name} for the filter property '{propertyName}'.", exception) + { + } + private static string BuildMessage(Exception exception) { const string baseMessage = "There was a parsing failure, likely due to an invalid comparison or logical operator. You may also be missing double quotes surrounding a string or guid."; diff --git a/QueryKit/FilterParser.cs b/QueryKit/FilterParser.cs index cfd21be..84a850b 100644 --- a/QueryKit/FilterParser.cs +++ b/QueryKit/FilterParser.cs @@ -67,6 +67,14 @@ public static Expression> ParseFilter(string input, IQueryKitCo { throw new ParsingException(e); } + catch (FormatException e) + { + throw new ParsingException(e); + } + catch (OverflowException e) + { + throw new ParsingException(e); + } finally { FilterValue.Parameterize = parameterizeBefore; @@ -350,6 +358,12 @@ private static Expression BuildClauseLikeV1142(string? cultureNumberPrefix, stri buildClause(cultureNumberPrefix); } + // A value that does not convert already has a ParsingException that names the value. + if (exception is ParsingException) + { + throw; + } + throw new ParsingException(exception); } } @@ -516,8 +530,23 @@ private static DateTimeOffset ParseDateTimeOffset(string value) { typeof(sbyte), value => sbyte.Parse(value, CultureInfo.InvariantCulture) }, }; + // A value that does not convert to the property type throws ParsingException with the value, the type, and the + // property name as the caller wrote it (the query name, not the member path). private static Expression CreateRightExpr(Expression leftExpr, string right, bool rightIsQuotedLiteral, ComparisonOperator op, - IQueryKitConfiguration? config = null, string? propertyPath = null, string? memberPath = null) + string propertyName, IQueryKitConfiguration? config = null, string? propertyPath = null, string? memberPath = null) + { + try + { + return CreateRightExprForProperty(leftExpr, right, rightIsQuotedLiteral, op, config, propertyPath, memberPath); + } + catch (InvalidFilterValueException e) + { + throw new ParsingException(e.Value, propertyName, e.TargetType, e.InnerException!); + } + } + + private static Expression CreateRightExprForProperty(Expression leftExpr, string right, bool rightIsQuotedLiteral, ComparisonOperator op, + IQueryKitConfiguration? config, string? propertyPath, string? memberPath) { var targetType = leftExpr.Type; @@ -681,7 +710,7 @@ private static Expression CreateRightExprFromType(Type leftExprType, string righ x = x.Trim('"'); } - var convertedValue = TypeConversionFunctions[elementType](x); + var convertedValue = ConvertValue(x, elementType, () => TypeConversionFunctions[elementType](x)); return Expression.Constant(convertedValue, elementType); }).ToArray(); @@ -696,27 +725,27 @@ private static Expression CreateRightExprFromType(Type leftExprType, string righ if (targetType == typeof(DateTime)) { - var dt = ParseDateTime(right); + var dt = ConvertValue(right, targetType, () => ParseDateTime(right)); return FilterValue.Create(dt, rawType); } if (targetType == typeof(DateTimeOffset)) { - var dto = ParseDateTimeOffset(right); + var dto = ConvertValue(right, targetType, () => ParseDateTimeOffset(right)); return FilterValue.Create(dto, rawType); } if (targetType == typeof(DateOnly)) { - var date = DateOnly.Parse(right, CultureInfo.InvariantCulture); + var date = ConvertValue(right, targetType, () => DateOnly.Parse(right, CultureInfo.InvariantCulture)); return FilterValue.Create(date, rawType); } if (targetType == typeof(TimeOnly)) { - var time = TimeOnly.Parse(right, CultureInfo.InvariantCulture); + var time = ConvertValue(right, targetType, () => TimeOnly.Parse(right, CultureInfo.InvariantCulture)); var fractionalTicks = time.Ticks % TimeSpan.TicksPerSecond; var millisecond = (int)(fractionalTicks / TimeSpan.TicksPerMillisecond); @@ -738,11 +767,11 @@ private static Expression CreateRightExprFromType(Type leftExprType, string righ } // Parse the GUID for direct comparison - var guidValue = Guid.Parse(right); + var guidValue = ConvertValue(right, targetType, () => Guid.Parse(right)); return FilterValue.Create(guidValue, typeof(Guid)); } - var convertedValue = conversionFunction(right); + var convertedValue = ConvertValue(right, targetType, () => conversionFunction(right)); return FilterValue.Create(convertedValue, leftExprType); } @@ -767,7 +796,7 @@ private static Expression CreateRightExprFromType(Type leftExprType, string righ x = x.Trim('"'); } - var enumValue = Enum.Parse(enumType, x); + var enumValue = ConvertValue(x, enumType, () => Enum.Parse(enumType, x)); var constant = Expression.Constant(enumValue, enumType); return constant; @@ -777,11 +806,7 @@ private static Expression CreateRightExprFromType(Type leftExprType, string righ return newArrayExpression; } - var parsed = Enum.TryParse(enumType, right, out var enumValue); - if (!parsed) - { - throw new InvalidOperationException($"Unsupported value '{right}' for type '{targetType.Name}'"); - } + var enumValue = ConvertValue(right, enumType, () => Enum.Parse(enumType, right)); return FilterValue.Create(enumValue, rawType); } @@ -802,6 +827,33 @@ private static Expression CreateRightExprFromType(Type leftExprType, string righ throw new InvalidOperationException($"Unsupported value '{right}' for type '{targetType.Name}'"); } + // Converts one filter value. The parse methods throw FormatException or OverflowException, and Enum.Parse throws + // ArgumentException. CreateRightExpr adds the property name and throws ParsingException. + private static TValue ConvertValue(string value, Type targetType, Func convert) + { + try + { + return convert(); + } + catch (Exception e) when (e is FormatException or OverflowException or ArgumentException) + { + throw new InvalidFilterValueException(value, targetType, e); + } + } + + private sealed class InvalidFilterValueException : Exception + { + public InvalidFilterValueException(string value, Type targetType, Exception inner) + : base(null, inner) + { + Value = value; + TargetType = targetType; + } + + public string Value { get; } + public Type TargetType { get; } + } + private static Type TransformTargetTypeIfNullable(Type targetType) { if (targetType.IsNullable()) @@ -990,12 +1042,12 @@ private static Parser ComparisonExprParser(ParameterExpression pa var leftExprForRightSide = guidConfig?.UsesConversion == true && guidConfig.ConversionTargetType == typeof(string) ? guidStringExpr : leftExpr; - return temp.op.GetExpression(guidStringExpr, CreateRightExpr(leftExprForRightSide, temp.right, temp.rightIsQuotedLiteral, temp.op, config, guidPropertyPath), + return temp.op.GetExpression(guidStringExpr, CreateRightExpr(leftExprForRightSide, temp.right, temp.rightIsQuotedLiteral, temp.op, temp.reference.Text, config, guidPropertyPath), config?.DbContextType, ResolveCaseMode(guidPropertyPath, config)); } // For non-string operators, use direct GUID comparison - return temp.op.GetExpression(leftExpr, CreateRightExpr(leftExpr, temp.right, temp.rightIsQuotedLiteral, temp.op, config, guidPropertyPath), + return temp.op.GetExpression(leftExpr, CreateRightExpr(leftExpr, temp.right, temp.rightIsQuotedLiteral, temp.op, temp.reference.Text, config, guidPropertyPath), config?.DbContextType); } @@ -1120,7 +1172,7 @@ private static Parser ComparisonExprParser(ParameterExpression pa } } - var rightExpr = CreateRightExpr(leftExprForComparison, temp.right, temp.rightIsQuotedLiteral, temp.op, config, propertyPath); + var rightExpr = CreateRightExpr(leftExprForComparison, temp.right, temp.rightIsQuotedLiteral, temp.op, temp.reference.Text, config, propertyPath); // Handle nested collection filtering if (leftExprForComparison is MethodCallExpression methodCall && IsNestedCollectionExpression(methodCall)) @@ -1354,7 +1406,7 @@ private static Parser PropertyListComparisonExprParser( leftExpr = HandleGuidConversion(leftExpr, leftExpr.Type); } - var rightExpr = CreateRightExpr(leftExpr, temp.right, temp.rightIsQuotedLiteral, temp.op, config, fullPropPath, reference.Path); + var rightExpr = CreateRightExpr(leftExpr, temp.right, temp.rightIsQuotedLiteral, temp.op, fullPropPath, config, fullPropPath, reference.Path); var comparison = temp.op.GetExpression(leftExpr, rightExpr, config?.DbContextType, ResolveCaseMode(reference.Path, config)); // Combine with AND for negative operators, OR for positive operators