From 486f057712aa5c94e2c32343b1d161fc00a0fe17 Mon Sep 17 00:00:00 2001 From: Valera V Harseko Date: Thu, 20 Aug 2026 19:06:09 +0300 Subject: [PATCH 1/2] [#890] Persist a compressed schema token before handing it out, and report the ones with no definition CompressedSchema.getAttributeId() registered a new token in the in-memory maps and only then wrote it to the storage. A failed write left the registration behind, so every later encode of the same attribute description took the lock-free fast path and succeeded: the entries written with that token could not be decoded after a restart, or - where the lost token was the last one - were decoded as whatever attribute reused its id. The registration is now persisted before it is published. The element is appended to the decode map first, because storeAttribute() is free to persist the whole content rather than the element it is handed - DefaultCompressedSchema rewrites its file from getAllAttributes() - and the id reaches the encode map, the lock-free path an encode takes to it, only once the store returned. A store that fails withdraws the appended element, so the next attempt allocates the id again and stores it. getObjectClassId() does the same. PersistentCompressedSchema no longer absorbs the IOException of its ASN.1 writing: swallowed there, it left the same divergence with nothing reported to the operator at all. Independently of that, a token with no definition is reported as the unknown token it is rather than let out of the decode path as an IndexOutOfBoundsException, and the null slots a decode map is padded with - a compressed schema written by an older server can carry gaps - are carried over by a schema reload and skipped when the whole content is saved, rather than dereferenced. --- .../opends/server/api/CompressedSchema.java | 162 ++++++- .../pluggable/PersistentCompressedSchema.java | 13 +- .../server/api/CompressedSchemaTestCase.java | 439 ++++++++++++++++++ 3 files changed, 600 insertions(+), 14 deletions(-) create mode 100644 opendj-server-legacy/src/test/java/org/opends/server/api/CompressedSchemaTestCase.java diff --git a/opendj-server-legacy/src/main/java/org/opends/server/api/CompressedSchema.java b/opendj-server-legacy/src/main/java/org/opends/server/api/CompressedSchema.java index 74088abf4a..92e83359e9 100644 --- a/opendj-server-legacy/src/main/java/org/opends/server/api/CompressedSchema.java +++ b/opendj-server-legacy/src/main/java/org/opends/server/api/CompressedSchema.java @@ -28,6 +28,7 @@ import java.util.List; import java.util.Map; import java.util.Map.Entry; +import java.util.NoSuchElementException; import java.util.Set; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.CopyOnWriteArrayList; @@ -155,7 +156,19 @@ private void reloadAttributeTypeMaps(Mappings mappings, Mappings newMappings) { for(int id=0;id ocMap = mappings.ocDecodeMap.get(id); + if (ocMap != null) + { + loadObjectClassesToMaps(id, ocMap.values(), newMappings, false); + } + else + { + // A gap, as in reloadAttributeTypeMaps(). + newMappings.ocDecodeMap.add(null); + } } } @@ -188,7 +210,7 @@ public final Attribute decodeAttribute(final ByteSequenceReader reader) // Before returning the attribute, make sure that the attribute type is not stale. final Mappings mappings = reloadMappingsIfSchemaChanged(); - final AttributeDescription ad = mappings.adDecodeMap.get(adId); + final AttributeDescription ad = decodeMapGet(mappings.adDecodeMap, adId); if (ad == null) { throw new DirectoryException(DirectoryServer.getCoreConfigManager().getServerErrorResultCode(), @@ -223,6 +245,39 @@ private ByteString readValue(final ByteSequenceReader reader) return reader.readByteSequence(reader.readBERLength()).toByteString(); } + /** + * Returns the element a token addresses, or {@code null} where it addresses none: the token can + * be outside the range of the decode map, or address one of the slots the map is padded with for + * the ids missing from the compressed schema it was loaded from. Both are reported to the caller + * as the unknown token they are, rather than let out of the decode path as an unchecked + * exception the callers of that path are not written for. + * + * @param decodeMap + * The decode map to look the token up in. + * @param id + * The decoded token. + * @return The element registered under the token, or {@code null} if there is none. + */ + private static T decodeMapGet(final List decodeMap, final int id) + { + if (id < 0) + { + return null; + } + try + { + return decodeMap.get(id); + } + catch (final IndexOutOfBoundsException e) + { + // Caught rather than kept away by a comparison against size(): size() and get() of a + // CopyOnWriteArrayList read the array separately, so the comparison would not make the + // lookup safe anyway, and this runs for every attribute of every entry read from a backend - + // the common path is left with the single read it had. + return null; + } + } + /** * Decodes an object class set from the provided byte string. * @@ -241,7 +296,7 @@ public final Map decodeObjectClasses( // Before returning the object classes, make sure that none of them are stale. final Mappings mappings = reloadMappingsIfSchemaChanged(); - Map ocMap = mappings.ocDecodeMap.get(ocId); + Map ocMap = decodeMapGet(mappings.ocDecodeMap, ocId); if (ocMap == null) { throw new DirectoryException(DirectoryServer.getCoreConfigManager().getServerErrorResultCode(), @@ -297,10 +352,7 @@ private int getAttributeId(final AttributeDescription ad) throws DirectoryExcept id = mappings.adEncodeMap.get(ad); if (id == null) { - id = mappings.adDecodeMap.size(); - mappings.adDecodeMap.add(ad); - mappings.adEncodeMap.put(ad, id); - storeAttribute(encodeId(id), ad.getAttributeType().getNameOrOID(), ad.getOptions()); + id = registerAttribute(mappings, ad); } return id; } @@ -310,6 +362,46 @@ private int getAttributeId(final AttributeDescription ad) throws DirectoryExcept } } + /** + * Registers a new attribute description and returns the id allocated to it. The registration is + * persisted before it is published, and is withdrawn if it cannot be persisted: an entry must + * never be written with a token whose definition did not reach the storage, because nothing + * stores it afterwards and the entry cannot be decoded once the server is restarted. + *

+ * Must be called with the exclusive lock held, which is what makes the id allocated here still + * the last element of the decode map when it has to be withdrawn. + */ + private int registerAttribute(final Mappings mappings, final AttributeDescription ad) throws DirectoryException + { + final int id = mappings.adDecodeMap.size(); + // Appended to the decode map first: storeAttribute() is free to persist the whole content of + // this compressed schema rather than the single element it is handed - DefaultCompressedSchema + // rewrites its file from getAllAttributes() - so the element being registered has to be part + // of it by then. The decode map is not what an encode reaches the id through, so nothing can + // yet write an entry carrying it. + mappings.adDecodeMap.add(ad); + boolean registered = false; + try + { + storeAttribute(encodeId(id), ad.getAttributeType().getNameOrOID(), ad.getOptions()); + // Published only once persisted: the encode map is read without the lock, so an id another + // thread finds there can be carried by an entry a moment later and must never be withdrawn. + mappings.adEncodeMap.put(ad, id); + registered = true; + } + finally + { + if (!registered) + { + // Withdrawn, so that the next attempt allocates the id again and stores it. Removed by + // index, and by the index of the last element: every append is made under the exclusive + // lock, so this is still the element appended above, and no other id shifts. + mappings.adDecodeMap.remove(mappings.adDecodeMap.size() - 1); + } + } + return id; + } + /** * Encodes the provided set of object classes to a byte array. If the same set * had been previously encoded, then the cached value will be used. Otherwise, @@ -354,10 +446,7 @@ private int getObjectClassId(final Map objectClasses) throw id = mappings.ocEncodeMap.get(objectClasses); if (id == null) { - id = mappings.ocDecodeMap.size(); - mappings.ocDecodeMap.add(objectClasses); - mappings.ocEncodeMap.put(objectClasses, id); - storeObjectClasses(encodeId(id), objectClasses.values()); + id = registerObjectClasses(mappings, objectClasses); } return id; } @@ -367,6 +456,35 @@ private int getObjectClassId(final Map objectClasses) throw } } + /** + * Registers a new object class set and returns the id allocated to it, persisting the + * registration before publishing it and withdrawing it if it cannot be persisted, exactly as + * {@link #registerAttribute(Mappings, AttributeDescription)} does. + *

+ * Must be called with the exclusive lock held. + */ + private int registerObjectClasses(final Mappings mappings, final Map objectClasses) + throws DirectoryException + { + final int id = mappings.ocDecodeMap.size(); + mappings.ocDecodeMap.add(objectClasses); + boolean registered = false; + try + { + storeObjectClasses(encodeId(id), objectClasses.values()); + mappings.ocEncodeMap.put(objectClasses, id); + registered = true; + } + finally + { + if (!registered) + { + mappings.ocDecodeMap.remove(mappings.ocDecodeMap.size() - 1); + } + } + return id; + } + /** * Returns a view of the encoded attributes in this compressed schema which can be used for saving * the entire content to disk. @@ -390,12 +508,23 @@ public Iterator>>> iterator() @Override public boolean hasNext() { + // Skips the gaps: a decode map padded with null for the ids missing from the + // compressed schema it was loaded from is still saved, and the ids around a gap are + // preserved by the token each element is written with. + while (id < adDecodeMap.size() && adDecodeMap.get(id) == null) + { + id++; + } return id < adDecodeMap.size(); } @Override public Entry>> next() { + if (!hasNext()) + { + throw new NoSuchElementException(); + } final byte[] encodedAttribute = encodeId(id); final AttributeDescription ad = adDecodeMap.get(id++); return new SimpleImmutableEntry>>( @@ -437,12 +566,21 @@ public Iterator>> iterator() @Override public boolean hasNext() { + // Skips the gaps, as in getAllAttributes(). + while (id < ocDecodeMap.size() && ocDecodeMap.get(id) == null) + { + id++; + } return id < ocDecodeMap.size(); } @Override public Entry> next() { + if (!hasNext()) + { + throw new NoSuchElementException(); + } final byte[] encodedObjectClasses = encodeId(id); final Map ocMap = ocDecodeMap.get(id++); return new SimpleImmutableEntry<>(encodedObjectClasses, ocMap.values()); diff --git a/opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/PersistentCompressedSchema.java b/opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/PersistentCompressedSchema.java index af484abb73..56312716b2 100644 --- a/opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/PersistentCompressedSchema.java +++ b/opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/PersistentCompressedSchema.java @@ -13,6 +13,7 @@ * * Copyright 2008-2009 Sun Microsystems, Inc. * Portions Copyright 2013-2016 ForgeRock AS. + * Portions Copyright 2026 3A Systems, LLC. */ package org.opends.server.backends.pluggable; @@ -112,7 +113,12 @@ protected void storeAttribute(final byte[] encodedAttribute, } catch (final IOException e) { - // TODO: Shouldn't happen but should log a message + // Reported rather than absorbed: the caller takes a store that returned for a definition + // that reached the tree, and an entry written with a token that never did cannot be decoded + // once the server is restarted. + logger.traceException(e); + throw new DirectoryException(DirectoryServer.getCoreConfigManager().getServerErrorResultCode(), + ERR_COMPSCHEMA_CANNOT_STORE_EX.get(e.getMessage()), e); } } @@ -133,7 +139,10 @@ protected void storeObjectClasses(final byte[] encodedObjectClasses, } catch (final IOException e) { - // TODO: Shouldn't happen but should log a message + // Reported rather than absorbed, as in storeAttribute(). + logger.traceException(e); + throw new DirectoryException(DirectoryServer.getCoreConfigManager().getServerErrorResultCode(), + ERR_COMPSCHEMA_CANNOT_STORE_EX.get(e.getMessage()), e); } } diff --git a/opendj-server-legacy/src/test/java/org/opends/server/api/CompressedSchemaTestCase.java b/opendj-server-legacy/src/test/java/org/opends/server/api/CompressedSchemaTestCase.java new file mode 100644 index 0000000000..feb75d378a --- /dev/null +++ b/opendj-server-legacy/src/test/java/org/opends/server/api/CompressedSchemaTestCase.java @@ -0,0 +1,439 @@ +/* + * 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.api; + +import static org.testng.Assert.*; + +import java.util.ArrayList; +import java.util.Arrays; +import java.util.Collection; +import java.util.Collections; +import java.util.LinkedHashMap; +import java.util.List; +import java.util.Map; +import java.util.Map.Entry; +import java.util.concurrent.Callable; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.Future; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.TimeoutException; + +import org.forgerock.i18n.LocalizableMessage; +import org.forgerock.opendj.ldap.ByteString; +import org.forgerock.opendj.ldap.ByteStringBuilder; +import org.forgerock.opendj.ldap.ByteSequenceReader; +import org.forgerock.opendj.ldap.ResultCode; +import org.forgerock.opendj.ldap.schema.ObjectClass; +import org.opends.server.TestCaseUtils; +import org.opends.server.core.DirectoryServer; +import org.opends.server.types.Attribute; +import org.opends.server.types.Attributes; +import org.opends.server.types.DirectoryException; +import org.testng.annotations.BeforeClass; +import org.testng.annotations.Test; + +/** + * Tests that a compressed schema never hands out a token whose definition was not persisted, and + * that a token it holds no definition for is reported rather than let out of the decode path as an + * unchecked exception. + */ +@SuppressWarnings("javadoc") +public class CompressedSchemaTestCase extends APITestCase +{ + /** A compressed schema whose store can be made to fail, recording what it did persist. */ + private static final class TestCompressedSchema extends CompressedSchema + { + private final Map storedAttributes = new LinkedHashMap<>(); + private final Map> storedObjectClasses = new LinkedHashMap<>(); + private int attributeStoreCount; + private int objectClassStoreCount; + private boolean failStore; + /** Counted down when a store is entered, when the store is gated. */ + private CountDownLatch enteredStore; + /** Awaited by a gated store, which holds the exclusive lock while it waits. */ + private CountDownLatch leaveStore; + + private TestCompressedSchema() + { + super(DirectoryServer.getInstance().getServerContext()); + } + + @Override + protected void storeAttribute(final byte[] encodedAttribute, final String attributeName, + final Iterable attributeOptions) throws DirectoryException + { + attributeStoreCount++; + awaitIfGated(); + failIfRequested(); + storedAttributes.put(token(encodedAttribute), attributeName); + } + + @Override + protected void storeObjectClasses(final byte[] encodedObjectClasses, final Collection objectClassNames) + throws DirectoryException + { + objectClassStoreCount++; + failIfRequested(); + storedObjectClasses.put(token(encodedObjectClasses), new ArrayList<>(objectClassNames)); + } + + private void failIfRequested() throws DirectoryException + { + if (failStore) + { + throw new DirectoryException(ResultCode.OTHER, LocalizableMessage.raw("the store failed")); + } + } + + private void awaitIfGated() throws DirectoryException + { + if (enteredStore == null) + { + return; + } + enteredStore.countDown(); + try + { + if (!leaveStore.await(30, TimeUnit.SECONDS)) + { + throw new DirectoryException(ResultCode.OTHER, LocalizableMessage.raw("the gated store timed out")); + } + } + catch (final InterruptedException e) + { + Thread.currentThread().interrupt(); + throw new DirectoryException(ResultCode.OTHER, LocalizableMessage.raw("the gated store was interrupted"), e); + } + } + + /** Loads a definition under the provided token, as an implementation does at startup. */ + private void loadAttributeAt(final int id, final String attributeName) + { + loadAttribute(encodedToken(id), attributeName, Collections. emptySet()); + } + + private void loadObjectClassesAt(final int id, final Collection objectClassNames) + { + loadObjectClasses(encodedToken(id), objectClassNames); + } + + /** The tokens the whole content would be saved under, as DefaultCompressedSchema saves it. */ + private List savedAttributeTokens() + { + final List tokens = new ArrayList<>(); + for (final Entry>> attribute : getAllAttributes()) + { + tokens.add(token(attribute.getKey())); + } + return tokens; + } + + private List savedObjectClassTokens() + { + final List tokens = new ArrayList<>(); + for (final Entry> objectClasses : getAllObjectClasses()) + { + tokens.add(token(objectClasses.getKey())); + } + return tokens; + } + } + + @BeforeClass + public void setUp() throws Exception + { + TestCaseUtils.startServer(); + } + + /** + * A registration whose store failed must be withdrawn: the next encode of the same attribute has + * to allocate and store the token again, rather than take the lock-free fast path and write an + * entry carrying a token whose definition is nowhere. + */ + @Test + public void attributeTokenIsWithdrawnWhenItCannotBeStored() throws Exception + { + final TestCompressedSchema compressedSchema = new TestCompressedSchema(); + final Attribute attribute = Attributes.create("description", "a value"); + + compressedSchema.failStore = true; + try + { + compressedSchema.encodeAttribute(new ByteStringBuilder(), attribute); + fail("the encode should have failed with the store"); + } + catch (final DirectoryException expected) + { + // The operation fails, which is what the caller is told. + } + assertEquals(compressedSchema.attributeStoreCount, 1); + assertTrue(compressedSchema.storedAttributes.isEmpty(), "nothing was persisted"); + + compressedSchema.failStore = false; + final ByteStringBuilder builder = new ByteStringBuilder(); + compressedSchema.encodeAttribute(builder, attribute); + assertEquals(compressedSchema.attributeStoreCount, 2, "the failed registration was left behind"); + + final int encodedToken = tokenOf(builder.toByteString()); + assertTrue(compressedSchema.storedAttributes.containsKey(encodedToken), + "the entry carries token " + encodedToken + ", which was never stored"); + final Attribute decoded = compressedSchema.decodeAttribute(builder.toByteString().asReader()); + assertEquals(decoded.getAttributeDescription(), attribute.getAttributeDescription()); + assertEquals(decoded.iterator().next().toString(), "a value"); + } + + /** The same for an object class set. */ + @Test + public void objectClassTokenIsWithdrawnWhenItCannotBeStored() throws Exception + { + final TestCompressedSchema compressedSchema = new TestCompressedSchema(); + final Map objectClasses = objectClasses("top", "person"); + + compressedSchema.failStore = true; + try + { + compressedSchema.encodeObjectClasses(new ByteStringBuilder(), objectClasses); + fail("the encode should have failed with the store"); + } + catch (final DirectoryException expected) + { + // The operation fails, which is what the caller is told. + } + assertEquals(compressedSchema.objectClassStoreCount, 1); + assertTrue(compressedSchema.storedObjectClasses.isEmpty(), "nothing was persisted"); + + compressedSchema.failStore = false; + final ByteStringBuilder builder = new ByteStringBuilder(); + compressedSchema.encodeObjectClasses(builder, objectClasses); + assertEquals(compressedSchema.objectClassStoreCount, 2, "the failed registration was left behind"); + + final int encodedToken = tokenOf(builder.toByteString()); + assertTrue(compressedSchema.storedObjectClasses.containsKey(encodedToken), + "the entry carries token " + encodedToken + ", which was never stored"); + assertEquals(compressedSchema.decodeObjectClasses(builder.toByteString().asReader()), objectClasses); + } + + /** + * The id of a registration reaches the encode map - the lock-free path an encode takes to it - + * only once the definition is persisted, so that no other thread can write an entry carrying an + * id that a failing store is about to withdraw. + */ + @Test + public void aTokenIsPublishedOnlyOnceItIsStored() throws Exception + { + final TestCompressedSchema compressedSchema = new TestCompressedSchema(); + final Attribute attribute = Attributes.create("description", "a value"); + final ExecutorService executor = Executors.newFixedThreadPool(2); + try + { + // One encode is held inside the store of the registration it made, holding the exclusive lock. + compressedSchema.enteredStore = new CountDownLatch(1); + compressedSchema.leaveStore = new CountDownLatch(1); + final Future registering = executor.submit(encoding(compressedSchema, attribute, null)); + assertTrue(compressedSchema.enteredStore.await(30, TimeUnit.SECONDS), "the store was never reached"); + + // Another encode of the same attribute must not be handed the id being stored. + final CountDownLatch started = new CountDownLatch(1); + final Future concurrent = executor.submit(encoding(compressedSchema, attribute, started)); + assertTrue(started.await(30, TimeUnit.SECONDS), "the concurrent encode was never started"); + try + { + fail("the token was handed out before it was stored: " + concurrent.get(500, TimeUnit.MILLISECONDS)); + } + catch (final TimeoutException expected) + { + // Waiting for the registration to be persisted, as it must. + } + + compressedSchema.leaveStore.countDown(); + assertEquals(registering.get(30, TimeUnit.SECONDS), Integer.valueOf(0)); + assertEquals(concurrent.get(30, TimeUnit.SECONDS), Integer.valueOf(0)); + assertEquals(compressedSchema.attributeStoreCount, 1, "the same token was stored twice"); + } + finally + { + executor.shutdownNow(); + } + } + + private static Callable encoding(final CompressedSchema compressedSchema, final Attribute attribute, + final CountDownLatch started) + { + return new Callable() + { + @Override + public Integer call() throws Exception + { + if (started != null) + { + started.countDown(); + } + final ByteStringBuilder builder = new ByteStringBuilder(); + compressedSchema.encodeAttribute(builder, attribute); + return tokenOf(builder.toByteString()); + } + }; + } + + /** A token with no definition is reported, whether it is out of range or below it. */ + @Test + public void unknownAttributeTokenIsReported() throws Exception + { + final TestCompressedSchema compressedSchema = new TestCompressedSchema(); + for (final int unknownToken : new int[] { -1, 0, 7 }) + { + try + { + compressedSchema.decodeAttribute(encodedAttribute(unknownToken).asReader()); + fail("the token " + unknownToken + " has no definition and should have been reported"); + } + catch (final DirectoryException expected) + { + // Reported as the unknown token it is. + } + } + } + + @Test + public void unknownObjectClassTokenIsReported() throws Exception + { + final TestCompressedSchema compressedSchema = new TestCompressedSchema(); + for (final int unknownToken : new int[] { -1, 0, 7 }) + { + try + { + compressedSchema.decodeObjectClasses(encodedToken(unknownToken, new ByteStringBuilder()).asReader()); + fail("the token " + unknownToken + " has no definition and should have been reported"); + } + catch (final DirectoryException expected) + { + // Reported as the unknown token it is. + } + } + } + + /** + * A compressed schema loaded from a storage that holds no definition for some of the tokens + * carries a gap. Decoding across the gap, reloading the maps for a changed schema and saving the + * whole content must all walk over it, and the ids around it must not shift - the entries already + * written carry them. + */ + @Test + public void aGapInTheDecodeMapsIsCarriedRatherThanDereferenced() throws Exception + { + final TestCompressedSchema compressedSchema = new TestCompressedSchema(); + compressedSchema.loadAttributeAt(2, "description"); + compressedSchema.loadObjectClassesAt(2, Arrays.asList("top", "person")); + + // The first decode also rebuilds the maps for the current schema, which is what used to walk + // into the gap with no null check. + try + { + compressedSchema.decodeAttribute(encodedAttribute(0).asReader()); + fail("the token 0 falls in the gap and should have been reported"); + } + catch (final DirectoryException expected) + { + // Reported as the unknown token it is. + } + try + { + compressedSchema.decodeObjectClasses(encodedToken(1, new ByteStringBuilder()).asReader()); + fail("the token 1 falls in the gap and should have been reported"); + } + catch (final DirectoryException expected) + { + // Reported as the unknown token it is. + } + + // What is around the gap is still reachable under the ids it was loaded with. + assertEquals(compressedSchema.decodeAttribute(encodedAttribute(2).asReader()) + .getAttributeDescription().getAttributeType().getNameOrOID(), "description"); + assertEquals(compressedSchema.decodeObjectClasses(encodedToken(2, new ByteStringBuilder()).asReader()), + objectClasses("top", "person")); + + // And the next registration allocates the id after the gap, not one inside it. + final ByteStringBuilder attributeBuilder = new ByteStringBuilder(); + compressedSchema.encodeAttribute(attributeBuilder, Attributes.create("cn", "a value")); + assertEquals(tokenOf(attributeBuilder.toByteString()), 3); + + final ByteStringBuilder objectClassesBuilder = new ByteStringBuilder(); + compressedSchema.encodeObjectClasses(objectClassesBuilder, objectClasses("top", "organizationalUnit")); + assertEquals(tokenOf(objectClassesBuilder.toByteString()), 3); + + // The whole content is still saveable, which is how DefaultCompressedSchema persists a store. + assertEquals(compressedSchema.savedAttributeTokens(), Arrays.asList(2, 3)); + assertEquals(compressedSchema.savedObjectClassTokens(), Arrays.asList(2, 3)); + } + + private static Map objectClasses(final String... names) + { + final Map objectClasses = new LinkedHashMap<>(names.length); + for (final String name : names) + { + objectClasses.put(DirectoryServer.getInstance().getServerContext().getSchema().getObjectClass(name), name); + } + return objectClasses; + } + + /** Encodes an attribute holding a single value under the provided token. */ + private static ByteString encodedAttribute(final int id) + { + final ByteStringBuilder builder = new ByteStringBuilder(); + encodedToken(id, builder); + builder.appendBERLength(1); + builder.appendBERLength(1); + builder.appendBytes(new byte[] { 'x' }); + return builder.toByteString(); + } + + private static ByteString encodedToken(final int id, final ByteStringBuilder builder) + { + final byte[] idBytes = encodedToken(id); + builder.appendBERLength(idBytes.length); + builder.appendBytes(idBytes); + return builder.toByteString(); + } + + /** Encodes a token the way CompressedSchema does, one byte being enough for the tests. */ + private static byte[] encodedToken(final int id) + { + return new byte[] { (byte) ((id + 1) & 0xFF) }; + } + + /** Decodes a token the way CompressedSchema does. */ + private static int token(final byte[] idBytes) + { + int id = 0; + for (final byte b : idBytes) + { + id <<= 8; + id |= b & 0xFF; + } + return id - 1; + } + + /** Reads the token an encoded attribute or object class set starts with. */ + private static int tokenOf(final ByteString encoded) + { + final ByteSequenceReader reader = encoded.asReader(); + final byte[] idBytes = new byte[reader.readBERLength()]; + reader.readBytes(idBytes); + return token(idBytes); + } +} From 8ab804798fb1ceaee275c60d52c954f552ad3ca3 Mon Sep 17 00:00:00 2001 From: Valera V Harseko Date: Fri, 21 Aug 2026 15:48:04 +0300 Subject: [PATCH 2/2] [#890] Pin the withdrawal with a test a leaked registration fails, and the rest of what the review found open The two tests named for the withdrawal passed with the finally block deleted from registerAttribute()/registerObjectClasses(): a registration that leaks its decode map element simply allocates the next id, and the store count, the token of the retry and the decode all still hold. They now assert the id the retry is given and the tokens the whole content would be saved under, which is what the leaked element shows up in. storeObjectClasses() is gated like storeAttribute(), and the ordering of registerObjectClasses() has a concurrency test of its own: publishing the id before storing it and rolling both maps back kept every object class test green. The ordering tests wait for the concurrent encode to park on the exclusive lock rather than for a timeout to pass. A latch counted down inside the task only proves the task body started, so a build handing out an id before storing it was recorded as a pass whenever the thread was slow between the countdown and the lock-free read. Nothing under the try of PersistentCompressedSchema's ASN.1 writing throws IOException - the sink is a ByteStringBuilder, whose OutputStream declares none - so the comment on that catch no longer claims a store reached the tree. What makes a withdrawal reachable in a running server is store()'s own catch, on a storage.write that failed. An unknown token is named as the storage holds it, with the id it decodes to: an all-zero token read as -1, a value appearing nowhere in the stored data. The IndexOutOfBoundsException the decode path now converts is traced, which the generic catch of Entry.decode() used to do. The counters DefaultCompressedSchema records are taken from the highest token written rather than from the number of records, which a gap makes smaller. getAllAttributes()/getAllObjectClasses() read the decode map the way decodeMapGet() does, and registerAttribute() says what a store must not re-enter. --- .../opends/server/api/CompressedSchema.java | 102 +++++++-- .../pluggable/PersistentCompressedSchema.java | 11 +- .../server/core/DefaultCompressedSchema.java | 29 ++- .../server/api/CompressedSchemaTestCase.java | 205 ++++++++++++++---- 4 files changed, 286 insertions(+), 61 deletions(-) diff --git a/opendj-server-legacy/src/main/java/org/opends/server/api/CompressedSchema.java b/opendj-server-legacy/src/main/java/org/opends/server/api/CompressedSchema.java index 92e83359e9..ac23049bdb 100644 --- a/opendj-server-legacy/src/main/java/org/opends/server/api/CompressedSchema.java +++ b/opendj-server-legacy/src/main/java/org/opends/server/api/CompressedSchema.java @@ -18,6 +18,7 @@ package org.opends.server.api; import static org.opends.messages.CoreMessages.*; +import static org.opends.server.util.StaticUtils.bytesToHexNoSpace; import java.util.AbstractMap.SimpleImmutableEntry; import java.util.Collection; @@ -34,6 +35,7 @@ import java.util.concurrent.CopyOnWriteArrayList; import java.util.concurrent.locks.ReentrantLock; +import org.forgerock.i18n.slf4j.LocalizedLogger; import org.forgerock.opendj.ldap.AttributeDescription; import org.forgerock.opendj.ldap.ByteSequenceReader; import org.forgerock.opendj.ldap.ByteString; @@ -60,6 +62,8 @@ mayInvoke = false) public class CompressedSchema { + private static final LocalizedLogger logger = LocalizedLogger.getLoggerForThisClass(); + /** Encloses all the encode and decode mappings for attribute and object classes. */ private static final class Mappings { @@ -206,7 +210,8 @@ public final Attribute decodeAttribute(final ByteSequenceReader reader) throws DirectoryException { // First decode the encoded attribute description id. - final int adId = decodeId(reader); + final byte[] adIdBytes = readIdBytes(reader); + final int adId = decodeId(adIdBytes); // Before returning the attribute, make sure that the attribute type is not stale. final Mappings mappings = reloadMappingsIfSchemaChanged(); @@ -214,7 +219,7 @@ public final Attribute decodeAttribute(final ByteSequenceReader reader) if (ad == null) { throw new DirectoryException(DirectoryServer.getCoreConfigManager().getServerErrorResultCode(), - ERR_COMPRESSEDSCHEMA_UNRECOGNIZED_AD_TOKEN.get(adId)); + ERR_COMPRESSEDSCHEMA_UNRECOGNIZED_AD_TOKEN.get(tokenInMessage(adIdBytes, adId))); } AttributeType attrType = ad.getAttributeType(); @@ -274,6 +279,11 @@ private static T decodeMapGet(final List decodeMap, final int id) // CopyOnWriteArrayList read the array separately, so the comparison would not make the // lookup safe anyway, and this runs for every attribute of every entry read from a backend - // the common path is left with the single read it had. + // + // Traced here because the caller turns this into a DirectoryException carrying the token: + // the generic catch of Entry.decode(), which used to convert this exception, logged the + // stack, and where the token came from is worth keeping for a corrupt store. + logger.traceException(e); return null; } } @@ -292,7 +302,8 @@ public final Map decodeObjectClasses( final ByteSequenceReader reader) throws DirectoryException { // First decode the encoded object class id. - final int ocId = decodeId(reader); + final byte[] ocIdBytes = readIdBytes(reader); + final int ocId = decodeId(ocIdBytes); // Before returning the object classes, make sure that none of them are stale. final Mappings mappings = reloadMappingsIfSchemaChanged(); @@ -300,7 +311,7 @@ public final Map decodeObjectClasses( if (ocMap == null) { throw new DirectoryException(DirectoryServer.getCoreConfigManager().getServerErrorResultCode(), - ERR_COMPRESSEDSCHEMA_UNKNOWN_OC_TOKEN.get(ocId)); + ERR_COMPRESSEDSCHEMA_UNKNOWN_OC_TOKEN.get(tokenInMessage(ocIdBytes, ocId))); } return ocMap; } @@ -369,7 +380,14 @@ private int getAttributeId(final AttributeDescription ad) throws DirectoryExcept * stores it afterwards and the entry cannot be decoded once the server is restarted. *

* Must be called with the exclusive lock held, which is what makes the id allocated here still - * the last element of the decode map when it has to be withdrawn. + * the last element of the decode map when it has to be withdrawn. The lock is reentrant, so + * that holds only while the store stays out of this compressed schema: an implementation of + * {@link #storeAttribute(byte[], String, Iterable)} must re-enter neither the encode, the load + * nor the decode path of it. Encoding or loading appends to the same decode map, and the + * withdrawal would take back whatever was appended last; decoding rebuilds the mappings when + * the schema has changed, and the withdrawal would then take the element out of a map that has + * already been replaced - leaving it in the live one, which is what this method exists to + * prevent. */ private int registerAttribute(final Mappings mappings, final AttributeDescription ad) throws DirectoryException { @@ -461,7 +479,8 @@ private int getObjectClassId(final Map objectClasses) throw * registration before publishing it and withdrawing it if it cannot be persisted, exactly as * {@link #registerAttribute(Mappings, AttributeDescription)} does. *

- * Must be called with the exclusive lock held. + * Must be called with the exclusive lock held, and under the same constraint on what + * {@link #storeObjectClasses(byte[], Collection)} may re-enter. */ private int registerObjectClasses(final Mappings mappings, final Map objectClasses) throws DirectoryException @@ -503,19 +522,27 @@ public Iterator>>> iterator() return new Iterator>>>() { private int id; - private List adDecodeMap = getMappings().adDecodeMap; + private final List adDecodeMap = getMappings().adDecodeMap; @Override public boolean hasNext() { // Skips the gaps: a decode map padded with null for the ids missing from the // compressed schema it was loaded from is still saved, and the ids around a gap are - // preserved by the token each element is written with. - while (id < adDecodeMap.size() && adDecodeMap.get(id) == null) + // preserved by the token each element is written with. Looked up through + // decodeMapGet(), because withdrawing a registration shortens the decode map and a + // CopyOnWriteArrayList reads its array separately for size() and for get(). In tree + // this iteration runs under the exclusive lock - the only caller of save() is a store + // - but the class is extensible and a subclass can reach here from anywhere. + while (id < adDecodeMap.size()) { + if (decodeMapGet(adDecodeMap, id) != null) + { + return true; + } id++; } - return id < adDecodeMap.size(); + return false; } @Override @@ -526,7 +553,12 @@ public Entry>> next() throw new NoSuchElementException(); } final byte[] encodedAttribute = encodeId(id); - final AttributeDescription ad = adDecodeMap.get(id++); + final AttributeDescription ad = decodeMapGet(adDecodeMap, id++); + if (ad == null) + { + // The decode map was shortened between hasNext() and here. + throw new NoSuchElementException(); + } return new SimpleImmutableEntry>>( encodedAttribute, new SimpleImmutableEntry>( @@ -566,12 +598,16 @@ public Iterator>> iterator() @Override public boolean hasNext() { - // Skips the gaps, as in getAllAttributes(). - while (id < ocDecodeMap.size() && ocDecodeMap.get(id) == null) + // Skips the gaps, and looks the elements up the same way, as in getAllAttributes(). + while (id < ocDecodeMap.size()) { + if (decodeMapGet(ocDecodeMap, id) != null) + { + return true; + } id++; } - return id < ocDecodeMap.size(); + return false; } @Override @@ -582,7 +618,12 @@ public Entry> next() throw new NoSuchElementException(); } final byte[] encodedObjectClasses = encodeId(id); - final Map ocMap = ocDecodeMap.get(id++); + final Map ocMap = decodeMapGet(ocDecodeMap, id++); + if (ocMap == null) + { + // The decode map was shortened between hasNext() and here. + throw new NoSuchElementException(); + } return new SimpleImmutableEntry<>(encodedObjectClasses, ocMap.values()); } @@ -824,12 +865,35 @@ private int decodeId(final byte[] idBytes) return id - 1; // Subtract 1 to compensate for old behavior. } - private int decodeId(final ByteSequenceReader reader) + /** + * Reads the encoded schema element ID at the current position. + * + * @param reader + * The byte string reader positioned on an encoded schema element ID. + * @return The encoded schema element ID, as the storage holds it. + */ + private static byte[] readIdBytes(final ByteSequenceReader reader) { - final int length = reader.readBERLength(); - final byte[] idBytes = new byte[length]; + final byte[] idBytes = new byte[reader.readBERLength()]; reader.readBytes(idBytes); - return decodeId(idBytes); + return idBytes; + } + + /** + * Names a token in a message as the storage holds it - the key a definition is written under - + * together with the id it decodes to. The id on its own is one less than what was read, so a + * token that no definition was ever written under is reported as a value appearing nowhere in + * the stored data: an all-zero token reads as the id -1. + * + * @param idBytes + * The encoded schema element ID, as it was read. + * @param id + * The schema element ID it decoded to. + * @return The token as a message should name it. + */ + private static String tokenInMessage(final byte[] idBytes, final int id) + { + return "0x" + bytesToHexNoSpace(idBytes) + " (id " + id + ")"; } /** diff --git a/opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/PersistentCompressedSchema.java b/opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/PersistentCompressedSchema.java index 56312716b2..76fb2561de 100644 --- a/opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/PersistentCompressedSchema.java +++ b/opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/PersistentCompressedSchema.java @@ -113,9 +113,14 @@ protected void storeAttribute(final byte[] encodedAttribute, } catch (final IOException e) { - // Reported rather than absorbed: the caller takes a store that returned for a definition - // that reached the tree, and an entry written with a token that never did cannot be decoded - // once the server is restarted. + // Reported rather than absorbed. Defensive as things stand: the writer encodes into a + // ByteStringBuilder, and none of the write methods of the OutputStream it hands out declares + // IOException, so nothing under this try can raise one - the catch compiles because the + // ASN1Writer interface declares it. What makes a withdrawal reachable in a running server is + // store()'s own catch, on a storage.write that failed. Were this one ever to fire, nothing + // would have reached the tree either, and a store that did not happen must not return + // normally: the caller would publish the token, and an entry written with a token whose + // definition is nowhere cannot be decoded once the server is restarted. logger.traceException(e); throw new DirectoryException(DirectoryServer.getCoreConfigManager().getServerErrorResultCode(), ERR_COMPSCHEMA_CANNOT_STORE_EX.get(e.getMessage()), e); diff --git a/opendj-server-legacy/src/main/java/org/opends/server/core/DefaultCompressedSchema.java b/opendj-server-legacy/src/main/java/org/opends/server/core/DefaultCompressedSchema.java index 04f975fc51..9523449424 100644 --- a/opendj-server-legacy/src/main/java/org/opends/server/core/DefaultCompressedSchema.java +++ b/opendj-server-legacy/src/main/java/org/opends/server/core/DefaultCompressedSchema.java @@ -153,6 +153,31 @@ private void load() } } + /** + * Returns the counter to record once the provided token has been written: the token after the + * highest one written so far, rather than the number of records written. A decode map carrying a + * gap - the compressed schema was loaded from a storage holding no definition for some of its + * ids - emits fewer records than the ids it spans, and a counter taken from the count would name + * a token that is live. Both ends of this file read the counters as "No longer used", but a + * release old enough to seed from them would re-issue those tokens. + * + * @param counter + * The counter as it stands. + * @param encodedToken + * The token just written. + * @return The counter to record. + */ + private static int counterAfter(final int counter, final byte[] encodedToken) + { + int token = 0; + for (final byte b : encodedToken) + { + token <<= 8; + token |= b & 0xFF; + } + return Math.max(counter, token + 1); + } + /** * Writes the compressed schema information to disk. * @@ -189,7 +214,7 @@ private void save() throws DirectoryException writer.writeOctetString(ocName); } writer.writeEndSequence(); - ocCounter++; + ocCounter = counterAfter(ocCounter, mapEntry.getKey()); } writer.writeEndSequence(); @@ -214,7 +239,7 @@ private void save() throws DirectoryException writer.writeOctetString(option); } writer.writeEndSequence(); - adCounter++; + adCounter = counterAfter(adCounter, mapEntry.getKey()); } writer.writeEndSequence(); diff --git a/opendj-server-legacy/src/test/java/org/opends/server/api/CompressedSchemaTestCase.java b/opendj-server-legacy/src/test/java/org/opends/server/api/CompressedSchemaTestCase.java index feb75d378a..21e63a468e 100644 --- a/opendj-server-legacy/src/test/java/org/opends/server/api/CompressedSchemaTestCase.java +++ b/opendj-server-legacy/src/test/java/org/opends/server/api/CompressedSchemaTestCase.java @@ -15,6 +15,7 @@ */ package org.opends.server.api; +import static org.opends.messages.CoreMessages.*; import static org.testng.Assert.*; import java.util.ArrayList; @@ -31,7 +32,7 @@ import java.util.concurrent.Executors; import java.util.concurrent.Future; import java.util.concurrent.TimeUnit; -import java.util.concurrent.TimeoutException; +import java.util.concurrent.atomic.AtomicReference; import org.forgerock.i18n.LocalizableMessage; import org.forgerock.opendj.ldap.ByteString; @@ -88,6 +89,7 @@ protected void storeObjectClasses(final byte[] encodedObjectClasses, final Colle throws DirectoryException { objectClassStoreCount++; + awaitIfGated(); failIfRequested(); storedObjectClasses.put(token(encodedObjectClasses), new ArrayList<>(objectClassNames)); } @@ -190,8 +192,15 @@ public void attributeTokenIsWithdrawnWhenItCannotBeStored() throws Exception assertEquals(compressedSchema.attributeStoreCount, 2, "the failed registration was left behind"); final int encodedToken = tokenOf(builder.toByteString()); + assertEquals(encodedToken, 0, "the withdrawn id was not allocated again"); assertTrue(compressedSchema.storedAttributes.containsKey(encodedToken), "the entry carries token " + encodedToken + ", which was never stored"); + // What the withdrawal exists for: an element left behind by the failed registration would be + // saved here under a token whose store never returned. Asserting the store count and the token + // of the retry is not enough on its own - a registration that leaks its decode map element + // simply allocates the next id, and every other assertion of this test still holds. + assertEquals(compressedSchema.savedAttributeTokens(), Collections.singletonList(0), + "the whole content still holds the element of the failed registration"); final Attribute decoded = compressedSchema.decodeAttribute(builder.toByteString().asReader()); assertEquals(decoded.getAttributeDescription(), attribute.getAttributeDescription()); assertEquals(decoded.iterator().next().toString(), "a value"); @@ -223,8 +232,12 @@ public void objectClassTokenIsWithdrawnWhenItCannotBeStored() throws Exception assertEquals(compressedSchema.objectClassStoreCount, 2, "the failed registration was left behind"); final int encodedToken = tokenOf(builder.toByteString()); + assertEquals(encodedToken, 0, "the withdrawn id was not allocated again"); assertTrue(compressedSchema.storedObjectClasses.containsKey(encodedToken), "the entry carries token " + encodedToken + ", which was never stored"); + // As in attributeTokenIsWithdrawnWhenItCannotBeStored(). + assertEquals(compressedSchema.savedObjectClassTokens(), Collections.singletonList(0), + "the whole content still holds the element of the failed registration"); assertEquals(compressedSchema.decodeObjectClasses(builder.toByteString().asReader()), objectClasses); } @@ -247,18 +260,12 @@ public void aTokenIsPublishedOnlyOnceItIsStored() throws Exception final Future registering = executor.submit(encoding(compressedSchema, attribute, null)); assertTrue(compressedSchema.enteredStore.await(30, TimeUnit.SECONDS), "the store was never reached"); - // Another encode of the same attribute must not be handed the id being stored. - final CountDownLatch started = new CountDownLatch(1); - final Future concurrent = executor.submit(encoding(compressedSchema, attribute, started)); - assertTrue(started.await(30, TimeUnit.SECONDS), "the concurrent encode was never started"); - try - { - fail("the token was handed out before it was stored: " + concurrent.get(500, TimeUnit.MILLISECONDS)); - } - catch (final TimeoutException expected) - { - // Waiting for the registration to be persisted, as it must. - } + // Another encode of the same attribute must not be handed the id being stored: it has to + // park on the exclusive lock until the store returns. + final AtomicReference concurrentThread = new AtomicReference<>(); + final Future concurrent = executor.submit(encoding(compressedSchema, attribute, concurrentThread)); + awaitParkedOnTheLock(concurrent, concurrentThread); + assertFalse(concurrent.isDone(), "the token was handed out before it was stored"); compressedSchema.leaveStore.countDown(); assertEquals(registering.get(30, TimeUnit.SECONDS), Integer.valueOf(0)); @@ -271,17 +278,78 @@ public void aTokenIsPublishedOnlyOnceItIsStored() throws Exception } } + /** The same for an object class set, whose registration orders the two maps the same way. */ + @Test + public void anObjectClassTokenIsPublishedOnlyOnceItIsStored() throws Exception + { + final TestCompressedSchema compressedSchema = new TestCompressedSchema(); + final Map objectClasses = objectClasses("top", "person"); + final ExecutorService executor = Executors.newFixedThreadPool(2); + try + { + compressedSchema.enteredStore = new CountDownLatch(1); + compressedSchema.leaveStore = new CountDownLatch(1); + final Future registering = executor.submit(encoding(compressedSchema, objectClasses, null)); + assertTrue(compressedSchema.enteredStore.await(30, TimeUnit.SECONDS), "the store was never reached"); + + final AtomicReference concurrentThread = new AtomicReference<>(); + final Future concurrent = executor.submit(encoding(compressedSchema, objectClasses, concurrentThread)); + awaitParkedOnTheLock(concurrent, concurrentThread); + assertFalse(concurrent.isDone(), "the token was handed out before it was stored"); + + compressedSchema.leaveStore.countDown(); + assertEquals(registering.get(30, TimeUnit.SECONDS), Integer.valueOf(0)); + assertEquals(concurrent.get(30, TimeUnit.SECONDS), Integer.valueOf(0)); + assertEquals(compressedSchema.objectClassStoreCount, 1, "the same token was stored twice"); + } + finally + { + executor.shutdownNow(); + } + } + + /** + * Waits for the provided encode to park on the exclusive lock, which is what it must do while + * another thread holds that lock inside a store. Waiting for the thread to park is what makes + * this prove the encode reached the lock-free read of the encode map: a latch counted down + * inside the task only proves the task body started, so a build that published an id before + * storing it would be recorded as a pass whenever the thread was slow between the two. + */ + private static void awaitParkedOnTheLock(final Future encode, final AtomicReference runningOn) + throws Exception + { + final long deadline = System.nanoTime() + TimeUnit.SECONDS.toNanos(30); + while (System.nanoTime() < deadline) + { + if (encode.isDone()) + { + fail("the token was handed out before it was stored: " + encode.get()); + } + final Thread thread = runningOn.get(); + if (thread != null) + { + final Thread.State state = thread.getState(); + if (state == Thread.State.WAITING || state == Thread.State.BLOCKED) + { + return; + } + } + Thread.sleep(1); + } + fail("the concurrent encode never parked on the exclusive lock"); + } + private static Callable encoding(final CompressedSchema compressedSchema, final Attribute attribute, - final CountDownLatch started) + final AtomicReference runningOn) { return new Callable() { @Override public Integer call() throws Exception { - if (started != null) + if (runningOn != null) { - started.countDown(); + runningOn.set(Thread.currentThread()); } final ByteStringBuilder builder = new ByteStringBuilder(); compressedSchema.encodeAttribute(builder, attribute); @@ -290,41 +358,104 @@ public Integer call() throws Exception }; } - /** A token with no definition is reported, whether it is out of range or below it. */ + private static Callable encoding(final CompressedSchema compressedSchema, + final Map objectClasses, final AtomicReference runningOn) + { + return new Callable() + { + @Override + public Integer call() throws Exception + { + if (runningOn != null) + { + runningOn.set(Thread.currentThread()); + } + final ByteStringBuilder builder = new ByteStringBuilder(); + compressedSchema.encodeObjectClasses(builder, objectClasses); + return tokenOf(builder.toByteString()); + } + }; + } + + /** + * A token with no definition is reported, whether it is below the range of the decode map, the + * first id past its end, or well beyond it - and against a populated map as well as an empty + * one, since it is the size of that map the lookup is measured against. + */ @Test public void unknownAttributeTokenIsReported() throws Exception { - final TestCompressedSchema compressedSchema = new TestCompressedSchema(); + final TestCompressedSchema empty = new TestCompressedSchema(); for (final int unknownToken : new int[] { -1, 0, 7 }) { - try - { - compressedSchema.decodeAttribute(encodedAttribute(unknownToken).asReader()); - fail("the token " + unknownToken + " has no definition and should have been reported"); - } - catch (final DirectoryException expected) - { - // Reported as the unknown token it is. - } + assertAttributeTokenIsReported(empty, unknownToken); + } + + final TestCompressedSchema populated = new TestCompressedSchema(); + populated.loadAttributeAt(0, "description"); + populated.loadAttributeAt(1, "cn"); + for (final int unknownToken : new int[] { -1, 2, 7 }) + { + assertAttributeTokenIsReported(populated, unknownToken); } } @Test public void unknownObjectClassTokenIsReported() throws Exception { - final TestCompressedSchema compressedSchema = new TestCompressedSchema(); + final TestCompressedSchema empty = new TestCompressedSchema(); for (final int unknownToken : new int[] { -1, 0, 7 }) { - try - { - compressedSchema.decodeObjectClasses(encodedToken(unknownToken, new ByteStringBuilder()).asReader()); - fail("the token " + unknownToken + " has no definition and should have been reported"); - } - catch (final DirectoryException expected) - { - // Reported as the unknown token it is. - } + assertObjectClassTokenIsReported(empty, unknownToken); } + + final TestCompressedSchema populated = new TestCompressedSchema(); + populated.loadObjectClassesAt(0, Arrays.asList("top", "person")); + populated.loadObjectClassesAt(1, Arrays.asList("top", "organizationalUnit")); + for (final int unknownToken : new int[] { -1, 2, 7 }) + { + assertObjectClassTokenIsReported(populated, unknownToken); + } + } + + private static void assertAttributeTokenIsReported(final TestCompressedSchema compressedSchema, + final int unknownToken) throws Exception + { + try + { + compressedSchema.decodeAttribute(encodedAttribute(unknownToken).asReader()); + fail("the token " + unknownToken + " has no definition and should have been reported"); + } + catch (final DirectoryException expected) + { + // Reported as the unknown token it is, and named as such: nothing else in this decode path + // is allowed to answer for a token, and the message the operator gets is what says which. + assertMessageIs(expected, ERR_COMPRESSEDSCHEMA_UNRECOGNIZED_AD_TOKEN.get(unknownToken), unknownToken); + } + } + + private static void assertObjectClassTokenIsReported(final TestCompressedSchema compressedSchema, + final int unknownToken) throws Exception + { + try + { + compressedSchema.decodeObjectClasses(encodedToken(unknownToken, new ByteStringBuilder()).asReader()); + fail("the token " + unknownToken + " has no definition and should have been reported"); + } + catch (final DirectoryException expected) + { + assertMessageIs(expected, ERR_COMPRESSEDSCHEMA_UNKNOWN_OC_TOKEN.get(unknownToken), unknownToken); + } + } + + /** Asserts that the exception carries the expected message, by resource and id rather than text. */ + private static void assertMessageIs(final DirectoryException reported, final LocalizableMessage expected, + final int unknownToken) + { + final LocalizableMessage message = reported.getMessageObject(); + assertEquals(message.resourceName() + "-" + message.ordinal(), + expected.resourceName() + "-" + expected.ordinal(), + "the token " + unknownToken + " was reported as something else: " + message); } /**