[
https://issues.apache.org/jira/browse/DIRAPI-328?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16837046#comment-16837046
]
Emmanuel Lecharny commented on DIRAPI-328:
------------------------------------------
Here are a few places where the use of {{Value.getString()}} is improper.
* {{LeafEvaluator.evalEquality()}}:
{code:java}
if ( attributeType.getSyntax().isHumanReadable() )
{
if ( node.getValue().isHumanReadable() )
{
value = node.getValue();
}
else
{
value = new Value( attributeType, node.getValue().getString() );
}
}
else
{
value = node.getValue();
}
{code}
* {{KeyDerivationInterceptor.add}}:
{code:java}
// Get the entry's password. We will use the first one.
Value userPassword = entry.get( userPasswordAT ).get();
String strUserPassword = userPassword.getString();
{code}
The {{userPassword}} attribute is an {{OctetString}}, we can't get its string
value this way. (side note: we expect the password to be a String, because the
{{javax.security.auth.kerberos.KerberosKey}} constructor expect a {{char()}}
based on the password)
* {{NormalizerInterceptor.addRdnAttributesToEntry}}:
{code:java}
for ( Ava ava : rdn )
{
Value value = ava.getValue();
String upValue = ava.getValue().getString();
{code}
Here, nothing says the {{AVA}} is for a H/R attribute.
* {{OperationalAttributeInterceptor.denormalizeTypes}}:
{code:java}
for ( Iterator<Ava> atavs = rdn.iterator(); atavs.hasNext(); /**/)
{
Ava atav = atavs.next();
String type = schemaManager.lookupAttributeTypeRegistry(
rdn.getNormType() ).getName();
buf.append( type ).append( '=' ).append(
atav.getValue().getString() );
{code}
* {{SchemaInterceptor.assertSyntaxes}} :
{code:java}
for ( Attribute attribute : entry )
{
AttributeType attributeType = attribute.getAttributeType();
SyntaxChecker syntaxChecker =
attributeType.getSyntax().getSyntaxChecker();
if ( syntaxChecker instanceof OctetStringSyntaxChecker )
{
// This is a speedup : no need to check the syntax of any value
// if all the syntaxes are accepted...
continue;
}
// Then loop on all values
for ( Value value : attribute )
{
if ( value.isSchemaAware() )
{
// No need to validate something which is already ok
continue;
}
if ( !syntaxChecker.isValidSyntax( value.getString() ) )
{
String message = I18n.err( I18n.ERR_280, value.getString(),
attribute.getUpId() );
{code}
Here, we are only safe because most of the binary attributes are using the
{{OctetStringSyntaxChecker}}, so the {{getString()}} method is not called.
* {{JdbmPartition.rebuildIndexes}}:
{code:java}
// And the user indexess
// Now work on the user defined userIndices
for ( Attribute attribute : entry )
{
AttributeType attributeType = attribute.getAttributeType();
String attributeOid = attributeType.getOid();
if ( hasUserIndexOn( attributeType ) )
{
Index<Object, String> idx = ( Index<Object, String> )
getUserIndex( attributeType );
// here lookup by attributeId is OK since we got
attributeId from
// the entry via the enumeration - it's in there as is
for sure
for ( Value value : attribute )
{
idx.add( partitionTxn, value.getString(), id );
{code}
Again, what saves us here is that people rarely index binary attributes...
* {{JdbmPartition.buildUserIndex}}:
{code:java}
LOG.info( "building the index for attribute type {}",
atType );
Tuple<String, Entry> tuple = cursor.get();
String id = tuple.getKey();
Entry entry = tuple.getValue();
Attribute entryAttr = entry.get( atType );
if ( entryAttr != null )
{
for ( Value value : entryAttr )
{
index.add( partitionTxn, value.getString(), id );
}
{code}
* {{LdifPartition.getFileName}} :
{code:java}
// Now, get the normalized value
String normValue = rdn.getAva().getValue().getString();
{code}
* {{MavibotPartition.buildUserIndex}}:
{code:java}
Attribute entryAttr = entry.get( atType );
if ( entryAttr != null )
{
for ( Value value : entryAttr )
{
index.add( partitionTxn, value.getString(), id );
}
{code}
* {{CVonfigPartitionReader.readSingleValueField}}:
{code:java}
Value value = fieldAttr.get();
String valueStr = "";
if ( value != null )
{
valueStr = value.getString();
}
{code}
We are very unlikely to store a binary value in the configuration.
* {{AbstractBtreePartition.rename}}:
{code:java}
if ( hasUserIndexOn( newRdnAttrType ) )
{
Index<?, String> userIndex = getUserIndex( newRdnAttrType );
String normalized =
oldAttributeType.getEquality().getNormalizer().normalize(
oldAttribute.get().getString() );
...
if ( mustRemove )
{
String oldNormType = oldAtav.getNormType();
String oldNormValue = oldAtav.getValue().getString();
{code}
* {{ApproximateCursor constructor}} :
{code:java}
if ( store.hasIndexOn( attributeType ) )
{
Index<V, String> index = ( Index<V, String> ) store.getIndex(
attributeType );
userIdxCursor = index.forwardCursor( partitionTxn, ( V )
value.getString() );
{code}
* {{EqualityCursor constructor}} :
{code:java}
if ( store.hasIndexOn( attributeType ) )
{
Index<V, String> userIndex = ( Index<V, String> ) store.getIndex(
attributeType );
String normalizedValue =
attributeType.getEquality().getNormalizer().normalize( value.getString() );
{code}
* {{GreaterEqCursor.before}}/{{LessEqCursor.before}} :
{code:java}
if ( userIdxCursor != null )
{
/*
* First we need to check and make sure this element is within
* bounds as mandated by the assertion node. To do so we compare
* it's value with the value of the node. If it is smaller or
* equal to this lower bound then we simply position the
* userIdxCursor before the first element. Otherwise we let the
* underlying userIdx Cursor position the element.
*/
if ( greaterEqEvaluator.getComparator().compare( element.getKey(),
greaterEqEvaluator.getExpression().getValue().getString() ) <=
0 )
{code}
* {{GreaterEqCursor.after}}/{{LessEqCursor.after}} :
{code:java}
if ( userIdxCursor != null )
{
int comparedValue = greaterEqEvaluator.getComparator().compare(
element.getKey(),
greaterEqEvaluator.getExpression().getValue().getString() );
{code}
* {{GreaterEqCursor.beforeFirst}}/{{LessEqCursor.beforeFirst}} :
{code:java}
if ( userIdxCursor != null )
{
IndexEntry<V, String> advanceTo = new IndexEntry<>();
String normalizedNode =
greaterEqEvaluator.getNormalizer().normalize(
greaterEqEvaluator.getExpression().getValue().getString() );
{code}
* {{GreaterEqCursor.previous}}/{{LessEqCursor.previous}} :
{code:java}
while ( userIdxCursor.previous() )
{
checkNotClosed();
IndexEntry<?, String> candidate = userIdxCursor.get();
if ( greaterEqEvaluator.getComparator().compare(
candidate.getKey(),
greaterEqEvaluator.getExpression().getValue().getString() )
>= 0 )
{code}
* {{GreaterEqEvaluator.evaluate}}/{{LessEqEvaluator.evaluate}}:
{code:java}
for ( Value value : attribute )
{
if ( ldapComparator.compare( value.getNormalized(),
node.getValue().getNormalized() ) >= 0 )
{
if ( indexEntry != null )
{
indexEntry.setKey( value.getString() );
{code}
* {{CursorBuilder.computeGreaterEq}}/{{CursorBuilder.computeLessEq}} :
{code:java}
if ( db.hasIndexOn( attributeType ) )
{
// Get the cursor using the index
Index<T, String> userIndex = ( Index<T, String> ) db.getIndex(
attributeType );
Cursor<IndexEntry<T, String>> userIdxCursor =
userIndex.forwardCursor( partitionTxn );
// Position the index on the element we should start from
IndexEntry<T, String> indexEntry = new IndexEntry<>();
indexEntry.setKey( ( T ) value.getString() );
{code}
* {{DefaultOptimizer.getEqualityScan}} :
{code:java}
if ( db.hasIndexOn( node.getAttributeType() ) )
{
Index<V, String> idx = ( Index<V, String> ) db.getIndex(
node.getAttributeType() );
String normalizedKey;
if ( node.getValue().isSchemaAware() )
{
normalizedKey = node.getValue().getNormalized();
}
else
{
normalizedKey =
node.getAttributeType().getEquality().getNormalizer().normalize(
node.getValue().getString() );
}
{code}
* {{DefaultOptimizer.getEqualityScan}} :
{code:java}
if ( db.hasIndexOn( node.getAttributeType() ) )
{
Index<V, String> idx = ( Index<V, String> ) db.getIndex(
node.getAttributeType() );
if ( isGreaterThan )
{
return idx.greaterThanCount( partitionTxn, ( V )
node.getValue().getString() );
}
else
{
return idx.lessThanCount( partitionTxn, ( V )
node.getValue().getString() );
}
}
{code}
* {{AddRequestDsml.toDsml}}
{code:java}
for ( Value value : attribute )
{
if ( ParserUtils.needsBase64Encoding( value.getString() ) )
{
Namespace xsdNamespace = new Namespace( "xsd",
ParserUtils.XML_SCHEMA_URI );
Namespace xsiNamespace = new Namespace( "xsi",
ParserUtils.XML_SCHEMA_INSTANCE_URI );
attributeElement.getDocument().getRootElement().add(
xsdNamespace );
attributeElement.getDocument().getRootElement().add(
xsiNamespace );
Element valueElement = attributeElement.addElement(
"value" ).addText(
ParserUtils.base64Encode( value.getString() ) );
valueElement
.addAttribute( new QName( "type", xsiNamespace ),
"xsd:" + ParserUtils.BASE64BINARY );
}
{code}
* {{AttributeValueAssertion.dumpObject}}:
{code:java}
else if ( object instanceof Value )
{
return ( ( Value ) object ).getString();
}
{code}
* {{CompareRequestDsml.toDsml}} :
{code:java}
if ( request.getAssertionValue() != null )
{
assertionElement.addElement( "value" ).setText(
request.getAssertionValue().getString() );
}
{code}
* {{ModifyRequestDsml.toDsml}} :
{code:java}
for ( Value value : modification.getAttribute() )
{
if ( value.getString() != null )
{
if ( ParserUtils.needsBase64Encoding( value.getString()
) )
{
Namespace xsdNamespace = new Namespace( "xsd",
ParserUtils.XML_SCHEMA_URI );
Namespace xsiNamespace = new Namespace( "xsi",
ParserUtils.XML_SCHEMA_INSTANCE_URI );
element.getDocument().getRootElement().add(
xsdNamespace );
element.getDocument().getRootElement().add(
xsiNamespace );
Element valueElement = modElement.addElement(
"value" ).addText(
ParserUtils.base64Encode( value.getString() ) );
valueElement.addAttribute( new QName( "type",
xsiNamespace ), "xsd:"
+ ParserUtils.BASE64BINARY );
}
{code}
* {{SearchRequestDsml.toDsml}} :
{code:java}
if ( value != null )
{
if ( ParserUtils.needsBase64Encoding( value ) )
{
Namespace xsdNamespace = new Namespace( "xsd",
ParserUtils.XML_SCHEMA_URI );
Namespace xsiNamespace = new Namespace( "xsi",
ParserUtils.XML_SCHEMA_INSTANCE_URI );
element.getDocument().getRootElement().add( xsdNamespace );
element.getDocument().getRootElement().add( xsiNamespace );
Element valueElement = newElement.addElement( VALUE
).addText(
ParserUtils.base64Encode( value ) );
valueElement
.addAttribute( new QName( "type", xsiNamespace ),
"xsd:" + ParserUtils.BASE64BINARY );
}
else
{
newElement.addElement( VALUE ).setText( value.getString() );
}
{code}
and
{code:java}
if ( value != null )
{
if ( ParserUtils.needsBase64Encoding( value ) )
{
Namespace xsdNamespace = new Namespace( "xsd",
ParserUtils.XML_SCHEMA_URI );
Namespace xsiNamespace = new Namespace( "xsi",
ParserUtils.XML_SCHEMA_INSTANCE_URI );
element.getDocument().getRootElement().add( xsdNamespace );
element.getDocument().getRootElement().add( xsiNamespace );
Element valueElement = newElement.addElement( VALUE
).addText(
ParserUtils.base64Encode( value.getString() ) );
valueElement.addAttribute( new QName( "type", xsiNamespace
), "xsd:" + ParserUtils.BASE64BINARY );
}
else
{
newElement.addElement( VALUE ).setText( value.getString() );
}
{code}
* {{SearchResultEntryDsml.toDsml}} :
{code:java}
for ( Value value : attribute )
{
if ( ParserUtils.needsBase64Encoding( value.getString() ) )
{
Namespace xsdNamespace = new Namespace( ParserUtils.XSD,
ParserUtils.XML_SCHEMA_URI );
Namespace xsiNamespace = new Namespace( ParserUtils.XSI,
ParserUtils.XML_SCHEMA_INSTANCE_URI );
Document doc = attributeElement.getDocument();
if ( doc != null )
{
Element docRoot = doc.getRootElement();
docRoot.add( xsdNamespace );
docRoot.add( xsiNamespace );
}
Element valueElement = attributeElement.addElement( "value"
).addText(
ParserUtils.base64Encode( value.getString() ) );
valueElement.addAttribute( new QName( "type", xsiNamespace
), ParserUtils.XSD + ":"
+ ParserUtils.BASE64BINARY );
}
else
{
attributeElement.addElement( "value" ).addText(
value.getString() );
}
{code}
Note: the {{DSML}} code needs to be reviewed, I suspect there are many
potential breakage when transforming values.
* {{Rdn.getValue}} :
{code:java}
switch ( nbAvas )
{
case 0:
return "";
case 1:
if ( ava.getNormType().equals( normalizedType ) )
{
if ( ava.getValue() != null )
{
return ava.getValue().getString();
}
else
{
return null;
}
default:
List<Ava> avaList = avaTypes.get( normalizedType );
if ( avaList != null )
{
for ( Ava elem : avaList )
{
if ( elem.getNormType().equals( normalizedType ) )
{
if ( elem.getValue() != null )
{
return elem.getValue().getString();
}
else
{
return null;
}
}
}
...
public String getValue()
{
switch ( nbAvas )
{
case 0:
return null;
case 1:
return ava.getValue().getString();
default:
return avas.get( 0 ).getValue().getString();
}
}
{code}
> Fix inconsistency in Attribute and Value
> ----------------------------------------
>
> Key: DIRAPI-328
> URL: https://issues.apache.org/jira/browse/DIRAPI-328
> Project: Directory Client API
> Issue Type: Improvement
> Reporter: Stefan Seelmann
> Assignee: Emmanuel Lecharny
> Priority: Major
> Fix For: 2.0.0.AM3
>
>
> Inconsistent method names:
> * Attribute: getString() and getBytes()
> * Value: getValue() and getBytes()
> * Proposal: Rename Value.getValue() to Value.getString()
> Inconsistent and wrong documented error handling:
> * Attribute: getString() and getBytes() check for isHumanReadable and throw
> LdapInvalidAttributeValueException
> * Value: getValue()/getString() tries to UTF-8 encode bytes if not human
> readable, which doesn't work always, and javadoc says "returns null"
> * Value: getBytes() doesn't check if is human readable
> * Proposal: Make Value behave like Attribute, i.e. throw
> LdapInvalidAttributeValueException
> Opinions?
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
