Skip to content

Commit ac31043

Browse files
bkoelmanbkoelman
committed
Fix invalid expression for total count on inverse relationship filter from resource definition
1 parent a84b258 commit ac31043

File tree

6 files changed

+270
-12
lines changed

6 files changed

+270
-12
lines changed

‎src/JsonApiDotNetCore/Queries/QueryLayerComposer.cs‎

Lines changed: 17 additions & 9 deletions
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/ConstellationDefinition.cs‎

Lines changed: 24 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,33 @@
11
using JetBrains.Annotations;
22
using JsonApiDotNetCore.Configuration;
3+
using JsonApiDotNetCore.Queries.Expressions;
4+
using JsonApiDotNetCore.Resources.Annotations;
35

46
namespace JsonApiDotNetCoreTests.IntegrationTests.ResourceDefinitions.Reading;
57

68
[UsedImplicitly(ImplicitUseKindFlags.InstantiatedNoFixedConstructorSignature)]
7-
public sealed class ConstellationDefinition(IResourceGraph resourceGraph, ResourceDefinitionHitCounter hitCounter)
8-
: HitCountingResourceDefinition<Moon, int>(resourceGraph, hitCounter)
9+
public sealed class ConstellationDefinition(
10+
IResourceGraph resourceGraph, IClientSettingsProvider clientSettingsProvider, ResourceDefinitionHitCounter hitCounter)
11+
: HitCountingResourceDefinition<Constellation, int>(resourceGraph, hitCounter)
912
{
13+
private readonly IClientSettingsProvider _clientSettingsProvider = clientSettingsProvider;
14+
1015
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+
}
1133
}

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

Lines changed: 2 additions & 0 deletions
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‎

Lines changed: 193 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ 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>();
@@ -408,6 +409,198 @@ await _testContext.RunOnDatabaseAsync(async dbContext =>
408409
}, options => options.WithStrictOrdering());
409410
}
410411

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+
411604
[Fact]
412605
public async Task Sort_from_resource_definition_is_applied()
413606
{

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

Lines changed: 20 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,16 +2,35 @@
22
using JetBrains.Annotations;
33
using JsonApiDotNetCore.Configuration;
44
using JsonApiDotNetCore.Queries.Expressions;
5+
using JsonApiDotNetCore.Resources.Annotations;
56

67
namespace JsonApiDotNetCoreTests.IntegrationTests.ResourceDefinitions.Reading;
78

89
[UsedImplicitly(ImplicitUseKindFlags.InstantiatedNoFixedConstructorSignature)]
910
// The constructor parameters will be resolved from the container, which means you can take on any dependency that is also defined in the container.
10-
public sealed class StarDefinition(IResourceGraph resourceGraph, ResourceDefinitionHitCounter hitCounter)
11+
public sealed class StarDefinition(IResourceGraph resourceGraph, IClientSettingsProvider clientSettingsProvider, ResourceDefinitionHitCounter hitCounter)
1112
: HitCountingResourceDefinition<Star, int>(resourceGraph, hitCounter)
1213
{
14+
private readonly IClientSettingsProvider _clientSettingsProvider = clientSettingsProvider;
15+
1316
protected override ResourceDefinitionExtensibilityPoints ExtensibilityPointsToTrack => ResourceDefinitionExtensibilityPoints.Reading;
1417

18+
public override FilterExpression? OnApplyFilter(FilterExpression? existingFilter)
19+
{
20+
FilterExpression? baseFilter = base.OnApplyFilter(existingFilter);
21+
22+
if (_clientSettingsProvider.AreVeryLargeStarsHidden)
23+
{
24+
AttrAttribute solarRadiusAttribute = ResourceType.GetAttributeByPropertyName(nameof(Star.SolarRadius));
25+
var solarRadiusChain = new ResourceFieldChainExpression(solarRadiusAttribute);
26+
var solarRadiusComparison = new ComparisonExpression(ComparisonOperator.LessThan, solarRadiusChain, new LiteralConstantExpression(2000M));
27+
28+
return LogicalExpression.Compose(LogicalOperator.And, baseFilter, solarRadiusComparison);
29+
}
30+
31+
return baseFilter;
32+
}
33+
1534
public override SortExpression OnApplySort(SortExpression? existingSort)
1635
{
1736
base.OnApplySort(existingSort);

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

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,17 +2,31 @@ namespace JsonApiDotNetCoreTests.IntegrationTests.ResourceDefinitions.Reading;
22

33
internal sealed class TestClientSettingsProvider : IClientSettingsProvider
44
{
5+
public bool AreVeryLargeStarsHidden { get; private set; }
6+
public bool AreConstellationsVisibleDuringWinterHidden { get; private set; }
57
public bool IsIncludePlanetMoonsBlocked { get; private set; }
68
public bool ArePlanetsWithPrivateNameHidden { get; private set; }
79
public bool IsStarGivingLightToMoonAutoIncluded { get; private set; }
810

911
public void ResetToDefaults()
1012
{
13+
AreVeryLargeStarsHidden = false;
14+
AreConstellationsVisibleDuringWinterHidden = false;
1115
IsIncludePlanetMoonsBlocked = false;
1216
ArePlanetsWithPrivateNameHidden = false;
1317
IsStarGivingLightToMoonAutoIncluded = false;
1418
}
1519

20+
public void HideVeryLargeStars()
21+
{
22+
AreVeryLargeStarsHidden = true;
23+
}
24+
25+
public void HideConstellationsVisibleDuringWinter()
26+
{
27+
AreConstellationsVisibleDuringWinterHidden = true;
28+
}
29+
1630
public void BlockIncludePlanetMoons()
1731
{
1832
IsIncludePlanetMoonsBlocked = true;

0 commit comments

Comments
 (0)