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 054e81a575..6bfdb31c97 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,7 @@ public void setValue(String expr, Object value, boolean throwExceptionOnFailure) 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 || logMissingProperties ? Boolean.TRUE : Boolean.FALSE); ognlUtil.setValue(expr, context, root, value); } @@ -246,7 +246,7 @@ public Object findValue(String expr, boolean throwExceptionOnFailure) { } protected void setupExceptionOnFailure(boolean throwExceptionOnFailure) { - if (throwExceptionOnFailure) { + if (throwExceptionOnFailure || logMissingProperties) { context.put(THROW_EXCEPTION_ON_FAILURE, true); } } @@ -339,8 +339,9 @@ protected Object handleOgnlException(String expr, boolean throwExceptionOnFailur } protected boolean shouldLogMissingPropertyWarning(OgnlException e) { - return (e instanceof NoSuchPropertyException || e instanceof MethodFailedException) - && devMode && logMissingProperties; + return (e instanceof NoSuchPropertyException || + (e instanceof MethodFailedException && e.getReason() instanceof NoSuchMethodException)) + && logMissingProperties; } private Object tryFindValue(String expr, Class asType) throws OgnlException { 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 e81511ee13..9a0bd5d1ee 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,13 +217,14 @@ public Object callMethod(Map context, Object target, String name, Object[] objec return null; } + 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; @@ -233,24 +234,27 @@ public Object callMethod(Map context, Object target, String name, Object[] objec if ((argTypes == null) || !invalidMethods.containsKey(mc)) { try { - Object value = OgnlRuntime.callMethod((OgnlContext) context, o, name, objects); + return OgnlRuntime.callMethod((OgnlContext) context, o, name, objects); + } catch (OgnlException e) { + reason = e.getReason(); - if (value != null) { - return value; + if (reason != null && !(reason instanceof NoSuchMethodException)) { + // method has found but thrown an exception + break; } - } catch (OgnlException e) { - // try the next one - Throwable reason = e.getReason(); - if (!context.containsKey(OgnlValueStack.THROW_EXCEPTION_ON_FAILURE) && (mc != null) && (reason != null) && (reason.getClass() == NoSuchMethodException.class)) { + if ((mc != null) && (reason != null)) { invalidMethods.put(mc, Boolean.TRUE); - } else if (reason != null) { - throw new MethodFailedException(o, name, e.getReason()); } + // continue and try the next one } } } + 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 b04f38f2dd..72a84e9dd9 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 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,95 @@ public void testFailOnMissingProperty() { } } + /** + * 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("" + logMissingProperties); + + 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); + + 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"); + + 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); + + 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(); + logger.removeAppender(testAppender); + } + } + public void testFailOnMissingMethod() { OgnlValueStack vs = createValueStack(); @@ -879,6 +975,49 @@ public void testStatics() { 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)); + } + + /** + * 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)); + assertNull(vs.findValue("getName()", String.class, true)); + } + public void testTop() { OgnlValueStack vs = createValueStack(); @@ -1293,6 +1432,19 @@ public void setDisplayName(String displayName) { 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 {