From f818c8f4e07eb4cb4a8d023dac39998d3b5f572f Mon Sep 17 00:00:00 2001 From: Bart Koelman <10324372+bkoelman@users.noreply.github.com> Date: Thu, 15 Jun 2023 01:27:08 +0200 Subject: [PATCH 1/3] Fixed: do not allow comparison of count() with null; do not treat null as a field name --- .../Queries/Internal/Parsing/FilterParser.cs | 84 ++++++++++++++----- .../Internal/Parsing/QueryExpressionParser.cs | 2 +- .../QueryStringParameters/FilterParseTests.cs | 10 ++- .../LegacyFilterParseTests.cs | 4 +- 4 files changed, 70 insertions(+), 30 deletions(-) diff --git a/src/JsonApiDotNetCore/Queries/Internal/Parsing/FilterParser.cs b/src/JsonApiDotNetCore/Queries/Internal/Parsing/FilterParser.cs index 541b50a220..9ecde27289 100644 --- a/src/JsonApiDotNetCore/Queries/Internal/Parsing/FilterParser.cs +++ b/src/JsonApiDotNetCore/Queries/Internal/Parsing/FilterParser.cs @@ -135,40 +135,34 @@ protected ComparisonExpression ParseComparison(string operatorName) EatText(operatorName); EatSingleCharacterToken(TokenKind.OpenParen); - // Allow equality comparison of a HasOne relationship with null. + // Allow equality comparison of a to-one relationship with null. FieldChainRequirements leftChainRequirements = comparisonOperator == ComparisonOperator.Equals ? FieldChainRequirements.EndsInAttribute | FieldChainRequirements.EndsInToOne : FieldChainRequirements.EndsInAttribute; QueryExpression leftTerm = ParseCountOrField(leftChainRequirements); - Converter rightConstantValueConverter; + + EatSingleCharacterToken(TokenKind.Comma); + + QueryExpression rightTerm; if (leftTerm is CountExpression) { - rightConstantValueConverter = GetConstantValueConverterForCount(); + Converter rightConstantValueConverter = GetConstantValueConverterForCount(); + rightTerm = ParseCountOrConstantOrField(FieldChainRequirements.EndsInAttribute, rightConstantValueConverter); } else if (leftTerm is ResourceFieldChainExpression fieldChain && fieldChain.Fields[^1] is AttrAttribute attribute) { - rightConstantValueConverter = GetConstantValueConverterForAttribute(attribute); + Converter rightConstantValueConverter = GetConstantValueConverterForAttribute(attribute); + rightTerm = ParseCountOrConstantOrNullOrField(FieldChainRequirements.EndsInAttribute, rightConstantValueConverter); } else { - // This temporary value never survives; it gets discarded during the second pass below. - rightConstantValueConverter = _ => 0; + rightTerm = ParseNull(); } - EatSingleCharacterToken(TokenKind.Comma); - - QueryExpression rightTerm = ParseCountOrConstantOrNullOrField(FieldChainRequirements.EndsInAttribute, rightConstantValueConverter); - EatSingleCharacterToken(TokenKind.CloseParen); - if (leftTerm is ResourceFieldChainExpression leftChain && leftChain.Fields[^1] is RelationshipAttribute && rightTerm is not NullConstantExpression) - { - // Run another pass over left chain to produce an error. - OnResolveFieldChain(leftChain.ToString(), FieldChainRequirements.EndsInAttribute); - } - return new ComparisonExpression(comparisonOperator, leftTerm, rightTerm); } @@ -201,8 +195,9 @@ protected AnyExpression ParseAny() EatText(Keywords.Any); EatSingleCharacterToken(TokenKind.OpenParen); - ResourceFieldChainExpression targetAttribute = ParseFieldChain(FieldChainRequirements.EndsInAttribute, null); - Converter constantValueConverter = GetConstantValueConverterForAttribute((AttrAttribute)targetAttribute.Fields[^1]); + ResourceFieldChainExpression targetAttributeChain = ParseFieldChain(FieldChainRequirements.EndsInAttribute, null); + var targetAttribute = (AttrAttribute)targetAttributeChain.Fields[^1]; + Converter constantValueConverter = GetConstantValueConverterForAttribute(targetAttribute); EatSingleCharacterToken(TokenKind.Comma); @@ -223,7 +218,7 @@ protected AnyExpression ParseAny() IImmutableSet constantSet = constantsBuilder.ToImmutable(); - return new AnyExpression(targetAttribute, constantSet); + return new AnyExpression(targetAttributeChain, constantSet); } protected HasExpression ParseHas() @@ -349,6 +344,25 @@ protected QueryExpression ParseCountOrField(FieldChainRequirements chainRequirem return ParseFieldChain(chainRequirements, "Count function or field name expected."); } + protected QueryExpression ParseCountOrConstantOrField(FieldChainRequirements chainRequirements, Converter constantValueConverter) + { + CountExpression? count = TryParseCount(); + + if (count != null) + { + return count; + } + + LiteralConstantExpression? constant = TryParseConstant(constantValueConverter); + + if (constant != null) + { + return constant; + } + + return ParseFieldChain(chainRequirements, "Count function, value between quotes or field name expected."); + } + protected QueryExpression ParseCountOrConstantOrNullOrField(FieldChainRequirements chainRequirements, Converter constantValueConverter) { CountExpression? count = TryParseCount(); @@ -368,6 +382,19 @@ protected QueryExpression ParseCountOrConstantOrNullOrField(FieldChainRequiremen return ParseFieldChain(chainRequirements, "Count function, value between quotes, null or field name expected."); } + protected LiteralConstantExpression? TryParseConstant(Converter constantValueConverter) + { + if (TokenStack.TryPeek(out Token? nextToken) && nextToken.Kind == TokenKind.QuotedText) + { + TokenStack.Pop(); + + object constantValue = constantValueConverter(nextToken.Value!); + return new LiteralConstantExpression(constantValue, nextToken.Value!); + } + + return null; + } + protected IdentifierExpression? TryParseConstantOrNull(Converter constantValueConverter) { if (TokenStack.TryPeek(out Token? nextToken)) @@ -392,13 +419,24 @@ protected QueryExpression ParseCountOrConstantOrNullOrField(FieldChainRequiremen protected LiteralConstantExpression ParseConstant(Converter constantValueConverter) { - if (TokenStack.TryPop(out Token? token) && token.Kind == TokenKind.QuotedText) + LiteralConstantExpression? constant = TryParseConstant(constantValueConverter); + + if (constant == null) + { + throw new QueryParseException("Value between quotes expected."); + } + + return constant; + } + + protected NullConstantExpression ParseNull() + { + if (TokenStack.TryPop(out Token? token) && token is { Kind: TokenKind.Text, Value: Keywords.Null }) { - object constantValue = constantValueConverter(token.Value!); - return new LiteralConstantExpression(constantValue, token.Value!); + return NullConstantExpression.Instance; } - throw new QueryParseException("Value between quotes expected."); + throw new QueryParseException("null expected."); } private Converter GetConstantValueConverterForCount() diff --git a/src/JsonApiDotNetCore/Queries/Internal/Parsing/QueryExpressionParser.cs b/src/JsonApiDotNetCore/Queries/Internal/Parsing/QueryExpressionParser.cs index 27466e3b0a..0e5c84a015 100644 --- a/src/JsonApiDotNetCore/Queries/Internal/Parsing/QueryExpressionParser.cs +++ b/src/JsonApiDotNetCore/Queries/Internal/Parsing/QueryExpressionParser.cs @@ -49,7 +49,7 @@ private void EatFieldChain(StringBuilder pathBuilder, string? alternativeErrorMe { while (true) { - if (TokenStack.TryPop(out Token? token) && token.Kind == TokenKind.Text) + if (TokenStack.TryPop(out Token? token) && token.Kind == TokenKind.Text && token.Value != Keywords.Null) { pathBuilder.Append(token.Value); diff --git a/test/JsonApiDotNetCoreTests/UnitTests/QueryStringParameters/FilterParseTests.cs b/test/JsonApiDotNetCoreTests/UnitTests/QueryStringParameters/FilterParseTests.cs index 140ce94c94..f1ff996998 100644 --- a/test/JsonApiDotNetCoreTests/UnitTests/QueryStringParameters/FilterParseTests.cs +++ b/test/JsonApiDotNetCoreTests/UnitTests/QueryStringParameters/FilterParseTests.cs @@ -63,7 +63,7 @@ public void Reader_Is_Enabled(JsonApiQueryStringParameters parametersDisabled, b "Relationship 'author' in 'posts.author' must be a to-many relationship on resource type 'blogPosts'.")] [InlineData("filter[posts.comments.author]", "equals(firstName,'some')", "Relationship 'author' in 'posts.comments.author' must be a to-many relationship on resource type 'comments'.")] - [InlineData("filter[posts]", "equals(author,'some')", "Attribute 'author' does not exist on resource type 'blogPosts'.")] + [InlineData("filter[posts]", "equals(author,'some')", "null expected.")] [InlineData("filter[posts]", "lessThan(author,null)", "Attribute 'author' does not exist on resource type 'blogPosts'.")] [InlineData("filter", " ", "Unexpected whitespace.")] [InlineData("filter", "contains(owner.displayName ,)", "Unexpected whitespace.")] @@ -73,12 +73,13 @@ public void Reader_Is_Enabled(JsonApiQueryStringParameters parametersDisabled, b [InlineData("filter", "equals'", "Unexpected ' outside text.")] [InlineData("filter", "equals(", "Count function or field name expected.")] [InlineData("filter", "equals('1'", "Count function or field name expected.")] - [InlineData("filter", "equals(count(posts),", "Count function, value between quotes, null or field name expected.")] + [InlineData("filter", "equals(count(posts),", "Count function, value between quotes or field name expected.")] + [InlineData("filter", "equals(count(posts),null)", "Count function, value between quotes or field name expected.")] [InlineData("filter", "equals(owner..displayName,'')", "Count function or field name expected.")] [InlineData("filter", "equals(owner.displayName.,'')", "Count function or field name expected.")] [InlineData("filter", "equals(title,')", "' expected.")] [InlineData("filter", "equals(title,null", ") expected.")] - [InlineData("filter", "equals(null", "Field 'null' does not exist on resource type 'blogs'.")] + [InlineData("filter", "equals(null", "Count function or field name expected.")] [InlineData("filter", "equals(title,(", "Count function, value between quotes, null or field name expected.")] [InlineData("filter", "equals(has(posts),'true')", "Field 'has' does not exist on resource type 'blogs'.")] [InlineData("filter", "has(posts,", "Filter function expected.")] @@ -86,7 +87,7 @@ public void Reader_Is_Enabled(JsonApiQueryStringParameters parametersDisabled, b [InlineData("filter", "contains(title,'a','b')", ") expected.")] [InlineData("filter", "contains(title,null)", "Value between quotes expected.")] [InlineData("filter[posts]", "contains(author,null)", "Attribute 'author' does not exist on resource type 'blogPosts'.")] - [InlineData("filter", "any(null,'a','b')", "Attribute 'null' does not exist on resource type 'blogs'.")] + [InlineData("filter", "any(null,'a','b')", "Field name expected.")] [InlineData("filter", "any('a','b','c')", "Field name expected.")] [InlineData("filter", "any(title,'b','c',)", "Value between quotes expected.")] [InlineData("filter", "any(title)", ", expected.")] @@ -137,6 +138,7 @@ public void Reader_Read_Fails(string parameterName, string parameterValue, strin [InlineData("filter[owner.posts]", "equals(caption,'some')", "owner.posts", "equals(caption,'some')")] [InlineData("filter[posts.comments]", "equals(createdAt,'2000-01-01')", "posts.comments", "equals(createdAt,'2000-01-01')")] [InlineData("filter", "equals(count(posts),'1')", null, "equals(count(posts),'1')")] + [InlineData("filter", "equals(count(posts),count(owner.posts))", null, "equals(count(posts),count(owner.posts))")] [InlineData("filter[posts]", "equals(caption,null)", "posts", "equals(caption,null)")] [InlineData("filter[posts]", "equals(author,null)", "posts", "equals(author,null)")] [InlineData("filter[posts]", "equals(author.userName,author.displayName)", "posts", "equals(author.userName,author.displayName)")] diff --git a/test/JsonApiDotNetCoreTests/UnitTests/QueryStringParameters/LegacyFilterParseTests.cs b/test/JsonApiDotNetCoreTests/UnitTests/QueryStringParameters/LegacyFilterParseTests.cs index 936dcebb7a..9804386baf 100644 --- a/test/JsonApiDotNetCoreTests/UnitTests/QueryStringParameters/LegacyFilterParseTests.cs +++ b/test/JsonApiDotNetCoreTests/UnitTests/QueryStringParameters/LegacyFilterParseTests.cs @@ -32,12 +32,12 @@ public LegacyFilterParseTests() [InlineData("filter[", "some", "Expected field name between brackets in filter parameter name.")] [InlineData("filter[]", "some", "Expected field name between brackets in filter parameter name.")] [InlineData("filter[some]", "other", "Field 'some' does not exist on resource type 'blogPosts'.")] - [InlineData("filter[author]", "some", "Attribute 'author' does not exist on resource type 'blogPosts'.")] + [InlineData("filter[author]", "some", "null expected.")] [InlineData("filter[author.posts]", "some", "Field 'posts' in 'author.posts' must be an attribute or a to-one relationship on resource type 'webAccounts'.")] [InlineData("filter[unknown.id]", "some", "Relationship 'unknown' in 'unknown.id' does not exist on resource type 'blogPosts'.")] [InlineData("filter", "expr:equals(some,'other')", "Field 'some' does not exist on resource type 'blogPosts'.")] - [InlineData("filter", "expr:equals(author,'Joe')", "Attribute 'author' does not exist on resource type 'blogPosts'.")] + [InlineData("filter", "expr:equals(author,'Joe')", "null expected.")] [InlineData("filter", "expr:has(author)", "Relationship 'author' must be a to-many relationship on resource type 'blogPosts'.")] [InlineData("filter", "expr:equals(count(author),'1')", "Relationship 'author' must be a to-many relationship on resource type 'blogPosts'.")] public void Reader_Read_Fails(string parameterName, string parameterValue, string errorMessage) From 5da2ad6c3f61ff002cf1c1d53dd40a7c773fd9b4 Mon Sep 17 00:00:00 2001 From: Bart Koelman <10324372+bkoelman@users.noreply.github.com> Date: Thu, 15 Jun 2023 02:32:13 +0200 Subject: [PATCH 2/3] Add extensibility point for pluggable filter value conversions --- .../QueryStringParserBenchmarks.cs | 2 +- .../Queries/Internal/Parsing/FilterParser.cs | 79 ++++++-- .../Internal/Parsing/QueryParseException.cs | 5 + .../QueryStrings/IFilterValueConverter.cs | 44 +++++ .../FilterQueryStringParameterReader.cs | 6 +- .../QueryStrings/Appointment.cs | 3 + .../FilterRewritingResourceDefinition.cs | 27 +++ .../FilterTimeRangeRewriter.cs | 34 ++++ .../RelativeTimeFilterValueConverter.cs | 54 +++++ .../RelativeTimeTests.cs | 184 ++++++++++++++++++ .../FilterValueConversion/TimeRange.cs | 13 ++ .../QueryStrings/QueryStringDbContext.cs | 1 + .../QueryStrings/QueryStringFakers.cs | 7 + .../IntegrationTests/QueryStrings/Reminder.cs | 13 ++ .../Queries/QueryExpressionRewriterTests.cs | 3 +- .../QueryStringParameters/FilterParseTests.cs | 2 +- .../FilterValueConversionTests.cs | 85 ++++++++ .../LegacyFilterParseTests.cs | 3 +- 18 files changed, 542 insertions(+), 23 deletions(-) create mode 100644 src/JsonApiDotNetCore/QueryStrings/IFilterValueConverter.cs create mode 100644 test/JsonApiDotNetCoreTests/IntegrationTests/QueryStrings/FilterValueConversion/FilterRewritingResourceDefinition.cs create mode 100644 test/JsonApiDotNetCoreTests/IntegrationTests/QueryStrings/FilterValueConversion/FilterTimeRangeRewriter.cs create mode 100644 test/JsonApiDotNetCoreTests/IntegrationTests/QueryStrings/FilterValueConversion/RelativeTimeFilterValueConverter.cs create mode 100644 test/JsonApiDotNetCoreTests/IntegrationTests/QueryStrings/FilterValueConversion/RelativeTimeTests.cs create mode 100644 test/JsonApiDotNetCoreTests/IntegrationTests/QueryStrings/FilterValueConversion/TimeRange.cs create mode 100644 test/JsonApiDotNetCoreTests/IntegrationTests/QueryStrings/Reminder.cs create mode 100644 test/JsonApiDotNetCoreTests/UnitTests/QueryStringParameters/FilterValueConversionTests.cs diff --git a/benchmarks/QueryString/QueryStringParserBenchmarks.cs b/benchmarks/QueryString/QueryStringParserBenchmarks.cs index 4218c2e3dc..b057f0b01e 100644 --- a/benchmarks/QueryString/QueryStringParserBenchmarks.cs +++ b/benchmarks/QueryString/QueryStringParserBenchmarks.cs @@ -38,7 +38,7 @@ public QueryStringParserBenchmarks() var resourceFactory = new ResourceFactory(new ServiceContainer()); var includeReader = new IncludeQueryStringParameterReader(request, resourceGraph, options); - var filterReader = new FilterQueryStringParameterReader(request, resourceGraph, resourceFactory, options); + var filterReader = new FilterQueryStringParameterReader(request, resourceGraph, resourceFactory, options, Enumerable.Empty()); var sortReader = new SortQueryStringParameterReader(request, resourceGraph); var sparseFieldSetReader = new SparseFieldSetQueryStringParameterReader(request, resourceGraph); var paginationReader = new PaginationQueryStringParameterReader(request, resourceGraph, options); diff --git a/src/JsonApiDotNetCore/Queries/Internal/Parsing/FilterParser.cs b/src/JsonApiDotNetCore/Queries/Internal/Parsing/FilterParser.cs index 9ecde27289..af4d499dbc 100644 --- a/src/JsonApiDotNetCore/Queries/Internal/Parsing/FilterParser.cs +++ b/src/JsonApiDotNetCore/Queries/Internal/Parsing/FilterParser.cs @@ -3,6 +3,7 @@ using JetBrains.Annotations; using JsonApiDotNetCore.Configuration; using JsonApiDotNetCore.Queries.Expressions; +using JsonApiDotNetCore.QueryStrings; using JsonApiDotNetCore.Resources; using JsonApiDotNetCore.Resources.Annotations; using JsonApiDotNetCore.Resources.Internal; @@ -13,14 +14,18 @@ namespace JsonApiDotNetCore.Queries.Internal.Parsing; public class FilterParser : QueryExpressionParser { private readonly IResourceFactory _resourceFactory; + private readonly IEnumerable _filterValueConverters; private readonly Action? _validateSingleFieldCallback; private ResourceType? _resourceTypeInScope; - public FilterParser(IResourceFactory resourceFactory, Action? validateSingleFieldCallback = null) + public FilterParser(IResourceFactory resourceFactory, IEnumerable filterValueConverters, + Action? validateSingleFieldCallback = null) { ArgumentGuard.NotNull(resourceFactory); + ArgumentGuard.NotNull(filterValueConverters); _resourceFactory = resourceFactory; + _filterValueConverters = filterValueConverters; _validateSingleFieldCallback = validateSingleFieldCallback; } @@ -153,7 +158,7 @@ protected ComparisonExpression ParseComparison(string operatorName) } else if (leftTerm is ResourceFieldChainExpression fieldChain && fieldChain.Fields[^1] is AttrAttribute attribute) { - Converter rightConstantValueConverter = GetConstantValueConverterForAttribute(attribute); + Converter rightConstantValueConverter = GetConstantValueConverterForAttribute(attribute, typeof(ComparisonExpression)); rightTerm = ParseCountOrConstantOrNullOrField(FieldChainRequirements.EndsInAttribute, rightConstantValueConverter); } else @@ -172,16 +177,11 @@ protected MatchTextExpression ParseTextMatch(string matchFunctionName) EatSingleCharacterToken(TokenKind.OpenParen); ResourceFieldChainExpression targetAttributeChain = ParseFieldChain(FieldChainRequirements.EndsInAttribute, null); - Type targetAttributeType = ((AttrAttribute)targetAttributeChain.Fields[^1]).Property.PropertyType; - - if (targetAttributeType != typeof(string)) - { - throw new QueryParseException("Attribute of type 'String' expected."); - } + var targetAttribute = (AttrAttribute)targetAttributeChain.Fields[^1]; EatSingleCharacterToken(TokenKind.Comma); - Converter constantValueConverter = stringValue => stringValue; + Converter constantValueConverter = GetConstantValueConverterForAttribute(targetAttribute, typeof(MatchTextExpression)); LiteralConstantExpression constant = ParseConstant(constantValueConverter); EatSingleCharacterToken(TokenKind.CloseParen); @@ -197,12 +197,12 @@ protected AnyExpression ParseAny() ResourceFieldChainExpression targetAttributeChain = ParseFieldChain(FieldChainRequirements.EndsInAttribute, null); var targetAttribute = (AttrAttribute)targetAttributeChain.Fields[^1]; - Converter constantValueConverter = GetConstantValueConverterForAttribute(targetAttribute); EatSingleCharacterToken(TokenKind.Comma); ImmutableHashSet.Builder constantsBuilder = ImmutableHashSet.CreateBuilder(); + Converter constantValueConverter = GetConstantValueConverterForAttribute(targetAttribute, typeof(AnyExpression)); LiteralConstantExpression constant = ParseConstant(constantValueConverter); constantsBuilder.Add(constant); @@ -444,23 +444,68 @@ private Converter GetConstantValueConverterForCount() return stringValue => ConvertStringToType(stringValue, typeof(int)); } - private object ConvertStringToType(string value, Type type) + private static object ConvertStringToType(string value, Type type) { try { return RuntimeTypeConverter.ConvertType(value, type)!; } - catch (FormatException) + catch (FormatException exception) { - throw new QueryParseException($"Failed to convert '{value}' of type 'String' to type '{type.Name}'."); + throw new QueryParseException($"Failed to convert '{value}' of type 'String' to type '{type.Name}'.", exception); } } - private Converter GetConstantValueConverterForAttribute(AttrAttribute attribute) + private Converter GetConstantValueConverterForAttribute(AttrAttribute attribute, Type outerExpressionType) { - return stringValue => attribute.Property.Name == nameof(Identifiable.Id) - ? DeObfuscateStringId(attribute.Type.ClrType, stringValue) - : ConvertStringToType(stringValue, attribute.Property.PropertyType); + return stringValue => + { + object? value = TryConvertFromStringUsingFilterValueConverters(attribute, stringValue, outerExpressionType); + + if (value != null) + { + return value; + } + + if (outerExpressionType == typeof(MatchTextExpression)) + { + if (attribute.Property.PropertyType != typeof(string)) + { + throw new QueryParseException("Attribute of type 'String' expected."); + } + } + else + { + // Partial text matching on an obfuscated ID usually fails. + if (attribute.Property.Name == nameof(Identifiable.Id)) + { + return DeObfuscateStringId(attribute.Type.ClrType, stringValue); + } + } + + return ConvertStringToType(stringValue, attribute.Property.PropertyType); + }; + } + + private object? TryConvertFromStringUsingFilterValueConverters(AttrAttribute attribute, string stringValue, Type outerExpressionType) + { + foreach (IFilterValueConverter converter in _filterValueConverters) + { + if (converter.CanConvert(attribute)) + { + object result = converter.Convert(attribute, stringValue, outerExpressionType); + + if (result == null) + { + throw new InvalidOperationException( + $"Converter '{converter.GetType().Name}' returned null for '{stringValue}' on attribute '{attribute.PublicName}'. Return a sentinel value instead."); + } + + return result; + } + } + + return null; } private object DeObfuscateStringId(Type resourceClrType, string stringId) diff --git a/src/JsonApiDotNetCore/Queries/Internal/Parsing/QueryParseException.cs b/src/JsonApiDotNetCore/Queries/Internal/Parsing/QueryParseException.cs index 2265ca56da..87cd15ab49 100644 --- a/src/JsonApiDotNetCore/Queries/Internal/Parsing/QueryParseException.cs +++ b/src/JsonApiDotNetCore/Queries/Internal/Parsing/QueryParseException.cs @@ -9,4 +9,9 @@ public QueryParseException(string message) : base(message) { } + + public QueryParseException(string message, Exception innerException) + : base(message, innerException) + { + } } diff --git a/src/JsonApiDotNetCore/QueryStrings/IFilterValueConverter.cs b/src/JsonApiDotNetCore/QueryStrings/IFilterValueConverter.cs new file mode 100644 index 0000000000..117513ba0b --- /dev/null +++ b/src/JsonApiDotNetCore/QueryStrings/IFilterValueConverter.cs @@ -0,0 +1,44 @@ +using JetBrains.Annotations; +using JsonApiDotNetCore.Queries.Expressions; +using JsonApiDotNetCore.Queries.Internal.Parsing; +using JsonApiDotNetCore.Resources; +using JsonApiDotNetCore.Resources.Annotations; + +namespace JsonApiDotNetCore.QueryStrings; + +/// +/// Provides conversion of a single-quoted value that occurs in a filter function of a query string. +/// +[PublicAPI] +public interface IFilterValueConverter +{ + /// + /// Indicates whether this converter can be used for the specified . + /// + /// + /// The JSON:API attribute this conversion applies to. + /// + bool CanConvert(AttrAttribute attribute); + + /// + /// Converts to the specified . + /// + /// + /// The JSON:API attribute this conversion applies to. + /// + /// + /// The literal text (without the surrounding single quotes) from the query string. + /// + /// + /// The filter function this conversion applies to, which can be , or + /// . + /// + /// + /// The converted value. Must not be null. In case the type differs from the resource property type, use a + /// from to produce a valid filter. + /// + /// + /// The conversion failed because is invalid. + /// + object Convert(AttrAttribute attribute, string value, Type outerExpressionType); +} diff --git a/src/JsonApiDotNetCore/QueryStrings/Internal/FilterQueryStringParameterReader.cs b/src/JsonApiDotNetCore/QueryStrings/Internal/FilterQueryStringParameterReader.cs index dace5b8ca4..3f1459fadc 100644 --- a/src/JsonApiDotNetCore/QueryStrings/Internal/FilterQueryStringParameterReader.cs +++ b/src/JsonApiDotNetCore/QueryStrings/Internal/FilterQueryStringParameterReader.cs @@ -28,14 +28,16 @@ public class FilterQueryStringParameterReader : QueryStringParameterReader, IFil public bool AllowEmptyValue => false; - public FilterQueryStringParameterReader(IJsonApiRequest request, IResourceGraph resourceGraph, IResourceFactory resourceFactory, IJsonApiOptions options) + public FilterQueryStringParameterReader(IJsonApiRequest request, IResourceGraph resourceGraph, IResourceFactory resourceFactory, IJsonApiOptions options, + IEnumerable filterValueConverters) : base(request, resourceGraph) { ArgumentGuard.NotNull(options); + ArgumentGuard.NotNull(filterValueConverters); _options = options; _scopeParser = new QueryStringParameterScopeParser(FieldChainRequirements.EndsInToMany); - _filterParser = new FilterParser(resourceFactory, ValidateSingleField); + _filterParser = new FilterParser(resourceFactory, filterValueConverters, ValidateSingleField); } protected void ValidateSingleField(ResourceFieldAttribute field, ResourceType resourceType, string path) diff --git a/test/JsonApiDotNetCoreTests/IntegrationTests/QueryStrings/Appointment.cs b/test/JsonApiDotNetCoreTests/IntegrationTests/QueryStrings/Appointment.cs index 2805745644..36a93f9ee3 100644 --- a/test/JsonApiDotNetCoreTests/IntegrationTests/QueryStrings/Appointment.cs +++ b/test/JsonApiDotNetCoreTests/IntegrationTests/QueryStrings/Appointment.cs @@ -19,4 +19,7 @@ public sealed class Appointment : Identifiable [Attr] public DateTimeOffset EndTime { get; set; } + + [HasMany] + public IList Reminders { get; set; } = new List(); } diff --git a/test/JsonApiDotNetCoreTests/IntegrationTests/QueryStrings/FilterValueConversion/FilterRewritingResourceDefinition.cs b/test/JsonApiDotNetCoreTests/IntegrationTests/QueryStrings/FilterValueConversion/FilterRewritingResourceDefinition.cs new file mode 100644 index 0000000000..b67de0ce5f --- /dev/null +++ b/test/JsonApiDotNetCoreTests/IntegrationTests/QueryStrings/FilterValueConversion/FilterRewritingResourceDefinition.cs @@ -0,0 +1,27 @@ +using JetBrains.Annotations; +using JsonApiDotNetCore.Configuration; +using JsonApiDotNetCore.Queries.Expressions; +using JsonApiDotNetCore.Resources; + +namespace JsonApiDotNetCoreTests.IntegrationTests.QueryStrings.FilterValueConversion; + +[UsedImplicitly(ImplicitUseKindFlags.InstantiatedNoFixedConstructorSignature)] +public class FilterRewritingResourceDefinition : JsonApiResourceDefinition + where TResource : class, IIdentifiable +{ + public FilterRewritingResourceDefinition(IResourceGraph resourceGraph) + : base(resourceGraph) + { + } + + public override FilterExpression? OnApplyFilter(FilterExpression? existingFilter) + { + if (existingFilter != null) + { + var rewriter = new FilterTimeRangeRewriter(); + return (FilterExpression)rewriter.Visit(existingFilter, null)!; + } + + return existingFilter; + } +} diff --git a/test/JsonApiDotNetCoreTests/IntegrationTests/QueryStrings/FilterValueConversion/FilterTimeRangeRewriter.cs b/test/JsonApiDotNetCoreTests/IntegrationTests/QueryStrings/FilterValueConversion/FilterTimeRangeRewriter.cs new file mode 100644 index 0000000000..3e3c903b39 --- /dev/null +++ b/test/JsonApiDotNetCoreTests/IntegrationTests/QueryStrings/FilterValueConversion/FilterTimeRangeRewriter.cs @@ -0,0 +1,34 @@ +using JsonApiDotNetCore.Queries.Expressions; + +namespace JsonApiDotNetCoreTests.IntegrationTests.QueryStrings.FilterValueConversion; + +internal sealed class FilterTimeRangeRewriter : QueryExpressionRewriter +{ + private static readonly Dictionary InverseComparisonOperatorTable = new() + { + [ComparisonOperator.GreaterThan] = ComparisonOperator.LessThan, + [ComparisonOperator.GreaterOrEqual] = ComparisonOperator.LessOrEqual, + [ComparisonOperator.Equals] = ComparisonOperator.Equals, + [ComparisonOperator.LessThan] = ComparisonOperator.GreaterThan, + [ComparisonOperator.LessOrEqual] = ComparisonOperator.GreaterOrEqual + }; + + public override QueryExpression? VisitComparison(ComparisonExpression expression, object? argument) + { + if (expression.Right is LiteralConstantExpression { TypedValue: TimeRange timeRange }) + { + var offsetComparison = + new ComparisonExpression(timeRange.Offset < TimeSpan.Zero ? InverseComparisonOperatorTable[expression.Operator] : expression.Operator, + expression.Left, new LiteralConstantExpression(timeRange.Time + timeRange.Offset)); + + ComparisonExpression? timeComparison = expression.Operator is ComparisonOperator.LessThan or ComparisonOperator.LessOrEqual + ? new ComparisonExpression(timeRange.Offset < TimeSpan.Zero ? ComparisonOperator.LessOrEqual : ComparisonOperator.GreaterOrEqual, + expression.Left, new LiteralConstantExpression(timeRange.Time)) + : null; + + return timeComparison == null ? offsetComparison : new LogicalExpression(LogicalOperator.And, offsetComparison, timeComparison); + } + + return base.VisitComparison(expression, argument); + } +} diff --git a/test/JsonApiDotNetCoreTests/IntegrationTests/QueryStrings/FilterValueConversion/RelativeTimeFilterValueConverter.cs b/test/JsonApiDotNetCoreTests/IntegrationTests/QueryStrings/FilterValueConversion/RelativeTimeFilterValueConverter.cs new file mode 100644 index 0000000000..aa82daa4d0 --- /dev/null +++ b/test/JsonApiDotNetCoreTests/IntegrationTests/QueryStrings/FilterValueConversion/RelativeTimeFilterValueConverter.cs @@ -0,0 +1,54 @@ +using JsonApiDotNetCore.Queries.Expressions; +using JsonApiDotNetCore.Queries.Internal.Parsing; +using JsonApiDotNetCore.QueryStrings; +using JsonApiDotNetCore.Resources.Annotations; +using JsonApiDotNetCore.Resources.Internal; +using Microsoft.AspNetCore.Authentication; + +namespace JsonApiDotNetCoreTests.IntegrationTests.QueryStrings.FilterValueConversion; + +internal sealed class RelativeTimeFilterValueConverter : IFilterValueConverter +{ + private readonly ISystemClock _systemClock; + + public RelativeTimeFilterValueConverter(ISystemClock systemClock) + { + _systemClock = systemClock; + } + + public bool CanConvert(AttrAttribute attribute) + { + return attribute.Type.ClrType == typeof(Reminder) && attribute.Property.PropertyType == typeof(DateTime); + } + + public object Convert(AttrAttribute attribute, string value, Type outerExpressionType) + { + // A leading +/- indicates a relative value, based on the current time. + + if (value.Length > 1 && value[0] is '+' or '-') + { + if (outerExpressionType != typeof(ComparisonExpression)) + { + throw new QueryParseException("A relative time can only be used in a comparison function."); + } + + var timeSpan = ConvertStringValueTo(value[1..]); + TimeSpan offset = value[0] == '-' ? -timeSpan : timeSpan; + return new TimeRange(_systemClock.UtcNow.UtcDateTime, offset); + } + + return ConvertStringValueTo(value); + } + + private static T ConvertStringValueTo(string value) + { + try + { + return (T)RuntimeTypeConverter.ConvertType(value, typeof(T))!; + } + catch (FormatException exception) + { + throw new QueryParseException($"Failed to convert '{value}' of type 'String' to type '{typeof(T).Name}'.", exception); + } + } +} diff --git a/test/JsonApiDotNetCoreTests/IntegrationTests/QueryStrings/FilterValueConversion/RelativeTimeTests.cs b/test/JsonApiDotNetCoreTests/IntegrationTests/QueryStrings/FilterValueConversion/RelativeTimeTests.cs new file mode 100644 index 0000000000..435d60c661 --- /dev/null +++ b/test/JsonApiDotNetCoreTests/IntegrationTests/QueryStrings/FilterValueConversion/RelativeTimeTests.cs @@ -0,0 +1,184 @@ +using System.Net; +using FluentAssertions; +using FluentAssertions.Extensions; +using Humanizer; +using JsonApiDotNetCore.Queries.Expressions; +using JsonApiDotNetCore.QueryStrings; +using JsonApiDotNetCore.Resources; +using JsonApiDotNetCore.Serialization.Objects; +using Microsoft.AspNetCore.Authentication; +using Microsoft.Extensions.DependencyInjection; +using TestBuildingBlocks; +using Xunit; + +namespace JsonApiDotNetCoreTests.IntegrationTests.QueryStrings.FilterValueConversion; + +public sealed class RelativeTimeTests : IClassFixture, QueryStringDbContext>> +{ + private readonly IntegrationTestContext, QueryStringDbContext> _testContext; + private readonly QueryStringFakers _fakers = new(); + + public RelativeTimeTests(IntegrationTestContext, QueryStringDbContext> testContext) + { + _testContext = testContext; + + testContext.UseController(); + testContext.UseController(); + + testContext.ConfigureServicesBeforeStartup(services => + { + services.AddSingleton(); + services.AddSingleton(); + services.AddScoped(typeof(IResourceDefinition<,>), typeof(FilterRewritingResourceDefinition<,>)); + }); + } + + [Theory] + [InlineData("-0:10:00", ComparisonOperator.GreaterThan, "0")] // more than 10 minutes ago + [InlineData("-0:10:00", ComparisonOperator.GreaterOrEqual, "0,1")] // at least 10 minutes ago + [InlineData("-0:10:00", ComparisonOperator.Equals, "1")] // exactly 10 minutes ago + [InlineData("-0:10:00", ComparisonOperator.LessThan, "2,3")] // less than 10 minutes ago + [InlineData("-0:10:00", ComparisonOperator.LessOrEqual, "1,2,3")] // at most 10 minutes ago + [InlineData("+0:10:00", ComparisonOperator.GreaterThan, "6")] // more than 10 minutes in the future + [InlineData("+0:10:00", ComparisonOperator.GreaterOrEqual, "5,6")] // at least 10 minutes in the future + [InlineData("+0:10:00", ComparisonOperator.Equals, "5")] // in exactly 10 minutes + [InlineData("+0:10:00", ComparisonOperator.LessThan, "3,4")] // less than 10 minutes in the future + [InlineData("+0:10:00", ComparisonOperator.LessOrEqual, "3,4,5")] // at most 10 minutes in the future + public async Task Can_filter_comparison_on_relative_time(string filterValue, ComparisonOperator comparisonOperator, string matchingRowsExpected) + { + // Arrange + var clock = _testContext.Factory.Services.GetRequiredService(); + + List reminders = _fakers.Reminder.Generate(7); + reminders[0].RemindsAt = clock.UtcNow.Add(TimeSpan.FromMinutes(-15)).DateTime.AsUtc(); + reminders[1].RemindsAt = clock.UtcNow.Add(TimeSpan.FromMinutes(-10)).DateTime.AsUtc(); + reminders[2].RemindsAt = clock.UtcNow.Add(TimeSpan.FromMinutes(-5)).DateTime.AsUtc(); + reminders[3].RemindsAt = clock.UtcNow.Add(TimeSpan.FromMinutes(0)).DateTime.AsUtc(); + reminders[4].RemindsAt = clock.UtcNow.Add(TimeSpan.FromMinutes(5)).DateTime.AsUtc(); + reminders[5].RemindsAt = clock.UtcNow.Add(TimeSpan.FromMinutes(10)).DateTime.AsUtc(); + reminders[6].RemindsAt = clock.UtcNow.Add(TimeSpan.FromMinutes(15)).DateTime.AsUtc(); + + await _testContext.RunOnDatabaseAsync(async dbContext => + { + await dbContext.ClearTableAsync(); + dbContext.Reminders.AddRange(reminders); + await dbContext.SaveChangesAsync(); + }); + + string route = $"/reminders?filter={comparisonOperator.ToString().Camelize()}(remindsAt,'{filterValue.Replace("+", "%2B")}')"; + + // Act + (HttpResponseMessage httpResponse, Document responseDocument) = await _testContext.ExecuteGetAsync(route); + + // Assert + httpResponse.ShouldHaveStatusCode(HttpStatusCode.OK); + + int[] matchingRowIndices = matchingRowsExpected.Split(',').Select(int.Parse).ToArray(); + responseDocument.Data.ManyValue.ShouldHaveCount(matchingRowIndices.Length); + + foreach (int rowIndex in matchingRowIndices) + { + responseDocument.Data.ManyValue.Should().ContainSingle(resource => resource.Id == reminders[rowIndex].StringId); + } + } + + [Fact] + public async Task Cannot_filter_comparison_on_invalid_relative_time() + { + // Arrange + const string route = "/reminders?filter=equals(remindsAt,'-*')"; + + // Act + (HttpResponseMessage httpResponse, Document responseDocument) = await _testContext.ExecuteGetAsync(route); + + // Assert + httpResponse.ShouldHaveStatusCode(HttpStatusCode.BadRequest); + + responseDocument.Errors.ShouldHaveCount(1); + + ErrorObject error = responseDocument.Errors[0]; + error.StatusCode.Should().Be(HttpStatusCode.BadRequest); + error.Title.Should().Be("The specified filter is invalid."); + error.Detail.Should().Be("Failed to convert '*' of type 'String' to type 'TimeSpan'."); + error.Source.ShouldNotBeNull(); + error.Source.Parameter.Should().Be("filter"); + } + + [Fact] + public async Task Cannot_filter_any_on_relative_time() + { + // Arrange + const string route = "/reminders?filter=any(remindsAt,'-0:10:00')"; + + // Act + (HttpResponseMessage httpResponse, Document responseDocument) = await _testContext.ExecuteGetAsync(route); + + // Assert + httpResponse.ShouldHaveStatusCode(HttpStatusCode.BadRequest); + + responseDocument.Errors.ShouldHaveCount(1); + + ErrorObject error = responseDocument.Errors[0]; + error.StatusCode.Should().Be(HttpStatusCode.BadRequest); + error.Title.Should().Be("The specified filter is invalid."); + error.Detail.Should().Be("A relative time can only be used in a comparison function."); + error.Source.ShouldNotBeNull(); + error.Source.Parameter.Should().Be("filter"); + } + + [Fact] + public async Task Cannot_filter_text_match_on_relative_time() + { + // Arrange + const string route = "/reminders?filter=startsWith(remindsAt,'-0:10:00')"; + + // Act + (HttpResponseMessage httpResponse, Document responseDocument) = await _testContext.ExecuteGetAsync(route); + + // Assert + httpResponse.ShouldHaveStatusCode(HttpStatusCode.BadRequest); + + responseDocument.Errors.ShouldHaveCount(1); + + ErrorObject error = responseDocument.Errors[0]; + error.StatusCode.Should().Be(HttpStatusCode.BadRequest); + error.Title.Should().Be("The specified filter is invalid."); + error.Detail.Should().Be("A relative time can only be used in a comparison function."); + error.Source.ShouldNotBeNull(); + error.Source.Parameter.Should().Be("filter"); + } + + [Fact] + public async Task Can_filter_comparison_on_relative_time_in_nested_expression() + { + // Arrange + var clock = _testContext.Factory.Services.GetRequiredService(); + + Calendar calendar = _fakers.Calendar.Generate(); + calendar.Appointments = _fakers.Appointment.Generate(2).ToHashSet(); + + calendar.Appointments.ElementAt(0).Reminders = _fakers.Reminder.Generate(1); + calendar.Appointments.ElementAt(0).Reminders[0].RemindsAt = clock.UtcNow.DateTime.AsUtc(); + + calendar.Appointments.ElementAt(1).Reminders = _fakers.Reminder.Generate(1); + calendar.Appointments.ElementAt(1).Reminders[0].RemindsAt = clock.UtcNow.Add(TimeSpan.FromMinutes(30)).DateTime.AsUtc(); + + await _testContext.RunOnDatabaseAsync(async dbContext => + { + dbContext.Calendars.Add(calendar); + await dbContext.SaveChangesAsync(); + }); + + string route = $"/calendars/{calendar.StringId}/appointments?filter=has(reminders,equals(remindsAt,'%2B0:30:00'))"; + + // Act + (HttpResponseMessage httpResponse, Document responseDocument) = await _testContext.ExecuteGetAsync(route); + + // Assert + httpResponse.ShouldHaveStatusCode(HttpStatusCode.OK); + + responseDocument.Data.ManyValue.ShouldHaveCount(1); + + responseDocument.Data.ManyValue[0].Id.Should().Be(calendar.Appointments.ElementAt(1).StringId); + } +} diff --git a/test/JsonApiDotNetCoreTests/IntegrationTests/QueryStrings/FilterValueConversion/TimeRange.cs b/test/JsonApiDotNetCoreTests/IntegrationTests/QueryStrings/FilterValueConversion/TimeRange.cs new file mode 100644 index 0000000000..e17eda9f9a --- /dev/null +++ b/test/JsonApiDotNetCoreTests/IntegrationTests/QueryStrings/FilterValueConversion/TimeRange.cs @@ -0,0 +1,13 @@ +namespace JsonApiDotNetCoreTests.IntegrationTests.QueryStrings.FilterValueConversion; + +internal sealed class TimeRange +{ + public DateTime Time { get; } + public TimeSpan Offset { get; } + + public TimeRange(DateTime time, TimeSpan offset) + { + Time = time; + Offset = offset; + } +} diff --git a/test/JsonApiDotNetCoreTests/IntegrationTests/QueryStrings/QueryStringDbContext.cs b/test/JsonApiDotNetCoreTests/IntegrationTests/QueryStrings/QueryStringDbContext.cs index 473a7428ba..3aa40b2725 100644 --- a/test/JsonApiDotNetCoreTests/IntegrationTests/QueryStrings/QueryStringDbContext.cs +++ b/test/JsonApiDotNetCoreTests/IntegrationTests/QueryStrings/QueryStringDbContext.cs @@ -21,6 +21,7 @@ public sealed class QueryStringDbContext : TestableDbContext public DbSet LoginAttempts => Set(); public DbSet Calendars => Set(); public DbSet Appointments => Set(); + public DbSet Reminders => Set(); public QueryStringDbContext(DbContextOptions options) : base(options) diff --git a/test/JsonApiDotNetCoreTests/IntegrationTests/QueryStrings/QueryStringFakers.cs b/test/JsonApiDotNetCoreTests/IntegrationTests/QueryStrings/QueryStringFakers.cs index ab7fc4b77e..8fad2f6b90 100644 --- a/test/JsonApiDotNetCoreTests/IntegrationTests/QueryStrings/QueryStringFakers.cs +++ b/test/JsonApiDotNetCoreTests/IntegrationTests/QueryStrings/QueryStringFakers.cs @@ -70,6 +70,12 @@ internal sealed class QueryStringFakers : FakerContainer .TruncateToWholeMilliseconds()) .RuleFor(appointment => appointment.EndTime, (faker, appointment) => appointment.StartTime.AddHours(faker.Random.Double(1, 4)))); + private readonly Lazy> _lazyReminderFaker = new(() => + new Faker() + .UseSeed(GetFakerSeed()) + .RuleFor(reminder => reminder.RemindsAt, faker => faker.Date.Future() + .TruncateToWholeMilliseconds())); + public Faker Blog => _lazyBlogFaker.Value; public Faker BlogPost => _lazyBlogPostFaker.Value; public Faker