From 3ac6835c5ca71cc5ee085680ade48aac6883d2fa Mon Sep 17 00:00:00 2001 From: Yasser Zamani Date: Thu, 30 May 2019 16:36:54 +0430 Subject: [PATCH] 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 {