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 f3f31835bc..0310728649 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. ! --> 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. 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 ec71a15fec..8073270708 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 @@ private void applyGlobalConfiguration(final GlobalCfg globalConfig, final CoreAt 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 @@ public boolean isConfigurationChangeAcceptable(GlobalCfg configuration, configAcceptable = false; } + if (!isServerErrorResultCodeAcceptable(configuration, unacceptableReasons)) + { + configAcceptable = false; + } + if (!isSubordinateDNsAcceptable(configuration, unacceptableReasons)) { configAcceptable = false; @@ -431,6 +437,80 @@ public boolean isConfigurationChangeAcceptable(GlobalCfg configuration, 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. + *

+ * {@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}. + *

+ * 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. + *

+ * 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. + *

+ * 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 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 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 989d0e5e87..3d544100ef 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 @@ && getBackend().getBackendID().equals(backend.getBackendID())) { 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 CONFLICT_RESULT_CODES = Collections.unmodifiableSet( newHashSet( @@ -470,6 +471,23 @@ && getBackend().getBackendID().equals(backend.getBackendID())) { // 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. + *

+ * 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; @@ -1914,8 +1932,7 @@ SynchronizationProviderResult handleConflictResolution( } 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 @@ -2111,8 +2128,7 @@ SynchronizationProviderResult handleConflictResolution( modifyDNOperation.getOriginalEntry()); if (hist.addedOrRenamedAfter(ctx.getCSN())) { - return new SynchronizationProviderResult.StopProcessing( - ResultCode.NO_OPERATION, null); + return conflictResolutionFoundNothingToDo(modifyDNOperation); } } else @@ -2167,8 +2183,7 @@ SynchronizationProviderResult handleConflictResolution( { // Every modifications filtered in this operation: the operation // becomes a no-op - return new SynchronizationProviderResult.StopProcessing( - ResultCode.NO_OPERATION, null); + return conflictResolutionFoundNothingToDo(modifyOperation); } } else @@ -2929,7 +2944,7 @@ private void replayChangeAndTheChangesWaitingForIt( 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 @@ -3452,6 +3467,37 @@ private boolean updateError(CSN csn) } } + /** + * 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. + *

+ * 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 @@ -3468,10 +3514,10 @@ private boolean updateError(CSN csn) 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 359087bda1..a1a0c0d3a0 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 @@ ERR_CONFIG_FILE_MODIFY_REJECTED_DUE_TO_EVALUATION_FAILURE_766=Entry '%s' cannot 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 0000000000..019ca3d3e3 --- /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. + *

+ * 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 errorLogRecords(LocalizableMessage message) + { + final String record = "msgID=" + message.ordinal() + " msg=" + message; + final List 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 b791c2a7da..77d0acfdd3 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 @@ public void call() throws Exception /** * 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 43993fc70d..028b77f7e3 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 @@ public void setIsolationPolicy(IsolationPolicy policy) 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 29659d1a2b..94855bca1b 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,13 +91,22 @@ public void setUpLocal() throws Exception TestCaseUtils.initializeTestBackend(true); queue = new TestSynchronousReplayQueue(); + startDomain(newDomainConfig()); + gen = new CSNGenerator(201, 0); + } + + private DomainFakeCfg newDomainConfig() + { final DomainFakeCfg conf = new DomainFakeCfg(baseDN, 1, new TreeSet()); conf.setIsolationPolicy(IsolationPolicy.ACCEPT_ALL_UPDATES); + return conf; + } + + private void startDomain(DomainFakeCfg conf) throws Exception + { domain = MultimasterReplication.createNewDomain(conf, queue); domain.start(); - - gen = new CSNGenerator(201, 0); } @AfterMethod @@ -161,15 +172,15 @@ private ModifyDNMsg modDnMsg(Entry entry, String entryUUID, String parentUUID, C * {@code ds-cfg-server-error-result-code} is set to one of the result codes conflict * resolution owns. *

- * 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. *

* {@code UpdateOperationTest.changeConflictResolutionCanNotSolveOnTheServerErrorCodeIsRetried} * covers the other half: a change which fails with that same code and which conflict @@ -216,6 +227,141 @@ public void modifyDnConflictIsSolvedWhileTheServerErrorCodeIsOneOfTheConflictCod "a change which was applied must be recorded as replayed"); } + /** + * Test case for [Issue 953]: a change conflict resolution finds already applied is + * recorded as replayed. + *

+ * 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. + *

+ * 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. + *

+ * 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