From 3ac6835c5ca71cc5ee085680ade48aac6883d2fa Mon Sep 17 00:00:00 2001 From: Yasser Zamani Date: Thu, 30 May 2019 16:36:54 +0430 Subject: [PATCH 1/5] fix logMissingProperties (WW-4999) Moves checking OgnlValueStack.THROW_EXCEPTION_ON_FAILURE outside loop because it shouldn't throw exception on first failure while is trying all root objects. Returns on first successful call because it's not rational and is confusing user to skip when user method successfully returns null as an actual result. Fixes WW-4999 via honoring (devMode && logMissingProperties) for OgnlValueStack.THROW_EXCEPTION_ON_FAILURE and REPORT_ERRORS_ON_NO_PROP. --- .../xwork2/ognl/OgnlValueStack.java | 5 +- .../ognl/accessor/CompoundRootAccessor.java | 17 +++---- .../xwork2/ognl/OgnlValueStackTest.java | 51 +++++++++++++++++++ 3 files changed, 62 insertions(+), 11 deletions(-) diff --git a/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlValueStack.java b/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlValueStack.java index 054e81a57..124354f3f 100644 --- a/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlValueStack.java +++ b/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlValueStack.java @@ -182,7 +182,8 @@ public class OgnlValueStack implements Serializable, ValueStack, ClearableValueS private void trySetValue(String expr, Object value, boolean throwExceptionOnFailure, Map context) throws OgnlException { context.put(XWorkConverter.CONVERSION_PROPERTY_FULLNAME, expr); - context.put(REPORT_ERRORS_ON_NO_PROP, (throwExceptionOnFailure) ? Boolean.TRUE : Boolean.FALSE); + context.put(REPORT_ERRORS_ON_NO_PROP, throwExceptionOnFailure || (devMode && logMissingProperties) + ? Boolean.TRUE : Boolean.FALSE); ognlUtil.setValue(expr, context, root, value); } @@ -246,7 +247,7 @@ public class OgnlValueStack implements Serializable, ValueStack, ClearableValueS } protected void setupExceptionOnFailure(boolean throwExceptionOnFailure) { - if (throwExceptionOnFailure) { + if (throwExceptionOnFailure || (devMode && logMissingProperties)) { context.put(THROW_EXCEPTION_ON_FAILURE, true); } } diff --git a/core/src/main/java/com/opensymphony/xwork2/ognl/accessor/CompoundRootAccessor.java b/core/src/main/java/com/opensymphony/xwork2/ognl/accessor/CompoundRootAccessor.java index e81511ee1..9a20a71bc 100644 --- a/core/src/main/java/com/opensymphony/xwork2/ognl/accessor/CompoundRootAccessor.java +++ b/core/src/main/java/com/opensymphony/xwork2/ognl/accessor/CompoundRootAccessor.java @@ -217,6 +217,7 @@ public class CompoundRootAccessor implements PropertyAccessor, MethodAccessor, C return null; } + Throwable reason = null; for (Object o : root) { if (o == null) { continue; @@ -233,24 +234,22 @@ public class CompoundRootAccessor implements PropertyAccessor, MethodAccessor, C if ((argTypes == null) || !invalidMethods.containsKey(mc)) { try { - Object value = OgnlRuntime.callMethod((OgnlContext) context, o, name, objects); - - if (value != null) { - return value; - } + return OgnlRuntime.callMethod((OgnlContext) context, o, name, objects); } catch (OgnlException e) { // try the next one - Throwable reason = e.getReason(); + reason = e.getReason(); - if (!context.containsKey(OgnlValueStack.THROW_EXCEPTION_ON_FAILURE) && (mc != null) && (reason != null) && (reason.getClass() == NoSuchMethodException.class)) { + if ((mc != null) && (reason != null) && (reason.getClass() == NoSuchMethodException.class)) { invalidMethods.put(mc, Boolean.TRUE); - } else if (reason != null) { - throw new MethodFailedException(o, name, e.getReason()); } } } } + if (context.containsKey(OgnlValueStack.THROW_EXCEPTION_ON_FAILURE)) { + throw new MethodFailedException(target, name, reason); + } + return null; } diff --git a/core/src/test/java/com/opensymphony/xwork2/ognl/OgnlValueStackTest.java b/core/src/test/java/com/opensymphony/xwork2/ognl/OgnlValueStackTest.java index b04f38f2d..a818f0063 100644 --- a/core/src/test/java/com/opensymphony/xwork2/ognl/OgnlValueStackTest.java +++ b/core/src/test/java/com/opensymphony/xwork2/ognl/OgnlValueStackTest.java @@ -34,9 +34,16 @@ import ognl.PropertyAccessor; import java.io.*; import java.math.BigDecimal; +import java.util.ArrayList; import java.util.HashMap; import java.util.LinkedHashMap; +import java.util.List; import java.util.Map; + +import org.apache.logging.log4j.LogManager; +import org.apache.logging.log4j.core.LogEvent; +import org.apache.logging.log4j.core.Logger; +import org.apache.logging.log4j.core.appender.AbstractAppender; import org.apache.struts2.StrutsConstants; @@ -238,6 +245,37 @@ public class OgnlValueStackTest extends XWorkTestCase { } } + public void testLogMissingProperties() { + OgnlValueStack vs = createValueStack(); + vs.setDevMode("true"); + vs.setLogMissingProperties("true"); + + Dog dog = new Dog(); + vs.push(dog); + + TestAppender testAppender = new TestAppender(); + Logger logger = (Logger) LogManager.getLogger(OgnlValueStack.class); + logger.addAppender(testAppender); + testAppender.start(); + + try { + vs.setValue("missingProp1", "missingProp1Value", false); + vs.findValue("missingProp2", false); + vs.findValue("missingProp3", Integer.class, false); + + assertEquals(3, testAppender.logEvents.size()); + assertEquals("Error setting value [missingProp1Value] with expression [missingProp1]", + testAppender.logEvents.get(0).getMessage().getFormattedMessage()); + assertEquals("Could not find property [missingProp2]!", + testAppender.logEvents.get(1).getMessage().getFormattedMessage()); + assertEquals("Could not find property [missingProp3]!", + testAppender.logEvents.get(2).getMessage().getFormattedMessage()); + } finally { + testAppender.stop(); + logger.removeAppender(testAppender); + } + } + public void testFailOnMissingMethod() { OgnlValueStack vs = createValueStack(); @@ -1293,6 +1331,19 @@ public class OgnlValueStackTest extends XWorkTestCase { this.displayName = displayName; } } + + class TestAppender extends AbstractAppender { + List logEvents = new ArrayList<>(); + + TestAppender() { + super("TestAppender", null, null, false, null); + } + + @Override + public void append(LogEvent logEvent) { + logEvents.add(logEvent); + } + } } enum MyNumbers { From 0999fba8c496588bb55df0a025875cc76b35ce96 Mon Sep 17 00:00:00 2001 From: Yasser Zamani Date: Fri, 31 May 2019 17:57:25 +0430 Subject: [PATCH 2/5] not log user exceptions as missing properties (WW-4999) Also reaks loop on user method exceptions - but continue to next objects in stack on NoSuchMethodException. --- .../xwork2/ognl/OgnlValueStack.java | 3 +- .../ognl/accessor/CompoundRootAccessor.java | 11 ++++-- .../xwork2/ognl/OgnlValueStackTest.java | 37 +++++++++++++++++++ 3 files changed, 47 insertions(+), 4 deletions(-) diff --git a/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlValueStack.java b/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlValueStack.java index 124354f3f..2116bdc4e 100644 --- a/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlValueStack.java +++ b/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlValueStack.java @@ -340,7 +340,8 @@ public class OgnlValueStack implements Serializable, ValueStack, ClearableValueS } protected boolean shouldLogMissingPropertyWarning(OgnlException e) { - return (e instanceof NoSuchPropertyException || e instanceof MethodFailedException) + return (e instanceof NoSuchPropertyException || + (e instanceof MethodFailedException && e.getReason() instanceof NoSuchMethodException)) && devMode && logMissingProperties; } diff --git a/core/src/main/java/com/opensymphony/xwork2/ognl/accessor/CompoundRootAccessor.java b/core/src/main/java/com/opensymphony/xwork2/ognl/accessor/CompoundRootAccessor.java index 9a20a71bc..9a0bd5d1e 100644 --- a/core/src/main/java/com/opensymphony/xwork2/ognl/accessor/CompoundRootAccessor.java +++ b/core/src/main/java/com/opensymphony/xwork2/ognl/accessor/CompoundRootAccessor.java @@ -218,13 +218,13 @@ public class CompoundRootAccessor implements PropertyAccessor, MethodAccessor, C } Throwable reason = null; + Class[] argTypes = getArgTypes(objects); for (Object o : root) { if (o == null) { continue; } Class clazz = o.getClass(); - Class[] argTypes = getArgTypes(objects); MethodCall mc = null; @@ -236,12 +236,17 @@ public class CompoundRootAccessor implements PropertyAccessor, MethodAccessor, C try { return OgnlRuntime.callMethod((OgnlContext) context, o, name, objects); } catch (OgnlException e) { - // try the next one reason = e.getReason(); - if ((mc != null) && (reason != null) && (reason.getClass() == NoSuchMethodException.class)) { + if (reason != null && !(reason instanceof NoSuchMethodException)) { + // method has found but thrown an exception + break; + } + + if ((mc != null) && (reason != null)) { invalidMethods.put(mc, Boolean.TRUE); } + // continue and try the next one } } } diff --git a/core/src/test/java/com/opensymphony/xwork2/ognl/OgnlValueStackTest.java b/core/src/test/java/com/opensymphony/xwork2/ognl/OgnlValueStackTest.java index a818f0063..e7e273cba 100644 --- a/core/src/test/java/com/opensymphony/xwork2/ognl/OgnlValueStackTest.java +++ b/core/src/test/java/com/opensymphony/xwork2/ognl/OgnlValueStackTest.java @@ -276,6 +276,43 @@ public class OgnlValueStackTest extends XWorkTestCase { } } + public void testNotLogUserExceptionsAsMissingProperties() { + OgnlValueStack vs = createValueStack(); + vs.setDevMode("true"); + vs.setLogMissingProperties("true"); + + Dog dog = new Dog(); + vs.push(dog); + + TestAppender testAppender = new TestAppender(); + Logger logger = (Logger) LogManager.getLogger(OgnlValueStack.class); + logger.addAppender(testAppender); + testAppender.start(); + + try { + vs.setValue("exception", "exceptionValue", false); + vs.findValue("exception", false); + vs.findValue("exception", String.class, false); + vs.findValue("getException()", false); + vs.findValue("getException()", String.class, false); + vs.findValue("bite", false); + vs.findValue("bite", void.class, false); + vs.findValue("getBite()", false); + vs.findValue("getBite()", void.class, false); + + assertEquals(8, testAppender.logEvents.size()); + for (int i = 0; i < testAppender.logEvents.size(); i += 2) { + assertTrue(testAppender.logEvents.get(i).getMessage().getFormattedMessage() + .startsWith("Caught an exception while evaluating expression '")); + assertEquals("NOTE: Previous warning message was issued due to devMode set to true.", + testAppender.logEvents.get(i + 1).getMessage().getFormattedMessage()); + } + } finally { + testAppender.stop(); + logger.removeAppender(testAppender); + } + } + public void testFailOnMissingMethod() { OgnlValueStack vs = createValueStack(); From d4dd3386cc02833fa7113a496092b2e09a53c9e4 Mon Sep 17 00:00:00 2001 From: Yasser Zamani Date: Sat, 1 Jun 2019 09:37:19 +0430 Subject: [PATCH 3/5] test not throw exception on top missing property (WW-4999) instead continue to next objects in stack Also tests not skip returned null values by user method --- .../xwork2/ognl/OgnlValueStackTest.java | 27 +++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/core/src/test/java/com/opensymphony/xwork2/ognl/OgnlValueStackTest.java b/core/src/test/java/com/opensymphony/xwork2/ognl/OgnlValueStackTest.java index e7e273cba..54b5ad725 100644 --- a/core/src/test/java/com/opensymphony/xwork2/ognl/OgnlValueStackTest.java +++ b/core/src/test/java/com/opensymphony/xwork2/ognl/OgnlValueStackTest.java @@ -954,6 +954,33 @@ public class OgnlValueStackTest extends XWorkTestCase { assertNull(vs.findValue("@com.nothing.here.Nothing@BLAH")); } + /** + * Fails on 2.5.20 and earlier - tested on 2.5 (5/5/2016) and failed + * @since 2.5.21 + */ + public void testNotThrowExceptionOnTopMissingProperty() { + OgnlValueStack vs = createValueStack(); + + Dog dog = new Dog(); + dog.setName("Rover"); + vs.push(dog); + + Cat cat = new Cat(); + vs.push(cat); + + vs.setValue("age", 12, true); + + assertEquals(12, vs.findValue("age", true)); + assertEquals(12, vs.findValue("age", Integer.class, true)); + assertEquals(12, vs.findValue("getAge()", true)); + assertEquals(12, vs.findValue("getAge()", Integer.class, true)); + + assertNull(vs.findValue("name", true)); + assertNull(vs.findValue("name", String.class, true)); + assertNull(vs.findValue("getName()", true)); + assertNull(vs.findValue("getName()", String.class, true)); + } + public void testTop() { OgnlValueStack vs = createValueStack(); From 9e01fbd2ddeb6dd1962f76a3bd8af227d370762d Mon Sep 17 00:00:00 2001 From: Yasser Zamani Date: Sat, 1 Jun 2019 12:52:22 +0430 Subject: [PATCH 4/5] decouple logMissingProperties from devMode (WW-4999) --- .../com/opensymphony/xwork2/ognl/OgnlValueStack.java | 7 +++---- .../opensymphony/xwork2/ognl/OgnlValueStackTest.java | 10 +--------- 2 files changed, 4 insertions(+), 13 deletions(-) diff --git a/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlValueStack.java b/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlValueStack.java index 2116bdc4e..6bfdb31c9 100644 --- a/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlValueStack.java +++ b/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlValueStack.java @@ -182,8 +182,7 @@ public class OgnlValueStack implements Serializable, ValueStack, ClearableValueS private void trySetValue(String expr, Object value, boolean throwExceptionOnFailure, Map context) throws OgnlException { context.put(XWorkConverter.CONVERSION_PROPERTY_FULLNAME, expr); - context.put(REPORT_ERRORS_ON_NO_PROP, throwExceptionOnFailure || (devMode && logMissingProperties) - ? Boolean.TRUE : Boolean.FALSE); + context.put(REPORT_ERRORS_ON_NO_PROP, throwExceptionOnFailure || logMissingProperties ? Boolean.TRUE : Boolean.FALSE); ognlUtil.setValue(expr, context, root, value); } @@ -247,7 +246,7 @@ public class OgnlValueStack implements Serializable, ValueStack, ClearableValueS } protected void setupExceptionOnFailure(boolean throwExceptionOnFailure) { - if (throwExceptionOnFailure || (devMode && logMissingProperties)) { + if (throwExceptionOnFailure || logMissingProperties) { context.put(THROW_EXCEPTION_ON_FAILURE, true); } } @@ -342,7 +341,7 @@ public class OgnlValueStack implements Serializable, ValueStack, ClearableValueS protected boolean shouldLogMissingPropertyWarning(OgnlException e) { return (e instanceof NoSuchPropertyException || (e instanceof MethodFailedException && e.getReason() instanceof NoSuchMethodException)) - && devMode && logMissingProperties; + && logMissingProperties; } private Object tryFindValue(String expr, Class asType) throws OgnlException { diff --git a/core/src/test/java/com/opensymphony/xwork2/ognl/OgnlValueStackTest.java b/core/src/test/java/com/opensymphony/xwork2/ognl/OgnlValueStackTest.java index 54b5ad725..f69fb7d98 100644 --- a/core/src/test/java/com/opensymphony/xwork2/ognl/OgnlValueStackTest.java +++ b/core/src/test/java/com/opensymphony/xwork2/ognl/OgnlValueStackTest.java @@ -247,7 +247,6 @@ public class OgnlValueStackTest extends XWorkTestCase { public void testLogMissingProperties() { OgnlValueStack vs = createValueStack(); - vs.setDevMode("true"); vs.setLogMissingProperties("true"); Dog dog = new Dog(); @@ -278,7 +277,6 @@ public class OgnlValueStackTest extends XWorkTestCase { public void testNotLogUserExceptionsAsMissingProperties() { OgnlValueStack vs = createValueStack(); - vs.setDevMode("true"); vs.setLogMissingProperties("true"); Dog dog = new Dog(); @@ -300,13 +298,7 @@ public class OgnlValueStackTest extends XWorkTestCase { vs.findValue("getBite()", false); vs.findValue("getBite()", void.class, false); - assertEquals(8, testAppender.logEvents.size()); - for (int i = 0; i < testAppender.logEvents.size(); i += 2) { - assertTrue(testAppender.logEvents.get(i).getMessage().getFormattedMessage() - .startsWith("Caught an exception while evaluating expression '")); - assertEquals("NOTE: Previous warning message was issued due to devMode set to true.", - testAppender.logEvents.get(i + 1).getMessage().getFormattedMessage()); - } + assertEquals(0, testAppender.logEvents.size()); } finally { testAppender.stop(); logger.removeAppender(testAppender); From a50af87644320a3ca85283ccab7bc72822cb230b Mon Sep 17 00:00:00 2001 From: Yasser Zamani Date: Sun, 2 Jun 2019 17:22:28 +0430 Subject: [PATCH 5/5] test false for logMissingProperties (WW-4999) --- .../xwork2/ognl/OgnlValueStackTest.java | 61 ++++++++++++++++--- 1 file changed, 53 insertions(+), 8 deletions(-) diff --git a/core/src/test/java/com/opensymphony/xwork2/ognl/OgnlValueStackTest.java b/core/src/test/java/com/opensymphony/xwork2/ognl/OgnlValueStackTest.java index f69fb7d98..72a84e9dd 100644 --- a/core/src/test/java/com/opensymphony/xwork2/ognl/OgnlValueStackTest.java +++ b/core/src/test/java/com/opensymphony/xwork2/ognl/OgnlValueStackTest.java @@ -245,9 +245,18 @@ public class OgnlValueStackTest extends XWorkTestCase { } } + /** + * monitors the resolution of WW-4999 + * @since 2.5.21 + */ public void testLogMissingProperties() { + testLogMissingProperties(true); + testLogMissingProperties(false); + } + + private void testLogMissingProperties(boolean logMissingProperties) { OgnlValueStack vs = createValueStack(); - vs.setLogMissingProperties("true"); + vs.setLogMissingProperties("" + logMissingProperties); Dog dog = new Dog(); vs.push(dog); @@ -262,19 +271,27 @@ public class OgnlValueStackTest extends XWorkTestCase { vs.findValue("missingProp2", false); vs.findValue("missingProp3", Integer.class, false); - assertEquals(3, testAppender.logEvents.size()); - assertEquals("Error setting value [missingProp1Value] with expression [missingProp1]", - testAppender.logEvents.get(0).getMessage().getFormattedMessage()); - assertEquals("Could not find property [missingProp2]!", - testAppender.logEvents.get(1).getMessage().getFormattedMessage()); - assertEquals("Could not find property [missingProp3]!", - testAppender.logEvents.get(2).getMessage().getFormattedMessage()); + if (logMissingProperties) { + assertEquals(3, testAppender.logEvents.size()); + assertEquals("Error setting value [missingProp1Value] with expression [missingProp1]", + testAppender.logEvents.get(0).getMessage().getFormattedMessage()); + assertEquals("Could not find property [missingProp2]!", + testAppender.logEvents.get(1).getMessage().getFormattedMessage()); + assertEquals("Could not find property [missingProp3]!", + testAppender.logEvents.get(2).getMessage().getFormattedMessage()); + } else { + assertEquals(0, testAppender.logEvents.size()); + } } finally { testAppender.stop(); logger.removeAppender(testAppender); } } + /** + * tests the correctness of distinguishing between user exception and NoSuchMethodException + * @since 2.5.21 + */ public void testNotLogUserExceptionsAsMissingProperties() { OgnlValueStack vs = createValueStack(); vs.setLogMissingProperties("true"); @@ -298,6 +315,18 @@ public class OgnlValueStackTest extends XWorkTestCase { vs.findValue("getBite()", false); vs.findValue("getBite()", void.class, false); + vs.setLogMissingProperties("false"); + + vs.setValue("exception", "exceptionValue", false); + vs.findValue("exception", false); + vs.findValue("exception", String.class, false); + vs.findValue("getException()", false); + vs.findValue("getException()", String.class, false); + vs.findValue("bite", false); + vs.findValue("bite", void.class, false); + vs.findValue("getBite()", false); + vs.findValue("getBite()", void.class, false); + assertEquals(0, testAppender.logEvents.size()); } finally { testAppender.stop(); @@ -966,7 +995,23 @@ public class OgnlValueStackTest extends XWorkTestCase { assertEquals(12, vs.findValue("age", Integer.class, true)); assertEquals(12, vs.findValue("getAge()", true)); assertEquals(12, vs.findValue("getAge()", Integer.class, true)); + } + /** + * Fails on 2.5.20 and earlier - tested on 2.5 (5/5/2016) and failed + * @since 2.5.21 + */ + public void testNotSkipUserReturnedNullValues() { + OgnlValueStack vs = createValueStack(); + + Dog dog = new Dog(); + dog.setName("Rover"); + vs.push(dog); + + Cat cat = new Cat(); + vs.push(cat); + + // should not skip returned null values from cat.name assertNull(vs.findValue("name", true)); assertNull(vs.findValue("name", String.class, true)); assertNull(vs.findValue("getName()", true));