[struts] 01/05: fix logMissingProperties (WW-4999)

[ya...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

yasserzamani pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/struts.git

commit b657a272d1d2ae93dc5fd0a1b7f81d6ae1772e89
Author: Yasser Zamani <ya...@apache.org>
AuthorDate: Thu May 30 16:36:54 2019 +0430

    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.
    
    (cherry picked from commit 3ac6835)
---
 .../opensymphony/xwork2/ognl/OgnlValueStack.java   |  5 ++-
 .../xwork2/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 6a0ef66..94ec450 100644
--- a/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlValueStack.java
+++ b/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlValueStack.java
@@ -181,7 +181,8 @@ public class OgnlValueStack implements Serializable, ValueStack, ClearableValueS
 
     private void trySetValue(String expr, Object value, boolean throwExceptionOnFailure, Map<String, Object> 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);
     }
 
@@ -245,7 +246,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 0403f61..2a4f5d2 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 b10cba7..617f876 100644
--- a/core/src/test/java/com/opensymphony/xwork2/ognl/OgnlValueStackTest.java
+++ b/core/src/test/java/com/opensymphony/xwork2/ognl/OgnlValueStackTest.java
@@ -33,9 +33,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;
 
 
@@ -239,6 +246,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();
 
@@ -1374,6 +1412,19 @@ public class OgnlValueStackTest extends XWorkTestCase {
             this.displayName = displayName;
         }
     }
+
+    class TestAppender extends AbstractAppender {
+        List<LogEvent> logEvents = new ArrayList<>();
+
+        TestAppender() {
+            super("TestAppender", null, null, false, null);
+        }
+
+        @Override
+        public void append(LogEvent logEvent) {
+            logEvents.add(logEvent);
+        }
+    }
 }
 
 enum MyNumbers {