Repository: OpenRefine/OpenRefine (java)

Issue:
Add Column by Fetching URL silently fails on malformed URLs
If an invalid URL is used for the Add Column by Fetching URLs operation, the operation silently results in a null value instead of an error, making it much more difficult for the user to figure out what the problem is. This was recently on the forum as a cause of confusion for a user: https://forum.openrefine.org/t/fetching-urls-with-arabic-values/1036/1

This behavior was checked by a test, so I coded the new HTTP support  to mimic it, but I think the test is wrong and we should fix both it and the code so that URL/URI errors get reported to the user in an intelligible fashion.


### To Reproduce
Steps to reproduce the behavior:
1. Create a project with a cell containing the text: https://transliterate.qcri.org/ar2en/احمد احمد
2. Select Add a Column by Fetching URLs
3. For "On error:" select "store error"


### Current Results
The operation silently fails with no error stored in the cell.

### Expected Behavior
The cell value should be set to an `EvalError` with the appropriate error code/message.

### Versions<!--  (please complete the following information)-->

 - OpenRefine: 3.8-snapshot (ie current HEAD)


Pull request:
Return error, not null, on bad URL. Fixes #6137.
Fixes #6137 

Changes proposed in this pull request:
- Instead of swallowing URL/URI syntax errors and returning `null` to the user, return an `EvalError` object with the message from the exception
- change the test behavior to match



Patch:
diff --git a/main/src/com/google/refine/util/HttpClient.java b/main/src/com/google/refine/util/HttpClient.java
index 6cdada46f..4ae8bfb71 100644
--- a/main/src/com/google/refine/util/HttpClient.java
+++ b/main/src/com/google/refine/util/HttpClient.java
@@ -28,9 +28,6 @@
 package com.google.refine.util;
 
 import java.io.IOException;
-import java.net.MalformedURLException;
-import java.net.URISyntaxException;
-import java.net.URL;
 import java.nio.charset.StandardCharsets;
 import java.util.Collections;
 import java.util.List;
@@ -244,20 +241,13 @@ public class HttpClient {
     }
 
     public String getResponse(String urlString, Header[] headers, HttpClientResponseHandler<String> responseHandler) throws IOException {
-        try {
-            // Use of URL constructor below is purely to get additional error checking to mimic
-            // previous behavior for the tests.
-            new URL(urlString).toURI();
-        } catch (IllegalArgumentException | MalformedURLException | URISyntaxException e) {
-            return null;
-        }
 
         HttpGet httpGet = new HttpGet(urlString);
 
         if (headers != null && headers.length > 0) {
             httpGet.setHeaders(headers);
         }
-        httpGet.setConfig(defaultRequestConfig); // FIXME: Redundant? already includes in client builder
+        httpGet.setConfig(defaultRequestConfig); // FIXME: Redundant? already included in client builder
         return httpClient.execute(httpGet, responseHandler);
     }
 
diff --git a/main/webapp/modules/core/scripts/util/dialog.js b/main/webapp/modules/core/scripts/util/dialog.js
index dca9275e4..db9dbcc9d 100644
--- a/main/webapp/modules/core/scripts/util/dialog.js
+++ b/main/webapp/modules/core/scripts/util/dialog.js
@@ -83,7 +83,7 @@ DialogSystem.showDialog = function(elmt, onCancel) {
   }
 
   elmt.attr("tabindex", -1);
-  elmt.focus();
+  elmt.trigger('focus');
 
   return level;
 };


Test patch:
diff --git a/main/tests/server/src/com/google/refine/operations/column/ColumnAdditionByFetchingURLsOperationTests.java b/main/tests/server/src/com/google/refine/operations/column/ColumnAdditionByFetchingURLsOperationTests.java
index 095dca2cc..9a148037c 100644
--- a/main/tests/server/src/com/google/refine/operations/column/ColumnAdditionByFetchingURLsOperationTests.java
+++ b/main/tests/server/src/com/google/refine/operations/column/ColumnAdditionByFetchingURLsOperationTests.java
@@ -51,6 +51,7 @@ import org.testng.annotations.Test;
 
 import com.google.refine.RefineTest;
 import com.google.refine.browsing.EngineConfig;
+import com.google.refine.expr.EvalError;
 import com.google.refine.expr.ExpressionUtils;
 import com.google.refine.model.AbstractOperation;
 import com.google.refine.model.Cell;
@@ -193,7 +194,7 @@ public class ColumnAdditionByFetchingURLsOperationTests extends RefineTest {
             server.enqueue(new MockResponse());
 
             Row row0 = new Row(2);
-            row0.setCell(0, new Cell("auinrestrsc", null)); // malformed -> null
+            row0.setCell(0, new Cell("auinrestrsc", null)); // malformed -> error
             project.rows.add(row0);
             Row row1 = new Row(2);
             row1.setCell(0, new Cell(url.toString(), null)); // fine
@@ -217,8 +218,8 @@ public class ColumnAdditionByFetchingURLsOperationTests extends RefineTest {
 
             int newCol = project.columnModel.getColumnByName("junk").getCellIndex();
             // Inspect rows
-            Assert.assertEquals(project.rows.get(0).getCellValue(newCol), null);
-            Assert.assertTrue(project.rows.get(1).getCellValue(newCol) != null);
+            Assert.assertTrue(project.rows.get(0).getCellValue(newCol) instanceof EvalError);
+            Assert.assertNotNull(project.rows.get(1).getCellValue(newCol));
             Assert.assertTrue(ExpressionUtils.isError(project.rows.get(2).getCellValue(newCol)));
         }
     }