Skip to content

Commit 334abdc

Browse files
gbiellemgbiellem
authored
Fix connection ordering check when Include() strips IOrderedQueryable for entities with read-only properties (#1262)
1 parent 5865e86 commit 334abdc

6 files changed

+271
-21
lines changed

‎src/GraphQL.EntityFramework/ConnectionConverter.cs‎

Lines changed: 25 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,27 @@
1-
static class ConnectionConverter
1+
static class ConnectionConverter
22
{
3+
internal static bool HasOrderingInExpressionTree(Expression expression)
4+
{
5+
if (expression is MethodCallExpression methodCall)
6+
{
7+
var methodName = methodCall.Method.Name;
8+
if (methodName is "OrderBy" or "OrderByDescending" or "ThenBy" or "ThenByDescending")
9+
{
10+
return true;
11+
}
12+
13+
foreach (var arg in methodCall.Arguments)
14+
{
15+
if (HasOrderingInExpressionTree(arg))
16+
{
17+
return true;
18+
}
19+
}
20+
}
21+
22+
return false;
23+
}
24+
325
public static Connection<T> ApplyConnectionContext<T>(List<T> list, int? first, string? afterString, int? last, string? beforeString)
426
where T : class
527
{
@@ -98,7 +120,7 @@ public static async Task<Connection<TItem>> ApplyConnectionContext<TDbContext, T
98120
where TItem : class
99121
where TDbContext : DbContext
100122
{
101-
if (queryable is not IOrderedQueryable<TItem>)
123+
if (queryable is not IOrderedQueryable<TItem> && !HasOrderingInExpressionTree(queryable.Expression))
102124
{
103125
throw new($"Connections require ordering. Either order the IQueryable being passed to AddQueryConnectionField, or use an orderBy in the query. Field: {context.FieldDefinition.Name}");
104126
}
@@ -229,4 +251,4 @@ static void Parse(string? afterString, string? beforeString, out int? after, out
229251
before = int.Parse(beforeString);
230252
}
231253
}
232-
}
254+
}

‎src/Tests/IntegrationTests/IntegrationTests.Connection_with_read_only_property_and_navigation.verified.txt‎

Lines changed: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,60 @@
1+
{
2+
target: {
3+
Data: {
4+
readOnlyEntitiesConnection: {
5+
totalCount: 3,
6+
items: [
7+
{
8+
firstName: Alice,
9+
computedInDb: Alice A,
10+
readOnlyParent: {
11+
property: Parent1
12+
}
13+
},
14+
{
15+
firstName: Bob,
16+
computedInDb: Bob B,
17+
readOnlyParent: {
18+
property: Parent1
19+
}
20+
}
21+
]
22+
}
23+
}
24+
},
25+
sql: [
26+
{
27+
Text:
28+
select COUNT(*)
29+
from ReadOnlyEntities as r
30+
},
31+
{
32+
Text:
33+
select r1.Id,
34+
r1.Age,
35+
r1.ComputedInDb,
36+
r1.FirstName,
37+
r1.LastName,
38+
r1.ReadOnlyParentId,
39+
r0.Id,
40+
r0.Property
41+
from (select r.Id,
42+
r.Age,
43+
r.ComputedInDb,
44+
r.FirstName,
45+
r.LastName,
46+
r.ReadOnlyParentId
47+
from ReadOnlyEntities as r
48+
order by r.FirstName
49+
offset @p rows fetch next @p1 rows only) as r1
50+
left outer join
51+
ReadOnlyParentEntities as r0
52+
on r1.ReadOnlyParentId = r0.Id
53+
order by r1.FirstName,
54+
Parameters: {
55+
@p: 0,
56+
@p1: 2
57+
}
58+
}
59+
]
60+
}

‎src/Tests/IntegrationTests/IntegrationTests.Connection_with_read_only_property_navigation_and_fragment.verified.txt‎

Lines changed: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,60 @@
1+
{
2+
target: {
3+
Data: {
4+
readOnlyEntitiesConnection: {
5+
totalCount: 3,
6+
items: [
7+
{
8+
firstName: Alice,
9+
computedInDb: Alice A,
10+
readOnlyParent: {
11+
property: Parent1
12+
}
13+
},
14+
{
15+
firstName: Bob,
16+
computedInDb: Bob B,
17+
readOnlyParent: {
18+
property: Parent1
19+
}
20+
}
21+
]
22+
}
23+
}
24+
},
25+
sql: [
26+
{
27+
Text:
28+
select COUNT(*)
29+
from ReadOnlyEntities as r
30+
},
31+
{
32+
Text:
33+
select r1.Id,
34+
r1.Age,
35+
r1.ComputedInDb,
36+
r1.FirstName,
37+
r1.LastName,
38+
r1.ReadOnlyParentId,
39+
r0.Id,
40+
r0.Property
41+
from (select r.Id,
42+
r.Age,
43+
r.ComputedInDb,
44+
r.FirstName,
45+
r.LastName,
46+
r.ReadOnlyParentId
47+
from ReadOnlyEntities as r
48+
order by r.FirstName
49+
offset @p rows fetch next @p1 rows only) as r1
50+
left outer join
51+
ReadOnlyParentEntities as r0
52+
on r1.ReadOnlyParentId = r0.Id
53+
order by r1.FirstName,
54+
Parameters: {
55+
@p: 0,
56+
@p1: 2
57+
}
58+
}
59+
]
60+
}

‎src/Tests/IntegrationTests/IntegrationTests.SchemaPrint.verified.txt‎

Lines changed: 50 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,18 @@
3737
where: [WhereExpression!],
3838
orderBy: [OrderBy!],
3939
ids: [ID!]): ChildConnection!
40+
readOnlyEntitiesConnection(
41+
"Only return edges after the specified cursor."
42+
after: String,
43+
"Specifies the maximum number of edges to return, starting after the cursor specified by 'after', or the first number of edges if 'after' is not specified."
44+
first: Int,
45+
"Only return edges prior to the specified cursor."
46+
before: String,
47+
"Specifies the maximum number of edges to return, starting prior to the cursor specified by 'before', or the last number of edges if 'before' is not specified."
48+
last: Int,
49+
where: [WhereExpression!],
50+
orderBy: [OrderBy!],
51+
ids: [ID!]): ReadOnlyEntityConnection!
4052
parentEntitiesFiltered(id: ID, ids: [ID!], where: [WhereExpression!], orderBy: [OrderBy!], skip: Int, take: Int): [FilterParent!]!
4153
parentEntitiesConnectionFiltered(
4254
"Only return edges after the specified cursor."
@@ -413,6 +425,44 @@ type ParentEdge {
413425
node: Parent!
414426
}
415427

428+
"A connection from an object to a list of objects of type `ReadOnlyEntity`."
429+
type ReadOnlyEntityConnection {
430+
"A count of the total number of objects in this connection, ignoring pagination. This allows a client to fetch the first five objects by passing \"5\" as the argument to `first`, then fetch the total count so it could display \"5 of 83\", for example. In cases where we employ infinite scrolling or don't have an exact count of entries, this field will return `null`."
431+
totalCount: Int
432+
"Information to aid in pagination."
433+
pageInfo: PageInfo!
434+
"A list of all of the edges returned in the connection."
435+
edges: [ReadOnlyEntityEdge]
436+
"A list of all of the objects returned in the connection. This is a convenience field provided for quickly exploring the API; rather than querying for \"{ edges { node } }\" when no edge data is needed, this field can be used instead. Note that when clients like Relay need to fetch the \"cursor\" field on the edge to enable efficient pagination, this shortcut cannot be used, and the full \"{ edges { node } } \" version should be used instead."
437+
items: [ReadOnlyEntity!]
438+
}
439+
440+
"An edge in a connection from an object to another object of type `ReadOnlyEntity`."
441+
type ReadOnlyEntityEdge {
442+
"A cursor for use in pagination"
443+
cursor: String!
444+
"The item at the end of the edge"
445+
node: ReadOnlyEntity!
446+
}
447+
448+
type ReadOnlyEntity {
449+
readOnlyParent: ReadOnlyParent
450+
age: Int!
451+
computedInDb: String!
452+
displayName: String!
453+
firstName: String
454+
id: ID!
455+
isAdult: Boolean!
456+
lastName: String
457+
readOnlyParentId: ID
458+
}
459+
460+
type ReadOnlyParent {
461+
children(id: ID, ids: [ID!], where: [WhereExpression!], orderBy: [OrderBy!], skip: Int, take: Int): [ReadOnlyEntity!]!
462+
id: ID!
463+
property: String
464+
}
465+
416466
type FilterParent {
417467
childrenConnection(
418468
"Only return edges after the specified cursor."
@@ -595,24 +645,6 @@ type Derived implements BaseEntity {
595645
status: String
596646
}
597647

598-
type ReadOnlyEntity {
599-
readOnlyParent: ReadOnlyParent
600-
age: Int!
601-
computedInDb: String!
602-
displayName: String!
603-
firstName: String
604-
id: ID!
605-
isAdult: Boolean!
606-
lastName: String
607-
readOnlyParentId: ID
608-
}
609-
610-
type ReadOnlyParent {
611-
children(id: ID, ids: [ID!], where: [WhereExpression!], orderBy: [OrderBy!], skip: Int, take: Int): [ReadOnlyEntity!]!
612-
id: ID!
613-
property: String
614-
}
615-
616648
type ManyToManyLeft {
617649
rights(id: ID, ids: [ID!], where: [WhereExpression!], orderBy: [OrderBy!], skip: Int, take: Int): [ManyToManyRight!]!
618650
id: String!

‎src/Tests/IntegrationTests/IntegrationTests_connection_ordering.cs‎

Lines changed: 67 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,67 @@
1+
public partial class IntegrationTests
2+
{
3+
[Fact]
4+
public async Task Connection_with_read_only_property_and_navigation()
5+
{
6+
// ReadOnlyEntity has read-only properties (DisplayName, IsAdult, ComputedInDb)
7+
// which cause SelectExpressionBuilder to bail (returns null).
8+
// This forces the AddIncludes path, where Include() strips IOrderedQueryable.
9+
// The ordering check in ConnectionConverter should still pass by inspecting
10+
// the expression tree for OrderBy/OrderByDescending.
11+
var query =
12+
"""
13+
{
14+
readOnlyEntitiesConnection(first:2) {
15+
totalCount
16+
items {
17+
firstName
18+
computedInDb
19+
readOnlyParent {
20+
property
21+
}
22+
}
23+
}
24+
}
25+
""";
26+
27+
var parent = new ReadOnlyParentEntity { Property = "Parent1" };
28+
var entity1 = new ReadOnlyEntity { FirstName = "Alice", LastName = "A", Age = 25, ReadOnlyParent = parent };
29+
var entity2 = new ReadOnlyEntity { FirstName = "Bob", LastName = "B", Age = 17, ReadOnlyParent = parent };
30+
var entity3 = new ReadOnlyEntity { FirstName = "Charlie", LastName = "C", Age = 30, ReadOnlyParent = parent };
31+
32+
await using var database = await sqlInstance.Build();
33+
await RunQuery(database, query, null, null, false, [parent, entity1, entity2, entity3]);
34+
}
35+
36+
[Fact]
37+
public async Task Connection_with_read_only_property_navigation_and_fragment()
38+
{
39+
var query =
40+
"""
41+
{
42+
readOnlyEntitiesConnection(first:2) {
43+
totalCount
44+
items {
45+
...readOnlyFields
46+
}
47+
}
48+
}
49+
50+
fragment readOnlyFields on ReadOnlyEntity {
51+
firstName
52+
computedInDb
53+
readOnlyParent {
54+
property
55+
}
56+
}
57+
""";
58+
59+
var parent = new ReadOnlyParentEntity { Property = "Parent1" };
60+
var entity1 = new ReadOnlyEntity { FirstName = "Alice", LastName = "A", Age = 25, ReadOnlyParent = parent };
61+
var entity2 = new ReadOnlyEntity { FirstName = "Bob", LastName = "B", Age = 17, ReadOnlyParent = parent };
62+
var entity3 = new ReadOnlyEntity { FirstName = "Charlie", LastName = "C", Age = 30, ReadOnlyParent = parent };
63+
64+
await using var database = await sqlInstance.Build();
65+
await RunQuery(database, query, null, null, false, [parent, entity1, entity2, entity3]);
66+
}
67+
}

‎src/Tests/IntegrationTests/Query.cs‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -80,6 +80,15 @@ public Query(IEfGraphQLService<IntegrationDbContext> efGraphQlService)
8080
name: "childEntitiesConnection",
8181
resolve: _ => _.DbContext.ChildEntities.OrderBy(_ => _.Parent));
8282

83+
// Connection with entity that has read-only properties.
84+
// Projection bails on read-only properties, falling back to AddIncludes.
85+
// Include() strips IOrderedQueryable, testing that the ordering check
86+
// handles this correctly.
87+
efGraphQlService.AddQueryConnectionField<ReadOnlyEntityGraphType, ReadOnlyEntity>(
88+
this,
89+
name: "readOnlyEntitiesConnection",
90+
resolve: _ => _.DbContext.ReadOnlyEntities.OrderBy(_ => _.FirstName));
91+
8392
AddQueryField(
8493
name: "parentEntitiesFiltered",
8594
resolve: _ => _.DbContext.FilterParentEntities);

0 commit comments

Comments
 (0)