details: https://code.openbravo.com/erp/devel/pi/rev/f422e1ebac8e changeset: 33024:f422e1ebac8e user: Asier Lostalé <asier.lostale <at> openbravo.com> date: Mon Nov 20 13:11:31 2017 +0100 summary: fixed bug 37324: slow login having many preferences - case 1
Retrieving all preferences for current role/org was slow having thousands of visible preferences because of how duplicate preferences with different visibility were checked. This check was performed by sequentially scanning a list of already saved preferences to verify whether current one was already in the list. It has now fixed by keeping a temporary map of preferences where the key defines if preference is the same or not, in this way it is not necessary to iterate over all the entries each time. details: https://code.openbravo.com/erp/devel/pi/rev/b817e5607d8b changeset: 33025:b817e5607d8b user: Asier Lostalé <asier.lostale <at> openbravo.com> date: Mon Nov 20 13:24:45 2017 +0100 summary: fixed bug 37324: slow login having many preferences - case 2 When there were thousands of preferences with ad_org visible from current org but not visible through visible at org field, loading all preferences was slow. The problem was, all those preferences were loaded as DAL objects to filter out later in memory those that were not visible. This gets fixed by filtering out directly in the query so that no unneeded rows are retrived from DB to instantiate DAL objects that later will be discarded immediatelly. details: https://code.openbravo.com/erp/devel/pi/rev/4399ee25ce29 changeset: 33026:4399ee25ce29 user: Asier Lostalé <asier.lostale <at> openbravo.com> date: Mon Nov 20 14:48:56 2017 +0100 summary: fixes 37329: implemented a more generic Utility.arrayListToString Implemented a more generic alternative to Utility.arrayListToString: * Receives as parameter a generic Collections instead of limiting it to ArrayList instances * Implemented it in a more performant manner based on OpenJDK AbstractCollection.toString details: https://code.openbravo.com/erp/devel/pi/rev/f2e2f4826906 changeset: 33027:f2e2f4826906 user: Asier Lostalé <asier.lostale <at> openbravo.com> date: Mon Nov 20 14:49:56 2017 +0100 summary: related to 37329: implemented a more generic Utility.arrayListToString Deprecated old arrayListToString method. details: https://code.openbravo.com/erp/devel/pi/rev/d7b72a4dec05 changeset: 33028:d7b72a4dec05 user: Asier Lostalé <asier.lostale <at> openbravo.com> date: Mon Nov 20 14:58:19 2017 +0100 summary: related to 37329: use new method where old was used details: https://code.openbravo.com/erp/devel/pi/rev/27c3e6dd6f54 changeset: 33029:27c3e6dd6f54 user: Asier Lostalé <asier.lostale <at> openbravo.com> date: Tue Nov 21 09:50:10 2017 +0100 summary: related to 37324: renamed PreferenceTest methods The need to be executed alphabetically because they are inter-dependent. Replaced letters with numbers to make this sorting easier. details: https://code.openbravo.com/erp/devel/pi/rev/f930ff85c698 changeset: 33030:f930ff85c698 user: Asier Lostalé <asier.lostale <at> openbravo.com> date: Tue Nov 21 09:53:27 2017 +0100 summary: related to 37324: added test case for getAllPreferences diffstat: modules/org.openbravo.client.application/src/org/openbravo/client/application/ADAlertDatasourceService.java | 26 +-- src-test/src/org/openbravo/test/preference/PreferenceTest.java | 72 ++++++-- src/org/openbravo/erpCommon/ad_actionButton/CreateFrom.java | 5 +- src/org/openbravo/erpCommon/ad_process/UpdateActuals.java | 51 +++--- src/org/openbravo/erpCommon/businessUtility/Preferences.java | 77 ++------- src/org/openbravo/erpCommon/obps/ActivationKey.java | 2 +- src/org/openbravo/erpCommon/utility/Utility.java | 59 ++++++- 7 files changed, 160 insertions(+), 132 deletions(-) diffs (truncated from 620 to 300 lines): diff -r 72ca71e6b78a -r f930ff85c698 modules/org.openbravo.client.application/src/org/openbravo/client/application/ADAlertDatasourceService.java --- a/modules/org.openbravo.client.application/src/org/openbravo/client/application/ADAlertDatasourceService.java Mon Nov 20 12:01:56 2017 +0100 +++ b/modules/org.openbravo.client.application/src/org/openbravo/client/application/ADAlertDatasourceService.java Tue Nov 21 09:53:27 2017 +0100 @@ -18,9 +18,10 @@ */ package org.openbravo.client.application; +import static org.openbravo.erpCommon.utility.Utility.commaSeparated; + import java.util.ArrayList; import java.util.HashMap; -import java.util.Iterator; import java.util.List; import java.util.Map; import java.util.Map.Entry; @@ -151,7 +152,7 @@ final String sql = "SELECT ad_alert_id FROM ad_alert WHERE isactive='Y'" + " AND ad_client_id " + OBDal.getInstance().getReadableClientsInClause() + " AND ad_org_id " + OBDal.getInstance().getReadableOrganizationsInClause() - + " AND ad_alertrule_id IN (" + toStringList(alertRuleList.getValue()) + ")" + + " AND ad_alertrule_id IN (" + commaSeparated(alertRuleList.getValue()) + ")" + filterClause + " AND coalesce(to_char(status), 'NEW') = :status"; final SQLQuery sqlQuery = OBDal.getInstance().getSession().createSQLQuery(sql); sqlQuery.setParameter("status", alertStatus); @@ -186,7 +187,7 @@ } if (alertList.size() <= chunkSize) { - return "e.id in (" + toStringList(alertList) + ")"; + return "e.id in (" + commaSeparated(alertList) + ")"; } // There are more than 1000 alerts to include in the where clause, Oracle doesn't @@ -195,30 +196,17 @@ while (alertList.size() > chunkSize) { alertListToRemove = new ArrayList<String>(alertList.subList(0, chunkSize - 1)); if (StringUtils.isEmpty(whereClause)) { - whereClause = "(e.id in (" + toStringList(alertListToRemove) + ")"; + whereClause = "(e.id in (" + commaSeparated(alertListToRemove) + ")"; } else { - whereClause += " or e.id in (" + toStringList(alertListToRemove) + ")"; + whereClause += " or e.id in (" + commaSeparated(alertListToRemove) + ")"; } alertList.removeAll(alertListToRemove); } if (!alertList.isEmpty()) { - whereClause += " or e.id in (" + toStringList(alertList) + "))"; + whereClause += " or e.id in (" + commaSeparated(alertList) + "))"; } else { whereClause += ")"; } return whereClause; } - - private String toStringList(List<String> list) { - StringBuilder result = new StringBuilder(list.size() * 35); - Iterator<String> iterator = list.iterator(); - while (iterator.hasNext()) { - String item = iterator.next(); - result.append("'").append(item).append("'"); - if (iterator.hasNext()) { - result.append(","); - } - } - return result.toString(); - } } \ No newline at end of file diff -r 72ca71e6b78a -r f930ff85c698 src-test/src/org/openbravo/test/preference/PreferenceTest.java --- a/src-test/src/org/openbravo/test/preference/PreferenceTest.java Mon Nov 20 12:01:56 2017 +0100 +++ b/src-test/src/org/openbravo/test/preference/PreferenceTest.java Tue Nov 21 09:53:27 2017 +0100 @@ -11,13 +11,16 @@ * under the License. * The Original Code is Openbravo ERP. * The Initial Developer of the Original Code is Openbravo SLU - * All portions are Copyright (C) 2010-2016 Openbravo SLU + * All portions are Copyright (C) 2010-2017 Openbravo SLU * All Rights Reserved. * Contributor(s): ______________________________________. ************************************************************************ */ package org.openbravo.test.preference; +import static org.hamcrest.Matchers.allOf; +import static org.hamcrest.Matchers.hasItem; +import static org.hamcrest.Matchers.hasSize; import static org.hamcrest.Matchers.is; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; @@ -29,6 +32,7 @@ import java.sql.PreparedStatement; import java.sql.ResultSet; import java.sql.SQLException; +import java.util.ArrayList; import java.util.Date; import java.util.List; @@ -56,6 +60,12 @@ import org.openbravo.model.common.enterprise.Organization; import org.openbravo.test.base.OBBaseTest; +/** + * Test cases covering Preferences visibility and conflict resolution handling. + * + * Note these test cases are dependent one on each other, therefore all of them must be executed one + * after the other sorted alphabetically. + */ @FixMethodOrder(MethodSorters.NAME_ASCENDING) public class PreferenceTest extends OBBaseTest { @@ -87,7 +97,7 @@ private static final String ORG_B2 = "B843C30461EA4501935CB1D125C9C25A"; @Test - public void testACreatePreference() { + public void test00CreatePreference() { setSystemAdministratorContext(); Preferences.setPreferenceValue("testProperty", "testValue", false, null, null, null, null, @@ -105,7 +115,7 @@ } @Test - public void testBOverwritePreference() { + public void test01OverwritePreference() { setSystemAdministratorContext(); Preferences.setPreferenceValue("testProperty", "newValue", false, null, null, null, null, null, @@ -124,7 +134,7 @@ } @Test - public void testCSamePropertyDifferentVisibility() { + public void test02SamePropertyDifferentVisibility() { setSystemAdministratorContext(); Role role = OBDal.getInstance().get(Role.class, SALES_ROLE_ID); // Sales @@ -142,7 +152,7 @@ } @Test - public void testDPropertyGet() throws PropertyException { + public void test03PropertyGet() throws PropertyException { setSystemAdministratorContext(); String value = Preferences.getPreferenceValue("testProperty", false, OBContext.getOBContext() .getCurrentClient(), OBContext.getOBContext().getCurrentOrganization(), OBContext @@ -157,7 +167,7 @@ } @Test - public void testEPLPropertyGet() throws SQLException { + public void test04PLPropertyGet() throws SQLException { setSystemAdministratorContext(); String value = getPLPreference("testProperty", false, OBContext.getOBContext() .getCurrentClient(), OBContext.getOBContext().getCurrentOrganization(), OBContext @@ -172,7 +182,7 @@ } @Test - public void testFWindowVisibility() throws PropertyException { + public void test05WindowVisibility() throws PropertyException { setSystemAdministratorContext(); Window window = OBDal.getInstance().get(Window.class, "276"); // Alert window Preferences.setPreferenceValue("testProperty", "alertGeneral", false, null, null, null, null, @@ -206,7 +216,7 @@ } @Test - public void testGPLWindowVisibility() throws SQLException { + public void test06PLWindowVisibility() throws SQLException { setSystemAdministratorContext(); Window window = OBDal.getInstance().get(Window.class, "276"); // Alert window Preferences.setPreferenceValue("testProperty", "alertGeneral", false, null, null, null, null, @@ -236,7 +246,7 @@ } @Test - public void testHOrgVisibility() throws PropertyException { + public void test07OrgVisibility() throws PropertyException { setSystemAdministratorContext(); Client client = OBDal.getInstance().get(Client.class, TEST_CLIENT_ID); Organization orgB = OBDal.getInstance().get(Organization.class, ORG_B); @@ -284,7 +294,7 @@ } @Test - public void testH1ClientVisibility() throws PropertyException { + public void test08ClientVisibility() throws PropertyException { setSystemAdministratorContext(); Client testClient = OBDal.getInstance().getProxy(Client.class, TEST_CLIENT_ID); Client systemClient = OBDal.getInstance().getProxy(Client.class, "0"); @@ -313,7 +323,7 @@ } @Test - public void testIPLOrgVisibility() throws SQLException { + public void test09PLOrgVisibility() throws SQLException { setSystemAdministratorContext(); Client client = OBDal.getInstance().get(Client.class, TEST_CLIENT_ID); @@ -351,7 +361,7 @@ } @Test - public void testJExceptionNotFound() { + public void test10ExceptionNotFound() { PropertyException exception = null; try { Preferences.getPreferenceValue("testNotExists", false, OBContext.getOBContext() @@ -367,7 +377,7 @@ } @Test - public void testKPLExceptionNotFound() { + public void test11PLExceptionNotFound() { SQLException exception = null; try { getPLPreference("testNotExists", false, OBContext.getOBContext().getCurrentClient(), @@ -383,7 +393,7 @@ } @Test - public void testLConflict() { + public void test12Conflict() { setSystemAdministratorContext(); Preference newPref = OBProvider.getInstance().get(Preference.class); newPref.setPropertyList(false); @@ -407,7 +417,7 @@ } @Test - public void testMPLConflict() { + public void test13PLConflict() { setSystemAdministratorContext(); SQLException exception = null; try { @@ -423,7 +433,7 @@ } @Test - public void testNSolvedConflict() throws PropertyException { + public void test14SolvedConflict() throws PropertyException { setSystemAdministratorContext(); // This piece of code doesn't work because of issue #13153 @@ -452,7 +462,7 @@ } @Test - public void testOPLSolvedConflict() throws SQLException { + public void test15PLSolvedConflict() throws SQLException { setSystemAdministratorContext(); String value = getPLPreference("testProperty", false, OBContext.getOBContext() .getCurrentClient(), OBContext.getOBContext().getCurrentOrganization(), OBContext @@ -461,7 +471,7 @@ } @Test - public void testPPreferenceClientOrgSetting() { + public void test16PreferenceClientOrgSetting() { setTestAdminContext(); Preference p = Preferences.setPreferenceValue("testProperty2", "testValue", false, null, null, null, null, null, null); @@ -471,7 +481,7 @@ } @Test - public void testQPreferenceListSetAndGet() throws PropertyException { + public void test17PreferenceListSetAndGet() throws PropertyException { setSystemAdministratorContext(); // Property configuration list @@ -513,7 +523,29 @@ } @Test - public void testRClean() { + public void test18GetAllPreferences() throws PropertyException { + // setSystemAdministratorContext(); + setTestUserContext(); + + List<Preference> allPrefs = Preferences.getAllPreferences(OBContext.getOBContext() + .getCurrentClient().getId(), OBContext.getOBContext().getCurrentOrganization().getId(), + OBContext.getOBContext().getUser().getId(), OBContext.getOBContext().getRole().getId()); + List<String> testPreferenceValues = new ArrayList<>(); + for (Preference pref : allPrefs) { + String key = pref.isPropertyList() ? pref.getProperty() : pref.getAttribute(); + if (key.startsWith("testProperty")) { + testPreferenceValues.add(key + " " + pref.getSearchKey()); + } + } + assertThat( + testPreferenceValues, + allOf(hasItem("testProperty2 testValue"), hasItem("testProperty B2"), + hasItem("testProperty alertGeneral"), hasItem("testPropertyList testPropValue"))); + assertThat(testPreferenceValues, hasSize(4)); + } + + @Test + public void test99Clean() { ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot _______________________________________________ Openbravo-commits mailing list Openbravo-commits@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openbravo-commits
