From 6dc8f80457d5e4f3182fcee3adeda01928ae68f3 Mon Sep 17 00:00:00 2001
From: Valery Kharseko <vharseko@3a-systems.ru>
Date: Wed, 16 Sep 2026 04:55:22 +0000
Subject: [PATCH] [#953] Refuse a server-error-result-code which does not report a failure (#980)
---
opendj-server-legacy/src/messages/org/opends/messages/config.properties | 9
opendj-server-legacy/src/test/java/org/opends/server/replication/plugin/NamingConflictTest.java | 174 ++++++++++++++++-
opendj-maven-plugin/src/main/resources/config/xml/org/forgerock/opendj/server/config/GlobalConfiguration.xml | 8
opendj-server-legacy/src/test/java/org/opends/server/replication/plugin/DomainFakeCfg.java | 12 +
opendj-server-legacy/src/main/java/org/opends/server/core/CoreConfigManager.java | 82 ++++++++
opendj-server-legacy/src/test/java/org/opends/server/replication/UpdateOperationTest.java | 2
opendj-server-legacy/src/main/java/org/opends/server/replication/plugin/LDAPReplicationDomain.java | 74 ++++++-
opendj-server-legacy/src/test/java/org/opends/server/core/ServerErrorResultCodeTestCase.java | 200 ++++++++++++++++++++
8 files changed, 531 insertions(+), 30 deletions(-)
diff --git a/opendj-maven-plugin/src/main/resources/config/xml/org/forgerock/opendj/server/config/GlobalConfiguration.xml b/opendj-maven-plugin/src/main/resources/config/xml/org/forgerock/opendj/server/config/GlobalConfiguration.xml
index f3f3183..0310728 100644
--- a/opendj-maven-plugin/src/main/resources/config/xml/org/forgerock/opendj/server/config/GlobalConfiguration.xml
+++ b/opendj-maven-plugin/src/main/resources/config/xml/org/forgerock/opendj/server/config/GlobalConfiguration.xml
@@ -14,6 +14,7 @@
Copyright 2007-2010 Sun Microsystems, Inc.
Portions Copyright 2011-2016 ForgeRock AS.
+ Portions Copyright 2026 3A Systems, LLC.
! -->
<adm:managed-object name="global" plural-name="globals"
package="org.forgerock.opendj.server.config"
@@ -172,6 +173,13 @@
<adm:synopsis>
Specifies the numeric value of the result code when request
processing fails due to an internal server error.
+ The value must be a result code which reports a failure. The five codes
+ which report a success - 0 (success), 5 (compare false), 6 (compare true),
+ 14 (SASL bind in progress) and 16654 (no operation) - are refused: the
+ server would then report a request it could not process with a code that
+ says it succeeded, and a replication domain reading that code records a
+ change it never applied as replayed. A code the server does not know
+ reports a failure and is accepted.
</adm:synopsis>
<adm:default-behavior>
<adm:defined>
diff --git a/opendj-server-legacy/src/main/java/org/opends/server/core/CoreConfigManager.java b/opendj-server-legacy/src/main/java/org/opends/server/core/CoreConfigManager.java
index ec71a15..8073270 100644
--- a/opendj-server-legacy/src/main/java/org/opends/server/core/CoreConfigManager.java
+++ b/opendj-server-legacy/src/main/java/org/opends/server/core/CoreConfigManager.java
@@ -30,6 +30,7 @@
import org.forgerock.opendj.server.config.meta.GlobalCfgDefn.InvalidAttributeSyntaxBehavior;
import org.forgerock.opendj.server.config.meta.GlobalCfgDefn.SingleStructuralObjectclassBehavior;
import org.forgerock.opendj.server.config.server.GlobalCfg;
+import org.forgerock.util.annotations.VisibleForTesting;
import org.opends.server.api.AuthenticationPolicy;
import org.opends.server.api.LocalBackend;
import org.opends.server.loggers.CommonAudit;
@@ -196,7 +197,7 @@
core.addMissingRDNAttributes = globalConfig.isAddMissingRDNAttributes();
core.allowAttributeNameExceptions = globalConfig.isAllowAttributeNameExceptions();
core.syntaxEnforcementPolicy = convert(globalConfig.getInvalidAttributeSyntaxBehavior());
- core.serverErrorResultCode = ResultCode.valueOf(globalConfig.getServerErrorResultCode());
+ core.serverErrorResultCode = serverErrorResultCode(globalConfig.getServerErrorResultCode());
core.singleStructuralClassPolicy = convert(globalConfig.getSingleStructuralObjectclassBehavior());
core.notifyAbandonedOperations = globalConfig.isNotifyAbandonedOperations();
@@ -423,6 +424,11 @@
configAcceptable = false;
}
+ if (!isServerErrorResultCodeAcceptable(configuration, unacceptableReasons))
+ {
+ configAcceptable = false;
+ }
+
if (!isSubordinateDNsAcceptable(configuration, unacceptableReasons))
{
configAcceptable = false;
@@ -431,6 +437,80 @@
return configAcceptable;
}
+ /**
+ * Returns the result code to put on an operation an internal error prevented this
+ * server from processing, reading the configured value and falling back on
+ * {@link ResultCode#OTHER} - the default of the setting - when it does not report a
+ * failure.
+ * <p>
+ * {@link #isConfigurationChangeAcceptable} refuses such a value, so the fallback is
+ * what a configuration written before that - or edited outside the server - runs into:
+ * the server starts on the code its own default names rather than refusing to start,
+ * and says which value it ignored. The core configuration is applied before the error
+ * loggers are configured, so at start-up the warning goes where every start-up message
+ * goes - the standard output of the server, {@code logs/server.out} when it was started
+ * by {@code start-ds} - rather than into {@code logs/errors}.
+ * <p>
+ * Package private for the tests: a value the fallback is for never gets past
+ * {@link #isConfigurationChangeAcceptable}, so no change to a running server can reach
+ * it, and the tests pin it directly rather than through a start-up.
+ *
+ * @param configured the configured numeric result code
+ * @return the result code to put on an internal error
+ */
+ @VisibleForTesting
+ static ResultCode serverErrorResultCode(int configured)
+ {
+ final ResultCode resultCode = ResultCode.valueOf(configured);
+ if (resultCode.isExceptional())
+ {
+ return resultCode;
+ }
+ logger.warn(WARN_CONFIG_CORE_SERVER_ERROR_RESULT_CODE_NOT_A_FAILURE, configured, ResultCode.OTHER);
+ return ResultCode.OTHER;
+ }
+
+ /**
+ * Returns whether the configured result code reports a failure, which the code this
+ * server puts on an internal error has to.
+ * <p>
+ * The setting is a plain integer and used to accept any of them, including the five
+ * codes {@code ResultCode} registers as reporting a success. Each of those means
+ * something of its own to whoever reads a result code, and the reader then acts on that
+ * meaning while the operation it came from failed: the replay of a replication domain
+ * reads {@code NO_OPERATION} as "conflict resolution found the change already applied"
+ * and records a change which never reached the backend as replayed (issue #953), and
+ * {@code SUCCESS} has {@code LDAPReplicationDomain.synchronize()} both record it and
+ * publish the operation which failed to every other server of the topology. The
+ * configuration itself is a third reader: {@link #applyConfigurationChange} puts this
+ * code on a change to {@code cn=config} which failed to apply and keeps the new core
+ * attributes only when the result is {@code SUCCESS}, so a code of 0 reported that failure
+ * as a success and applied the change all the same. No reader can tell the two meanings
+ * apart once they are the same integer, which is why the value is refused here rather
+ * than worked around at each of them.
+ * <p>
+ * A code {@code ResultCode} does not know reports a failure - {@code valueOf()} answers
+ * an unknown code which does - so an administrator keeps the freedom to put a private
+ * code on an internal error.
+ *
+ * @param configuration the configuration to check
+ * @param unacceptableReasons where the reason is reported when the value is refused
+ * @return whether the configured result code is acceptable
+ */
+ private static boolean isServerErrorResultCodeAcceptable(
+ GlobalCfg configuration, List<LocalizableMessage> unacceptableReasons)
+ {
+ final int configured = configuration.getServerErrorResultCode();
+ final ResultCode resultCode = ResultCode.valueOf(configured);
+ if (resultCode.isExceptional())
+ {
+ return true;
+ }
+ unacceptableReasons.add(
+ ERR_CONFIG_CORE_SERVER_ERROR_RESULT_CODE_NOT_A_FAILURE.get(configured, resultCode));
+ return false;
+ }
+
private boolean isSubordinateDNsAcceptable(GlobalCfg configuration, List<LocalizableMessage> unacceptableReasons)
{
try
diff --git a/opendj-server-legacy/src/main/java/org/opends/server/replication/plugin/LDAPReplicationDomain.java b/opendj-server-legacy/src/main/java/org/opends/server/replication/plugin/LDAPReplicationDomain.java
index 0b87409..f8c6fcc 100644
--- a/opendj-server-legacy/src/main/java/org/opends/server/replication/plugin/LDAPReplicationDomain.java
+++ b/opendj-server-legacy/src/main/java/org/opends/server/replication/plugin/LDAPReplicationDomain.java
@@ -459,9 +459,10 @@
new AtomicLong(UNREPLAYED_CHANGE_ALERT_NEVER_SENT);
/**
* The result codes conflict resolution knows how to solve. The result code the server
- * puts on an internal error is configurable and is not validated as a result code, so
- * it could be set to one of these: it must never take a change away from
- * {@code solveNamingConflict()}, which is the only thing which can solve them.
+ * puts on an internal error is configurable, and every one of these reports a failure -
+ * which is all the configuration asks of it - so it can be set to one of them: it must
+ * never take a change away from {@code solveNamingConflict()}, which is the only thing
+ * which can solve them.
*/
private static final Set<ResultCode> CONFLICT_RESULT_CODES = Collections.unmodifiableSet(
newHashSet(
@@ -470,6 +471,23 @@
// solveNamingConflict(ModifyDNOperation) solves these two as well
ResultCode.UNWILLING_TO_PERFORM, ResultCode.OBJECTCLASS_VIOLATION));
+ /**
+ * The attachment which says that conflict resolution turned this operation into a
+ * no-op, so that {@link #replay} reads that decision rather than the result code which
+ * reports it.
+ * <p>
+ * The code conflict resolution reports for a no-op is {@code NO_OPERATION}, and the
+ * code this server puts on an internal error is a configuration knob: while nothing
+ * validated it, the two could be the same code, and every change an internal error kept
+ * out of the backend was then read as a change conflict resolution had found already
+ * applied and recorded in the ServerState - the silent divergence of issue #889, one
+ * branch earlier (issue #953). The configuration refuses a code which does not report a
+ * failure now, so they can not be the same code anymore; the decision travels on the
+ * operation all the same, so that what the replay acts on is what conflict resolution
+ * decided rather than a value an administrator owns.
+ */
+ private static final String CONFLICT_RESOLUTION_NO_OP = "replicationConflictResolutionNoOp";
+
private final PersistentServerState state;
private volatile boolean generationIdSavedStatus;
@@ -1915,8 +1933,7 @@
}
if (replayedEntryDN != null)
{
- return new SynchronizationProviderResult.StopProcessing(
- ResultCode.NO_OPERATION, null);
+ return conflictResolutionFoundNothingToDo(addOperation);
}
/* The parent entry may have been renamed here since the change was done
@@ -2112,8 +2129,7 @@
modifyDNOperation.getOriginalEntry());
if (hist.addedOrRenamedAfter(ctx.getCSN()))
{
- return new SynchronizationProviderResult.StopProcessing(
- ResultCode.NO_OPERATION, null);
+ return conflictResolutionFoundNothingToDo(modifyDNOperation);
}
}
else
@@ -2168,8 +2184,7 @@
{
// Every modifications filtered in this operation: the operation
// becomes a no-op
- return new SynchronizationProviderResult.StopProcessing(
- ResultCode.NO_OPERATION, null);
+ return conflictResolutionFoundNothingToDo(modifyOperation);
}
}
else
@@ -2926,7 +2941,7 @@
if (result != ResultCode.SUCCESS)
{
- if (result == ResultCode.NO_OPERATION)
+ if (isConflictResolutionNoOp(op))
{
// Pre-operation conflict resolution detected that the operation
// was a no-op. For example, an add which has already been
@@ -3450,6 +3465,37 @@
}
/**
+ * Stops an operation conflict resolution found nothing left to do for, and marks it so
+ * that the replay reads that decision off the operation rather than off the result code
+ * this answer carries.
+ *
+ * @param op the operation conflict resolution turned into a no-op
+ * @return the answer which stops the operation
+ */
+ private static SynchronizationProviderResult conflictResolutionFoundNothingToDo(PluginOperation op)
+ {
+ op.setAttachment(CONFLICT_RESOLUTION_NO_OP, Boolean.TRUE);
+ return new SynchronizationProviderResult.StopProcessing(ResultCode.NO_OPERATION, null);
+ }
+
+ /**
+ * Returns whether conflict resolution turned the replayed operation into a no-op, which
+ * says that the change it carries is in the data and can be recorded as replayed.
+ * <p>
+ * Only {@link #conflictResolutionFoundNothingToDo} answers {@code true} here. The
+ * result code that answer carries says the same thing, but it is a code the
+ * configuration can name as well - see {@link #CONFLICT_RESOLUTION_NO_OP} - and a
+ * change which failed must never be read as one which was already applied.
+ *
+ * @param op the operation which was replayed
+ * @return {@code true} if conflict resolution found nothing left to do for the change
+ */
+ private static boolean isConflictResolutionNoOp(Operation op)
+ {
+ return Boolean.TRUE.equals(op.getAttachment(CONFLICT_RESOLUTION_NO_OP));
+ }
+
+ /**
* Returns whether the provided result code reports a failure of this server rather
* than a change which can not be applied: the backend being offline or rebuilt
* (OPENDJ-49), or the storage failing to serve the operation.
@@ -3465,10 +3511,10 @@
static boolean isServerFailure(ResultCode result, ResultCode serverErrorResultCode)
{
/*
- * The result code the server puts on an internal error is configurable and is not
- * validated as a result code, so it may well be one conflict resolution knows how to
- * solve: such a setting must not take a change away from solveNamingConflict(), which
- * is the only thing which can solve them. A change it could not solve either is a
+ * The result code the server puts on an internal error is configurable and only has
+ * to report a failure, so it may well be one conflict resolution knows how to solve:
+ * such a setting must not take a change away from solveNamingConflict(), which is
+ * the only thing which can solve them. A change it could not solve either is a
* failure of the server all the same, which replay() acts on once conflict resolution
* has reported it.
*/
diff --git a/opendj-server-legacy/src/messages/org/opends/messages/config.properties b/opendj-server-legacy/src/messages/org/opends/messages/config.properties
index 359087b..a1a0c0d 100644
--- a/opendj-server-legacy/src/messages/org/opends/messages/config.properties
+++ b/opendj-server-legacy/src/messages/org/opends/messages/config.properties
@@ -12,6 +12,7 @@
#
# Copyright 2006-2010 Sun Microsystems, Inc.
# Portions Copyright 2013-2016 ForgeRock AS.
+# Portions Copyright 2026 3A Systems, LLC.
@@ -872,3 +873,11 @@
contained an expression '%s' that could not be evaluated: %s
ERR_CONFIG_FILE_READ_FAILED_DUE_TO_EVALUATION_FAILURE_767=Entry '%s' cannot be read because attribute '%s' \
contained an expression '%s' that could not be evaluated: %s
+ERR_CONFIG_CORE_SERVER_ERROR_RESULT_CODE_NOT_A_FAILURE_768=The value '%s' is not acceptable for attribute \
+ ds-cfg-server-error-result-code because result code '%s' does not report a failure. This server puts that code \
+ on the operations an internal error prevents it from processing, so a code which reports a success leaves a \
+ failed operation indistinguishable from one which succeeded: a replication domain would record a change it \
+ never applied as replayed, or publish an operation which failed to the whole topology
+WARN_CONFIG_CORE_SERVER_ERROR_RESULT_CODE_NOT_A_FAILURE_769=The value '%s' configured in attribute \
+ ds-cfg-server-error-result-code does not report a failure and is ignored: result code '%s' is used instead for \
+ the operations an internal error prevents this server from processing
diff --git a/opendj-server-legacy/src/test/java/org/opends/server/core/ServerErrorResultCodeTestCase.java b/opendj-server-legacy/src/test/java/org/opends/server/core/ServerErrorResultCodeTestCase.java
new file mode 100644
index 0000000..019ca3d
--- /dev/null
+++ b/opendj-server-legacy/src/test/java/org/opends/server/core/ServerErrorResultCodeTestCase.java
@@ -0,0 +1,200 @@
+/*
+ * The contents of this file are subject to the terms of the Common Development and
+ * Distribution License (the License). You may not use this file except in compliance with the
+ * License.
+ *
+ * You can obtain a copy of the License at legal/CDDLv1.0.txt. See the License for the
+ * specific language governing permission and limitations under the License.
+ *
+ * When distributing Covered Software, include this CDDL Header Notice in each file and include
+ * the License file at legal/CDDLv1.0.txt. If applicable, add the following below the CDDL
+ * Header, with the fields enclosed by brackets [] replaced by your own identifying
+ * information: "Portions copyright [year] [name of copyright owner]".
+ *
+ * Copyright 2026 3A Systems, LLC.
+ */
+package org.opends.server.core;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.forgerock.opendj.ldap.ModificationType.REPLACE;
+import static org.forgerock.opendj.ldap.requests.Requests.newModifyRequest;
+import static org.opends.messages.ConfigMessages.ERR_CONFIG_CORE_SERVER_ERROR_RESULT_CODE_NOT_A_FAILURE;
+import static org.opends.messages.ConfigMessages.WARN_CONFIG_CORE_SERVER_ERROR_RESULT_CODE_NOT_A_FAILURE;
+import static org.opends.server.protocols.internal.InternalClientConnection.getRootConnection;
+import static org.testng.Assert.assertEquals;
+
+import java.util.ArrayList;
+import java.util.List;
+
+import org.forgerock.i18n.LocalizableMessage;
+import org.forgerock.opendj.ldap.ResultCode;
+import org.opends.server.TestCaseUtils;
+import org.testng.annotations.AfterMethod;
+import org.testng.annotations.BeforeClass;
+import org.testng.annotations.DataProvider;
+import org.testng.annotations.Test;
+
+/**
+ * Tests the validation of {@code ds-cfg-server-error-result-code}, the result code this
+ * server puts on an internal error.
+ * <p>
+ * The setting says "this server failed". A code which does not mean a failure says
+ * something else to every reader of a result code, and the readers which act on what it
+ * usually means then act on an operation which failed: the replay of a replication domain
+ * reads {@code NO_OPERATION} as "conflict resolution found the change already applied"
+ * and records a change which never reached the backend as replayed (issue #953), and
+ * {@code SUCCESS} has {@code LDAPReplicationDomain.synchronize()} publish an operation
+ * which failed to the whole topology. Neither reader can tell the two apart once they are
+ * the same integer, so the value is kept out of the configuration instead.
+ */
+@SuppressWarnings("javadoc")
+public class ServerErrorResultCodeTestCase extends CoreTestCase
+{
+ /** The code to put back, or {@code null} when this test did not change it. */
+ private Integer resultCodeToRestore;
+
+ @BeforeClass
+ public void startServer() throws Exception
+ {
+ TestCaseUtils.startServer();
+ }
+
+ @AfterMethod
+ public void tearDown()
+ {
+ if (resultCodeToRestore != null)
+ {
+ final int resultCode = resultCodeToRestore;
+ resultCodeToRestore = null;
+ assertEquals(setServerErrorResultCode(resultCode).getResultCode(), ResultCode.SUCCESS,
+ "the server error result code could not be put back");
+ }
+ }
+
+ /**
+ * The result codes which do not report a failure: {@code ResultCode} registers exactly
+ * these five as success codes, and every other value - including one it does not know,
+ * which it answers with an unknown code of its own - reports a failure.
+ */
+ @DataProvider
+ public Object[][] resultCodesWhichAreNotAFailure()
+ {
+ return new Object[][] {
+ { ResultCode.SUCCESS },
+ { ResultCode.COMPARE_FALSE },
+ { ResultCode.COMPARE_TRUE },
+ { ResultCode.SASL_BIND_IN_PROGRESS },
+ { ResultCode.NO_OPERATION },
+ };
+ }
+
+ @Test(dataProvider = "resultCodesWhichAreNotAFailure")
+ public void serverErrorResultCodeCanNotBeSetToACodeWhichIsNotAFailure(ResultCode resultCode)
+ {
+ final ResultCode inForce = getServerErrorResultCode();
+ // Remembered although the change is expected to be refused: the day it is not, the
+ // failure must show here and not as a success code left in force for every test class
+ // which runs after this one.
+ resultCodeToRestore = inForce.intValue();
+
+ final ModifyOperation refusal = setServerErrorResultCode(resultCode.intValue());
+ assertEquals(refusal.getResultCode(), ResultCode.UNWILLING_TO_PERFORM,
+ "the server accepted " + resultCode + " as the code it puts on an internal error");
+ assertThat(refusal.getErrorMessage().toString())
+ .as("the refusal does not name the attribute and the code it turned down")
+ .contains(ERR_CONFIG_CORE_SERVER_ERROR_RESULT_CODE_NOT_A_FAILURE.get(resultCode.intValue(), resultCode)
+ .toString());
+ assertEquals(getServerErrorResultCode(), inForce,
+ "a refused change to the server error result code was applied all the same");
+ }
+
+ /**
+ * The start-up path does not go through the acceptability check, so a configuration
+ * written before the check existed - or edited outside the server - can still hold a
+ * code which does not report a failure: the server starts on the default of the setting
+ * rather than on that code, and rather than not at all. A code which reports a failure,
+ * registered or not, is taken as it is.
+ */
+ @Test(dataProvider = "resultCodesWhichAreNotAFailure")
+ public void aCodeWhichIsNotAFailureFallsBackOnTheDefaultAtStartUp(ResultCode resultCode)
+ {
+ assertEquals(CoreConfigManager.serverErrorResultCode(resultCode.intValue()), ResultCode.OTHER,
+ "the server started on " + resultCode + " as the code it puts on an internal error");
+ assertThat(errorLogRecords(
+ WARN_CONFIG_CORE_SERVER_ERROR_RESULT_CODE_NOT_A_FAILURE.get(resultCode.intValue(), ResultCode.OTHER)))
+ .as("the server did not say which value it ignored")
+ .isNotEmpty();
+ }
+
+ @Test
+ public void aCodeWhichIsAFailureIsTakenAsItIsAtStartUp()
+ {
+ assertEquals(CoreConfigManager.serverErrorResultCode(ResultCode.UNWILLING_TO_PERFORM.intValue()),
+ ResultCode.UNWILLING_TO_PERFORM);
+ assertEquals(CoreConfigManager.serverErrorResultCode(9999), ResultCode.valueOf(9999),
+ "the server did not start on a result code it does not know");
+ assertThat(errorLogRecords(WARN_CONFIG_CORE_SERVER_ERROR_RESULT_CODE_NOT_A_FAILURE.get(9999, ResultCode.OTHER)))
+ .as("the server warned about a code it took as it is")
+ .isEmpty();
+ }
+
+ @Test
+ public void serverErrorResultCodeCanBeSetToAnErrorCode()
+ {
+ resultCodeToRestore = getServerErrorResultCode().intValue();
+
+ assertEquals(setServerErrorResultCode(ResultCode.UNWILLING_TO_PERFORM.intValue()).getResultCode(),
+ ResultCode.SUCCESS, "the server refused an error result code");
+ assertEquals(getServerErrorResultCode(), ResultCode.UNWILLING_TO_PERFORM);
+ }
+
+ /**
+ * A code {@code ResultCode} does not know is a failure - it answers an unknown code
+ * which reports one - so the administrator keeps the freedom to put a private code on
+ * an internal error.
+ */
+ @Test
+ public void serverErrorResultCodeCanBeSetToACodeWhichIsNotRegistered()
+ {
+ resultCodeToRestore = getServerErrorResultCode().intValue();
+
+ assertEquals(setServerErrorResultCode(9999).getResultCode(), ResultCode.SUCCESS,
+ "the server refused a result code it does not know");
+ assertEquals(getServerErrorResultCode().intValue(), 9999);
+ }
+
+ private static ResultCode getServerErrorResultCode()
+ {
+ return DirectoryServer.getCoreConfigManager().getServerErrorResultCode();
+ }
+
+ /**
+ * Changes the code through an internal operation rather than through {@code ldapmodify},
+ * so that a refusal can be read in full: the result code and the reason the server gives
+ * for it, not only an exit code which is not zero.
+ */
+ private static ModifyOperation setServerErrorResultCode(int resultCode)
+ {
+ return getRootConnection().processModify(newModifyRequest("cn=config")
+ .addModification(REPLACE, "ds-cfg-server-error-result-code", String.valueOf(resultCode)));
+ }
+
+ /**
+ * Returns the records of the error log which carry the given message, by its ID and its
+ * text. The test writer is fed by both start-up publishers, so a message it holds is there
+ * more than once: what matters is whether it is there at all.
+ */
+ private static List<String> errorLogRecords(LocalizableMessage message)
+ {
+ final String record = "msgID=" + message.ordinal() + " msg=" + message;
+ final List<String> records = new ArrayList<>();
+ for (String logged : TestCaseUtils.ERROR_TEXT_WRITER.getMessages())
+ {
+ if (logged.contains(record))
+ {
+ records.add(logged);
+ }
+ }
+ return records;
+ }
+}
diff --git a/opendj-server-legacy/src/test/java/org/opends/server/replication/UpdateOperationTest.java b/opendj-server-legacy/src/test/java/org/opends/server/replication/UpdateOperationTest.java
index b791c2a..77d0acf 100644
--- a/opendj-server-legacy/src/test/java/org/opends/server/replication/UpdateOperationTest.java
+++ b/opendj-server-legacy/src/test/java/org/opends/server/replication/UpdateOperationTest.java
@@ -2356,7 +2356,7 @@
/**
* Test case for [Issue 889]: the result code the server puts on an internal error is
- * configurable and is not validated as a result code, so it can be set to one conflict
+ * configurable and only has to report a failure, so it can be set to one conflict
* resolution knows how to solve. Such a change is left to conflict resolution, and when
* that can not solve it either the change is retried as the storage failure it is -
* recording it as replayed after one attempt would be issue #889 again.
diff --git a/opendj-server-legacy/src/test/java/org/opends/server/replication/plugin/DomainFakeCfg.java b/opendj-server-legacy/src/test/java/org/opends/server/replication/plugin/DomainFakeCfg.java
index 43993fc..028b77f 100644
--- a/opendj-server-legacy/src/test/java/org/opends/server/replication/plugin/DomainFakeCfg.java
+++ b/opendj-server-legacy/src/test/java/org/opends/server/replication/plugin/DomainFakeCfg.java
@@ -18,6 +18,7 @@
package org.opends.server.replication.plugin;
import java.net.InetAddress;
+import java.util.Collections;
import java.util.SortedSet;
import java.util.TreeSet;
@@ -228,6 +229,17 @@
this.policy = policy;
}
+ /**
+ * Excludes attributes from replication, as {@code ds-cfg-fractional-exclude} does.
+ *
+ * @param values the values of the setting, each of the form {@code class:attr1,attr2}
+ * or {@code *:attr1,attr2}
+ */
+ public void addFractionalExclude(String... values)
+ {
+ Collections.addAll(fractionalExcludes, values);
+ }
+
@Override
public int getAssuredSdLevel()
{
diff --git a/opendj-server-legacy/src/test/java/org/opends/server/replication/plugin/NamingConflictTest.java b/opendj-server-legacy/src/test/java/org/opends/server/replication/plugin/NamingConflictTest.java
index 29659d1..94855bc 100644
--- a/opendj-server-legacy/src/test/java/org/opends/server/replication/plugin/NamingConflictTest.java
+++ b/opendj-server-legacy/src/test/java/org/opends/server/replication/plugin/NamingConflictTest.java
@@ -34,6 +34,7 @@
import org.forgerock.opendj.ldap.ResultCode;
import org.forgerock.opendj.server.config.meta.ReplicationDomainCfgDefn.IsolationPolicy;
import org.opends.server.TestCaseUtils;
+import org.opends.server.api.MonitorData;
import org.opends.server.core.DirectoryServer;
import org.opends.server.core.ModifyDNOperation;
import org.opends.server.core.ModifyOperationBasis;
@@ -48,6 +49,7 @@
import org.opends.server.replication.protocol.ModifyMsg;
import org.opends.server.replication.protocol.OperationContext;
import org.opends.server.replication.protocol.UpdateMsg;
+import org.opends.server.types.Attribute;
import org.opends.server.types.Entry;
import org.opends.server.types.OperationType;
import org.testng.annotations.AfterMethod;
@@ -89,15 +91,24 @@
TestCaseUtils.initializeTestBackend(true);
queue = new TestSynchronousReplayQueue();
-
- final DomainFakeCfg conf = new DomainFakeCfg(baseDN, 1, new TreeSet<String>());
- conf.setIsolationPolicy(IsolationPolicy.ACCEPT_ALL_UPDATES);
- domain = MultimasterReplication.createNewDomain(conf, queue);
- domain.start();
+ startDomain(newDomainConfig());
gen = new CSNGenerator(201, 0);
}
+ private DomainFakeCfg newDomainConfig()
+ {
+ final DomainFakeCfg conf = new DomainFakeCfg(baseDN, 1, new TreeSet<String>());
+ conf.setIsolationPolicy(IsolationPolicy.ACCEPT_ALL_UPDATES);
+ return conf;
+ }
+
+ private void startDomain(DomainFakeCfg conf) throws Exception
+ {
+ domain = MultimasterReplication.createNewDomain(conf, queue);
+ domain.start();
+ }
+
@AfterMethod
public void tearDown() throws Exception
{
@@ -161,15 +172,15 @@
* {@code ds-cfg-server-error-result-code} is set to one of the result codes conflict
* resolution owns.
* <p>
- * That setting is a plain integer which is not validated as a result code, so it can be
- * one of them. Here it is {@code UNWILLING_TO_PERFORM}, which is what a ModifyDN whose
- * new superior is - on this replica - a subordinate of the entry being moved comes back
- * with, and only conflict resolution can turn such a change into an operation which
- * applies: it resolves both DNs again from the entryUUIDs the message carries. Reading
- * the code as a failure of the server would take the change away from it - the message
- * would never be rewritten, so no attempt would apply any better than the first - and
- * the change would be retried in place, delivered again and finally given up on, with
- * the entry left where it was.
+ * That setting only has to report a failure, which every code conflict resolution owns
+ * does, so it can be one of them. Here it is {@code UNWILLING_TO_PERFORM}, which is what
+ * a ModifyDN whose new superior is - on this replica - a subordinate of the entry being
+ * moved comes back with, and only conflict resolution can turn such a change into an
+ * operation which applies: it resolves both DNs again from the entryUUIDs the message
+ * carries. Reading the code as a failure of the server would take the change away from
+ * it - the message would never be rewritten, so no attempt would apply any better than
+ * the first - and the change would be retried in place, delivered again and finally
+ * given up on, with the entry left where it was.
* <p>
* {@code UpdateOperationTest.changeConflictResolutionCanNotSolveOnTheServerErrorCodeIsRetried}
* covers the other half: a change which fails with that same code and which conflict
@@ -217,6 +228,141 @@
}
/**
+ * Test case for [Issue 953]: a change conflict resolution finds already applied is
+ * recorded as replayed.
+ * <p>
+ * The replay used to read that from the result code conflict resolution reports -
+ * {@code NO_OPERATION} - which is a code {@code ds-cfg-server-error-result-code} could
+ * name as well, so that every change an internal error kept out of the backend was read
+ * as one which was already in it and recorded in the ServerState. The configuration
+ * refuses a code which does not report a failure now, and the replay reads what
+ * conflict resolution decided rather than the code which carries it. This pins the
+ * other half: a change which really is already applied still ends up in the ServerState,
+ * or the replication server would keep sending it for good.
+ * <p>
+ * Recording the CSN alone would not pin it: a change the replay gives up on is recorded
+ * as replayed too, so that the changes which follow it get through, and that is the road
+ * an answer conflict resolution did not mark takes - {@code NO_OPERATION} is not a code
+ * {@code solveNamingConflict()} can do anything with. What tells the two roads apart is
+ * the count of the changes given up on, which a change which was already applied must
+ * not add to. One case per answer conflict resolution marks: this one for an add,
+ * {@link #modifyDnOlderThanARenameIsRecordedAsReplayed} for a ModifyDN and
+ * {@link #modifyOfExcludedAttributesOnlyIsRecordedAsReplayed} for a modify.
+ */
+ @Test
+ public void changeAlreadyAppliedIsRecordedAsReplayed() throws Exception
+ {
+ final Entry entry = createAndAddEntry("changeAlreadyApplied");
+ final String parentUUID = getEntryUUID(baseDN);
+ final String entryUUID = getEntryUUID(entry.getName());
+ final int givenUpBefore = failedReplayedUpdates();
+
+ /*
+ * An add of an entry whose entryUUID is already in the data: conflict resolution
+ * answers that this change has already been replayed, before the operation reaches
+ * the backend and comes back with ENTRY_ALREADY_EXISTS.
+ */
+ final CSN csn = gen.newCSN();
+ replayMsg(addMsg(entry, csn, parentUUID, entryUUID));
+
+ assertRecordedAsReplayedAndNotGivenUpOn(csn, givenUpBefore,
+ "a change which is already in the data");
+ }
+
+ /**
+ * Test case for [Issue 953], the ModifyDN half of
+ * {@link #changeAlreadyAppliedIsRecordedAsReplayed}: a ModifyDN older than a rename the
+ * entry has already been through is a change conflict resolution finds nothing left to
+ * do for, and it is recorded as replayed rather than given up on.
+ * <p>
+ * The entry is found again from its entryUUID under the name the newer rename gave it,
+ * and it is the historical information of the entry - renamed after the CSN of this
+ * change - which has conflict resolution cancel the operation.
+ */
+ @Test
+ public void modifyDnOlderThanARenameIsRecordedAsReplayed() throws Exception
+ {
+ final Entry entry = createAndAddEntry("modDnOlderThanRename");
+ final String parentUUID = getEntryUUID(baseDN);
+ final String entryUUID = getEntryUUID(entry.getName());
+ final int givenUpBefore = failedReplayedUpdates();
+
+ // Two consecutive CSNs, replayed in the reverse order.
+ final CSN older = gen.newCSN();
+ final CSN newer = gen.newCSN();
+ replayMsg(modDnMsg(entry, entryUUID, parentUUID, newer, "cn=renamedAfter"));
+ replayMsg(modDnMsg(entry, entryUUID, parentUUID, older, "cn=renamedBefore"));
+
+ assertTrue(entryExists(DN.valueOf("cn=renamedAfter," + TEST_ROOT_DN_STRING)),
+ "the older ModifyDN was applied over the newer one");
+ assertRecordedAsReplayedAndNotGivenUpOn(older, givenUpBefore,
+ "a ModifyDN older than a rename the entry has been through");
+ }
+
+ /**
+ * Test case for [Issue 953], the modify half of
+ * {@link #changeAlreadyAppliedIsRecordedAsReplayed}: on a fractional replica, a modify
+ * of attributes the replica does not replicate is a change conflict resolution finds
+ * nothing left to do for once it has filtered every modification out, and it is recorded
+ * as replayed rather than given up on.
+ */
+ @Test
+ public void modifyOfExcludedAttributesOnlyIsRecordedAsReplayed() throws Exception
+ {
+ // The domain of this class replicates everything: replace it with one which does not
+ // replicate the description of any entry.
+ MultimasterReplication.deleteDomain(baseDN);
+ final DomainFakeCfg conf = newDomainConfig();
+ conf.addFractionalExclude("*:description");
+ startDomain(conf);
+
+ final Entry entry = TestCaseUtils.addEntry(
+ "dn: cn=modOfExcludedAttributes," + TEST_ROOT_DN_STRING,
+ "objectClass: top",
+ "objectClass: person",
+ "cn: modOfExcludedAttributes",
+ "sn: Excluded");
+ final String entryUUID = getEntryUUID(entry.getName());
+ final int givenUpBefore = failedReplayedUpdates();
+
+ final CSN csn = gen.newCSN();
+ replayMsg(new ModifyMsg(csn, entry.getName(),
+ generatemods("description", "not replicated here"), entryUUID));
+
+ assertFalse(DirectoryServer.getEntry(entry.getName()).hasAttribute(
+ getServerContext().getSchema().getAttributeType("description")),
+ "an attribute this replica does not replicate was written by the replayed modify");
+ assertRecordedAsReplayedAndNotGivenUpOn(csn, givenUpBefore,
+ "a modify of attributes this replica does not replicate");
+ }
+
+ private void assertRecordedAsReplayedAndNotGivenUpOn(CSN csn, int givenUpBefore, String change)
+ {
+ assertTrue(domain.getServerState().cover(csn), change + " was not recorded as replayed");
+ assertEquals(failedReplayedUpdates(), givenUpBefore,
+ change + " was counted as one the replay could not apply");
+ }
+
+ /**
+ * Returns how many changes this domain has given up on, read off the monitoring it
+ * publishes: the domain of this class has no replication server, so its monitor entry is
+ * not looked up.
+ */
+ private int failedReplayedUpdates()
+ {
+ final MonitorData monitor = new MonitorData();
+ domain.addAdditionalMonitoring(monitor);
+ for (Attribute attribute : monitor)
+ {
+ if ("replayed-updates-failed".equals(attribute.getAttributeDescription().getNameOrOID()))
+ {
+ return Integer.parseInt(attribute.iterator().next().toString());
+ }
+ }
+ throw new AssertionError("replayed-updates-failed is not in the monitoring of the domain");
+ }
+
+ /**
* Test case for [Issue 955]: a ModifyDN whose entry and whose new superior are both
* gone from this replica is a conflict between a delete and this ModifyDN, and it is
* solved as such rather than left to be delivered again until this replica gives up on
--
Gitblit v1.10.0