[#1153] Parse the whole DN string, and build or split DN strings through DN instead of string operations

[vharseko]

Problem

DN.valueOf() silently drops the rest of the string after ;, after a closing quote, or after a hex string followed by a space. The server decodes the DN of every operation with it, so ldapdelete "uid=user.4,ou=People,dc=example,dc=com;uid=nobody" deletes uid=user.4. Around the parser, DN strings are also built or split by hand in many places: one connection handler named LDAP, internal makes every cn=monitor search fail with 80 (Other), a replicated base DN with : breaks the changelog cookie, and referral URLs for non-ASCII DNs are not UTF-8. See #1153 for the full list.

Decisions

The issue left two choices open:

• ; after an RDN is accepted as the RFC 2253 RDN separator: cn=a;dc=b is cn=a,dc=b. RFC 4514 allows other representations, but not dropping input, so any other content after an RDN is now rejected.
• An empty value is accepted wherever it appears, like the RFC 4514 grammar (string may be empty): cn=,dc=x, cn="",dc=x and cn=x+sn=,dc=y parse, as cn= and sn=+cn=x already did. Whether an attribute allows an empty value is left to its syntax. DNTestCase and DistinguishedNameEqualityMatchingRuleTest move these DNs from the illegal ones to the valid ones. A server before this change cannot read such a DN, which matters during a rolling upgrade: see Upgrade notes.

Change

The SDK parser (opendj-core):

• DN.decode() reads one separator (, or ;) after each RDN and throws ERR_DN_TRAILING_GARBAGE on anything else. AVA.valueOf() rejects trailing content (ERR_AVA_TRAILING_GARBAGE), as RDN.valueOf() already did.
• A hex string value ends at + too.
• An empty value is no longer rejected before , or a closing quote.
• A trailing lone \ is rejected (ERR_ATTR_SYNTAX_DN_TRAILING_ESCAPE). The other leniencies of item 5 are kept.
• The hex string is still kept as raw bytes (item 4, record only).

DN strings built or split by hand:

• NameAndOptionalUIDSyntaxImpl / UniqueMemberEqualityMatchingRuleImpl: a #' preceded by an unescaped \ belongs to the DN.
• MultiDomainServerState: the cookie is split at each unescaped ;, and each domain at its last :.
• cli.Utils.getAdministratorDN(): DN.child().
• Monitor DNs. A monitor instance name is a relative DN: the replication monitors and TestMonitorProvider use it to build a tree, so the name cannot simply become a single escaped RDN. Instead:
  • the providers that put a configured name into it escape that part with DN.escapeAttributeValue(): connection handler, client connections, LDAP and HTTP statistics, backend, storage, JE/PDB database, disk space, entry cache, and the replicated base DN of ReplicationMonitor and ReplicationServerDomain;
  • DirectoryServer.getMonitorProviderDN() makes the whole name the value of one RDN when it does not parse, or does not name an entry below cn=monitor (x\ parses as cn=x\,cn=monitor). A third-party provider can therefore no longer break the rest of cn=monitor.
• LDAPURL: percent-encodes the UTF-8 octets, including characters outside the BMP.
• UserAttr: splits at the first #.
• Control panel: the new entry panels and the duplicate entry panel build the DN with DN.child() (AbstractNewEntryPanel.getNewEntryDN()). Utilities.unescapeUtf8() leaves an escaped backslash alone, so cn=a\\41 is no longer shown as cn=a\A.
• JMXMBean.getJmxName() walks the RDNs. A single-valued RDN whose value is made of letters, digits and spaces keeps the name it always had. Any other value is percent-encoded, except that a space is still written as _ (a literal _ becomes %5F), so user-root, userroot and user_root no longer share one name. The SNMP extension finds the connection handlers by Connection_Handler and their statistics by _Statistics in these names, which is why the space stays _. The AVAs of a multi-valued RDN are joined with +, so cn=a+sn=b is now cn-a+sn-b (was cn-asn-b).
  • Compatibility: the JMX name of every monitor entry whose value holds other characters changes, the LDAP/LDAPS connection handlers among them: cn-LDAP_Connection_Handler_0000_port_1389 becomes cn-LDAP_Connection_Handler_0%2E0%2E0%2E0_port_1389. A JMX client that looks such an MBean up by its full name has to be updated.
• Backend and index configuration DNs in the control panel and the installer (DeleteBaseDNAndBackendTask, DeleteIndexTask, InstallerHelper.deleteBackend()) are built with DN.child() (Utilities.getBackendConfigDN(), Utilities.getIndexConfigDN()), so a backend ID or a VLV index name such as monitor,ou=a b names its own entry.
• MakeLDIF <_DN>: joins the RDNs instead of replacing every comma.
• TaskClient.getTaskDN(): DN.child().
• PatternDN (item 8): decodes \HH inside quotes, reads an empty value before ,, ; or +, ends a hex string at +, and rejects a trailing lone \ with the message DN.valueOf() uses.

Found on the way: PatternRDN.matchesRDN() compared the AVAs of a multi-valued RDN, in the order of the DN string, with the pattern types sorted by name, so the pattern sn=x+cn=y,dc=z did not match the DN sn=x+cn=y,dc=z. It now looks each AVA up by its type.

Upgrade

• Stored DNs written with ; (for example member: cn=a;dc=example,dc=com) were indexed under the key of the part before the first ;. They now read as the whole DN, so an indexed equality search misses them until the index is rebuilt. The same goes for a value stored while its syntax was not enforced that only now parses. Such a value cannot be recognized from the index keys (its old key is that of a legitimate cn=a), only by reading the entries. So rather than rebuilding every DN index on every server, a new 5.2.0 upgrade task (verifyAndRebuildDNEqualityIndexes) works as follows:
  • it finds the equality indexes whose matching rule is distinguishedNameMatch or uniqueMemberMatch in each enabled pluggable backend, reading the instance's schema files so that custom attributes count;
  • it runs verify-index --countErrors on them under each base DN;
  • it rebuilds them only where the verification reports errors.
    The verification computes each entry's keys with the new matching rules, so a missing key is an error. A stale old key is harmless, because the filter is evaluated again on the candidates. The task asks first, with yes as the default answer.
    When the indexes of a base DN cannot be verified, for example in a JDBC or Cassandra backend whose database is down during the upgrade, the task does not rebuild them: it warns, names the indexes and the base DN, and the upgrade goes on, leaving these indexes as they were for the administrator to verify once the backend can be read. A rebuild that fails, after the verification found missing keys, fails the upgrade, as the other index rebuilds of the upgrade do: it may have left the indexes untrusted, so that searches cannot use them. The indexes of the base DNs after it are then neither verified nor rebuilt, since the same cause would most likely make their rebuild fail too: each of them is named in a warning. To tell the two cases apart, VerifyIndex.countIndexErrors() returns -1 when the indexes could not be verified; the tool itself, with --countErrors, exits with 1 both then and on a single error, and keeps doing so.
• Docker: run.sh now runs upgrade -n --force, as the deb/rpm packages already do. With -n alone, the long tasks took their default answer, which defers index rebuilds that nobody runs afterwards in a container. The README says the upgrade may now take longer than the start period.
• The upgrade chapter of the installation guide (chap-upgrade.adoc) describes the above, the new JMX names of the monitor MBeans, and what is still manual: a stored DN with other trailing content no longer parses.
• ACIs: a DN with ; between RDNs now names the whole DN. An ACI whose DN has other trailing content no longer decodes; the server logs the ACI and enters lockdown mode when it loads it. A global ACI (ds-cfg-global-aci) of that form stops the server from starting instead, and has to be corrected in config.ldif.
• Rolling upgrade: a server before this change closes its replication connection when it receives a change to an entry whose DN holds an empty value, receives the same change again after reconnecting, and so stops replicating. Such entries must not be added or renamed until every server is upgraded; they cannot be imported into an older server either.

Tests

Every new test was first run against the code from master and failed for the reason it targets:

• opendj-core: DNTestCase, AVATestCase, DistinguishedNameEqualityMatchingRuleTest, UniqueMemberEqualityMatchingRuleTest, the new NameAndOptionalUIDSyntaxTest, and TemplateTagTestCase.
• opendj-cli: UtilsTestCase.
• opendj-server-legacy:
  • MultiDomainServerStateTest, LDAPURLTestCase;
  • new: PatternDNTest, UserAttrTest, JMXMBeanNameTest, TaskClientTaskDNTest, NewEntryDNTestCase, UnescapeUtf8TestCase, ConfigDNTestCase, ReplicationMonitorNameTest, ReplicationServerDomainMonitorNameTest;
  • new DNEqualityIndexesUpgradeTestCase: the index lookup on a config and a schema directory (disabled backend, backend ID with ,, Equality in capitals, a custom attribute SUP distinguishedName, a later schema file redefining it), the schema files listed in name order, the base DNs selected per backend, verify-then-rebuild on a PDB backend whose trusted member index has its keys removed (IndexKeyRemover, a test helper in the pluggable package): rebuilt once with its notice, then only verified, a failed rebuild that fails the upgrade and names the base DNs after it, and a warning without a rebuild, not a failure, for a base DN that no backend holds and for a backend whose verification throws;
  • new DeleteBackendTestCase: InstallerHelper.deleteBackend("a,b") deletes the configuration of that backend;
  • new SNMPMonitorConnectionHandlerNameTest: registers MBeans under the names getJmxName() gives a connection handler and its statistics, for IPv4, IPv6 and host names, and checks that SNMPMonitor finds both;
  • new MonitorDNTestCase. It adds through cn=config a connection handler named LDAP,ou=internal (legacy LDAP, LDAPConnectionHandler2 and HTTP), JE and PDB backends with the ID monitor,ou=a b, and an entry cache FIFO,ou=a b. It then checks that cn=monitor is searchable and that each provider owns its own entry, not a branch entry.

The round-2 tests (the trailing \ rows of PatternDNTest, the connection handler rows of JMXMBeanNameTest, the IP rows of SNMPMonitorConnectionHandlerNameTest) fail against the round-1 code. Mutants, each caught: no hex flush at the closing quote, no hex flush before an escaped character inside quotes (one PatternDNTest row each), a literal _ kept in an encoded JMX value (distinctDNs), and the backend config DN built by concatenation (ConfigDNTestCase). Upgrade task mutants, each caught by DNEqualityIndexesUpgradeTestCase: no attribute taken as comparing DNs, the instance's schema files not read, disabled backends kept, an inconsistent index not rebuilt, and an index rebuilt without verification.

Round-3 mutants, each caught: the HTTP statistics name not escaped (the HTTP row of MonitorDNTestCase), another result code or another message for a trailing \ (PatternDNTest, four rows each), a verification failure that fails the upgrade again, the backend IDs joined wrongly when the base DNs are selected, the schema files read in reverse order, an earlier attribute definition kept (DNEqualityIndexesUpgradeTestCase), and the installer building the backend config DN by concatenation (DeleteBackendTestCase).

Round-4 mutants, each caught by DNEqualityIndexesUpgradeTestCase: a failed rebuild that only warns, indexes that cannot be verified rebuilt anyway, rebuilt indexes reported as consistent, the schema files not sorted, and VerifyIndex.countIndexErrors() counting errors only with --countErrors.

Round-5 mutants, each caught by DNEqualityIndexesUpgradeTestCase: the base DNs after a failed rebuild verified and rebuilt anyway, the failed rebuild not rethrown once they are named, and a backend whose verification throws counted as one error in VerifyIndex.

For the monitor changes, each of the 14 guards was removed in turn (one mutant per escape, the scope check and the fallback), and each mutant turned the test red.

A full mvn -Pprecommit verify -pl opendj-server-legacy -am run passed in every module before opendj-server-legacy, including javadoc. In opendj-server-legacy, 33503 tests ran: 461 were skipped and 1 failed, FileChangelogDBTest.replicaDBLosingTheRaceAgainstShutdownIsNotCreated (its 90 s race timeout, while another build was running on the machine). Run on its own, the class is green.

Fixes #1153

[maximthomas]

praise: The parser now refuses what it used to drop, and the monitor-name escaping is proven guard by guard.

• DN.decode() reads exactly one ,/; after each RDN and throws ERR_DN_TRAILING_GARBAGE otherwise, which closes the ldapdelete "uid=user.4,...;uid=nobody" hole at the one place every operation's DN goes through.
• DirectoryServer.getMonitorProviderDN() falls back to a single-RDN name when an instance name does not parse or leaves cn=monitor, so a third-party provider can no longer break the whole cn=monitor subtree; MonitorDNTestCase turned red for each of the 14 removed guards.
• PatternRDN.matchesRDN() now looks each AVA up by type, fixing sn=x+cn=y,dc=z not matching itself.

---

issue (blocking): Percent-encoding the JMX name of every IP-bearing monitor drops all LDAP/LDAPS connection handlers from the SNMP dsApplIfOpsTable.

opendj-server-legacy/src/main/java/org/opends/server/config/JMXMBean.java:145-160, opendj-server-legacy/src/snmp/src/org/opends/server/snmp/DsMIBImpl.java:335-336, SNMPMonitor.java:124-125, :195-197, :231

LDAPConnectionHandler names a handler LDAP Connection Handler <ip> port <port> (:620-634, getHostAddress()), so the value always holds . or : and appendJmxRdn now registers its monitor MBean as Rdn2=cn-LDAP%20Connection%20Handler%200%2E0%2E0%2E0%20port%201389 instead of cn-LDAP_Connection_Handler_0000_port_1389. The SNMP extension finds connection handlers by substring on that name: DsMIBImpl.isAConnectionHandler requires Rdn2.contains("Connection_Handler"), SNMPMonitor the same plus "_Statistics". With an SNMP connection handler enabled, no LDAP/LDAPS handler gets a MIB row and getConnectionHandlersStatistics returns nothing. This ships: the snmp profile activates on opendmk/jdmkrt.jar, which is committed. SNMPSyncManagerV2AccessTest and SNMPTrapManagerTest never read dsApplIfOpsTable, which is why CI is green. Keeping _ for a space in the encoded branch keeps names distinct (a literal _ is %5F there, and a plain value never holds %) and keeps the SNMP matchers and the concat("_Statistics") at SNMPMonitor:231 working.

        for (byte b : ava.getAttributeValue().toByteArray())
        {
          char c = (char) (b & 0xFF);
          if (c == ' ')
          {
            // The SNMP extension matches "Connection_Handler" and "_Statistics" in these names.
            buffer.append('_');
          }
          else if (c < 0x80 && (isAlpha(c) || isDigit(c)))
          {
            buffer.append(c);
          }
          else
          {
            buffer.append('%').append(byteToHex(b));
          }
        }

Pin: a JMXMBeanNameTest row asserting that getJmxName(DN.valueOf("cn=LDAP Connection Handler 0.0.0.0 port 1389,cn=monitor")) contains Rdn2=cn-LDAP_Connection_Handler_0%2E0%2E0%2E0_port_1389; it is red against the current encoding. Or: update the matchers in DsMIBImpl and SNMPMonitor to the new form.

---

issue (non-blocking): PatternDN still accepts an unquoted value that ends in a lone \, which DN.valueOf() now rejects.

opendj-server-legacy/src/main/java/org/opends/server/authorization/dseecompat/PatternDN.java:1151-1156

At the end of input the unquoted branch flushes the hex chars and breaks without checking escaped, so cn=a\ decodes as the pattern cn=a. Before this PR both parsers read it that way; now the SDK throws ERR_ATTR_SYNTAX_DN_TRAILING_ESCAPE and PatternDN does not, against PatternDNTest's premise that a pattern reads like the DN DN.valueOf() reads. An access-log user-dn-equal-to / request-target-dn-* criterion or a wildcard ACI userdn/target with a trailing \ is accepted instead of failing.

        if (pos >= length)
        {
          if (escaped)
          {
            // A trailing lone backslash, which DN.valueOf() rejects too.
            throw illegalCharacter(dnString, pos - 1, '\\');
          }
          // This is the end of the DN and therefore the end of the value.
          // If there are any hex characters, then we need to deal with them accordingly.
          appendHexChars(dnString, valueString, hexChars);
          break;
        }

Pin: @Test(expectedExceptions = DirectoryException.class) on PatternDN.decode("cn=a\\"). It is red today, because decode returns cn=a, and green once the guard is in.

---

issue (non-blocking): Backend config DNs are still concatenated from an unescaped backend ID in the control panel and the installer (pre-existing).

opendj-server-legacy/src/main/java/org/opends/guitools/controlpanel/task/DeleteBaseDNAndBackendTask.java:422, :473, opendj-server-legacy/src/main/java/org/opends/guitools/controlpanel/task/DeleteIndexTask.java:243, opendj-server-legacy/src/main/java/org/opends/quicksetup/installer/InstallerHelper.java:336

ds-cfg-backend-id has no pattern, and this PR's MonitorDNTestCase uses the legal ID monitor,ou=a b. For that ID these sites build ds-cfg-backend-id=monitor,ou=a b,cn=Backends,cn=config, so the control-panel delete targets an entry that is not the backend. The code predates the PR, and none of these files is in the diff, so a follow-up issue is enough.

    DN dn = DN.valueOf("cn=Backends,cn=config").child("ds-cfg-backend-id", backend.getBackendID());

---

suggestion (non-blocking): Tell operators to rebuild the DN-syntax equality indexes, because stored values written with RFC 2253 ; now normalize differently.

opendj-core/src/main/java/org/forgerock/opendj/ldap/DN.java:281-288

Before this PR, DN.decode stopped at the first ;, so a value such as member: cn=a;dc=b passed the DN syntax and was indexed under the key of cn=a. After the upgrade, the value and the filter both normalize to cn=a,dc=b, so an indexed equality search misses the entry until rebuild-index runs. A stored value with trailing content no longer parses. Reach is limited to values that clients wrote with ; or malformed. A release note is enough, or an Upgrade task like the 3.5.0 one (Upgrade.java:403-405, rebuildIndexesNamed(... EMR_DN_NAME, ... EMR_UNIQUE_MEMBER_NAME ...)). Not run: the index miss was traced by reading, and the ACI-load path was not traced.

---

suggestion (non-blocking): Release-note that entries with an empty RDN value cannot be read by older peers.

opendj-core/src/main/java/org/forgerock/opendj/ldap/AVA.java:336-338

Accepting an empty value is the stated decision, and IA5 String allows one, so dc=,dc=example,dc=com can pass schema checking. An older DS/RS decodes the DN of every update message with DN.valueOf (ByteArrayScanner.nextDN), and that call throws on the empty value. During a rolling upgrade such an Add therefore does not replicate to the old servers, and the entry cannot be imported into an older version. Not run: this needs a mixed-version topology.

---

suggestion (non-blocking): Pin the new hex-pair flush at the closing quote in PatternDN.

opendj-server-legacy/src/test/java/org/opends/server/authorization/dseecompat/PatternDNTest.java:54-57, opendj-server-legacy/src/main/java/org/opends/server/authorization/dseecompat/PatternDN.java:1107-1111

Each quoted row ends in a plain character ("a\,b", "a\2Cb", "J\C3\B6rg"), so the regular-character branch always flushes the pending hex pairs before the quote. If the appendHexChars call at :1110 is deleted, all 24 cases still pass, and cn="ab\2C" decodes as cn=ab while DN.valueOf reads ab,.

      { "cn=\"ab\\2C\",dc=x" },
      { "cn=\"\\C3\\B6\\,\",dc=x" },

Pin: these two rows in patternsWithoutWildcard(). The first fails if the flush at :1110 is deleted, the second if the flush at :1096 is deleted.

---

suggestion (non-blocking): The six control-panel call sites of getNewEntryDN are not covered by any test.

opendj-server-legacy/src/main/java/org/opends/guitools/controlpanel/ui/NewUserPanel.java:316

NewEntryDNTestCase calls only the static AbstractNewEntryPanel.getNewEntryDN. No test references NewUserPanel, NewGroupPanel, NewOrganizationPanel, NewOrganizationalUnitPanel, NewDomainPanel or DuplicateEntryPanel, so changing one call site back to attr + "=" + value + "," + parent leaves every test green. The tests do cover the escaping, which lives in the helper. Pinning the call sites would need a headless Swing harness that the tree does not have, so this is your call.

---

nitpick (non-blocking): JMX names of a multi-valued RDN with plain values do change (+ between the AVAs), but the description says such values keep their name.

opendj-server-legacy/src/main/java/org/opends/server/config/JMXMBean.java:131

cn=a+sn=b went from cn-asn-b to cn-a+sn-b. The + is right and documented in the appendJmxRdn javadoc, because without it cn=a+sn=b and cn=as+n=b would share a name. Only the description's wording ("keeps the name it always had") needs the qualifier "single-valued RDN". No in-tree producer registers a multi-AVA monitor or config DN, and plainNamesAreUnchanged has single-AVA rows only.

[vharseko]

Thanks for the review. Round 2 is a3c43b4. The branch is rebased onto the current master (92ea60a, which brings #1162, #1163 and #1164); the round-1 commit is unchanged by the rebase, now 80ee6b9.

JMX names vs. SNMP (blocking): fixed. The encoded branch now writes a space as _, and a literal _ stays %5F, so the names stay distinct. cn=LDAP Connection Handler 0.0.0.0 port 1389,cn=monitor is Rdn2=cn-LDAP_Connection_Handler_0%2E0%2E0%2E0_port_1389, and its statistics entry ends in _Statistics.

• JMXMBeanNameTest: your row plus the statistics name, and a new distinctDNs group (a.b c, a.b_c, a.b%20c, a.b%5Fc).
• New SNMPMonitorConnectionHandlerNameTest ties the naming to the real matchers. It registers MBeans under the names getJmxName() gives a handler and its statistics (IPv4, IPv6, host name), then checks that SNMPMonitor.getConnectionHandlers(), getConnectionHandlersStatistics(), getConnectionHandlerStatistics() and getConnectionHandler() find them. Against the round-1 JMXMBean the IPv4, LDAPS and IPv6 rows fail and the localhost row passes. Your two JMXMBeanNameTest rows fail there too.
• Mutant: with a literal _ kept in the encoded branch, distinctDNs fails (user root/user_root, a.b c/a.b_c).
• The description now says what still changes: the handler names go from 0000 to 0%2E0%2E0%2E0, so a JMX client that looks the MBean up by its full name has to follow. That is the cost of injective names.

PatternDN trailing \: fixed. The guard throws with the SDK's ERR_ATTR_SYNTAX_DN_TRAILING_ESCAPE, the same message DN.valueOf() uses. PatternDNTest.patternWithATrailingBackslashIsRejected covers cn=a\, cn=\, cn=a,dc=x\ and the wildcard case cn=a*\. All four rows fail against the round-1 PatternDN.

Backend config DNs: fixed in this PR rather than in a follow-up, because it is the same defect class. Utilities.getBackendConfigDN() and Utilities.getIndexConfigDN() build them with DN.child(), and the five call sites use them: DeleteBaseDNAndBackendTask:422/:473, DeleteIndexTask (the VLV ds-cfg-name and attribute ds-cfg-attribute DNs too) and InstallerHelper.deleteBackend(). ConfigDNTestCase checks the helpers with monitor,ou=a b, a+sn=b, a\b and a. With the backend DN built by concatenation again, 8 of its 10 cases fail; only userRoot passes. As with the new-entry panels, the call sites themselves have no Swing-free test.

Rebuilding DN indexes after ; values: an upgrade task that verifies first. A ; value cannot be told apart by its index key, which is that of a legitimate cn=a; only the entries reveal it. An Upgrade task like the 3.5.0 one would therefore rebuild member/uniqueMember on every server, and the deb/rpm packages run upgrade -n --force. So the new 5.2.0 task verifyAndRebuildDNEqualityIndexes works as follows:

• it looks up the equality indexes with distinguishedNameMatch/uniqueMemberMatch in every enabled pluggable backend, including attributes from the instance's custom schema files;
• it runs verify-index --countErrors on them per base DN;
• it rebuilds only where errors are reported.

Complete-mode verification computes each entry's keys with the new rules, so the missing key of a ; value, or of a value that only now parses, counts as an error. The stale old key is harmless, since candidates are filtered again. The default answer is yes. run.sh in the Docker image now uses upgrade -n --force like the native packages, so the container does not just defer the task.

• DNEqualityIndexesUpgradeTestCase covers the index lookup (disabled backend, a backend ID with ,, a custom SUP distinguishedName attribute). It also covers verify-then-rebuild on a PDB backend whose trusted member index has had its keys removed: rebuilt once, only verified the second time. Five mutants of the task are caught.

Empty RDN values vs. older peers: documented in the upgrade chapter (chap-upgrade.adoc), as an IMPORTANT note in "To Upgrade Replicated Servers", next to the ACI and stored-DN notes. While tracing it:

• The empty-value case is worse than one Add not replicating. An older server decodes the DN inside Session.receive() (LDAPUpdateMsg → ByteArrayScanner.nextDN()), so the DataFormatException closes its replication session. After it reconnects it is sent the same change again, so it stops replicating altogether. The note says so, and says not to add or rename such entries until every server is upgraded. This is traced by reading; I did not run a mixed topology.
• The ACI load path: a DN with ; between RDNs now names the whole DN. An ACI with other trailing content no longer decodes, and AciListenerManager logs it and puts the server in lockdown mode. The note covers that too and links the lockdown section.

Closing-quote flush: pinned with your two rows, cn="ab\2C",dc=x and cn="\C3\B6\,",dc=x. With the appendHexChars call at the closing quote removed, only the first row fails. With the one before an escaped non-hex character removed, only the second row fails.

Control-panel call sites without tests: left as is. There is no headless Swing harness, and the escaping is pinned in getNewEntryDN().

Description wording: fixed. It now says "single-valued RDN" and gives the cn-a+sn-b example.

[maximthomas]

praise: Round 2 closes every round-1 finding where the bug is, and pins each fix with a test that fails against the round-1 code.

• JMXMBean.appendJmxRdn still writes _ for a space in the encoded branch, so the SNMP extension's Connection_Handler / _Statistics match survives; SNMPMonitorConnectionHandlerNameTest pins it for IPv4, IPv6 and host names.
• UpgradeTasks.verifyAndRebuildDNEqualityIndexes runs verify-index first, rebuilds only where it reports errors, and skips when the DN indexes are already queued for a rebuild.
• PatternDN now rejects a trailing \ as DN.valueOf() does, with the patternsWithATrailingBackslash rows.

---

issue (non-blocking): The HTTP connection handler's statistics monitor name is still not escaped.

opendj-server-legacy/src/main/java/org/opends/server/protocols/http/HTTPConnectionHandler.java:477

LDAPConnectionHandler.java:683 and LDAPConnectionHandler2.java:753 now build DN.escapeAttributeValue(handlerName) + " Statistics", and ConnectionHandlerMonitor / ClientConnectionMonitorProvider escape the name for every handler, HTTP included. For an HTTP handler whose cn holds , and = (e.g. HTTP,ou=x), the handler entry lands at cn=HTTP\,ou\=x 0.0.0.0 port 8080,cn=monitor, but the statistics entry parses as cn=HTTP,ou=x 0.0.0.0 port 8080 Statistics,cn=monitor, two levels down, so ConfigFromConnection.isConnectionHandler (parent().equals(monitorDN)) never finds it. This is not a regression: the entry was already misplaced at the base. DN is already imported there.

statTracker = new HTTPStatistics(DN.escapeAttributeValue(handlerName) + " Statistics");

Pin: add HTTPConnectionHandler to MonitorDNTestCase.connectionHandlerClasses if it can start there.

---

question (non-blocking): Is the DN-index verification meant to scan enabled JDBC and Cassandra backends, and should the upgrade fail when their remote store is unreachable?

opendj-server-legacy/src/main/java/org/opends/server/tools/upgrade/UpgradeUtils.java:352

getDNEqualityIndexedAttributesPerBackend selects every enabled ds-cfg-pluggable-backend, and JDBCBackendConfiguration.xml / CASBackendConfiguration.xml extend pluggable-backend. verifyAndRebuildIndexes rebuilds on any nonzero verify result and throws ERR_UPGRADE_PERFORMING_POST_TASKS_FAIL when the rebuild fails. With the database down at upgrade time, the upgrade exits with an error after the version has been bumped, the later post-upgrade tasks are postponed, and the Docker run.sh never marks the container healthy. If that is intended (chap-upgrade says "every enabled backend"), this is fine as is. If remote stores are out of scope, skip them or warn instead of failing. This becomes an issue if the JDBC/CAS storages cannot be opened by an offline verify-index at all, because every such upgrade would then rebuild or fail. Not run: that needs a JDBC/Cassandra instance.

---

suggestion (non-blocking): The upgrade chapter does not say that monitor MBeans get new JMX names.

opendj-server-legacy/src/main/java/org/opends/server/config/JMXMBean.java:135, opendj-doc-generated-ref/src/main/asciidoc/install-guide/chap-upgrade.adoc:300-335

The PR description states the rename, and JMXMBeanNameTest pins it: cn-LDAP_Connection_Handler_0000_port_1389 becomes cn-LDAP_Connection_Handler_0%2E0%2E0%2E0_port_1389, and replication monitor names with ( or : change too. The new upgrade section covers DN indexes, ACIs and empty RDN values, but not this. A JMX consumer that keys on the ObjectName (exporter rules, check_jmx, dashboards) loses these MBeans after the upgrade of a default install. The in-repo SNMP extension keeps working.

The JMX names of monitor MBeans whose RDN value holds characters other than letters, digits and spaces change:
those characters are now percent-encoded instead of dropped. For example,
`cn-LDAP_Connection_Handler_0000_port_1389` becomes `cn-LDAP_Connection_Handler_0%2E0%2E0%2E0_port_1389`.
Update JMX monitoring rules that match these names after the upgrade.

---

suggestion (non-blocking): No test reaches postUpgrade() of verifyAndRebuildDNEqualityIndexes, so the skip, the backend-ID join and the per-base-DN loop are unpinned.

opendj-server-legacy/src/main/java/org/opends/server/tools/upgrade/UpgradeTasks.java:697-730, opendj-server-legacy/src/test/java/org/opends/server/tools/upgrade/DNEqualityIndexesUpgradeTestCase.java:219

DNEqualityIndexesUpgradeTestCase calls only the two static helpers. UpgradeTestCase stops at ERR_UPGRADE_VERSION_UP_TO_DATE before any task runs. A mutant that keys the :718-724 join wrongly, so that every backend hits baseDNs == null -> continue, stays green, which means the task could do nothing on a real upgrade. Moving the selection into a helper makes it testable:

static Map<String, Set<String>> getDNEqualityIndexesToVerify(
    final Map<String, Set<String>> attributesPerBackend, final Map<String, Set<String>> baseDNsPerBackend)
{
  final Map<String, Set<String>> attributesPerBaseDN = new TreeMap<>();
  for (final Map.Entry<String, Set<String>> backend : attributesPerBackend.entrySet())
  {
    final Set<String> baseDNs = baseDNsPerBackend.get(backend.getKey());
    if (baseDNs != null)
    {
      for (final String baseDN : baseDNs)
      {
        attributesPerBaseDN.put(baseDN, backend.getValue());
      }
    }
  }
  return attributesPerBaseDN;
}

Pin: attributes {"a,b": [member], "c": [uniqueMember]} and base DNs {"a,b": [o=a]} give exactly {o=a: [member]}. The :703 skip and the register("5.2.0", …) have no cheap pin.

---

suggestion (non-blocking): The four call sites moved to Utilities.getBackendConfigDN are unpinned; ConfigDNTestCase tests only the helper.

opendj-server-legacy/src/main/java/org/opends/quicksetup/installer/InstallerHelper.java:336, opendj-server-legacy/src/main/java/org/opends/guitools/controlpanel/task/DeleteBaseDNAndBackendTask.java:422, :473, opendj-server-legacy/src/main/java/org/opends/guitools/controlpanel/task/DeleteIndexTask.java:243

Reverting any caller to DN.valueOf("ds-cfg-backend-id=" + id + ",cn=Backends,cn=config") keeps the suite green. InstallerHelper.deleteBackend needs no GUI because it deletes through the server's configuration handler:

// after adding a disabled backend config entry with ds-cfg-backend-id "a,b"
// (the shape DNEqualityIndexesUpgradeTestCase already adds)
new InstallerHelper().deleteBackend("a,b");
assertFalse(DirectoryServer.entryExists(DN.valueOf("ds-cfg-backend-id=a\\,b,cn=Backends,cn=config")));

The old concatenation turns it red. The two control-panel tasks are Swing-bound.

---

suggestion (non-blocking): The sort order and overwrite flag of readSchemaFiles are unpinned.

opendj-server-legacy/src/test/java/org/opends/server/tools/upgrade/DNEqualityIndexesUpgradeTestCase.java:101

The fixture writes only 99-user.ldif, and its OID is absent from the core schema. Arrays.sort on one file and addSchema(entry, false) with no conflict behave the same as the real code, so dropping either stays green. In production, a later file that redefines an attribute's EQUALITY rule decides whether its index is verified.

# 10-first.ldif, written before 99-user.ldif (which keeps SUP distinguishedName)
dn: cn=schema
objectClass: top
objectClass: ldapSubentry
objectClass: subschema
attributeTypes: ( 1.3.6.1.4.1.26027.1.999.1153 NAME 'myManager' EQUALITY caseIgnoreMatch SYNTAX 1.3.6.1.4.1.1466.115.121.1.15 )

Pin: myManager is selected. Swap the two definitions between the files and it is not. overwrite=false turns the first assertion red.

---

suggestion (non-blocking): patternWithATrailingBackslashIsRejected pins the rejection but not the message or result code that commit a3c43b4 says match DN.valueOf().

opendj-server-legacy/src/test/java/org/opends/server/authorization/dseecompat/PatternDNTest.java:118

A mutant that throws another message or result code stays green.

@Test(dataProvider = "patternsWithATrailingBackslash")
public void patternWithATrailingBackslashIsRejected(String pattern) throws Exception
{
  try
  {
    PatternDN.decode(pattern);
    fail("expected a DirectoryException for " + pattern);
  }
  catch (DirectoryException e)
  {
    assertThat(e.getResultCode()).isEqualTo(ResultCode.INVALID_DN_SYNTAX);
    assertThat(e.getMessageObject().toString())
        .isEqualTo(ERR_ATTR_SYNTAX_DN_TRAILING_ESCAPE.get(pattern).toString());
  }
}

---

suggestion (non-blocking): The finally block of the verify-then-rebuild case hides a setup failure behind a NO_SUCH_OBJECT assertion.

opendj-server-legacy/src/test/java/org/opends/server/tools/upgrade/DNEqualityIndexesUpgradeTestCase.java:224-230

If addEntry(indexBranch) or addEntry(memberIndex) fails, deleteEntry(memberIndex.getName()) asserts SUCCESS and throws a new AssertionError. That error replaces the real cause, and the two remaining deletes are skipped, which leaves the backend config entry in place for the rest of the class.

finally
{
  setBackendEnabled(false);
  for (final DN dn : Arrays.asList(memberIndex.getName(), indexBranch.getName(), backend.getName()))
  {
    if (DirectoryServer.entryExists(dn))
    {
      TestCaseUtils.deleteEntry(dn);
    }
  }
}

---

nitpick (non-blocking): The new run.sh comment says --force is needed for verifying indexes, but upgrade -n alone already verifies.

opendj-packages/opendj-docker/run.sh:123-124

The verify task asks confirmYN(summary, YES) (UpgradeTasks.java:692), and chap-upgrade itself says "upgrade --no-prompt performs the verification too". --force only changes the default-no answers.

# --force accepts the tasks whose default answer is no, such as rebuilding indexes:
# nobody is there to run them by hand afterwards, as the native packages do too

[vharseko]

Thanks for the approval and the second round. Round 3 is afd91ea. The branch is rebased onto the current master (85b28b3, which brings #1165, #1167, #1168, #1169 and #1175); the rebase leaves the round-1 and round-2 commits unchanged, now 9b4e723 and aa0a36b.

HTTP statistics monitor name: fixed. HTTPConnectionHandler now builds DN.escapeAttributeValue(handlerName) + " Statistics", as both LDAP handlers do. MonitorDNTestCase.connectionHandlerClasses has an HTTP row: the handler LDAP,ou=internal starts on a free port, and its handler, client connections and statistics entries each sit right below cn=monitor. With the old line back, only the statistics assertion of the HTTP row fails.

DN index verification on JDBC/Cassandra backends: kept in scope, but it no longer fails the upgrade. These backends hold DN values too, and when their database can be read their indexes can miss the same keys, so skipping them would leave the problem unseen. What changes is the failure path. When verify-index fails and the rebuild fails as well, the task no longer throws ERR_UPGRADE_PERFORMING_POST_TASKS_FAIL. It logs the cause, warns with the indexes and the base DN (WARN_UPGRADE_VERIFY_DN_EQUALITY_INDEXES_FAILED), and the upgrade goes on with the next base DN and the remaining post-upgrade tasks. The server works without those keys, so a container whose database is down still finishes its upgrade and turns healthy. The step moved into UpgradeTasks.verifyAndRebuildOrWarn(), so it can be tested. DNEqualityIndexesUpgradeTestCase.indexesThatCannotBeVerifiedOnlyWarn uses a base DN that no backend holds: verifyAndRebuildIndexes throws ClientException there, and verifyAndRebuildOrWarn sends a WARNING notification instead. The verify-then-rebuild case now also goes through verifyAndRebuildOrWarn and checks the "match the entries" notice. The upgrade chapter tells administrators to verify such a backend by hand once it can be read. I did not run this against a real JDBC or Cassandra instance.

JMX names in the upgrade chapter: added, after the DN list, with your example. It also says that the replication monitor names, which hold (, ) and :, change, and that the SNMP connection handler still finds the connection handlers.

Base DN selection: extracted as UpgradeTasks.getDNEqualityIndexesToVerify(), as you suggested. eachBaseDNGetsTheIndexesOfItsBackend gives {"a,b": [member], "c": [uniqueMember]} with base DNs {"a,b": [o=a, o=b], "d": [o=d]} and expects exactly {o=a: [member], o=b: [member]}. The wrong-key mutant fails it.

InstallerHelper.deleteBackend(): pinned by the new DeleteBackendTestCase. It adds a disabled PDB backend with the ID a,b and its cn=Index branch, calls deleteBackend("a,b"), and expects both entries to be gone. With the concatenated DN back, the test fails. The two control-panel tasks are still Swing-bound.

Schema file order and overwrite: pinned by aLaterSchemaFileDecidesTheMatchingRule, using your two definitions of myManager. With caseIgnoreMatch in 10-first.ldif and SUP distinguishedName in 99-user.ldif, myManager is selected; swapped, it is not. The files are written in reverse name order. Both mutants fail it: reading the files in reverse order, and addSchema(entry, false).

Trailing \ result code and message: pinned. patternWithATrailingBackslashIsRejected checks INVALID_DN_SYNTAX and ERR_ATTR_SYNTAX_DN_TRAILING_ESCAPE.get(pattern). For the three rows without a wildcard it also checks that DN.valueOf(pattern) throws the same message. A mutant with another result code fails all four rows, and so does a mutant with another message.

Test cleanup: fixed. The finally block deletes only the entries that exist, so a failed setup surfaces as itself and the backend configuration does not stay behind for the rest of the class.

run.sh comment: fixed. It now says --force performs the tasks whose default answer is no, such as rebuilding indexes.

Tests: MonitorDNTestCase, PatternDNTest, DNEqualityIndexesUpgradeTestCase, DeleteBackendTestCase, ConfigDNTestCase, JMXMBeanNameTest and UpgradeTestCase pass. Eight mutants, one per point above, each turn its test red: the HTTP name, the result code, the message, the rethrow, the backend ID join, the file order, the overwrite flag and the installer DN.

[maximthomas]

praise: Round 3 fixes each round-2 point in the code it was about, and adds a test for each fix.

• HTTPConnectionHandler.java:477 now builds DN.escapeAttributeValue(handlerName) + " Statistics", and the HTTPConnectionHandler row of MonitorDNTestCase.java:128 pins it.
• DNEqualityIndexesUpgradeTestCase.indexesThatCannotBeVerifiedOnlyWarn pins the new warn path, eachBaseDNGetsTheIndexesOfItsBackend pins the base DN selection, and the cleanup no longer hides a failed setup.

---

question (non-blocking): Is verifyAndRebuildOrWarn meant to warn and go on after a rebuild that has started and then failed, or only when the backend cannot be read?

opendj-server-legacy/src/main/java/org/opends/server/tools/upgrade/UpgradeTasks.java:806-811, :773-777

verifyAndRebuildIndexes throws only when RebuildIndex returns non-zero (:864-866). So an unreachable JDBC/Cassandra database and a PDB/JE rebuild that failed midway (temp directory full, for instance) reach the same catch. In the second case, OnDiskMergeImporter's rebuild strategy has already untrusted and deleted the index in beforePhaseOne (:1477-1480), and only afterPhaseTwo (:1484-1486) trusts it again. The member index is therefore left empty and untrusted. The upgrade still exits 0, and run.sh:126-127 marks the container ready. The javadoc's reason, "the server works without this task", holds only when the index was never touched. If only unreachable backends are meant: rethrow when the rebuild had started, so that performPostUpgradeTasks exits with an error as postUpgradeRebuildIndexes does. RebuildIndex's return code does not tell the two cases apart, so the check has to come before the rebuild. If both cases are meant: the warning should at least say what state the indexes are in:

WARN_UPGRADE_VERIFY_DN_EQUALITY_INDEXES_FAILED=Index(es) '%s' for base DN '%s' could not be verified \
 or rebuilt, see the upgrade log. If the rebuild had started, these indexes are now untrusted and \
 searches cannot use them. The upgrade goes on: once the backend can be read, rebuild them with \
 rebuild-index, or verify them with verify-index first

---

issue (non-blocking): The upgrade chapter says an ACI that no longer decodes puts the server into lockdown mode. A global ACI in that form stops the server from starting instead.

opendj-doc-generated-ref/src/main/asciidoc/install-guide/chap-upgrade.adoc:312

This holds for ACIs stored in entries. A ds-cfg-global-aci goes through AciHandler.processGlobalAcis (:980-997), which turns the decode failure into an InitializationException. AccessControlConfigManager (:268-272) rethrows it, and DirectoryServer.startServer calls initializeAccessControl at :1486 without catching it. An administrator who follows the note would look for lockdown mode on a server that is not running.

A global ACI (`ds-cfg-global-aci` in `config.ldif`) of this form stops the server from starting, and the error names the ACI: correct it in `config.ldif` while the server is stopped.

---

suggestion (non-blocking): aLaterSchemaFileDecidesTheMatchingRule catches the reverse-sort mutant. It does not reliably catch a mutant that drops Arrays.sort(files).

opendj-server-legacy/src/test/java/org/opends/server/tools/upgrade/DNEqualityIndexesUpgradeTestCase.java:146-148, opendj-server-legacy/src/main/java/org/opends/server/tools/upgrade/UpgradeUtils.java:376-380

File.listFiles returns the files in readdir order, not in creation order. On the Linux cells, the temp directory is /tmp on ext4. With dir_index, ext4 reads even a one-block directory in hash order, under a seed that differs per filesystem; APFS also lists by name hash. Both rows write the same two names, so they always see the same order. When that order puts 10-first.ldif first, both rows stay green without the sort. Whether today's CI image catches the mutant was not measured.

// UpgradeUtils: readSchemaFiles uses this instead of its own listFiles + sort
static File[] schemaFilesInReadOrder(final File schemaDirectory)
{
  final File[] files = schemaDirectory.listFiles((dir, name) -> name.toLowerCase().endsWith(".ldif"));
  if (files != null)
  {
    // The server reads them in this order, so a later file may redefine an attribute of an earlier one
    Arrays.sort(files);
  }
  return files;
}

// DNEqualityIndexesUpgradeTestCase
@Test
public void schemaFilesAreReadInNameOrder() throws Exception
{
  final File directory = Files.createTempDirectory("dn-equality-indexes").toFile();
  try
  {
    for (char c = 'z'; c >= 'a'; c--)
    {
      assertThat(new File(directory, "50-" + c + ".ldif").createNewFile()).isTrue();
    }
    final List<String> names = new ArrayList<>();
    for (final File file : UpgradeUtils.schemaFilesInReadOrder(directory))
    {
      names.add(file.getName());
    }
    assertThat(names).hasSize(26).isSorted();
  }
  finally
  {
    StaticUtils.recursiveDelete(directory);
  }
}

Pin: a mutant that drops the sort now survives only if readdir happens to return all 26 names already sorted.

---

suggestion (non-blocking): No test sees the INFO_UPGRADE_VERIFY_DN_EQUALITY_INDEXES_INCONSISTENT arm of verifyAndRebuildOrWarn.

opendj-server-legacy/src/test/java/org/opends/server/tools/upgrade/DNEqualityIndexesUpgradeTestCase.java:313-322, opendj-server-legacy/src/main/java/org/opends/server/tools/upgrade/UpgradeTasks.java:814-815

The rebuild case at :313 calls verifyAndRebuildIndexes directly. verifyAndRebuildOrWarn is called only once the index is consistent (:319) and on the warn path (:355). A mutant that always reports CONSISTENT therefore stays green, and an administrator would be told that indexes the upgrade had just rebuilt were consistent. Replace the first call:

final List<TextOutputCallback> rebuildNotifications = new ArrayList<>();
UpgradeTasks.verifyAndRebuildOrWarn(newContext(rebuildNotifications), configFile, BASE_DN, attributes, false, out);
assertThat(messagesOf(rebuildNotifications))
    .contains(INFO_UPGRADE_VERIFY_DN_EQUALITY_INDEXES_INCONSISTENT.get("member", BASE_DN).toString());
// the existing isFalse() call that follows still proves the rebuild happened

This also needs import static org.opends.messages.ToolMessages.INFO_UPGRADE_VERIFY_DN_EQUALITY_INDEXES_INCONSISTENT;.

Pin: collapsing the ternary at UpgradeTasks.java:814-815 to the CONSISTENT message turns this assertion red.

[vharseko]

Round 4 is 4e17178. The branch is rebased onto the current master (e9e614a, which brings #1115 and #1166). The only conflict was in run.sh, where #1115 rewrote the block after the upgrade: the block is now #1115's, with upgrade -n --force and its comment. The round-1 to round-3 commits are otherwise unchanged: a7b1e9d, b8bf670, 9f9db6d.

A rebuild that fails after it started: it now fails the upgrade. The warning was meant only for a backend that cannot be read. A PDB/JE rebuild that fails half-way does leave the index deleted and untrusted, and the effect is wider than slow searches: an equality search on member from a user without the unindexed-search privilege is then refused with insufficientAccessRights (EntryContainer.java:1090). As you note, the exit code cannot tell the cases apart. That holds for verify-index too: with --countErrors it exits with 1 both when the verification fails and when it finds one error. So the split is made at the verification:

• VerifyIndex.countIndexErrors() returns the number of errors, or -1 when the indexes could not be verified. mainVerifyIndex() now calls the same code and keeps its exit codes.
• UpgradeTasks.verifyAndRebuildIndexes() returns CONSISTENT, REBUILT or NOT_VERIFIED. Indexes that cannot be verified are no longer rebuilt: whatever keeps them from being read would most likely make the rebuild fail too, after it had deleted them.
• verifyAndRebuildOrWarn() warns and goes on only for NOT_VERIFIED. A rebuild that fails throws ERR_UPGRADE_PERFORMING_POST_TASKS_FAIL, as postUpgradeRebuildIndexes does, so upgrade exits with an error and the container does not report itself healthy.
• The warning now says the indexes were left as they were. The upgrade chapter says that a failed rebuild fails the upgrade and may leave the indexes untrusted until they are rebuilt.

The unreachable JDBC or Cassandra database still gives a warning and a healthy container. What is given up is a verification that fails on a corrupted index which a rebuild would repair: it now gives the warning, and the administrator rebuilds the index.

Global ACI: fixed in the upgrade chapter, after the lockdown sentence: a global ACI of this form stops the server from starting, and has to be corrected in config.ldif while the server is stopped. I did not use the wording "the error names the ACI": the startup error names the ds-cfg-global-aci attribute and its configuration entry, and the decoding error does not always quote the ACI.

Schema file order: fixed as suggested. UpgradeUtils.schemaFilesInReadOrder() lists and sorts the files, and readSchemaFiles() uses it. schemaFilesAreReadInNameOrder writes 50-z.ldif down to 50-a.ldif plus a .txt file, and expects the 26 LDIF names in sorted order. The comment in aLaterSchemaFileDecidesTheMatchingRule that claimed the reverse writing order defeats the listing is corrected.

INFO_UPGRADE_VERIFY_DN_EQUALITY_INDEXES_INCONSISTENT: covered. The rebuild case now goes through verifyAndRebuildOrWarn(), checks that notice, and then checks that the index verifies as CONSISTENT. The verify-only call checks that the notice is not sent again.

New tests in DNEqualityIndexesUpgradeTestCase:

• aRebuildThatFailsFailsTheUpgrade: a PDB backend whose trusted member index misses its keys, with a plain file where the rebuild creates its temporary directory (import-tmp). verifyAndRebuildOrWarn() throws ClientException, the output shows the rebuild failed (not the verification), and no warning is sent.
• indexesThatCannotBeVerifiedOnlyWarn now expects NOT_VERIFIED from verifyAndRebuildIndexes(), not an exception.
• The fixture is shared by the two backend tests. Each test has its own database directory, because removing the backend leaves its database behind.

Tests: DNEqualityIndexesUpgradeTestCase (8), VerifyIndexTestCase, ArgumentParserToolsTestCase, RebuildIndexTestCase and UpgradeTestCase pass. Five mutants, each caught: a failed rebuild that only warns again (aRebuildThatFailsFailsTheUpgrade), indexes that cannot be verified rebuilt anyway (indexesThatCannotBeVerifiedOnlyWarn), rebuilt indexes reported as consistent (rebuildsAnIndexOnlyWhenItMissesKeys), the schema files not sorted (schemaFilesAreReadInNameOrder; on this macOS machine both aLaterSchemaFileDecidesTheMatchingRule rows fail too), and countIndexErrors() counting errors only with --countErrors (rebuildsAnIndexOnlyWhenItMissesKeys and aRebuildThatFailsFailsTheUpgrade).

[maximthomas]

praise: The new commit separates "the indexes cannot be verified" from "the rebuild failed", and handles each one the way the rest of the upgrade does.

• UpgradeTasks.verifyAndRebuildIndexes returns NOT_VERIFIED when VerifyIndex.countIndexErrors gives -1, and leaves those indexes alone with a warning. A failed rebuild throws (UpgradeTasks.java:883-885), so the upgrade no longer exits 0 with an untrusted index.
• chap-upgrade.adoc:312 now says that a global ACI that no longer decodes stops the server from starting.
• The round-4 test bodies ran green on Linux at this head: DNEqualityIndexesUpgradeTestCase, 8 tests, build-maven (ubuntu-latest, 11).

---

issue (non-blocking): A failed rebuild now ends the per-base-DN loop, and the base DNs after it are neither verified nor named.

opendj-server-legacy/src/main/java/org/opends/server/tools/upgrade/UpgradeTasks.java:720-724, :817-821

verifyAndRebuildOrWarn rethrows the ClientException of a failed rebuild, and the loop over indexesPerBaseDN in postUpgrade does not catch it. Upgrade.performPostUpgradeTasks (Upgrade.java:924-929) then prints only the generic "Process aborted" message. It calls postponePostUpgrade for the later tasks and never for this one, so INFO_UPGRADE_VERIFY_DN_EQUALITY_INDEXES_DECLINED is not printed either. Take member indexes under o=a and o=b, and a rebuild under o=a that fails because the temporary directory is full. The output names only o=a, and chap-upgrade.adoc:308 sends the operator to rebuild that one. Under o=b the keys from the old parser stay, and indexed equality searches miss entries. At the previous head the loop went on after a failure. If you keep the loop as it is, a sentence in chap-upgrade.adoc:308 saying that the base DNs after a failed rebuild were not verified would also do.

        ClientException firstFailure = null;
        for (final Map.Entry<String, Set<String>> baseDN : indexesPerBaseDN.entrySet())
        {
          try
          {
            verifyAndRebuildOrWarn(context, configFile.getAbsolutePath(), baseDN.getKey(), baseDN.getValue(), true,
                UpgradeLog.getPrintStream());
          }
          catch (final ClientException e)
          {
            // Verify the other base DNs too: each failed rebuild gets its own failed progress line
            if (firstFailure == null)
            {
              firstFailure = e;
            }
          }
        }
        if (firstFailure != null)
        {
          throw firstFailure;
        }

---

suggestion (non-blocking): indexesThatCannotBeVerifiedOnlyWarn reaches NOT_VERIFIED only through ERR_NO_BACKENDS_FOR_BASE. No test reaches the unreadable-backend catch that its javadoc names.

opendj-server-legacy/src/test/java/org/opends/server/tools/upgrade/DNEqualityIndexesUpgradeTestCase.java:358, opendj-server-legacy/src/main/java/org/opends/server/tools/VerifyIndex.java:327-335

No backend holds o=held by no backend, so VerifyIndex.verifyIndex returns at :272-275. The road the javadoc describes, a JDBC or Cassandra backend whose database is down, throws out of backend.verifyBackend into the catch at :327-335. Both PDB tests need verifyBackend to return normally, so nothing reaches that catch. If either return VERIFY_FAILED there goes back to return 1, an unreachable database counts as REBUILT, and the rebuild that follows fails the upgrade instead of warning. Every test stays green, because VerifyIndexTestCase goes through mainVerifyIndex, which maps -1 and 1 to the same exit code. This is traced, not run.

    catch (InitializationException e)
    {
      printWrappedText(err, ERR_VERIFYINDEX_ERROR_DURING_VERIFY.get(e.getMessage()));
      return 1; // the mutant no test kills; the same at :335
    }

Pin: in a fixture where verifyBackend throws, assert NOT_VERIFIED, and assert that the missing keys are still missing afterwards. One way may be the PDB fixture with its db directory moved aside after setBackendEnabled(false). Whether that makes the read-only open throw was not run.

---

issue (non-blocking): rebuildsAnIndexOnlyWhenItMissesKeys keeps writing to the PrintStream that the rebuild closed.

opendj-server-legacy/src/test/java/org/opends/server/tools/upgrade/DNEqualityIndexesUpgradeTestCase.java:294-302

The first verifyAndRebuildOrWarn (:294) rebuilds, and RebuildIndex.rebuildIndexesWithinMultipleBackends closes out in its finally block (RebuildIndex.java:583-585). The calls at :297 and :302 write to that closed stream, and their output is dropped. If the post-rebuild verification goes red, its output: %s description (:298) is missing verify-index's reason line. Only the diagnostics are affected: the test passes and fails in the same cases. ByteArrayOutputStream.close() does nothing, so a new stream over the same buffer works.

      // RebuildIndex closed `out`: the calls after the rebuild write to a stream of their own
      final PrintStream afterRebuild = new PrintStream(output, true);
      assertThat(UpgradeTasks.verifyAndRebuildIndexes(configFile, BASE_DN, MEMBER, false, afterRebuild))
          .as("verification of the rebuilt index; output: %s", output)
          .isEqualTo(UpgradeTasks.IndexVerification.CONSISTENT);

      final List<TextOutputCallback> notifications = new ArrayList<>();
      UpgradeTasks.verifyAndRebuildOrWarn(newContext(notifications), configFile, BASE_DN, MEMBER, false, afterRebuild);

[vharseko]

Round 5 is c726f10. The branch is rebased onto the current master (58666fd, which brings #1174 and #1176), without conflicts; the round-1 to round-4 commits are unchanged: 7384c3c, 7faaaf8, 4a6c484, 56fde9d.

A failed rebuild and the base DNs after it: the next base DNs are now named, but not verified. I did not take the suggested loop that goes on to the next base DNs. The cause in your example, a full temporary directory, is shared by every base DN, and the rebuild deletes and untrusts an index before it fails (OnDiskMergeImporter's beforePhaseOne, as you traced in round 3). Going on would turn one untrusted index into one per base DN with missing keys. So after the first failed rebuild:

• UpgradeTasks.verifyAndRebuildOrWarn(context, configFile, indexesPerBaseDN, …), the loop moved out of postUpgrade, neither verifies nor rebuilds the indexes of the next base DNs. Each of them gets a warning, WARN_UPGRADE_VERIFY_DN_EQUALITY_INDEXES_SKIPPED: "Index(es) 'member' for base DN 'o=b' were not verified, as the rebuild of other indexes failed. Once the cause of that failure is fixed, verify them manually with verify-index, and rebuild them with rebuild-index where it reports errors".
• The loop then throws the rebuild's ClientException, so upgrade still exits with an error.
• The upgrade chapter says that the base DNs after a failed rebuild are named and left to the administrator.
• The stream each base DN writes to (UpgradeLog.getPrintStream()) is now closed once that base DN is done. Before, only a rebuild closed it.

A backend whose verification throws: pinned, with another fixture. I did not move the db directory aside. VerifyJob throws when it is asked for an index that the backend does not have (ERR_ATTRIBUTE_INDEX_NOT_CONFIGURED), and BackendImpl.verifyBackend turns that into the DirectoryException that reaches the catch (Exception e) of VerifyIndex.verifyIndex. indexesThatTheBackendFailsToVerifyAreLeftAsTheyWere verifies member and uniqueMember on the PDB backend, which only indexes member. It expects the warning, and then expects verify-index still to count errors in member. The catch (InitializationException e) next to it stays unpinned: I found no fixture that makes verifyBackend throw that exception. The javadoc of indexesThatCannotBeVerifiedOnlyWarn now says what it covers: a base DN that no backend holds.

The stream closed by the rebuild: fixed as suggested. After the rebuild, rebuildsAnIndexOnlyWhenItMissesKeys writes to a new PrintStream over the same buffer.

One more test fix, found with the first mutant below. Under that mutant, verify-index runs in the test process for a base DN that no backend holds. Disabling the PDB backend afterwards then fails with a NullPointerException in DiskSpaceMonitor (result code Other). The fixture's cleanup asserted that the backend was disabled, so it hid the failure of the test and left the backend configured, and the next three tests failed as well. The cleanup now disables the backend without asserting the result, and always deletes the entries. This only affects the tests, where the tools share the process with a running server.

Tests: DNEqualityIndexesUpgradeTestCase (10), VerifyIndexTestCase, RebuildIndexTestCase and UpgradeTestCase pass. aFailedRebuildLeavesTheIndexesOfTheNextBaseDNsUnverified fails against the round-4 loop. Three mutants, each caught: the next base DNs verified and rebuilt after a failed rebuild, the failed rebuild not rethrown once the loop ends (both by aFailedRebuildLeavesTheIndexesOfTheNextBaseDNsUnverified), and return 1 in the catch (Exception e) of VerifyIndex.verifyIndex (indexesThatTheBackendFailsToVerifyAreLeftAsTheyWere).

[maximthomas]

praise: The round-5 commit answers both round-4 issues where they were raised.

• The new Map overload of UpgradeTasks.verifyAndRebuildOrWarn names every base DN after a failed rebuild with WARN_UPGRADE_VERIFY_DN_EQUALITY_INDEXES_SKIPPED, and rethrows the first failure after the loop (UpgradeTasks.java:806-832). The test javadoc says why the rest are skipped rather than verified (DNEqualityIndexesUpgradeTestCase.java:339-343).
• Each base DN gets its own upgrade-log stream, closed in a finally (:814, :823-827). This also stops the previous head from leaking one FileOutputStream per base DN that was only verified: VerifyIndex never closes the stream it is given.
• The new cases ran green on Linux at this head: DNEqualityIndexesUpgradeTestCase, 10 tests, build-maven (ubuntu-latest, 11).