Skip to content

Commit b0c0838

Browse files
bkoelmanbkoelman
authored
Merge pull request #1711 from json-api-dotnet/crashfix-total
Fix invalid expression for total count on inverse relationship filter from resource definition
2 parents b97fdd4 + ac31043 commit b0c0838

File tree

11 files changed

+323
-12
lines changed

11 files changed

+323
-12
lines changed

‎src/JsonApiDotNetCore/Queries/QueryLayerComposer.cs

+17-9
Original file line numberDiff line numberDiff line change
@@ -96,18 +96,25 @@ public QueryLayerComposer(IEnumerable<IQueryConstraintProvider> constraintProvid
9696
// @formatter:wrap_chained_method_calls restore
9797

9898
FilterExpression? primaryFilter = GetFilter(Array.Empty<QueryExpression>(), hasManyRelationship.LeftType);
99-
FilterExpression? secondaryFilter = GetFilter(filtersInSecondaryScope, hasManyRelationship.RightType);
10099

101-
FilterExpression inverseFilter = GetInverseRelationshipFilter(primaryId, hasManyRelationship, inverseRelationship);
100+
if (primaryFilter != null && inverseRelationship is HasOneAttribute)
101+
{
102+
// We can't lift the field chains in a primary filter, because there's no way for a custom filter expression to express
103+
// the scope of its chains. See https://github.com/json-api-dotnet/JsonApiDotNetCore/issues/1671.
104+
return null;
105+
}
106+
107+
FilterExpression? secondaryFilter = GetFilter(filtersInSecondaryScope, hasManyRelationship.RightType);
108+
FilterExpression inverseFilter = GetInverseRelationshipFilter(primaryId, primaryFilter, hasManyRelationship, inverseRelationship);
102109

103-
return LogicalExpression.Compose(LogicalOperator.And, inverseFilter, primaryFilter, secondaryFilter);
110+
return LogicalExpression.Compose(LogicalOperator.And, inverseFilter, secondaryFilter);
104111
}
105112

106-
private static FilterExpression GetInverseRelationshipFilter<TId>([DisallowNull] TId primaryId, HasManyAttribute relationship,
107-
RelationshipAttribute inverseRelationship)
113+
private static FilterExpression GetInverseRelationshipFilter<TId>([DisallowNull] TId primaryId, FilterExpression? primaryFilter,
114+
HasManyAttribute relationship, RelationshipAttribute inverseRelationship)
108115
{
109116
return inverseRelationship is HasManyAttribute hasManyInverseRelationship
110-
? GetInverseHasManyRelationshipFilter(primaryId, relationship, hasManyInverseRelationship)
117+
? GetInverseHasManyRelationshipFilter(primaryId, primaryFilter, relationship, hasManyInverseRelationship)
111118
: GetInverseHasOneRelationshipFilter(primaryId, relationship, (HasOneAttribute)inverseRelationship);
112119
}
113120

