From 89af014e7dbcce8d22b59f4b499a23d05e754dbe Mon Sep 17 00:00:00 2001
From: Ludovic Poitou <ludovic.poitou@forgerock.com>
Date: Mon, 04 Jul 2011 20:09:22 +0000
Subject: [PATCH] Fix for OPENDJ-223. Modify operation isn't replayed on replica exactly as on original server. The conflict resolution code didn't consider the case where a single modify operation could contain multiple modifications on an attribute, especially a replace after adds or delete. All changeNumbers would then be equals. Replace always wins over all precedent operations on the attribute, as Modifications are ordered (Sequence of).
---
opendj-sdk/opends/src/server/org/opends/server/replication/plugin/AttrHistoricalMultiple.java | 6 +-
opendj-sdk/opends/tests/unit-tests-testng/src/server/org/opends/server/replication/plugin/ModifyConflictTest.java | 84 +++++++++++++++++++++++++++++++++++-------
2 files changed, 73 insertions(+), 17 deletions(-)
diff --git a/opendj-sdk/opends/src/server/org/opends/server/replication/plugin/AttrHistoricalMultiple.java b/opendj-sdk/opends/src/server/org/opends/server/replication/plugin/AttrHistoricalMultiple.java
index 06b0ed0..e406df3 100644
--- a/opendj-sdk/opends/src/server/org/opends/server/replication/plugin/AttrHistoricalMultiple.java
+++ b/opendj-sdk/opends/src/server/org/opends/server/replication/plugin/AttrHistoricalMultiple.java
@@ -23,6 +23,7 @@
*
*
* Copyright 2006-2010 Sun Microsystems, Inc.
+ * Portions Copyright 2011 ForgeRock AS
*/
package org.opends.server.replication.plugin;
@@ -244,10 +245,9 @@
{
// We are replaying an operation that was already done
// on another master server and this operation has a potential
- // conflict
- // with some more recent operations on this same entry
+ // conflict with some more recent operations on this same entry
// we need to take the more complex path to solve them
- if ((ChangeNumber.compare(changeNumber, getLastUpdateTime()) <= 0) ||
+ if ((ChangeNumber.compare(changeNumber, getLastUpdateTime()) < 0) ||
(m.getModificationType() != ModificationType.REPLACE))
{
// the attribute was modified after this change -> conflict
diff --git a/opendj-sdk/opends/tests/unit-tests-testng/src/server/org/opends/server/replication/plugin/ModifyConflictTest.java b/opendj-sdk/opends/tests/unit-tests-testng/src/server/org/opends/server/replication/plugin/ModifyConflictTest.java
index b524506..f4b5afc 100644
--- a/opendj-sdk/opends/tests/unit-tests-testng/src/server/org/opends/server/replication/plugin/ModifyConflictTest.java
+++ b/opendj-sdk/opends/tests/unit-tests-testng/src/server/org/opends/server/replication/plugin/ModifyConflictTest.java
@@ -23,11 +23,10 @@
*
*
* Copyright 2006-2010 Sun Microsystems, Inc.
+ * Portions Copyright 2011 ForgeRock AS
*/
package org.opends.server.replication.plugin;
-import org.opends.server.replication.protocol.LDAPUpdateMsg;
-
import java.util.ArrayList;
import java.util.HashMap;
import java.util.LinkedList;
@@ -46,11 +45,9 @@
import org.opends.server.protocols.internal.InternalClientConnection;
import org.opends.server.replication.ReplicationTestCase;
import org.opends.server.replication.common.ChangeNumber;
-import org.opends.server.replication.plugin.FakeOperation;
-import org.opends.server.replication.plugin.FakeOperationComparator;
-import org.opends.server.replication.plugin.EntryHistorical;
import org.opends.server.replication.protocol.ModifyContext;
import org.opends.server.replication.protocol.ReplicationMsg;
+import org.opends.server.replication.protocol.LDAPUpdateMsg;
import org.opends.server.types.*;
import org.opends.server.workflowelement.localbackend.LocalBackendAddOperation;
import org.opends.server.workflowelement.localbackend.LocalBackendModifyOperation;
@@ -886,6 +883,63 @@
}
/**
+ * Test that a DEL of one value and REPLACE with no value, of the same
+ * attribute done in a single operation correctly results in the value
+ * being kept.
+ *
+ * This is not a conflict in the strict definition but does exert
+ * the conflict resolution code.
+ */
+ @Test()
+ public void DelAndReplaceSameOp() throws Exception
+ {
+ Entry entry = initializeEntry();
+
+ // load historical from the entry
+ EntryHistorical hist = EntryHistorical.newInstanceFromEntry(entry);
+
+ /*
+ * simulate a add of the description attribute done at time t10
+ */
+ testModify(entry, hist, DESCRIPTION, ModificationType.ADD,
+ "init value", 10, true);
+ /*
+ * simulate a add of the description attribute done at time t10
+ */
+ testModify(entry, hist, DESCRIPTION, ModificationType.ADD,
+ "second value", 11, true);
+ /*
+ * Now simulate a delete of one value and a replace with no value
+ * in the same operation
+ */
+
+ Attribute attr = Attributes.create(DESCRIPTION, "init value");
+ Modification mod1 = new Modification(ModificationType.DELETE, attr);
+
+ Attribute attr2 = Attributes.empty(DESCRIPTION);
+ Modification mod2 = new Modification(ModificationType.REPLACE, attr2);
+
+ List<Modification> mods = new LinkedList<Modification>();
+ mods.add(mod1);
+ mods.add(mod2);
+
+ List<Modification> mods2 = new LinkedList<Modification>(mods);
+ replayModifies(entry, hist, mods, 12);
+ assertEquals(mods.size(), 2,
+ "DEL one value, del by Replace of the same attribute was not correct");
+ assertEquals(mods.get(0), mod1,
+ "DEL one value, del by Replace of the same attribute was not correct");
+ assertEquals(mods.get(1), mod2,
+ "DEL one value, del by Replace of the same attribute was not correct");
+
+ // Replay the same modifs again
+ replayModifies(entry, hist, mods2, 12);
+ assertEquals(mods2.size(), 2,
+ "DEL one value, del by Replace of the same attribute was not correct");
+
+ }
+
+ /**
* Test that a ADD and DEL of the same attribute same value done
* in a single operation correctly results in the value being kept.
*
@@ -994,7 +1048,7 @@
/**
* Test that conflict between a modify-add and modify-add for
- * multi-valued attributes are handled correctly.
+ * single-valued attributes are handled correctly.
*/
@Test()
public void addAndAddSingle() throws Exception
@@ -1005,7 +1059,7 @@
EntryHistorical hist = EntryHistorical.newInstanceFromEntry(entry);
/*
- * simulate a add of the description attribute done at time t10
+ * simulate a add of the displayName attribute done at time t10
*/
testModify(entry, hist, DISPLAYNAME, ModificationType.ADD,
"init value", 10, true);
@@ -1036,7 +1090,8 @@
}
/**
- * Test that conflict between add delete and add are handled correctly.
+ * Test that conflict between add, delete and add on aingle valued attribute
+ * are handled correctly.
*/
@Test()
public void addDelAddSingle() throws Exception
@@ -1047,13 +1102,13 @@
EntryHistorical hist = EntryHistorical.newInstanceFromEntry(entry);
/*
- * simulate a add of the description attribute done at time t1
+ * simulate a add of the displayName attribute done at time t1
*/
testModify(entry, hist, DISPLAYNAME, ModificationType.ADD,
"init value", 1, true);
/*
- * simulate a del of the description attribute done at time t3
+ * simulate a del of the displayName attribute done at time t3
* this should be processed normally
*/
testModify(entry, hist, DISPLAYNAME, ModificationType.DELETE,
@@ -1069,12 +1124,13 @@
}
/**
- * Test that conflict between add, add and delete are handled correctly.
+ * Test that conflict between add, add and delete on single valued attributes
+ * are handled correctly.
*
* This test simulate the case where a simple add is done on a first server
* and at a later date but before replication happens, a add followed by
* a delete of the second value is done on another server.
- * The test checks that the firs tvalue wins and stay in the entry.
+ * The test checks that the first value wins and stays in the entry.
*/
@Test()
public void addAddDelSingle() throws Exception
@@ -1085,13 +1141,13 @@
EntryHistorical hist = EntryHistorical.newInstanceFromEntry(entry);
/*
- * simulate a add of the description attribute done at time t1
+ * simulate a add of the displayName attribute done at time t1
*/
testModify(entry, hist, DISPLAYNAME, ModificationType.ADD,
"first value", 1, true);
/*
- * simulate a add of the description attribute done at time t2
+ * simulate a add of the displayName attribute done at time t2
* with a second value. This should not work because there is already
* a value
*/
--
Gitblit v1.10.0