Merge pull request #2026 from OpenRefine/issue-1990

Fix JSON history corruption.

main/src/com/google/refine/commands/history/ApplyOperationsCommand.java

@@ -45,6 +45,7 @@ import com.fasterxml.jackson.databind.node.ObjectNode;
 import com.google.refine.commands.Command;
 import com.google.refine.model.AbstractOperation;
 import com.google.refine.model.Project;
+import com.google.refine.operations.UnknownOperation;
 import com.google.refine.process.Process;
 import com.google.refine.util.ParsingUtilities;
 
@@ -79,7 +80,7 @@ public class ApplyOperationsCommand extends Command {
     
     protected void reconstructOperation(Project project, ObjectNode obj) throws IOException {
         AbstractOperation operation = ParsingUtilities.mapper.convertValue(obj, AbstractOperation.class);
-        if (operation != null) {
+        if (operation != null && !(operation instanceof UnknownOperation)) {
             try {
                 Process process = operation.createProcess(project, new Properties());
                 

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

@@ -64,7 +64,11 @@ public class FileHistoryEntryManager implements HistoryEntryManager{
     @Override
     public void save(HistoryEntry historyEntry, Writer writer, Properties options) {
         try {
+        	if("save".equals(options.getProperty("mode"))) {
+        		ParsingUtilities.saveWriter.writeValue(writer, historyEntry);
+        	} else {
         		ParsingUtilities.defaultWriter.writeValue(writer, historyEntry);
+        	}
         } catch (IOException e) {
             e.printStackTrace();
         }

main/src/com/google/refine/model/AbstractOperation.java

@@ -51,7 +51,8 @@ import com.google.refine.process.QuickHistoryEntryProcess;
 @JsonTypeInfo(
         use=JsonTypeInfo.Id.CUSTOM,
         include=JsonTypeInfo.As.PROPERTY,
-        property="op")
+        property="op",
+        visible=true) // for UnknownOperation, which needs to read its own id
 @JsonTypeIdResolver(OperationResolver.class)
 abstract public class AbstractOperation  {
     public Process createProcess(Project project, Properties options) throws Exception {

main/src/com/google/refine/operations/OperationResolver.java

@@ -51,11 +51,20 @@ public class OperationResolver extends TypeIdResolverBase {
 
     @Override
     public String idFromValueAndType(Object instance, Class<?> type) {
-        return OperationRegistry.s_opClassToName.get(type);
+        String id = OperationRegistry.s_opClassToName.get(type);
+        if (id != null) {
+        	return id;
+        } else { // this happens for an UnknownOperation
+        	return ((AbstractOperation) instance).getOperationId();
+        }
     }
     
     @Override
     public JavaType typeFromId(DatabindContext context, String id) throws IOException {
-        return factory.constructSimpleType(OperationRegistry.resolveOperationId(id), new JavaType[0]);
+    	Class<? extends AbstractOperation> opClass = OperationRegistry.resolveOperationId(id);
+    	if (opClass == null) {
+    		opClass = UnknownOperation.class;
+    	}
+    	return factory.constructSimpleType(opClass, new JavaType[0]);
     }
 }

main/src/com/google/refine/operations/UnknownOperation.java

@@ -0,0 +1,62 @@
+package com.google.refine.operations;
+
+import java.util.HashMap;
+import java.util.Map;
+
+import com.fasterxml.jackson.annotation.JsonAnyGetter;
+import com.fasterxml.jackson.annotation.JsonAnySetter;
+import com.fasterxml.jackson.annotation.JsonCreator;
+import com.fasterxml.jackson.annotation.JsonProperty;
+import com.google.refine.model.AbstractOperation;
+import com.google.refine.model.Project;
+
+/**
+ * An operation that is unknown to the current OpenRefine
+ * instance, but might be interpretable by another instance
+ * (for instance, a later version of OpenRefine, or using 
+ * an extension).
+ * 
+ * This class holds the JSON serialization of the operation,
+ * in the interest of being able to serialize it later, hence
+ * avoiding to discard it and lose metadata.
+ * 
+ * @author Antonin Delpeuch
+ *
+ */
+public class UnknownOperation extends AbstractOperation {
+	
+	// Map storing the JSON serialization of the operation in an agnostic way
+	private Map<String, Object> properties;
+	
+	// Operation code and description stored separately
+	private String opCode;
+	private String description;
+	
+	@JsonCreator
+	public UnknownOperation(
+			@JsonProperty("op") String opCode,
+			@JsonProperty("description") String description) {
+		properties = new HashMap<>();
+		this.opCode = opCode;
+		this.description = description;
+	}
+	
+	@JsonAnySetter
+	public void setAttribute(String key, Object value) {
+		properties.put(key, value);
+	}
+	
+	@JsonAnyGetter
+	public Map<String,Object> getAttributes() {
+		return properties;
+	}
+	
+    @JsonProperty("op")
+    public String getOperationId() {
+        return opCode;
+    }
+    
+    protected String getBriefDescription(Project project) {
+        return description;
+    }
+}

main/tests/server/src/com/google/refine/tests/history/FileHistoryEntryManagerTests.java

@@ -0,0 +1,40 @@
+package com.google.refine.tests.history;
+
+import static org.mockito.Mockito.mock;
+
+import java.io.IOException;
+import java.io.StringWriter;
+import java.util.Properties;
+
+import org.testng.annotations.BeforeMethod;
+import org.testng.annotations.Test;
+
+import com.google.refine.history.HistoryEntry;
+import com.google.refine.io.FileHistoryEntryManager;
+import com.google.refine.model.Project;
+import com.google.refine.operations.OperationRegistry;
+import com.google.refine.operations.column.ColumnAdditionOperation;
+import com.google.refine.tests.RefineTest;
+import com.google.refine.tests.util.TestUtils;
+
+public class FileHistoryEntryManagerTests extends RefineTest {
+	
+	Project project;
+	FileHistoryEntryManager sut = new FileHistoryEntryManager();
+	
+    @BeforeMethod
+    public void setUp() {
+        project = mock(Project.class);
+        OperationRegistry.registerOperation(getCoreModule(), "column-addition", ColumnAdditionOperation.class);
+    }
+    
+	@Test
+	public void testWriteHistoryEntry() throws IOException {
+		StringWriter writer = new StringWriter();
+		HistoryEntry historyEntry = HistoryEntry.load(project, HistoryEntryTests.fullJson);
+		Properties options = new Properties();
+		options.setProperty("mode", "save");
+		sut.save(historyEntry, writer, options);
+		TestUtils.equalAsJson(HistoryEntryTests.fullJson, writer.toString());
+	}
+}

main/tests/server/src/com/google/refine/tests/history/HistoryEntryTests.java

@@ -28,6 +28,8 @@ package com.google.refine.tests.history;
 
 import static org.mockito.Mockito.mock;
 
+import java.io.IOException;
+
 import org.testng.annotations.BeforeMethod;
 import org.testng.annotations.BeforeTest;
 import org.testng.annotations.Test;
@@ -41,6 +43,30 @@ import com.google.refine.tests.util.TestUtils;
 
 public class HistoryEntryTests extends RefineTest {
 	
+    public static final String fullJson = "{"
+            + "\"id\":1533633623158,"
+            + "\"description\":\"Create new column uri based on column country by filling 269 rows with grel:\\\"https://www.wikidata.org/wiki/\\\"+cell.recon.match.id\","
+            + "\"time\":\"2018-08-07T09:06:37Z\","
+            + "\"operation\":{\"op\":\"core/column-addition\","
+            + "   \"description\":\"Create column uri at index 2 based on column country using expression grel:\\\"https://www.wikidata.org/wiki/\\\"+cell.recon.match.id\","
+            + "   \"engineConfig\":{\"mode\":\"row-based\",\"facets\":[]},"
+            + "   \"newColumnName\":\"uri\","
+            + "   \"columnInsertIndex\":2,"
+            + "   \"baseColumnName\":\"country\","
+            + "   \"expression\":\"grel:\\\"https://www.wikidata.org/wiki/\\\"+cell.recon.match.id\","
+            + "   \"onError\":\"set-to-blank\"}"
+            + "}";
+    
+	public static final String unknownOperationJson = "{"
+            + "\"id\":1533633623158,"
+            + "\"description\":\"some mysterious operation\","
+            + "\"time\":\"2018-08-07T09:06:37Z\","
+            + "\"operation\":{\"op\":\"someextension/unknown-operation\","
+            + "   \"description\":\"some mysterious operation\","
+            + "   \"some_parameter\":234\n"
+		    + "}\n"
+            + "}";
+    
     Project project;
     
     @BeforeTest
@@ -63,26 +89,20 @@ public class HistoryEntryTests extends RefineTest {
     
     @Test
     public void serializeHistoryEntryWithOperation() throws Exception {
-        String json = "{"
-                + "\"id\":1533633623158,"
-                + "\"description\":\"Create new column uri based on column country by filling 269 rows with grel:\\\"https://www.wikidata.org/wiki/\\\"+cell.recon.match.id\","
-                + "\"time\":\"2018-08-07T09:06:37Z\","
-                + "\"operation\":{\"op\":\"core/column-addition\","
-                + "   \"description\":\"Create column uri at index 2 based on column country using expression grel:\\\"https://www.wikidata.org/wiki/\\\"+cell.recon.match.id\","
-                + "   \"engineConfig\":{\"mode\":\"row-based\",\"facets\":[]},"
-                + "   \"newColumnName\":\"uri\","
-                + "   \"columnInsertIndex\":2,"
-                + "   \"baseColumnName\":\"country\","
-                + "   \"expression\":\"grel:\\\"https://www.wikidata.org/wiki/\\\"+cell.recon.match.id\","
-                + "   \"onError\":\"set-to-blank\"}"
-                + "}";
         String jsonSimple = "{"
                 + "\"id\":1533633623158,"
                 + "\"description\":\"Create new column uri based on column country by filling 269 rows with grel:\\\"https://www.wikidata.org/wiki/\\\"+cell.recon.match.id\","
                 + "\"time\":\"2018-08-07T09:06:37Z\"}";
         
-        HistoryEntry historyEntry = HistoryEntry.load(project, json);
+        HistoryEntry historyEntry = HistoryEntry.load(project, fullJson);
         TestUtils.isSerializedTo(historyEntry, jsonSimple, false);
-        TestUtils.isSerializedTo(historyEntry, json, true);
+        TestUtils.isSerializedTo(historyEntry, fullJson, true);
+    }
+    
+    @Test
+    public void deserializeUnknownOperation() throws IOException {
+    	// Unknown operations are serialized back as they were parsed
+    	HistoryEntry entry = HistoryEntry.load(project, unknownOperationJson);
+    	TestUtils.isSerializedTo(entry, unknownOperationJson, true);
     }
 }