From a141034da9ca73beb15ca36dcf8bd6c57f322905 Mon Sep 17 00:00:00 2001
From: Valery Kharseko <vharseko@3a-systems.ru>
Date: Wed, 09 Sep 2026 07:02:57 +0000
Subject: [PATCH] [#939] Pin isServerFailure() directly, so the conflict result codes are guarded (#960)
---
opendj-server-legacy/src/main/java/org/opends/server/replication/plugin/LDAPReplicationDomain.java | 6 +
opendj-server-legacy/src/test/java/org/opends/server/replication/plugin/IsServerFailureTest.java | 242 ++++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 247 insertions(+), 1 deletions(-)
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 7fa68c0..423fc2b 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
@@ -2865,12 +2865,16 @@
* 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.
+ * <p>
+ * Package private for the tests, which pin what it answers for every registered result
+ * code directly rather than through a running server.
*
* @param result the result code of a replayed operation
* @param serverErrorResultCode the result code this server puts on an internal error
* @return {@code true} if the operation failed on the server itself
*/
- private static boolean isServerFailure(ResultCode result, ResultCode serverErrorResultCode)
+ @VisibleForTesting
+ static boolean isServerFailure(ResultCode result, ResultCode serverErrorResultCode)
{
/*
* The result code the server puts on an internal error is configurable and is not
diff --git a/opendj-server-legacy/src/test/java/org/opends/server/replication/plugin/IsServerFailureTest.java b/opendj-server-legacy/src/test/java/org/opends/server/replication/plugin/IsServerFailureTest.java
new file mode 100644
index 0000000..d7dffe0
--- /dev/null
+++ b/opendj-server-legacy/src/test/java/org/opends/server/replication/plugin/IsServerFailureTest.java
@@ -0,0 +1,242 @@
+/*
+ * 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.replication.plugin;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.opends.server.replication.plugin.LDAPReplicationDomain.isServerFailure;
+
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.List;
+
+import org.assertj.core.api.SoftAssertions;
+import org.forgerock.opendj.ldap.ResultCode;
+import org.opends.server.DirectoryServerTestCase;
+import org.testng.annotations.DataProvider;
+import org.testng.annotations.Test;
+
+/**
+ * Tests which result codes {@link LDAPReplicationDomain} counts as a failure of the server
+ * rather than as a change only conflict resolution can apply.
+ * <p>
+ * {@code ds-cfg-server-error-result-code} is a plain integer which is not validated as a
+ * result code, so an administrator can set it to a code {@code solveNamingConflict()} owns.
+ * The replay must still leave such a change to conflict resolution: a replica which took
+ * every one of them for a failure of the server would retry them in place, hold its
+ * ServerState back over them and give up on them once the give-up delay is spent - for
+ * conflicts conflict resolution would have solved on the attempt it never got.
+ * <p>
+ * Only {@code UNWILLING_TO_PERFORM} has an end-to-end test of that rule, because each of the
+ * other codes would need a scenario which fails with exactly that code and which nothing but
+ * conflict resolution can apply - and {@code OBJECTCLASS_VIOLATION} has none a replayed
+ * operation reaches through the server's own paths, the schema checks which raise it being
+ * skipped for synchronization operations. Forcing a code with {@code ShortCircuitPlugin} does
+ * not stand in for one: the operation applies for real once that budget is spent, so such a
+ * test passes with the code removed from the set (#910, #938). So the rule is pinned here on
+ * the predicate itself, with no server in it.
+ * <p>
+ * The codes below are written out rather than read from {@code CONFLICT_RESULT_CODES}: a data
+ * provider fed by the set under test agrees with it whatever it holds, which is exactly what
+ * leaves the set unguarded today. Every result code which can be configured is then swept
+ * through the predicate against that list, so a code put into the set is as visible as one
+ * taken out of it, and the set itself stays private - nothing here reads it.
+ * <p>
+ * What this can not see is the set drifting away from the branches of
+ * {@code solveNamingConflict()} it is the union of: a code dropped from both at once leaves
+ * the table below still describing the predicate correctly.
+ * <p>
+ * Nor is the predicate the whole of the rule at the call site: {@code replay()} answers
+ * {@code SUCCESS} and {@code NO_OPERATION} before it consults this at all, so a change which
+ * came back with one of them never reaches the table below whatever is configured - that is
+ * issue #953. {@code BUSY} is answered ahead of it too, but only inside the retry loop: once
+ * the in-place attempts are spent, the predicate is asked about it after all.
+ */
+@SuppressWarnings("javadoc")
+public class IsServerFailureTest extends DirectoryServerTestCase
+{
+ /**
+ * The result codes the four {@code solveNamingConflict()} overloads dispatch on. Not
+ * everything they resolve: the ModifyDN overload answers {@code NOTHING_TO_DO} from three
+ * early returns before it ever looks at the result code.
+ */
+ private static final List<ResultCode> CONFLICT_CODES = Arrays.asList(
+ ResultCode.NO_SUCH_OBJECT, // 32, all four overloads
+ ResultCode.ENTRY_ALREADY_EXISTS, // 68, the Add and ModifyDN overloads
+ ResultCode.NOT_ALLOWED_ON_RDN, // 67, the Modify overload
+ ResultCode.NOT_ALLOWED_ON_NONLEAF, // 66, the Delete overload
+ ResultCode.UNWILLING_TO_PERFORM, // 53, the ModifyDN overload
+ ResultCode.OBJECTCLASS_VIOLATION); // 65, the ModifyDN overload
+
+ /**
+ * Codes conflict resolution does not solve. {@code OTHER} is the default of
+ * {@code ds-cfg-server-error-result-code}, and the property is an integer with no upper
+ * limit, so a value no result code is named after is a setting like any other.
+ * {@code UNAVAILABLE} is not here: it is a failure of the server whatever is configured,
+ * which {@link #unavailableIsAServerFailureWhateverIsConfigured()} covers.
+ */
+ private static final List<ResultCode> CODES_OUTSIDE_THE_CONFLICT_SET = Arrays.asList(
+ ResultCode.OTHER, // 80, the default
+ ResultCode.CONSTRAINT_VIOLATION, // 19
+ ResultCode.INSUFFICIENT_ACCESS_RIGHTS, // 50
+ ResultCode.valueOf(9999)); // a value no result code is named after
+
+ @DataProvider(name = "conflictResultCodes")
+ public Object[][] conflictResultCodes()
+ {
+ return rowsOf(CONFLICT_CODES);
+ }
+
+ @DataProvider(name = "codesOutsideTheConflictSet")
+ public Object[][] codesOutsideTheConflictSet()
+ {
+ return rowsOf(CODES_OUTSIDE_THE_CONFLICT_SET);
+ }
+
+ /**
+ * Every result code an administrator could put in {@code ds-cfg-server-error-result-code}.
+ * The property is an integer with {@code lower-limit="0"}, so {@code UNDEFINED} (-1) is not
+ * one of them - it is the null object of {@link ResultCode} rather than a setting - and a
+ * value no result code is named after is.
+ */
+ private static List<ResultCode> configurableCodes()
+ {
+ final List<ResultCode> codes = new ArrayList<>();
+ for (ResultCode code : ResultCode.values())
+ {
+ if (code.intValue() >= 0)
+ {
+ codes.add(code);
+ }
+ }
+ codes.add(ResultCode.valueOf(9999));
+ return codes;
+ }
+
+ private static Object[][] rowsOf(List<ResultCode> codes)
+ {
+ final Object[][] rows = new Object[codes.size()][];
+ for (int i = 0; i < codes.size(); i++)
+ {
+ rows[i] = new Object[] { codes.get(i) };
+ }
+ return rows;
+ }
+
+ /** A result which is neither {@code UNAVAILABLE} nor the configured code. */
+ @DataProvider(name = "resultAndAnotherConfiguredCode")
+ public Object[][] resultAndAnotherConfiguredCode()
+ {
+ return new Object[][] {
+ // a conflict under the default setting: the carve-out for CONFLICT_RESULT_CODES does
+ // not come into it, the result simply is not what this server puts on an internal error
+ { ResultCode.NO_SUCH_OBJECT, ResultCode.OTHER },
+ { ResultCode.ENTRY_ALREADY_EXISTS, ResultCode.OTHER },
+ { ResultCode.NOT_ALLOWED_ON_RDN, ResultCode.OTHER },
+ { ResultCode.NOT_ALLOWED_ON_NONLEAF, ResultCode.OTHER },
+ { ResultCode.UNWILLING_TO_PERFORM, ResultCode.OTHER },
+ { ResultCode.OBJECTCLASS_VIOLATION, ResultCode.OTHER },
+ // a conflict code while a different conflict code is configured - the deployment
+ // this whole class is written about, meeting a conflict it did not configure
+ { ResultCode.NO_SUCH_OBJECT, ResultCode.UNWILLING_TO_PERFORM },
+ // and a failure which is not the configured code is not this server's either
+ { ResultCode.OTHER, ResultCode.NO_SUCH_OBJECT },
+ { ResultCode.CONSTRAINT_VIOLATION, ResultCode.UNWILLING_TO_PERFORM },
+ { ResultCode.INSUFFICIENT_ACCESS_RIGHTS, ResultCode.OTHER },
+ { ResultCode.valueOf(9999), ResultCode.OTHER },
+ };
+ }
+
+ /**
+ * Sweeps every registered result code through the predicate against the list above. The
+ * named tables below say what the rule is on the codes which matter; this says the set
+ * behind it holds those and nothing else, so that a code put into
+ * {@code CONFLICT_RESULT_CODES} - which stops a storage failure carrying it from being
+ * retried - fails here as loudly as a code taken out of it.
+ * <p>
+ * It speaks for the predicate, not for what the replay hands it: {@code SUCCESS} and
+ * {@code NO_OPERATION} never reach the method, and what it answers for them is what the
+ * rest of this sweep is written from rather than a rule of its own.
+ * <p>
+ * Soft assertions so that a drift in both directions at once - a code added and another
+ * dropped - is reported in one run rather than one code per run.
+ */
+ @Test
+ public void everyRegisteredCodeIsAServerFailureExactlyWhenConflictResolutionDoesNotOwnIt()
+ {
+ final SoftAssertions softly = new SoftAssertions();
+ for (ResultCode code : configurableCodes())
+ {
+ softly.assertThat(isServerFailure(code, code))
+ .as("%s (%d) set as server-error-result-code is a failure of this server unless "
+ + "conflict resolution is the only thing which can solve it",
+ code, code.intValue())
+ .isEqualTo(!CONFLICT_CODES.contains(code));
+ }
+ softly.assertAll();
+ }
+
+ @Test(dataProvider = "conflictResultCodes")
+ public void conflictCodeConfiguredAsTheServerErrorCodeIsLeftToConflictResolution(ResultCode code)
+ {
+ assertThat(isServerFailure(code, code))
+ .as("%s (%d) set as server-error-result-code must not take a change away from "
+ + "solveNamingConflict(), which is the only thing which can solve it",
+ code, code.intValue())
+ .isFalse();
+ }
+
+ @Test(dataProvider = "codesOutsideTheConflictSet")
+ public void codeOutsideTheConflictSetConfiguredAsTheServerErrorCodeIsAServerFailure(ResultCode code)
+ {
+ assertThat(isServerFailure(code, code))
+ .as("%s (%d) set as server-error-result-code is this server reporting an internal "
+ + "error, and conflict resolution can not solve it",
+ code, code.intValue())
+ .isTrue();
+ }
+
+ /**
+ * "Whatever is configured" is every code which can be configured, rather than a handful of
+ * them: the predicate reads the configured code only after {@code UNAVAILABLE} has not
+ * matched, so a change which stops that short circuit for some code has to be looked for
+ * across all of them.
+ */
+ @Test
+ public void unavailableIsAServerFailureWhateverIsConfigured()
+ {
+ final SoftAssertions softly = new SoftAssertions();
+ for (ResultCode serverErrorResultCode : configurableCodes())
+ {
+ softly.assertThat(isServerFailure(ResultCode.UNAVAILABLE, serverErrorResultCode))
+ .as("the backend being offline or rebuilt is a failure of the server while "
+ + "server-error-result-code is %s (%d) just as much as it is by default",
+ serverErrorResultCode, serverErrorResultCode.intValue())
+ .isTrue();
+ }
+ softly.assertAll();
+ }
+
+ @Test(dataProvider = "resultAndAnotherConfiguredCode")
+ public void codeWhichIsNotTheConfiguredOneIsNotAServerFailure(
+ ResultCode result, ResultCode serverErrorResultCode)
+ {
+ assertThat(isServerFailure(result, serverErrorResultCode))
+ .as("%s (%d) is not what this server puts on an internal error - it puts %s (%d) - "
+ + "so it is the operation which failed rather than the server",
+ result, result.intValue(), serverErrorResultCode, serverErrorResultCode.intValue())
+ .isFalse();
+ }
+}
--
Gitblit v1.10.0