From 95f11facf4db28b8daa5ce288e8637ff2f2aaba5 Mon Sep 17 00:00:00 2001
From: Antoine Kapps
Date: Tue, 20 Dec 2016 00:04:29 +0100
Subject: [PATCH] Fix incorrect behaviour of RepositoryItemReader#jumpToItem()
on restart
* Corrects unit tests with expected behavior.
* Creates integration tests highlighting Hibernate lazy-loading failure if the reader has requested the page on open().
Issue #1074
---
build.gradle | 1 +
.../RepositoryItemReaderIntegrationTests.java | 116 ++++++++++++
.../batch/item/sample/books/Author.java | 70 +++++++
.../batch/item/sample/books/Book.java | 56 ++++++
.../sample/books/data/AuthorRepository.java | 25 +++
.../item/sample/books/data/SimpleService.java | 55 ++++++
...epositoryItemReaderCommonTests-context.xml | 56 ++++++
.../batch/item/database/init-books-schema.sql | 24 +++
.../batch/item/data/RepositoryItemReader.java | 22 ++-
.../item/data/RepositoryItemReaderTests.java | 175 ++++++++++++------
10 files changed, 529 insertions(+), 71 deletions(-)
create mode 100644 spring-batch-infrastructure-tests/src/test/java/org/springframework/batch/item/database/RepositoryItemReaderIntegrationTests.java
create mode 100644 spring-batch-infrastructure-tests/src/test/java/org/springframework/batch/item/sample/books/Author.java
create mode 100644 spring-batch-infrastructure-tests/src/test/java/org/springframework/batch/item/sample/books/Book.java
create mode 100644 spring-batch-infrastructure-tests/src/test/java/org/springframework/batch/item/sample/books/data/AuthorRepository.java
create mode 100644 spring-batch-infrastructure-tests/src/test/java/org/springframework/batch/item/sample/books/data/SimpleService.java
create mode 100644 spring-batch-infrastructure-tests/src/test/resources/org/springframework/batch/item/database/RepositoryItemReaderCommonTests-context.xml
create mode 100644 spring-batch-infrastructure-tests/src/test/resources/org/springframework/batch/item/database/init-books-schema.sql
diff --git a/build.gradle b/build.gradle
index 3f8c90181..4e818fbf1 100644
--- a/build.gradle
+++ b/build.gradle
@@ -478,6 +478,7 @@ project('spring-batch-infrastructure-tests') {
testCompile("org.springframework:spring-oxm:$springVersion") {
exclude group: 'commons-lang', module: 'commons-lang'
}
+ testCompile "org.springframework.data:spring-data-jpa:$springDataJpaVersion"
testCompile "org.springframework:spring-jdbc:$springVersion"
testCompile "org.springframework:spring-test:$springVersion"
testCompile "org.mockito:mockito-core:$mockitoVersion"
diff --git a/spring-batch-infrastructure-tests/src/test/java/org/springframework/batch/item/database/RepositoryItemReaderIntegrationTests.java b/spring-batch-infrastructure-tests/src/test/java/org/springframework/batch/item/database/RepositoryItemReaderIntegrationTests.java
new file mode 100644
index 000000000..71db4c2d4
--- /dev/null
+++ b/spring-batch-infrastructure-tests/src/test/java/org/springframework/batch/item/database/RepositoryItemReaderIntegrationTests.java
@@ -0,0 +1,116 @@
+/*
+ * Copyright 2020 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
+ *
+ * http://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.item.database;
+
+import static org.junit.Assert.assertEquals;
+
+import java.util.List;
+
+import org.junit.After;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+
+import org.springframework.batch.item.ExecutionContext;
+import org.springframework.batch.item.data.RepositoryItemReader;
+import org.springframework.batch.item.sample.books.Author;
+import org.springframework.batch.item.sample.books.Book;
+import org.springframework.batch.item.sample.books.data.SimpleService;
+import org.springframework.beans.factory.annotation.Autowired;
+import org.springframework.test.context.ContextConfiguration;
+import org.springframework.test.context.junit4.SpringJUnit4ClassRunner;
+
+
+@RunWith(SpringJUnit4ClassRunner.class)
+@ContextConfiguration(locations = "RepositoryItemReaderCommonTests-context.xml")
+public class RepositoryItemReaderIntegrationTests {
+
+ private static final String CONTEXT_KEY = "RepositoryItemReader.read.count";
+
+ @Autowired
+ private SimpleService service;
+
+ @Autowired
+ private RepositoryItemReader reader;
+
+ @After
+ public void reinitializeReader() {
+ reader.close();
+ }
+
+ @Test
+ public void testReadFromFirstPos() throws Exception {
+ service.openReader(new ExecutionContext());
+
+ final List books = service.nextAuthorBooks();
+
+ assertEquals("Books list size", 2, books.size());
+ assertEquals("First book", "author 1 - book 1", books.get(0).getName());
+ assertEquals("Second book", "author 1 - book 2", books.get(1).getName());
+ }
+
+ @Test
+ public void testReadFromWithinPage() throws Exception {
+ reader.setCurrentItemCount(1);
+ service.openReader(new ExecutionContext());
+
+ final List books = service.nextAuthorBooks();
+
+ assertEquals("Books list size", 2, books.size());
+ assertEquals("First book", "author 2 - book 1", books.get(0).getName());
+ assertEquals("Second book", "author 2 - book 2", books.get(1).getName());
+ }
+
+ @Test
+ public void testReadFromNewPage() throws Exception {
+ reader.setPageSize(2);
+ reader.setCurrentItemCount(2); // 3rd item = 1rst of page 2
+ service.openReader(new ExecutionContext());
+
+ final List books = service.nextAuthorBooks();
+
+ assertEquals("Books list size", 2, books.size());
+ assertEquals("First book", "author 3 - book 1", books.get(0).getName());
+ assertEquals("Second book", "author 3 - book 2", books.get(1).getName());
+ }
+
+ @Test
+ public void testReadFromWithinPage_Restart() throws Exception {
+ final ExecutionContext executionContext = new ExecutionContext();
+ executionContext.putInt(CONTEXT_KEY, 1);
+ service.openReader(executionContext);
+
+ final List books = service.nextAuthorBooks();
+
+ assertEquals("Books list size", 2, books.size());
+ assertEquals("First book", "author 2 - book 1", books.get(0).getName());
+ assertEquals("Second book", "author 2 - book 2", books.get(1).getName());
+ }
+
+ @Test
+ public void testReadFromNewPage_Restart() throws Exception {
+ reader.setPageSize(2);
+ final ExecutionContext executionContext = new ExecutionContext();
+ executionContext.putInt(CONTEXT_KEY, 2);
+ service.openReader(executionContext);
+
+ final List books = service.nextAuthorBooks();
+
+ assertEquals("Books list size", 2, books.size());
+ assertEquals("First book", "author 3 - book 1", books.get(0).getName());
+ assertEquals("Second book", "author 3 - book 2", books.get(1).getName());
+ }
+
+}
diff --git a/spring-batch-infrastructure-tests/src/test/java/org/springframework/batch/item/sample/books/Author.java b/spring-batch-infrastructure-tests/src/test/java/org/springframework/batch/item/sample/books/Author.java
new file mode 100644
index 000000000..dc48a751a
--- /dev/null
+++ b/spring-batch-infrastructure-tests/src/test/java/org/springframework/batch/item/sample/books/Author.java
@@ -0,0 +1,70 @@
+/*
+ * Copyright 2020 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
+ *
+ * http://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.item.sample.books;
+
+import javax.persistence.Basic;
+import javax.persistence.Entity;
+import javax.persistence.Id;
+import javax.persistence.JoinColumn;
+import javax.persistence.OneToMany;
+import javax.persistence.Table;
+import java.util.List;
+import java.util.Objects;
+
+/**
+ * Basic domain object with a lazy one-to-many association.
+ *
+ * @author Antoine Kapps
+ */
+@Entity
+@Table(name = "T_AUTHORS")
+public class Author {
+
+ @Id
+ private int id;
+
+ @Basic
+ private String name;
+
+ @OneToMany
+ @JoinColumn(name = "AUTHOR_ID")
+ private List books;
+
+ public int getId() {
+ return id;
+ }
+ public String getName() {
+ return name;
+ }
+ public List getBooks() {
+ return books;
+ }
+
+ @Override
+ public boolean equals(Object o) {
+ if (this == o) return true;
+ if (o == null || getClass() != o.getClass()) return false;
+ Author author = (Author) o;
+ return id == author.id &&
+ Objects.equals(name, author.name) &&
+ Objects.equals(books, author.books);
+ }
+
+ @Override
+ public int hashCode() {
+ return Objects.hash(id, name, books);
+ }
+}
diff --git a/spring-batch-infrastructure-tests/src/test/java/org/springframework/batch/item/sample/books/Book.java b/spring-batch-infrastructure-tests/src/test/java/org/springframework/batch/item/sample/books/Book.java
new file mode 100644
index 000000000..1e84db1eb
--- /dev/null
+++ b/spring-batch-infrastructure-tests/src/test/java/org/springframework/batch/item/sample/books/Book.java
@@ -0,0 +1,56 @@
+/*
+ * Copyright 2020 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
+ *
+ * http://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.item.sample.books;
+
+import javax.persistence.Entity;
+import javax.persistence.Id;
+import javax.persistence.Table;
+import java.util.Objects;
+
+/**
+ * Simple domain object implied in an association with {@link Author}.
+ *
+ * @author Antoine Kapps
+ */
+@Entity
+@Table(name = "T_BOOKS")
+public class Book {
+
+ @Id
+ private int id;
+ private String name;
+
+ public int getId() {
+ return id;
+ }
+ public String getName() {
+ return name;
+ }
+
+ @Override
+ public boolean equals(Object o) {
+ if (this == o) return true;
+ if (o == null || getClass() != o.getClass()) return false;
+ Book book = (Book) o;
+ return id == book.id &&
+ Objects.equals(name, book.name);
+ }
+
+ @Override
+ public int hashCode() {
+ return Objects.hash(id, name);
+ }
+}
diff --git a/spring-batch-infrastructure-tests/src/test/java/org/springframework/batch/item/sample/books/data/AuthorRepository.java b/spring-batch-infrastructure-tests/src/test/java/org/springframework/batch/item/sample/books/data/AuthorRepository.java
new file mode 100644
index 000000000..472cc28e9
--- /dev/null
+++ b/spring-batch-infrastructure-tests/src/test/java/org/springframework/batch/item/sample/books/data/AuthorRepository.java
@@ -0,0 +1,25 @@
+/*
+ * Copyright 2020 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
+ *
+ * http://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.item.sample.books.data;
+
+import org.springframework.batch.item.sample.books.Author;
+import org.springframework.data.repository.PagingAndSortingRepository;
+import org.springframework.stereotype.Repository;
+
+
+@Repository
+public interface AuthorRepository extends PagingAndSortingRepository {
+}
diff --git a/spring-batch-infrastructure-tests/src/test/java/org/springframework/batch/item/sample/books/data/SimpleService.java b/spring-batch-infrastructure-tests/src/test/java/org/springframework/batch/item/sample/books/data/SimpleService.java
new file mode 100644
index 000000000..fdd5848a5
--- /dev/null
+++ b/spring-batch-infrastructure-tests/src/test/java/org/springframework/batch/item/sample/books/data/SimpleService.java
@@ -0,0 +1,55 @@
+/*
+ * Copyright 2020 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
+ *
+ * http://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.item.sample.books.data;
+
+import javax.transaction.Transactional;
+import java.util.ArrayList;
+import java.util.List;
+
+import org.springframework.batch.item.ExecutionContext;
+import org.springframework.batch.item.data.RepositoryItemReader;
+import org.springframework.batch.item.sample.books.Author;
+import org.springframework.batch.item.sample.books.Book;
+
+/**
+ * A simple service based upon a {@link RepositoryItemReader}
+ */
+public class SimpleService {
+
+ private final RepositoryItemReader itemReader;
+
+ public SimpleService(RepositoryItemReader itemReader) {
+ this.itemReader = itemReader;
+ }
+
+ // Prepare the reader
+ public void openReader(ExecutionContext executionContext) throws Exception {
+ itemReader.open(executionContext);
+ }
+
+ // Reads next Author and returns his (lazy-loaded) books, inside a transaction (simulates the chunk's transaction)
+ @Transactional
+ public List nextAuthorBooks() throws Exception {
+ List result = new ArrayList<>();
+
+ final Author nextAuthor = itemReader.read();
+ if (nextAuthor != null) {
+ result.addAll(nextAuthor.getBooks());
+ }
+
+ return result;
+ }
+}
diff --git a/spring-batch-infrastructure-tests/src/test/resources/org/springframework/batch/item/database/RepositoryItemReaderCommonTests-context.xml b/spring-batch-infrastructure-tests/src/test/resources/org/springframework/batch/item/database/RepositoryItemReaderCommonTests-context.xml
new file mode 100644
index 000000000..3c4afdb79
--- /dev/null
+++ b/spring-batch-infrastructure-tests/src/test/resources/org/springframework/batch/item/database/RepositoryItemReaderCommonTests-context.xml
@@ -0,0 +1,56 @@
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+ classpath:org/springframework/batch/item/database/init-books-schema.sql
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
\ No newline at end of file
diff --git a/spring-batch-infrastructure-tests/src/test/resources/org/springframework/batch/item/database/init-books-schema.sql b/spring-batch-infrastructure-tests/src/test/resources/org/springframework/batch/item/database/init-books-schema.sql
new file mode 100644
index 000000000..2dc44ce4a
--- /dev/null
+++ b/spring-batch-infrastructure-tests/src/test/resources/org/springframework/batch/item/database/init-books-schema.sql
@@ -0,0 +1,24 @@
+DROP TABLE T_BOOKS if exists;
+DROP TABLE T_AUTHORS if exists;
+
+CREATE TABLE T_AUTHORS (
+ ID BIGINT NOT NULL PRIMARY KEY,
+ NAME VARCHAR(45)
+);
+
+CREATE TABLE T_BOOKS(
+ ID BIGINT NOT NULL PRIMARY KEY,
+ NAME VARCHAR(45),
+ AUTHOR_ID BIGINT NOT NULL
+);
+
+INSERT INTO T_AUTHORS (id, name) VALUES (1, 'author 1');
+INSERT INTO T_AUTHORS (id, name) VALUES (2, 'author 2');
+INSERT INTO T_AUTHORS (id, name) VALUES (3, 'author 3');
+
+INSERT INTO T_BOOKS (id, name, author_id) VALUES (1, 'author 1 - book 1', 1);
+INSERT INTO T_BOOKS (id, name, author_id) VALUES (2, 'author 1 - book 2', 1);
+INSERT INTO T_BOOKS (id, name, author_id) VALUES (3, 'author 2 - book 1', 2);
+INSERT INTO T_BOOKS (id, name, author_id) VALUES (4, 'author 2 - book 2', 2);
+INSERT INTO T_BOOKS (id, name, author_id) VALUES (5, 'author 3 - book 1', 3);
+INSERT INTO T_BOOKS (id, name, author_id) VALUES (6, 'author 3 - book 2', 3);
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 ae3b6b2d4..d2590a71f 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
@@ -1,5 +1,5 @@
/*
- * Copyright 2012-2019 the original author or authors.
+ * Copyright 2012-2020 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.
@@ -71,6 +71,7 @@ import org.springframework.util.MethodInvoker;
*
*
* @author Michael Minella
+ * @author Antoine Kapps
* @since 2.2
*/
public class RepositoryItemReader extends AbstractItemCountingItemStreamItemReader implements InitializingBean {
@@ -91,7 +92,7 @@ public class RepositoryItemReader extends AbstractItemCountingItemStreamItemR
private volatile List results;
- private Object lock = new Object();
+ private final Object lock = new Object();
private String methodName;
@@ -156,20 +157,24 @@ public class RepositoryItemReader extends AbstractItemCountingItemStreamItemR
protected T doRead() throws Exception {
synchronized (lock) {
- if(results == null || current >= results.size()) {
+ boolean nextPageNeeded = (results != null && current >= results.size());
+
+ if (results == null || nextPageNeeded) {
if (logger.isDebugEnabled()) {
logger.debug("Reading page " + page);
}
results = doPageRead();
-
- current = 0;
page ++;
if(results.size() <= 0) {
return null;
}
+
+ if (nextPageNeeded) {
+ current = 0;
+ }
}
if(current < results.size()) {
@@ -186,11 +191,8 @@ public class RepositoryItemReader extends AbstractItemCountingItemStreamItemR
@Override
protected void jumpToItem(int itemLastIndex) throws Exception {
synchronized (lock) {
- page = (itemLastIndex - 1) / pageSize;
- current = (itemLastIndex - 1) % pageSize;
-
- results = doPageRead();
- page++;
+ page = itemLastIndex / pageSize;
+ current = itemLastIndex % pageSize;
}
}
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 5f0ba2ee4..22e42f1b2 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
@@ -1,5 +1,5 @@
/*
- * Copyright 2013-2014 the original author or authors.
+ * Copyright 2013-2020 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,7 +16,9 @@
package org.springframework.batch.item.data;
import java.util.ArrayList;
-import java.util.HashMap;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.List;
import java.util.Map;
import org.junit.Before;
@@ -35,12 +37,17 @@ import org.springframework.data.domain.Sort;
import org.springframework.data.domain.Sort.Direction;
import org.springframework.data.repository.PagingAndSortingRepository;
+import static java.util.Collections.singletonList;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertNull;
+import static org.junit.Assert.assertSame;
import static org.junit.Assert.assertTrue;
import static org.junit.Assert.fail;
+import static org.mockito.Matchers.any;
import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.never;
+import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;
@SuppressWarnings("serial")
@@ -54,8 +61,7 @@ public class RepositoryItemReaderTests {
@Before
public void setUp() throws Exception {
MockitoAnnotations.initMocks(this);
- sorts = new HashMap<>();
- sorts.put("id", Direction.ASC);
+ sorts = Collections.singletonMap("id", Direction.ASC);
reader = new RepositoryItemReader<>();
reader.setRepository(repository);
reader.setPageSize(1);
@@ -66,9 +72,10 @@ public class RepositoryItemReaderTests {
@Test
public void testAfterPropertiesSet() throws Exception {
try {
- new RepositoryItemReader<>().afterPropertiesSet();
+ new RepositoryItemReader<>().afterPropertiesSet();
fail();
} catch (IllegalStateException e) {
+ // expected
}
try {
@@ -77,6 +84,7 @@ public class RepositoryItemReaderTests {
reader.afterPropertiesSet();
fail();
} catch (IllegalStateException iae) {
+ // expected
}
try {
@@ -86,6 +94,7 @@ public class RepositoryItemReaderTests {
reader.afterPropertiesSet();
fail();
} catch (IllegalStateException iae) {
+ // expected
}
try {
@@ -95,6 +104,7 @@ public class RepositoryItemReaderTests {
reader.afterPropertiesSet();
fail();
} catch (IllegalStateException iae) {
+ // expected
}
reader = new RepositoryItemReader<>();
@@ -120,14 +130,13 @@ public class RepositoryItemReaderTests {
}
@Test
- @SuppressWarnings("serial")
public void testDoReadFirstReadResults() throws Exception {
ArgumentCaptor pageRequestContainer = ArgumentCaptor.forClass(PageRequest.class);
final Object result = new Object();
- when(repository.findAll(pageRequestContainer.capture())).thenReturn(new PageImpl<>(new ArrayList