WW-4891 Debug tag should not display anything when not in dev mode #201

HedjuHor · 2018-01-14T15:00:33Z

one solution
if a user really wants to show the Debug Flag on his production release, he can use <s:debug disabled="false"/>

got another solution 17b5984
this will solve the problem with client/server side control

coveralls · 2018-01-14T15:24:34Z

Coverage increased (+0.1%) to 46.511% when pulling 02b61b2 on HedjuHor:WW-4891 into 29b29a9 on apache:master.

lukaszlenart

Left few comments, looks good to me but have some security related concerns

lukaszlenart · 2018-01-15T11:41:28Z

core/src/main/java/org/apache/struts2/components/Debug.java

@@ -34,13 +34,12 @@
 import org.apache.struts2.StrutsException;

 @StrutsTag(name="debug", tldTagClass="org.apache.struts2.views.jsp.ui.DebugTag",
-        description="Prints debugging information")
+        description="Prints debugging information (Only if 'struts.devMode' enabled or disable tag is set 'false')")


I think this is a bit misleading, I would rephrase it:
(Only if 'struts.devMode' is enabled or a 'disabled' attribute is set 'false')

lukaszlenart · 2018-01-15T11:47:18Z

core/src/main/java/org/apache/struts2/components/Debug.java


-        return result;
+    protected boolean showDebug() {
+        return (devMode || "false".equalsIgnoreCase(disabled));


hm... the disabled attribute can be controlled by client ($(".selector").attr("disabled", "false")) but I do not see an option to do so on a page rendering on server side. Maybe some other pair of 👀 will catch this.

There is also a property named disabled in it's super class UIBean which is different than html disabled attribute.

It's exactly the same, there is no another disabled property in Debug tag.

$(".selector").attr("disabled", "false")

I meant your mentioned code above is at client side html and is not equal to that disabled java property at server side which this PR uses.

Yes but that property is settable via client -> <input disabled="false"/> or ?disabled=true - I'm not saying it's true in this case, that's why I asked for second pair of 👀

That's why asked for second pair of 👀

To be clear: disabled is a user settable property, that's the purpose of this property, a pure client side thing. Right now it was used to control server side logic and I wonder if this is a good idea.

Aha... yes I agree. However, debug.ftl does not use it currently.

@HedjuHor , I found all references of UIBean.disabled and saw it never used in java codes and always has been used inside ftl (free marker template) files. As Łukasz correctly warned, if you look it's usages inside ftl files, you'll see currently it's purpose is only to generate or not generate html disabled attribute. So I also think it may not a good idea to use it here, in not it's business ;)

@HedjuHor , @lukaszlenart , what do you feel using it inside debug.ftl instead as below:

Index: core/src/main/resources/template/simple/debug.ftl IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== --- core/src/main/resources/template/simple/debug.ftl (date 1515941681000) +++ core/src/main/resources/template/simple/debug.ftl (revision ) @@ -18,6 +18,8 @@ * under the License. */ --> +<#if parameters.disabled!false> +<#else > <script type="text/javascript"> <!-- function toggleDebug(debugId) { @@ -86,3 +88,4 @@ </#list> </table> </div> +</#if> \ No newline at end of file Index: core/src/main/java/org/apache/struts2/components/Debug.java IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== --- core/src/main/java/org/apache/struts2/components/Debug.java (date 1515941681000) +++ core/src/main/java/org/apache/struts2/components/Debug.java (revision ) @@ -41,6 +41,7 @@ protected ReflectionProvider reflectionProvider; + public Debug(ValueStack stack, HttpServletRequest request, HttpServletResponse response) { super(stack, request, response); } @@ -57,38 +58,31 @@ public boolean start(Writer writer) { boolean result = super.start(writer); - if (showDebug()) { - ValueStack stack = getStack(); - Iterator iter = stack.getRoot().iterator(); - List stackValues = new ArrayList(stack.getRoot().size()); - while (iter.hasNext()) { - Object o = iter.next(); - Map values; - try { - values = reflectionProvider.getBeanMap(o); - } catch (Exception e) { - throw new StrutsException("Caught an exception while getting the property values of " + o, e); - } - stackValues.add(new DebugMapEntry(o.getClass().getName(), values)); - } + ValueStack stack = getStack(); + Iterator iter = stack.getRoot().iterator(); + List stackValues = new ArrayList(stack.getRoot().size()); + while (iter.hasNext()) { + Object o = iter.next(); + Map values; + try { + values = reflectionProvider.getBeanMap(o); + } catch (Exception e) { + throw new StrutsException("Caught an exception while getting the property values of " + o, e); + } + stackValues.add(new DebugMapEntry(o.getClass().getName(), values)); + } - addParameter("stackValues", stackValues); - } + addParameter("stackValues", stackValues); + return result; } @Override public boolean end(Writer writer, String body) { - if (showDebug()) { - return super.end(writer, body); - } else { - popComponentStack(); - return false; - } - } - - protected boolean showDebug() { - return (devMode || "false".equalsIgnoreCase(disabled)); + if (!(devMode || "false".equalsIgnoreCase(disabled))) { + disabled = "true"; + } + return super.end(writer, body); } private static class DebugMapEntry implements Map.Entry {

