Repository: OpenRefine/OpenRefine (java)

Issue:
Implement `forEachItem` for JSON object
Users have ongoing complaints about the difficult in parsing  JSON which has arbitrary structure, most often in the form of JSON API responses which were designed to return objects keyed by arbitrary identifiers rather than just returning array as the vast more of APIs do.

Examples provided in #2186 include:
* Wikimedia Commons -> https://commons.wikimedia.org/w/api.php?action=query&titles=File:Google%202015%20logo.svg&prop=imageinfo&iiprop=url&format=json
* MySociety MapIt -> https://mapit.mysociety.org/point/4326/0.1293497,51.5464828

And the issue has been discussed both on the OpenRefine mailing list and StackOverflow:
* https://groups.google.com/d/msg/openrefine/O72bg8GZkj8/vBoEzmGxBwAJ
* https://stackoverflow.com/questions/10782737/google-refine-iterate-over-a-json-dictionary

### Proposed solution

Similar to the `forEachIndex` function that we have for arrays, we would implement a `forEachItem` iterator control which would accept a JSON node object, two variable names (one for key and one for value), and a GREL expression. It would iterate over each item in the JSON object, assigning the string value of the key to the key variable and the value for the item to the value variable and then execute the GREL expression.

The control would **only** accept a JSON object to iterate over and would explicitly **not** support iterating over objects that implement the `HasFields` interface.

### Alternatives considered
* extend `forEach()` to iterate over JSON object items. This wouldn't provide access to the field name / key, but would probably satisfy the vast majority of cases since the key is usually included in the child object as an id.
* implement iteration for every object that implements HasFields. Since OpenRefine `HasFields` objects typically have a small fixed set of fields, this seems to have limit value.


Pull request:
Add support for JSON Objects to forEachIndex() control
Fixes #3147

I'm on the fence as to whether this should be a separate control (name TBD), as I originally spec'd, or if rolling the functionality into the existing `forEachIndex` is close enough semantically given that the array index and the object key are both performing essentially the same function. This pull request adopts the latter approach. Doing it the other way would basically involve cloning the code, giving it a new function name, and making the error messages more specific.

Changes proposed in this pull request:
- add tests for `forEachIndex`
- enhance error checking to reject using the same variable name for both index and value
- add support for JSON objects withe same structure, but the object key instead of the array index returned in the index/key variable


Patch:
diff --git a/main/resources/com/google/refine/grel/ControlEvalError.properties b/main/resources/com/google/refine/grel/ControlEvalError.properties
index 9cb0e415e..d2a138e61 100644
--- a/main/resources/com/google/refine/grel/ControlEvalError.properties
+++ b/main/resources/com/google/refine/grel/ControlEvalError.properties
@@ -1,5 +1,5 @@
 filter=First argument is not an array
-foreach_index=First argument to forEach is not an array
+foreach_index=First argument to forEachIndex is not an array or JSON object
 foreach=First argument to forEach is not an array or JSON object
 for_range=First, second, and third arguments of forRange must all be numbers
 
@@ -12,3 +12,4 @@ expects_five_args={0} expects five arguments
 expects_second_arg_var_name={0} expects second argument to be a variable name
 expects_second_arg_index_var_name={0} expects second argument to be the index's variable name
 expects_third_arg_element_var_name={0} expects second argument to be the element's variable name
+expects_second_third_args_different={0} expects the second and third arguments to be different variable names
diff --git a/main/src/com/google/refine/grel/ast/VariableExpr.java b/main/src/com/google/refine/grel/ast/VariableExpr.java
index 4ed7a3370..3188a5dc2 100644
--- a/main/src/com/google/refine/grel/ast/VariableExpr.java
+++ b/main/src/com/google/refine/grel/ast/VariableExpr.java
@@ -45,7 +45,11 @@ public class VariableExpr implements Evaluable {
     final protected String _name;
 
     public VariableExpr(String name) {
-        _name = name;
+        if (name != null || !name.isEmpty()) {
+            _name = name;
+        } else {
+            throw new IllegalArgumentException("Illegal variable name ");
+        }
     }
 
     @Override
@@ -61,4 +65,14 @@ public class VariableExpr implements Evaluable {
     public String getName() {
         return _name;
     }
+
+    @Override
+    public boolean equals(Object other) {
+        return other instanceof VariableExpr && getName().equals(((VariableExpr) other).getName());
+    }
+
+    @Override
+    public int hashCode() {
+        return _name.hashCode();
+    }
 }
diff --git a/main/src/com/google/refine/grel/controls/ForEachIndex.java b/main/src/com/google/refine/grel/controls/ForEachIndex.java
index 53cf9ca18..367781026 100644
--- a/main/src/com/google/refine/grel/controls/ForEachIndex.java
+++ b/main/src/com/google/refine/grel/controls/ForEachIndex.java
@@ -34,10 +34,16 @@ OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 package com.google.refine.grel.controls;
 
 import java.util.ArrayList;
+import java.util.Iterator;
 import java.util.List;
+import java.util.Map;
 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;
 import com.google.refine.expr.util.JsonValueConverter;
@@ -59,6 +65,8 @@ public class ForEachIndex implements Control {
         } else if (!(args[2] instanceof VariableExpr)) {
             // variable name";
             return ControlEvalError.expects_third_arg_element_var_name(ControlFunctionRegistry.getControlName(this));
+        } else if (args[1].equals(args[2])) {
+            return ControlEvalError.expects_second_third_args_different(ControlFunctionRegistry.getControlName(this));
         }
         return null;
     }
