From d918f3956a1d1dbfbcc405c15718e34d37beb996 Mon Sep 17 00:00:00 2001 From: YongHwan Date: Wed, 18 Oct 2023 16:33:59 +0900 Subject: [PATCH 1/4] Add overload to ReactiveSortingRepository accepting a Limit parameter. See #2923 --- .../reactive/ReactiveSortingRepository.java | 38 ++++- .../ReactiveSortingRepositoryTest.java | 131 ++++++++++++++++++ 2 files changed, 166 insertions(+), 3 deletions(-) create mode 100644 src/test/java/org/springframework/data/repository/reactive/ReactiveSortingRepositoryTest.java diff --git a/src/main/java/org/springframework/data/repository/reactive/ReactiveSortingRepository.java b/src/main/java/org/springframework/data/repository/reactive/ReactiveSortingRepository.java index 6ed255cf75..f720ea6c11 100644 --- a/src/main/java/org/springframework/data/repository/reactive/ReactiveSortingRepository.java +++ b/src/main/java/org/springframework/data/repository/reactive/ReactiveSortingRepository.java @@ -18,18 +18,19 @@ import reactor.core.publisher.Flux; import reactor.core.publisher.Mono; +import org.springframework.data.domain.Limit; import org.springframework.data.domain.Sort; import org.springframework.data.repository.NoRepositoryBean; import org.springframework.data.repository.Repository; /** - * Repository fragment to provide methods to retrieve entities using the sorting abstraction. In many cases it - * should be combined with {@link ReactiveCrudRepository} or a similar repository interface in order to add CRUD - * functionality. + * Repository fragment to provide methods to retrieve entities using the sorting abstraction. In many cases it should be + * combined with {@link ReactiveCrudRepository} or a similar repository interface in order to add CRUD functionality. * * @author Mark Paluch * @author Christoph Strobl * @author Jens Schauder + * @author YongHwan Kwon * @since 2.0 * @see Sort * @see Mono @@ -48,4 +49,35 @@ public interface ReactiveSortingRepository extends Repository { * @throws IllegalArgumentException in case the given {@link Sort} is {@literal null}. */ Flux findAll(Sort sort); + + /** + * Returns all entities sorted by the given options with limit. + * + * @param sort the {@link Sort} specification to sort the results by, can be {@link Sort#unsorted()}, must not be + * {@literal null}. + * @param limit the {@link Limit} to limit the results, must not be {@literal null} or negative. + * @return all entities sorted by the given options. + * @throws IllegalArgumentException in case the given {@link Sort} is {@literal null} or {@link Limit} is + * {@literal null} or negative. + */ + default Flux findAll(Sort sort, Limit limit) { + if (limit == null) { + throw new IllegalArgumentException("Limit must not be null"); + } + + if (limit.isUnlimited() || limit.max() == Integer.MAX_VALUE) { + return findAll(sort); + } + + final int maxLimit = limit.max(); + if (maxLimit < 0) { + throw new IllegalArgumentException("Limit value cannot be negative"); + } + + if (maxLimit == 0) { + throw new IllegalArgumentException("Limit value cannot be zero"); + } + + return findAll(sort).take(maxLimit); + } } diff --git a/src/test/java/org/springframework/data/repository/reactive/ReactiveSortingRepositoryTest.java b/src/test/java/org/springframework/data/repository/reactive/ReactiveSortingRepositoryTest.java new file mode 100644 index 0000000000..2b267cc5bc --- /dev/null +++ b/src/test/java/org/springframework/data/repository/reactive/ReactiveSortingRepositoryTest.java @@ -0,0 +1,131 @@ +/* + * Copyright 2014-2023 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. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.springframework.data.repository.reactive; + +import static org.mockito.Mockito.*; + +import reactor.core.publisher.Flux; +import reactor.test.StepVerifier; + +import java.util.List; + +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Nested; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.Spy; +import org.mockito.junit.jupiter.MockitoExtension; +import org.springframework.data.domain.Limit; +import org.springframework.data.domain.Sort; + +/** + * Unit tests for {@link ReactiveSortingRepository}. + * + * @author YongHwan Kwon + */ +@ExtendWith(MockitoExtension.class) +class ReactiveSortingRepositoryTest { + + private static final Limit LIMIT_TWO = Limit.of(2); + private static final Limit LIMIT_NEGATIVE_ONE = Limit.of(-1); + private static final Limit LIMIT_ZERO = Limit.of(0); + private final Sort defaultSort = Sort.unsorted(); + @Spy ReactiveSortingRepository repository; + + record DummyEntity(Integer id) { + + } + + @DisplayName("Entity fetching tests") + @Nested + class EntityFetchingTests { + + @DisplayName("Return only the limited number of entities when a limit is given") + @Test + void shouldReturnLimitedEntitiesWhenLimitIsGiven() { + when(repository.findAll(defaultSort)).thenReturn(Flux.fromIterable(List.of(new DummyEntity(1), new DummyEntity(2), new DummyEntity(3)))); + + final Flux results = repository.findAll(defaultSort, LIMIT_TWO); + + StepVerifier.create(results).expectNextMatches(entity -> 1 == entity.id()) + .expectNextMatches(entity -> 2 == entity.id()).expectComplete().verify(); + + verify(repository).findAll(defaultSort); + } + + @DisplayName("Return all entities when no limit is set") + @Test + void shouldReturnAllEntitiesWhenNoLimitIsSet() { + when(repository.findAll(defaultSort)).thenReturn(Flux.fromIterable(List.of(new DummyEntity(1), new DummyEntity(2), new DummyEntity(3)))); + + final Flux results = repository.findAll(defaultSort, Limit.unlimited()); + + StepVerifier.create(results).expectNextMatches(entity -> 1 == entity.id()) + .expectNextMatches(entity -> 2 == entity.id()).expectNextMatches(entity -> 3 == entity.id()).expectComplete() + .verify(); + + verify(repository).findAll(defaultSort); + } + + @DisplayName("Exception tests for invalid limits") + @Nested + class ExceptionTests { + + @DisplayName("Throw IllegalArgumentException when null limit is given") + @Test + void shouldThrowExceptionWhenNullLimitIsGiven() { + Flux resultFlux; + try { + resultFlux = repository.findAll(defaultSort, null); + } catch (Exception e) { + resultFlux = Flux.error(e); + } + + StepVerifier.create(resultFlux).expectErrorMatches(throwable -> throwable instanceof IllegalArgumentException + && "Limit must not be null".equals(throwable.getMessage())).verify(); + } + + @DisplayName("Throw IllegalArgumentException when a negative limit is given") + @Test + void shouldThrowExceptionWhenNegativeLimitIsGiven() { + Flux resultFlux; + try { + resultFlux = repository.findAll(defaultSort, LIMIT_NEGATIVE_ONE); + } catch (Exception e) { + System.out.println(e.getMessage()); + resultFlux = Flux.error(e); + } + + StepVerifier.create(resultFlux).expectErrorMatches(throwable -> throwable instanceof IllegalArgumentException + && "Limit value cannot be negative".equals(throwable.getMessage())).verify(); + } + + @DisplayName("Throw IllegalArgumentException when a zero limit is given") + @Test + void shouldThrowExceptionWhenZeroLimitIsGiven() { + Flux resultFlux; + try { + resultFlux = repository.findAll(defaultSort, LIMIT_ZERO); + } catch (Exception e) { + resultFlux = Flux.error(e); + } + + StepVerifier.create(resultFlux).expectErrorMatches(throwable -> throwable instanceof IllegalArgumentException + && "Limit value cannot be zero".equals(throwable.getMessage())).verify(); + } + } + } +} From 312363a4bab85eaf6687d5c65503930ecccfd943 Mon Sep 17 00:00:00 2001 From: YongHwan Date: Wed, 18 Oct 2023 21:18:44 +0900 Subject: [PATCH 2/4] Refactored code for enhanced readability. Related #2923 --- .../reactive/ReactiveSortingRepository.java | 6 +- .../ReactiveSortingRepositoryTest.java | 123 +++++++++--------- 2 files changed, 66 insertions(+), 63 deletions(-) diff --git a/src/main/java/org/springframework/data/repository/reactive/ReactiveSortingRepository.java b/src/main/java/org/springframework/data/repository/reactive/ReactiveSortingRepository.java index f720ea6c11..78145dd3e8 100644 --- a/src/main/java/org/springframework/data/repository/reactive/ReactiveSortingRepository.java +++ b/src/main/java/org/springframework/data/repository/reactive/ReactiveSortingRepository.java @@ -65,7 +65,7 @@ default Flux findAll(Sort sort, Limit limit) { throw new IllegalArgumentException("Limit must not be null"); } - if (limit.isUnlimited() || limit.max() == Integer.MAX_VALUE) { + if (limit.isUnlimited()) { return findAll(sort); } @@ -78,6 +78,10 @@ default Flux findAll(Sort sort, Limit limit) { throw new IllegalArgumentException("Limit value cannot be zero"); } + if (maxLimit == Integer.MAX_VALUE) { + return findAll(sort); + } + return findAll(sort).take(maxLimit); } } diff --git a/src/test/java/org/springframework/data/repository/reactive/ReactiveSortingRepositoryTest.java b/src/test/java/org/springframework/data/repository/reactive/ReactiveSortingRepositoryTest.java index 2b267cc5bc..af980dcfb1 100644 --- a/src/test/java/org/springframework/data/repository/reactive/ReactiveSortingRepositoryTest.java +++ b/src/test/java/org/springframework/data/repository/reactive/ReactiveSortingRepositoryTest.java @@ -15,17 +15,20 @@ */ package org.springframework.data.repository.reactive; +import static org.junit.jupiter.api.Assertions.*; import static org.mockito.Mockito.*; import reactor.core.publisher.Flux; import reactor.test.StepVerifier; -import java.util.List; +import java.util.stream.IntStream; import org.junit.jupiter.api.DisplayName; import org.junit.jupiter.api.Nested; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; import org.mockito.Spy; import org.mockito.junit.jupiter.MockitoExtension; import org.springframework.data.domain.Limit; @@ -39,92 +42,88 @@ @ExtendWith(MockitoExtension.class) class ReactiveSortingRepositoryTest { - private static final Limit LIMIT_TWO = Limit.of(2); - private static final Limit LIMIT_NEGATIVE_ONE = Limit.of(-1); - private static final Limit LIMIT_ZERO = Limit.of(0); - private final Sort defaultSort = Sort.unsorted(); - @Spy ReactiveSortingRepository repository; - - record DummyEntity(Integer id) { - - } + @Spy ReactiveSortingRepository repository; @DisplayName("Entity fetching tests") @Nested class EntityFetchingTests { - @DisplayName("Return only the limited number of entities when a limit is given") + @DisplayName("Return a single entity for a limit of one") @Test - void shouldReturnLimitedEntitiesWhenLimitIsGiven() { - when(repository.findAll(defaultSort)).thenReturn(Flux.fromIterable(List.of(new DummyEntity(1), new DummyEntity(2), new DummyEntity(3)))); + void shouldReturnOnlyOneEntityForLimitOfOne() { + when(repository.findAll(Sort.unsorted())).thenReturn(Flux.range(1, 20)); - final Flux results = repository.findAll(defaultSort, LIMIT_TWO); + final Flux results = repository.findAll(Sort.unsorted(), Limit.of(1)); - StepVerifier.create(results).expectNextMatches(entity -> 1 == entity.id()) - .expectNextMatches(entity -> 2 == entity.id()).expectComplete().verify(); + StepVerifier.create(results) + .expectNext(1) + .expectComplete() + .verify(); + - verify(repository).findAll(defaultSort); + verify(repository).findAll(Sort.unsorted()); } - @DisplayName("Return all entities when no limit is set") + @DisplayName("Return ten entities for a limit of ten") @Test - void shouldReturnAllEntitiesWhenNoLimitIsSet() { - when(repository.findAll(defaultSort)).thenReturn(Flux.fromIterable(List.of(new DummyEntity(1), new DummyEntity(2), new DummyEntity(3)))); + void shouldReturnTenEntitiesForLimitOfTen() { + when(repository.findAll(Sort.unsorted())).thenReturn(Flux.range(1, 20)); - final Flux results = repository.findAll(defaultSort, Limit.unlimited()); + final Flux results = repository.findAll(Sort.unsorted(), Limit.of(10)); - StepVerifier.create(results).expectNextMatches(entity -> 1 == entity.id()) - .expectNextMatches(entity -> 2 == entity.id()).expectNextMatches(entity -> 3 == entity.id()).expectComplete() + StepVerifier.create(results) + .expectNext(1, 2, 3, 4, 5, 6, 7, 8, 9, 10) + .expectComplete() .verify(); + + verify(repository).findAll(Sort.unsorted()); + } - verify(repository).findAll(defaultSort); + @DisplayName("Return all entities with unlimited") + @Test + void shouldReturnAllEntitiesForUnlimited() { + when(repository.findAll(Sort.unsorted())).thenReturn(Flux.range(1, 20)); + + final Flux results = repository.findAll(Sort.unsorted(), Limit.unlimited()); + + StepVerifier.create(results) + .expectNextSequence(IntStream.rangeClosed(1, 20).boxed().toList()) + .expectComplete() + .verify(); + + verify(repository).findAll(Sort.unsorted()); + } + + @DisplayName("Return all entities with maximum integer limit") + @Test + void shouldReturnAllEntitiesForLimitOfMaxValue() { + when(repository.findAll(Sort.unsorted())).thenReturn(Flux.range(1, 20)); + + final Flux results = repository.findAll(Sort.unsorted(), Limit.unlimited()); + + StepVerifier.create(results) + .expectNextSequence(IntStream.rangeClosed(1, 20).boxed().toList()) + .expectComplete() + .verify(); + + verify(repository).findAll(Sort.unsorted()); } @DisplayName("Exception tests for invalid limits") @Nested class ExceptionTests { - @DisplayName("Throw IllegalArgumentException when null limit is given") + @DisplayName("Throw IllegalArgumentException for null limit") @Test - void shouldThrowExceptionWhenNullLimitIsGiven() { - Flux resultFlux; - try { - resultFlux = repository.findAll(defaultSort, null); - } catch (Exception e) { - resultFlux = Flux.error(e); - } - - StepVerifier.create(resultFlux).expectErrorMatches(throwable -> throwable instanceof IllegalArgumentException - && "Limit must not be null".equals(throwable.getMessage())).verify(); + void shouldThrowIllegalArgumentExceptionWhenNullLimitIsGiven() { + assertThrows(IllegalArgumentException.class, () -> repository.findAll(Sort.unsorted(), null)); } - @DisplayName("Throw IllegalArgumentException when a negative limit is given") - @Test - void shouldThrowExceptionWhenNegativeLimitIsGiven() { - Flux resultFlux; - try { - resultFlux = repository.findAll(defaultSort, LIMIT_NEGATIVE_ONE); - } catch (Exception e) { - System.out.println(e.getMessage()); - resultFlux = Flux.error(e); - } - - StepVerifier.create(resultFlux).expectErrorMatches(throwable -> throwable instanceof IllegalArgumentException - && "Limit value cannot be negative".equals(throwable.getMessage())).verify(); - } - - @DisplayName("Throw IllegalArgumentException when a zero limit is given") - @Test - void shouldThrowExceptionWhenZeroLimitIsGiven() { - Flux resultFlux; - try { - resultFlux = repository.findAll(defaultSort, LIMIT_ZERO); - } catch (Exception e) { - resultFlux = Flux.error(e); - } - - StepVerifier.create(resultFlux).expectErrorMatches(throwable -> throwable instanceof IllegalArgumentException - && "Limit value cannot be zero".equals(throwable.getMessage())).verify(); + @DisplayName("Throw IllegalArgumentException for invalid limit values") + @ParameterizedTest + @ValueSource(ints = { -1, Integer.MIN_VALUE, 0 }) + void shouldThrowIllegalArgumentExceptionForInvalidLimits(int limitValue) { + assertThrows(IllegalArgumentException.class, () -> repository.findAll(Sort.unsorted(), Limit.of(limitValue))); } } } From 986f604d5a9007995ff3933b160f05fb9261d1cd Mon Sep 17 00:00:00 2001 From: YongHwan Date: Thu, 19 Oct 2023 10:10:16 +0900 Subject: [PATCH 3/4] Fix incorrect argument in test code. replace `Limit.unlimited()` with `Limit.of(Integer.MAX_VALUE)` Related #2923 --- .../data/repository/reactive/ReactiveSortingRepositoryTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/test/java/org/springframework/data/repository/reactive/ReactiveSortingRepositoryTest.java b/src/test/java/org/springframework/data/repository/reactive/ReactiveSortingRepositoryTest.java index af980dcfb1..e82f9fd580 100644 --- a/src/test/java/org/springframework/data/repository/reactive/ReactiveSortingRepositoryTest.java +++ b/src/test/java/org/springframework/data/repository/reactive/ReactiveSortingRepositoryTest.java @@ -99,7 +99,7 @@ void shouldReturnAllEntitiesForUnlimited() { void shouldReturnAllEntitiesForLimitOfMaxValue() { when(repository.findAll(Sort.unsorted())).thenReturn(Flux.range(1, 20)); - final Flux results = repository.findAll(Sort.unsorted(), Limit.unlimited()); + final Flux results = repository.findAll(Sort.unsorted(), Limit.of(Integer.MAX_VALUE)); StepVerifier.create(results) .expectNextSequence(IntStream.rangeClosed(1, 20).boxed().toList()) From 227532d9e426673025b0a04b7e34cfd549dec243 Mon Sep 17 00:00:00 2001 From: YongHwan Date: Thu, 19 Oct 2023 16:38:19 +0900 Subject: [PATCH 4/4] Remove whitespace. Related #2923 --- .../data/repository/reactive/ReactiveSortingRepositoryTest.java | 1 - 1 file changed, 1 deletion(-) diff --git a/src/test/java/org/springframework/data/repository/reactive/ReactiveSortingRepositoryTest.java b/src/test/java/org/springframework/data/repository/reactive/ReactiveSortingRepositoryTest.java index e82f9fd580..cce7b87653 100644 --- a/src/test/java/org/springframework/data/repository/reactive/ReactiveSortingRepositoryTest.java +++ b/src/test/java/org/springframework/data/repository/reactive/ReactiveSortingRepositoryTest.java @@ -59,7 +59,6 @@ void shouldReturnOnlyOneEntityForLimitOfOne() { .expectNext(1) .expectComplete() .verify(); - verify(repository).findAll(Sort.unsorted()); }