Roy Golan has uploaded a new change for review. Change subject: core: remove unneeded reflection use in SyntaxChecker ......................................................................
core: remove unneeded reflection use in SyntaxChecker Using the built-in valueOf method of Enum instead of fancy, expensive other ways to try to get the enum value. The former code did eventually a valueOf check which is case sensitive and then another EnumUtils method which was case *insensitive*. Since the inner EnumUtils call would only be read on a match there is no point in repeating the enum fetching twice, and in a very expensive way when we have a ready made method for it. Change-Id: Idd74aad49d292e64dec6f5925ffd186f13b9887c Signed-off-by: Roy Golan <[email protected]> --- M backend/manager/modules/searchbackend/pom.xml M backend/manager/modules/searchbackend/src/main/java/org/ovirt/engine/core/searchbackend/SyntaxChecker.java M backend/manager/modules/searchbackend/src/test/java/org/ovirt/engine/core/searchbackend/SyntaxCheckerTest.java 3 files changed, 63 insertions(+), 16 deletions(-) git pull ssh://gerrit.ovirt.org:29418/ovirt-engine refs/changes/61/9461/1 diff --git a/backend/manager/modules/searchbackend/pom.xml b/backend/manager/modules/searchbackend/pom.xml index 0ffc79b..16a8ca8 100644 --- a/backend/manager/modules/searchbackend/pom.xml +++ b/backend/manager/modules/searchbackend/pom.xml @@ -24,7 +24,21 @@ <artifactId>common</artifactId> <version>${engine.version}</version> </dependency> - </dependencies> + + <dependency> + <groupId>${engine.groupId}</groupId> + <artifactId>utils</artifactId> + <version>${engine.version}</version> + </dependency> + + <dependency> + <groupId>${engine.groupId}</groupId> + <artifactId>utils</artifactId> + <version>${engine.version}</version> + <type>test-jar</type> + <scope>test</scope> + </dependency> +</dependencies> <build> <plugins> diff --git a/backend/manager/modules/searchbackend/src/main/java/org/ovirt/engine/core/searchbackend/SyntaxChecker.java b/backend/manager/modules/searchbackend/src/main/java/org/ovirt/engine/core/searchbackend/SyntaxChecker.java index 77ea2d5..8c20dd6 100644 --- a/backend/manager/modules/searchbackend/src/main/java/org/ovirt/engine/core/searchbackend/SyntaxChecker.java +++ b/backend/manager/modules/searchbackend/src/main/java/org/ovirt/engine/core/searchbackend/SyntaxChecker.java @@ -7,18 +7,16 @@ import java.util.ListIterator; import java.util.Map; -import org.apache.commons.logging.Log; -import org.apache.commons.logging.LogFactory; import org.ovirt.engine.core.common.config.Config; import org.ovirt.engine.core.common.config.ConfigValues; import org.ovirt.engine.core.common.errors.SqlInjectionException; -import org.ovirt.engine.core.common.utils.EnumUtils; -import org.ovirt.engine.core.compat.EnumCompat; import org.ovirt.engine.core.compat.IntegerCompat; import org.ovirt.engine.core.compat.RefObject; import org.ovirt.engine.core.compat.Regex; import org.ovirt.engine.core.compat.StringFormat; import org.ovirt.engine.core.compat.StringHelper; +import org.ovirt.engine.core.utils.log.Log; +import org.ovirt.engine.core.utils.log.LogFactory; public class SyntaxChecker implements ISyntaxChecker { @@ -894,33 +892,42 @@ return retval; } - private String getPagePhrase(SyntaxContainer syntax, String pageNumber) { + protected String getPagePhrase(SyntaxContainer syntax, String pageNumber) { String result = ""; Integer page = IntegerCompat.tryParse(pageNumber); if (page == null) { page = 1; } - String pagingTypeStr = Config.<String> GetValue(ConfigValues.DBPagingType); - if (EnumCompat.IsDefined(PagingType.class, pagingTypeStr)) { - PagingType pagingType = EnumUtils.valueOf(PagingType.class, pagingTypeStr, true); + PagingType pagingType = getPagingType(); + if (pagingType != null) { String pagingSyntax = Config.<String> GetValue(ConfigValues.DBPagingSyntax); switch (pagingType) { case Range: - result = StringFormat - .format(pagingSyntax, (page - 1) * syntax.getMaxCount() + 1, page * syntax.getMaxCount()); + result = + String.format(pagingSyntax, (page - 1) * syntax.getMaxCount() + 1, page * syntax.getMaxCount()); break; case Offset: - result = StringFormat.format(pagingSyntax, (page - 1) * syntax.getMaxCount() + 1, syntax.getMaxCount()); + result = String.format(pagingSyntax, (page - 1) * syntax.getMaxCount() + 1, syntax.getMaxCount()); break; } - } else { - log.error(StringFormat.format("Unknown paging type %1$s", pagingTypeStr)); } return result; } + private PagingType getPagingType() { + String val = Config.<String> GetValue(ConfigValues.DBPagingType); + PagingType type = null; + try { + type = PagingType.valueOf(val); + } catch (Exception e) { + log.errorFormat("Unknown paging type %1$s", val); + } + + return type; + } + private enum ConditionType { None, FreeText, diff --git a/backend/manager/modules/searchbackend/src/test/java/org/ovirt/engine/core/searchbackend/SyntaxCheckerTest.java b/backend/manager/modules/searchbackend/src/test/java/org/ovirt/engine/core/searchbackend/SyntaxCheckerTest.java index 1562950..0ebcb60 100644 --- a/backend/manager/modules/searchbackend/src/test/java/org/ovirt/engine/core/searchbackend/SyntaxCheckerTest.java +++ b/backend/manager/modules/searchbackend/src/test/java/org/ovirt/engine/core/searchbackend/SyntaxCheckerTest.java @@ -1,10 +1,18 @@ package org.ovirt.engine.core.searchbackend; +import static org.junit.Assert.assertTrue; + import java.util.Arrays; -import junit.framework.TestCase; +import org.junit.Rule; +import org.junit.Test; +import org.ovirt.engine.core.common.config.ConfigValues; +import org.ovirt.engine.core.utils.MockConfigRule; -public class SyntaxCheckerTest extends TestCase { +public class SyntaxCheckerTest { + + @Rule + public MockConfigRule mcr = new MockConfigRule(); public void dumpCompletionArray(SyntaxContainer res) { System.out.print("["); @@ -25,6 +33,7 @@ /** * Test the following where each word should be the completion for the earlier portion Vms : Events = */ + @Test public void testVMCompletion() { SyntaxChecker chkr = new SyntaxChecker(20, true); SyntaxContainer res = null; @@ -62,4 +71,21 @@ assertTrue("asc", contains(res, "asc")); } + @Test + public void testGetPagPhrase() { + mcr.mockConfigValue(ConfigValues.DBPagingType, "wrongPageType"); + mcr.mockConfigValue(ConfigValues.DBPagingSyntax, "wrongPageSyntax"); + SyntaxChecker chkr = new SyntaxChecker(20, true); + SyntaxContainer res = new SyntaxContainer(""); + res.setMaxCount(0); + + // check wrong config values + assertTrue(chkr.getPagePhrase(res, "1") == ""); + + mcr.mockConfigValue(ConfigValues.DBPagingType, "Range"); + mcr.mockConfigValue(ConfigValues.DBPagingSyntax, " WHERE RowNum BETWEEN %1$s AND %2$s"); + + // check valid config values + assertTrue(chkr.getPagePhrase(res, "1") != ""); + } } -- To view, visit http://gerrit.ovirt.org/9461 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: newchange Gerrit-Change-Id: Idd74aad49d292e64dec6f5925ffd186f13b9887c Gerrit-PatchSet: 1 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Roy Golan <[email protected]> _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
