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(); }