From b2e436a3e0dfe42020873ece132c9f61d70e8993 Mon Sep 17 00:00:00 2001 From: Elliot Wannemacher Date: Mon, 11 Nov 2024 13:08:52 -0500 Subject: [PATCH] fix #427 - change ConnectionPaging Last to be exclusive, fix handling when Last > BeforeNum --- .../ConnectionEdgeExtension.cs | 8 ++- .../ConnectionPaging/ConnectionHelper.cs | 26 +++++-- .../ConnectionPaging/ConnectionPagingTests.cs | 72 +++++++++++++++++++ .../ConnectionPaging/SkipTakeTests.cs | 17 ++++- 4 files changed, 113 insertions(+), 10 deletions(-) diff --git a/src/EntityGraphQL/Schema/FieldExtensions/ConnectionPaging/ConnectionEdgeExtension.cs b/src/EntityGraphQL/Schema/FieldExtensions/ConnectionPaging/ConnectionEdgeExtension.cs index 680c4d9b..b625bdb6 100644 --- a/src/EntityGraphQL/Schema/FieldExtensions/ConnectionPaging/ConnectionEdgeExtension.cs +++ b/src/EntityGraphQL/Schema/FieldExtensions/ConnectionPaging/ConnectionEdgeExtension.cs @@ -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 @@ -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), @@ -187,7 +189,7 @@ ParameterReplacer parameterReplacer new List { 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, diff --git a/src/EntityGraphQL/Schema/FieldExtensions/ConnectionPaging/ConnectionHelper.cs b/src/EntityGraphQL/Schema/FieldExtensions/ConnectionPaging/ConnectionHelper.cs index 3a6e4194..49035429 100644 --- a/src/EntityGraphQL/Schema/FieldExtensions/ConnectionPaging/ConnectionHelper.cs +++ b/src/EntityGraphQL/Schema/FieldExtensions/ConnectionPaging/ConnectionHelper.cs @@ -64,7 +64,7 @@ public static unsafe string SerializeCursor(int index) /// /// Used at runtime in the expression built above /// - 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) @@ -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); } /// /// Used at runtime in the expression built above /// - 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; } /// /// Used at runtime in the expression built above /// - 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); } } } diff --git a/src/tests/EntityGraphQL.Tests/ConnectionPaging/ConnectionPagingTests.cs b/src/tests/EntityGraphQL.Tests/ConnectionPaging/ConnectionPagingTests.cs index a02b58f8..84bd9f37 100644 --- a/src/tests/EntityGraphQL.Tests/ConnectionPaging/ConnectionPagingTests.cs +++ b/src/tests/EntityGraphQL.Tests/ConnectionPaging/ConnectionPagingTests.cs @@ -263,6 +263,78 @@ name id Assert.Equal(expectedLastCursor, Enumerable.Last(people.edges).cursor); } + [Fact] + public void TestCursorFirstAfterMatchesLastBefore() + { + // Issue #427 + var schema = SchemaBuilder.FromObject(); + 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() { diff --git a/src/tests/EntityGraphQL.Tests/ConnectionPaging/SkipTakeTests.cs b/src/tests/EntityGraphQL.Tests/ConnectionPaging/SkipTakeTests.cs index 12a52e76..f7c38960 100644 --- a/src/tests/EntityGraphQL.Tests/ConnectionPaging/SkipTakeTests.cs +++ b/src/tests/EntityGraphQL.Tests/ConnectionPaging/SkipTakeTests.cs @@ -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]