From 83a01a738516c09e9b52ea9b9a4e9bea93ae53bf Mon Sep 17 00:00:00 2001
From: Nicolas Capponi <nicolas.capponi@forgerock.com>
Date: Wed, 17 Jun 2015 08:06:08 +0000
Subject: [PATCH] OPENDJ-2134 CR-7260 AttributeBuilder always normalizes attribute values, even when there is only one value
---
opendj-server-legacy/src/test/java/org/opends/server/backends/jeb/TestBackendImpl.java | 4
opendj-server-legacy/src/main/java/org/opends/server/types/AttributeBuilder.java | 373 +++++++++++++++++++++++++++++-----------------
opendj-server-legacy/src/test/java/org/opends/server/types/AttributeBuilderTest.java | 73 +++-----
3 files changed, 264 insertions(+), 186 deletions(-)
diff --git a/opendj-server-legacy/src/main/java/org/opends/server/types/AttributeBuilder.java b/opendj-server-legacy/src/main/java/org/opends/server/types/AttributeBuilder.java
index c8d98d1..7efe7d9 100644
--- a/opendj-server-legacy/src/main/java/org/opends/server/types/AttributeBuilder.java
+++ b/opendj-server-legacy/src/main/java/org/opends/server/types/AttributeBuilder.java
@@ -32,8 +32,6 @@
import java.util.Iterator;
import java.util.LinkedHashSet;
import java.util.List;
-import java.util.Map;
-import java.util.Map.Entry;
import java.util.NoSuchElementException;
import java.util.Set;
import java.util.SortedSet;
@@ -106,10 +104,11 @@
mayInstantiate = true,
mayExtend = false,
mayInvoke = true)
-public final class AttributeBuilder
- implements Iterable<ByteString>
+public final class AttributeBuilder implements Iterable<ByteString>
{
+ private static final LocalizedLogger logger = LocalizedLogger.getLoggerForThisClass();
+
/**
* A real attribute - options handled by sub-classes.
*/
@@ -125,13 +124,12 @@
private final String name;
/**
- * The unmodifiable map of values for this attribute. The key is the
- * attribute value normalized according to the equality matching rule and
- * the value is the non normalized value.
+ * The unmodifiable set of attribute values, which are lazily normalized.
+ * <p>
+ * When required, the attribute values are normalized according to the equality
+ * matching rule.
*/
- private final Map<ByteString, ByteString> values;
-
-
+ private final Set<AttributeValue> values;
/**
* Creates a new real attribute.
@@ -143,10 +141,7 @@
* @param values
* The attribute values.
*/
- private RealAttribute(
- AttributeType attributeType,
- String name,
- Map<ByteString, ByteString> values)
+ private RealAttribute(AttributeType attributeType, String name, Set<AttributeValue> values)
{
this.attributeType = attributeType;
this.name = name;
@@ -154,7 +149,6 @@
}
- /** {@inheritDoc} */
@Override
public final ConditionResult approximatelyEqualTo(ByteString assertionValue)
{
@@ -176,11 +170,11 @@
}
ConditionResult result = ConditionResult.FALSE;
- for (ByteString v : values.values())
+ for (AttributeValue v : values)
{
try
{
- result = assertion.matches(matchingRule.normalizeAttributeValue(v));
+ result = assertion.matches(matchingRule.normalizeAttributeValue(v.getValue()));
}
catch (Exception e)
{
@@ -197,14 +191,12 @@
- /** {@inheritDoc} */
@Override
public final boolean contains(ByteString value)
{
- return values.containsKey(normalize(attributeType, value));
+ return values.contains(createAttributeValue(attributeType, value));
}
- /** {@inheritDoc} */
@Override
public ConditionResult matchesEqualityAssertion(ByteString assertionValue)
{
@@ -212,9 +204,9 @@
{
MatchingRule eqRule = getAttributeType().getEqualityMatchingRule();
final Assertion assertion = eqRule.getAssertion(assertionValue);
- for (ByteString normalizedValue : values.keySet())
+ for (AttributeValue value : values)
{
- if (assertion.matches(normalizedValue).toBoolean())
+ if (assertion.matches(value.getNormalizedValue()).toBoolean())
{
return ConditionResult.TRUE;
}
@@ -227,7 +219,6 @@
}
}
- /** {@inheritDoc} */
@Override
public final AttributeType getAttributeType()
{
@@ -236,7 +227,6 @@
- /** {@inheritDoc} */
@Override
public final String getName()
{
@@ -245,7 +235,6 @@
- /** {@inheritDoc} */
@Override
public final ConditionResult greaterThanOrEqualTo(ByteString assertionValue)
{
@@ -267,11 +256,11 @@
}
ConditionResult result = ConditionResult.FALSE;
- for (ByteString v : values.values())
+ for (AttributeValue v : values)
{
try
{
- if (assertion.matches(matchingRule.normalizeAttributeValue(v)).toBoolean())
+ if (assertion.matches(matchingRule.normalizeAttributeValue(v.getValue())).toBoolean())
{
return ConditionResult.TRUE;
}
@@ -290,7 +279,6 @@
- /** {@inheritDoc} */
@Override
public final boolean isVirtual()
{
@@ -298,17 +286,12 @@
}
-
- /** {@inheritDoc} */
@Override
public final Iterator<ByteString> iterator()
{
- return values.values().iterator();
+ return getUnmodifiableIterator(values);
}
-
-
- /** {@inheritDoc} */
@Override
public final ConditionResult lessThanOrEqualTo(ByteString assertionValue)
{
@@ -330,11 +313,11 @@
}
ConditionResult result = ConditionResult.FALSE;
- for (ByteString v : values.values())
+ for (AttributeValue v : values)
{
try
{
- if (assertion.matches(matchingRule.normalizeAttributeValue(v)).toBoolean())
+ if (assertion.matches(matchingRule.normalizeAttributeValue(v.getValue())).toBoolean())
{
return ConditionResult.TRUE;
}
@@ -354,11 +337,8 @@
- /** {@inheritDoc} */
@Override
- public final ConditionResult matchesSubstring(
- ByteString subInitial,
- List<ByteString> subAny, ByteString subFinal)
+ public final ConditionResult matchesSubstring(ByteString subInitial, List<ByteString> subAny, ByteString subFinal)
{
MatchingRule matchingRule = attributeType.getSubstringMatchingRule();
if (matchingRule == null)
@@ -379,11 +359,11 @@
}
ConditionResult result = ConditionResult.FALSE;
- for (ByteString value : values.values())
+ for (AttributeValue value : values)
{
try
{
- if (assertion.matches(matchingRule.normalizeAttributeValue(value)).toBoolean())
+ if (assertion.matches(matchingRule.normalizeAttributeValue(value.getValue())).toBoolean())
{
return ConditionResult.TRUE;
}
@@ -403,33 +383,30 @@
- /** {@inheritDoc} */
@Override
public final int size()
{
return values.size();
}
- /** {@inheritDoc} */
@Override
public int hashCode()
{
int hashCode = getAttributeType().hashCode();
- for (ByteString value : values.keySet())
+ for (AttributeValue value : values)
{
hashCode += value.hashCode();
}
return hashCode;
}
- /** {@inheritDoc} */
@Override
public final void toString(StringBuilder buffer)
{
buffer.append("Attribute(");
buffer.append(getNameWithOptions());
buffer.append(", {");
- buffer.append(Utils.joinAsString(", ", values.keySet()));
+ buffer.append(Utils.joinAsString(", ", values));
buffer.append("})");
}
@@ -467,10 +444,7 @@
* The normalized attribute options.
*/
private RealAttributeManyOptions(
- AttributeType attributeType,
- String name,
- Map<ByteString, ByteString> values,
- Set<String> options,
+ AttributeType attributeType, String name, Set<AttributeValue> values, Set<String> options,
SortedSet<String> normalizedOptions)
{
super(attributeType, name, values);
@@ -513,8 +487,7 @@
/**
* A real attribute with no options.
*/
- private static final class RealAttributeNoOptions
- extends RealAttribute
+ private static final class RealAttributeNoOptions extends RealAttribute
{
/**
@@ -527,10 +500,7 @@
* @param values
* The attribute values.
*/
- private RealAttributeNoOptions(
- AttributeType attributeType,
- String name,
- Map<ByteString, ByteString> values)
+ private RealAttributeNoOptions(AttributeType attributeType, String name, Set<AttributeValue> values)
{
super(attributeType, name, values);
}
@@ -623,7 +593,7 @@
private RealAttributeSingleOption(
AttributeType attributeType,
String name,
- Map<ByteString, ByteString> values,
+ Set<AttributeValue> values,
String option)
{
super(attributeType, name, values);
@@ -674,8 +644,7 @@
* @param <T>
* The type of elements to be contained in this small set.
*/
- private static final class SmallSet<T>
- extends AbstractSet<T>
+ private static final class SmallSet<T> extends AbstractSet<T>
{
/** The set of elements if there are more than one. */
@@ -684,8 +653,6 @@
/** The first element. */
private T firstElement;
-
-
/**
* Creates a new small set which is initially empty.
*/
@@ -694,8 +661,22 @@
// No implementation required.
}
+ /**
+ * Creates a new small set with an initial capacity.
+ *
+ * @param initialCapacity
+ * The capacity of the set
+ */
+ public SmallSet(int initialCapacity)
+ {
+ Reject.ifFalse(initialCapacity >= 0);
- /** {@inheritDoc} */
+ if (initialCapacity > 1)
+ {
+ elements = new LinkedHashSet<T>(initialCapacity);
+ }
+ }
+
@Override
public boolean add(T e)
{
@@ -725,9 +706,6 @@
return elements.add(e);
}
-
-
- /** {@inheritDoc} */
@Override
public boolean addAll(Collection<? extends T> c)
{
@@ -759,9 +737,6 @@
}
}
-
-
- /** {@inheritDoc} */
@Override
public void clear()
{
@@ -769,9 +744,6 @@
elements = null;
}
-
-
- /** {@inheritDoc} */
@Override
public Iterator<T> iterator()
{
@@ -783,19 +755,14 @@
{
return new Iterator<T>()
{
-
private boolean hasNext = true;
-
-
@Override
public boolean hasNext()
{
return hasNext;
}
-
-
@Override
public T next()
{
@@ -808,17 +775,10 @@
return firstElement;
}
-
-
@Override
public void remove()
{
- if (hasNext || firstElement == null)
- {
- throw new IllegalStateException();
- }
-
- firstElement = null;
+ throw new UnsupportedOperationException();
}
};
@@ -829,9 +789,6 @@
}
}
-
-
- /** {@inheritDoc} */
@Override
public boolean remove(Object o)
{
@@ -853,7 +810,6 @@
return false;
}
- /** {@inheritDoc} */
@Override
public boolean contains(Object o)
{
@@ -865,7 +821,6 @@
return (firstElement != null && firstElement.equals(o));
}
-
/**
* Sets the initial capacity of this small set. If this small set
* already contains elements or if its capacity has already been
@@ -893,9 +848,6 @@
}
}
-
-
- /** {@inheritDoc} */
@Override
public int size()
{
@@ -915,7 +867,130 @@
}
+ /**
+ * An attribute value which is lazily normalized.
+ * <p>
+ * Stores the value in user-provided form and a reference to the associated
+ * attribute type. The normalized form of the value will be initialized upon
+ * first request. The normalized form of the value should only be used in
+ * cases where equality matching between two values can be performed with
+ * byte-for-byte comparisons of the normalized values.
+ */
+ private static final class AttributeValue
+ {
+ private final AttributeType attributeType;
+
+ /** User-provided value. */
+ private final ByteString value;
+
+ /** Normalized value, which is {@code null} until computation is required. */
+ private ByteString normalizedValue;
+
+ /**
+ * Construct a new attribute value.
+ *
+ * @param attributeType
+ * The attribute type.
+ * @param value
+ * The value of the attribute.
+ */
+ private AttributeValue(AttributeType attributeType, ByteString value)
+ {
+ this.attributeType = attributeType;
+ this.value = value;
+ }
+
+ /**
+ * Retrieves the normalized form of this attribute value.
+ *
+ * @return The normalized form of this attribute value.
+ */
+ public ByteString getNormalizedValue()
+ {
+ if (normalizedValue == null)
+ {
+ normalizedValue = normalize(attributeType, value);
+ }
+ return normalizedValue;
+ }
+
+ boolean isNormalized()
+ {
+ return normalizedValue != null;
+ }
+
+ /**
+ * Retrieves the user-defined form of this attribute value.
+ *
+ * @return The user-defined form of this attribute value.
+ */
+ public ByteString getValue()
+ {
+ return value;
+ }
+
+ /**
+ * Indicates whether the provided object is an attribute value that is equal
+ * to this attribute value. It will be considered equal if the normalized
+ * representations of both attribute values are equal.
+ *
+ * @param o
+ * The object for which to make the determination.
+ * @return <CODE>true</CODE> if the provided object is an attribute value
+ * that is equal to this attribute value, or <CODE>false</CODE> if
+ * not.
+ */
+ @Override
+ public boolean equals(Object o)
+ {
+ if (this == o)
+ {
+ return true;
+ }
+ else if (o instanceof AttributeValue)
+ {
+ AttributeValue attrValue = (AttributeValue) o;
+ try
+ {
+ return getNormalizedValue().equals(attrValue.getNormalizedValue());
+ }
+ catch (Exception e)
+ {
+ logger.traceException(e);
+ return value.equals(attrValue.getValue());
+ }
+ }
+ return false;
+ }
+
+ /**
+ * Retrieves the hash code for this attribute value. It will be calculated
+ * using the normalized representation of the value.
+ *
+ * @return The hash code for this attribute value.
+ */
+ @Override
+ public int hashCode()
+ {
+ try
+ {
+ return getNormalizedValue().hashCode();
+ }
+ catch (Exception e)
+ {
+ logger.traceException(e);
+ return value.hashCode();
+ }
+ }
+
+ @Override
+ public String toString()
+ {
+ return value != null ? value.toString() : "null";
+ }
+
+ }
/**
* Creates an attribute that has no options.
@@ -973,11 +1048,8 @@
/** The set of options. */
private final SmallSet<String> options = new SmallSet<String>();
- /** The map of normalized values => values for this attribute. */
- private Map<ByteString, ByteString> values =
- new SmallMap<ByteString, ByteString>();
-
-
+ /** The set of attribute values, which are lazily normalized. */
+ private Set<AttributeValue> values = new SmallSet<>();
/**
* Creates a new attribute builder with an undefined attribute type
@@ -992,8 +1064,6 @@
// No implementation required.
}
-
-
/**
* Creates a new attribute builder from an existing attribute.
* <p>
@@ -1119,16 +1189,26 @@
*/
public boolean add(ByteString attributeValue)
{
- return values.put(normalize(attributeValue), attributeValue) == null;
+ AttributeValue value = createAttributeValue(attributeType, attributeValue);
+ boolean isNewValue = values.add(value);
+ if (!isNewValue)
+ {
+ // AttributeValue is already present, but the user-provided value may be different
+ // There is no direct way to check this, so remove and add to ensure
+ // the last user-provided value is recorded
+ values.remove(value);
+ values.add(value);
+ }
+ return isNewValue;
}
- private ByteString normalize(ByteString attributeValue)
+ /** Creates an attribute value with delayed normalization. */
+ private static AttributeValue createAttributeValue(AttributeType attributeType, ByteString attributeValue)
{
- return normalize(attributeType, attributeValue);
+ return new AttributeValue(attributeType, attributeValue);
}
- private static ByteString normalize(AttributeType attributeType,
- ByteString attributeValue)
+ private static ByteString normalize(AttributeType attributeType, ByteString attributeValue)
{
try
{
@@ -1210,7 +1290,7 @@
*/
public boolean contains(ByteString value)
{
- return values.containsKey(normalize(value));
+ return values.contains(createAttributeValue(attributeType, value));
}
@@ -1280,7 +1360,7 @@
@Override
public Iterator<ByteString> iterator()
{
- return values.values().iterator();
+ return getUnmodifiableIterator(values);
}
@@ -1297,11 +1377,9 @@
*/
public boolean remove(ByteString value)
{
- return values.remove(normalize(value)) != null;
+ return values.remove(createAttributeValue(attributeType, value));
}
-
-
/**
* Removes the specified attribute value from this attribute builder
* if it is present.
@@ -1623,7 +1701,49 @@
return values.size();
}
+ /** Returns an iterator on values corresponding to the provided attribute values set. */
+ private static Iterator<ByteString> getUnmodifiableIterator(Set<AttributeValue> set)
+ {
+ final Iterator<AttributeValue> iterator = set.iterator();
+ return new Iterator<ByteString>()
+ {
+ @Override
+ public boolean hasNext()
+ {
+ return iterator.hasNext();
+ }
+ @Override
+ public ByteString next()
+ {
+ return iterator.next().getValue();
+ }
+
+ @Override
+ public void remove()
+ {
+ throw new UnsupportedOperationException();
+ }
+ };
+ }
+
+ /**
+ * Indicates if the values for this attribute have been normalized.
+ * <p>
+ * This method is intended for tests.
+ */
+ boolean isNormalized()
+ {
+ Iterator<AttributeValue> iterator = values.iterator();
+ while (iterator.hasNext())
+ {
+ if (iterator.next().isNormalized())
+ {
+ return true;
+ }
+ }
+ return false;
+ }
/**
* Returns an attribute representing the content of this attribute
@@ -1647,43 +1767,21 @@
"Undefined attribute type or name");
}
- // First determine the minimum representation required for the set
- // of values.
- final int size = values.size();
- Map<ByteString, ByteString> newValues;
- if (size == 0)
- {
- newValues = Collections.emptyMap();
- }
- else if (size == 1)
- {
- Entry<ByteString, ByteString> entry = values.entrySet().iterator().next();
- newValues = Collections.singletonMap(entry.getKey(), entry.getValue());
- values.clear();
- }
- else
- {
- newValues = Collections.unmodifiableMap(values);
- values = new SmallMap<ByteString, ByteString>();
- }
-
// Now create the appropriate attribute based on the options.
Attribute attribute;
switch (options.size())
{
case 0:
- attribute = new RealAttributeNoOptions(attributeType, name, newValues);
+ attribute = new RealAttributeNoOptions(attributeType, name, values);
break;
case 1:
attribute =
- new RealAttributeSingleOption(attributeType, name, newValues,
- options.firstElement);
+ new RealAttributeSingleOption(attributeType, name, values, options.firstElement);
break;
default:
attribute =
- new RealAttributeManyOptions(attributeType, name, newValues,
- Collections.unmodifiableSet(options.elements), Collections
- .unmodifiableSortedSet(normalizedOptions));
+ new RealAttributeManyOptions(attributeType, name, values,
+ Collections.unmodifiableSet(options.elements), Collections.unmodifiableSortedSet(normalizedOptions));
break;
}
@@ -1692,6 +1790,7 @@
name = null;
normalizedOptions = null;
options.clear();
+ values = new SmallSet<AttributeValue>();
return attribute;
}
@@ -1714,7 +1813,7 @@
}
builder.append(", {");
- builder.append(Utils.joinAsString(", ", values.keySet()));
+ builder.append(Utils.joinAsString(", ", values));
builder.append("})");
return builder.toString();
diff --git a/opendj-server-legacy/src/test/java/org/opends/server/backends/jeb/TestBackendImpl.java b/opendj-server-legacy/src/test/java/org/opends/server/backends/jeb/TestBackendImpl.java
index 1b59f52..b81ae92 100644
--- a/opendj-server-legacy/src/test/java/org/opends/server/backends/jeb/TestBackendImpl.java
+++ b/opendj-server-legacy/src/test/java/org/opends/server/backends/jeb/TestBackendImpl.java
@@ -1240,7 +1240,7 @@
//No indexes should be used.
String debugString =
result.get(0).getAttribute("debugsearchindex").get(0).toString();
- assertThat(debugString).contains("not-indexed");
+ assertThat(debugString).containsIgnoringCase("not-indexed");
resultCode = TestCaseUtils.applyModifications(true,
"dn: ds-cfg-attribute=givenName,cn=Index, ds-cfg-backend-id=indexRoot,cn=Backends,cn=config",
@@ -1374,7 +1374,7 @@
//No indexes should be used.
String debugString =
result.get(0).getAttribute("debugsearchindex").get(0).toString();
- assertThat(debugString).contains("not-indexed");
+ assertThat(debugString).containsIgnoringCase("not-indexed");
}
@Test(dependsOnMethods = "testSearchNotIndexed")
diff --git a/opendj-server-legacy/src/test/java/org/opends/server/types/AttributeBuilderTest.java b/opendj-server-legacy/src/test/java/org/opends/server/types/AttributeBuilderTest.java
index 8f80b7e..447ea3c 100644
--- a/opendj-server-legacy/src/test/java/org/opends/server/types/AttributeBuilderTest.java
+++ b/opendj-server-legacy/src/test/java/org/opends/server/types/AttributeBuilderTest.java
@@ -22,12 +22,13 @@
*
*
* Copyright 2006-2008 Sun Microsystems, Inc.
- * Portions Copyright 2011-2014 ForgeRock AS.
+ * Portions Copyright 2011-2015 ForgeRock AS.
*/
package org.opends.server.types;
import org.forgerock.opendj.ldap.ByteString;
+import static org.assertj.core.api.Assertions.*;
import static org.testng.Assert.*;
import java.util.*;
@@ -1635,7 +1636,7 @@
try
{
i.remove();
- Assert.fail("value iterator() supports remove");
+ Assert.fail("iterator() should not support remove");
}
catch (UnsupportedOperationException e)
{
@@ -1802,56 +1803,34 @@
Assert.assertEquals(a.size(), values.length);
}
-
-
/**
- * Tests that the generated attribute is optimized correctly for
- * storage of attribute values. This test is very implementation
- * dependent, but because Attributes are so performance sensitive it
- * is worth doing.
- *
- * @param testCase
- * Test case index (useful for debugging).
- * @param a
- * The attribute.
- * @param type
- * The expected attribute type.
- * @param name
- * The expected user provided attribute name.
- * @param options
- * The expected attribute options.
- * @param values
- * The expected attribute values.
+ * Test that normalization of attribute value is performed lazily when there is one
+ * attribute value.
*/
- @Test(dataProvider = "createAttributes", dependsOnMethods = "testAttributeIterator")
- public void testAttributeValueOptimization(int testCase, Attribute a,
- AttributeType type, String name, String[] options, String[] values)
- throws Exception
+ @Test
+ public void testAttributeValueNormalization() throws Exception
{
- // Determine the value set implementation class.
- Class<?> actualClass = a.iterator().getClass();
- Class<?> expectedClass = null;
- switch (values.length)
- {
- case 0:
- // Attribute must be optimized for zero values.
- expectedClass = Collections.emptySet().iterator().getClass();
- break;
- case 1:
- // Attribute must be optimized for single value.
- expectedClass = Collections.singleton(0).iterator().getClass();
- break;
- default:
- // Attribute must be optimized for many values.
- expectedClass = Collections.unmodifiableCollection(new HashSet<Object>())
- .iterator().getClass();
- break;
- }
- Assert.assertEquals(actualClass, expectedClass);
+ AttributeBuilder a1 = new AttributeBuilder("cn");
+ assertThat(a1.isNormalized()).isFalse();
+
+ AttributeBuilder a2 = new AttributeBuilder("cn");
+ a2.add("one");
+ assertThat(a2.isNormalized()).isFalse();
+
+ AttributeBuilder a3 = new AttributeBuilder("cn");
+ a3.add("one");
+ a3.add("two");
+ assertThat(a3.isNormalized()).isTrue();
+
+ // no normalization on contains for 0-element and 1-element set
+ a1.contains(ByteString.valueOf("one"));
+ assertThat(a1.isNormalized()).isFalse();
+
+ // normalization on contains for 1-element set
+ a2.contains(ByteString.valueOf("one"));
+ assertThat(a2.isNormalized()).isTrue();
}
-
-
/** Creates a new attribute. */
private Attribute createAttribute(AttributeType type, String name,
String[] options, String[] values)
--
Gitblit v1.10.0