Save preferences JSON using UTF-8 encoding. Bulletproof prefs load. (#2657)

* Save preferences JSON using UTF-8 encoding. Bulletproof prefs load.

Fixes #2543. Fixes #2627.

Always use UTF-8 to write JSON because platform default encoding
might not be legal JSON (e.g. ISO 8859-1).

Also be more conservative about keeping backups if we fail to write.

* Handle case where backup prefs is better than more recent

* Recover from corrupted prefs with null starred list.

Fixes #2544. Replaces null with an empty list.

* Run tests with non-UTF-8 encoding

Make sure that we don't depend on UTF-8 being the default encoding
because it isn't true everywhere (e.g. Windows)

* Add test for non-ASCII chars in workspace.json

This depends on the default Java encoding being something
other than UTF-8 to test properly.

main/src/com/google/refine/io/FileProjectManager.java

@@ -40,6 +40,8 @@ import java.io.FileWriter;
 import java.io.IOException;
 import java.io.InputStream;
 import java.io.OutputStream;
+import java.io.OutputStreamWriter;
+import java.nio.charset.StandardCharsets;
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map.Entry;
@@ -307,13 +309,14 @@ public class FileProjectManager extends ProjectManager  {
     }
 
     protected boolean saveToFile(File file) throws IOException {
-        FileWriter writer = new FileWriter(file);
+        OutputStream stream = new FileOutputStream(file);
         boolean saveWasNeeded = saveNeeded();
         try {
-            ParsingUtilities.defaultWriter.writeValue(writer, this);
+            // writeValue(OutputStream) is documented to use JsonEncoding.UTF8
+            ParsingUtilities.defaultWriter.writeValue(stream, this);
             saveProjectMetadata();
         } finally {
-            writer.close();
+            stream.close();
         }
         return saveWasNeeded;
     }

main/src/com/google/refine/io/ProjectMetadataUtilities.java

@@ -39,6 +39,7 @@ import java.io.FileReader;
 import java.io.IOException;
 import java.io.OutputStreamWriter;
 import java.io.Writer;
+import java.nio.charset.StandardCharsets;
 import java.time.Instant;
 import java.time.LocalDateTime;
 import java.time.ZoneId;
@@ -60,25 +61,33 @@ public class ProjectMetadataUtilities {
     public static void save(ProjectMetadata projectMeta, File projectDir) throws IOException  {
         File tempFile = new File(projectDir, "metadata.temp.json");
         saveToFile(projectMeta, tempFile);
+        if (tempFile.length() == 0) {
+            throw new IOException("Failed to save project metadata - keeping backups");
+        }
+
+        // TODO Do we want to make sure we can successfully deserialize the file too?
 
         File file = new File(projectDir, "metadata.json");
         File oldFile = new File(projectDir, "metadata.old.json");
 
-        if (oldFile.exists()) {
-            oldFile.delete();
-        }
-        
         if (file.exists()) {
+            if(file.length() > 0) {
+                if (oldFile.exists()) {
+                    oldFile.delete();
+                }
             file.renameTo(oldFile);
+            } else {
+                file.delete();
+            }
         }
 
         tempFile.renameTo(file);
     }
     
     protected static void saveToFile(ProjectMetadata projectMeta, File metadataFile) throws IOException   {
-        Writer writer = new OutputStreamWriter(new FileOutputStream(metadataFile));
+        Writer writer = new OutputStreamWriter(new FileOutputStream(metadataFile), StandardCharsets.UTF_8);
         try {
-            ParsingUtilities.defaultWriter.writeValue(writer, projectMeta);
+            ParsingUtilities.saveWriter.writeValue(writer, projectMeta);
         } finally {
             writer.close();
         }
@@ -121,7 +130,7 @@ public class ProjectMetadataUtilities {
      * creation and modification times based on whatever files are available.
      * 
      * @param projectDir the project directory
-     * @param id the proejct id
+     * @param id the project id
      * @return
      */
     static public ProjectMetadata recover(File projectDir, long id) {

main/src/com/google/refine/preference/PreferenceStore.java

@@ -37,6 +37,7 @@ import java.util.HashMap;
 import java.util.Iterator;
 import java.util.Map;
 import java.util.Set;
+import java.util.logging.Logger;
 
 import com.fasterxml.jackson.annotation.JsonIgnore;
 import com.fasterxml.jackson.annotation.JsonInclude;
@@ -100,6 +101,12 @@ public class PreferenceStore  {
             if (entries.get(key) != null) {
                 JsonNode o = entries.get(key);
                 Object loaded = loadObject(o);
+                if (loaded == null) {
+                    if ("scripting.starred-expressions".contentEquals(key)) {
+                        // HACK to work around preferences corruption
+                        loaded = new TopList(10);
+                    }
+                }
                 _prefs.put(key, loaded);
             }
         }

main/tests/server/src/com/google/refine/io/FileProjectManagerTests.java

@@ -27,6 +27,7 @@
 package com.google.refine.io;
 
 import static org.mockito.Mockito.mock;
+import static org.testng.Assert.assertEquals;
 
 import java.io.File;
 import java.io.IOException;
@@ -50,7 +51,7 @@ public class FileProjectManagerTests  {
 
         protected FileProjectManagerStub(File dir) {
             super(dir);
-            _projectsMetadata.put(1234L, mock(ProjectMetadata.class));
+            _projectsMetadata.put(5555L, mock(ProjectMetadata.class));
         }
     }
     
@@ -69,11 +70,27 @@ public class FileProjectManagerTests  {
                 "             \"class\" : \"com.google.refine.preference.TopList\",\n" + 
                 "             \"list\" : [ ],\n" + 
                 "             \"top\" : 2147483647\n" + 
-                "           }\n" + 
+                "           }\n" +
                 "         }\n" + 
                 "       },\n" + 
-                "       \"projectIDs\" : [ 1234 ]\n" + 
+                "       \"projectIDs\" : [ 5555 ]\n" +
                 "     }";
         TestUtils.isSerializedTo(manager, json);
+
     }
+
+    /**
+     * Test that we can save and restore non-ASCII characters.
+     * For best effectiveness, this should be run with a non-UTF8
+     * default encoding for Java e.g. java -Dfile.encoding=cp1252
+     * to simulate running on a Windows system
+     */
+    @Test
+    public void saveReloadMultinationalCharacter () throws IOException {
+        FileProjectManager manager = new FileProjectManagerStub(workspaceDir);
+        manager.getPreferenceStore().put("testPref", "Refiné");
+        manager.saveWorkspace();
+        manager = new FileProjectManagerStub(workspaceDir);
+        assertEquals(manager.getPreferenceStore().get("testPref"), "Refiné");
+        }
 }

pom.xml

@@ -57,7 +57,7 @@
     <jee.port>3333</jee.port>
     <refine.data>/tmp/refine</refine.data>
     <surefire.version>2.22.2</surefire.version>
-    <surefireArgs></surefireArgs>
+    <surefireArgs>-Dfile.encoding=cp1252</surefireArgs>
     <project.build.sourceEncoding>UTF-8</project.build.sourceEncoding>
     <jackson.version>2.11.0</jackson.version>
   </properties>