From 15bb8aa4821f399e16e8dd1d0318ccba13f5b7bc Mon Sep 17 00:00:00 2001 From: Mark Paluch Date: Wed, 5 Jul 2023 10:02:37 +0200 Subject: [PATCH] Polishing. Refine documentation. Introduce isLimited method to mirror isPresent/isSorted semantics. Introduce Pageable.toLimit() method to deduplicate code. See #2827 Original pull request: #2836 --- .../asciidoc/repositories-paging-sorting.adoc | 8 +- ...pository-query-return-types-reference.adoc | 2 +- .../springframework/data/domain/Limit.java | 50 ++++--- .../springframework/data/domain/Pageable.java | 16 +++ .../repository/query/ParameterAccessor.java | 8 +- .../query/ParametersParameterAccessor.java | 2 +- .../data/repository/query/QueryMethod.java | 124 +++++++++--------- .../data/domain/LimitUnitTests.java | 2 + 8 files changed, 118 insertions(+), 94 deletions(-) diff --git a/src/main/asciidoc/repositories-paging-sorting.adoc b/src/main/asciidoc/repositories-paging-sorting.adoc index 9f372c652..85e5172ab 100644 --- a/src/main/asciidoc/repositories-paging-sorting.adoc +++ b/src/main/asciidoc/repositories-paging-sorting.adoc @@ -64,23 +64,23 @@ By default, this query is derived from the query you actually trigger. [IMPORTANT] ==== Special parameters may only be used once within a query method. + -Some of the special parameters described above are mutually exclusive. +Some special parameters described above are mutually exclusive. Please consider the following list of invalid parameter combinations. |=== | Parameters | Example | Reason -| `Pageable` & `Sort` +| `Pageable` and `Sort` | `findBy...(Pageable page, Sort sort)` | `Pageable` already defines `Sort` -| `Pageable` & `Limit` +| `Pageable` and `Limit` | `findBy...(Pageable page, Limit limit)` | `Pageable` already defines a limit. |=== -The `Top` keyword used to limit results can be used to along with `Pageable` whereas `Top` defines the total maximum of results, whereas the Pageable parameter may reduce this this number. +The `Top` keyword used to limit results can be used to along with `Pageable` whereas `Top` defines the total maximum of results, whereas the Pageable parameter may reduce this number. ==== [[repositories.scrolling.guidance]] diff --git a/src/main/asciidoc/repository-query-return-types-reference.adoc b/src/main/asciidoc/repository-query-return-types-reference.adoc index a64027e21..7a33e30d8 100644 --- a/src/main/asciidoc/repository-query-return-types-reference.adoc +++ b/src/main/asciidoc/repository-query-return-types-reference.adoc @@ -25,7 +25,7 @@ Some store modules may define their own result wrapper types. |`Optional`|A Java 8 or Guava `Optional`. Expects the query method to return one result at most. If no result is found, `Optional.empty()` or `Optional.absent()` is returned. More than one result triggers an `IncorrectResultSizeDataAccessException`. |`Option`|Either a Scala or Vavr `Option` type. Semantically the same behavior as Java 8's `Optional`, described earlier. |`Stream`|A Java 8 `Stream`. -|`Streamable`|A convenience extension of `Iterable` that directy exposes methods to stream, map and filter results, concatenate them etc. +|`Streamable`|A convenience extension of `Iterable` that directly exposes methods to stream, map and filter results, concatenate them etc. |Types that implement `Streamable` and take a `Streamable` constructor or factory method argument|Types that expose a constructor or `….of(…)`/`….valueOf(…)` factory method taking a `Streamable` as argument. See <> for details. |Vavr `Seq`, `List`, `Map`, `Set`|Vavr collection types. See <> for details. |`Future`|A `Future`. Expects a method to be annotated with `@Async` and requires Spring's asynchronous method execution capability to be enabled. diff --git a/src/main/java/org/springframework/data/domain/Limit.java b/src/main/java/org/springframework/data/domain/Limit.java index 9cd68cf50..48719eb0a 100644 --- a/src/main/java/org/springframework/data/domain/Limit.java +++ b/src/main/java/org/springframework/data/domain/Limit.java @@ -28,25 +28,12 @@ import org.springframework.util.ClassUtils; * over using {@literal null} or {@link java.util.Optional#empty()} to indicate the absence of an actual {@link Limit}. *

