Extend forEach() to support JSON objects (#3150)

* Refactor GREL Get tests

- move helper up to RefineTest
- move tests to the correct module

* Extend forEach() to support JSON objects - fixes #3149

Also add tests for existing forEach forms in addition to the new one

* Add a couple more tests

main/src/com/google/refine/grel/controls/ForEach.java

@@ -38,7 +38,9 @@ import java.util.Collection;
 import java.util.List;
 import java.util.Properties;
 
+import com.fasterxml.jackson.databind.JsonNode;
 import com.fasterxml.jackson.databind.node.ArrayNode;
+import com.fasterxml.jackson.databind.node.ObjectNode;
 import com.google.refine.expr.EvalError;
 import com.google.refine.expr.Evaluable;
 import com.google.refine.expr.ExpressionUtils;
@@ -53,7 +55,7 @@ public class ForEach implements Control {
         if (args.length != 3) {
             return ControlFunctionRegistry.getControlName(this) + " expects 3 arguments";
         } else if (!(args[1] instanceof VariableExpr)) {
-            return ControlFunctionRegistry.getControlName(this) + 
+            return ControlFunctionRegistry.getControlName(this) +
                 " expects second argument to be a variable name";
         }
         return null;
@@ -64,8 +66,9 @@ public class ForEach implements Control {
         Object o = args[0].evaluate(bindings);
         if (ExpressionUtils.isError(o)) {
             return o;
-        } else if (!ExpressionUtils.isArrayOrCollection(o) && !(o instanceof ArrayNode)) {
+        } else if (!ExpressionUtils.isArrayOrCollection(o) && !(o instanceof ArrayNode)
-            return new EvalError("First argument to forEach is not an array");
+                && !(o instanceof ObjectNode)) {
+            return new EvalError("First argument to forEach is not an array or JSON object");
         }
         
         String name = ((VariableExpr) args[1]).getName();
@@ -105,6 +108,18 @@ public class ForEach implements Control {
                     
                     Object r = args[2].evaluate(bindings);
                     
+                    results.add(r);
+                }
+            } else if (o instanceof ObjectNode) {
+                ObjectNode obj = (ObjectNode) o;
+                results = new ArrayList<Object>(obj.size());
+                for (JsonNode v : obj) {
+                    if (v != null) {
+                        bindings.put(name, v);
+                    } else {
+                        bindings.remove(name);
+                    }
+                    Object r = args[2].evaluate(bindings);
                     results.add(r);
                 }
             } else {

main/tests/server/src/com/google/refine/RefineTest.java

@@ -58,6 +58,9 @@ import com.fasterxml.jackson.databind.node.BooleanNode;
 import com.fasterxml.jackson.databind.node.IntNode;
 import com.fasterxml.jackson.databind.node.ObjectNode;
 import com.fasterxml.jackson.databind.node.TextNode;
+import com.google.refine.expr.Evaluable;
+import com.google.refine.expr.MetaParser;
+import com.google.refine.expr.ParsingException;
 import com.google.refine.grel.ControlFunctionRegistry;
 import com.google.refine.grel.Function;
 import com.google.refine.importers.SeparatorBasedImporter;
@@ -332,6 +335,22 @@ public class RefineTest extends PowerMockTestCase {
         }
     }
 
+
+    /**
+     * Parse and evaluate a GREL expression and compare the result to the expect value
+     *
+     * @param bindings
+     * @param test
+     * @throws ParsingException
+     */
+    protected void parseEval(Properties bindings, String[] test)
+    throws ParsingException {
+        Evaluable eval = MetaParser.parse("grel:" + test[0]);
+        Object result = eval.evaluate(bindings);
+        Assert.assertEquals(result.toString(), test[1], "Wrong result for expression: " + test[0]);
+    }
+
+
     @AfterMethod
     public void TearDown() throws Exception {
         bindings = null;

main/tests/server/src/com/google/refine/expr/functions/GetTests.java

@@ -26,15 +26,64 @@
  ******************************************************************************/
 package com.google.refine.expr.functions;
 
+import java.util.Properties;
+
+import org.slf4j.LoggerFactory;
+import org.testng.Assert;
+import org.testng.annotations.AfterMethod;
+import org.testng.annotations.BeforeMethod;
+import org.testng.annotations.BeforeTest;
 import org.testng.annotations.Test;
 
+import com.google.refine.RefineTest;
+import com.google.refine.expr.Evaluable;
+import com.google.refine.expr.ExpressionUtils;
+import com.google.refine.expr.MetaParser;
+import com.google.refine.expr.ParsingException;
+import com.google.refine.model.Project;
 import com.google.refine.util.TestUtils;
 
-public class GetTests {
+public class GetTests extends RefineTest {
+
+    Project project;
+    Properties bindings;
+
+    @Override
+    @BeforeTest
+    public void init() {
+        logger = LoggerFactory.getLogger(this.getClass());
+    }
+
+    @BeforeMethod
+    public void SetUp() {
+        project = new Project();
+        bindings = ExpressionUtils.createBindings(project);
+    }
+
+    @AfterMethod
+    public void TearDown() {
+        project = null;
+        bindings = null;
+    }
+
     @Test
     public void serializeGet() {
         String json = "{\"description\":\"If o has fields, returns the field named 'from' of o. If o is an array, returns o[from, to]. if o is a string, returns o.substring(from, to)\",\"params\":\"o, number or string from, optional number to\",\"returns\":\"Depends on actual arguments\"}";
         TestUtils.isSerializedTo(new Get(), json);
     }
+
+    @Test
+    public void testGetJsonFieldExists() throws ParsingException {
+        String test[] = { "\"[{\\\"one\\\": \\\"1\\\"}]\".parseJson()[0].one", "1" };
+        parseEval(bindings, test);
+    }
+
+    @Test
+    public void testGetJsonFieldAbsent() throws ParsingException {
+        String test =  "\"[{\\\"one\\\": \\\"1\\\"}]\".parseJson()[0].two";
+        Evaluable eval = MetaParser.parse("grel:" + test);
+        Assert.assertNull(eval.evaluate(bindings));
+    }
+
 }
 

main/tests/server/src/com/google/refine/grel/GrelTests.java

@@ -167,19 +167,6 @@ public class GrelTests extends RefineTest {
         }
     }
 
-    @Test
-    public void testGetJsonFieldExists() throws ParsingException {
-        String test[] = { "\"[{\\\"one\\\": \\\"1\\\"}]\".parseJson()[0].one", "1" };
-        parseEval(bindings, test);
-    }
-
-    @Test
-    public void testGetJsonFieldAbsent() throws ParsingException {
-        String test =  "\"[{\\\"one\\\": \\\"1\\\"}]\".parseJson()[0].two";
-        Evaluable eval = MetaParser.parse("grel:" + test);
-        Assert.assertNull(eval.evaluate(bindings));
-    }
-    
     @Test
     public void testJoinJsonArray() throws ParsingException {
         String test[] = { "\"{\\\"values\\\":[\\\"one\\\",\\\"two\\\",\\\"three\\\"]}\".parseJson().values.join(\",\")", "one,two,three" };
@@ -206,13 +193,5 @@ public class GrelTests extends RefineTest {
                 Assert.fail("Unexpected parse failure for cross function: " + test);                
             }
     }
-    
-    private void parseEval(Properties bindings, String[] test)
-    throws ParsingException {
-        Evaluable eval = MetaParser.parse("grel:" + test[0]);
-        Object result = eval.evaluate(bindings);
-        Assert.assertEquals(result.toString(), test[1], 
-                            "Wrong result for expression: "+test[0]);
-    }
 
 }

main/tests/server/src/com/google/refine/grel/controls/ForEachTests.java

@@ -26,15 +26,80 @@
  ******************************************************************************/
 package com.google.refine.grel.controls;
 
+import static org.testng.Assert.assertTrue;
+import static org.testng.Assert.fail;
+
+import java.util.Properties;
+
 import org.testng.annotations.Test;
 
+import com.google.refine.RefineTest;
+import com.google.refine.expr.EvalError;
+import com.google.refine.expr.Evaluable;
+import com.google.refine.expr.MetaParser;
+import com.google.refine.expr.ParsingException;
 import com.google.refine.util.TestUtils;
 
-public class ForEachTests {
+public class ForEachTests extends RefineTest {
     @Test
     public void serializeForEach() {
         String json = "{\"description\":\"Evaluates expression a to an array. Then for each array element, binds its value to variable name v, evaluates expression e, and pushes the result onto the result array.\",\"params\":\"expression a, variable v, expression e\",\"returns\":\"array\"}";
         TestUtils.isSerializedTo(new ForEach(), json);
     }
+
+    private void assertParseError(String expression) throws ParsingException {
+        Evaluable eval = MetaParser.parse("grel:" + expression);
+        Object result = eval.evaluate(bindings);
+        assertTrue(result instanceof EvalError, "Expression didn't return error : " + expression);
+    }
+
+    @Test
+    public void testInvalidParams() throws ParsingException {
+        bindings = new Properties();
+        bindings.put("v", "");
+        assertParseError("forEach('test', v, v)");
+        try {
+            assertParseError("forEach([], 1, 1)");
+            fail("Didn't throw a ParsingException for wrong argument type");
+        } catch (ParsingException e) {}
+        try {
+            assertParseError("forEach([], v)");
+            fail("Didn't throw a ParsingException for 2 arguments");
+        } catch (ParsingException e) {}
+        try {
+            assertParseError("forEach([])");
+            fail("Didn't throw a ParsingException for 1 argument");
+        } catch (ParsingException e) {}
+    }
+
+    @Test
+    public void testForEachJsonObject() throws ParsingException {
+        String json = "{"
+                + "\"2511\": {\"parent_area\": null, \"generation_high\": 40, \"all_names\": {}, \"id\": 2511, \"codes\": {\"ons\": \"00AB\", \"gss\": \"E09000002\", \"local-authority-eng\": \"BDG\", \"local-authority-canonical\": \"BDG\", \"unit_id\": \"10949\"}, \"name\": \"Barking and Dagenham Borough Council\", \"country\": \"E\", \"type_name\": \"London borough\", \"generation_low\": 1, \"country_name\": \"England\", \"type\": \"LBO\"},"
+                + "\"2247\": {\"parent_area\": null, \"generation_high\": 40, \"all_names\": {}, \"id\": 2247, \"codes\": {\"unit_id\": \"41441\"}, \"name\": \"Greater London Authority\", \"country\": \"E\", \"type_name\": \"Greater London Authority\", \"generation_low\": 1, \"country_name\": \"England\", \"type\": \"GLA\"},"
+                + "\"8706\": {\"parent_area\": 2511, \"generation_high\": 40, \"all_names\": {}, \"id\": 8706, \"codes\": {\"ons\": \"00ABGH\", \"gss\": \"E05000036\", \"unit_id\": \"10936\"}, \"name\": \"Mayesbrook\", \"country\": \"E\", \"type_name\": \"London borough ward\", \"generation_low\": 1, \"country_name\": \"England\", \"type\": \"LBW\"}"
+                + "}";
+
+        String test[] =  {"forEach('" + json + "'.parseJson(), v, v.id).sort().join(',')", "2247,2511,8706"};
+        bindings = new Properties();
+        bindings.put("v", "");
+        parseEval(bindings, test);
+    }
+
+    @Test
+    public void testForEachArray() throws ParsingException {
+        String test[] =  {"forEach([5,4,3,2.0], v, v*2).join(',')", "10,8,6,4.0"};
+        bindings = new Properties();
+        bindings.put("v", "");
+        parseEval(bindings, test);
+    }
+
+    @Test
+    public void testForEachJsonArray() throws ParsingException {
+        String test[] =  {"forEach('[3,2,1.0]'.parseJson(), v, v*2).join(',')", "6,4,2.0"};
+        bindings = new Properties();
+        bindings.put("v", "");
+        parseEval(bindings, test);
+    }
 }