@@ -120,14 +127,15 @@ private static ComparisonExpression GetInverseHasOneRelationshipFilter<TId>([Dis
120127
return new ComparisonExpression(ComparisonOperator.Equals, idChain, new LiteralConstantExpression(primaryId));
121128
}
122129

123-
private static HasExpression GetInverseHasManyRelationshipFilter<TId>([DisallowNull] TId primaryId, HasManyAttribute relationship,
124-
HasManyAttribute inverseRelationship)
130+
private static HasExpression GetInverseHasManyRelationshipFilter<TId>([DisallowNull] TId primaryId, FilterExpression? primaryFilter,
131+
HasManyAttribute relationship, HasManyAttribute inverseRelationship)
125132
{
126133
AttrAttribute idAttribute = GetIdAttribute(relationship.LeftType);
127134
var idChain = new ResourceFieldChainExpression(ImmutableArray.Create<ResourceFieldAttribute>(idAttribute));
128135
var idComparison = new ComparisonExpression(ComparisonOperator.Equals, idChain, new LiteralConstantExpression(primaryId));
129136

130-
return new HasExpression(new ResourceFieldChainExpression(inverseRelationship), idComparison);
137+
FilterExpression filter = LogicalExpression.Compose(LogicalOperator.And, idComparison, primaryFilter)!;
138+
return new HasExpression(new ResourceFieldChainExpression(inverseRelationship), filter);
131139
}
132140

133141
/// <inheritdoc />

‎test/JsonApiDotNetCoreTests/IntegrationTests/ResourceDefinitions/Reading/Constellation.cs

+21
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
using System.ComponentModel.DataAnnotations;
2+
using JetBrains.Annotations;
3+
using JsonApiDotNetCore.Resources;
4+
using JsonApiDotNetCore.Resources.Annotations;
5+
6+
namespace JsonApiDotNetCoreTests.IntegrationTests.ResourceDefinitions.Reading;
7+
8+
[UsedImplicitly(ImplicitUseTargetFlags.Members)]
9+
[Resource(ControllerNamespace = "JsonApiDotNetCoreTests.IntegrationTests.ResourceDefinitions.Reading")]
10+
public sealed class Constellation : Identifiable<int>
11+
{
12+
[Attr]
13+
public string Name { get; set; } = null!;
14+
15+
[Attr]
16+
[Required]
17+
public Season? VisibleDuring { get; set; }
18+
19+
[HasMany]
20+
public ISet<Star> Stars { get; set; } = new HashSet<Star>();
21+
}

‎test/JsonApiDotNetCoreTests/IntegrationTests/ResourceDefinitions/Reading/ConstellationDefinition.cs

+33
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,33 @@
1+
using JetBrains.Annotations;
2+
using JsonApiDotNetCore.Configuration;
3+
using JsonApiDotNetCore.Queries.Expressions;
4+
using JsonApiDotNetCore.Resources.Annotations;
5+
6+
namespace JsonApiDotNetCoreTests.IntegrationTests.ResourceDefinitions.Reading;
7+
8+
[UsedImplicitly(ImplicitUseKindFlags.InstantiatedNoFixedConstructorSignature)]
9+
public sealed class ConstellationDefinition(
10+
IResourceGraph resourceGraph, IClientSettingsProvider clientSettingsProvider, ResourceDefinitionHitCounter hitCounter)
11+
: HitCountingResourceDefinition<Constellation, int>(resourceGraph, hitCounter)
12+
{
13+
private readonly IClientSettingsProvider _clientSettingsProvider = clientSettingsProvider;
14+
15+
protected override ResourceDefinitionExtensibilityPoints ExtensibilityPointsToTrack => ResourceDefinitionExtensibilityPoints.Reading;
16+
17+
public override FilterExpression? OnApplyFilter(FilterExpression? existingFilter)
18+
{
19+
FilterExpression? baseFilter = base.OnApplyFilter(existingFilter);
20+
21+
if (_clientSettingsProvider.AreConstellationsVisibleDuringWinterHidden)
22+
{
23+
AttrAttribute visibleDuringAttribute = ResourceType.GetAttributeByPropertyName(nameof(Constellation.VisibleDuring));
24+
var visibleDuringChain = new ResourceFieldChainExpression(visibleDuringAttribute);
25+
var visibleDuringComparison = new ComparisonExpression(ComparisonOperator.Equals, visibleDuringChain, new LiteralConstantExpression(Season.Winter));
26+
var notVisibleDuringComparison = new NotExpression(visibleDuringComparison);
27+
28+
return LogicalExpression.Compose(LogicalOperator.And, baseFilter, notVisibleDuringComparison);
29+
}
30+
31+
return baseFilter;
32+
}
33+
}

‎test/JsonApiDotNetCoreTests/IntegrationTests/ResourceDefinitions/Reading/IClientSettingsProvider.cs

+2
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,8 @@ namespace JsonApiDotNetCoreTests.IntegrationTests.ResourceDefinitions.Reading;
22

33
public interface IClientSettingsProvider
44
{
5+
bool AreVeryLargeStarsHidden { get; }
6+
bool AreConstellationsVisibleDuringWinterHidden { get; }
57
bool IsIncludePlanetMoonsBlocked { get; }
68
bool ArePlanetsWithPrivateNameHidden { get; }
79
bool IsStarGivingLightToMoonAutoIncluded { get; }

‎test/JsonApiDotNetCoreTests/IntegrationTests/ResourceDefinitions/Reading/ResourceDefinitionReadTests.cs

+194-2
Original file line numberDiff line numberDiff line change
@@ -17,12 +17,14 @@ public ResourceDefinitionReadTests(IntegrationTestContext<TestableStartup<Univer
1717
{
1818
_testContext = testContext;
1919

20+
testContext.UseController<ConstellationsController>();
2021
testContext.UseController<StarsController>();
2122
testContext.UseController<PlanetsController>();
2223
testContext.UseController<MoonsController>();
2324

2425
testContext.ConfigureServices(services =>
2526
{
27+
services.AddResourceDefinition<ConstellationDefinition>();
2628
services.AddResourceDefinition<StarDefinition>();
2729
services.AddResourceDefinition<PlanetDefinition>();
2830
services.AddResourceDefinition<MoonDefinition>();
@@ -323,7 +325,6 @@ public async Task Filter_from_resource_definition_is_applied_at_secondary_endpoi
323325

324326
await _testContext.RunOnDatabaseAsync(async dbContext =>
325327
{
326-
await dbContext.ClearTableAsync<Planet>();
327328
dbContext.Stars.Add(star);
328329
await dbContext.SaveChangesAsync();
329330
});
@@ -375,7 +376,6 @@ public async Task Filter_from_resource_definition_is_applied_at_relationship_end
375376

376377
await _testContext.RunOnDatabaseAsync(async dbContext =>
377378
{
378-
await dbContext.ClearTableAsync<Planet>();
379379
dbContext.Stars.Add(star);
380380
await dbContext.SaveChangesAsync();
381381
});
@@ -409,6 +409,198 @@ await _testContext.RunOnDatabaseAsync(async dbContext =>
409409
}, options => options.WithStrictOrdering());
410410
}
411411

412+
[Fact]
413+
public async Task No_total_when_resource_definition_has_filter_on_inverse_ManyToOne_at_secondary_endpoint()
414+
{
415+
// Arrange
416+
var hitCounter = _testContext.Factory.Services.GetRequiredService<ResourceDefinitionHitCounter>();
417+
418+
var settingsProvider = (TestClientSettingsProvider)_testContext.Factory.Services.GetRequiredService<IClientSettingsProvider>();
419+
settingsProvider.HideVeryLargeStars();
420+
421+
Star star = _fakers.Star.GenerateOne();
422+
star.Planets = _fakers.Planet.GenerateSet(1);
423+
424+
await _testContext.RunOnDatabaseAsync(async dbContext =>
425+
{
426+
dbContext.Stars.Add(star);
427+
await dbContext.SaveChangesAsync();
428+
});
429+
430+
string route = $"/stars/{star.StringId}/planets";
431+
432+
// Act
433+
(HttpResponseMessage httpResponse, Document responseDocument) = await _testContext.ExecuteGetAsync<Document>(route);
434+
435+
// Assert
436+
httpResponse.ShouldHaveStatusCode(HttpStatusCode.OK);
437+
438+
responseDocument.Data.ManyValue.Should().HaveCount(1);
439+
responseDocument.Data.ManyValue[0].Id.Should().Be(star.Planets.ElementAt(0).StringId);
440+
441+
responseDocument.Meta.Should().BeNull();
442+
443+
hitCounter.HitExtensibilityPoints.Should().BeEquivalentTo(new[]
444+
{
445+
(typeof(Star), ResourceDefinitionExtensibilityPoints.OnApplyFilter),
446+
(typeof(Planet), ResourceDefinitionExtensibilityPoints.OnApplyPagination),
447+
(typeof(Planet), ResourceDefinitionExtensibilityPoints.OnApplyFilter),
448+
(typeof(Planet), ResourceDefinitionExtensibilityPoints.OnApplySort),
449+
(typeof(Planet), ResourceDefinitionExtensibilityPoints.OnApplySparseFieldSet),
450+
(typeof(Planet), ResourceDefinitionExtensibilityPoints.OnApplyIncludes),
451+
(typeof(Star), ResourceDefinitionExtensibilityPoints.OnApplySparseFieldSet),
452+
(typeof(Star), ResourceDefinitionExtensibilityPoints.OnApplyFilter),
453+
(typeof(Planet), ResourceDefinitionExtensibilityPoints.OnApplySparseFieldSet),
454+
(typeof(Planet), ResourceDefinitionExtensibilityPoints.GetMeta)
455+
}, options => options.WithStrictOrdering());
456+
}
457+
458+
[Fact]
459+
public async Task Has_total_when_resource_definition_has_filter_on_inverse_ManyToMany_at_secondary_endpoint()
460+
{
461+
// Arrange
462+
var hitCounter = _testContext.Factory.Services.GetRequiredService<ResourceDefinitionHitCounter>();
463+
464+
var settingsProvider = (TestClientSettingsProvider)_testContext.Factory.Services.GetRequiredService<IClientSettingsProvider>();
465+
settingsProvider.HideConstellationsVisibleDuringWinter();
466+
467+
Constellation constellation = _fakers.Constellation.GenerateOne();
468+
constellation.VisibleDuring = Season.Winter;
469+
constellation.Stars = _fakers.Star.GenerateSet(1);
470+
471+
await _testContext.RunOnDatabaseAsync(async dbContext =>
472+
{
473+
dbContext.Constellations.Add(constellation);
474+
await dbContext.SaveChangesAsync();
475+
});
476+
477+
string route = $"/constellations/{constellation.StringId}/stars";
478+
479+
// Act
480+
(HttpResponseMessage httpResponse, Document responseDocument) = await _testContext.ExecuteGetAsync<Document>(route);
481+
482+
// Assert
483+
httpResponse.ShouldHaveStatusCode(HttpStatusCode.NotFound);
484+
485+
responseDocument.Errors.Should().HaveCount(1);
486+
487+
ErrorObject error = responseDocument.Errors[0];
488+
error.StatusCode.Should().Be(HttpStatusCode.NotFound);
489+
error.Title.Should().Be("The requested resource does not exist.");
490+
error.Detail.Should().Be($"Resource of type 'constellations' with ID '{constellation.StringId}' does not exist.");
491+
492+
responseDocument.Meta.Should().ContainTotal(0);
493+
494+
hitCounter.HitExtensibilityPoints.Should().BeEquivalentTo(new[]
495+
{
496+
(typeof(Constellation), ResourceDefinitionExtensibilityPoints.OnApplyFilter),
497+
(typeof(Star), ResourceDefinitionExtensibilityPoints.OnApplyFilter),
498+
(typeof(Star), ResourceDefinitionExtensibilityPoints.OnApplyPagination),
499+
(typeof(Star), ResourceDefinitionExtensibilityPoints.OnApplyFilter),
500+
(typeof(Star), ResourceDefinitionExtensibilityPoints.OnApplySort),
501+
(typeof(Star), ResourceDefinitionExtensibilityPoints.OnApplySparseFieldSet),
502+
(typeof(Star), ResourceDefinitionExtensibilityPoints.OnApplyIncludes),
503+
(typeof(Constellation), ResourceDefinitionExtensibilityPoints.OnApplySparseFieldSet),
504+
(typeof(Constellation), ResourceDefinitionExtensibilityPoints.OnApplyFilter)
505+
}, options => options.WithStrictOrdering());
506+
}
507+
508+
[Fact]
509+
public async Task No_total_when_resource_definition_has_filter_on_inverse_ManyToOne_at_relationship_endpoint()
510+
{
511+
// Arrange
512+
var hitCounter = _testContext.Factory.Services.GetRequiredService<ResourceDefinitionHitCounter>();
513+
514+
var settingsProvider = (TestClientSettingsProvider)_testContext.Factory.Services.GetRequiredService<IClientSettingsProvider>();
515+
settingsProvider.HideVeryLargeStars();
516+
517+
Star star = _fakers.Star.GenerateOne();
518+
star.Planets = _fakers.Planet.GenerateSet(1);
519+
520+
await _testContext.RunOnDatabaseAsync(async dbContext =>
521+
{
522+
dbContext.Stars.Add(star);
523+
await dbContext.SaveChangesAsync();
524+
});
525+
526+
string route = $"/stars/{star.StringId}/relationships/planets";
527+
528+
// Act
529+
(HttpResponseMessage httpResponse, Document responseDocument) = await _testContext.ExecuteGetAsync<Document>(route);
530+
531+
// Assert
532+
httpResponse.ShouldHaveStatusCode(HttpStatusCode.OK);
533+
534+
responseDocument.Data.ManyValue.Should().HaveCount(1);
535+
responseDocument.Data.ManyValue[0].Id.Should().Be(star.Planets.ElementAt(0).StringId);
536+
537+
responseDocument.Meta.Should().BeNull();
538+
539+
hitCounter.HitExtensibilityPoints.Should().BeEquivalentTo(new[]
540+
{
541+
(typeof(Star), ResourceDefinitionExtensibilityPoints.OnApplyFilter),
542+
(typeof(Planet), ResourceDefinitionExtensibilityPoints.OnApplyPagination),
543+
(typeof(Planet), ResourceDefinitionExtensibilityPoints.OnApplyFilter),
544+
(typeof(Planet), ResourceDefinitionExtensibilityPoints.OnApplySort),
545+
(typeof(Planet), ResourceDefinitionExtensibilityPoints.OnApplySparseFieldSet),
546+
(typeof(Planet), ResourceDefinitionExtensibilityPoints.OnApplyIncludes),
547+
(typeof(Planet), ResourceDefinitionExtensibilityPoints.OnApplySparseFieldSet),
548+
(typeof(Star), ResourceDefinitionExtensibilityPoints.OnApplySparseFieldSet),
549+
(typeof(Star), ResourceDefinitionExtensibilityPoints.OnApplyFilter)
550+
}, options => options.WithStrictOrdering());
551+
}
552+
553+
[Fact]
554+
public async Task Has_total_when_resource_definition_has_filter_on_inverse_ManyToMany_at_relationship_endpoint()
555+
{
556+
// Arrange
557+
var hitCounter = _testContext.Factory.Services.GetRequiredService<ResourceDefinitionHitCounter>();
558+
559+
var settingsProvider = (TestClientSettingsProvider)_testContext.Factory.Services.GetRequiredService<IClientSettingsProvider>();
560+
settingsProvider.HideConstellationsVisibleDuringWinter();
561+
562+
Constellation constellation = _fakers.Constellation.GenerateOne();
563+
constellation.VisibleDuring = Season.Winter;
564+
constellation.Stars = _fakers.Star.GenerateSet(1);
565+
566+
await _testContext.RunOnDatabaseAsync(async dbContext =>
567+
{
568+
dbContext.Constellations.Add(constellation);
569+
await dbContext.SaveChangesAsync();
570+
});
571+
572+
string route = $"/constellations/{constellation.StringId}/relationships/stars";
573+
574+
// Act
575+
(HttpResponseMessage httpResponse, Document responseDocument) = await _testContext.ExecuteGetAsync<Document>(route);
576+
577+
// Assert
578+
httpResponse.ShouldHaveStatusCode(HttpStatusCode.NotFound);
579+
580+
responseDocument.Errors.Should().HaveCount(1);
581+
582+
ErrorObject error = responseDocument.Errors[0];
583+
error.StatusCode.Should().Be(HttpStatusCode.NotFound);
584+
error.Title.Should().Be("The requested resource does not exist.");
585+
error.Detail.Should().Be($"Resource of type 'constellations' with ID '{constellation.StringId}' does not exist.");
586+
587+
responseDocument.Meta.Should().ContainTotal(0);
588+
589+
hitCounter.HitExtensibilityPoints.Should().BeEquivalentTo(new[]
590+
{
591+
(typeof(Constellation), ResourceDefinitionExtensibilityPoints.OnApplyFilter),
592+
(typeof(Star), ResourceDefinitionExtensibilityPoints.OnApplyFilter),
593+
(typeof(Star), ResourceDefinitionExtensibilityPoints.OnApplyPagination),
594+
(typeof(Star), ResourceDefinitionExtensibilityPoints.OnApplyFilter),
595+
(typeof(Star), ResourceDefinitionExtensibilityPoints.OnApplySort),
596+
(typeof(Star), ResourceDefinitionExtensibilityPoints.OnApplySparseFieldSet),
597+
(typeof(Star), ResourceDefinitionExtensibilityPoints.OnApplyIncludes),
598+
(typeof(Star), ResourceDefinitionExtensibilityPoints.OnApplySparseFieldSet),
599+
(typeof(Constellation), ResourceDefinitionExtensibilityPoints.OnApplySparseFieldSet),
600+
(typeof(Constellation), ResourceDefinitionExtensibilityPoints.OnApplyFilter)
601+
}, options => options.WithStrictOrdering());
602+
}
603+
412604
[Fact]
413605
public async Task Sort_from_resource_definition_is_applied()
414606
{

‎test/JsonApiDotNetCoreTests/IntegrationTests/ResourceDefinitions/Reading/Season.cs

+12
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
using JetBrains.Annotations;
2+
3+
namespace JsonApiDotNetCoreTests.IntegrationTests.ResourceDefinitions.Reading;
4+
5+
[UsedImplicitly(ImplicitUseTargetFlags.Members)]
6+
public enum Season
7+
{
8+
Winter,
9+
Spring,
10+
Summer,
11+
Fall
12+
}

‎test/JsonApiDotNetCoreTests/IntegrationTests/ResourceDefinitions/Reading/Star.cs

+3
Original file line numberDiff line numberDiff line change
@@ -25,4 +25,7 @@ public sealed class Star : Identifiable<int>
2525

2626
[HasMany]
2727
public ISet<Planet> Planets { get; set; } = new HashSet<Planet>();
28+
29+
[HasMany]
30+
public ISet<Constellation> IsPartOf { get; set; } = new HashSet<Constellation>();
2831
}

0 commit comments

Comments
 (0)