* {@link Limit} itself does not make assumptions about the actual {@link #max()} value sign. This means that a negative - * value may be valid within a defined context. Implementations should override {@link #isUnlimited()} to fit their - * needs and enforce restrictions if needed. - * + * value may be valid within a defined context. + * * @author Christoph Strobl * @since 3.2 */ -public sealed interface Limit permits Limited, Unlimited { - - /** - * @return the max number of potential results. - */ - int max(); - - /** - * @return {@literal true} if no limiting (maximum value) should be applied. - */ - default boolean isUnlimited() { - return Unlimited.INSTANCE.equals(this); - } +public sealed interface Limit permits Limited,Unlimited { /** * @return a {@link Limit} instance that does not define {@link #max()} and answers {@link #isUnlimited()} with @@ -66,6 +53,23 @@ public sealed interface Limit permits Limited, Unlimited { return new Limited(max); } + /** + * @return the max number of potential results. + */ + int max(); + + /** + * @return {@literal true} if limiting (maximum value) should be applied. + */ + boolean isLimited(); + + /** + * @return {@literal true} if no limiting (maximum value) should be applied. + */ + default boolean isUnlimited() { + return !isLimited(); + } + final class Limited implements Limit { private final int max; @@ -79,6 +83,11 @@ public sealed interface Limit permits Limited, Unlimited { return max; } + @Override + public boolean isLimited() { + return true; + } + @Override public boolean equals(Object obj) { @@ -97,7 +106,7 @@ public sealed interface Limit permits Limited, Unlimited { @Override public int hashCode() { - return (int) (max ^ (max >>> 32)); + return max ^ (max >>> 32); } } @@ -110,12 +119,13 @@ public sealed interface Limit permits Limited, Unlimited { @Override public int max() { throw new IllegalStateException( - "Unlimited does not define 'max'. Please check 'isUnlimited' before attempting to read 'max'"); + "Unlimited does not define 'max'. Please check 'isLimited' before attempting to read 'max'"); } @Override - public boolean isUnlimited() { - return true; + public boolean isLimited() { + return false; } + } } diff --git a/src/main/java/org/springframework/data/domain/Pageable.java b/src/main/java/org/springframework/data/domain/Pageable.java index 08f1dca73..2d84b3f76 100644 --- a/src/main/java/org/springframework/data/domain/Pageable.java +++ b/src/main/java/org/springframework/data/domain/Pageable.java @@ -173,6 +173,22 @@ public interface Pageable { return isUnpaged() ? Optional.empty() : Optional.of(this); } + /** + * Returns an {@link Limit} from this pageable if the page request {@link #isPaged() is paged} or + * {@link Limit#unlimited()} otherwise. + * + * @return + * @since 3.2 + */ + default Limit toLimit() { + + if (isUnpaged()) { + return Limit.unlimited(); + } + + return Limit.of(getPageSize()); + } + /** * Returns an {@link OffsetScrollPosition} from this pageable if the page request {@link #isPaged() is paged}. * diff --git a/src/main/java/org/springframework/data/repository/query/ParameterAccessor.java b/src/main/java/org/springframework/data/repository/query/ParameterAccessor.java index c009dda68..4dbdb6d2a 100644 --- a/src/main/java/org/springframework/data/repository/query/ParameterAccessor.java +++ b/src/main/java/org/springframework/data/repository/query/ParameterAccessor.java @@ -60,15 +60,11 @@ public interface ParameterAccessor extends Iterable { * assignable to {@link Limit} can be found {@link Limit} will be created out of {@link Pageable#getPageSize()} if * present. * - * @return {@link Limit#unlimited()} by default. + * @return * @since 3.2 */ default Limit getLimit() { - - if (getPageable().isUnpaged()) { - return Limit.unlimited(); - } - return Limit.of(getPageable().getPageSize()); + return getPageable().toLimit(); } /** diff --git a/src/main/java/org/springframework/data/repository/query/ParametersParameterAccessor.java b/src/main/java/org/springframework/data/repository/query/ParametersParameterAccessor.java index 15f05b099..85882380d 100644 --- a/src/main/java/org/springframework/data/repository/query/ParametersParameterAccessor.java +++ b/src/main/java/org/springframework/data/repository/query/ParametersParameterAccessor.java @@ -157,7 +157,7 @@ public class ParametersParameterAccessor implements ParameterAccessor { if (parameters.hasPageableParameter()) { Pageable pageable = (Pageable) values[parameters.getPageableIndex()]; - return pageable.isPaged() ? Limit.of(pageable.getPageSize()) : Limit.unlimited(); + return pageable.toLimit(); } return Limit.unlimited(); diff --git a/src/main/java/org/springframework/data/repository/query/QueryMethod.java b/src/main/java/org/springframework/data/repository/query/QueryMethod.java index 822be10a7..bc3e66cf7 100644 --- a/src/main/java/org/springframework/data/repository/query/QueryMethod.java +++ b/src/main/java/org/springframework/data/repository/query/QueryMethod.java @@ -103,6 +103,68 @@ public class QueryMethod { validate(); } + private void validate() { + + QueryMethodValidator.validate(method); + + if (hasParameterOfType(method, Pageable.class)) { + + if (!isStreamQuery()) { + assertReturnTypeAssignable(method, QueryExecutionConverters.getAllowedPageableTypes()); + } + + if (hasParameterOfType(method, Sort.class)) { + throw new IllegalStateException(String.format("Method must not have Pageable *and* Sort parameters. " + + "Use sorting capabilities on Pageable instead; Offending method: %s", method)); + } + } + + if (hasParameterOfType(method, ScrollPosition.class)) { + assertReturnTypeAssignable(method, Collections.singleton(Window.class)); + } + + Assert.notNull(this.parameters, + () -> String.format("Parameters extracted from method '%s' must not be null", method.getName())); + + if (isPageQuery()) { + Assert.isTrue(this.parameters.hasPageableParameter(), + String.format("Paging query needs to have a Pageable parameter; Offending method: %s", method)); + } + + if (isScrollQuery()) { + + Assert.isTrue(this.parameters.hasScrollPositionParameter() || this.parameters.hasPageableParameter(), + String.format("Scroll query needs to have a ScrollPosition parameter; Offending method: %s", method)); + } + } + + private boolean calculateIsCollectionQuery() { + + if (isPageQuery() || isSliceQuery() || isScrollQuery()) { + return false; + } + + TypeInformation returnTypeInformation = metadata.getReturnType(method); + + // Check against simple wrapper types first + if (metadata.getDomainTypeInformation() + .isAssignableFrom(NullableWrapperConverters.unwrapActualType(returnTypeInformation))) { + return false; + } + + Class returnType = returnTypeInformation.getType(); + + if (QueryExecutionConverters.supports(returnType) && !QueryExecutionConverters.isSingleValue(returnType)) { + return true; + } + + if (QueryExecutionConverters.supports(unwrappedReturnType)) { + return !QueryExecutionConverters.isSingleValue(unwrappedReturnType); + } + + return TypeInformation.of(unwrappedReturnType).isCollectionLike(); + } + /** * Creates a {@link Parameters} instance. * @@ -265,68 +327,6 @@ public class QueryMethod { return method.toString(); } - public void validate() { - - QueryMethodValidator.validate(method); - - if (hasParameterOfType(method, Pageable.class)) { - - if (!isStreamQuery()) { - assertReturnTypeAssignable(method, QueryExecutionConverters.getAllowedPageableTypes()); - } - - if (hasParameterOfType(method, Sort.class)) { - throw new IllegalStateException(String.format("Method must not have Pageable *and* Sort parameters. " - + "Use sorting capabilities on Pageable instead; Offending method: %s", method)); - } - } - - if (hasParameterOfType(method, ScrollPosition.class)) { - assertReturnTypeAssignable(method, Collections.singleton(Window.class)); - } - - Assert.notNull(this.parameters, - () -> String.format("Parameters extracted from method '%s' must not be null", method.getName())); - - if (isPageQuery()) { - Assert.isTrue(this.parameters.hasPageableParameter(), - String.format("Paging query needs to have a Pageable parameter; Offending method: %s", method)); - } - - if (isScrollQuery()) { - - Assert.isTrue(this.parameters.hasScrollPositionParameter() || this.parameters.hasPageableParameter(), - String.format("Scroll query needs to have a ScrollPosition parameter; Offending method: %s", method)); - } - } - - private boolean calculateIsCollectionQuery() { - - if (isPageQuery() || isSliceQuery() || isScrollQuery()) { - return false; - } - - TypeInformation returnTypeInformation = metadata.getReturnType(method); - - // Check against simple wrapper types first - if (metadata.getDomainTypeInformation() - .isAssignableFrom(NullableWrapperConverters.unwrapActualType(returnTypeInformation))) { - return false; - } - - Class returnType = returnTypeInformation.getType(); - - if (QueryExecutionConverters.supports(returnType) && !QueryExecutionConverters.isSingleValue(returnType)) { - return true; - } - - if (QueryExecutionConverters.supports(unwrappedReturnType)) { - return !QueryExecutionConverters.isSingleValue(unwrappedReturnType); - } - - return TypeInformation.of(unwrappedReturnType).isCollectionLike(); - } - private static Class potentiallyUnwrapReturnTypeFor(RepositoryMetadata metadata, Method method) { TypeInformation returnType = metadata.getReturnType(method); diff --git a/src/test/java/org/springframework/data/domain/LimitUnitTests.java b/src/test/java/org/springframework/data/domain/LimitUnitTests.java index 5d4164cfa..0678744b3 100644 --- a/src/test/java/org/springframework/data/domain/LimitUnitTests.java +++ b/src/test/java/org/springframework/data/domain/LimitUnitTests.java @@ -22,6 +22,8 @@ import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.ValueSource; /** + * Unit tests for {@link Limit}. + * * @author Christoph Strobl * @soundtrack Rise Against - Tragedy + Time */