From ce7f77c585850c6d0a5bba070a863943313cecb3 Mon Sep 17 00:00:00 2001 From: rstoyanchev Date: Tue, 26 Mar 2024 20:34:52 +0000 Subject: [PATCH] Improve logic to resolve pagination arguments The presence of either "after" or "first" leads to forward pagination. Else if either "before" or "last" leads to backward pagination. Fall back on forward if none are provided at all. In addition, a small adjustment to backward pagination. If a count is not provided, use 1 rather than 0, to advance to previous item. Closes gh-929 --- .../SubrangeMethodArgumentResolver.java | 19 ++- .../graphql/data/query/RepositoryUtils.java | 23 ++-- .../graphql/data/query/ScrollSubrange.java | 7 +- .../SubrangeMethodArgumentResolverTests.java | 82 ++++++++++--- .../data/query/RepositoryUtilsTests.java | 109 +++++++++++------- .../data/query/ScrollSubrangeTests.java | 2 +- 6 files changed, 165 insertions(+), 77 deletions(-) diff --git a/spring-graphql/src/main/java/org/springframework/graphql/data/method/annotation/support/SubrangeMethodArgumentResolver.java b/spring-graphql/src/main/java/org/springframework/graphql/data/method/annotation/support/SubrangeMethodArgumentResolver.java index 2bb37b93..e39ef21b 100644 --- a/spring-graphql/src/main/java/org/springframework/graphql/data/method/annotation/support/SubrangeMethodArgumentResolver.java +++ b/spring-graphql/src/main/java/org/springframework/graphql/data/method/annotation/support/SubrangeMethodArgumentResolver.java @@ -1,5 +1,5 @@ /* - * Copyright 2020-2023 the original author or authors. + * Copyright 2020-2024 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -52,11 +52,18 @@ public class SubrangeMethodArgumentResolver

implements HandlerMethodArgumentR @Override public Object resolveArgument(MethodParameter parameter, DataFetchingEnvironment environment) throws Exception { - boolean forward = !environment.getArguments().containsKey("last"); - Integer count = environment.getArgument(forward ? "first" : "last"); - String cursor = environment.getArgument(forward ? "after" : "before"); - P position = (cursor != null ? this.cursorStrategy.fromCursor(cursor) : null); - return createSubrange(position, count, forward); + boolean forward = true; + String cursor = environment.getArgument("after"); + Integer count = environment.getArgument("first"); + if (cursor == null && count == null) { + cursor = environment.getArgument("before"); + count = environment.getArgument("last"); + if (cursor != null || count != null) { + forward = false; + } + } + P pos = (cursor != null ? this.cursorStrategy.fromCursor(cursor) : null); + return createSubrange(pos, count, forward); } /** diff --git a/spring-graphql/src/main/java/org/springframework/graphql/data/query/RepositoryUtils.java b/spring-graphql/src/main/java/org/springframework/graphql/data/query/RepositoryUtils.java index 3734f1c1..4c18a242 100644 --- a/spring-graphql/src/main/java/org/springframework/graphql/data/query/RepositoryUtils.java +++ b/spring-graphql/src/main/java/org/springframework/graphql/data/query/RepositoryUtils.java @@ -93,18 +93,21 @@ class RepositoryUtils { return forward -> ScrollPosition.offset(); } - public static ScrollSubrange defaultScrollSubrange() { - return ScrollSubrange.create(ScrollPosition.offset(), 20, true); - } - public static ScrollSubrange getScrollSubrange( - DataFetchingEnvironment env, CursorStrategy strategy) { + DataFetchingEnvironment env, CursorStrategy cursorStrategy) { - boolean forward = !env.getArguments().containsKey("last"); - Integer count = env.getArgument(forward ? "first" : "last"); - String cursor = env.getArgument(forward ? "after" : "before"); - ScrollPosition position = (cursor != null ? strategy.fromCursor(cursor) : null); - return ScrollSubrange.create(position, count, forward); + boolean forward = true; + String cursor = env.getArgument("after"); + Integer count = env.getArgument("first"); + if (cursor == null && count == null) { + cursor = env.getArgument("before"); + count = env.getArgument("last"); + if (cursor != null || count != null) { + forward = false; + } + } + ScrollPosition pos = (cursor != null ? cursorStrategy.fromCursor(cursor) : null); + return ScrollSubrange.create(pos, count, forward); } } diff --git a/spring-graphql/src/main/java/org/springframework/graphql/data/query/ScrollSubrange.java b/spring-graphql/src/main/java/org/springframework/graphql/data/query/ScrollSubrange.java index 4aefdcf6..8507becd 100644 --- a/spring-graphql/src/main/java/org/springframework/graphql/data/query/ScrollSubrange.java +++ b/spring-graphql/src/main/java/org/springframework/graphql/data/query/ScrollSubrange.java @@ -110,9 +110,10 @@ public final class ScrollSubrange extends Subrange { position = position.advanceBy(1); } else { - int countOrZero = (count != null ? count : 0); - if (position.getOffset() >= countOrZero) { - position = position.advanceBy(-countOrZero); + // Advance back by 1 at least to item before position + int advanceCount = (count != null ? count : 1); + if (position.getOffset() >= advanceCount) { + position = position.advanceBy(-advanceCount); } else { count = (int) position.getOffset(); diff --git a/spring-graphql/src/test/java/org/springframework/graphql/data/method/annotation/support/SubrangeMethodArgumentResolverTests.java b/spring-graphql/src/test/java/org/springframework/graphql/data/method/annotation/support/SubrangeMethodArgumentResolverTests.java index 8043355f..3826a23c 100644 --- a/spring-graphql/src/test/java/org/springframework/graphql/data/method/annotation/support/SubrangeMethodArgumentResolverTests.java +++ b/spring-graphql/src/test/java/org/springframework/graphql/data/method/annotation/support/SubrangeMethodArgumentResolverTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2020-2023 the original author or authors. + * Copyright 2020-2024 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -16,10 +16,9 @@ package org.springframework.graphql.data.method.annotation.support; +import java.util.Collections; import java.util.Map; -import graphql.schema.DataFetchingEnvironment; -import graphql.schema.DataFetchingEnvironmentImpl; import org.junit.jupiter.api.Test; import org.springframework.core.MethodParameter; @@ -28,6 +27,7 @@ import org.springframework.graphql.Book; import org.springframework.graphql.data.method.annotation.QueryMapping; import org.springframework.graphql.data.pagination.CursorStrategy; import org.springframework.graphql.data.pagination.Subrange; +import org.springframework.lang.Nullable; import org.springframework.stereotype.Controller; import static org.assertj.core.api.Assertions.assertThat; @@ -41,8 +41,7 @@ public class SubrangeMethodArgumentResolverTests extends ArgumentResolverTestSup private final SubrangeMethodArgumentResolver resolver = new SubrangeMethodArgumentResolver<>(new MyPositionCursorStrategy()); - private final MethodParameter param = - methodParam(BookController.class, "getBooks", Subrange.class); + private final MethodParameter param = methodParam(BookController.class, "getBooks", Subrange.class); @Test @@ -54,30 +53,87 @@ public class SubrangeMethodArgumentResolverTests extends ArgumentResolverTestSup } @Test - void forwardPagination() throws Exception { + void forward() throws Exception { int count = 10; int index = 25; Map arguments = Map.of("first", count, "after", String.valueOf(index)); Object result = this.resolver.resolveArgument(this.param, environment(arguments)); - testRequest(count, index, result, true); + assertResult(true, count, index, result); } @Test - void backwardPagination() throws Exception { + void forwardWithCountOnly() throws Exception { + int count = 10; + Map arguments = Map.of("first", count); + Object result = this.resolver.resolveArgument(this.param, environment(arguments)); + + assertResult(true, count, null, result); + } + + @Test + void forwardWithIndexOnly() throws Exception { + int index = 25; + Map arguments = Map.of("after", String.valueOf(index)); + Object result = this.resolver.resolveArgument(this.param, environment(arguments)); + + assertResult(true, null, index, result); + } + + @Test + void backward() throws Exception { int count = 20; int index = 100; Map arguments = Map.of("last", count, "before", String.valueOf(index)); Object result = this.resolver.resolveArgument(this.param, environment(arguments)); - testRequest(count, index, result, false); + assertResult(false, count, index, result); } - private static void testRequest(int count, int index, Object result, boolean forward) { + @Test + void backwardWithCountOnly() throws Exception { + int count = 10; + Map arguments = Map.of("last", count); + Object result = this.resolver.resolveArgument(this.param, environment(arguments)); + + assertResult(false, count, null, result); + } + + @Test + void backwardWithIndexOnly() throws Exception { + int index = 25; + Map arguments = Map.of("before", String.valueOf(index)); + Object result = this.resolver.resolveArgument(this.param, environment(arguments)); + + assertResult(false, null, index, result); + } + + @Test + void noInput() throws Exception { + Object result = this.resolver.resolveArgument(this.param, environment(Collections.emptyMap())); + assertResult(true, null, null, result); + } + + private static void assertResult( + boolean forward, @Nullable Integer count, @Nullable Integer index, @Nullable Object result) { + + assertThat(result).isNotNull(); Subrange subrange = (Subrange) result; - assertThat(subrange.position().get().index()).isEqualTo(index); - assertThat(subrange.count().orElse(0)).isEqualTo(count); assertThat(subrange.forward()).isEqualTo(forward); + + if (count != null) { + assertThat(subrange.count().orElse(0)).isEqualTo(count); + } + else { + assertThat(subrange.count()).isNotPresent(); + } + + if (index != null) { + assertThat(subrange.position().get().index()).isEqualTo(index); + } + else { + assertThat(subrange.position()).isNotPresent(); + } } @@ -94,7 +150,6 @@ public class SubrangeMethodArgumentResolverTests extends ArgumentResolverTestSup public Window getBooksWithUnknownPosition(Subrange subrange) { return null; } - } @@ -115,7 +170,6 @@ public class SubrangeMethodArgumentResolverTests extends ArgumentResolverTestSup public MyPosition fromCursor(String cursor) { return new MyPosition(Integer.parseInt(cursor)); } - } diff --git a/spring-graphql/src/test/java/org/springframework/graphql/data/query/RepositoryUtilsTests.java b/spring-graphql/src/test/java/org/springframework/graphql/data/query/RepositoryUtilsTests.java index 239fab79..ccf75350 100644 --- a/spring-graphql/src/test/java/org/springframework/graphql/data/query/RepositoryUtilsTests.java +++ b/spring-graphql/src/test/java/org/springframework/graphql/data/query/RepositoryUtilsTests.java @@ -22,6 +22,7 @@ import java.util.Map; import graphql.schema.DataFetchingEnvironment; import graphql.schema.DataFetchingEnvironmentImpl; import org.junit.jupiter.api.Test; +import org.testcontainers.shaded.org.checkerframework.checker.nullness.qual.Nullable; import org.springframework.data.domain.OffsetScrollPosition; import org.springframework.data.domain.ScrollPosition; @@ -39,33 +40,59 @@ public class RepositoryUtilsTests { @Test - void buildScrollSubrangeForward() { - OffsetScrollPosition offset = ScrollPosition.offset(10); + void forward() { + OffsetScrollPosition pos = ScrollPosition.offset(10); int count = 5; - - DataFetchingEnvironment env = environment( - Map.of("first", count, "after", cursorStrategy.toCursor(offset))); - + DataFetchingEnvironment env = environment(Map.of("first", count, "after", cursorStrategy.toCursor(pos))); ScrollSubrange range = RepositoryUtils.getScrollSubrange(env, cursorStrategy); - assertThat(range.position().get()).isEqualTo(ScrollPosition.offset(11)); - assertThat(range.count().getAsInt()).isEqualTo(count); - assertThat(range.forward()).isTrue(); + assertSubrange(true, count, pos.advanceBy(1), range); } @Test - void buildScrollSubrangeBackward() { - OffsetScrollPosition offset = ScrollPosition.offset(10); + void backward() { + OffsetScrollPosition pos = ScrollPosition.offset(10); int count = 5; - - DataFetchingEnvironment env = environment( - Map.of("last", count, "before", cursorStrategy.toCursor(offset))); - + DataFetchingEnvironment env = environment(Map.of("last", count, "before", cursorStrategy.toCursor(pos))); ScrollSubrange range = RepositoryUtils.getScrollSubrange(env, cursorStrategy); - assertThat(range.position().get()).isEqualTo(ScrollPosition.offset(5)); - assertThat(range.count().getAsInt()).isEqualTo(count); - assertThat(range.forward()).isTrue(); + assertSubrange(true, count, ScrollPosition.offset(5), range); + } + + @Test + void forwardWithCountOnly() { + int count = 5; + DataFetchingEnvironment env = environment(Map.of("first", count)); + ScrollSubrange range = RepositoryUtils.getScrollSubrange(env, cursorStrategy); + + assertSubrange(true, count, null, range); + } + + @Test + void forwardWithPositionOnly() { + OffsetScrollPosition pos = ScrollPosition.offset(10); + DataFetchingEnvironment env = environment(Map.of("after", cursorStrategy.toCursor(pos))); + ScrollSubrange range = RepositoryUtils.getScrollSubrange(env, cursorStrategy); + + assertSubrange(true, null, pos.advanceBy(1), range); + } + + @Test + void backwardWithCountOnly() { + int count = 5; + DataFetchingEnvironment env = environment(Map.of("last", count)); + ScrollSubrange range = RepositoryUtils.getScrollSubrange(env, cursorStrategy); + + assertSubrange(false, count, null, range); + } + + @Test + void backwardWithPositionOnly() { + OffsetScrollPosition pos = ScrollPosition.offset(10); + DataFetchingEnvironment env = environment(Map.of("before", cursorStrategy.toCursor(pos))); + ScrollSubrange range = RepositoryUtils.getScrollSubrange(env, cursorStrategy); + + assertSubrange(true, null, pos.advanceBy(-1), range); } @Test @@ -73,31 +100,7 @@ public class RepositoryUtilsTests { DataFetchingEnvironment env = environment(Collections.emptyMap()); ScrollSubrange range = RepositoryUtils.getScrollSubrange(env, cursorStrategy); - assertThat(range.position()).isNotPresent(); - assertThat(range.count()).isNotPresent(); - assertThat(range.forward()).isTrue(); - } - - @Test - void buildScrollSubrangeForwardWithoutPosition() { - int count = 5; - DataFetchingEnvironment env = environment(Map.of("first", count)); - ScrollSubrange range = RepositoryUtils.getScrollSubrange(env, cursorStrategy); - - assertThat(range.position()).isNotPresent(); - assertThat(range.count().getAsInt()).isEqualTo(count); - assertThat(range.forward()).isTrue(); - } - - @Test - void buildScrollSubrangeBackwardWithoutPosition() { - int count = 5; - DataFetchingEnvironment env = environment(Map.of("last", count)); - ScrollSubrange range = RepositoryUtils.getScrollSubrange(env, cursorStrategy); - - assertThat(range.position()).isNotPresent(); - assertThat(range.count().getAsInt()).isEqualTo(count); - assertThat(range.forward()).isFalse(); + assertSubrange(true, null, null, range); } private static DataFetchingEnvironment environment(Map arguments) { @@ -106,4 +109,24 @@ public class RepositoryUtilsTests { .build(); } + private static void assertSubrange( + boolean forward, @Nullable Integer count, @Nullable ScrollPosition pos, ScrollSubrange subrange) { + + assertThat(subrange.forward()).isEqualTo(forward); + + if (count != null) { + assertThat(subrange.count().orElse(0)).isEqualTo(count); + } + else { + assertThat(subrange.count()).isNotPresent(); + } + + if (pos != null) { + assertThat(subrange.position().get()).isEqualTo(pos); + } + else { + assertThat(subrange.position()).isNotPresent(); + } + } + } diff --git a/spring-graphql/src/test/java/org/springframework/graphql/data/query/ScrollSubrangeTests.java b/spring-graphql/src/test/java/org/springframework/graphql/data/query/ScrollSubrangeTests.java index c8babeea..bc317ae4 100644 --- a/spring-graphql/src/test/java/org/springframework/graphql/data/query/ScrollSubrangeTests.java +++ b/spring-graphql/src/test/java/org/springframework/graphql/data/query/ScrollSubrangeTests.java @@ -121,7 +121,7 @@ public class ScrollSubrangeTests { void offsetBackwardWithNullCount() { ScrollSubrange subrange = ScrollSubrange.create(ScrollPosition.offset(30), null, false); - assertThat(getOffset(subrange)).isEqualTo(30); + assertThat(getOffset(subrange)).isEqualTo(29); assertThat(subrange.count()).isNotPresent(); assertThat(subrange.forward()).isTrue(); }