Repository: OpenRefine/OpenRefine (java)

Issue:
Mass edit of new blank cells corrupts project in 3.9.2
Initially mentioned in [this forum thread](https://forum.openrefine.org/t/after-adding-rows-and-editing-a-cell-project-failing-to-load/2214)

### To Reproduce
Steps to reproduce the behavior:
1. Using OpenRefine 3.9.2
2. Open any project
3. Add multiple rows to the end of the project
4. Edit one of the cells and click "Apply to all identical cells"



### Current Results
The project cannot be loaded. After editing and upon re-opening the project, the browser is redirected to the loading screen.
### Expected Behavior
The result entered into the edit dialog box should be applied to all empty cells in the column.

### Versions
 - Operating System: Ubuntu 24.04
 - Browser Version: Firefox 136.0.3
 - JRE or JDK Version: 21.0.5
 - OpenRefine: 3.9.2


Pull request:
Backport: Fix deserialization of RowAdditionOperation
Fixes #7245 on branch `3.9`.

Cherry-picked from 1f9d248e3d641ea2509b83909573f375a04e72b8 (PR #7247)


Patch:
diff --git a/main/src/com/google/refine/operations/row/RowAdditionOperation.java b/main/src/com/google/refine/operations/row/RowAdditionOperation.java
index c7e3bc7d9..d829c5f90 100644
--- a/main/src/com/google/refine/operations/row/RowAdditionOperation.java
+++ b/main/src/com/google/refine/operations/row/RowAdditionOperation.java
@@ -28,6 +28,7 @@
 package com.google.refine.operations.row;
 
 import java.util.List;
+import java.util.stream.Collectors;
 
 import com.fasterxml.jackson.annotation.JsonCreator;
 import com.fasterxml.jackson.annotation.JsonProperty;
@@ -44,15 +45,39 @@ public class RowAdditionOperation extends AbstractOperation {
     final private List<Row> _rows;
     final private int _insertionIndex;
 
+    public RowAdditionOperation(
+            List<Row> rows,
+            int insertionIndex) {
+        _rows = rows;
+        _insertionIndex = insertionIndex;
+    }
+
+    /**
+     * Deserialization constructor to provide compatibility for the legacy serialization format. In this format, only
+     * the number of rows is relevant: the contents of the rows must be ignored, because they might have been corrupted
+     * due to mutability issues in https://github.com/OpenRefine/OpenRefine/issues/7245.
+     * 
+     * @param addedRows
+     *            the rows to add to the project
+     * @param rows
+     *            a legacy serialization field, whose length is is the only thing that matters. If provided, it will be
+     *            converted to a list of empty rows of the same size.
+     * @param insertionIndex
+     *            the place in the grid where to insert this list.
+     * @deprecated should not be called directly, is only provided for JSON deserialization.
+     */
+    @Deprecated
     @JsonCreator
     public RowAdditionOperation(
-            @JsonProperty("rows") List<Row> rows,
+            @JsonProperty("addedRows") List<Row> addedRows,
+            @JsonProperty("rows") List<Object> rows,
             @JsonProperty("insertionIndex") int insertionIndex) {
-        _rows = rows;
+        _rows = addedRows != null ? addedRows
+                : (rows == null ? List.of() : rows.stream().map(r -> new Row(0)).collect(Collectors.toList()));
         _insertionIndex = insertionIndex;
     }
 
-    @JsonProperty("rows")
+    @JsonProperty("addedRows")
     public List<Row> getRows() {
         return _rows;
     }
diff --git a/modules/core/src/main/java/com/google/refine/model/Cell.java b/modules/core/src/main/java/com/google/refine/model/Cell.java
index 119f90e0d..084452272 100644
--- a/modules/core/src/main/java/com/google/refine/model/Cell.java
+++ b/modules/core/src/main/java/com/google/refine/model/Cell.java
@@ -193,6 +193,13 @@ public class Cell implements HasFields, Serializable {
         return new Cell((Serializable) value, recon);
     }
 
+    /**
+     * Return a deep copy of this object, making sure that if one instance is modified, the other isn't.
+     */
+    public Cell deepCopy() {
+        return new Cell(value, recon == null ? null : recon.dup());
+    }
+
     @Override
     public String toString() {
         // TODO this is kept like this for now, but it should rather be a string which makes the difference
diff --git a/modules/core/src/main/java/com/google/refine/model/Row.java b/modules/core/src/main/java/com/google/refine/model/Row.java
index 2aff00999..5550ec9fa 100644
--- a/modules/core/src/main/java/com/google/refine/model/Row.java
+++ b/modules/core/src/main/java/com/google/refine/model/Row.java
@@ -38,6 +38,7 @@ import java.io.Writer;
 import java.util.ArrayList;
 import java.util.List;
 import java.util.Properties;
+import java.util.stream.Collectors;
 
 import com.fasterxml.jackson.annotation.JsonCreator;
 import com.fasterxml.jackson.annotation.JsonIgnore;
@@ -93,6 +94,19 @@ public class Row implements HasFields {
         return row;
     }
 
+    /**
+     * Make a deep copy of the row, making sure that if one is modified, the other isn't.
+     */
+    public Row deepCopy() {
+        Row row = new Row(cells.size());
+        row.flagged = flagged;
+        row.starred = starred;
+        row.cells.addAll(cells.stream()
+                .map(cell -> cell == null ? null : cell.deepCopy())
+                .collect(Collectors.toList()));
+        return row;
+    }
+
     @Override
     public Object getField(String name, Properties bindings) {
         if (FLAGGED.equals(name)) {
diff --git a/modules/core/src/main/java/com/google/refine/model/changes/RowAdditionChange.java b/modules/core/src/main/java/com/google/refine/model/changes/RowAdditionChange.java
index f3efb0f9c..5671d27e6 100644
--- a/modules/core/src/main/java/com/google/refine/model/changes/RowAdditionChange.java
+++ b/modules/core/src/main/java/com/google/refine/model/changes/RowAdditionChange.java
@@ -33,6 +33,7 @@ import java.io.Writer;
 import java.util.ArrayList;
 import java.util.List;
 import java.util.Properties;
+import java.util.stream.Collectors;
 
 import com.google.refine.ProjectManager;
 import com.google.refine.history.Change;
@@ -57,7 +58,7 @@ public class RowAdditionChange implements Change {
     @Override
     public void apply(Project project) {
         synchronized (project) {
-            project.rows.addAll(_insertionIndex, _additionalRows);
+            project.rows.addAll(_insertionIndex, _additionalRows.stream().map(row -> row.deepCopy()).collect(Collectors.toList()));
 
             project.update();
             project.columnModel.clearPrecomputes();


Test patch:
diff --git a/main/tests/server/src/com/google/refine/operations/row/RowAdditionOperationTests.java b/main/tests/server/src/com/google/refine/operations/row/RowAdditionOperationTests.java
index d029e0121..bbc3658cb 100644
--- a/main/tests/server/src/com/google/refine/operations/row/RowAdditionOperationTests.java
+++ b/main/tests/server/src/com/google/refine/operations/row/RowAdditionOperationTests.java
@@ -30,6 +30,7 @@ package com.google.refine.operations.row;
 import static org.testng.Assert.assertEquals;
 
 import java.io.IOException;
+import java.io.Serializable;
 import java.util.ArrayList;
 import java.util.List;
 
@@ -43,6 +44,8 @@ import org.testng.annotations.BeforeTest;
 import org.testng.annotations.Test;
 
 import com.google.refine.RefineTest;
+import com.google.refine.model.Cell;
+import com.google.refine.model.Project;
 import com.google.refine.model.Row;
 import com.google.refine.operations.OperationDescription;
 import com.google.refine.operations.OperationRegistry;
@@ -51,7 +54,8 @@ import com.google.refine.util.TestUtils;
 
 public class RowAdditionOperationTests extends RefineTest {
 
-    String json;
+    String legacyJson;
+    String newJson;
 
     @BeforeSuite
     public void registerOperation() {
@@ -60,9 +64,17 @@ public class RowAdditionOperationTests extends RefineTest {
 
     @BeforeMethod
     public void setUp() {
-        json = "{"
+        // JSON serialization before 3.9.3, which might contain some corrupted cells because of mutability issues,
+        // manifesting themselves in https://github.com/OpenRefine/OpenRefine/issues/7245
+        legacyJson = "{"
                 + "\"op\":\"core/row-addition\","
-                + "\"rows\":[{\"starred\":false,\"flagged\":false,\"cells\":[]},{\"starred\":false,\"flagged\":false,\"cells\":[]}],"
+                + "\"rows\":[{\"starred\":false,\"flagged\":false,\"cells\":[{\"foo\":3}]},{\"starred\":false,\"flagged\":false,\"cells\":[]}],"
+                + "\"index\":0,"
+                + "\"description\":" + new TextNode(OperationDescription.row_addition_brief()).toString() + "}";
+        // JSON serialization from 3.9.3 on
+        newJson = "{"
+                + "\"op\":\"core/row-addition\","
+                + "\"addedRows\":[{\"starred\":false,\"flagged\":false,\"cells\":[]},{\"starred\":false,\"flagged\":false,\"cells\":[]}],"
                 + "\"index\":0,"
                 + "\"description\":" + new TextNode(OperationDescription.row_addition_brief()).toString() + "}";
     }
@@ -74,9 +86,15 @@ public class RowAdditionOperationTests extends RefineTest {
     }
 
     @Test
-    public void testDeserialization() throws IOException {
-        RowAdditionOperation op = ParsingUtilities.mapper.readValue(json, RowAdditionOperation.class);
-        TestUtils.isSerializedTo(op, json);
+    public void testLegacyDeserialization() throws IOException {
+        RowAdditionOperation op = ParsingUtilities.mapper.readValue(legacyJson, RowAdditionOperation.class);
+        TestUtils.isSerializedTo(op, newJson);
+    }
+
+    @Test
+    public void testNewDeserialization() throws IOException {
+        RowAdditionOperation op = ParsingUtilities.mapper.readValue(newJson, RowAdditionOperation.class);
+        TestUtils.isSerializedTo(op, newJson);
     }
 
     @Test
@@ -89,7 +107,36 @@ public class RowAdditionOperationTests extends RefineTest {
         RowAdditionOperation op = new RowAdditionOperation(rows, index);
         ObjectMapper objectMapper = new ObjectMapper();
         String serializedObject = objectMapper.writeValueAsString(op);
-        assertEquals(serializedObject, json);
+        assertEquals(serializedObject, newJson);
     }
 
+
+    // regression test for https://github.com/OpenRefine/OpenRefine/issues/7245
+    @Test
+    public void mutabilityBugRegressionTest() throws Exception {
+        Project project = createProject(new String[] { "foo", "bar" },
+                new Serializable[][] {
+                        { "a", "b " },
+                        { null, "c" },
+                });
+
+        RowAdditionOperation op = ParsingUtilities.mapper.readValue(newJson, RowAdditionOperation.class);
+
+        // add the two rows at the beginning
+        runOperation(op, project);
+        // and then edit a cell in the first row
+        project.rows.get(0).setCell(0, new Cell("hello", null));
+
+        Project expected = createProject(new String[] { "foo", "bar" },
+                new Serializable[][] {
+                        { "hello", null },
+                        { null, null },
+                        { "a", "b " },
+                        { null, "c" },
+                });
+        assertProjectEquals(project, expected);
+
+        // the operation metadata is still unchanged
+        TestUtils.isSerializedTo(op, newJson);
+    }
 }
diff --git a/modules/core/src/test/java/com/google/refine/model/CellTests.java b/modules/core/src/test/java/com/google/refine/model/CellTests.java
index 1fecf9e1e..b2048df01 100644
--- a/modules/core/src/test/java/com/google/refine/model/CellTests.java
+++ b/modules/core/src/test/java/com/google/refine/model/CellTests.java
@@ -184,4 +184,17 @@ public class CellTests {
         assertNotEquals(c1, "foo");
         assertEquals(c1.hashCode(), new Cell("foo", recon).hashCode());
     }
+
+    @Test
+    public void testDeepCopy() {
+        Recon recon = mock(Recon.class);
+        Recon reconDup = mock(Recon.class);
+        when(recon.dup()).thenReturn(reconDup);
+
+        Cell c1 = new Cell("foo", recon);
+        Cell c2 = c1.deepCopy();
+
+        assertEquals(c2.value, "foo");
+        assertEquals(c2.recon, reconDup);
+    }
 }
diff --git a/modules/core/src/test/java/com/google/refine/model/RowTests.java b/modules/core/src/test/java/com/google/refine/model/RowTests.java
index 8337acd1b..c37662ef6 100644
--- a/modules/core/src/test/java/com/google/refine/model/RowTests.java
+++ b/modules/core/src/test/java/com/google/refine/model/RowTests.java
@@ -35,6 +35,9 @@ package com.google.refine.model;
 
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.when;
+import static org.testng.Assert.assertEquals;
+import static org.testng.Assert.assertFalse;
+import static org.testng.Assert.assertTrue;
 
 import java.io.StringWriter;
 import java.util.Properties;
@@ -187,4 +190,18 @@ public class RowTests extends RefineTest {
         Assert.assertTrue((Boolean) row.getField("starred", options));
     }
 
+    @Test
+    public void deepCopy() {
+        Row row = new Row(1);
+        row.flagged = true;
+        row.setCell(0, new Cell("original value", null));
+
+        Row copied = row.deepCopy();
+        row.starred = true;
+        row.setCell(0, new Cell("new value", null));
+
+        assertTrue(copied.flagged);
+        assertFalse(copied.starred);
+        assertEquals(copied.cells.get(0).value, "original value");
+    }
 }
diff --git a/modules/core/src/test/java/com/google/refine/model/changes/RowAdditionChangeTests.java b/modules/core/src/test/java/com/google/refine/model/changes/RowAdditionChangeTests.java
index b742aa489..3d6a48ffd 100644
--- a/modules/core/src/test/java/com/google/refine/model/changes/RowAdditionChangeTests.java
+++ b/modules/core/src/test/java/com/google/refine/model/changes/RowAdditionChangeTests.java
@@ -29,7 +29,7 @@ package com.google.refine.model.changes;
 
 import static org.testng.Assert.assertEquals;
 import static org.testng.Assert.assertNotNull;
-import static org.testng.Assert.assertSame;
+import static org.testng.Assert.assertNotSame;
 import static org.testng.Assert.assertTrue;
 
 import java.io.IOException;
@@ -107,13 +107,18 @@ public class RowAdditionChangeTests extends RefineTest {
     }
 
     @Test
-    // After prepend apply, project's new prepended rows are identical those passed to constructor
+    // After prepend apply, project's new prepended rows are equal those passed to constructor
+    // but not identical, because otherwise modifying the row will change the operation's metadata.
+    // See https://github.com/OpenRefine/OpenRefine/issues/7245
     public void testPrependApplyRowIdentity() {
         change.apply(project);
         for (int i = insertionIndex; i < newRows.size(); i++) {
             Row actual = project.rows.get(i);
             Row expected = newRows.get(i);
-            assertSame(actual, expected);
+            assertNotSame(actual, expected);
+            assertEquals(actual.flagged, expected.flagged);
+            assertEquals(actual.starred, expected.starred);
+            assertEquals(actual.cells, expected.cells);
         }
     }
 
@@ -205,6 +210,8 @@ public class RowAdditionChangeTests extends RefineTest {
 
     @Test
     // After append apply, project's new append rows are identical those passed to constructor
+    // but not identical, because otherwise modifying the row will change the operation's metadata.
+    // See https://github.com/OpenRefine/OpenRefine/issues/7245
     public void testAppendApplyRowIdentity() {
         insertionIndex = project.rows.size();
         change = new RowAdditionChange(newRows, insertionIndex);
@@ -213,7 +220,10 @@ public class RowAdditionChangeTests extends RefineTest {
         for (int i = insertionIndex; i < newRows.size(); i++) {
             Row actual = project.rows.get(insertionIndex + i);
             Row expected = newRows.get(i);
-            assertSame(actual, expected);
+            assertNotSame(actual, expected);
+            assertEquals(actual.flagged, expected.flagged);
+            assertEquals(actual.starred, expected.starred);
+            assertEquals(actual.cells, expected.cells);
         }
     }