Skip to content

fix #427 - fix ConnectionPaging GetSkipNumber off-by-one when using Before arg, fix handling when Last > BeforeNum #428

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
lukemurray merged 1 commit into from
Nov 12, 2024
Merged

fix #427 - fix ConnectionPaging GetSkipNumber off-by-one when using Before arg, fix handling when Last > BeforeNum #428

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions src/EntityGraphQL/Schema/FieldExtensions/ConnectionPaging/ConnectionEdgeExtension.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,9 +100,10 @@ CompileContext compileContext
nameof(EnumerableExtensions.Skip),
[listType],
expression,
Expression.Call(typeof(ConnectionHelper), nameof(ConnectionHelper.GetSkipNumber), null, argumentParam)
Expression.Call(typeof(ConnectionHelper), nameof(ConnectionHelper.GetSkipNumber), null, argumentParam, Expression.Constant(true))
),
Expression.Call(typeof(ConnectionHelper), nameof(ConnectionHelper.GetTakeNumber), null, argumentParam)
Expression.Call(typeof(ConnectionHelper), nameof(ConnectionHelper.GetTakeNumber), null, argumentParam,
Expression.Call(typeof(ConnectionHelper), nameof(ConnectionHelper.GetSkipNumber), null, argumentParam, Expression.Constant(false)))
);

// we have moved the expression from the parent node to here. We need to call the before callback
Expand Down Expand Up @@ -175,6 +176,7 @@ ParameterReplacer parameterReplacer
);

var idxParam = Expression.Parameter(typeof(int), "cursor_idx");
var offsetParam = Expression.Call(typeof(ConnectionHelper), nameof(ConnectionHelper.GetSkipNumber), null, argumentParam, Expression.Constant(false));
// now select with cursor
baseExpression = Expression.Call(
typeof(Enumerable),
Expand All @@ -187,7 +189,7 @@ ParameterReplacer parameterReplacer
new List<MemberBinding>
{
Expression.Bind(edgeType.GetProperty("Node")!, Expression.PropertyOrField(edgeParam, "Node")),
Expression.Bind(edgeType.GetProperty("Cursor")!, Expression.Call(typeof(ConnectionHelper), "GetCursor", null, argumentParam, idxParam))
Expression.Bind(edgeType.GetProperty("Cursor")!, Expression.Call(typeof(ConnectionHelper), "GetCursor", null, argumentParam, idxParam, offsetParam))
}
),
edgeParam,
Expand Down
26 changes: 20 additions & 6 deletions src/EntityGraphQL/Schema/FieldExtensions/ConnectionPaging/ConnectionHelper.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ public static unsafe string SerializeCursor(int index)
/// <summary>
/// Used at runtime in the expression built above
/// </summary>
public static string GetCursor(dynamic arguments, int idx)
public static string GetCursor(dynamic arguments, int idx, int? offset = null)
{
var index = idx + 1;
if (arguments.AfterNum != null)
Expand All @@ -76,29 +76,43 @@ public static string GetCursor(dynamic arguments, int idx)
else
index += arguments.TotalCount - (arguments.Last ?? 0);
}

if (offset < 0) index = idx + 1;

return SerializeCursor(index);
}

/// <summary>
/// Used at runtime in the expression built above
/// </summary>
public static int? GetSkipNumber(dynamic arguments)
public static int? GetSkipNumber(dynamic arguments, bool fixNegativeOffset = true)
{
if (arguments.AfterNum != null)
return arguments.AfterNum;
if (arguments.Last != null)
return (arguments.BeforeNum ?? arguments.TotalCount) - arguments.Last;
{
var c = ((arguments.BeforeNum-1) ?? arguments.TotalCount) - arguments.Last;

// Enumerable.Skip does not accept negative numbers.
if (fixNegativeOffset)
c = c > 0 ? c : 0;
return c;
}
return 0;
}

/// <summary>
/// Used at runtime in the expression built above
/// </summary>
public static int? GetTakeNumber(dynamic arguments)
public static int? GetTakeNumber(dynamic arguments, int? offset = 0)
{
if (arguments.First == null && arguments.Last == null && arguments.BeforeNum == null)
if (arguments.First == null && arguments.Last == null && arguments.BeforeNum == null || offset == null)
return null;
return arguments.First ?? arguments.Last ?? (arguments.BeforeNum - 1);

// In cases where we have Last > BeforeNum, we need to take fewer results than Last says to
// See SkipTakeTests.TestLastAndBefore_WhenLastGreaterThanBeforeNum
var offsetAdjustedLast = offset >= 0 ? arguments.Last : arguments.Last + offset;
return arguments.First ?? offsetAdjustedLast ?? (arguments.BeforeNum - 1);
}
}
}
72 changes: 72 additions & 0 deletions src/tests/EntityGraphQL.Tests/ConnectionPaging/ConnectionPagingTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -263,6 +263,78 @@ name id
Assert.Equal(expectedLastCursor, Enumerable.Last(people.edges).cursor);
}

[Fact]
public void TestCursorFirstAfterMatchesLastBefore()
{
// Issue #427
var schema = SchemaBuilder.FromObject<TestDataContext>();
var data = new TestDataContext();
FillData(data);

schema.Query().ReplaceField("people", ctx => ctx.People.OrderBy(p => p.Id), "Return list of people with paging metadata").UseConnectionPaging();
var gql = new QueryRequest
{
Query =
@"{
people(first: 2 after: ""MQ=="") {
edges {
node {
name id
}
cursor
}
pageInfo {
startCursor
endCursor
hasNextPage
hasPreviousPage
}
totalCount
}
}",
};

var result = schema.ExecuteRequestWithContext(gql, data, null, null);
Assert.Null(result.Errors);

dynamic firstPeople = result.Data!["people"]!;

var gql2 = new QueryRequest
{
Query =
@"{
people(last: 2 before: ""NA=="") {
edges {
node {
name id
}
cursor
}
pageInfo {
startCursor
endCursor
hasNextPage
hasPreviousPage
}
totalCount
}
}",
};

result = schema.ExecuteRequestWithContext(gql2, data, null, null);
Assert.Null(result.Errors);

dynamic lastPeople = result.Data!["people"]!;

// cursors MQ, Mg, Mw, NA, NQ
// first Mg, Mw
// last Mg, Mw

Assert.Equal(firstPeople.pageInfo.startCursor, lastPeople.pageInfo.startCursor);
Assert.Equal(Enumerable.First(firstPeople.edges).cursor, Enumerable.First(lastPeople.edges).cursor);
Assert.Equal(Enumerable.First(firstPeople.edges).node.id, Enumerable.First(lastPeople.edges).node.id);
}

[Fact]
public void TestMergeArguments()
{
Expand Down
17 changes: 16 additions & 1 deletion src/tests/EntityGraphQL.Tests/ConnectionPaging/SkipTakeTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,22 @@ public void TestLastAndBefore()
Assert.Equal(4, take);

var skip = ConnectionHelper.GetSkipNumber(args);
Assert.Equal(3, skip);
Assert.Equal(2, skip);
}

[Fact]
public void TestLastAndBefore_WhenLastGreaterThanBeforeNum()
{
var args = new ConnectionArgs { Last = 4, BeforeNum = 2, };

var skip = ConnectionHelper.GetSkipNumber(args);
Assert.Equal(0, skip);

var offset = ConnectionHelper.GetSkipNumber(args, false);

var take = ConnectionHelper.GetTakeNumber(args, offset);
Assert.Equal(1, take);

}

[Fact]
Expand Down