@@ -68,8 +76,8 @@ public class ForEachIndex implements Control {
         Object o = args[0].evaluate(bindings);
         if (ExpressionUtils.isError(o)) {
             return o;
-        } else if (!ExpressionUtils.isArrayOrCollection(o) && !(o instanceof ArrayNode)) {
-            return ControlEvalError.foreach_index();
+        } else if (!ExpressionUtils.isArrayOrCollection(o) && !(o instanceof ArrayNode) && !(o instanceof ObjectNode)) {
+            return new EvalError(ControlEvalError.foreach_index());
         }
 
         String indexName = ((VariableExpr) args[1]).getName();
@@ -108,6 +116,21 @@ public class ForEachIndex implements Control {
 
                     Object r = args[3].evaluate(bindings);
 
+                    results.add(r);
+                }
+            } else if (o instanceof ObjectNode) {
+                ObjectNode obj = (ObjectNode) o;
+                results = new ArrayList<>(obj.size());
+                for (Iterator<Map.Entry<String, JsonNode>> it = obj.fields(); it.hasNext();) {
+                    Map.Entry<String, JsonNode> entry = it.next();
+                    if (entry != null) {
+                        bindings.put(indexName, entry.getKey());
+                        bindings.put(elementName, entry.getValue());
+                    } else {
+                        bindings.remove(indexName);
+                        bindings.remove(elementName);
+                    }
+                    Object r = args[3].evaluate(bindings);
                     results.add(r);
                 }
             } else {


Test patch:
diff --git a/main/tests/server/src/com/google/refine/grel/controls/ForEachIndexTests.java b/main/tests/server/src/com/google/refine/grel/controls/ForEachIndexTests.java
index d2d353cbf..6f25f3ee9 100644
--- a/main/tests/server/src/com/google/refine/grel/controls/ForEachIndexTests.java
+++ b/main/tests/server/src/com/google/refine/grel/controls/ForEachIndexTests.java
@@ -1,5 +1,5 @@
 /*******************************************************************************
- * Copyright (C) 2018, OpenRefine contributors
+ * Copyright (C) 2018, 2023, OpenRefine contributors
  * All rights reserved.
  * 
  * Redistribution and use in source and binary forms, with or without
@@ -27,15 +27,90 @@
 
 package com.google.refine.grel.controls;
 
+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.MetaParser;
+import com.google.refine.expr.ParsingException;
 import com.google.refine.util.TestUtils;
 
-public class ForEachIndexTests {
+public class ForEachIndexTests extends RefineTest {
 
     @Test
     public void serializeForEachIndex() {
         String json = "{\"description\":\"Evaluates expression a to an array. Then for each array element, binds its index to variable i and its value to variable name v, evaluates expression e, and pushes the result onto the result array.\",\"params\":\"expression a, variable i, variable v, expression e\",\"returns\":\"array\"}";
         TestUtils.isSerializedTo(new ForEachIndex(), json);
     }
+
+    private void assertParseException(String expression) {
+        assertParseException(expression, null);
+    }
+
+    private void assertParseException(String expression, String message) {
+        try {
+            MetaParser.parse("grel:" + expression);
+            if (message == null) {
+                fail("Expression didn't return error : " + expression);
+            } else {
+                fail(message);
+            }
+        } catch (ParsingException e) {
+            // success
+        }
+    }
+
+    @Test
+    public void testInvalidParams() throws ParsingException {
+        bindings = new Properties();
+        bindings.put("v", "");
+        assertParseException("forEachIndex('test', v, v)");
+        assertParseException("forEachIndex([], 1, 1, 1)", "Didn't throw a ParsingException for wrong argument type");
+        assertParseException("forEachIndex([], v, v, v)", "Didn't throw ParsingException for duplicate variables");
+        assertParseException("forEachIndex([], v, v)", "Didn't throw a ParsingException for 3 arguments");
+        assertParseException("forEachIndex([], v)", "Didn't throw a ParsingException for 2 arguments");
+        assertParseException("forEachIndex([])", "Didn't throw a ParsingException for 1 argument");
+    }
+
+    @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 = { "forEachIndex('" + json + "'.parseJson(), k, v, v.id).sort().join(',')", "2247,2511,8706" };
+        bindings = new Properties();
+        bindings.put("k", "");
+        bindings.put("v", "");
+        parseEval(bindings, test);
+
+        String[] test2 = { "forEachIndex('" + json + "'.parseJson(), k, v, k).sort().join(',')", "2247,2511,8706" };
+        parseEval(bindings, test2);
+        String[] test3 = { "forEachIndex('" + json + "'.parseJson(), k, v, k.toNumber()==v.id).join(',')", "true,true,true" };
+        parseEval(bindings, test3);
+    }
+
+    @Test
+    public void testForEachIndexArray() throws ParsingException {
+        bindings = new Properties();
+        bindings.put("k", "");
+        bindings.put("v", "");
+        parseEval(bindings, new String[] { "forEachIndex([5,4,3,2.0], k, v, v*2).join(',')", "10,8,6,4.0" });
+        parseEval(bindings, new String[] { "forEachIndex([5,4,3,2.0], k, v, k).join(',')", "0,1,2,3" });
+    }
+
+    @Test
+    public void testForEachIndexJsonArray() throws ParsingException {
+        bindings = new Properties();
+        bindings.put("k", "");
+        bindings.put("v", "");
+        parseEval(bindings, new String[] { "forEachIndex('[3,2,1.0]'.parseJson(), k, v, v*2).join(',')", "6,4,2.0" });
+        parseEval(bindings, new String[] { "forEachIndex('[3,2,1.0]'.parseJson(), k, v, k).join(',')", "0,1,2" });
+    }
+
 }