I have been tested above patch on this PR and passes all tests :)

firstly, thank you to all
@lukaszlenart
...a pure client side thing. Right now it was used to control server side logic and I wonder if this is a good idea...
i see the problem, looking 4 a better solution

@yasserzamani

debug.ftl change looks nice.

Debug.java i would keep my version with the 'showDebug' methode for performance reason. If developer keep the DebugTag in the Jsp-files and defMode is disabled it always unnecessarily fetch the stackValues

👍 I tested following patch on your PR which passes all tests also:

Index: core/src/main/resources/template/simple/debug.ftl IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== --- core/src/main/resources/template/simple/debug.ftl (date 1515941681000) +++ core/src/main/resources/template/simple/debug.ftl (revision ) @@ -18,6 +18,8 @@ * under the License. */ --> +<#if parameters.disabled!false> +<#else > <script type="text/javascript"> <!-- function toggleDebug(debugId) { @@ -86,3 +88,4 @@ </#list> </table> </div> +</#if> \ No newline at end of file Index: core/src/main/java/org/apache/struts2/components/Debug.java IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== --- core/src/main/java/org/apache/struts2/components/Debug.java (date 1515941681000) +++ core/src/main/java/org/apache/struts2/components/Debug.java (revision ) @@ -41,6 +41,7 @@ protected ReflectionProvider reflectionProvider; + public Debug(ValueStack stack, HttpServletRequest request, HttpServletResponse response) { super(stack, request, response); } @@ -57,7 +58,11 @@ public boolean start(Writer writer) { boolean result = super.start(writer); - if (showDebug()) { + if (!(devMode || "false".equalsIgnoreCase(disabled))) { + disabled = "true"; + } + + if (!"true".equalsIgnoreCase(disabled)) { ValueStack stack = getStack(); Iterator iter = stack.getRoot().iterator(); List stackValues = new ArrayList(stack.getRoot().size()); @@ -74,23 +79,10 @@ addParameter("stackValues", stackValues); } + return result; } - @Override - public boolean end(Writer writer, String body) { - if (showDebug()) { - return super.end(writer, body); - } else { - popComponentStack(); - return false; - } - } - - protected boolean showDebug() { - return (devMode || "false".equalsIgnoreCase(disabled)); - } - private static class DebugMapEntry implements Map.Entry { private Object key; private Object value;

…r this action only

coveralls · 2018-01-19T00:21:18Z

Coverage increased (+0.2%) to 46.609% when pulling 17b5984 on HedjuHor:WW-4891 into 29b29a9 on apache:master.

lukaszlenart · 2018-01-19T08:08:40Z

core/src/main/java/org/apache/struts2/components/Debug.java


-        return result;
+    protected boolean showDebug() {
+        return (devMode || Boolean.TRUE == PrepareOperations.getDevModeOverride());


Ah! So you wanted to have an option to enable the <s:debug/> on production that's why you had used the disabled property. In such case getDevModeOverride is a better choice 👍

yes :-) , or more specifically for the Struts Showcase. 99 percent of users will not use it

lukaszlenart · 2018-01-19T08:12:21Z

I'm good with this PR 👍 if no objections please hit the button and I will prepare a new release :)

WW-4891 Debug tag should not display anything when not in dev mode

02b61b2

lukaszlenart requested changes Jan 16, 2018

View reviewed changes

WW-4891 PrepareOperations overrideDevMode for explicitly switch on fo…

17b5984

…r this action only

lukaszlenart reviewed Jan 19, 2018

View reviewed changes

lukaszlenart approved these changes Jan 19, 2018

View reviewed changes

yasserzamani merged commit 9beb940 into apache:master Jan 20, 2018

WW-4891 Debug tag should not display anything when not in dev mode #201

WW-4891 Debug tag should not display anything when not in dev mode #201

Uh oh!

Conversation

HedjuHor commented Jan 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Jan 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lukaszlenart left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HedjuHor Jan 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Jan 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HedjuHor Jan 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lukaszlenart commented Jan 19, 2018

Uh oh!

Uh oh!

HedjuHor commented Jan 14, 2018 •

edited

Loading

coveralls commented Jan 14, 2018 •

edited

Loading

HedjuHor Jan 17, 2018 •

edited

Loading

coveralls commented Jan 19, 2018 •

edited

Loading

HedjuHor Jan 19, 2018 •

edited

Loading