From 61a1899fc70a568e79b32c07a0e016cb8ebb1f10 Mon Sep 17 00:00:00 2001 From: Teemu Suo-Anttila Date: Wed, 19 Feb 2014 12:13:52 +0200 Subject: [PATCH] Fix SQLContainer paging and caching issue (#11199) Change-Id: I884c0f0a27a124a49698b141ac63c93950df428d --- .../data/util/sqlcontainer/SQLContainer.java | 180 ++++++++++-------- .../table/TableScrollingWithSQLContainer.java | 74 +++++++ .../TableScrollingWithSQLContainerTest.java | 35 ++++ 3 files changed, 210 insertions(+), 79 deletions(-) create mode 100644 uitest/src/com/vaadin/tests/components/table/TableScrollingWithSQLContainer.java create mode 100644 uitest/src/com/vaadin/tests/components/table/TableScrollingWithSQLContainerTest.java diff --git a/server/src/com/vaadin/data/util/sqlcontainer/SQLContainer.java b/server/src/com/vaadin/data/util/sqlcontainer/SQLContainer.java index e9a1a2d98f..32b46df166 100644 --- a/server/src/com/vaadin/data/util/sqlcontainer/SQLContainer.java +++ b/server/src/com/vaadin/data/util/sqlcontainer/SQLContainer.java @@ -50,7 +50,7 @@ public class SQLContainer implements Container, Container.Filterable, Container.Indexed, Container.Sortable, Container.ItemSetChangeNotifier { /** Query delegate */ - private QueryDelegate delegate; + private QueryDelegate queryDelegate; /** Auto commit mode, default = false */ private boolean autoCommit = false; @@ -61,6 +61,9 @@ public class SQLContainer implements Container, Container.Filterable, /** Number of items to cache = CACHE_RATIO x pageLength */ public static final int CACHE_RATIO = 2; + /** Amount of cache to overlap with previous page */ + private int cacheOverlap = pageLength; + /** Item and index caches */ private final Map itemIndexes = new HashMap(); private final CacheMap cachedItems = new CacheMap(); @@ -127,9 +130,9 @@ public class SQLContainer implements Container, Container.Filterable, throw new IllegalArgumentException( "QueryDelegate must not be null."); } - this.delegate = delegate; + queryDelegate = delegate; getPropertyIds(); - cachedItems.setCacheLimit(CACHE_RATIO * getPageLength()); + cachedItems.setCacheLimit(CACHE_RATIO * getPageLength() + cacheOverlap); } /**************************************/ @@ -147,7 +150,8 @@ public class SQLContainer implements Container, Container.Filterable, @Override public Object addItem() throws UnsupportedOperationException { - Object emptyKey[] = new Object[delegate.getPrimaryKeyColumns().size()]; + Object emptyKey[] = new Object[queryDelegate.getPrimaryKeyColumns() + .size()]; RowId itemId = new TemporaryRowId(emptyKey); // Create new empty column properties for the row item. List itemProperties = new ArrayList(); @@ -167,13 +171,13 @@ public class SQLContainer implements Container, Container.Filterable, if (autoCommit) { /* Add and commit instantly */ try { - if (delegate instanceof TableQuery) { - itemId = ((TableQuery) delegate) + if (queryDelegate instanceof TableQuery) { + itemId = ((TableQuery) queryDelegate) .storeRowImmediately(newRowItem); } else { - delegate.beginTransaction(); - delegate.storeRow(newRowItem); - delegate.commit(); + queryDelegate.beginTransaction(); + queryDelegate.storeRow(newRowItem); + queryDelegate.commit(); } refresh(); if (notificationsEnabled) { @@ -185,7 +189,7 @@ public class SQLContainer implements Container, Container.Filterable, getLogger().log(Level.WARNING, "Failed to add row to DB. Rolling back.", e); try { - delegate.rollback(); + queryDelegate.rollback(); } catch (SQLException ee) { getLogger().log(Level.SEVERE, "Failed to roll back row addition", e); @@ -231,7 +235,8 @@ public class SQLContainer implements Container, Container.Filterable, if (itemId instanceof RowId && !(itemId instanceof TemporaryRowId)) { try { - return delegate.containsRowWithKey(((RowId) itemId).getId()); + return queryDelegate.containsRowWithKey(((RowId) itemId) + .getId()); } catch (Exception e) { /* Query failed, just return false. */ getLogger().log(Level.WARNING, "containsId query failed", e); @@ -328,9 +333,9 @@ public class SQLContainer implements Container, Container.Filterable, ResultSet rs = null; try { // Load ALL rows :( - delegate.beginTransaction(); - rs = delegate.getResults(0, 0); - List pKeys = delegate.getPrimaryKeyColumns(); + queryDelegate.beginTransaction(); + rs = queryDelegate.getResults(0, 0); + List pKeys = queryDelegate.getPrimaryKeyColumns(); while (rs.next()) { RowId id = null; if (pKeys.isEmpty()) { @@ -350,12 +355,12 @@ public class SQLContainer implements Container, Container.Filterable, } rs.getStatement().close(); rs.close(); - delegate.commit(); + queryDelegate.commit(); } catch (SQLException e) { getLogger().log(Level.WARNING, "getItemIds() failed, rolling back.", e); try { - delegate.rollback(); + queryDelegate.rollback(); } catch (SQLException e1) { getLogger().log(Level.SEVERE, "Failed to roll back state", e1); } @@ -426,9 +431,9 @@ public class SQLContainer implements Container, Container.Filterable, return false; } try { - delegate.beginTransaction(); - boolean success = delegate.removeRow((RowItem) i); - delegate.commit(); + queryDelegate.beginTransaction(); + boolean success = queryDelegate.removeRow((RowItem) i); + queryDelegate.commit(); refresh(); if (notificationsEnabled) { CacheFlushNotifier.notifyOfCacheFlush(this); @@ -441,7 +446,7 @@ public class SQLContainer implements Container, Container.Filterable, getLogger().log(Level.WARNING, "Failed to remove row, rolling back", e); try { - delegate.rollback(); + queryDelegate.rollback(); } catch (SQLException ee) { /* Nothing can be done here */ getLogger().log(Level.SEVERE, @@ -452,7 +457,7 @@ public class SQLContainer implements Container, Container.Filterable, getLogger().log(Level.WARNING, "Failed to remove row, rolling back", e); try { - delegate.rollback(); + queryDelegate.rollback(); } catch (SQLException ee) { /* Nothing can be done here */ getLogger().log(Level.SEVERE, @@ -479,29 +484,29 @@ public class SQLContainer implements Container, Container.Filterable, if (autoCommit) { /* Remove and commit instantly. */ try { - delegate.beginTransaction(); + queryDelegate.beginTransaction(); boolean success = true; for (Object id : getItemIds()) { - if (!delegate.removeRow((RowItem) getItem(id))) { + if (!queryDelegate.removeRow((RowItem) getItem(id))) { success = false; } } if (success) { - delegate.commit(); + queryDelegate.commit(); getLogger().log(Level.FINER, "All rows removed from DB..."); refresh(); if (notificationsEnabled) { CacheFlushNotifier.notifyOfCacheFlush(this); } } else { - delegate.rollback(); + queryDelegate.rollback(); } return success; } catch (SQLException e) { getLogger().log(Level.WARNING, "removeAllItems() failed, rolling back", e); try { - delegate.rollback(); + queryDelegate.rollback(); } catch (SQLException ee) { /* Nothing can be done here */ getLogger().log(Level.SEVERE, "Failed to roll back", ee); @@ -511,7 +516,7 @@ public class SQLContainer implements Container, Container.Filterable, getLogger().log(Level.WARNING, "removeAllItems() failed, rolling back", e); try { - delegate.rollback(); + queryDelegate.rollback(); } catch (SQLException ee) { /* Nothing can be done here */ getLogger().log(Level.SEVERE, "Failed to roll back", ee); @@ -650,21 +655,31 @@ public class SQLContainer implements Container, Container.Filterable, int size = size(); // this protects against infinite looping int counter = 0; + int oldIndex; while (counter < size) { - for (Integer i : itemIndexes.keySet()) { - if (itemIndexes.get(i).equals(itemId)) { - return i; + if (itemIndexes.containsValue(itemId)) { + for (Integer idx : itemIndexes.keySet()) { + if (itemIndexes.get(idx).equals(itemId)) { + return idx; + } } - counter++; } + oldIndex = currentOffset; // load in the next page. - int nextIndex = (currentOffset / (pageLength * CACHE_RATIO) + 1) - * (pageLength * CACHE_RATIO); + int nextIndex = currentOffset + pageLength * CACHE_RATIO + + cacheOverlap; if (nextIndex >= size) { // Container wrapped around, start from index 0. nextIndex = 0; } updateOffsetAndCache(nextIndex); + + // Update counter + if (currentOffset > oldIndex) { + counter += currentOffset - oldIndex; + } else { + counter += size - oldIndex; + } } // safeguard in case item not found return -1; @@ -958,7 +973,8 @@ public class SQLContainer implements Container, Container.Filterable, */ private void setPageLengthInternal(int pageLength) { this.pageLength = pageLength > 0 ? pageLength : DEFAULT_PAGE_LENGTH; - cachedItems.setCacheLimit(CACHE_RATIO * getPageLength()); + cacheOverlap = getPageLength(); + cachedItems.setCacheLimit(CACHE_RATIO * getPageLength() + cacheOverlap); } /** @@ -994,24 +1010,24 @@ public class SQLContainer implements Container, Container.Filterable, try { getLogger().log(Level.FINER, "Commiting changes through delegate..."); - delegate.beginTransaction(); + queryDelegate.beginTransaction(); /* Perform buffered deletions */ for (RowItem item : removedItems.values()) { - if (!delegate.removeRow(item)) { + if (!queryDelegate.removeRow(item)) { throw new SQLException("Removal failed for row with ID: " + item.getId()); } } /* Perform buffered modifications */ for (RowItem item : modifiedItems) { - if (delegate.storeRow(item) > 0) { + if (queryDelegate.storeRow(item) > 0) { /* * Also reset the modified state in the item in case it is * reused e.g. in a form. */ item.commit(); } else { - delegate.rollback(); + queryDelegate.rollback(); refresh(); throw new ConcurrentModificationException( "Item with the ID '" + item.getId() @@ -1020,9 +1036,9 @@ public class SQLContainer implements Container, Container.Filterable, } /* Perform buffered additions */ for (RowItem item : addedItems) { - delegate.storeRow(item); + queryDelegate.storeRow(item); } - delegate.commit(); + queryDelegate.commit(); removedItems.clear(); addedItems.clear(); modifiedItems.clear(); @@ -1031,10 +1047,10 @@ public class SQLContainer implements Container, Container.Filterable, CacheFlushNotifier.notifyOfCacheFlush(this); } } catch (SQLException e) { - delegate.rollback(); + queryDelegate.rollback(); throw e; } catch (OptimisticLockException e) { - delegate.rollback(); + queryDelegate.rollback(); throw e; } } @@ -1065,15 +1081,15 @@ public class SQLContainer implements Container, Container.Filterable, void itemChangeNotification(RowItem changedItem) { if (autoCommit) { try { - delegate.beginTransaction(); - if (delegate.storeRow(changedItem) == 0) { - delegate.rollback(); + queryDelegate.beginTransaction(); + if (queryDelegate.storeRow(changedItem) == 0) { + queryDelegate.rollback(); refresh(); throw new ConcurrentModificationException( "Item with the ID '" + changedItem.getId() + "' has been externally modified."); } - delegate.commit(); + queryDelegate.commit(); if (notificationsEnabled) { CacheFlushNotifier.notifyOfCacheFlush(this); } @@ -1082,7 +1098,7 @@ public class SQLContainer implements Container, Container.Filterable, getLogger().log(Level.WARNING, "itemChangeNotification failed, rolling back...", e); try { - delegate.rollback(); + queryDelegate.rollback(); } catch (SQLException ee) { /* Nothing can be done here */ getLogger().log(Level.SEVERE, "Rollback failed", e); @@ -1106,14 +1122,19 @@ public class SQLContainer implements Container, Container.Filterable, * Index of the item that was requested, but not found in cache */ private void updateOffsetAndCache(int index) { - if (itemIndexes.containsKey(index)) { - return; - } - currentOffset = (index / (pageLength * CACHE_RATIO)) - * (pageLength * CACHE_RATIO); + + int oldOffset = currentOffset; + + currentOffset = (index / pageLength) * pageLength - cacheOverlap; + if (currentOffset < 0) { currentOffset = 0; } + + if (oldOffset == currentOffset && !cachedItems.isEmpty()) { + return; + } + getPage(); } @@ -1128,18 +1149,18 @@ public class SQLContainer implements Container, Container.Filterable, } try { try { - delegate.setFilters(filters); + queryDelegate.setFilters(filters); } catch (UnsupportedOperationException e) { getLogger().log(Level.FINE, "The query delegate doesn't support filtering", e); } try { - delegate.setOrderBy(sorters); + queryDelegate.setOrderBy(sorters); } catch (UnsupportedOperationException e) { getLogger().log(Level.FINE, "The query delegate doesn't support sorting", e); } - int newSize = delegate.getCount(); + int newSize = queryDelegate.getCount(); sizeUpdated = new Date(); sizeDirty = false; if (newSize != size) { @@ -1163,13 +1184,13 @@ public class SQLContainer implements Container, Container.Filterable, private void getPropertyIds() throws SQLException { propertyIds.clear(); propertyTypes.clear(); - delegate.setFilters(null); - delegate.setOrderBy(null); + queryDelegate.setFilters(null); + queryDelegate.setOrderBy(null); ResultSet rs = null; ResultSetMetaData rsmd = null; try { - delegate.beginTransaction(); - rs = delegate.getResults(0, 1); + queryDelegate.beginTransaction(); + rs = queryDelegate.getResults(0, 1); rsmd = rs.getMetaData(); boolean resultExists = rs.next(); Class type = null; @@ -1208,9 +1229,9 @@ public class SQLContainer implements Container, Container.Filterable, boolean persistable = !rsmd.isReadOnly(i); - if (delegate instanceof TableQuery) { + if (queryDelegate instanceof TableQuery) { if (rsmd.getColumnLabel(i).equals( - ((TableQuery) delegate).getVersionColumn())) { + ((TableQuery) queryDelegate).getVersionColumn())) { readOnly = true; } } @@ -1219,19 +1240,20 @@ public class SQLContainer implements Container, Container.Filterable, propertyPersistable.put(colName, persistable); propertyNullable.put(colName, rsmd.isNullable(i) == ResultSetMetaData.columnNullable); - propertyPrimaryKey.put(colName, delegate.getPrimaryKeyColumns() + propertyPrimaryKey.put(colName, queryDelegate + .getPrimaryKeyColumns() .contains(rsmd.getColumnLabel(i))); propertyTypes.put(colName, type); } rs.getStatement().close(); rs.close(); - delegate.commit(); + queryDelegate.commit(); getLogger().log(Level.FINER, "Property IDs fetched."); } catch (SQLException e) { getLogger().log(Level.WARNING, "Failed to fetch property ids, rolling back", e); try { - delegate.rollback(); + queryDelegate.rollback(); } catch (SQLException e1) { getLogger().log(Level.SEVERE, "Failed to roll back", e1); } @@ -1262,23 +1284,23 @@ public class SQLContainer implements Container, Container.Filterable, itemIndexes.clear(); try { try { - delegate.setOrderBy(sorters); + queryDelegate.setOrderBy(sorters); } catch (UnsupportedOperationException e) { /* The query delegate doesn't support sorting. */ /* No need to do anything. */ getLogger().log(Level.FINE, "The query delegate doesn't support sorting", e); } - delegate.beginTransaction(); - int fetchedRows = pageLength * CACHE_RATIO; - rs = delegate.getResults(currentOffset, fetchedRows); + queryDelegate.beginTransaction(); + int fetchedRows = pageLength * CACHE_RATIO + cacheOverlap; + rs = queryDelegate.getResults(currentOffset, fetchedRows); rsmd = rs.getMetaData(); - List pKeys = delegate.getPrimaryKeyColumns(); + List pKeys = queryDelegate.getPrimaryKeyColumns(); // } /* Create new items and column properties */ ColumnProperty cp = null; int rowCount = currentOffset; - if (!delegate.implementationRespectsPagingLimits()) { + if (!queryDelegate.implementationRespectsPagingLimits()) { rowCount = currentOffset = 0; setPageLengthInternal(size); } @@ -1351,14 +1373,14 @@ public class SQLContainer implements Container, Container.Filterable, } rs.getStatement().close(); rs.close(); - delegate.commit(); + queryDelegate.commit(); getLogger().log(Level.FINER, "Fetched {0} rows starting from {1}", new Object[] { fetchedRows, currentOffset }); } catch (SQLException e) { getLogger().log(Level.WARNING, "Failed to fetch rows, rolling back", e); try { - delegate.rollback(); + queryDelegate.rollback(); } catch (SQLException e1) { getLogger().log(Level.SEVERE, "Failed to roll back", e1); } @@ -1431,8 +1453,8 @@ public class SQLContainer implements Container, Container.Filterable, */ private boolean isColumnIdentifierValid(String identifier) { if (identifier.equalsIgnoreCase("rownum") - && delegate instanceof TableQuery) { - TableQuery tq = (TableQuery) delegate; + && queryDelegate instanceof TableQuery) { + TableQuery tq = (TableQuery) queryDelegate; if (tq.getSqlGenerator() instanceof MSSQLGenerator || tq.getSqlGenerator() instanceof OracleGenerator) { return false; @@ -1447,7 +1469,7 @@ public class SQLContainer implements Container, Container.Filterable, * @return current querydelegate */ protected QueryDelegate getQueryDelegate() { - return delegate; + return queryDelegate; } /************************************/ @@ -1634,8 +1656,8 @@ public class SQLContainer implements Container, Container.Filterable, * @param listener */ public void addRowIdChangeListener(RowIdChangeListener listener) { - if (delegate instanceof QueryDelegate.RowIdChangeNotifier) { - ((QueryDelegate.RowIdChangeNotifier) delegate) + if (queryDelegate instanceof QueryDelegate.RowIdChangeNotifier) { + ((QueryDelegate.RowIdChangeNotifier) queryDelegate) .addListener(listener); } } @@ -1655,8 +1677,8 @@ public class SQLContainer implements Container, Container.Filterable, * @param listener */ public void removeRowIdChangeListener(RowIdChangeListener listener) { - if (delegate instanceof QueryDelegate.RowIdChangeNotifier) { - ((QueryDelegate.RowIdChangeNotifier) delegate) + if (queryDelegate instanceof QueryDelegate.RowIdChangeNotifier) { + ((QueryDelegate.RowIdChangeNotifier) queryDelegate) .removeListener(listener); } } diff --git a/uitest/src/com/vaadin/tests/components/table/TableScrollingWithSQLContainer.java b/uitest/src/com/vaadin/tests/components/table/TableScrollingWithSQLContainer.java new file mode 100644 index 0000000000..f6a6f24021 --- /dev/null +++ b/uitest/src/com/vaadin/tests/components/table/TableScrollingWithSQLContainer.java @@ -0,0 +1,74 @@ +package com.vaadin.tests.components.table; + +import java.sql.SQLException; + +import com.vaadin.data.util.sqlcontainer.DataGenerator; +import com.vaadin.data.util.sqlcontainer.SQLContainer; +import com.vaadin.data.util.sqlcontainer.SQLTestsConstants; +import com.vaadin.data.util.sqlcontainer.connection.SimpleJDBCConnectionPool; +import com.vaadin.data.util.sqlcontainer.query.QueryDelegate; +import com.vaadin.data.util.sqlcontainer.query.TableQuery; +import com.vaadin.server.VaadinRequest; +import com.vaadin.ui.Button; +import com.vaadin.ui.Button.ClickEvent; +import com.vaadin.ui.Button.ClickListener; +import com.vaadin.ui.Table; +import com.vaadin.ui.UI; +import com.vaadin.ui.VerticalLayout; + +@SuppressWarnings("serial") +public class TableScrollingWithSQLContainer extends UI { + + /** Table should never end up calling indexOfId in this case */ + private class LimitedSQLContainer extends SQLContainer { + + public LimitedSQLContainer(QueryDelegate delegate) throws SQLException { + super(delegate); + } + + @Override + public int indexOfId(Object itemId) { + throw new RuntimeException("This function should not be called"); + } + } + + static final String TABLE = "table"; + + @Override + public void init(VaadinRequest request) { + try { + SimpleJDBCConnectionPool connectionPool = new SimpleJDBCConnectionPool( + SQLTestsConstants.dbDriver, SQLTestsConstants.dbURL, + SQLTestsConstants.dbUser, SQLTestsConstants.dbPwd, 2, 2); + DataGenerator.addPeopleToDatabase(connectionPool); + DataGenerator.addFiveThousandPeople(connectionPool); + + TableQuery query = new TableQuery("people", connectionPool, + SQLTestsConstants.sqlGen); + + SQLContainer container = new LimitedSQLContainer(query); + + final VerticalLayout rootLayout = new VerticalLayout(); + + final Table table = new Table(); + table.setContainerDataSource(container); + table.setCurrentPageFirstItemIndex(300); + rootLayout.addComponent(table); + + table.setImmediate(true); + + rootLayout.addComponent(new Button("GOTO 200", new ClickListener() { + + @Override + public void buttonClick(ClickEvent event) { + table.setCurrentPageFirstItemIndex(200); + } + })); + + setContent(rootLayout); + + } catch (Exception e) { + e.printStackTrace(); + } + } +} \ No newline at end of file diff --git a/uitest/src/com/vaadin/tests/components/table/TableScrollingWithSQLContainerTest.java b/uitest/src/com/vaadin/tests/components/table/TableScrollingWithSQLContainerTest.java new file mode 100644 index 0000000000..97c780e0e8 --- /dev/null +++ b/uitest/src/com/vaadin/tests/components/table/TableScrollingWithSQLContainerTest.java @@ -0,0 +1,35 @@ +/* + * Copyright 2000-2013 Vaadin Ltd. + * + * 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 com.vaadin.tests.components.table; + +import org.junit.Assert; +import org.junit.Test; +import org.openqa.selenium.By; + +import com.vaadin.tests.tb3.MultiBrowserTest; + +public class TableScrollingWithSQLContainerTest extends MultiBrowserTest { + + @Test + public void verifySQLContainerIndexOfIDNotCalled() { + openTestURL(); + + vaadinElement("/VVerticalLayout[0]/VButton[0]").click(); + + Assert.assertTrue("SQLContainer indexOfId was called", driver + .findElements(By.className("v-errorindicator")).isEmpty()); + } +} -- 2.39.5