From a7668ae26a268c905e7d3337c154297904a55461 Mon Sep 17 00:00:00 2001 From: Bart Koelman <10324372+bkoelman@users.noreply.github.com> Date: Thu, 16 Feb 2023 01:41:33 +0100 Subject: [PATCH 01/14] Changes LiteralConstantExpression to contain a typed constant instead of a string value. This effectively moves the type resolving/conversion logic from the EF query production phase to the filter parsing phase. By providing a richer QueryLayer model, it becomes easier to implement non-relational and non-EF Core-based repositories. And it enables to produce better errors earlier. Note we still need to wrap nullable values (see WhereClauseBuilder.ResolveCommonType), due to https://bradwilson.typepad.com/blog/2008/07/creating-nullab.html. --- .../Expressions/LiteralConstantExpression.cs | 25 ++-- .../Queries/Internal/Parsing/FilterParser.cs | 120 ++++++++++-------- .../Queries/Internal/QueryLayerComposer.cs | 8 +- .../QueryableBuilding/WhereClauseBuilder.cs | 44 +++---- .../CompositeKeys/CarExpressionRewriter.cs | 7 +- .../Filtering/FilterDataTypeTests.cs | 7 +- .../Filtering/FilterOperatorTests.cs | 44 +++++++ 7 files changed, 156 insertions(+), 99 deletions(-) diff --git a/src/JsonApiDotNetCore/Queries/Expressions/LiteralConstantExpression.cs b/src/JsonApiDotNetCore/Queries/Expressions/LiteralConstantExpression.cs index 578643d5db..e8041f0cf2 100644 --- a/src/JsonApiDotNetCore/Queries/Expressions/LiteralConstantExpression.cs +++ b/src/JsonApiDotNetCore/Queries/Expressions/LiteralConstantExpression.cs @@ -8,13 +8,22 @@ namespace JsonApiDotNetCore.Queries.Expressions; [PublicAPI] public class LiteralConstantExpression : IdentifierExpression { - public string Value { get; } + private readonly string _stringValue; - public LiteralConstantExpression(string text) + public object TypedValue { get; } + + public LiteralConstantExpression(object typedValue) + : this(typedValue, typedValue.ToString()!) + { + } + + public LiteralConstantExpression(object typedValue, string stringValue) { - ArgumentGuard.NotNull(text); + ArgumentGuard.NotNull(typedValue); + ArgumentGuard.NotNull(stringValue); - Value = text; + TypedValue = typedValue; + _stringValue = stringValue; } public override TResult Accept(QueryExpressionVisitor visitor, TArgument argument) @@ -24,8 +33,8 @@ public override TResult Accept(QueryExpressionVisitor rightConstantValueConverter; + + if (leftTerm is CountExpression) + { + rightConstantValueConverter = GetConstantValueConverterForCount(); + } + else if (leftTerm is ResourceFieldChainExpression fieldChain && fieldChain.Fields[^1] is AttrAttribute attribute) + { + rightConstantValueConverter = GetConstantValueConverterForAttribute(attribute); + } + else + { + // This temporary value never survives; it gets discarded during the second pass below. + rightConstantValueConverter = _ => 0; + } EatSingleCharacterToken(TokenKind.Comma); - QueryExpression rightTerm = ParseCountOrConstantOrNullOrField(FieldChainRequirements.EndsInAttribute); + QueryExpression rightTerm = ParseCountOrConstantOrNullOrField(FieldChainRequirements.EndsInAttribute, rightConstantValueConverter); EatSingleCharacterToken(TokenKind.CloseParen); - if (leftTerm is ResourceFieldChainExpression leftChain) + if (leftTerm is ResourceFieldChainExpression leftChain && leftChain.Fields[^1] is RelationshipAttribute && rightTerm is not NullConstantExpression) { - if (leftChainRequirements.HasFlag(FieldChainRequirements.EndsInToOne) && rightTerm is not NullConstantExpression) - { - // Run another pass over left chain to have it fail when chain ends in relationship. - OnResolveFieldChain(leftChain.ToString(), FieldChainRequirements.EndsInAttribute); - } - - PropertyInfo leftProperty = leftChain.Fields[^1].Property; - - if (leftProperty.Name == nameof(Identifiable.Id) && rightTerm is LiteralConstantExpression rightConstant) - { - string id = DeObfuscateStringId(leftProperty.ReflectedType!, rightConstant.Value); - rightTerm = new LiteralConstantExpression(id); - } + // Run another pass over left chain to produce an error. + OnResolveFieldChain(leftChain.ToString(), FieldChainRequirements.EndsInAttribute); } return new ComparisonExpression(comparisonOperator, leftTerm, rightTerm); @@ -173,16 +177,23 @@ protected MatchTextExpression ParseTextMatch(string matchFunctionName) EatText(matchFunctionName); EatSingleCharacterToken(TokenKind.OpenParen); - ResourceFieldChainExpression targetAttribute = ParseFieldChain(FieldChainRequirements.EndsInAttribute, null); + 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."); + } EatSingleCharacterToken(TokenKind.Comma); - LiteralConstantExpression constant = ParseConstant(); + Converter constantValueConverter = stringValue => stringValue; + LiteralConstantExpression constant = ParseConstant(constantValueConverter); EatSingleCharacterToken(TokenKind.CloseParen); var matchKind = Enum.Parse(matchFunctionName.Pascalize()); - return new MatchTextExpression(targetAttribute, constant, matchKind); + return new MatchTextExpression(targetAttributeChain, constant, matchKind); } protected AnyExpression ParseAny() @@ -191,19 +202,20 @@ protected AnyExpression ParseAny() EatSingleCharacterToken(TokenKind.OpenParen); ResourceFieldChainExpression targetAttribute = ParseFieldChain(FieldChainRequirements.EndsInAttribute, null); + Converter constantValueConverter = GetConstantValueConverterForAttribute((AttrAttribute)targetAttribute.Fields[^1]); EatSingleCharacterToken(TokenKind.Comma); ImmutableHashSet.Builder constantsBuilder = ImmutableHashSet.CreateBuilder(); - LiteralConstantExpression constant = ParseConstant(); + LiteralConstantExpression constant = ParseConstant(constantValueConverter); constantsBuilder.Add(constant); while (TokenStack.TryPeek(out Token? nextToken) && nextToken.Kind == TokenKind.Comma) { EatSingleCharacterToken(TokenKind.Comma); - constant = ParseConstant(); + constant = ParseConstant(constantValueConverter); constantsBuilder.Add(constant); } @@ -211,32 +223,9 @@ protected AnyExpression ParseAny() IImmutableSet constantSet = constantsBuilder.ToImmutable(); - PropertyInfo targetAttributeProperty = targetAttribute.Fields[^1].Property; - - if (targetAttributeProperty.Name == nameof(Identifiable.Id)) - { - constantSet = DeObfuscateIdConstants(constantSet, targetAttributeProperty); - } - return new AnyExpression(targetAttribute, constantSet); } - private IImmutableSet DeObfuscateIdConstants(IImmutableSet constantSet, - PropertyInfo targetAttributeProperty) - { - ImmutableHashSet.Builder idConstantsBuilder = ImmutableHashSet.CreateBuilder(); - - foreach (LiteralConstantExpression idConstant in constantSet) - { - string stringId = idConstant.Value; - string id = DeObfuscateStringId(targetAttributeProperty.ReflectedType!, stringId); - - idConstantsBuilder.Add(new LiteralConstantExpression(id)); - } - - return idConstantsBuilder.ToImmutable(); - } - protected HasExpression ParseHas() { EatText(Keywords.Has); @@ -360,7 +349,7 @@ protected QueryExpression ParseCountOrField(FieldChainRequirements chainRequirem return ParseFieldChain(chainRequirements, "Count function or field name expected."); } - protected QueryExpression ParseCountOrConstantOrNullOrField(FieldChainRequirements chainRequirements) + protected QueryExpression ParseCountOrConstantOrNullOrField(FieldChainRequirements chainRequirements, Converter constantValueConverter) { CountExpression? count = TryParseCount(); @@ -369,7 +358,7 @@ protected QueryExpression ParseCountOrConstantOrNullOrField(FieldChainRequiremen return count; } - IdentifierExpression? constantOrNull = TryParseConstantOrNull(); + IdentifierExpression? constantOrNull = TryParseConstantOrNull(constantValueConverter); if (constantOrNull != null) { @@ -379,7 +368,7 @@ protected QueryExpression ParseCountOrConstantOrNullOrField(FieldChainRequiremen return ParseFieldChain(chainRequirements, "Count function, value between quotes, null or field name expected."); } - protected IdentifierExpression? TryParseConstantOrNull() + protected IdentifierExpression? TryParseConstantOrNull(Converter constantValueConverter) { if (TokenStack.TryPeek(out Token? nextToken)) { @@ -392,28 +381,55 @@ protected QueryExpression ParseCountOrConstantOrNullOrField(FieldChainRequiremen if (nextToken.Kind == TokenKind.QuotedText) { TokenStack.Pop(); - return new LiteralConstantExpression(nextToken.Value!); + + object constantValue = constantValueConverter(nextToken.Value!); + return new LiteralConstantExpression(constantValue, nextToken.Value!); } } return null; } - protected LiteralConstantExpression ParseConstant() + protected LiteralConstantExpression ParseConstant(Converter constantValueConverter) { if (TokenStack.TryPop(out Token? token) && token.Kind == TokenKind.QuotedText) { - return new LiteralConstantExpression(token.Value!); + object constantValue = constantValueConverter(token.Value!); + return new LiteralConstantExpression(constantValue, token.Value!); } throw new QueryParseException("Value between quotes expected."); } - private string DeObfuscateStringId(Type resourceClrType, string stringId) + private Converter GetConstantValueConverterForCount() + { + return stringValue => ConvertStringToType(stringValue, typeof(int)); + } + + private object ConvertStringToType(string value, Type type) + { + try + { + return RuntimeTypeConverter.ConvertType(value, type)!; + } + catch (FormatException) + { + throw new QueryParseException($"Failed to convert '{value}' of type 'String' to type '{type.Name}'."); + } + } + + private Converter GetConstantValueConverterForAttribute(AttrAttribute attribute) + { + return stringValue => attribute.Property.Name == nameof(IIdentifiable.Id) + ? DeObfuscateStringId(attribute.Type.ClrType, stringValue) + : ConvertStringToType(stringValue, attribute.Property.PropertyType); + } + + private object DeObfuscateStringId(Type resourceClrType, string stringId) { IIdentifiable tempResource = _resourceFactory.CreateInstance(resourceClrType); tempResource.StringId = stringId; - return tempResource.GetTypedId().ToString()!; + return tempResource.GetTypedId(); } protected override IImmutableList OnResolveFieldChain(string path, FieldChainRequirements chainRequirements) diff --git a/src/JsonApiDotNetCore/Queries/Internal/QueryLayerComposer.cs b/src/JsonApiDotNetCore/Queries/Internal/QueryLayerComposer.cs index 0a1ff48ca0..e22b4ba86b 100644 --- a/src/JsonApiDotNetCore/Queries/Internal/QueryLayerComposer.cs +++ b/src/JsonApiDotNetCore/Queries/Internal/QueryLayerComposer.cs @@ -114,14 +114,14 @@ private static FilterExpression GetInverseHasOneRelationshipFilter(TId prim AttrAttribute idAttribute = GetIdAttribute(relationship.LeftType); var idChain = new ResourceFieldChainExpression(ImmutableArray.Create(inverseRelationship, idAttribute)); - return new ComparisonExpression(ComparisonOperator.Equals, idChain, new LiteralConstantExpression(primaryId!.ToString()!)); + return new ComparisonExpression(ComparisonOperator.Equals, idChain, new LiteralConstantExpression(primaryId!)); } private static FilterExpression GetInverseHasManyRelationshipFilter(TId primaryId, HasManyAttribute relationship, HasManyAttribute inverseRelationship) { AttrAttribute idAttribute = GetIdAttribute(relationship.LeftType); var idChain = new ResourceFieldChainExpression(ImmutableArray.Create(idAttribute)); - var idComparison = new ComparisonExpression(ComparisonOperator.Equals, idChain, new LiteralConstantExpression(primaryId!.ToString()!)); + var idComparison = new ComparisonExpression(ComparisonOperator.Equals, idChain, new LiteralConstantExpression(primaryId!)); return new HasExpression(new ResourceFieldChainExpression(inverseRelationship), idComparison); } @@ -360,12 +360,12 @@ private IncludeExpression RewriteIncludeForSecondaryEndpoint(IncludeExpression? if (ids.Count == 1) { - var constant = new LiteralConstantExpression(ids.Single()!.ToString()!); + var constant = new LiteralConstantExpression(ids.Single()!); filter = new ComparisonExpression(ComparisonOperator.Equals, idChain, constant); } else if (ids.Count > 1) { - ImmutableHashSet constants = ids.Select(id => new LiteralConstantExpression(id!.ToString()!)).ToImmutableHashSet(); + ImmutableHashSet constants = ids.Select(id => new LiteralConstantExpression(id!)).ToImmutableHashSet(); filter = new AnyExpression(idChain, constants); } diff --git a/src/JsonApiDotNetCore/Queries/Internal/QueryableBuilding/WhereClauseBuilder.cs b/src/JsonApiDotNetCore/Queries/Internal/QueryableBuilding/WhereClauseBuilder.cs index 1198a488ff..87cd066a85 100644 --- a/src/JsonApiDotNetCore/Queries/Internal/QueryableBuilding/WhereClauseBuilder.cs +++ b/src/JsonApiDotNetCore/Queries/Internal/QueryableBuilding/WhereClauseBuilder.cs @@ -12,7 +12,7 @@ namespace JsonApiDotNetCore.Queries.Internal.QueryableBuilding; /// calls. /// [PublicAPI] -public class WhereClauseBuilder : QueryClauseBuilder +public class WhereClauseBuilder : QueryClauseBuilder { private static readonly CollectionConverter CollectionConverter = new(); private static readonly ConstantExpression NullConstant = Expression.Constant(null); @@ -53,7 +53,7 @@ private Expression WhereExtensionMethodCall(LambdaExpression predicate) return Expression.Call(_extensionType, "Where", LambdaScope.Parameter.Type.AsArray(), _source, predicate); } - public override Expression VisitHas(HasExpression expression, Type? argument) + public override Expression VisitHas(HasExpression expression, object? argument) { Expression property = Visit(expression.TargetCollection, argument); @@ -85,7 +85,7 @@ private static MethodCallExpression AnyExtensionMethodCall(Type elementType, Exp : Expression.Call(typeof(Enumerable), "Any", elementType.AsArray(), source); } - public override Expression VisitIsType(IsTypeExpression expression, Type? argument) + public override Expression VisitIsType(IsTypeExpression expression, object? argument) { Expression property = expression.TargetToOneRelationship != null ? Visit(expression.TargetToOneRelationship, argument) : LambdaScope.Accessor; TypeBinaryExpression typeCheck = Expression.TypeIs(property, expression.DerivedType.ClrType); @@ -101,7 +101,7 @@ public override Expression VisitIsType(IsTypeExpression expression, Type? argume return Expression.AndAlso(typeCheck, filter); } - public override Expression VisitMatchText(MatchTextExpression expression, Type? argument) + public override Expression VisitMatchText(MatchTextExpression expression, object? argument) { Expression property = Visit(expression.TargetAttribute, argument); @@ -125,7 +125,7 @@ public override Expression VisitMatchText(MatchTextExpression expression, Type? return Expression.Call(property, "Contains", null, text); } - public override Expression VisitAny(AnyExpression expression, Type? argument) + public override Expression VisitAny(AnyExpression expression, object? argument) { Expression property = Visit(expression.TargetAttribute, argument); @@ -133,8 +133,7 @@ public override Expression VisitAny(AnyExpression expression, Type? argument) foreach (LiteralConstantExpression constant in expression.Constants) { - object? value = ConvertTextToTargetType(constant.Value, property.Type); - valueList.Add(value); + valueList.Add(constant.TypedValue); } ConstantExpression collection = Expression.Constant(valueList); @@ -146,7 +145,7 @@ private static Expression ContainsExtensionMethodCall(Expression collection, Exp return Expression.Call(typeof(Enumerable), "Contains", value.Type.AsArray(), collection, value); } - public override Expression VisitLogical(LogicalExpression expression, Type? argument) + public override Expression VisitLogical(LogicalExpression expression, object? argument) { var termQueue = new Queue(expression.Terms.Select(filter => Visit(filter, argument))); @@ -179,18 +178,18 @@ private static BinaryExpression Compose(Queue argumentQueue, Func constant.Value).ToArray(); + string[] carStringIds = expression.Constants.Select(constant => (string)constant.TypedValue).ToArray(); return RewriteFilterOnCarStringIds(expression.TargetAttribute, carStringIds); } @@ -100,7 +101,7 @@ private FilterExpression CreateEqualityComparisonOnCompositeKey(ResourceFieldCha string licensePlateValue) { ResourceFieldChainExpression regionIdChain = ReplaceLastAttributeInChain(existingCarIdChain, _regionIdAttribute); - var regionIdComparison = new ComparisonExpression(ComparisonOperator.Equals, regionIdChain, new LiteralConstantExpression(regionIdValue.ToString())); + var regionIdComparison = new ComparisonExpression(ComparisonOperator.Equals, regionIdChain, new LiteralConstantExpression(regionIdValue)); ResourceFieldChainExpression licensePlateChain = ReplaceLastAttributeInChain(existingCarIdChain, _licensePlateAttribute); var licensePlateComparison = new ComparisonExpression(ComparisonOperator.Equals, licensePlateChain, new LiteralConstantExpression(licensePlateValue)); diff --git a/test/JsonApiDotNetCoreTests/IntegrationTests/QueryStrings/Filtering/FilterDataTypeTests.cs b/test/JsonApiDotNetCoreTests/IntegrationTests/QueryStrings/Filtering/FilterDataTypeTests.cs index b464a15083..b910b7e42e 100644 --- a/test/JsonApiDotNetCoreTests/IntegrationTests/QueryStrings/Filtering/FilterDataTypeTests.cs +++ b/test/JsonApiDotNetCoreTests/IntegrationTests/QueryStrings/Filtering/FilterDataTypeTests.cs @@ -304,7 +304,7 @@ await _testContext.RunOnDatabaseAsync(async dbContext => } [Fact] - public async Task Cannot_filter_equality_on_incompatible_value() + public async Task Cannot_filter_equality_on_incompatible_values() { // Arrange var resource = new FilterableResource @@ -331,9 +331,10 @@ await _testContext.RunOnDatabaseAsync(async dbContext => ErrorObject error = responseDocument.Errors[0]; error.StatusCode.Should().Be(HttpStatusCode.BadRequest); - error.Title.Should().Be("Query creation failed due to incompatible types."); + error.Title.Should().Be("The specified filter is invalid."); error.Detail.Should().Be("Failed to convert 'ABC' of type 'String' to type 'Int32'."); - error.Source.Should().BeNull(); + error.Source.ShouldNotBeNull(); + error.Source.Parameter.Should().Be("filter"); } [Theory] diff --git a/test/JsonApiDotNetCoreTests/IntegrationTests/QueryStrings/Filtering/FilterOperatorTests.cs b/test/JsonApiDotNetCoreTests/IntegrationTests/QueryStrings/Filtering/FilterOperatorTests.cs index b9fd8e2b2a..69dd7ca706 100644 --- a/test/JsonApiDotNetCoreTests/IntegrationTests/QueryStrings/Filtering/FilterOperatorTests.cs +++ b/test/JsonApiDotNetCoreTests/IntegrationTests/QueryStrings/Filtering/FilterOperatorTests.cs @@ -696,6 +696,28 @@ await _testContext.RunOnDatabaseAsync(async dbContext => responseDocument.Data.ManyValue[0].Attributes.ShouldContainKey("someString").With(value => value.Should().Be(resource.SomeString)); } + [Fact] + public async Task Cannot_filter_text_match_on_non_string_value() + { + // Arrange + const string route = "/filterableResources?filter=contains(someInt32,'123')"; + + // 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("Attribute of type 'String' expected."); + error.Source.ShouldNotBeNull(); + error.Source.Parameter.Should().Be("filter"); + } + [Theory] [InlineData("yes", "no", "'yes'")] [InlineData("two", "one two", "'one','two','three'")] @@ -842,6 +864,28 @@ await _testContext.RunOnDatabaseAsync(async dbContext => responseDocument.Data.ManyValue[0].Id.Should().Be(resource.StringId); } + [Fact] + public async Task Cannot_filter_on_count_with_incompatible_value() + { + // Arrange + const string route = "/filterableResources?filter=equals(count(children),'ABC')"; + + // 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 'ABC' of type 'String' to type 'Int32'."); + error.Source.ShouldNotBeNull(); + error.Source.Parameter.Should().Be("filter"); + } + [Theory] [InlineData("and(equals(someString,'ABC'),equals(someInt32,'11'))")] [InlineData("and(equals(someString,'ABC'),equals(someInt32,'11'),equals(someEnum,'Tuesday'))")] From a05a12e9d4bdb1ca43c8e3d55c7962d8abba7199 Mon Sep 17 00:00:00 2001 From: Bart Koelman <10324372+bkoelman@users.noreply.github.com> Date: Thu, 16 Feb 2023 19:59:40 +0100 Subject: [PATCH 02/14] Simplify test --- .../UnitTests/ResourceGraph/ResourceGraphBuilderTests.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/JsonApiDotNetCoreTests/UnitTests/ResourceGraph/ResourceGraphBuilderTests.cs b/test/JsonApiDotNetCoreTests/UnitTests/ResourceGraph/ResourceGraphBuilderTests.cs index 3fc221c190..97a35603b3 100644 --- a/test/JsonApiDotNetCoreTests/UnitTests/ResourceGraph/ResourceGraphBuilderTests.cs +++ b/test/JsonApiDotNetCoreTests/UnitTests/ResourceGraph/ResourceGraphBuilderTests.cs @@ -361,7 +361,7 @@ public void Can_override_capabilities_on_Id_property() IResourceGraph resourceGraph = builder.Build(); ResourceType resourceType = resourceGraph.GetResourceType(); - AttrAttribute idAttribute = resourceType.Attributes.Single(attr => attr.Property.Name == nameof(Identifiable.Id)); + AttrAttribute idAttribute = resourceType.GetAttributeByPropertyName(nameof(Identifiable.Id)); idAttribute.Capabilities.Should().Be(AttrCapabilities.AllowFilter); } From bcc297515ef03d530395a2300747ca8891bbc0ce Mon Sep 17 00:00:00 2001 From: Bart Koelman <10324372+bkoelman@users.noreply.github.com> Date: Fri, 24 Feb 2023 02:24:48 +0100 Subject: [PATCH 03/14] Removed pointless assertion (the preceeding code casts to HasManyAttribute, so this can never fail) --- .../Repositories/EntityFrameworkCoreRepository.cs | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/JsonApiDotNetCore/Repositories/EntityFrameworkCoreRepository.cs b/src/JsonApiDotNetCore/Repositories/EntityFrameworkCoreRepository.cs index 7cdd114301..50c300b036 100644 --- a/src/JsonApiDotNetCore/Repositories/EntityFrameworkCoreRepository.cs +++ b/src/JsonApiDotNetCore/Repositories/EntityFrameworkCoreRepository.cs @@ -529,8 +529,6 @@ public virtual async Task RemoveFromToManyRelationshipAsync(TResource leftResour if (!rightResourceIdsToStore.SetEquals(rightResourceIdsStored)) { - AssertIsNotClearingRequiredToOneRelationship(relationship, leftResourceTracked, rightResourceIdsToStore); - await UpdateRelationshipAsync(relationship, leftResourceTracked, rightResourceIdsToStore, cancellationToken); await _resourceDefinitionAccessor.OnWritingAsync(leftResourceTracked, WriteOperationKind.RemoveFromRelationship, cancellationToken); From 2bf940566173a32c2db3c34fafda3a06a2eb67df Mon Sep 17 00:00:00 2001 From: Bart Koelman <10324372+bkoelman@users.noreply.github.com> Date: Fri, 24 Feb 2023 02:30:04 +0100 Subject: [PATCH 04/14] Removed resource ID from error message, as the error is unrelated to any specific record --- .../Errors/CannotClearRequiredRelationshipException.cs | 5 ++--- .../Repositories/EntityFrameworkCoreRepository.cs | 8 ++++---- .../ModelState/NoModelStateValidationTests.cs | 4 +--- .../RequiredRelationships/DefaultBehaviorTests.cs | 4 +--- 4 files changed, 8 insertions(+), 13 deletions(-) diff --git a/src/JsonApiDotNetCore/Errors/CannotClearRequiredRelationshipException.cs b/src/JsonApiDotNetCore/Errors/CannotClearRequiredRelationshipException.cs index b73b256d25..97377b0d7b 100644 --- a/src/JsonApiDotNetCore/Errors/CannotClearRequiredRelationshipException.cs +++ b/src/JsonApiDotNetCore/Errors/CannotClearRequiredRelationshipException.cs @@ -10,12 +10,11 @@ namespace JsonApiDotNetCore.Errors; [PublicAPI] public sealed class CannotClearRequiredRelationshipException : JsonApiException { - public CannotClearRequiredRelationshipException(string relationshipName, string resourceId, string resourceType) + public CannotClearRequiredRelationshipException(string relationshipName, string resourceType) : base(new ErrorObject(HttpStatusCode.BadRequest) { Title = "Failed to clear a required relationship.", - Detail = $"The relationship '{relationshipName}' on resource type '{resourceType}' " + - $"with ID '{resourceId}' cannot be cleared because it is a required relationship." + Detail = $"The relationship '{relationshipName}' on resource type '{resourceType}' cannot be cleared because it is a required relationship." }) { } diff --git a/src/JsonApiDotNetCore/Repositories/EntityFrameworkCoreRepository.cs b/src/JsonApiDotNetCore/Repositories/EntityFrameworkCoreRepository.cs index 50c300b036..074e902341 100644 --- a/src/JsonApiDotNetCore/Repositories/EntityFrameworkCoreRepository.cs +++ b/src/JsonApiDotNetCore/Repositories/EntityFrameworkCoreRepository.cs @@ -269,7 +269,7 @@ public virtual async Task UpdateAsync(TResource resourceFromRequest, TResource r object? rightValueEvaluated = await VisitSetRelationshipAsync(resourceFromDatabase, relationship, rightValue, WriteOperationKind.UpdateResource, cancellationToken); - AssertIsNotClearingRequiredToOneRelationship(relationship, resourceFromDatabase, rightValueEvaluated); + AssertIsNotClearingRequiredToOneRelationship(relationship, rightValueEvaluated); await UpdateRelationshipAsync(relationship, resourceFromDatabase, rightValueEvaluated, cancellationToken); } @@ -288,7 +288,7 @@ public virtual async Task UpdateAsync(TResource resourceFromRequest, TResource r _dbContext.ResetChangeTracker(); } - protected void AssertIsNotClearingRequiredToOneRelationship(RelationshipAttribute relationship, TResource leftResource, object? rightValue) + protected void AssertIsNotClearingRequiredToOneRelationship(RelationshipAttribute relationship, object? rightValue) { if (relationship is HasOneAttribute) { @@ -300,7 +300,7 @@ protected void AssertIsNotClearingRequiredToOneRelationship(RelationshipAttribut if (isRelationshipRequired && isClearingRelationship) { string resourceName = _resourceGraph.GetResourceType().PublicName; - throw new CannotClearRequiredRelationshipException(relationship.PublicName, leftResource.StringId!, resourceName); + throw new CannotClearRequiredRelationshipException(relationship.PublicName, resourceName); } } } @@ -403,7 +403,7 @@ public virtual async Task SetRelationshipAsync(TResource leftResource, object? r object? rightValueEvaluated = await VisitSetRelationshipAsync(leftResource, relationship, rightValue, WriteOperationKind.SetRelationship, cancellationToken); - AssertIsNotClearingRequiredToOneRelationship(relationship, leftResource, rightValueEvaluated); + AssertIsNotClearingRequiredToOneRelationship(relationship, rightValueEvaluated); await UpdateRelationshipAsync(relationship, leftResource, rightValueEvaluated, cancellationToken); diff --git a/test/JsonApiDotNetCoreTests/IntegrationTests/InputValidation/ModelState/NoModelStateValidationTests.cs b/test/JsonApiDotNetCoreTests/IntegrationTests/InputValidation/ModelState/NoModelStateValidationTests.cs index 671123930e..6cd623ac94 100644 --- a/test/JsonApiDotNetCoreTests/IntegrationTests/InputValidation/ModelState/NoModelStateValidationTests.cs +++ b/test/JsonApiDotNetCoreTests/IntegrationTests/InputValidation/ModelState/NoModelStateValidationTests.cs @@ -128,8 +128,6 @@ await _testContext.RunOnDatabaseAsync(async dbContext => ErrorObject error = responseDocument.Errors[0]; error.StatusCode.Should().Be(HttpStatusCode.BadRequest); error.Title.Should().Be("Failed to clear a required relationship."); - - error.Detail.Should().Be($"The relationship 'rootDirectory' on resource type 'systemVolumes' with ID '{existingVolume.StringId}' " + - "cannot be cleared because it is a required relationship."); + error.Detail.Should().Be("The relationship 'rootDirectory' on resource type 'systemVolumes' cannot be cleared because it is a required relationship."); } } diff --git a/test/JsonApiDotNetCoreTests/IntegrationTests/RequiredRelationships/DefaultBehaviorTests.cs b/test/JsonApiDotNetCoreTests/IntegrationTests/RequiredRelationships/DefaultBehaviorTests.cs index 3e1dfcaef0..5828de90d3 100644 --- a/test/JsonApiDotNetCoreTests/IntegrationTests/RequiredRelationships/DefaultBehaviorTests.cs +++ b/test/JsonApiDotNetCoreTests/IntegrationTests/RequiredRelationships/DefaultBehaviorTests.cs @@ -256,9 +256,7 @@ await _testContext.RunOnDatabaseAsync(async dbContext => ErrorObject error = responseDocument.Errors[0]; error.StatusCode.Should().Be(HttpStatusCode.BadRequest); error.Title.Should().Be("Failed to clear a required relationship."); - - error.Detail.Should().Be($"The relationship 'customer' on resource type 'orders' with ID '{existingOrder.StringId}' " + - "cannot be cleared because it is a required relationship."); + error.Detail.Should().Be("The relationship 'customer' on resource type 'orders' cannot be cleared because it is a required relationship."); } [Fact] From 1c9a7c97c995da5c3f27f1d9f2e88abcaf7aaa3c Mon Sep 17 00:00:00 2001 From: Bart Koelman <10324372+bkoelman@users.noreply.github.com> Date: Fri, 24 Feb 2023 09:44:10 +0100 Subject: [PATCH 05/14] Fix naming/comment --- .../Internal/QueryableBuilding/SelectClauseBuilder.cs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/JsonApiDotNetCore/Queries/Internal/QueryableBuilding/SelectClauseBuilder.cs b/src/JsonApiDotNetCore/Queries/Internal/QueryableBuilding/SelectClauseBuilder.cs index 1f1c10301a..9c7c7f9d81 100644 --- a/src/JsonApiDotNetCore/Queries/Internal/QueryableBuilding/SelectClauseBuilder.cs +++ b/src/JsonApiDotNetCore/Queries/Internal/QueryableBuilding/SelectClauseBuilder.cs @@ -136,10 +136,10 @@ private ICollection ToPropertySelectors(FieldSelectors fieldSe if (fieldSelectors.ContainsReadOnlyAttribute || fieldSelectors.ContainsOnlyRelationships) { - // If a read-only attribute is selected, its calculated value likely depends on another property, so select all properties. - // And only selecting relationships implicitly means to select all attributes too. + // If a read-only attribute is selected, its calculated value likely depends on another property, so fetch all scalar properties. + // And only selecting relationships implicitly means to fetch all scalar properties as well. - IncludeAllAttributes(elementType, propertySelectors); + IncludeAllScalarProperties(elementType, propertySelectors); } IncludeFields(fieldSelectors, propertySelectors); @@ -148,7 +148,7 @@ private ICollection ToPropertySelectors(FieldSelectors fieldSe return propertySelectors.Values; } - private void IncludeAllAttributes(Type elementType, Dictionary propertySelectors) + private void IncludeAllScalarProperties(Type elementType, Dictionary propertySelectors) { IEntityType entityModel = _entityModel.GetEntityTypes().Single(type => type.ClrType == elementType); IEnumerable entityProperties = entityModel.GetProperties().Where(property => !property.IsShadowProperty()).ToArray(); From 9e1f9dcedd6eb7ca707ad6e58c35623676a01d78 Mon Sep 17 00:00:00 2001 From: Bart Koelman <10324372+bkoelman@users.noreply.github.com> Date: Fri, 24 Feb 2023 16:57:48 +0100 Subject: [PATCH 06/14] Apply new suggestions from Resharper EAP --- .../Configuration/ResourceDescriptorAssemblyCache.cs | 5 +---- .../Middleware/JsonApiRoutingConvention.cs | 8 ++++---- .../Internal/QueryableBuilding/SelectClauseBuilder.cs | 5 +---- .../Internal/PaginationQueryStringParameterReader.cs | 6 +----- .../Serialization/Response/MetaBuilder.cs | 2 +- 5 files changed, 8 insertions(+), 18 deletions(-) diff --git a/src/JsonApiDotNetCore/Configuration/ResourceDescriptorAssemblyCache.cs b/src/JsonApiDotNetCore/Configuration/ResourceDescriptorAssemblyCache.cs index e73b48ee3d..0e9f5d753f 100644 --- a/src/JsonApiDotNetCore/Configuration/ResourceDescriptorAssemblyCache.cs +++ b/src/JsonApiDotNetCore/Configuration/ResourceDescriptorAssemblyCache.cs @@ -12,10 +12,7 @@ internal sealed class ResourceDescriptorAssemblyCache public void RegisterAssembly(Assembly assembly) { - if (!_resourceDescriptorsPerAssembly.ContainsKey(assembly)) - { - _resourceDescriptorsPerAssembly[assembly] = null; - } + _resourceDescriptorsPerAssembly.TryAdd(assembly, null); } public IReadOnlyCollection GetResourceDescriptors() diff --git a/src/JsonApiDotNetCore/Middleware/JsonApiRoutingConvention.cs b/src/JsonApiDotNetCore/Middleware/JsonApiRoutingConvention.cs index 5c6b84cba7..a6ef712adf 100644 --- a/src/JsonApiDotNetCore/Middleware/JsonApiRoutingConvention.cs +++ b/src/JsonApiDotNetCore/Middleware/JsonApiRoutingConvention.cs @@ -101,10 +101,10 @@ public void Apply(ApplicationModel application) $"resource type '{resourceClrType}', which does not exist in the resource graph."); } - if (_controllerPerResourceTypeMap.ContainsKey(resourceType)) + if (_controllerPerResourceTypeMap.TryGetValue(resourceType, out ControllerModel? existingModel)) { throw new InvalidConfigurationException( - $"Multiple controllers found for resource type '{resourceType}': '{_controllerPerResourceTypeMap[resourceType].ControllerType}' and '{controller.ControllerType}'."); + $"Multiple controllers found for resource type '{resourceType}': '{existingModel.ControllerType}' and '{controller.ControllerType}'."); } _resourceTypePerControllerTypeMap.Add(controller.ControllerType, resourceType); @@ -119,10 +119,10 @@ public void Apply(ApplicationModel application) string template = TemplateFromResource(controller) ?? TemplateFromController(controller); - if (_registeredControllerNameByTemplate.ContainsKey(template)) + if (_registeredControllerNameByTemplate.TryGetValue(template, out string? controllerName)) { throw new InvalidConfigurationException( - $"Cannot register '{controller.ControllerType.FullName}' for template '{template}' because '{_registeredControllerNameByTemplate[template]}' was already registered for this template."); + $"Cannot register '{controller.ControllerType.FullName}' for template '{template}' because '{controllerName}' was already registered for this template."); } _registeredControllerNameByTemplate.Add(template, controller.ControllerType.FullName!); diff --git a/src/JsonApiDotNetCore/Queries/Internal/QueryableBuilding/SelectClauseBuilder.cs b/src/JsonApiDotNetCore/Queries/Internal/QueryableBuilding/SelectClauseBuilder.cs index 9c7c7f9d81..6a4f43d7e1 100644 --- a/src/JsonApiDotNetCore/Queries/Internal/QueryableBuilding/SelectClauseBuilder.cs +++ b/src/JsonApiDotNetCore/Queries/Internal/QueryableBuilding/SelectClauseBuilder.cs @@ -185,10 +185,7 @@ private static void IncludeEagerLoads(ResourceType resourceType, Dictionary values) { ArgumentGuard.NotNull(values); - _meta = values.Keys.Union(_meta.Keys).ToDictionary(key => key, key => values.ContainsKey(key) ? values[key] : _meta[key]); + _meta = values.Keys.Union(_meta.Keys).ToDictionary(key => key, key => values.TryGetValue(key, out object? value) ? value : _meta[key]); } /// From 82f04b29c0f16aa6675dc1e4663553af281d60d4 Mon Sep 17 00:00:00 2001 From: Bart Koelman <10324372+bkoelman@users.noreply.github.com> Date: Sun, 26 Feb 2023 09:42:20 +0100 Subject: [PATCH 07/14] Improve change tracking performance | Method | Mean | Error | StdDev | Ratio | Gen0 | Allocated | Alloc Ratio | |---------------- |---------:|----------:|----------:|------:|-------:|----------:|------------:| | TrackChanges | 3.741 us | 0.0122 us | 0.0114 us | 1.00 | 0.0229 | 6.41 KB | 1.00 | | NewTrackChanges | 1.359 us | 0.0070 us | 0.0066 us | 0.36 | 0.0095 | 2.62 KB | 0.41 | using BenchmarkDotNet.Attributes; using JsonApiDotNetCore.Configuration; using JsonApiDotNetCore.Middleware; using JsonApiDotNetCore.Resources; using JsonApiDotNetCore.Resources.Annotations; using Microsoft.Extensions.Logging.Abstractions; namespace Benchmarks.ChangeTracking; // ReSharper disable once ClassCanBeSealed.Global [MarkdownExporter] [MemoryDiagnoser] public class ChangeTrackerBenchmarks { private readonly JsonApiRequest _request; private readonly TargetedFields _targetedFields; public ChangeTrackerBenchmarks() { IJsonApiOptions options = new JsonApiOptions(); IResourceGraph resourceGraph = new ResourceGraphBuilder(options, NullLoggerFactory.Instance).Add().Build(); ResourceType resourceType = resourceGraph.GetResourceType(); _request = new JsonApiRequest { PrimaryResourceType = resourceType, IsCollection = true }; _targetedFields = new TargetedFields(); foreach (AttrAttribute attribute in resourceType.Attributes) { _targetedFields.Attributes.Add(attribute); } } [Benchmark(Baseline = true)] public void TrackChanges() { var changeTracker = new ResourceChangeTracker(_request, _targetedFields); var resource = new ExampleResource { Id = 1, Attr1 = "some", Attr2 = "more", Attr3 = "other", Attr4 = false, Attr5 = 123, Attr6 = default, Attr7 = default, Attr8 = default, Attr9 = DayOfWeek.Sunday }; changeTracker.SetInitiallyStoredAttributeValues(resource); resource = new ExampleResource { Id = 1, Attr1 = "new", Attr2 = "change", Attr3 = "this", Attr4 = true, Attr5 = 456, Attr6 = default, Attr7 = default, Attr8 = default, Attr9 = DayOfWeek.Saturday }; changeTracker.SetFinallyStoredAttributeValues(resource); changeTracker.HasImplicitChanges(); } [Benchmark] public void NewTrackChanges() { var changeTracker = new NewResourceChangeTracker(_request, _targetedFields); var resource = new ExampleResource { Id = 1, Attr1 = "some", Attr2 = "more", Attr3 = "other", Attr4 = false, Attr5 = 123, Attr6 = default, Attr7 = default, Attr8 = default, Attr9 = DayOfWeek.Sunday }; changeTracker.SetInitiallyStoredAttributeValues(resource); resource = new ExampleResource { Id = 1, Attr1 = "new", Attr2 = "change", Attr3 = "this", Attr4 = true, Attr5 = 456, Attr6 = default, Attr7 = default, Attr8 = default, Attr9 = DayOfWeek.Saturday }; changeTracker.SetFinallyStoredAttributeValues(resource); changeTracker.HasImplicitChanges(); } private sealed class ExampleResource : Identifiable { [Attr] public string? Attr1 { get; set; } [Attr] public string? Attr2 { get; set; } [Attr] public string? Attr3 { get; set; } [Attr] public bool Attr4 { get; set; } [Attr] public int Attr5 { get; set; } [Attr] public DateTime Attr6 { get; set; } [Attr] public DateTimeOffset Attr7 { get; set; } [Attr] public TimeSpan Attr8 { get; set; } [Attr] public DayOfWeek Attr9 { get; set; } } } --- .../Resources/ResourceChangeTracker.cs | 26 +++++++++---------- 1 file changed, 12 insertions(+), 14 deletions(-) diff --git a/src/JsonApiDotNetCore/Resources/ResourceChangeTracker.cs b/src/JsonApiDotNetCore/Resources/ResourceChangeTracker.cs index 89ba115a64..bcb9648320 100644 --- a/src/JsonApiDotNetCore/Resources/ResourceChangeTracker.cs +++ b/src/JsonApiDotNetCore/Resources/ResourceChangeTracker.cs @@ -1,4 +1,3 @@ -using System.Text.Json; using JetBrains.Annotations; using JsonApiDotNetCore.Middleware; using JsonApiDotNetCore.Resources.Annotations; @@ -13,9 +12,9 @@ public sealed class ResourceChangeTracker : IResourceChangeTracker? _initiallyStoredAttributeValues; - private IDictionary? _requestAttributeValues; - private IDictionary? _finallyStoredAttributeValues; + private IDictionary? _initiallyStoredAttributeValues; + private IDictionary? _requestAttributeValues; + private IDictionary? _finallyStoredAttributeValues; public ResourceChangeTracker(IJsonApiRequest request, ITargetedFields targetedFields) { @@ -50,15 +49,14 @@ public void SetFinallyStoredAttributeValues(TResource resource) _finallyStoredAttributeValues = CreateAttributeDictionary(resource, _request.PrimaryResourceType!.Attributes); } - private IDictionary CreateAttributeDictionary(TResource resource, IEnumerable attributes) + private IDictionary CreateAttributeDictionary(TResource resource, IEnumerable attributes) { - var result = new Dictionary(); + var result = new Dictionary(); foreach (AttrAttribute attribute in attributes) { object? value = attribute.GetValue(resource); - string json = JsonSerializer.Serialize(value); - result.Add(attribute.PublicName, json); + result.Add(attribute.PublicName, value); } return result; @@ -71,21 +69,21 @@ public bool HasImplicitChanges() { foreach (string key in _initiallyStoredAttributeValues.Keys) { - if (_requestAttributeValues.TryGetValue(key, out string? requestValue)) + if (_requestAttributeValues.TryGetValue(key, out object? requestValue)) { - string actualValue = _finallyStoredAttributeValues[key]; + object? actualValue = _finallyStoredAttributeValues[key]; - if (requestValue != actualValue) + if (!Equals(requestValue, actualValue)) { return true; } } else { - string initiallyStoredValue = _initiallyStoredAttributeValues[key]; - string finallyStoredValue = _finallyStoredAttributeValues[key]; + object? initiallyStoredValue = _initiallyStoredAttributeValues[key]; + object? finallyStoredValue = _finallyStoredAttributeValues[key]; - if (initiallyStoredValue != finallyStoredValue) + if (!Equals(initiallyStoredValue, finallyStoredValue)) { return true; } From 8f19d63826ccd2f561b9d2255086daeb0a52ce3d Mon Sep 17 00:00:00 2001 From: Bart Koelman <10324372+bkoelman@users.noreply.github.com> Date: Sun, 26 Feb 2023 08:49:57 +0100 Subject: [PATCH 08/14] Add workaround for failing 'dotnet pack' Caused by an unintentional breaking change in .NET SDK v7.0.200, which the latest AppVeyor image uses --- appveyor.yml | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/appveyor.yml b/appveyor.yml index 3328af0009..befc9d9154 100644 --- a/appveyor.yml +++ b/appveyor.yml @@ -1,6 +1,8 @@ image: - Ubuntu2004 - - Visual Studio 2022 + # Downgrade to workaround error NETSDK1194 during 'dotnet pack': The "--output" option isn't supported when building a solution. + # https://stackoverflow.com/questions/75453953/how-to-fix-github-actions-dotnet-publish-workflow-error-the-output-option-i + - Previous Visual Studio 2022 version: '{build}' @@ -32,7 +34,7 @@ for: - matrix: only: - - image: Visual Studio 2022 + - image: Previous Visual Studio 2022 services: - postgresql15 install: From d1ea9adf5bd61cd18e0aed11a22d68addb866ecf Mon Sep 17 00:00:00 2001 From: Bart Koelman <10324372+bkoelman@users.noreply.github.com> Date: Sun, 26 Feb 2023 10:08:13 +0100 Subject: [PATCH 09/14] Fixed: a duplicate trailing slash was rendered in Location header when request path ended with a slash. For example: /workItems/ => /workItems//1 --- src/JsonApiDotNetCore/Controllers/BaseJsonApiController.cs | 2 +- .../ReadWrite/Creating/CreateResourceTests.cs | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/JsonApiDotNetCore/Controllers/BaseJsonApiController.cs b/src/JsonApiDotNetCore/Controllers/BaseJsonApiController.cs index 22efab2840..fb3cd2bd2d 100644 --- a/src/JsonApiDotNetCore/Controllers/BaseJsonApiController.cs +++ b/src/JsonApiDotNetCore/Controllers/BaseJsonApiController.cs @@ -207,7 +207,7 @@ public virtual async Task PostAsync([FromBody] TResource resource TResource? newResource = await _create.CreateAsync(resource, cancellationToken); string resourceId = (newResource ?? resource).StringId!; - string locationUrl = $"{HttpContext.Request.Path}/{resourceId}"; + string locationUrl = HttpContext.Request.Path.Add($"/{resourceId}"); if (newResource == null) { diff --git a/test/JsonApiDotNetCoreTests/IntegrationTests/ReadWrite/Creating/CreateResourceTests.cs b/test/JsonApiDotNetCoreTests/IntegrationTests/ReadWrite/Creating/CreateResourceTests.cs index 587b7d8277..da8c60a34f 100644 --- a/test/JsonApiDotNetCoreTests/IntegrationTests/ReadWrite/Creating/CreateResourceTests.cs +++ b/test/JsonApiDotNetCoreTests/IntegrationTests/ReadWrite/Creating/CreateResourceTests.cs @@ -49,7 +49,7 @@ public async Task Sets_location_header_for_created_resource() } }; - const string route = "/workItems"; + const string route = "/workItems/"; // Act (HttpResponseMessage httpResponse, Document responseDocument) = await _testContext.ExecutePostAsync(route, requestBody); @@ -61,7 +61,7 @@ public async Task Sets_location_header_for_created_resource() httpResponse.Headers.Location.Should().Be($"/workItems/{newWorkItemId}"); responseDocument.Links.ShouldNotBeNull(); - responseDocument.Links.Self.Should().Be("http://localhost/workItems"); + responseDocument.Links.Self.Should().Be("http://localhost/workItems/"); responseDocument.Links.First.Should().BeNull(); responseDocument.Data.SingleValue.ShouldNotBeNull(); From eae0e780d8417923a963380189c8e3e97134ec1f Mon Sep 17 00:00:00 2001 From: Bart Koelman <10324372+bkoelman@users.noreply.github.com> Date: Tue, 28 Feb 2023 11:33:11 +0100 Subject: [PATCH 10/14] Update usages of "paging" with "pagination" --- docs/usage/options.md | 4 ++-- .../Resources/Annotations/LinkTypes.shared.cs | 4 ++-- .../Configuration/IJsonApiOptions.cs | 2 +- .../Queries/IPaginationContext.cs | 13 +++++----- .../PaginationQueryStringParameterReader.cs | 2 +- .../Resources/IResourceDefinition.cs | 2 +- .../Serialization/Response/LinkBuilder.cs | 2 +- .../PaginationWithTotalCountTests.cs | 10 ++++---- .../Pagination/RangeValidationTests.cs | 6 ++--- .../RangeValidationWithMaximumTests.cs | 6 ++--- .../UnitTests/Links/LinkInclusionTests.cs | 24 +++++++++---------- .../PaginationParseTests.cs | 4 ++-- 12 files changed, 40 insertions(+), 39 deletions(-) diff --git a/docs/usage/options.md b/docs/usage/options.md index 3512a9d9f2..6c896b9698 100644 --- a/docs/usage/options.md +++ b/docs/usage/options.md @@ -22,7 +22,7 @@ options.AllowClientGeneratedIds = true; ## Pagination -The default page size used for all resources can be overridden in options (10 by default). To disable paging, set it to `null`. +The default page size used for all resources can be overridden in options (10 by default). To disable pagination, set it to `null`. The maximum page size and number allowed from client requests can be set too (unconstrained by default). You can also include the total number of resources in each response. @@ -38,7 +38,7 @@ options.IncludeTotalResourceCount = true; ``` To retrieve the total number of resources on secondary and relationship endpoints, the reverse of the relationship must to be available. For example, in `GET /customers/1/orders`, both the relationships `[HasMany] Customer.Orders` and `[HasOne] Order.Customer` must be defined. -If `IncludeTotalResourceCount` is set to `false` (or the inverse relationship is unavailable on a non-primary endpoint), best-effort paging links are returned instead. This means no `last` link and the `next` link only occurs when the current page is full. +If `IncludeTotalResourceCount` is set to `false` (or the inverse relationship is unavailable on a non-primary endpoint), best-effort pagination links are returned instead. This means no `last` link and the `next` link only occurs when the current page is full. ## Relative Links diff --git a/src/JsonApiDotNetCore.Annotations/Resources/Annotations/LinkTypes.shared.cs b/src/JsonApiDotNetCore.Annotations/Resources/Annotations/LinkTypes.shared.cs index 61c7e9d927..7e996828b9 100644 --- a/src/JsonApiDotNetCore.Annotations/Resources/Annotations/LinkTypes.shared.cs +++ b/src/JsonApiDotNetCore.Annotations/Resources/Annotations/LinkTypes.shared.cs @@ -5,8 +5,8 @@ public enum LinkTypes { Self = 1 << 0, Related = 1 << 1, - Paging = 1 << 2, + Pagination = 1 << 2, NotConfigured = 1 << 3, None = 1 << 4, - All = Self | Related | Paging + All = Self | Related | Pagination } diff --git a/src/JsonApiDotNetCore/Configuration/IJsonApiOptions.cs b/src/JsonApiDotNetCore/Configuration/IJsonApiOptions.cs index bc7d17d89f..fcec2af464 100644 --- a/src/JsonApiDotNetCore/Configuration/IJsonApiOptions.cs +++ b/src/JsonApiDotNetCore/Configuration/IJsonApiOptions.cs @@ -99,7 +99,7 @@ public interface IJsonApiOptions bool IncludeTotalResourceCount { get; } /// - /// The page size (10 by default) that is used when not specified in query string. Set to null to not use paging by default. + /// The page size (10 by default) that is used when not specified in query string. Set to null to not use pagination by default. /// PageSize? DefaultPageSize { get; } diff --git a/src/JsonApiDotNetCore/Queries/IPaginationContext.cs b/src/JsonApiDotNetCore/Queries/IPaginationContext.cs index 44578e5277..e39b3ca354 100644 --- a/src/JsonApiDotNetCore/Queries/IPaginationContext.cs +++ b/src/JsonApiDotNetCore/Queries/IPaginationContext.cs @@ -3,18 +3,19 @@ namespace JsonApiDotNetCore.Queries; /// -/// Tracks values used for pagination, which is a combined effort from options, query string parsing and fetching the total number of rows. +/// Tracks values used for top-level pagination, which is a combined effort from options, query string parsing, resource definition callbacks and +/// fetching the total number of rows. /// public interface IPaginationContext { /// - /// The value 1, unless specified from query string. Never null. Cannot be higher than options.MaximumPageNumber. + /// The value 1, unless overridden from query string or resource definition. Should not be higher than . /// PageNumber PageNumber { get; set; } /// - /// The default page size from options, unless specified in query string. Can be null, which means no paging. Cannot be higher than - /// options.MaximumPageSize. + /// The default page size from options, unless overridden from query string or resource definition. Should not be higher than + /// . Can be null, which means pagination is disabled. /// PageSize? PageSize { get; set; } @@ -25,12 +26,12 @@ public interface IPaginationContext bool IsPageFull { get; set; } /// - /// The total number of resources. null when is set to false. + /// The total number of resources, or null when is set to false. /// int? TotalResourceCount { get; set; } /// - /// The total number of resource pages. null when is set to false or + /// The total number of resource pages, or null when is set to false or /// is null. /// int? TotalPageCount { get; } diff --git a/src/JsonApiDotNetCore/QueryStrings/Internal/PaginationQueryStringParameterReader.cs b/src/JsonApiDotNetCore/QueryStrings/Internal/PaginationQueryStringParameterReader.cs index f97c03399e..c6ec12afb6 100644 --- a/src/JsonApiDotNetCore/QueryStrings/Internal/PaginationQueryStringParameterReader.cs +++ b/src/JsonApiDotNetCore/QueryStrings/Internal/PaginationQueryStringParameterReader.cs @@ -73,7 +73,7 @@ public virtual void Read(string parameterName, StringValues parameterValue) } catch (QueryParseException exception) { - throw new InvalidQueryStringParameterException(parameterName, "The specified paging is invalid.", exception.Message, exception); + throw new InvalidQueryStringParameterException(parameterName, "The specified pagination is invalid.", exception.Message, exception); } } diff --git a/src/JsonApiDotNetCore/Resources/IResourceDefinition.cs b/src/JsonApiDotNetCore/Resources/IResourceDefinition.cs index 9af79831b2..31e4b9592a 100644 --- a/src/JsonApiDotNetCore/Resources/IResourceDefinition.cs +++ b/src/JsonApiDotNetCore/Resources/IResourceDefinition.cs @@ -60,7 +60,7 @@ public interface IResourceDefinition /// An optional existing pagination, coming from query string. Can be null. /// /// - /// The changed pagination, or null to use the first page with default size from options. To disable paging, set + /// The changed pagination, or null to use the first page with default size from options. To disable pagination, set /// to null. /// PaginationExpression? OnApplyPagination(PaginationExpression? existingPagination); diff --git a/src/JsonApiDotNetCore/Serialization/Response/LinkBuilder.cs b/src/JsonApiDotNetCore/Serialization/Response/LinkBuilder.cs index fb86eb084c..c6d32e2648 100644 --- a/src/JsonApiDotNetCore/Serialization/Response/LinkBuilder.cs +++ b/src/JsonApiDotNetCore/Serialization/Response/LinkBuilder.cs @@ -85,7 +85,7 @@ private static string NoAsyncSuffix(string actionName) links.Related = GetLinkForRelationshipRelated(_request.PrimaryId!, _request.Relationship); } - if (_request.IsCollection && _paginationContext.PageSize != null && ShouldIncludeTopLevelLink(LinkTypes.Paging, resourceType)) + if (_request.IsCollection && _paginationContext.PageSize != null && ShouldIncludeTopLevelLink(LinkTypes.Pagination, resourceType)) { SetPaginationInTopLevelLinks(resourceType!, links); } diff --git a/test/JsonApiDotNetCoreTests/IntegrationTests/QueryStrings/Pagination/PaginationWithTotalCountTests.cs b/test/JsonApiDotNetCoreTests/IntegrationTests/QueryStrings/Pagination/PaginationWithTotalCountTests.cs index 194e73a5dc..3f04403ee0 100644 --- a/test/JsonApiDotNetCoreTests/IntegrationTests/QueryStrings/Pagination/PaginationWithTotalCountTests.cs +++ b/test/JsonApiDotNetCoreTests/IntegrationTests/QueryStrings/Pagination/PaginationWithTotalCountTests.cs @@ -88,7 +88,7 @@ await _testContext.RunOnDatabaseAsync(async dbContext => ErrorObject error = responseDocument.Errors[0]; error.StatusCode.Should().Be(HttpStatusCode.BadRequest); - error.Title.Should().Be("The specified paging is invalid."); + error.Title.Should().Be("The specified pagination is invalid."); error.Detail.Should().Be("This query string parameter can only be used on a collection of resources (not on a single resource)."); error.Source.ShouldNotBeNull(); error.Source.Parameter.Should().Be("page[number]"); @@ -185,7 +185,7 @@ await _testContext.RunOnDatabaseAsync(async dbContext => ErrorObject error = responseDocument.Errors[0]; error.StatusCode.Should().Be(HttpStatusCode.BadRequest); - error.Title.Should().Be("The specified paging is invalid."); + error.Title.Should().Be("The specified pagination is invalid."); error.Detail.Should().Be("This query string parameter can only be used on a collection of resources (not on a single resource)."); error.Source.ShouldNotBeNull(); error.Source.Parameter.Should().Be("page[size]"); @@ -463,7 +463,7 @@ public async Task Cannot_paginate_in_unknown_scope() ErrorObject error = responseDocument.Errors[0]; error.StatusCode.Should().Be(HttpStatusCode.BadRequest); - error.Title.Should().Be("The specified paging is invalid."); + error.Title.Should().Be("The specified pagination is invalid."); error.Detail.Should().Be($"Relationship '{Unknown.Relationship}' does not exist on resource type 'webAccounts'."); error.Source.ShouldNotBeNull(); error.Source.Parameter.Should().Be("page[number]"); @@ -485,7 +485,7 @@ public async Task Cannot_paginate_in_unknown_nested_scope() ErrorObject error = responseDocument.Errors[0]; error.StatusCode.Should().Be(HttpStatusCode.BadRequest); - error.Title.Should().Be("The specified paging is invalid."); + error.Title.Should().Be("The specified pagination is invalid."); error.Detail.Should().Be($"Relationship '{Unknown.Relationship}' in 'posts.{Unknown.Relationship}' does not exist on resource type 'blogPosts'."); error.Source.ShouldNotBeNull(); error.Source.Parameter.Should().Be("page[size]"); @@ -528,7 +528,7 @@ await _testContext.RunOnDatabaseAsync(async dbContext => } [Fact] - public async Task Returns_all_resources_when_paging_is_disabled() + public async Task Returns_all_resources_when_pagination_is_disabled() { // Arrange var options = (JsonApiOptions)_testContext.Factory.Services.GetRequiredService(); diff --git a/test/JsonApiDotNetCoreTests/IntegrationTests/QueryStrings/Pagination/RangeValidationTests.cs b/test/JsonApiDotNetCoreTests/IntegrationTests/QueryStrings/Pagination/RangeValidationTests.cs index bcbd864d65..6b715f0825 100644 --- a/test/JsonApiDotNetCoreTests/IntegrationTests/QueryStrings/Pagination/RangeValidationTests.cs +++ b/test/JsonApiDotNetCoreTests/IntegrationTests/QueryStrings/Pagination/RangeValidationTests.cs @@ -42,7 +42,7 @@ public async Task Cannot_use_negative_page_number() ErrorObject error = responseDocument.Errors[0]; error.StatusCode.Should().Be(HttpStatusCode.BadRequest); - error.Title.Should().Be("The specified paging is invalid."); + error.Title.Should().Be("The specified pagination is invalid."); error.Detail.Should().Be("Page number cannot be negative or zero."); error.Source.ShouldNotBeNull(); error.Source.Parameter.Should().Be("page[number]"); @@ -64,7 +64,7 @@ public async Task Cannot_use_zero_page_number() ErrorObject error = responseDocument.Errors[0]; error.StatusCode.Should().Be(HttpStatusCode.BadRequest); - error.Title.Should().Be("The specified paging is invalid."); + error.Title.Should().Be("The specified pagination is invalid."); error.Detail.Should().Be("Page number cannot be negative or zero."); error.Source.ShouldNotBeNull(); error.Source.Parameter.Should().Be("page[number]"); @@ -123,7 +123,7 @@ public async Task Cannot_use_negative_page_size() ErrorObject error = responseDocument.Errors[0]; error.StatusCode.Should().Be(HttpStatusCode.BadRequest); - error.Title.Should().Be("The specified paging is invalid."); + error.Title.Should().Be("The specified pagination is invalid."); error.Detail.Should().Be("Page size cannot be negative."); error.Source.ShouldNotBeNull(); error.Source.Parameter.Should().Be("page[size]"); diff --git a/test/JsonApiDotNetCoreTests/IntegrationTests/QueryStrings/Pagination/RangeValidationWithMaximumTests.cs b/test/JsonApiDotNetCoreTests/IntegrationTests/QueryStrings/Pagination/RangeValidationWithMaximumTests.cs index cdbb9ea4be..66cb0dca57 100644 --- a/test/JsonApiDotNetCoreTests/IntegrationTests/QueryStrings/Pagination/RangeValidationWithMaximumTests.cs +++ b/test/JsonApiDotNetCoreTests/IntegrationTests/QueryStrings/Pagination/RangeValidationWithMaximumTests.cs @@ -71,7 +71,7 @@ public async Task Cannot_use_page_number_over_maximum() ErrorObject error = responseDocument.Errors[0]; error.StatusCode.Should().Be(HttpStatusCode.BadRequest); - error.Title.Should().Be("The specified paging is invalid."); + error.Title.Should().Be("The specified pagination is invalid."); error.Detail.Should().Be($"Page number cannot be higher than {MaximumPageNumber}."); error.Source.ShouldNotBeNull(); error.Source.Parameter.Should().Be("page[number]"); @@ -93,7 +93,7 @@ public async Task Cannot_use_zero_page_size() ErrorObject error = responseDocument.Errors[0]; error.StatusCode.Should().Be(HttpStatusCode.BadRequest); - error.Title.Should().Be("The specified paging is invalid."); + error.Title.Should().Be("The specified pagination is invalid."); error.Detail.Should().Be("Page size cannot be unconstrained."); error.Source.ShouldNotBeNull(); error.Source.Parameter.Should().Be("page[size]"); @@ -144,7 +144,7 @@ public async Task Cannot_use_page_size_over_maximum() ErrorObject error = responseDocument.Errors[0]; error.StatusCode.Should().Be(HttpStatusCode.BadRequest); - error.Title.Should().Be("The specified paging is invalid."); + error.Title.Should().Be("The specified pagination is invalid."); error.Detail.Should().Be($"Page size cannot be higher than {MaximumPageSize}."); error.Source.ShouldNotBeNull(); error.Source.Parameter.Should().Be("page[size]"); diff --git a/test/JsonApiDotNetCoreTests/UnitTests/Links/LinkInclusionTests.cs b/test/JsonApiDotNetCoreTests/UnitTests/Links/LinkInclusionTests.cs index cbc63de58f..cc4583ed4c 100644 --- a/test/JsonApiDotNetCoreTests/UnitTests/Links/LinkInclusionTests.cs +++ b/test/JsonApiDotNetCoreTests/UnitTests/Links/LinkInclusionTests.cs @@ -21,37 +21,37 @@ public sealed class LinkInclusionTests [InlineData(LinkTypes.NotConfigured, LinkTypes.None, LinkTypes.None)] [InlineData(LinkTypes.NotConfigured, LinkTypes.Self, LinkTypes.Self)] [InlineData(LinkTypes.NotConfigured, LinkTypes.Related, LinkTypes.Related)] - [InlineData(LinkTypes.NotConfigured, LinkTypes.Paging, LinkTypes.Paging)] + [InlineData(LinkTypes.NotConfigured, LinkTypes.Pagination, LinkTypes.Pagination)] [InlineData(LinkTypes.NotConfigured, LinkTypes.All, LinkTypes.All)] [InlineData(LinkTypes.None, LinkTypes.NotConfigured, LinkTypes.None)] [InlineData(LinkTypes.None, LinkTypes.None, LinkTypes.None)] [InlineData(LinkTypes.None, LinkTypes.Self, LinkTypes.None)] [InlineData(LinkTypes.None, LinkTypes.Related, LinkTypes.None)] - [InlineData(LinkTypes.None, LinkTypes.Paging, LinkTypes.None)] + [InlineData(LinkTypes.None, LinkTypes.Pagination, LinkTypes.None)] [InlineData(LinkTypes.None, LinkTypes.All, LinkTypes.None)] [InlineData(LinkTypes.Self, LinkTypes.NotConfigured, LinkTypes.Self)] [InlineData(LinkTypes.Self, LinkTypes.None, LinkTypes.Self)] [InlineData(LinkTypes.Self, LinkTypes.Self, LinkTypes.Self)] [InlineData(LinkTypes.Self, LinkTypes.Related, LinkTypes.Self)] - [InlineData(LinkTypes.Self, LinkTypes.Paging, LinkTypes.Self)] + [InlineData(LinkTypes.Self, LinkTypes.Pagination, LinkTypes.Self)] [InlineData(LinkTypes.Self, LinkTypes.All, LinkTypes.Self)] [InlineData(LinkTypes.Related, LinkTypes.NotConfigured, LinkTypes.Related)] [InlineData(LinkTypes.Related, LinkTypes.None, LinkTypes.Related)] [InlineData(LinkTypes.Related, LinkTypes.Self, LinkTypes.Related)] [InlineData(LinkTypes.Related, LinkTypes.Related, LinkTypes.Related)] - [InlineData(LinkTypes.Related, LinkTypes.Paging, LinkTypes.Related)] + [InlineData(LinkTypes.Related, LinkTypes.Pagination, LinkTypes.Related)] [InlineData(LinkTypes.Related, LinkTypes.All, LinkTypes.Related)] - [InlineData(LinkTypes.Paging, LinkTypes.NotConfigured, LinkTypes.Paging)] - [InlineData(LinkTypes.Paging, LinkTypes.None, LinkTypes.Paging)] - [InlineData(LinkTypes.Paging, LinkTypes.Self, LinkTypes.Paging)] - [InlineData(LinkTypes.Paging, LinkTypes.Related, LinkTypes.Paging)] - [InlineData(LinkTypes.Paging, LinkTypes.Paging, LinkTypes.Paging)] - [InlineData(LinkTypes.Paging, LinkTypes.All, LinkTypes.Paging)] + [InlineData(LinkTypes.Pagination, LinkTypes.NotConfigured, LinkTypes.Pagination)] + [InlineData(LinkTypes.Pagination, LinkTypes.None, LinkTypes.Pagination)] + [InlineData(LinkTypes.Pagination, LinkTypes.Self, LinkTypes.Pagination)] + [InlineData(LinkTypes.Pagination, LinkTypes.Related, LinkTypes.Pagination)] + [InlineData(LinkTypes.Pagination, LinkTypes.Pagination, LinkTypes.Pagination)] + [InlineData(LinkTypes.Pagination, LinkTypes.All, LinkTypes.Pagination)] [InlineData(LinkTypes.All, LinkTypes.NotConfigured, LinkTypes.All)] [InlineData(LinkTypes.All, LinkTypes.None, LinkTypes.All)] [InlineData(LinkTypes.All, LinkTypes.Self, LinkTypes.All)] [InlineData(LinkTypes.All, LinkTypes.Related, LinkTypes.All)] - [InlineData(LinkTypes.All, LinkTypes.Paging, LinkTypes.All)] + [InlineData(LinkTypes.All, LinkTypes.Pagination, LinkTypes.All)] [InlineData(LinkTypes.All, LinkTypes.All, LinkTypes.All)] public void Applies_cascading_settings_for_top_level_links(LinkTypes linksInResourceType, LinkTypes linksInOptions, LinkTypes expected) { @@ -117,7 +117,7 @@ public void Applies_cascading_settings_for_top_level_links(LinkTypes linksInReso topLevelLinks.Related.Should().BeNull(); } - if (expected.HasFlag(LinkTypes.Paging)) + if (expected.HasFlag(LinkTypes.Pagination)) { topLevelLinks.First.ShouldNotBeNull(); topLevelLinks.Last.ShouldNotBeNull(); diff --git a/test/JsonApiDotNetCoreTests/UnitTests/QueryStringParameters/PaginationParseTests.cs b/test/JsonApiDotNetCoreTests/UnitTests/QueryStringParameters/PaginationParseTests.cs index 5dc6e3ab75..615bae48e2 100644 --- a/test/JsonApiDotNetCoreTests/UnitTests/QueryStringParameters/PaginationParseTests.cs +++ b/test/JsonApiDotNetCoreTests/UnitTests/QueryStringParameters/PaginationParseTests.cs @@ -82,7 +82,7 @@ public void Reader_Read_Page_Number_Fails(string parameterValue, string errorMes ErrorObject error = exception.Errors[0]; error.StatusCode.Should().Be(HttpStatusCode.BadRequest); - error.Title.Should().Be("The specified paging is invalid."); + error.Title.Should().Be("The specified pagination is invalid."); error.Detail.Should().Be(errorMessage); error.Source.ShouldNotBeNull(); error.Source.Parameter.Should().Be("page[number]"); @@ -118,7 +118,7 @@ public void Reader_Read_Page_Size_Fails(string parameterValue, string errorMessa ErrorObject error = exception.Errors[0]; error.StatusCode.Should().Be(HttpStatusCode.BadRequest); - error.Title.Should().Be("The specified paging is invalid."); + error.Title.Should().Be("The specified pagination is invalid."); error.Detail.Should().Be(errorMessage); error.Source.ShouldNotBeNull(); error.Source.Parameter.Should().Be("page[size]"); From 2e1b804d7a7ad96811b1ed19235e28104409c611 Mon Sep 17 00:00:00 2001 From: Bart Koelman <10324372+bkoelman@users.noreply.github.com> Date: Tue, 28 Feb 2023 11:13:35 +0100 Subject: [PATCH 11/14] Fixed: missing test coverage for applying default page size in includes --- .../Pagination/PaginationWithTotalCountTests.cs | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/test/JsonApiDotNetCoreTests/IntegrationTests/QueryStrings/Pagination/PaginationWithTotalCountTests.cs b/test/JsonApiDotNetCoreTests/IntegrationTests/QueryStrings/Pagination/PaginationWithTotalCountTests.cs index 3f04403ee0..9174c84058 100644 --- a/test/JsonApiDotNetCoreTests/IntegrationTests/QueryStrings/Pagination/PaginationWithTotalCountTests.cs +++ b/test/JsonApiDotNetCoreTests/IntegrationTests/QueryStrings/Pagination/PaginationWithTotalCountTests.cs @@ -500,6 +500,7 @@ public async Task Uses_default_page_number_and_size() Blog blog = _fakers.Blog.Generate(); blog.Posts = _fakers.BlogPost.Generate(3); + blog.Posts.ToList().ForEach(post => post.Labels = _fakers.Label.Generate(3).ToHashSet()); await _testContext.RunOnDatabaseAsync(async dbContext => { @@ -507,7 +508,7 @@ await _testContext.RunOnDatabaseAsync(async dbContext => await dbContext.SaveChangesAsync(); }); - string route = $"/blogs/{blog.StringId}/posts"; + string route = $"/blogs/{blog.StringId}/posts?include=labels"; // Act (HttpResponseMessage httpResponse, Document responseDocument) = await _testContext.ExecuteGetAsync(route); @@ -519,10 +520,12 @@ await _testContext.RunOnDatabaseAsync(async dbContext => responseDocument.Data.ManyValue[0].Id.Should().Be(blog.Posts[0].StringId); responseDocument.Data.ManyValue[1].Id.Should().Be(blog.Posts[1].StringId); + responseDocument.Included.ShouldHaveCount(4); + responseDocument.Links.ShouldNotBeNull(); responseDocument.Links.Self.Should().Be($"{HostPrefix}{route}"); responseDocument.Links.First.Should().Be(responseDocument.Links.Self); - responseDocument.Links.Last.Should().Be($"{HostPrefix}{route}?page%5Bnumber%5D=2"); + responseDocument.Links.Last.Should().Be($"{responseDocument.Links.Self}&page%5Bnumber%5D=2"); responseDocument.Links.Prev.Should().BeNull(); responseDocument.Links.Next.Should().Be(responseDocument.Links.Last); } From 4055c6f33b8ac6bca28018f0b54e8b5ea9f00544 Mon Sep 17 00:00:00 2001 From: Bart Koelman <10324372+bkoelman@users.noreply.github.com> Date: Tue, 28 Feb 2023 17:44:05 +0100 Subject: [PATCH 12/14] Simplify meta:total assertions and produce better error on failure Before: Expected element.GetInt32() to be 25, but found 2 (difference of -23). After: Expected responseDocument.Meta to be 25, but found 2 (difference of -23). --- .../Meta/TopLevelCountTests.cs | 19 +++----------- .../NamingConventions/KebabCasingTests.cs | 7 +----- .../Reading/ResourceDefinitionReadTests.cs | 25 +++---------------- .../ObjectAssertionsExtensions.cs | 10 ++++++++ 4 files changed, 18 insertions(+), 43 deletions(-) diff --git a/test/JsonApiDotNetCoreTests/IntegrationTests/Meta/TopLevelCountTests.cs b/test/JsonApiDotNetCoreTests/IntegrationTests/Meta/TopLevelCountTests.cs index 53d6841b10..ff3360be30 100644 --- a/test/JsonApiDotNetCoreTests/IntegrationTests/Meta/TopLevelCountTests.cs +++ b/test/JsonApiDotNetCoreTests/IntegrationTests/Meta/TopLevelCountTests.cs @@ -1,5 +1,4 @@ using System.Net; -using System.Text.Json; using FluentAssertions; using JsonApiDotNetCore.Configuration; using JsonApiDotNetCore.Resources; @@ -54,11 +53,7 @@ await _testContext.RunOnDatabaseAsync(async dbContext => // Assert httpResponse.ShouldHaveStatusCode(HttpStatusCode.OK); - responseDocument.Meta.ShouldContainKey("total").With(value => - { - JsonElement element = value.Should().BeOfType().Subject; - element.GetInt32().Should().Be(1); - }); + responseDocument.Meta.Should().ContainTotal(1); } [Fact] @@ -84,11 +79,7 @@ await _testContext.RunOnDatabaseAsync(async dbContext => // Assert httpResponse.ShouldHaveStatusCode(HttpStatusCode.OK); - responseDocument.Meta.ShouldContainKey("total").With(value => - { - JsonElement element = value.Should().BeOfType().Subject; - element.GetInt32().Should().Be(1); - }); + responseDocument.Meta.Should().ContainTotal(1); } [Fact] @@ -108,11 +99,7 @@ await _testContext.RunOnDatabaseAsync(async dbContext => // Assert httpResponse.ShouldHaveStatusCode(HttpStatusCode.OK); - responseDocument.Meta.ShouldContainKey("total").With(value => - { - JsonElement element = value.Should().BeOfType().Subject; - element.GetInt32().Should().Be(0); - }); + responseDocument.Meta.Should().ContainTotal(0); } [Fact] diff --git a/test/JsonApiDotNetCoreTests/IntegrationTests/NamingConventions/KebabCasingTests.cs b/test/JsonApiDotNetCoreTests/IntegrationTests/NamingConventions/KebabCasingTests.cs index ab0a4a4a7e..d70f50de0e 100644 --- a/test/JsonApiDotNetCoreTests/IntegrationTests/NamingConventions/KebabCasingTests.cs +++ b/test/JsonApiDotNetCoreTests/IntegrationTests/NamingConventions/KebabCasingTests.cs @@ -1,5 +1,4 @@ using System.Net; -using System.Text.Json; using FluentAssertions; using JsonApiDotNetCore.Serialization.Objects; using TestBuildingBlocks; @@ -57,11 +56,7 @@ await _testContext.RunOnDatabaseAsync(async dbContext => responseDocument.Included[0].Relationships.Should().BeNull(); responseDocument.Included[0].Links.ShouldNotBeNull().Self.Should().Be($"/public-api/diving-boards/{pools[1].DivingBoards[0].StringId}"); - responseDocument.Meta.ShouldContainKey("total").With(value => - { - JsonElement element = value.Should().BeOfType().Subject; - element.GetInt32().Should().Be(2); - }); + responseDocument.Meta.Should().ContainTotal(2); } [Fact] diff --git a/test/JsonApiDotNetCoreTests/IntegrationTests/ResourceDefinitions/Reading/ResourceDefinitionReadTests.cs b/test/JsonApiDotNetCoreTests/IntegrationTests/ResourceDefinitions/Reading/ResourceDefinitionReadTests.cs index db80d4c14b..6176b28a38 100644 --- a/test/JsonApiDotNetCoreTests/IntegrationTests/ResourceDefinitions/Reading/ResourceDefinitionReadTests.cs +++ b/test/JsonApiDotNetCoreTests/IntegrationTests/ResourceDefinitions/Reading/ResourceDefinitionReadTests.cs @@ -1,5 +1,4 @@ using System.Net; -using System.Text.Json; using FluentAssertions; using JsonApiDotNetCore.Configuration; using JsonApiDotNetCore.Serialization.Objects; @@ -238,11 +237,7 @@ await _testContext.RunOnDatabaseAsync(async dbContext => responseDocument.Data.ManyValue[0].Id.Should().Be(planets[1].StringId); responseDocument.Data.ManyValue[1].Id.Should().Be(planets[3].StringId); - responseDocument.Meta.ShouldContainKey("total").With(value => - { - JsonElement element = value.Should().BeOfType().Subject; - element.GetInt32().Should().Be(2); - }); + responseDocument.Meta.Should().ContainTotal(2); hitCounter.HitExtensibilityPoints.Should().BeEquivalentTo(new[] { @@ -297,11 +292,7 @@ await _testContext.RunOnDatabaseAsync(async dbContext => responseDocument.Data.ManyValue.ShouldHaveCount(1); responseDocument.Data.ManyValue[0].Id.Should().Be(planets[3].StringId); - responseDocument.Meta.ShouldContainKey("total").With(value => - { - JsonElement element = value.Should().BeOfType().Subject; - element.GetInt32().Should().Be(1); - }); + responseDocument.Meta.Should().ContainTotal(1); hitCounter.HitExtensibilityPoints.Should().BeEquivalentTo(new[] { @@ -349,11 +340,7 @@ await _testContext.RunOnDatabaseAsync(async dbContext => responseDocument.Data.ManyValue[0].Id.Should().Be(star.Planets.ElementAt(1).StringId); responseDocument.Data.ManyValue[1].Id.Should().Be(star.Planets.ElementAt(3).StringId); - responseDocument.Meta.ShouldContainKey("total").With(value => - { - JsonElement element = value.Should().BeOfType().Subject; - element.GetInt32().Should().Be(2); - }); + responseDocument.Meta.Should().ContainTotal(2); hitCounter.HitExtensibilityPoints.Should().BeEquivalentTo(new[] { @@ -405,11 +392,7 @@ await _testContext.RunOnDatabaseAsync(async dbContext => responseDocument.Data.ManyValue[0].Id.Should().Be(star.Planets.ElementAt(1).StringId); responseDocument.Data.ManyValue[1].Id.Should().Be(star.Planets.ElementAt(3).StringId); - responseDocument.Meta.ShouldContainKey("total").With(value => - { - JsonElement element = value.Should().BeOfType().Subject; - element.GetInt32().Should().Be(2); - }); + responseDocument.Meta.Should().ContainTotal(2); hitCounter.HitExtensibilityPoints.Should().BeEquivalentTo(new[] { diff --git a/test/TestBuildingBlocks/ObjectAssertionsExtensions.cs b/test/TestBuildingBlocks/ObjectAssertionsExtensions.cs index a295e1eaf9..ee2be771e1 100644 --- a/test/TestBuildingBlocks/ObjectAssertionsExtensions.cs +++ b/test/TestBuildingBlocks/ObjectAssertionsExtensions.cs @@ -2,6 +2,7 @@ using System.Text.Encodings.Web; using System.Text.Json; using FluentAssertions; +using FluentAssertions.Collections; using FluentAssertions.Numeric; using FluentAssertions.Primitives; using JetBrains.Annotations; @@ -65,4 +66,13 @@ private static string ToJsonString(JsonDocument document) writer.Flush(); return Encoding.UTF8.GetString(stream.ToArray()); } + + /// + /// Asserts that a "meta" dictionary contains a single element named "total" with the specified value. + /// + [CustomAssertion] + public static void ContainTotal(this GenericDictionaryAssertions, string, object?> source, int expectedTotal) + { + source.ContainKey("total").WhoseValue.Should().BeOfType().Subject.GetInt32().Should().Be(expectedTotal); + } } From adc51e1e6dce66bc67aa52f8599990eb1900bc3f Mon Sep 17 00:00:00 2001 From: Bart Koelman <10324372+bkoelman@users.noreply.github.com> Date: Thu, 2 Mar 2023 11:54:47 +0100 Subject: [PATCH 13/14] Clarify documentation --- src/JsonApiDotNetCore/Resources/IResourceDefinition.cs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/JsonApiDotNetCore/Resources/IResourceDefinition.cs b/src/JsonApiDotNetCore/Resources/IResourceDefinition.cs index 31e4b9592a..41d366b5c3 100644 --- a/src/JsonApiDotNetCore/Resources/IResourceDefinition.cs +++ b/src/JsonApiDotNetCore/Resources/IResourceDefinition.cs @@ -241,9 +241,9 @@ Task OnAddToRelationshipAsync(TResource leftResource, HasManyAttribute hasManyRe /// /// /// - /// The original resource as retrieved from the underlying data store. The indication "left" specifies that is - /// declared on . Be aware that for performance reasons, not the full relationship is populated, but only the subset of - /// resources to be removed. + /// Identifier of the left resource. The indication "left" specifies that is declared on + /// . In contrast to other relationship methods, only the left ID and only the subset of right resources to be removed + /// are retrieved from the underlying data store. /// /// /// The to-many relationship being removed from. From 039ee720ab40d8dd1a2baf6e56a5ed2e43ea7cea Mon Sep 17 00:00:00 2001 From: Bart Koelman <10324372+bkoelman@users.noreply.github.com> Date: Fri, 10 Mar 2023 13:25:06 +0100 Subject: [PATCH 14/14] Package updates --- .config/dotnet-tools.json | 4 ++-- Directory.Build.props | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/.config/dotnet-tools.json b/.config/dotnet-tools.json index d77e271e4d..e713343770 100644 --- a/.config/dotnet-tools.json +++ b/.config/dotnet-tools.json @@ -21,13 +21,13 @@ ] }, "dotnet-reportgenerator-globaltool": { - "version": "5.1.15", + "version": "5.1.19", "commands": [ "reportgenerator" ] }, "docfx": { - "version": "2.60.2", + "version": "2.62.2", "commands": [ "docfx" ] diff --git a/Directory.Build.props b/Directory.Build.props index 34807583da..a7ee36a2d9 100644 --- a/Directory.Build.props +++ b/Directory.Build.props @@ -4,7 +4,7 @@ 6.0.* 7.0.* 7.0.* - 4.4.* + 4.5.* 2.14.1 5.1.3 $(MSBuildThisFileDirectory)CodingGuidelines.ruleset @@ -17,7 +17,7 @@ - +