From 4ea54388e73e4cec93667a29442ec4acfee449dc Mon Sep 17 00:00:00 2001 From: Mahmoud Ben Hassine Date: Sat, 18 Feb 2023 04:46:47 +0100 Subject: [PATCH] Fix inconsistent state validation rules in RepositoryItemReader Before this commit, state validation rules in RepositoryItemReader were not consistent with those applied in its builder. This commit makes validation rules consistent between the two ways of creating a RepositoryItemReader. This commit also adds a getter for the component name in ExecutionContextUserSupport to be able to assert on it where appropriate down the hierarchy. Resolves #4276 --- .../springframework/batch/item/ItemStreamSupport.java | 10 +++++++++- .../batch/item/data/RepositoryItemReader.java | 8 +++++++- .../data/builder/RepositoryItemReaderBuilder.java | 11 +++-------- .../batch/item/util/ExecutionContextUserSupport.java | 5 +++-- .../batch/item/data/RepositoryItemReaderTests.java | 7 +++++++ .../builder/RepositoryItemReaderBuilderTests.java | 7 +++++++ 6 files changed, 36 insertions(+), 12 deletions(-) diff --git a/spring-batch-infrastructure/src/main/java/org/springframework/batch/item/ItemStreamSupport.java b/spring-batch-infrastructure/src/main/java/org/springframework/batch/item/ItemStreamSupport.java index 4313c592a..6d72ffa4a 100644 --- a/spring-batch-infrastructure/src/main/java/org/springframework/batch/item/ItemStreamSupport.java +++ b/spring-batch-infrastructure/src/main/java/org/springframework/batch/item/ItemStreamSupport.java @@ -1,5 +1,5 @@ /* - * Copyright 2006-2022 the original author or authors. + * Copyright 2006-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. @@ -72,6 +72,14 @@ public abstract class ItemStreamSupport implements ItemStream { this.setExecutionContextName(name); } + /** + * Get the name of the component + * @return the name of the component + */ + public String getName() { + return executionContextUserSupport.getName(); + } + protected void setExecutionContextName(String name) { executionContextUserSupport.setName(name); } diff --git a/spring-batch-infrastructure/src/main/java/org/springframework/batch/item/data/RepositoryItemReader.java b/spring-batch-infrastructure/src/main/java/org/springframework/batch/item/data/RepositoryItemReader.java index 21b03c4f8..cab3b1ee7 100644 --- a/spring-batch-infrastructure/src/main/java/org/springframework/batch/item/data/RepositoryItemReader.java +++ b/spring-batch-infrastructure/src/main/java/org/springframework/batch/item/data/RepositoryItemReader.java @@ -37,6 +37,7 @@ import org.springframework.lang.Nullable; import org.springframework.util.Assert; import org.springframework.util.ClassUtils; import org.springframework.util.MethodInvoker; +import org.springframework.util.StringUtils; /** *

