From e77e21cb7926f4689b9903bb65ae81bc80a56e7a Mon Sep 17 00:00:00 2001 From: kjg Date: Sat, 8 Nov 2025 13:13:33 +0900 Subject: [PATCH] Fix default skip policy when only retry is configured When retry is configured without explicit skip settings, the default SkipPolicy should be NeverSkipItemSkipPolicy instead of AlwaysSkipItemSkipPolicy to prevent silent data loss when items fail after retry exhaustion. This commit removes the `skipLimit > 0` condition from the skip policy initialization, ensuring NeverSkipItemSkipPolicy is used when no skip exceptions are explicitly configured via the skip() method. Resolves gh-5077 Signed-off-by: kjg --- .../builder/ChunkOrientedStepBuilder.java | 9 +-- .../ChunkOrientedStepBuilderTests.java | 71 +++++++++++++++++++ 2 files changed, 76 insertions(+), 4 deletions(-) create mode 100644 spring-batch-core/src/test/java/org/springframework/batch/core/step/builder/ChunkOrientedStepBuilderTests.java diff --git a/spring-batch-core/src/main/java/org/springframework/batch/core/step/builder/ChunkOrientedStepBuilder.java b/spring-batch-core/src/main/java/org/springframework/batch/core/step/builder/ChunkOrientedStepBuilder.java index 5942265d11..7906772b62 100644 --- a/spring-batch-core/src/main/java/org/springframework/batch/core/step/builder/ChunkOrientedStepBuilder.java +++ b/spring-batch-core/src/main/java/org/springframework/batch/core/step/builder/ChunkOrientedStepBuilder.java @@ -48,8 +48,8 @@ import org.springframework.batch.core.step.StepInterruptionPolicy; import org.springframework.batch.core.step.ThreadStepInterruptionPolicy; import org.springframework.batch.core.step.item.ChunkOrientedStep; -import org.springframework.batch.core.step.skip.AlwaysSkipItemSkipPolicy; import org.springframework.batch.core.step.skip.LimitCheckingExceptionHierarchySkipPolicy; +import org.springframework.batch.core.step.skip.NeverSkipItemSkipPolicy; import org.springframework.batch.core.step.skip.SkipLimitExceededException; import org.springframework.batch.core.step.skip.SkipPolicy; import org.springframework.batch.infrastructure.item.ItemProcessor; @@ -320,7 +320,8 @@ public ChunkOrientedStepBuilder retryLimit(long retryLimit) { * Set the skip policy for the step. This policy determines how the step handles * skipping items in case of failures. It can be used to define the conditions under * which items should be skipped and how many times an item can be skipped before the - * step fails. Defaults to {@link AlwaysSkipItemSkipPolicy}. + * step fails. Defaults to {@link NeverSkipItemSkipPolicy} when no skip configuration + * is provided, preventing silent data loss after retry exhaustion. * @param skipPolicy the skip policy to use * @return this for fluent chaining */ @@ -420,12 +421,12 @@ public ChunkOrientedStep build() { } chunkOrientedStep.setRetryPolicy(this.retryPolicy); if (this.skipPolicy == null) { - if (!this.skippableExceptions.isEmpty() || this.skipLimit > 0) { + if (!this.skippableExceptions.isEmpty()) { this.skipPolicy = new LimitCheckingExceptionHierarchySkipPolicy(this.skippableExceptions, this.skipLimit); } else { - this.skipPolicy = new AlwaysSkipItemSkipPolicy(); + this.skipPolicy = new NeverSkipItemSkipPolicy(); } } chunkOrientedStep.setSkipPolicy(this.skipPolicy); diff --git a/spring-batch-core/src/test/java/org/springframework/batch/core/step/builder/ChunkOrientedStepBuilderTests.java b/spring-batch-core/src/test/java/org/springframework/batch/core/step/builder/ChunkOrientedStepBuilderTests.java new file mode 100644 index 0000000000..9237f5da94 --- /dev/null +++ b/spring-batch-core/src/test/java/org/springframework/batch/core/step/builder/ChunkOrientedStepBuilderTests.java @@ -0,0 +1,71 @@ +/* + * Copyright 2025-present 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.batch.core.step.builder; + +import java.lang.reflect.Field; +import java.util.List; + +import org.junit.jupiter.api.Test; + +import org.springframework.batch.core.repository.JobRepository; +import org.springframework.batch.core.step.item.ChunkOrientedStep; +import org.springframework.batch.core.step.skip.NeverSkipItemSkipPolicy; +import org.springframework.batch.core.step.skip.SkipPolicy; +import org.springframework.batch.infrastructure.item.support.ListItemReader; +import org.springframework.jdbc.support.JdbcTransactionManager; + +import static org.junit.jupiter.api.Assertions.assertInstanceOf; +import static org.mockito.Mockito.mock; + +/** + * @author Mahmoud Ben Hassine + */ +class ChunkOrientedStepBuilderTests { + + private final JobRepository jobRepository = mock(JobRepository.class); + + private final JdbcTransactionManager transactionManager = mock(JdbcTransactionManager.class); + + @Test + void testDefaultSkipPolicyWhenOnlyRetryConfigured() throws Exception { + // Given: ChunkOrientedStepBuilder with only retry configured (no skip) + ChunkOrientedStep step = new StepBuilder("testStep", jobRepository) + .chunk(10, transactionManager) + .reader(new ListItemReader<>(List.of("item1"))) + .writer(items -> { + }) + .faultTolerant() + .retry(Exception.class) + .retryLimit(3) + // No skip configuration! + .build(); + + // When: We get the SkipPolicy from the built step + SkipPolicy skipPolicy = getSkipPolicyFromStep(step); + + // Then: It should be NeverSkipItemSkipPolicy (not AlwaysSkipItemSkipPolicy) + assertInstanceOf(NeverSkipItemSkipPolicy.class, skipPolicy, + "When only retry is configured, default SkipPolicy should be NeverSkipItemSkipPolicy " + + "to prevent silent data loss after retry exhaustion"); + } + + private SkipPolicy getSkipPolicyFromStep(ChunkOrientedStep step) throws Exception { + Field skipPolicyField = ChunkOrientedStep.class.getDeclaredField("skipPolicy"); + skipPolicyField.setAccessible(true); + return (SkipPolicy) skipPolicyField.get(step); + } + +}