SLING-8860 - Issue a warning when data-sly-test is passed a constant …

…value for evaluation
apache · Dec 2, 2019 · a972a36 · a972a36
1 parent 3cd4b99
commit a972a36
Show file tree

Hide file tree

Showing 2 changed files with 40 additions and 2 deletions.
diff --git a/src/main/java/org/apache/sling/scripting/sightly/impl/plugin/TestPlugin.java b/src/main/java/org/apache/sling/scripting/sightly/impl/plugin/TestPlugin.java
@@ -20,6 +20,14 @@
 
 import org.apache.sling.scripting.sightly.compiler.commands.Conditional;
 import org.apache.sling.scripting.sightly.compiler.commands.VariableBinding;
+import org.apache.sling.scripting.sightly.compiler.expression.ExpressionNode;
+import org.apache.sling.scripting.sightly.compiler.expression.nodes.ArrayLiteral;
+import org.apache.sling.scripting.sightly.compiler.expression.nodes.Atom;
+import org.apache.sling.scripting.sightly.compiler.expression.nodes.BinaryOperation;
+import org.apache.sling.scripting.sightly.compiler.expression.nodes.BinaryOperator;
+import org.apache.sling.scripting.sightly.compiler.expression.nodes.Identifier;
+import org.apache.sling.scripting.sightly.compiler.expression.nodes.MapLiteral;
+import org.apache.sling.scripting.sightly.compiler.expression.nodes.NullLiteral;
 import org.apache.sling.scripting.sightly.impl.compiler.PushStream;
 import org.apache.sling.scripting.sightly.compiler.expression.Expression;
 import org.apache.sling.scripting.sightly.impl.compiler.frontend.CompilerContext;
@@ -44,14 +52,27 @@ public PluginInvoke invoke(final Expression expressionNode, final PluginCallInfo
             @Override
             public void beforeElement(PushStream stream, String tagName) {
                 String variableName = decodeVariableName(callInfo);
+                ExpressionNode root = expressionNode.getRoot();
+                boolean constantValueComparison =
+                        root instanceof Atom && !(root instanceof Identifier) ||
+                        root instanceof NullLiteral ||
+                        root instanceof ArrayLiteral ||
+                        root instanceof MapLiteral;
+                if (!constantValueComparison && root instanceof BinaryOperation) {
+                    constantValueComparison = ((BinaryOperation) root).getOperator() == BinaryOperator.CONCATENATE;
+                }
+                if (constantValueComparison) {
+                    stream.warn(new PushStream.StreamMessage("data-sly-test: redundant constant value comparison",
+                            expressionNode.getRawText()));
+                }
                 globalBinding = variableName != null;
                 if (variableName == null) {
                     variableName = compilerContext.generateVariable("testVariable");
                 }
                 if (globalBinding) {
-                    stream.write(new VariableBinding.Global(variableName, expressionNode.getRoot()));
+                    stream.write(new VariableBinding.Global(variableName, root));
                 } else {
-                    stream.write(new VariableBinding.Start(variableName, expressionNode.getRoot()));
+                    stream.write(new VariableBinding.Start(variableName, root));
                 }
                 stream.write(new Conditional.Start(variableName, true));
             }

diff --git a/src/test/java/org/apache/sling/scripting/sightly/impl/compiler/SightlyCompilerTest.java b/src/test/java/org/apache/sling/scripting/sightly/impl/compiler/SightlyCompilerTest.java
@@ -23,6 +23,7 @@
 import java.io.Reader;
 import java.io.StringReader;
 import java.util.List;
+import java.util.function.Supplier;
 
 import org.apache.sling.scripting.sightly.compiler.CompilationResult;
 import org.apache.sling.scripting.sightly.compiler.CompilationUnit;
@@ -160,6 +161,22 @@ public void testUnknownExpressionOptions() {
         assertEquals(1, compileSource("<div data-sly-text='${\"text\" @ i18nn}'>Replaced</div>").getWarnings().size());
     }
 
+    @Test
+    public void testRedundantDataSlyTest() {
+        assertEquals("data-sly-test with boolean constant should have raised a warning.", 1,
+                compileSource("<span data-sly-test=\"${true}\">if true</span>").getWarnings().size());
+        assertEquals("data-sly-test with number constant should have raised a warning.", 1,
+                compileSource("<span data-sly-test=\"${0}\">if true</span>").getWarnings().size());
+        assertEquals("data-sly-test with string constant should have raised a warning.", 1,
+                compileSource("<span data-sly-test=\"${'a'}\">if true</span>").getWarnings().size());
+        assertEquals("data-sly-test with null literal should have raised a warning.", 1,
+                compileSource("<span data-sly-test=\"${}\">if true</span>").getWarnings().size());
+        assertEquals("data-sly-test with array literal should have raised a warning.", 1,
+                compileSource("<span data-sly-test=\"${[1, 2, 3]}\">if true</span>").getWarnings().size());
+        assertEquals("data-sly-test with string concatenation should have raised a warning.", 1,
+                compileSource("<span data-sly-test=\"${properties}}\">if true</span>").getWarnings().size());
+    }
+
     private CompilationResult compileFile(final String file) {
         InputStream stream = this.getClass().getResourceAsStream(file);
         final Reader reader = new InputStreamReader(stream);