diff --git a/main/src/com/google/refine/model/changes/MassChange.java b/main/src/com/google/refine/model/changes/MassChange.java index 0c27e4224..cdef9f84a 100644 --- a/main/src/com/google/refine/model/changes/MassChange.java +++ b/main/src/com/google/refine/model/changes/MassChange.java @@ -33,7 +33,9 @@ OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. package com.google.refine.model.changes; - import java.io.IOException; +import java.io.IOException; +import com.google.common.collect.Lists; + import java.io.LineNumberReader; import java.io.Writer; import java.util.ArrayList; @@ -70,8 +72,8 @@ public class MassChange implements Change { @Override public void revert(Project project) { synchronized (project) { - for (int i = _changes.size() - 1; i >= 0; i--){ - _changes.get(i).revert(project); + for (Change change : Lists.reverse(_changes)){ + change.revert(project); } if (_updateRowContextDependencies) { diff --git a/main/tests/server/src/com/google/refine/tests/model/changes/MassChangeTests.java b/main/tests/server/src/com/google/refine/tests/model/changes/MassChangeTests.java new file mode 100644 index 000000000..1de455304 --- /dev/null +++ b/main/tests/server/src/com/google/refine/tests/model/changes/MassChangeTests.java @@ -0,0 +1,67 @@ + +package com.google.refine.tests.model.changes; + +import static org.testng.AssertJUnit.assertTrue; + +import java.io.File; +import java.io.IOException; +import java.util.ArrayList; +import java.util.List; + +import org.slf4j.LoggerFactory; +import org.testng.annotations.BeforeMethod; +import org.testng.annotations.BeforeTest; +import org.testng.annotations.Test; + +import com.google.refine.ProjectManager; +import com.google.refine.ProjectMetadata; +import com.google.refine.model.ModelException; +import com.google.refine.model.Project; +import com.google.refine.model.changes.CellAtRow; +import com.google.refine.model.changes.ColumnAdditionChange; +import com.google.refine.model.changes.MassChange; +import com.google.refine.history.Change; +import com.google.refine.io.FileProjectManager; +import com.google.refine.tests.RefineTest; +import com.google.refine.tests.util.TestUtils; + +public class MassChangeTests extends RefineTest { + + Project project; + + @Override + @BeforeTest + public void init() { + logger = LoggerFactory.getLogger(this.getClass()); + } + + @BeforeMethod + public void SetUp() + throws IOException, ModelException { + File dir = TestUtils.createTempDirectory("openrefine-test-workspace-dir"); + FileProjectManager.initialize(dir); + project = new Project(); + ProjectMetadata pm = new ProjectMetadata(); + pm.setName("TNG Test Project"); + ProjectManager.singleton.registerProject(project, pm); + } + + /** + * Test case for #914 - Demonstrates MassChange revert doesn't work by + * adding two columns to a project with a MassChange and then reverting. + * Without the fix, column "a" will be removed before column "b", causing + * column "b" removal to fail because it won't be found at index 1 as + * expected. + */ + @Test + public void testWrongReverseOrder() + throws Exception { + List changes = new ArrayList(); + changes.add(new ColumnAdditionChange("a", 0, new ArrayList())); + changes.add(new ColumnAdditionChange("b", 1, new ArrayList())); + MassChange massChange = new MassChange(changes, false); + massChange.apply(project); + massChange.revert(project); + assertTrue(project.columnModel.columns.isEmpty()); + } +}