Repository: OpenRefine/OpenRefine (java)

Issue:
case-sensitive sort does not produce expected result
<!-- Describe the bug - Please add a clear and concise description of the bug above this line. You can delete this line if you want. It will be hidden in the final bug report -->

### To Reproduce
Steps to reproduce the behavior:

1. Create a project with the supplied "temp.csv"
![image](https://github.com/OpenRefine/OpenRefine/assets/42559726/119803ef-f1a1-4554-b72a-2717e26d84d3)

2. Configure a sort on the single column as follows
![image](https://github.com/OpenRefine/OpenRefine/assets/42559726/9f56aff4-45fa-4af0-a54b-e8178c424c01)

3. Apply the sort by "Reorder rows permanently"
![image](https://github.com/OpenRefine/OpenRefine/assets/42559726/8cee7a26-63e9-4630-aafc-c9cb7f02e4c6)


### Current Results
<!-- What results occurred or were shown. -->
Sort only moves the character "c"

### Expected Behavior
<!-- A clear and concise description of what you expected to happen or to show. -->
Here an example within Excel on what the sort should be showing
![image](https://github.com/OpenRefine/OpenRefine/assets/42559726/1182ec7c-0a00-435f-af72-26a0e61032dd)

### Versions<!--  (please complete the following information)-->
 - Operating System: Windows 10
 - Browser Version:  Chromium 116.0.5845.180
 - JRE or JDK Version: using the "openrefine-win-with-java-3.7.4.zip" release
 - OpenRefine: openrefine-win-with-java-3.7.4

### Datasets
[temp.csv](https://github.com/OpenRefine/OpenRefine/files/12572271/temp.csv)

### Additional context
<!-- Add any other context about the problem here. -->
Looking at the oldest version of JDK that OpenRefine supports: https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/text/Collator.html#TERTIARY

https://github.com/OpenRefine/OpenRefine/blob/2b07cfa5ced9bcfdc6d485fe1849af1c79ac320a/main/src/com/google/refine/sorting/StringCriterion.java#L57C50-L57C50

Shouldn't the Collator over here be set to "TERTIARY" or even "IDENTICAL"?
Checking the blame history shows the Collator initially set to the "IDENTICAL" strength.



Pull request:
Fix case sensitive string sort. Fixes #6047
Fixes #6047

Changes proposed in this pull request:
- Fix the case sensitive parameter being ignored for string sorts
- Correct parameter ordering for asserts in tests
- Add tests for reversed sort parameter, empty item position, and string sort


Patch:
diff --git a/extensions/database/src/com/google/refine/extension/database/DatabaseConfiguration.java b/extensions/database/src/com/google/refine/extension/database/DatabaseConfiguration.java
index c9b8976bd..4f4a14bef 100644
--- a/extensions/database/src/com/google/refine/extension/database/DatabaseConfiguration.java
+++ b/extensions/database/src/com/google/refine/extension/database/DatabaseConfiguration.java
@@ -29,6 +29,9 @@
 
 package com.google.refine.extension.database;
 
+import java.net.URI;
+import java.net.URISyntaxException;
+
 public class DatabaseConfiguration {
 
     private String connectionName;
@@ -123,4 +126,17 @@ public class DatabaseConfiguration {
                 + databaseSchema + ", useSSL=" + useSSL + "]";
     }
 
+    public URI toURI() {
+        try {
+            return new URI(
+                    "jdbc:" + databaseType.toLowerCase(),
+                    databaseHost + ((databasePort == 0) ? "" : (":" + databasePort)),
+                    "/" + databaseName,
+                    useSSL ? "useSSL=true" : null,
+                    null
+            );
+        } catch (URISyntaxException e) {
+            throw new IllegalArgumentException(e);
+        }
+    }
 }
diff --git a/extensions/database/src/com/google/refine/extension/database/mariadb/MariaDBConnectionManager.java b/extensions/database/src/com/google/refine/extension/database/mariadb/MariaDBConnectionManager.java
index 422735d1c..93d63c00e 100644
--- a/extensions/database/src/com/google/refine/extension/database/mariadb/MariaDBConnectionManager.java
+++ b/extensions/database/src/com/google/refine/extension/database/mariadb/MariaDBConnectionManager.java
@@ -134,7 +134,7 @@ public class MariaDBConnectionManager {
 
             Class.forName(type.getClassPath());
             DriverManager.setLoginTimeout(10);
-            String dbURL = getDatabaseUrl(databaseConfiguration);
+            String dbURL = databaseConfiguration.toURI().toString();
             connection = DriverManager.getConnection(dbURL, databaseConfiguration.getDatabaseUser(),
                     databaseConfiguration.getDatabasePassword());
 
@@ -162,14 +162,6 @@ public class MariaDBConnectionManager {
                 logger.warn("Non-Managed connection could not be closed. Whoops!", e);
             }
         }
-
     }
 
-    private static String getDatabaseUrl(DatabaseConfiguration dbConfig) {
-
-        int port = dbConfig.getDatabasePort();
-        return "jdbc:" + dbConfig.getDatabaseType().toLowerCase() + "://" + dbConfig.getDatabaseHost()
-                + ((port == 0) ? "" : (":" + port)) + "/" + dbConfig.getDatabaseName();
-
-    }
 }
diff --git a/extensions/database/src/com/google/refine/extension/database/mysql/MySQLConnectionManager.java b/extensions/database/src/com/google/refine/extension/database/mysql/MySQLConnectionManager.java
index f4a7ce5dd..4fea86a90 100644
--- a/extensions/database/src/com/google/refine/extension/database/mysql/MySQLConnectionManager.java
+++ b/extensions/database/src/com/google/refine/extension/database/mysql/MySQLConnectionManager.java
@@ -129,7 +129,7 @@ public class MySQLConnectionManager {
                     return connection;
                 }
             }
-            String dbURL = getDatabaseUrl(databaseConfiguration);
+            String dbURL = databaseConfiguration.toURI().toString();
             Class.forName(type.getClassPath());
 
             // logger.info("*** type.getClassPath() ::{}, {}**** ", type.getClassPath());
@@ -166,11 +166,4 @@ public class MySQLConnectionManager {
 
     }
 
-    private String getDatabaseUrl(DatabaseConfiguration dbConfig) {
-
-        int port = dbConfig.getDatabasePort();
-        return "jdbc:" + dbConfig.getDatabaseType() + "://" + dbConfig.getDatabaseHost()
-                + ((port == 0) ? "" : (":" + port)) + "/" + dbConfig.getDatabaseName() + "?useSSL=" + dbConfig.isUseSSL();
-
-    }
 }
diff --git a/extensions/database/src/com/google/refine/extension/database/pgsql/PgSQLConnectionManager.java b/extensions/database/src/com/google/refine/extension/database/pgsql/PgSQLConnectionManager.java
index bea76d091..1637c222e 100644
--- a/extensions/database/src/com/google/refine/extension/database/pgsql/PgSQLConnectionManager.java
+++ b/extensions/database/src/com/google/refine/extension/database/pgsql/PgSQLConnectionManager.java
@@ -136,7 +136,7 @@ public class PgSQLConnectionManager {
 
             Class.forName(type.getClassPath());
             DriverManager.setLoginTimeout(10);
-            String dbURL = getDatabaseUrl(databaseConfiguration);
+            String dbURL = databaseConfiguration.toURI().toString();
             connection = DriverManager.getConnection(dbURL, databaseConfiguration.getDatabaseUser(),
                     databaseConfiguration.getDatabasePassword());
 
@@ -165,11 +165,4 @@ public class PgSQLConnectionManager {
 
     }
 
-    private static String getDatabaseUrl(DatabaseConfiguration dbConfig) {
-
-        int port = dbConfig.getDatabasePort();
-        return "jdbc:" + dbConfig.getDatabaseType().toLowerCase() + "://" + dbConfig.getDatabaseHost()
-                + ((port == 0) ? "" : (":" + port)) + "/" + dbConfig.getDatabaseName();
-
-    }
 }
diff --git a/extensions/database/src/com/google/refine/extension/database/sqlite/SQLiteConnectionManager.java b/extensions/database/src/com/google/refine/extension/database/sqlite/SQLiteConnectionManager.java
index 5b9b4cfcd..7d42e002a 100644
--- a/extensions/database/src/com/google/refine/extension/database/sqlite/SQLiteConnectionManager.java
+++ b/extensions/database/src/com/google/refine/extension/database/sqlite/SQLiteConnectionManager.java
@@ -35,6 +35,8 @@ import com.google.refine.extension.database.SQLType;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
+import java.net.URI;
+import java.net.URISyntaxException;
 import java.sql.Connection;
 import java.sql.DriverManager;
 import java.sql.SQLException;
@@ -66,7 +68,12 @@ public class SQLiteConnectionManager {
     }
 
     public static String getDatabaseUrl(DatabaseConfiguration dbConfig) {
-        return "jdbc:" + dbConfig.getDatabaseType().toLowerCase() + ":" + dbConfig.getDatabaseName();
+        try {
+            URI uri = new URI("jdbc:" + dbConfig.getDatabaseType().toLowerCase(), dbConfig.getDatabaseName(), null);
+            return uri.toASCIIString();
+        } catch (URISyntaxException e) {
+            throw new IllegalArgumentException(e);
+        }
     }
 
     /**
diff --git a/main/src/com/google/refine/sorting/StringCriterion.java b/main/src/com/google/refine/sorting/StringCriterion.java
index 9abcc6f40..8d314ce4b 100644
--- a/main/src/com/google/refine/sorting/StringCriterion.java
+++ b/main/src/com/google/refine/sorting/StringCriterion.java
@@ -54,11 +54,11 @@ public class StringCriterion extends Criterion {
         super();
         collator = Collator.getInstance();
         collator.setDecomposition(Collator.FULL_DECOMPOSITION);
-        collator.setStrength(Collator.SECONDARY);
     }
 
     @Override
     public KeyMaker createKeyMaker() {
+        collator.setStrength(caseSensitive ? Collator.IDENTICAL : Collator.SECONDARY);
         return new KeyMaker() {
 
             @Override


Test patch:
diff --git a/extensions/database/tests/src/com/google/refine/extension/database/DatabaseConfigurationTest.java b/extensions/database/tests/src/com/google/refine/extension/database/DatabaseConfigurationTest.java
new file mode 100644
index 000000000..5a571e8f0
--- /dev/null
+++ b/extensions/database/tests/src/com/google/refine/extension/database/DatabaseConfigurationTest.java
@@ -0,0 +1,21 @@
+package com.google.refine.extension.database;
+
+import org.testng.annotations.Test;
+
+import static org.testng.Assert.assertEquals;
+
+public class DatabaseConfigurationTest {
+
+    @Test
+    public void testToURI() {
+        DatabaseConfiguration config = new DatabaseConfiguration();
+        config.setDatabaseType("mysql");
+        config.setDatabaseHost("my.host");
+        // maliciously crafted database name which attempts to enable local file reads for an exploit
+        config.setDatabaseName("test?allowLoadLocalInfile=true#");
+
+        String url = config.toURI().toString();
+        // the database name is escaped, preventing the exploit
+        assertEquals(url, "jdbc:mysql://my.host/test%3FallowLoadLocalInfile=true%23");
+    }
+}
diff --git a/main/tests/cypress/package.json b/main/tests/cypress/package.json
index c81d6360a..ff8194229 100644
--- a/main/tests/cypress/package.json
+++ b/main/tests/cypress/package.json
@@ -16,7 +16,7 @@
         "lint": "prettier --check . && eslint ."
     },
     "dependencies": {
-        "cypress": "13.1.0",
+        "cypress": "13.2.0",
         "cypress-file-upload": "^5.0.8"
     },
     "devDependencies": {
diff --git a/main/tests/cypress/yarn.lock b/main/tests/cypress/yarn.lock
index 208e0a6ab..f5f0020d7 100644
--- a/main/tests/cypress/yarn.lock
+++ b/main/tests/cypress/yarn.lock
@@ -116,10 +116,10 @@
   resolved "https://registry.yarnpkg.com/@types/color-name/-/color-name-1.1.1.tgz"
   integrity sha512-rr+OQyAjxze7GgWrSaJwydHStIhHq2lvY3BOC2Mj7KnzI7XK0Uw1TOOdI9lDoajEbSWLiYgoo4f1R51erQfhPQ==
 
-"@types/node@*", "@types/node@^16.18.39":
-  version "16.18.39"
-  resolved "https://registry.yarnpkg.com/@types/node/-/node-16.18.39.tgz#aa39a1a87a40ef6098ee69689a1acb0c1b034832"
-  integrity sha512-8q9ZexmdYYyc5/cfujaXb4YOucpQxAV4RMG0himLyDUOEr8Mr79VrqsFI+cQ2M2h89YIuy95lbxuYjxT4Hk4kQ==
+"@types/node@*", "@types/node@^18.17.5":
+  version "18.17.15"
+  resolved "https://registry.yarnpkg.com/@types/node/-/node-18.17.15.tgz#31301a273b9ca7d568fe6d1c35ae52e0fb3f8d6a"
+  integrity sha512-2yrWpBk32tvV/JAd3HNHWuZn/VDN1P+72hWirHnvsvTGSqbANi+kSeuQR9yAHnbvaBvHDsoTdXV0Fe+iRtHLKA==
 
 "@types/sinonjs__fake-timers@8.1.1":
   version "8.1.1"
@@ -431,14 +431,14 @@ cypress-file-upload@^5.0.8:
   resolved "https://registry.yarnpkg.com/cypress-file-upload/-/cypress-file-upload-5.0.8.tgz#d8824cbeaab798e44be8009769f9a6c9daa1b4a1"
   integrity sha512-+8VzNabRk3zG6x8f8BWArF/xA/W0VK4IZNx3MV0jFWrJS/qKn8eHfa5nU73P9fOQAgwHFJx7zjg4lwOnljMO8g==
 
-cypress@13.1.0:
-  version "13.1.0"
-  resolved "https://registry.yarnpkg.com/cypress/-/cypress-13.1.0.tgz#18f268e66662cd91b1766db18bd1f63a66592205"
-  integrity sha512-LUKxCYlB973QBFls1Up4FAE9QIYobT+2I8NvvAwMfQS2YwsWbr6yx7y9hmsk97iqbHkKwZW3MRjoK1RToBFVdQ==
+cypress@13.2.0:
+  version "13.2.0"
+  resolved "https://registry.yarnpkg.com/cypress/-/cypress-13.2.0.tgz#10f73d06a0764764ffbb903a31e96e2118dcfc1d"
+  integrity sha512-AvDQxBydE771GTq0TR4ZUBvv9m9ffXuB/ueEtpDF/6gOcvFR96amgwSJP16Yhqw6VhmwqspT5nAGzoxxB+D89g==
   dependencies:
     "@cypress/request" "^3.0.0"
     "@cypress/xvfb" "^1.2.4"
-    "@types/node" "^16.18.39"
+    "@types/node" "^18.17.5"
     "@types/sinonjs__fake-timers" "8.1.1"
     "@types/sizzle" "^2.3.2"
     arch "^2.2.0"
diff --git a/main/tests/server/src/com/google/refine/operations/row/RowReorderOperationTests.java b/main/tests/server/src/com/google/refine/operations/row/RowReorderOperationTests.java
index 87cc866ea..6a460b934 100644
--- a/main/tests/server/src/com/google/refine/operations/row/RowReorderOperationTests.java
+++ b/main/tests/server/src/com/google/refine/operations/row/RowReorderOperationTests.java
@@ -63,7 +63,9 @@ public class RowReorderOperationTests extends RefineTest {
                         "8,b\n" +
                         ",d\n" +
                         "2,f\n" +
-                        "1,h\n");
+                        "1,h\n" +
+                        "9,F\n" +
+                        "10,f\n");
     }
 
     @AfterMethod
@@ -81,10 +83,48 @@ public class RowReorderOperationTests extends RefineTest {
         Process process = op.createProcess(project, new Properties());
         process.performImmediate();
 
-        Assert.assertEquals("h", project.rows.get(0).cells.get(1).value);
-        Assert.assertEquals("f", project.rows.get(1).cells.get(1).value);
-        Assert.assertEquals("b", project.rows.get(2).cells.get(1).value);
-        Assert.assertEquals("d", project.rows.get(3).cells.get(1).value);
+        Assert.assertEquals(project.rows.get(0).cells.get(1).value, "h");
+        Assert.assertEquals(project.rows.get(1).cells.get(1).value, "f");
+        Assert.assertEquals(project.rows.get(2).cells.get(1).value, "b");
+        Assert.assertEquals(project.rows.get(3).cells.get(1).value, "F");
+        Assert.assertEquals(project.rows.get(4).cells.get(1).value, "f");
+        Assert.assertEquals(project.rows.get(5).cells.get(1).value, "d");
+    }
+
+    @Test
+    public void testReverseSort() throws Exception {
+        String sortingJson = "{\"criteria\":[{\"column\":\"key\",\"valueType\":\"number\",\"reverse\":true,\"blankPosition\":-1,\"errorPosition\":1}]}";
+        SortingConfig sortingConfig = SortingConfig.reconstruct(sortingJson);
+        project.rows.get(1).cells.set(0, new Cell("", null));
+        AbstractOperation op = new RowReorderOperation(
+                Mode.RowBased, sortingConfig);
+        Process process = op.createProcess(project, new Properties());
+        process.performImmediate();
+
+        Assert.assertEquals(project.rows.get(5).cells.get(1).value, "h");
+        Assert.assertEquals(project.rows.get(4).cells.get(1).value, "f");
+        Assert.assertEquals(project.rows.get(3).cells.get(1).value, "b");
+        Assert.assertEquals(project.rows.get(2).cells.get(1).value, "F");
+        Assert.assertEquals(project.rows.get(1).cells.get(1).value, "f");
+        Assert.assertEquals(project.rows.get(0).cells.get(1).value, "d"); // controlled by blankPosition, not reverse
+    }
+
+    @Test
+    public void testStringSort() throws Exception {
+        String sortingJson = "{\"criteria\":[{\"column\":\"first\",\"valueType\":\"string\",\"reverse\":false,\"blankPosition\":2,\"errorPosition\":1,\"caseSensitive\":true}]}";
+        SortingConfig sortingConfig = SortingConfig.reconstruct(sortingJson);
+        project.rows.get(1).cells.set(0, new Cell("", null));
+        AbstractOperation op = new RowReorderOperation(
+                Mode.RowBased, sortingConfig);
+        Process process = op.createProcess(project, new Properties());
+        process.performImmediate();
+
+        Assert.assertEquals(project.rows.get(0).cells.get(1).value, "b");
+        Assert.assertEquals(project.rows.get(1).cells.get(1).value, "d");
+        Assert.assertEquals(project.rows.get(2).cells.get(1).value, "f");
+        Assert.assertEquals(project.rows.get(3).cells.get(1).value, "f");
+        Assert.assertEquals(project.rows.get(4).cells.get(1).value, "F");
+        Assert.assertEquals(project.rows.get(5).cells.get(1).value, "h");
     }
 
     @Test