@@ -76,6 +77,7 @@ import org.springframework.util.MethodInvoker; * * @author Michael Minella * @author Antoine Kapps + * @author Mahmoud Ben Hassine * @since 2.2 */ public class RepositoryItemReader extends AbstractItemCountingItemStreamItemReader implements InitializingBean { @@ -121,7 +123,7 @@ public class RepositoryItemReader extends AbstractItemCountingItemStreamItemR } /** - * @param pageSize The number of items to retrieve per page. + * @param pageSize The number of items to retrieve per page. Must be greater than 0. */ public void setPageSize(int pageSize) { this.pageSize = pageSize; @@ -150,6 +152,10 @@ public class RepositoryItemReader extends AbstractItemCountingItemStreamItemR Assert.state(repository != null, "A PagingAndSortingRepository is required"); Assert.state(pageSize > 0, "Page size must be greater than 0"); Assert.state(sort != null, "A sort is required"); + Assert.state(this.methodName != null && !this.methodName.isEmpty(), "methodName is required."); + if (isSaveState()) { + Assert.state(StringUtils.hasText(getName()), "A name is required when saveState is set to true."); + } } @Nullable diff --git a/spring-batch-infrastructure/src/main/java/org/springframework/batch/item/data/builder/RepositoryItemReaderBuilder.java b/spring-batch-infrastructure/src/main/java/org/springframework/batch/item/data/builder/RepositoryItemReaderBuilder.java index 50098723f..e90a21f0e 100644 --- a/spring-batch-infrastructure/src/main/java/org/springframework/batch/item/data/builder/RepositoryItemReaderBuilder.java +++ b/spring-batch-infrastructure/src/main/java/org/springframework/batch/item/data/builder/RepositoryItemReaderBuilder.java @@ -1,5 +1,5 @@ /* - * Copyright 2017-2022 the original author or authors. + * Copyright 2017-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. @@ -16,20 +16,14 @@ package org.springframework.batch.item.data.builder; -import java.lang.reflect.Method; -import java.util.ArrayList; import java.util.Arrays; import java.util.List; import java.util.Map; import org.springframework.batch.item.data.RepositoryItemReader; -import org.springframework.cglib.proxy.Enhancer; -import org.springframework.cglib.proxy.MethodInterceptor; -import org.springframework.cglib.proxy.MethodProxy; import org.springframework.data.domain.Sort; import org.springframework.data.repository.PagingAndSortingRepository; import org.springframework.util.Assert; -import org.springframework.util.CollectionUtils; import org.springframework.util.StringUtils; /** @@ -148,7 +142,7 @@ public class RepositoryItemReaderBuilder { /** * Establish the pageSize for the generated RepositoryItemReader. - * @param pageSize The number of items to retrieve per page. + * @param pageSize The number of items to retrieve per page. Must be greater than 0. * @return The current instance of the builder. * @see RepositoryItemReader#setPageSize(int) */ @@ -191,6 +185,7 @@ public class RepositoryItemReaderBuilder { public RepositoryItemReader build() { Assert.notNull(this.sorts, "sorts map is required."); Assert.notNull(this.repository, "repository is required."); + Assert.isTrue(this.pageSize > 0, "Page size must be greater than 0"); Assert.hasText(this.methodName, "methodName is required."); if (this.saveState) { Assert.state(StringUtils.hasText(this.name), "A name is required when saveState is set to true."); diff --git a/spring-batch-infrastructure/src/main/java/org/springframework/batch/item/util/ExecutionContextUserSupport.java b/spring-batch-infrastructure/src/main/java/org/springframework/batch/item/util/ExecutionContextUserSupport.java index 389de9fc9..661ab42b8 100644 --- a/spring-batch-infrastructure/src/main/java/org/springframework/batch/item/util/ExecutionContextUserSupport.java +++ b/spring-batch-infrastructure/src/main/java/org/springframework/batch/item/util/ExecutionContextUserSupport.java @@ -1,5 +1,5 @@ /* - * Copyright 2006-2007 the original author or authors. + * Copyright 2006-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. @@ -23,6 +23,7 @@ import org.springframework.util.Assert; * generating keys for {@link ExecutionContext} based on the name. * * @author Robert Kasanicky + * @author Mahmoud Ben Hassine */ public class ExecutionContextUserSupport { @@ -40,7 +41,7 @@ public class ExecutionContextUserSupport { /** * @return name used to uniquely identify this instance's entries in shared context. */ - protected String getName() { + public String getName() { return this.name; } diff --git a/spring-batch-infrastructure/src/test/java/org/springframework/batch/item/data/RepositoryItemReaderTests.java b/spring-batch-infrastructure/src/test/java/org/springframework/batch/item/data/RepositoryItemReaderTests.java index 3adefb0de..a11b513d5 100644 --- a/spring-batch-infrastructure/src/test/java/org/springframework/batch/item/data/RepositoryItemReaderTests.java +++ b/spring-batch-infrastructure/src/test/java/org/springframework/batch/item/data/RepositoryItemReaderTests.java @@ -93,6 +93,13 @@ class RepositoryItemReaderTests { reader.setRepository(repository); reader.setPageSize(1); reader.setSort(sorts); + assertThrows(IllegalStateException.class, reader::afterPropertiesSet); + + reader = new RepositoryItemReader<>(); + reader.setRepository(repository); + reader.setPageSize(1); + reader.setSort(sorts); + reader.setMethodName("findAll"); reader.afterPropertiesSet(); } diff --git a/spring-batch-infrastructure/src/test/java/org/springframework/batch/item/data/builder/RepositoryItemReaderBuilderTests.java b/spring-batch-infrastructure/src/test/java/org/springframework/batch/item/data/builder/RepositoryItemReaderBuilderTests.java index 72bb17e28..05a56d86c 100644 --- a/spring-batch-infrastructure/src/test/java/org/springframework/batch/item/data/builder/RepositoryItemReaderBuilderTests.java +++ b/spring-batch-infrastructure/src/test/java/org/springframework/batch/item/data/builder/RepositoryItemReaderBuilderTests.java @@ -141,6 +141,13 @@ class RepositoryItemReaderBuilderTests { assertEquals("repository is required.", exception.getMessage()); } + @Test + void testInvalidPageSize() { + var builder = new RepositoryItemReaderBuilder<>().repository(repository).sorts(this.sorts).pageSize(-1); + Exception exception = assertThrows(IllegalArgumentException.class, builder::build); + assertEquals("Page size must be greater than 0", exception.getMessage()); + } + @Test void testArguments() throws Exception { List args = new ArrayList<>(3);